Skip to content

bug fixes of both issues of #838 #839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Mar 12, 2024
Merged

bug fixes of both issues of #838 #839

merged 11 commits into from
Mar 12, 2024

Conversation

@kagahd
Copy link
Contributor

@kagahd kagahd commented Feb 26, 2024

  • Bug 1 is fixed by converting a single String in dc.subject to an array of one String so that the loop iterates not over the characters of the String but over the String(s) of the array.
  • Bug 2 is fixed by joining the full path to the filename without its file extension.

@kagahd
fix bug 1 of bpatrik#838 : If the tagged video contains only one keyw…
75e4ed7 
…ord, the keyword is shown in PiGallery2 character by character, each prefixed by a hash sign, all separated by comma and space
@kagahd
fix bug 2 of bpatrik#838 : If the xmp file does not include the file …
3ee030d 
…extension of the tagged file, no keywords and ratings are shown in PiGallery's frontend.
@kagahd
add unit tests
8702586
@kagahd
add two more unit tests
917b738
@kagahd kagahd mentioned this pull request Feb 29, 2024
Closed
@bpatrik
Copy link
Owner

bpatrik commented Mar 2, 2024

Thank you for your addition!

There as a bigger MetadataLoader.ts change with one the latest PR that conflicts with the PR. Can you please resolve the conflict?

kagahd added 2 commits March 4, 2024 11:55
@kagahd
Merge branch 'master' into issue838
0f0a66c 
# Conflicts:
#	src/backend/model/fileaccess/MetadataLoader.ts
#	test/backend/unit/model/threading/MetaDataLoader.spec.ts
@kagahd
resolve merge conflicts due to PR bpatrik#829
fb65910
@kagahd
Copy link
Contributor Author

kagahd commented Mar 4, 2024

There as a bigger MetadataLoader.ts change with one the latest PR that conflicts with the PR. Can you please resolve the conflict?

Hi @bpatrik, all merge conflicts due to PR #829 have been resolved.

@bpatrik
Copy link
Owner

bpatrik commented Mar 7, 2024

Can you please rebase again?

There was a bug in the MetadataLoader over the week that got fixed now.

@bpatrik bpatrik added this to the Next (probably v2.5) milestone Mar 7, 2024
@bpatrik bpatrik self-requested a review March 7, 2024 22:00
@kagahd
Copy link
Contributor Author

kagahd commented Mar 11, 2024

Hi @bpatrik, all merge conflicts due to the bugfix of PR #829 have been resolved again for a second time.

@bpatrik
Copy link
Owner

bpatrik commented Mar 11, 2024

Thanks!

The sidecar tests are failing, can you please take a look?

@kagahd
Copy link
Contributor Author

kagahd commented Mar 11, 2024

The sidecar tests are failing, can you please take a look?

Not for me 😄
When I resolved the merge conflicts due to PR #829, some sidecar tests also failed because of a different creationDate. However, my code does not touch creationDate at all. It seems that the MetadataLoader.ts changes done in #829 read the creationDate inconsistently.
How to proceed? I can't reproduce the failing tests, see screenshot.
PiGallery2TestsOK

@bpatrik
Copy link
Owner

bpatrik commented Mar 11, 2024

My guess those files do not have a creation date in metadata and app falls back to the filesystem's date, so the test are flaky.

Did you happen to touch those files?

@grasdk do you have any idea?

I take a look later but I won't be at computer during the next few weeks.

@grasdk
Copy link
Contributor

grasdk commented Mar 11, 2024

I'll take a look tonight CET.

@kagahd did you try removing the node modules dir or git clone your code into a new empty dir and rebuild everything from scratch? This would have an affect on the environment similar to what the automated test that fails has.

Also: what is your timezone? I added timezone support..if metadata does not have timezone, local timezone is assumed. Your timezone might not be the same as the build server's timezone, which may cause irreproducible test results between your environment and the build server's. You can mitigate that by.expmicitly adfing timezone offset tonyour test data.

/Regards

@grasdk
Copy link
Contributor

grasdk commented Mar 11, 2024

@bpatrik was right. In the absense of reading creation date from metadata, file date was read. Since the test files were created at different times on the server and on @kagahd 's test environment, there was a failure.

I made a not-too-pretty pull request for @kagahd 's branch: kagahd#1 that fixes the immediate problem. I think the test data that this PR brings is worth it.

Also I think that these tests will benefit the other pull request for metadata: #841

@kagahd
Merge pull request #1 from grasdk/feature/kagahd_issue838
f356eae 
Added read of CreationDate to sidecar and fixed tests
@kagahd
Copy link
Contributor Author

kagahd commented Mar 12, 2024

Thanks @grasdk for the PR that I merged already.
@bpatrik , this PR should now be ready to merge.

@bpatrik
Copy link
Owner

bpatrik commented Mar 12, 2024

Thank you for both of you!

@bpatrik bpatrik merged commit 3ec3b5a into bpatrik:master Mar 12, 2024
@grasdk grasdk mentioned this pull request Mar 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bpatrik bpatrik Awaiting requested review from bpatrik

Assignees

@kagahd kagahd

Labels

enhancement

Projects

None yet

Milestone

Next (probably v3.0)

Development

Successfully merging this pull request may close these issues.

3 participants

@kagahd @bpatrik @grasdk