Add option to make the smart albums only consider photos of the user #3879
Conversation
📝 WalkthroughWalkthroughAdds a per-owner scoping option for smart albums that, when enabled and a user is authenticated, filters smart-album photo queries to the current user's photos. Implements conditional query logic in smart-album classes, a migration to add the config option, and translations in 21 language files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/SmartAlbums/UnsortedAlbum.php (1)
15-19: Unsorted per-owner behavior tightly couples to BaseSmartAlbum override semanticsGiven the current
BaseSmartAlbum::photos()implementation, this condition:if ($this->public_permissions !== null && (!Auth::check() || !Configs::getValueAsBool('enable_smart_album_per_owner'))) { // ... }means Unsorted’s per-owner scoping only ever kicks in when:
- the album is public, and
- the user is authenticated, and
enable_smart_album_per_owneris true, andSA_override_visibilityis enabled (due to howparent::photos()behaves).That shares the same “docs say overrides must be disabled” mismatch as in
BaseSmartAlbum. Once you decide how to resolve that at the base class, this method’sifcondition may need to be revisited to match the intended matrix of:
- public vs private Unsorted,
- logged-in vs anonymous,
- per-owner flag on/off,
- SA_override_visibility on/off.
For readability, you might also consider inverting the condition to make the per-owner path explicit:
Optional readability refactor
public function photos(): Builder { - if ($this->public_permissions !== null && (!Auth::check() || !Configs::getValueAsBool('enable_smart_album_per_owner'))) { - return Photo::query()->leftJoin(PA::PHOTO_ALBUM, 'photos.id', '=', PA::PHOTO_ID) - ->with(['size_variants', 'statistics', 'palette', 'tags']) - ->where($this->smart_photo_condition); - } - - return parent::photos(); + // If Unsoted is public and per-owner scoping does not apply, expose all unsorted photos. + if ($this->public_permissions !== null && (!Auth::check() || !Configs::getValueAsBool('enable_smart_album_per_owner'))) { + return Photo::query() + ->leftJoin(PA::PHOTO_ALBUM, 'photos.id', '=', PA::PHOTO_ID) + ->with(['size_variants', 'statistics', 'palette', 'tags']) + ->where($this->smart_photo_condition); + } + + // Otherwise, defer to the base smart album behavior (including any per-owner logic there). + return parent::photos(); }I’d recommend revisiting this after you finalize the intended
enable_smart_album_per_owner×SA_override_visibilitybehavior inBaseSmartAlbum.Also applies to: 42-56
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/SmartAlbums/BaseSmartAlbum.php(2 hunks)app/SmartAlbums/UnsortedAlbum.php(2 hunks)database/migrations/2025_12_19_161600_star_album_option.php(1 hunks)lang/ar/all_settings.php(2 hunks)lang/cz/all_settings.php(2 hunks)lang/de/all_settings.php(2 hunks)lang/el/all_settings.php(2 hunks)lang/en/all_settings.php(2 hunks)lang/es/all_settings.php(2 hunks)lang/fa/all_settings.php(2 hunks)lang/fr/all_settings.php(2 hunks)lang/hu/all_settings.php(2 hunks)lang/it/all_settings.php(2 hunks)lang/ja/all_settings.php(2 hunks)lang/nl/all_settings.php(2 hunks)lang/no/all_settings.php(2 hunks)lang/pl/all_settings.php(2 hunks)lang/pt/all_settings.php(2 hunks)lang/ru/all_settings.php(2 hunks)lang/sk/all_settings.php(2 hunks)lang/sv/all_settings.php(2 hunks)lang/vi/all_settings.php(2 hunks)lang/zh_CN/all_settings.php(2 hunks)lang/zh_TW/all_settings.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case
Apply PSR-4 coding standard
in_array() should be used with true as the third parameter for strict comparison
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty()
Use the moneyphp/money library for handling monetary values and currencies in PHP
Never use floats or doubles to represent monetary values; use integers to represent the smallest currency unit (e.g., cents for USD)
Files:
lang/ar/all_settings.phplang/fa/all_settings.phplang/zh_CN/all_settings.phplang/cz/all_settings.phpapp/SmartAlbums/UnsortedAlbum.phplang/de/all_settings.phplang/el/all_settings.phplang/es/all_settings.phplang/ja/all_settings.phplang/it/all_settings.phplang/no/all_settings.phplang/sv/all_settings.phplang/hu/all_settings.phplang/pt/all_settings.phplang/nl/all_settings.phpdatabase/migrations/2025_12_19_161600_star_album_option.phplang/sk/all_settings.phplang/vi/all_settings.phpapp/SmartAlbums/BaseSmartAlbum.phplang/en/all_settings.phplang/zh_TW/all_settings.phplang/pl/all_settings.phplang/ru/all_settings.phplang/fr/all_settings.php
🧠 Learnings (7)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3626
File: database/migrations/2025_06_07_144157_photo_tags_to_table.php:87-105
Timestamp: 2025-08-18T10:19:04.946Z
Learning: In the Lychee photo management system, the migration `2025_06_07_144157_photo_tags_to_table.php` runs on data that only contains comma-separated tag names in tag_albums.show_tags - OR/AND expressions do not exist at this migration time, so handling them is unnecessary.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/OrderService.php:65-73
Timestamp: 2025-09-16T21:56:01.607Z
Learning: The validation for album-photo membership in PurchasableService::getEffectivePurchasableForPhoto() uses a JOIN query with the photo_album pivot table to ensure that only purchasables for albums that actually contain the specified photo are returned, preventing price spoofing attacks where clients could use arbitrary album_ids.
📚 Learning: 2025-09-16T21:56:01.607Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/OrderService.php:65-73
Timestamp: 2025-09-16T21:56:01.607Z
Learning: The validation for album-photo membership in OrderService::addPhotoToOrder() is implemented within PurchasableService::getEffectivePurchasableForPhoto() using a whereHas('albums') constraint that ensures the photo belongs to the specified album_id before applying pricing logic.
Applied to files:
app/SmartAlbums/UnsortedAlbum.phpapp/SmartAlbums/BaseSmartAlbum.php
📚 Learning: 2025-09-16T21:56:01.607Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/OrderService.php:65-73
Timestamp: 2025-09-16T21:56:01.607Z
Learning: The validation for album-photo membership in PurchasableService::getEffectivePurchasableForPhoto() uses a JOIN query with the photo_album pivot table to ensure that only purchasables for albums that actually contain the specified photo are returned, preventing price spoofing attacks where clients could use arbitrary album_ids.
Applied to files:
app/SmartAlbums/UnsortedAlbum.phpapp/SmartAlbums/BaseSmartAlbum.php
📚 Learning: 2025-08-18T10:19:04.946Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3626
File: database/migrations/2025_06_07_144157_photo_tags_to_table.php:87-105
Timestamp: 2025-08-18T10:19:04.946Z
Learning: In the Lychee photo management system, the migration `2025_06_07_144157_photo_tags_to_table.php` runs on data that only contains comma-separated tag names in tag_albums.show_tags - OR/AND expressions do not exist at this migration time, so handling them is unnecessary.
Applied to files:
database/migrations/2025_12_19_161600_star_album_option.php
📚 Learning: 2025-09-22T12:35:38.842Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3706
File: tests/Unit/Http/Controllers/ImportFromServerControllerTest.php:59-66
Timestamp: 2025-09-22T12:35:38.842Z
Learning: In the Lychee codebase, the Configs class extends Laravel's Model, making config values database-backed. Tests using the DatabaseTransactions trait automatically have config changes rolled back without requiring manual restoration in tearDown() methods.
Applied to files:
database/migrations/2025_12_19_161600_star_album_option.php
📚 Learning: 2025-09-13T14:01:20.039Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/OrderService.php:0-0
Timestamp: 2025-09-13T14:01:20.039Z
Learning: In PurchasableService::getEffectivePurchasableForPhoto(), both the photo-specific and album-level SQL queries include ->where('is_active', true), so the returned purchasable is guaranteed to be active and doesn't need additional is_active checks in calling code.
Applied to files:
app/SmartAlbums/BaseSmartAlbum.php
📚 Learning: 2025-08-27T08:48:27.520Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3654
File: lang/cz/gallery.php:210-210
Timestamp: 2025-08-27T08:48:27.520Z
Learning: For this Lychee project, the maintainer prefers to keep language strings in English across all locale files rather than translating them to local languages.
Applied to files:
lang/fr/all_settings.php
🧬 Code graph analysis (2)
app/SmartAlbums/UnsortedAlbum.php (3)
app/Models/Configs.php (2)
Configs(65-369)getValueAsBool(284-287)app/SmartAlbums/BaseSmartAlbum.php (1)
public_permissions(187-190)app/Http/Controllers/VueController.php (1)
check(93-105)
app/SmartAlbums/BaseSmartAlbum.php (2)
app/Models/Configs.php (2)
Configs(65-369)getValueAsBool(284-287)app/Services/Auth/SessionOrTokenGuard.php (1)
id(227-230)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
🔇 Additional comments (29)
lang/it/all_settings.php (2)
305-305: Provide Italian translation instead of English text.This is an Italian localization file (
lang/it/), but the text is in English. Italian-speaking users will not benefit from proper localization.💡 Suggested Italian translation
Based on the feature description, consider:
- 'enable_smart_album_per_owner' => 'Only display pictures owned by the user in smart albums.', + 'enable_smart_album_per_owner' => 'Mostra solo le foto di proprietà dell\'utente negli album intelligenti.',Note: Please verify this translation with a native Italian speaker or use a professional translation service to ensure accuracy.
⛔ Skipped due to learnings
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3641 File: lang/no/settings.php:9-9 Timestamp: 2025-08-22T06:11:18.329Z Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3654 File: lang/pl/gallery.php:210-210 Timestamp: 2025-08-27T08:48:32.956Z Learning: The user ildyria does not prioritize strict localization consistency for new menu items in language files, as indicated by their "Lang = don't care" response when suggested to translate 'Import from Server' to Polish in lang/pl/gallery.php.
602-602: Provide Italian translation instead of English text.The details entry is in English but should be in Italian for this localization file.
💡 Suggested Italian translation
Consider:
- 'enable_smart_album_per_owner' => 'This setting is only applied to logged-in users. "Smart album visibility overrides" must be disabled for this to take effect.', + 'enable_smart_album_per_owner' => 'Questa impostazione si applica solo agli utenti autenticati. "Visibilità degli album intelligenti sovrascrive" deve essere disabilitato affinché abbia effetto.',Note: Please verify this translation with a native Italian speaker or use a professional translation service. Also ensure the referenced setting name "Smart album visibility overrides" matches its translated name elsewhere in the codebase.
⛔ Skipped due to learnings
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3641 File: lang/no/settings.php:9-9 Timestamp: 2025-08-22T06:11:18.329Z Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.lang/sk/all_settings.php (1)
305-305: LGTM: per-owner smart album setting strings look correct.Key
enable_smart_album_per_owneris named consistently, descriptions match the intended behavior and surrounding style, and PHP syntax is valid.Also applies to: 602-602
lang/hu/all_settings.php (1)
305-305: LGTM: new config entry is consistent and well-formed.
enable_smart_album_per_owneris added in both documentation and details with coherent descriptions and correct PHP syntax.Also applies to: 602-602
lang/pl/all_settings.php (1)
305-305: LGTM: per-owner smart album setting added correctly.Key name, description, and placement match the rest of the configuration schema and are syntactically valid.
Also applies to: 602-602
lang/cz/all_settings.php (1)
305-305: LGTM: config key and explanations are consistent.
enable_smart_album_per_owneris introduced in both sections with clear wording and correct array syntax; no issues found.Also applies to: 602-602
lang/en/all_settings.php (1)
305-305: LGTM: English source strings accurately describe the new option.The documentation and details for
enable_smart_album_per_ownercorrectly scope the feature to logged-in users and note the interaction with “Smart album visibility overrides”; syntax and placement are fine.Also applies to: 602-602
lang/vi/all_settings.php (1)
305-305: LGTM: new setting entries are consistent and syntactically correct.
enable_smart_album_per_owneris added to both arrays with the same clear descriptions used elsewhere and valid PHP array syntax.Also applies to: 602-602
lang/es/all_settings.php (2)
305-305: New documentation key matches config and behaviorKey
enable_smart_album_per_owneris correctly named and the description is clear and consistent with other locales.
602-602: Details text correctly explains scope and dependencyThe description accurately limits the setting to logged‑in users and notes the interaction with “Smart album visibility overrides.”
lang/de/all_settings.php (2)
303-303: Config key wired into documentation section correctly
enable_smart_album_per_owneris added in the expected place with wording consistent with other languages and the feature intent.
600-600: Details entry clearly documents runtime behaviorThe details string correctly documents that this applies only to logged‑in users and depends on “Smart album visibility overrides” being disabled.
lang/pt/all_settings.php (2)
305-305: Documentation entry for per‑owner smart albums is consistentKey name and description align with the new
enable_smart_album_per_ownerfeature and other locales.
602-602: Details text correctly states constraints for the settingClarifies logged‑in‑only scope and dependency on “Smart album visibility overrides,” matching backend behavior.
lang/nl/all_settings.php (2)
305-305: Per‑owner smart album documentation string added correctly
enable_smart_album_per_owneris introduced with clear text and in the standard position among smart‑album‑related options.
602-602: Details description matches feature semanticsAccurately documents that the flag affects only authenticated users and is gated by the “Smart album visibility overrides” setting.
lang/zh_TW/all_settings.php (2)
305-305: New documentation key is consistent and correctly namedThe
enable_smart_album_per_ownerdocumentation entry mirrors other locales and the underlying config key.
602-602: Details entry documents feature interaction accuratelyThe explanation around logged‑in scope and “Smart album visibility overrides” behavior is correct and consistent with the rest of the PR.
lang/zh_CN/all_settings.php (2)
305-305: Documentation string for per‑owner smart albums looks goodKey name, placement, and text are consistent with the new configuration option and other locale files.
602-602: Details description correctly captures usage conditionsThe details text matches the intended behavior (logged‑in only, disabled when smart album visibility overrides are on).
lang/ja/all_settings.php (1)
305-305: New per-owner smart album setting strings look consistentThe new
enable_smart_album_per_ownerentries in bothdocumentationanddetailssections are consistent with other locales and the feature behavior; no issues from a code perspective.Also applies to: 602-602
lang/fr/all_settings.php (1)
303-303: enable_smart_album_per_owner entries added correctlyBoth
detailsanddocumentationentries forenable_smart_album_per_ownerare syntactically correct and aligned with the new feature; keeping the text in English here is consistent with recent additions.Also applies to: 599-599
lang/ar/all_settings.php (1)
305-305: Localization entries for per-owner smart albums are consistentThe
enable_smart_album_per_ownerstrings in both sections match the intended behavior and other locales; nothing to change here.Also applies to: 602-602
lang/ru/all_settings.php (1)
305-305: Per-owner smart album config strings added as expectedThe added
enable_smart_album_per_ownerkeys are well-placed and mirror the other language files; no issues spotted.Also applies to: 602-602
lang/el/all_settings.php (1)
305-305: Greek locale: new per-owner setting wired correctly
enable_smart_album_per_owneris added to bothdocumentationanddetailswith the same wording as other locales; looks good.Also applies to: 602-602
lang/no/all_settings.php (1)
305-305: Norwegian locale: per-owner smart album setting wired in correctlyThe
enable_smart_album_per_ownerentries are added to bothdocumentationanddetailswith the same wording and placement as other locales; everything looks fine.Also applies to: 602-602
database/migrations/2025_12_19_161600_star_album_option.php (1)
1-35: LGTM!The migration correctly follows the BaseConfigMigration pattern with all required fields properly defined. The license header is present, and the code adheres to PSR-4 standards.
lang/sv/all_settings.php (1)
305-305: Provide Swedish translations instead of English text.The Swedish translation file contains English text for the new
enable_smart_album_per_ownerconfiguration option. Swedish-speaking users will see English text for this setting, which breaks the localization experience.Also applies to: 602-602
⛔ Skipped due to learnings
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3641 File: lang/no/settings.php:9-9 Timestamp: 2025-08-22T06:11:18.329Z Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3654 File: lang/es/gallery.php:210-210 Timestamp: 2025-08-27T08:48:45.672Z Learning: The project maintainer ildyria has indicated that language localization consistency is not a priority ("Lang = don't care"), meaning English text in non-English language files is acceptable and should not be flagged as an issue.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3637 File: lang/nl/renamer.php:10-94 Timestamp: 2025-08-20T20:35:04.474Z Learning: In Lychee, translation files are initially created with English strings as placeholders, and actual translations are handled through Weblate (a web-based translation management system). This means finding English text in non-English locale files (like lang/nl/, lang/de/, etc.) is expected and part of their translation workflow, not an issue to flag.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3654 File: lang/pl/gallery.php:210-210 Timestamp: 2025-08-27T08:48:32.956Z Learning: The user ildyria does not prioritize strict localization consistency for new menu items in language files, as indicated by their "Lang = don't care" response when suggested to translate 'Import from Server' to Polish in lang/pl/gallery.php.lang/fa/all_settings.php (1)
305-305: Provide Persian/Farsi translations instead of English text.The Persian/Farsi translation file contains English text for the new
enable_smart_album_per_ownerconfiguration option. Persian/Farsi-speaking users will see English text for this setting, which breaks the localization experience.Also applies to: 602-602
⛔ Skipped due to learnings
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3641 File: lang/no/settings.php:9-9 Timestamp: 2025-08-22T06:11:18.329Z Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3654 File: lang/es/gallery.php:210-210 Timestamp: 2025-08-27T08:48:45.672Z Learning: The project maintainer ildyria has indicated that language localization consistency is not a priority ("Lang = don't care"), meaning English text in non-English language files is acceptable and should not be flagged as an issue.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3654 File: lang/pl/gallery.php:210-210 Timestamp: 2025-08-27T08:48:32.956Z Learning: The user ildyria does not prioritize strict localization consistency for new menu items in language files, as indicated by their "Lang = don't care" response when suggested to translate 'Import from Server' to Polish in lang/pl/gallery.php.Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3637 File: lang/nl/renamer.php:10-94 Timestamp: 2025-08-20T20:35:04.474Z Learning: In Lychee, translation files are initially created with English strings as placeholders, and actual translations are handled through Weblate (a web-based translation management system). This means finding English text in non-English locale files (like lang/nl/, lang/de/, etc.) is expected and part of their translation workflow, not an issue to flag.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.