Skip to content
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

feat(server): add album filter to search tab #16979

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

DarrenMa
Copy link

feat(server): Add Album Filter to Search Tab #12258

Description

Implements the server API part of allowing for search of assets within albums.

Fixes #12258

How Has This Been Tested?

Test Setup

Total of 6 images:

Mario Album

  • 20160331195151!SMB_Small_Mario_Jumping_Sprite.png
  • 287px-Mario_art15.png
  • 537px-Brosscared.png

Luigi Album

  • 537px-Brosscared.png (also in Mario Album)
  • 553px-NSLU_Luigi_and_Yoshi_Artwork.png

No Album

  • 588px-Peach_&_Bowser.png
  • 600px-Bowser_SMB3_artwork.jpg

Note: 537px-Brosscared.png appears in both the Mario and Luigi albums.

Test Cases

  1. Smart search 'mario' (no album filter) → Expected: 6 results (all images).
  2. Metadata search 'mario' (no album filter) → Expected: 2 results (only images with 'mario' in the filename).
  3. Smart search 'mario' (Mario Album only) → Expected: 3 results (all files within the album).
  4. Metadata search 'mario' (Mario Album only) → Expected: 2 results (only files with 'mario' in the filename).
  5. Metadata search 'mario' (Luigi Album only) → Expected: 0 results.
  6. Metadata search 'mario' (Mario + Luigi Albums) → Expected: 2 results (only 'mario' files).
  7. Metadata search 'luigi' (Mario + Luigi Albums) → Expected: 1 result (only the 'luigi' file).
  8. Smart search 'bowser' (Mario + Luigi Albums) → Expected: 4 results (only album images, excluding Bowser images).
  9. Random search. Without album retrieves any random image of the 6. Specifying luigi albumId always retrieves only 1 of either of the 2 within the album.

Unittests

  • Added a GOOD and a BAD test for 'POST /search/metadata'.
  • These tests check requests where albumIds is empty.

API Changes

The /api/search/smart endpoint accepts new optional param "albumIds":[]
The /api/search/metadata endpoint accepts new optional param "albumIds":[]
The /api/search/random endpoint accepts new optional param "albumIds":[]

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
    • I didn't run make open-api
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Your description mentions tests but I don't see those in the diff, did you forget to push them?

.gitignore Outdated
@@ -23,3 +23,4 @@ mobile/android/fastlane/report.xml
mobile/ios/fastlane/report.xml

vite.config.js.timestamp-*
requests.http
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patterns which are specific to a particular repository but which do not need to be shared with other related repositories (e.g., auxiliary files that live inside the repository but are specific to one user’s workflow) should go into the $GIT_DIR/info/exclude file.

(from https://git-scm.com/docs/gitignore)

@bo0tzz bo0tzz changed the title feat(server): Add Album Filter to Search Tab #12258 feat(server): add album filter to search tab Mar 19, 2025
@DarrenMa
Copy link
Author

Thanks!

Your description mentions tests but I don't see those in the diff, did you forget to push them?

Hi bo0tzz.

Fixed the .gitignore.

The functional tests with the actual images I did locally on my machine.

The unittests that I modified are here: main...DarrenMa:immich:feature/12258#diff-cb7433da384004a1abf31a700895fcfe157a0b98bfb09a67704e1d3cde417163
I only added 2 checks, a bad check and a good to tests that were already there.

I'm at your disposal for any updates that you require.

mertalev
mertalev previously approved these changes Mar 19, 2025
Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@mertalev mertalev dismissed their stale review March 19, 2025 18:15

You added e2e tests for when albumIds is null or empty, but it doesn't test that it filters by album when it's non-empty.

@DarrenMa
Copy link
Author

I added an e2e for a valued albumIds param and a bad check for an invalid format albumsIds param.
Is it correct as is or should the tests be done differently?
Thank you for your time.

@alextran1502 alextran1502 requested a review from mertalev March 21, 2025 17:45
@znebby
Copy link

znebby commented Mar 26, 2025

@DarrenMa so you've added the following:

  1. Search for images in any of given albums.

It would be great to expand this to allow the also these searches:

  1. Restrict to images that are in multiple given albums. Basically AND instead of OR for albums.
  2. Restrict to images that are NOT in given albums.
  3. Combination of 1 + 3
  4. Combination of 2 + 3

@danieldietzler
Copy link
Member

@DarrenMa so you've added the following:

1. Search for images in any of given albums.

It would be great to expand this to allow the also these searches:

2. Restrict to images that are in multiple given albums. Basically AND instead of OR for albums.

3. Restrict to images that are NOT in given albums.

4. Combination of 1 + 3

5. Combination of 2 + 3

This is far beyond the scope of this PR and also requires some brain cells to come up with a solution that still performs well (negatively lookups generally are slow). Also, this is something that's not limited to album names but pretty much every search filter. So please don't post in some unrelated PR. There are already feature requests for all of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants