Skip to content

Search: Add altitude range filter #3800

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

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

Gilbert32
Copy link
Contributor

@Gilbert32 Gilbert32 commented Oct 7, 2023

This pull request adds 2 search parameters, maxalt (Maximum Altitude inclusive) and minalt (Minimum Altitude inclusive).

It uses the column photos.photo_altitude to filter photos based on the specified query.
I have added the 2 parameters and basic tests based on my need, however given I don't have extensive knowledge in the photoprism source code, and go, There's the possibility I might have missed to implement it in some place in the source code.
I also added basic tests and they passed.

Acceptance Criteria:

  • Features and enhancements must be fully implemented so that they can be released at any time without additional work
  • Automated unit and/or acceptance tests are mandatory to ensure the changes work as expected and to reduce repetitive manual work
  • Frontend components must be responsive to work and look properly on phones, tablets, and desktop computers; you must have tested them on all major browsers and different devices
  • Documentation and translation updates should be provided if needed
  • In case you submit database-related changes, they must be tested and compatible with SQLite 3 and MariaDB 10.5.12+

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2023

CLA assistant check
All committers have signed the CLA.

@Gilbert32
Copy link
Contributor Author

I have added a docs commit on my fork of photoprism-docs repository, however i'm not sure how to proceed. Should I create a PR for that if this PR was merged?

photoprism/photoprism-docs@master...Gilbert32:photoprism-docs:feature/filter_altitude

@lastzero
Copy link
Member

Thank you! I think it makes sense to implement this in a similar way as I've just added this:

@Gilbert32
Copy link
Contributor Author

Awesome! I will adjust my code as soon as I have free time. Until then, I'm marking the issue as not fully implemented.

@Gilbert32 Gilbert32 force-pushed the feature/altitude_filter branch from 9c3ccb9 to e8a491e Compare October 14, 2023 20:04
@Gilbert32
Copy link
Contributor Author

I've updated the PR, now the code uses IntRange instead of separate min and max filters.

@lastzero
Copy link
Member

@Gilbert32 I'll merge this now, but may need to change a few details:

  • From what I know, the altitude can be negative.
  • The filter docs should mention that it has to be specified in meters (m).

Thank you for your contribution! 🥇

@lastzero lastzero self-requested a review October 20, 2023 10:21
@lastzero lastzero added enhancement Enhancement or improvement of an existing feature metadata Related to Exif, XMP, IPTC & Co. merged Changes are merged, but may require further testing and removed work-in-progress labels Oct 20, 2023
@lastzero lastzero merged commit 132237e into photoprism:develop Oct 20, 2023
@lastzero lastzero changed the title Add MinAlt and MaxAlt filters Search: Add altitude range filter Oct 20, 2023
lastzero added a commit that referenced this pull request Oct 20, 2023
Signed-off-by: Michael Mayer <michael@photoprism.app>
@graciousgrey graciousgrey added tested Changes have been tested successfully released Available in the stable release labels Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement of an existing feature merged Changes are merged, but may require further testing metadata Related to Exif, XMP, IPTC & Co. released Available in the stable release tested Changes have been tested successfully
Projects
Status: Release 🌈
Development

Successfully merging this pull request may close these issues.

4 participants