Fix possible deletion issue #3935
Conversation
ildyria
added
the
ignore-for-release
📝 WalkthroughWalkthroughThe pull request modifies the Album Delete action to optimize performance. It adds the DB facade for raw queries, expands the album query's exclusion list, and replaces Eloquent-based sub-album retrieval with a direct database query to reduce overhead. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
|
@coderabbitai review ? |
|
@ildyria: I'll review the changes in this pull request. 🧠 Learnings used✅ 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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Actions/Album/Delete.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/Delete.php
🧠 Learnings (7)
📓 Common learnings
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.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/PurchasableService.php:286-296
Timestamp: 2025-09-18T13:37:32.687Z
Learning: The deleteMultipleAlbumPurchasables() method in PurchasableService is designed to delete ALL purchasables (both album-level with photo_id IS NULL and photo-level with photo_id IS NOT NULL) for the specified album_ids. This is the intended behavior when albums are being deleted, as all photos within those albums will also be deleted, making their purchasables obsolete.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3605
File: docs/Lifecycle-of-a-request-photo-upload.md:144-151
Timestamp: 2025-08-12T11:31:09.853Z
Learning: In Lychee, the BaseApiRequest class has been modified to change Laravel's default execution order - processValidatedValues() runs BEFORE authorize(), not after. This means that in Lychee request classes, $this->album and other processed properties are available during authorization checks, unlike standard Laravel behavior.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3922
File: app/Listeners/RecomputeAlbumSizeOnPhotoMutation.php:63-69
Timestamp: 2026-01-03T12:36:02.514Z
Learning: In Lychee, "deleting a photo from an album" (removing the photo-album association) is distinct from "deleting a photo entirely". When PhotoDeleted event is dispatched with a specific album_id, it indicates the photo was removed from that particular album only. The photo may still exist in other albums due to the many-to-many relationship, so only the specified album's size statistics need recomputation.
📚 Learning: 2026-01-03T12:36:02.514Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3922
File: app/Listeners/RecomputeAlbumSizeOnPhotoMutation.php:63-69
Timestamp: 2026-01-03T12:36:02.514Z
Learning: In Lychee, "deleting a photo from an album" (removing the photo-album association) is distinct from "deleting a photo entirely". When PhotoDeleted event is dispatched with a specific album_id, it indicates the photo was removed from that particular album only. The photo may still exist in other albums due to the many-to-many relationship, so only the specified album's size statistics need recomputation.
Applied to files:
app/Actions/Album/Delete.php
📚 Learning: 2025-09-21T20:07:11.825Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Http/Resources/Shop/CatalogResource.php:11-16
Timestamp: 2025-09-21T20:07:11.825Z
Learning: Spatie\LaravelData\DataCollection does NOT extend Illuminate\Support\Collection - it only implements some collection methods. To use Collection type, you must explicitly collect with Collection::class as the second parameter.
Applied to files:
app/Actions/Album/Delete.php
📚 Learning: 2025-09-21T20:07:11.825Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Http/Resources/Shop/CatalogResource.php:11-16
Timestamp: 2025-09-21T20:07:11.825Z
Learning: Spatie\LaravelData\DataCollection extends Illuminate\Support\Collection, so DataCollection instances can be assigned to Collection-typed properties without runtime errors.
Applied to files:
app/Actions/Album/Delete.php
📚 Learning: 2025-09-13T11:23:06.438Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Http/Requests/ShopManagement/DeletePurchasablesRequest.php:42-45
Timestamp: 2025-09-13T11:23:06.438Z
Learning: In Lychee codebase, avoid using database validation rules like 'exists' and array validation rules like 'distinct' in request classes. Let model operations (like findOrFail) handle entity validation instead.
Applied to files:
app/Actions/Album/Delete.php
📚 Learning: 2025-09-13T14:07:11.118Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Http/Controllers/Shop/CatalogController.php:48-52
Timestamp: 2025-09-13T14:07:11.118Z
Learning: In CatalogController, the children_purchasables query intentionally uses parent_id to fetch only direct children albums, not the entire subtree via nested set bounds. This is a design decision to show purchasables from immediate child albums only.
Applied to files:
app/Actions/Album/Delete.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/Delete.php
🧬 Code graph analysis (1)
app/Actions/Album/Delete.php (3)
app/Models/AccessPermission.php (1)
album(112-115)app/Actions/Albums/Top.php (1)
get(67-136)app/Actions/Album/PositionData.php (1)
get(19-45)
🔇 Additional comments (2)
app/Actions/Album/Delete.php (2)
31-31: LGTM - DB facade import for query optimization.The import is necessary for the direct table query on lines 105-108, which bypasses Eloquent overhead as intended by this performance optimization.
91-91: LGTM - Appropriate exclusion of unused relationships.The expanded exclusion list correctly omits relationships that aren't needed during deletion. These relations (
owner,access_permissions,statistics) are handled separately in dedicated queries later in the method, so excluding them here reduces unnecessary eager loading overhead.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.