Fix propagation not happening #3933
Conversation
|
Warning Rate limit exceeded@ildyria has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PhotoSaved event dispatch is relocated from the Save pipe to the SetParent pipe, triggering when a photo-album relationship is established. A new test validates that uploading a photo dispatches the correct album recomputation jobs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
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 PR right away. ✅ 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 (3)
app/Actions/Photo/Pipes/Shared/Save.phpapp/Actions/Photo/Pipes/Shared/SetParent.phptests/ImageProcessing/Photo/PhotoAddTest.php
💤 Files with no reviewable changes (1)
- app/Actions/Photo/Pipes/Shared/Save.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tests/ImageProcessing/Photo/PhotoAddTest.phpapp/Actions/Photo/Pipes/Shared/SetParent.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
No need to mock the database in tests; use the in-memory SQLite database instead
Files:
tests/ImageProcessing/Photo/PhotoAddTest.php
🧠 Learnings (7)
📓 Common learnings
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.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/PurchasableService.php:150-168
Timestamp: 2025-09-18T07:09:17.176Z
Learning: In PurchasablePhotoRequest, the validation for photo-album membership is implemented at the request validation level rather than in the service layer, using a count comparison to ensure all photos belong to the target album before creating purchasables.
📚 Learning: 2025-08-21T09:08:59.512Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: app/Console/Commands/Sync.php:212-218
Timestamp: 2025-08-21T09:08:59.512Z
Learning: ProcessImageJob is used by the Web API for single photo uploads and only needs photo title renaming (shall_rename_photo_title). ImportImageJob is used by the CLI for bulk imports and receives the full ImportMode with both photo and album renaming flags from the sync command.
Applied to files:
tests/ImageProcessing/Photo/PhotoAddTest.php
📚 Learning: 2025-12-22T14:11:17.217Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T14:11:17.217Z
Learning: Applies to tests/Feature_v2/**/*.php : Tests in the tests/Feature_v2 directory should extend from BaseApiWithDataTest
Applied to files:
tests/ImageProcessing/Photo/PhotoAddTest.php
📚 Learning: 2025-08-25T19:07:01.275Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 2562
File: tests/Feature_v2/Photo/PhotoZipUploadTest.php:81-95
Timestamp: 2025-08-25T19:07:01.275Z
Learning: In the Lychee codebase, when testing ZIP file uploads in PhotoZipUploadTest, Laravel middleware does not trigger for ZipInvalidException, so the exception is thrown directly to PHPUnit rather than being converted to an HTTP response. This means using $this->expectException(ZipInvalidException::class) is the correct approach rather than asserting HTTP status codes.
Applied to files:
tests/ImageProcessing/Photo/PhotoAddTest.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:
tests/ImageProcessing/Photo/PhotoAddTest.phpapp/Actions/Photo/Pipes/Shared/SetParent.php
📚 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/Photo/Pipes/Shared/SetParent.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 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/Photo/Pipes/Shared/SetParent.php
🧬 Code graph analysis (2)
tests/ImageProcessing/Photo/PhotoAddTest.php (2)
app/Jobs/RecomputeAlbumSizeJob.php (1)
RecomputeAlbumSizeJob(32-194)app/Jobs/RecomputeAlbumStatsJob.php (1)
RecomputeAlbumStatsJob(37-365)
app/Actions/Photo/Pipes/Shared/SetParent.php (1)
app/Events/PhotoSaved.php (1)
PhotoSaved(14-25)
🔇 Additional comments (1)
tests/ImageProcessing/Photo/PhotoAddTest.php (1)
166-192: Well-structured test for job dispatch verification.The test correctly uses selective queue faking to verify that photo uploads trigger the expected recomputation jobs. The approach of faking only the recompute jobs (lines 170-173) while allowing
ProcessImageJobto execute ensures the upload completes normally, and the closure-based assertions (lines 182-184, 187-189) properly validate that both jobs receive the correct album context.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.