Fix unexpected password propagation behaviour #3927
Conversation
📝 WalkthroughWalkthroughIntroduces unlock propagation functionality for albums sharing the same password. When an album is unlocked, the system propagates this unlock to related albums with matching passwords, controlled by a new expert-level configuration flag that defaults to disabled. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks✅ Passed checks (1 passed)
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 review ? |
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
|
@ildyria: I'll review the changes in this PR now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review ? |
|
@ildyria: I'll review the changes in this PR now. ✅ Actions performedReview triggered.
|
![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 (2)
app/Actions/Album/Unlock.php (2)
79-79: Remove unusedowner_idselection.The
owner_idfield is selected in the query but never used in the subsequent logic. This adds unnecessary data retrieval overhead.🔎 Proposed fix
$albums = BaseAlbumImpl::query() - ->select(['base_albums.id', 'base_albums.owner_id', APC::PASSWORD]) + ->select(['base_albums.id', APC::PASSWORD]) ->join(APC::ACCESS_PERMISSIONS, 'base_album_id', '=', 'base_albums.id', 'inner')
85-89: Check if album is already unlocked before calling unlock.The loop calls
$this->album_policy->unlock($album)for every matching album without checking if it's already unlocked. This could add redundant entries to the session.🔎 Proposed fix
/** @var BaseAlbumImpl $album */ foreach ($albums as $album) { - if (Hash::check($password, $album->password)) { + if (Hash::check($password, $album->password) && !$this->album_policy->isUnlocked($album)) { $this->album_policy->unlock($album); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Actions/Album/Unlock.phpdatabase/migrations/2026_01_22_000000_propagate_unlock_option.php
🧰 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 in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
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() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Actions/Album/Unlock.phpdatabase/migrations/2026_01_22_000000_propagate_unlock_option.php
🧠 Learnings (4)
📚 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/Actions/Album/Unlock.php
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Actions/Album/Unlock.phpdatabase/migrations/2026_01_22_000000_propagate_unlock_option.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/2026_01_22_000000_propagate_unlock_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/2026_01_22_000000_propagate_unlock_option.php
🧬 Code graph analysis (1)
app/Actions/Album/Unlock.php (2)
app/Http/Controllers/Gallery/AlbumController.php (1)
unlock(354-357)app/Policies/AlbumPolicy.php (1)
unlock(546-549)
🔇 Additional comments (1)
database/migrations/2026_01_22_000000_propagate_unlock_option.php (1)
1-35: Confirm the migration timestamp is intentional.No ordering conflicts were found with other migrations. The timestamp
2026_01_22_000000chronologically follows the previous migrations (2026_01_01_*) and has no subsequent migrations that would be affected. However, if this date is not intentional for scheduled deployment, update the timestamp to reflect the actual creation date.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.