-
-
Notifications
You must be signed in to change notification settings - Fork 318
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.
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
Smart album - On This Day - new feature #1607
Conversation
Co-authored-by: Benoît Viguier <[email protected]>
@aldjordje I sent you an Triage invite so you are able to run the pipelines without waiting for our approval. :) |
Co-authored-by: Benoît Viguier <[email protected]>
Co-authored-by: qwerty287 <[email protected]>
@aldjordje is this ready for review ? :) |
@ildyria Thank you for enabling exceptions for 500 errors to be displayed. Without it, I couldn't figure out what the problem was :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% LGTM.
Please add the following standalone test to AlbumTest.php
.
- Get today date
- Log as admin
- Upload photo -> get the photo ID
- Get photo model and set taken_at of the picture to today - 1 year.
- Open "On this day"
- Check that photo ID is present in the response.
- Log out.
This will guarantee that the SQL request is also working. :)
Some pictures do not have a taken_at
(exif data missing), maybe in those case we could consider the created_at
instead ? See https://github.com/aldjordje/Lychee/pull/1/files :)
Improve query to support created_at when taken_at is null
@ildyria I was thinking about the test and I couldn't think of a way to write it. I'll add it today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
The smart album "On This Day" will allow users to see all the photos taken on this day.
Closes #1599