Webshop - Drop php8.3 #3868
Webshop - Drop php8.3 #3868
Conversation
|
Important Review skippedToo many files! 126 files out of 276 files are above the max files limit of 150. You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR upgrades the licensing system from Plus to Pro edition, replaces the legacy PayPal gateway with a custom implementation using PayPal Server SDK, adds Pro-status infrastructure for feature gating, updates test fixtures and traits to use Pro prerequisites, and adds fulfillment-related fields and logic to preserve size variants for purchased orders. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/Actions/Diagnostics/Pipes/Infos/VersionInfo.php (1)
79-95: Update docblock to reflect Pro/Signature instead of PlusThe
getVersionString()implementation now distinguishesSE,Pro, andSignature, but the comment still refers to “Plus for premium edition”. Consider updating the docblock to avoid confusion:- * SE for supporter edition - * Plus for premium edition + * SE for supporter edition + * Pro for pro edition + * Signature for signature editionroutes/api_v2_shop.php (1)
17-18: Align comment with new Pro-only middlewareThe middleware is now
support:pro, but the comment still says “Only for SE sorry.” Consider updating the comment for clarity:-// Only for SE sorry. -Route::middleware('support:pro')->group(function (): void { +// Only for Pro edition. +Route::middleware('support:pro')->group(function (): void {app/Rules/ConfigKeyRequireSupportRule.php (1)
48-52: Pro-level gating logic looks correct; double-check target URLThe new level-2/Pro check mirrors the existing supporter gating and looks fine. The message mentions “Pro Edition” but still links to the
get-supporter-editionURL; if there is a dedicated Pro landing page, consider updating the link to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
app/Actions/Diagnostics/Pipes/Infos/VersionInfo.php(1 hunks)app/Contracts/Http/Requests/HasProStatusBoolean.php(1 hunks)app/Http/Controllers/Admin/SettingsController.php(1 hunks)app/Http/Middleware/ConfigIntegrity.php(2 hunks)app/Http/Requests/Settings/GetAllConfigsRequest.php(1 hunks)app/Http/Requests/Traits/HasProStatusBooleanTrait.php(1 hunks)app/Rules/ConfigKeyRequireSupportRule.php(1 hunks)composer.json(1 hunks)database/migrations/2025_12_15_161819_webshop_level_2.php(1 hunks)routes/api_v2_shop.php(1 hunks)tests/Constants/FreeVerifyier.php(1 hunks)tests/Constants/ProVerifyier.php(1 hunks)tests/Constants/SupporterVerifyier.php(1 hunks)tests/Traits/RequirePro.php(1 hunks)tests/Traits/RequireSupport.php(2 hunks)tests/Unit/Middleware/ConfigIntegrityTest.php(2 hunks)tests/Webshop/BasketControllerTest.php(4 hunks)tests/Webshop/CatalogControllerTest.php(2 hunks)tests/Webshop/Checkout/BaseCheckoutControllerTest.php(3 hunks)tests/Webshop/Checkout/CheckoutControllerTest.php(1 hunks)tests/Webshop/OrderManagement/FlushOldOrdersTest.php(3 hunks)tests/Webshop/OrderManagement/OrderListingAccessTest.php(4 hunks)tests/Webshop/Purchasables/ShopManagementControllerTest.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
app/Rules/ConfigKeyRequireSupportRule.phpapp/Http/Controllers/Admin/SettingsController.phpdatabase/migrations/2025_12_15_161819_webshop_level_2.phptests/Traits/RequirePro.phptests/Traits/RequireSupport.phpapp/Contracts/Http/Requests/HasProStatusBoolean.phptests/Webshop/CatalogControllerTest.phpapp/Actions/Diagnostics/Pipes/Infos/VersionInfo.phptests/Webshop/Checkout/BaseCheckoutControllerTest.phpapp/Http/Requests/Settings/GetAllConfigsRequest.phproutes/api_v2_shop.phptests/Webshop/Purchasables/ShopManagementControllerTest.phptests/Webshop/BasketControllerTest.phpapp/Http/Requests/Traits/HasProStatusBooleanTrait.phptests/Constants/ProVerifyier.phptests/Webshop/Checkout/CheckoutControllerTest.phptests/Webshop/OrderManagement/OrderListingAccessTest.phptests/Unit/Middleware/ConfigIntegrityTest.phpapp/Http/Middleware/ConfigIntegrity.phptests/Constants/FreeVerifyier.phptests/Webshop/OrderManagement/FlushOldOrdersTest.phptests/Constants/SupporterVerifyier.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Traits/RequirePro.phptests/Traits/RequireSupport.phptests/Webshop/CatalogControllerTest.phptests/Webshop/Checkout/BaseCheckoutControllerTest.phptests/Webshop/Purchasables/ShopManagementControllerTest.phptests/Webshop/BasketControllerTest.phptests/Constants/ProVerifyier.phptests/Webshop/Checkout/CheckoutControllerTest.phptests/Webshop/OrderManagement/OrderListingAccessTest.phptests/Unit/Middleware/ConfigIntegrityTest.phptests/Constants/FreeVerifyier.phptests/Webshop/OrderManagement/FlushOldOrdersTest.phptests/Constants/SupporterVerifyier.php
**/Http/Requests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In Requests, if a user is provided by the query parameter, it should be placed in $this->user2; $this->user is reserved for the user making the request
Files:
app/Contracts/Http/Requests/HasProStatusBoolean.phpapp/Http/Requests/Settings/GetAllConfigsRequest.phpapp/Http/Requests/Traits/HasProStatusBooleanTrait.php
tests/Unit/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests in the tests/Unit directory should extend from AbstractTestCase
Files:
tests/Unit/Middleware/ConfigIntegrityTest.php
🧠 Learnings (7)
📚 Learning: 2025-08-20T20:32:03.676Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:106-108
Timestamp: 2025-08-20T20:32:03.676Z
Learning: In the Lychee codebase, ildyria intentionally uses destructive migration patterns where up() calls down() to ensure clean state, as indicated by "Working as intended" and "No mercy" comments in migrations like database/migrations/2025_08_19_160144_create_renamer_rules_table.php.
Applied to files:
database/migrations/2025_12_15_161819_webshop_level_2.php
📚 Learning: 2025-09-13T11:30:21.765Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php:237-239
Timestamp: 2025-09-13T11:30:21.765Z
Learning: In tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php, directly modifying the shared $this->test_order fixture in loops is an intentional design choice, not something that needs to be refactored to create fresh orders for each iteration.
Applied to files:
tests/Webshop/CatalogControllerTest.phptests/Webshop/Checkout/BaseCheckoutControllerTest.phptests/Webshop/Purchasables/ShopManagementControllerTest.phptests/Webshop/BasketControllerTest.phptests/Webshop/Checkout/CheckoutControllerTest.phptests/Webshop/OrderManagement/OrderListingAccessTest.phptests/Webshop/OrderManagement/FlushOldOrdersTest.php
📚 Learning: 2025-11-25T21:26:03.890Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T21:26:03.890Z
Learning: Applies to tests/Feature_v2/**/*.php : Tests in the tests/Feature_v2 directory should extend from BaseApiWithDataTest
Applied to files:
tests/Webshop/CatalogControllerTest.phptests/Webshop/Checkout/BaseCheckoutControllerTest.phptests/Webshop/Purchasables/ShopManagementControllerTest.phptests/Webshop/BasketControllerTest.phptests/Webshop/Checkout/CheckoutControllerTest.phptests/Webshop/OrderManagement/OrderListingAccessTest.phptests/Webshop/OrderManagement/FlushOldOrdersTest.php
📚 Learning: 2025-10-03T10:58:59.321Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3732
File: app/Http/Resources/Shop/OrderResource.php:62-64
Timestamp: 2025-10-03T10:58:59.321Z
Learning: In app/Http/Resources/Shop/OrderResource.php, the can_process_payment flag is intentionally set to false in order listings (via OrderService::getAll()) because items are explicitly excluded with without(['items']). This is by design: listings are lightweight, and the actual payment capability is only computed when fetching individual orders with full item data loaded.
Applied to files:
tests/Webshop/OrderManagement/OrderListingAccessTest.php
📚 Learning: 2025-09-21T20:22:26.400Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3703
File: app/Services/MoneyService.php:29-32
Timestamp: 2025-09-21T20:22:26.400Z
Learning: In the Lychee codebase, configuration keys added to the SE_FIELDS in ConfigIntegrity middleware are guaranteed to exist and contain valid values, so additional null checks or fallbacks are not needed when accessing them via Configs::getValueAsString().
Applied to files:
tests/Unit/Middleware/ConfigIntegrityTest.phpapp/Http/Middleware/ConfigIntegrity.php
📚 Learning: 2025-09-21T20:22:26.400Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3703
File: app/Services/MoneyService.php:29-32
Timestamp: 2025-09-21T20:22:26.400Z
Learning: In the Lychee webshop implementation, configuration keys like 'webshop_currency' are guaranteed to exist through the migration system and ConfigIntegrity middleware (SE_FIELDS), making additional null checks or fallbacks unnecessary when ildyria has architectural guarantees in place.
Applied to files:
tests/Unit/Middleware/ConfigIntegrityTest.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:
tests/Unit/Middleware/ConfigIntegrityTest.php
🧬 Code graph analysis (16)
app/Rules/ConfigKeyRequireSupportRule.php (5)
app/Contracts/Http/Requests/HasProStatusBoolean.php (1)
is_pro(13-13)app/Http/Requests/Traits/HasProStatusBooleanTrait.php (1)
is_pro(20-27)tests/Constants/FreeVerifyier.php (1)
is_pro(32-35)tests/Constants/ProVerifyier.php (1)
is_pro(32-35)tests/Constants/SupporterVerifyier.php (1)
is_pro(32-35)
app/Http/Controllers/Admin/SettingsController.php (6)
tests/Constants/FreeVerifyier.php (2)
when(49-54)is_pro(32-35)tests/Constants/ProVerifyier.php (2)
when(49-54)is_pro(32-35)tests/Constants/SupporterVerifyier.php (2)
when(49-54)is_pro(32-35)app/Contracts/Http/Requests/HasProStatusBoolean.php (1)
is_pro(13-13)app/Http/Requests/Traits/HasProStatusBooleanTrait.php (1)
is_pro(20-27)app/Eloquent/FixedQueryBuilderTrait.php (1)
where(66-76)
database/migrations/2025_12_15_161819_webshop_level_2.php (1)
app/Eloquent/FixedQueryBuilderTrait.php (1)
where(66-76)
tests/Traits/RequirePro.php (3)
tests/Constants/FreeVerifyier.php (2)
get_status(17-20)validate(56-59)tests/Constants/ProVerifyier.php (2)
get_status(17-20)validate(56-59)tests/Constants/SupporterVerifyier.php (2)
get_status(17-20)validate(56-59)
tests/Traits/RequireSupport.php (1)
tests/Constants/ProVerifyier.php (1)
ProVerifyier(15-60)
app/Contracts/Http/Requests/HasProStatusBoolean.php (4)
app/Http/Requests/Traits/HasProStatusBooleanTrait.php (1)
is_pro(20-27)tests/Constants/FreeVerifyier.php (1)
is_pro(32-35)tests/Constants/ProVerifyier.php (1)
is_pro(32-35)tests/Constants/SupporterVerifyier.php (1)
is_pro(32-35)
tests/Webshop/CatalogControllerTest.php (1)
tests/Traits/RequirePro.php (2)
requirePro(30-53)resetPro(55-61)
tests/Webshop/Checkout/BaseCheckoutControllerTest.php (1)
tests/Traits/RequirePro.php (2)
requirePro(30-53)resetPro(55-61)
app/Http/Requests/Settings/GetAllConfigsRequest.php (1)
app/Http/Requests/AbstractEmptyRequest.php (1)
AbstractEmptyRequest(11-31)
tests/Webshop/Purchasables/ShopManagementControllerTest.php (5)
tests/Webshop/BasketControllerTest.php (2)
setUp(49-65)tearDown(67-71)tests/Webshop/CatalogControllerTest.php (2)
setUp(39-44)tearDown(46-50)tests/Webshop/Checkout/CheckoutControllerTest.php (2)
setUp(31-35)tearDown(37-41)tests/Webshop/OrderManagement/FlushOldOrdersTest.php (2)
setUp(43-53)tearDown(55-59)tests/Traits/RequirePro.php (2)
requirePro(30-53)resetPro(55-61)
tests/Webshop/BasketControllerTest.php (1)
tests/Traits/RequirePro.php (2)
requirePro(30-53)resetPro(55-61)
tests/Webshop/Checkout/CheckoutControllerTest.php (3)
tests/Webshop/BasketControllerTest.php (2)
setUp(49-65)tearDown(67-71)tests/Webshop/CatalogControllerTest.php (2)
setUp(39-44)tearDown(46-50)tests/Traits/RequirePro.php (2)
requirePro(30-53)resetPro(55-61)
tests/Webshop/OrderManagement/OrderListingAccessTest.php (1)
tests/Traits/RequirePro.php (2)
requirePro(30-53)resetPro(55-61)
tests/Unit/Middleware/ConfigIntegrityTest.php (1)
app/Http/Middleware/ConfigIntegrity.php (1)
ConfigIntegrity(17-131)
tests/Constants/FreeVerifyier.php (4)
app/Contracts/Http/Requests/HasProStatusBoolean.php (1)
is_pro(13-13)app/Http/Requests/Traits/HasProStatusBooleanTrait.php (1)
is_pro(20-27)tests/Constants/ProVerifyier.php (2)
is_pro(32-35)is_signature(37-40)tests/Constants/SupporterVerifyier.php (2)
is_pro(32-35)is_signature(37-40)
tests/Constants/SupporterVerifyier.php (4)
app/Contracts/Http/Requests/HasProStatusBoolean.php (1)
is_pro(13-13)app/Http/Requests/Traits/HasProStatusBooleanTrait.php (1)
is_pro(20-27)tests/Constants/FreeVerifyier.php (2)
is_pro(32-35)is_signature(37-40)tests/Constants/ProVerifyier.php (2)
is_pro(32-35)is_signature(37-40)
⏰ 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). (20)
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- 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.3 - postgresql -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
🔇 Additional comments (22)
database/migrations/2025_12_15_161819_webshop_level_2.php (2)
23-26: LGTM!The down() method correctly reverts the level changes, maintaining proper symmetry with the up() method. This ensures clean rollback behavior.
15-18: Implementation is correct; config keys are guaranteed to exist through the migration system.The webshop_% config keys are created by earlier migrations (2025_09_20 and 2025_11_22) and are guaranteed to exist by the ConfigIntegrity middleware's SE_FIELDS and PRO_FIELDS definitions, which enforce architectural consistency. This migration correctly updates their access level to 2 (Pro Edition) in proper sequence.
composer.json (1)
66-66: Major lycheeverify version bump – ensure runtime/test verificationUpdating
lychee-org/lycheeverifyto^2.0.0is a major bump; the surrounding code changes look aligned with the new Status/Verify APIs, but please make sure the full test suite (especially config/webshop/license-related tests) passes against v2 and that no legacy PLUS/SE-only paths remain.tests/Constants/SupporterVerifyier.php (1)
32-40: Supporter verifier behavior consistent with new Pro/Signature statuses
is_pro()andis_signature()both returningfalseforSupporterVerifyier, whilecheck()only allowsFREE_EDITIONandSUPPORTER_EDITION, correctly models a supporter license that cannot access Pro/Signature features. Looks good.app/Http/Controllers/Admin/SettingsController.php (1)
45-52: Config level gating for Pro vs non‑Pro looks coherentThe additional
when(!$request->is_pro(), fn ($q) => $q->where('level', '<', 2))correctly withholds level‑2 configs from non‑Pro users while keeping the existing SE/preview logic intact. This matches the intended “level 2 == Pro” separation.app/Contracts/Http/Requests/HasProStatusBoolean.php (1)
1-14: New Pro-status request contract is minimal and well-formedThe
HasProStatusBooleaninterface cleanly declaresis_pro(): bool, follows the project’s licensing/header and PSR‑4 conventions, and aligns with the corresponding trait implementation.tests/Traits/RequireSupport.php (1)
21-41: Pro helper in RequireSupport trait is consistent with existing helpersAdding
use Tests\Constants\ProVerifyier;andgetPro(): VerifyInterfaceparallelsgetFree()/getSupporter()and gives tests an easy way to obtain a Pro verifier without touching global state. No issues.tests/Webshop/Purchasables/ShopManagementControllerTest.php (1)
21-50: Webshop management tests now correctly assume Pro editionSwitching to
RequirePro, and usingrequirePro()/resetPro()insetUp/tearDown, brings this test in line with thesupport:promiddleware guarding the Shop Management endpoints. This keeps test assumptions consistent with the new licensing model.tests/Constants/FreeVerifyier.php (1)
32-39: Free verifier additions are consistent with edition semanticsAdding
is_pro()andis_signature()returningfalsealigns with this class representing the free edition and should satisfy the updatedVerifyInterfacewithout changing existing test behavior.tests/Unit/Middleware/ConfigIntegrityTest.php (1)
40-56: Solid SE/PRO integrity coverage in testsThe added checks for level‑1 SE keys vs
SE_FIELDSand level‑2 PRO keys vsPRO_FIELDS, plus the mirrored existence checks intestKeys(), give good safety around config levels and constants. Using a shared$failedflag with a finalassertFalsekeeps the test readable and robust.Also applies to: 72-79
app/Http/Requests/Traits/HasProStatusBooleanTrait.php (1)
1-28: Pro-status trait is consistent and efficiently memoizedThe trait follows existing request-trait patterns (license header, namespace, docblock) and the
is_pro()implementation correctly memoizes the verification result using a nullableboolsentinel, avoiding repeated external checks.tests/Webshop/BasketControllerTest.php (1)
28-29: Webshop basket tests correctly switched to Pro prerequisitesUsing
RequireProin the class and callingrequirePro()/resetPro()insetUp()/tearDown()aligns the basket tests with the new Pro‑gated webshop behavior while preserving the existing test flow.Also applies to: 44-45, 53-54, 69-71
tests/Webshop/OrderManagement/FlushOldOrdersTest.php (1)
26-27: Flush-old-orders tests now aligned with Pro edition setupSwitching from
RequireSEtoRequireProand updatingsetUp()/tearDown()keeps the maintenance tests consistent with the Pro‑based verification model, without affecting the core assertions around old order flushing.Also applies to: 39-40, 46-47, 57-58
app/Http/Requests/Settings/GetAllConfigsRequest.php (1)
11-15: Adding Pro-status capability to GetAllConfigsRequest is clean and non-intrusiveImplementing
HasProStatusBooleanand pulling inHasProStatusBooleanTraitalongside the existing SE counterparts cleanly exposesis_pro()on this request without altering authorization or validation behavior.Also applies to: 20-23
tests/Webshop/OrderManagement/OrderListingAccessTest.php (1)
26-27: Order listing tests correctly migrated to Pro edition setupUsing
RequireProand the correspondingrequirePro()/resetPro()hooks brings these order listing/access tests in line with the new Pro‑edition gating while leaving the actual access-control assertions unchanged.Also applies to: 39-40, 50-51, 85-86
app/Http/Middleware/ConfigIntegrity.php (2)
96-110: LGTM! PRO_FIELDS constant follows SE_FIELDS pattern.The new PRO_FIELDS constant is well-structured and follows the same pattern as SE_FIELDS. All configuration keys are consistently named with the
webshop_prefix and use snake_case naming convention as per coding guidelines.Based on learnings, configuration keys added to PRO_FIELDS are guaranteed to exist and contain valid values, similar to SE_FIELDS.
120-130: LGTM! PRO_FIELDS level enforcement implemented correctly.The handle() method now correctly enforces two-tier configuration protection:
- SE_FIELDS: level = 1
- PRO_FIELDS: level = 2
The implementation follows the same pattern as SE_FIELDS and is properly wrapped in exception handling for pre-installation scenarios.
tests/Webshop/Checkout/BaseCheckoutControllerTest.php (1)
27-27: LGTM! Test trait updated from RequireSE to RequirePro.This is a straightforward mechanical refactor to switch test prerequisites from SE (Support Edition) to PRO (Pro Edition) status. The changes are consistent:
- Import updated to
Tests\Traits\RequirePro- Trait usage updated
- Setup calls
requirePro()- Teardown calls
resetPro()The test logic remains unchanged, only the edition prerequisite has been updated.
Also applies to: 42-42, 73-73, 78-78
tests/Traits/RequirePro.php (1)
1-62: LGTM! Well-designed test trait for PRO status simulation.This trait provides clean test helpers for simulating PRO edition status:
requirePro(): Creates a fake Verify instance that avoids DB queries and returns the specified status (defaults to PRO_EDITION). The anonymous class override approach is appropriate for test doubles.resetPro(): Restores the real Verify instance for cleanup.The implementation follows good testing practices:
- Constructor bypass prevents DB queries during tests
- Flexible status injection enables testing different edition scenarios
- Proper container binding/unbinding in setup/teardown
tests/Webshop/CatalogControllerTest.php (1)
23-23: LGTM! Test trait updated from RequireSE to RequirePro.This is a straightforward mechanical refactor switching test prerequisites from SE to PRO edition status. The changes mirror those in other test files:
- Import:
Tests\Traits\RequireSE→Tests\Traits\RequirePro- Trait usage updated
- Setup:
requireSe()→requirePro()- Teardown:
resetSe()→resetPro()Test assertions and logic remain unchanged.
Also applies to: 37-37, 43-43, 48-48
tests/Webshop/Checkout/CheckoutControllerTest.php (1)
22-22: LGTM! Test trait updated from RequireSE to RequirePro.Another mechanical refactor switching from SE to PRO edition prerequisites. Changes are consistent with the pattern across all test files:
- Import:
Tests\Traits\RequireSE→Tests\Traits\RequirePro- Trait usage updated
- Setup:
requireSe()→requirePro()- Teardown:
resetSe()→resetPro()Test logic and assertions remain unchanged.
Also applies to: 29-29, 34-34, 39-39
tests/Constants/ProVerifyier.php (1)
15-60: LGTM! Well-implemented test double for PRO edition status.This test verifier correctly implements the VerifyInterface contract for PRO edition:
get_status(): Returns PRO_EDITIONcheck(): Correctly allows FREE, SUPPORTER, and PRO editions (PRO users have access to all lower tier features)is_supporter()/is_pro(): Return true as expected for PRO tieris_signature(): Returns false (PRO is below SIGNATURE tier)authorize(): Properly throws SupporterOnlyOperationException when check failswhen(): Correctly evaluates both callable and non-callable valuesvalidate(): Returns true for test scenariosThe implementation is consistent with similar test verifiers (FreeVerifyier, SupporterVerifyier) as shown in related code snippets.
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Actions/Diagnostics/Pipes/Checks/OldLicenseCheck.php (1)
48-48: Consider using a translation key for the error message.The error message is hard-coded in English, while similar messages in the UI layer use translation keys (e.g.,
dialogs.register.expired_license). For consistency with the localization approach used elsewhere, consider using__('dialogs.register.expired_license')or creating a dedicated diagnostic message translation key.Apply this diff to use the existing translation key:
- $data[] = DiagnosticData::error('Your license has expired. Go to keygen.lycheeorg.dev to retrieve a new one or erase the value in the license field.', self::class); + $data[] = DiagnosticData::error(__('dialogs.register.expired_license'), self::class);Note: If the translation key contains HTML, you may need to strip tags or create a separate plain-text diagnostic message key.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
app/Actions/Diagnostics/Errors.php(2 hunks)app/Actions/Diagnostics/Pipes/Checks/OldLicenseCheck.php(1 hunks)app/Actions/Diagnostics/Pipes/Checks/WebshopCheck.php(3 hunks)app/Http/Resources/GalleryConfigs/InitConfig.php(3 hunks)lang/ar/dialogs.php(1 hunks)lang/cz/dialogs.php(1 hunks)lang/de/dialogs.php(1 hunks)lang/el/dialogs.php(1 hunks)lang/en/dialogs.php(1 hunks)lang/es/dialogs.php(1 hunks)lang/fa/dialogs.php(1 hunks)lang/fr/dialogs.php(1 hunks)lang/hu/dialogs.php(1 hunks)lang/it/dialogs.php(1 hunks)lang/ja/dialogs.php(1 hunks)lang/nl/dialogs.php(1 hunks)lang/no/dialogs.php(1 hunks)lang/pl/dialogs.php(1 hunks)lang/pt/dialogs.php(1 hunks)lang/ru/dialogs.php(1 hunks)lang/sk/dialogs.php(1 hunks)lang/sv/dialogs.php(1 hunks)lang/vi/dialogs.php(1 hunks)lang/zh_CN/dialogs.php(1 hunks)resources/js/components/settings/General.vue(3 hunks)resources/js/lychee.d.ts(1 hunks)resources/js/stores/LycheeState.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/es/dialogs.phplang/de/dialogs.phplang/fa/dialogs.phpapp/Http/Resources/GalleryConfigs/InitConfig.phplang/pl/dialogs.phplang/ja/dialogs.phpapp/Actions/Diagnostics/Pipes/Checks/OldLicenseCheck.phpapp/Actions/Diagnostics/Pipes/Checks/WebshopCheck.phplang/cz/dialogs.phpapp/Actions/Diagnostics/Errors.phplang/zh_CN/dialogs.phplang/no/dialogs.phplang/ru/dialogs.phplang/vi/dialogs.phplang/en/dialogs.phplang/el/dialogs.phplang/it/dialogs.phplang/nl/dialogs.phplang/sk/dialogs.phplang/sv/dialogs.phplang/fr/dialogs.phplang/pt/dialogs.phplang/hu/dialogs.phplang/ar/dialogs.php
**/Http/Resources/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Resource classes should extend from Spatie Data instead of JsonResource
Files:
app/Http/Resources/GalleryConfigs/InitConfig.php
**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function functionName() {} syntax instead of const function = () => {}
Files:
resources/js/stores/LycheeState.tsresources/js/components/settings/General.vueresources/js/lychee.d.ts
**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.vue: Use TypeScript in composition API for Vue3 and use PrimeVue for UI components
Do not use await async calls in Vue3; use .then() instead
In Vue3 components, the order should be: , then <script lang="ts">, then <style>Files:
resources/js/components/settings/General.vue🧠 Learnings (6)
📚 Learning: 2025-08-20T20:35:04.474Z
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.Applied to files:
lang/nl/dialogs.php📚 Learning: 2025-08-22T06:11:18.329Z
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.Applied to files:
lang/hu/dialogs.php📚 Learning: 2025-09-28T08:46:37.299Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/stores/UserState.ts:7-11 Timestamp: 2025-09-28T08:46:37.299Z Learning: In Lychee codebase, the type `App.Enum.OauthProvidersType` is properly defined within the `App.Enum` namespace in `resources/js/lychee.d.ts` file. References to `App.Enum.OauthProvidersType` in TypeScript files are valid and resolve correctly.Applied to files:
resources/js/lychee.d.ts📚 Learning: 2025-09-28T08:39:34.280Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/stores/NsfwConsentedState.ts:3-5 Timestamp: 2025-09-28T08:39:34.280Z Learning: In Lychee codebase, all Pinia store files follow the pattern of declaring the type alias before the store definition (e.g., `export type StoreType = ReturnType<typeof useStore>; export const useStore = defineStore(...)`). This pattern compiles successfully with TypeScript 5.9 and vue-tsc.Applied to files:
resources/js/lychee.d.ts📚 Learning: 2025-09-28T12:48:30.559Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/views/gallery-panels/Timeline.vue:392-0 Timestamp: 2025-09-28T12:48:30.559Z Learning: In the Lychee frontend, delete operations at the root/timeline level are gated by `rootRights.can_edit` rather than a separate `can_delete` permission. The `RootAlbumRightsResource` only exposes `can_edit`, `can_upload`, `can_see_live_metrics`, and `can_import_from_server` properties. Album-level rights have `can_delete`, but timeline/root contexts use `can_edit` for delete permissions.Applied to files:
resources/js/lychee.d.ts📚 Learning: 2025-09-28T12:04:53.277Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/stores/TagState.ts:34-36 Timestamp: 2025-09-28T12:04:53.277Z Learning: In the Lychee codebase, different API services have different response structures. TagsService.get() returns TagWithPhotosResource directly with fields id, name, and photos (no "resource" wrapper field), while AlbumService.get() returns a response with data.resource that can be null. Always check the actual TypeScript definitions before assuming response structure.Applied to files:
resources/js/lychee.d.ts🧬 Code graph analysis (3)
app/Http/Resources/GalleryConfigs/InitConfig.php (1)
app/Models/Configs.php (2)
Configs(65-369)getValueAsString(256-259)app/Actions/Diagnostics/Pipes/Checks/OldLicenseCheck.php (2)
app/DTO/DiagnosticData.php (2)
DiagnosticData(13-72)error(40-43)app/Models/Configs.php (2)
Configs(65-369)getValueAsString(256-259)app/Actions/Diagnostics/Errors.php (1)
app/Actions/Diagnostics/Pipes/Checks/OldLicenseCheck.php (1)
OldLicenseCheck(21-52)🪛 GitHub Check: 2️⃣ PHP 8.3 - PHPStan
app/Http/Resources/GalleryConfigs/InitConfig.php
[failure] 230-230:
Only booleans are allowed in &&, string given on the left side.⏰ 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)
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
🔇 Additional comments (30)
lang/vi/dialogs.php (1)
74-74: Missing Vietnamese translation.This is a Vietnamese translation file (
lang/vi/), but theexpired_licenseentry contains English text instead of Vietnamese. Vietnamese-speaking users will see English text in their interface, degrading the user experience.Please provide a proper Vietnamese translation for this message, for example:
- 'expired_license' => 'Your license key has expired. Empty the field or get a new one at <a href="https://keygen.lycheeorg.dev" class="text-primary-500 underline">keygen.lycheeorg.dev</a>.', + 'expired_license' => 'Khóa giấy phép của bạn đã hết hạn. Để trống trường này hoặc lấy khóa mới tại <a href="https://keygen.lycheeorg.dev" class="text-primary-500 underline">keygen.lycheeorg.dev</a>.',Note: The translation above is a suggestion. Please verify with a native Vietnamese speaker or professional translator for 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: 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/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.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: 0 File: :0-0 Timestamp: 2025-11-26T21:50:26.687Z Learning: In the LycheeOrg/Lychee repository, lang/* files should not be reviewed at all. These files should be completely excluded from code review.lang/sv/dialogs.php (1)
74-74: LGTM! Translation placeholder added correctly.The expired_license key has been added with proper formatting. Based on learnings, English strings in non-English locale files are expected as placeholders for Weblate translation workflow.
lang/it/dialogs.php (1)
74-74: LGTM! Translation placeholder added correctly.The expired_license key follows proper formatting and aligns with the broader multilingual support for license expiry messaging.
lang/nl/dialogs.php (1)
74-74: LGTM! Translation placeholder added correctly.The expired_license key has been added with proper formatting. Based on learnings, English strings in non-English locale files are expected as placeholders for the Weblate translation workflow.
app/Actions/Diagnostics/Pipes/Checks/WebshopCheck.php (2)
18-19: LGTM! Dependency injection for PRO edition verification added correctly.The LycheeVerify integration follows proper dependency injection patterns and the constructor signature is updated appropriately.
Also applies to: 29-29
42-45: Status::PRO_EDITION constant is confirmed to be available in lycheeverify ^2.0.0.The code correctly uses strict comparison and the constant is used consistently throughout the codebase. No issues found.
lang/el/dialogs.php (1)
74-74: LGTM! Translation placeholder added correctly.The expired_license key has been added with proper formatting and aligns with the multilingual license expiry support.
lang/ja/dialogs.php (1)
74-74: LGTM! Translation placeholder added correctly.The expired_license key has been added with proper formatting, completing the multilingual support for license expiry messaging.
lang/ru/dialogs.php (1)
73-73: Fix formatting: separate array keys onto individual lines.Multiple array keys are incorrectly placed on a single line, which violates PHP array formatting standards and impairs readability.
Apply this diff to fix the formatting:
- 'invalid_license' => 'Неверный лицензионный ключ.', 'expired_license' => 'Your license key has expired. Empty the field or get a new one at <a href="https://keygen.lycheeorg.dev" class="text-primary-500 underline">keygen.lycheeorg.dev</a>.', 'register' => 'Зарегистрировать', + 'invalid_license' => 'Неверный лицензионный ключ.', + 'expired_license' => 'Your license key has expired. Empty the field or get a new one at <a href="https://keygen.lycheeorg.dev" class="text-primary-500 underline">keygen.lycheeorg.dev</a>.', + 'register' => 'Зарегистрировать',⛔ 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.app/Actions/Diagnostics/Pipes/Checks/OldLicenseCheck.php (1)
30-51: LGTM!The diagnostic logic is sound:
- Skips check when configs table doesn't exist (install/migration scenario)
- Skips when no license is set
- Only reports an error when a license exists but Verify status is FREE_EDITION (indicating expiration)
The implementation correctly uses strict comparisons and follows PSR-4 standards.
app/Actions/Diagnostics/Errors.php (1)
29-29: LGTM!The OldLicenseCheck is correctly integrated into the diagnostic pipeline. The placement after ConfigSanityCheck and before DBSupportCheck is logical, ensuring license validation occurs early in the diagnostic flow.
Also applies to: 56-56
resources/js/lychee.d.ts (1)
841-841: LGTM!The new type fields
is_lycheeorg_disclaimer_enabledandcomplete_urlcorrectly extend the shop resource types to support the enhanced checkout flow.Also applies to: 847-847
lang/zh_CN/dialogs.php (1)
74-74: English text in Chinese locale file.The
expired_licensetranslation key contains English text in a Simplified Chinese (zh_CN) locale file. Users with Chinese locale will see an English message. Consider translating to Chinese for consistency.⛔ Skipped due to learnings
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.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.lang/cz/dialogs.php (1)
74-74: English text in Czech locale file.The
expired_licensetranslation key contains English text in a Czech locale file. Users with Czech locale will see an English message. Consider translating to Czech for consistency.⛔ Skipped due to learnings
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/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.lang/de/dialogs.php (1)
74-74: English text in German locale file.The
expired_licensetranslation key contains English text in a German locale file. Users with German locale will see an English message. Consider translating to German for consistency.Example German translation:
'expired_license' => 'Ihr Lizenzschlüssel ist abgelaufen. Leeren Sie das Feld oder holen Sie sich einen neuen unter <a href="https://keygen.lycheeorg.dev" class="text-primary-500 underline">keygen.lycheeorg.dev</a>.',⛔ Skipped due to learnings
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.lang/pt/dialogs.php (1)
74-74: English text in Portuguese locale file.The
expired_licensetranslation key contains English text in a Portuguese locale file. While this may be intentional for initial rollout, users with Portuguese locale will see an English message. Consider translating to Portuguese for consistency with other localized strings in this file.Example Portuguese translation:
'expired_license' => 'Sua chave de licença expirou. Esvazie o campo ou obtenha uma nova em <a href="https://keygen.lycheeorg.dev" class="text-primary-500 underline">keygen.lycheeorg.dev</a>.',⛔ Skipped due to learnings
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/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/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.lang/pl/dialogs.php (1)
74-74: English text in Polish locale file.The
expired_licensetranslation key contains English text in a Polish locale file. Users with Polish locale will see an English message. Consider translating to Polish for consistency.⛔ Skipped due to learnings
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.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.lang/hu/dialogs.php (1)
74-74: LGTM!The PHP structure and syntax are correct for this translation entry.
Based on learnings, reviewing only PHP-related issues for translation files, not translation content.
lang/es/dialogs.php (1)
74-74: LGTM!The PHP structure and syntax are correct for this translation entry.
Based on learnings, reviewing only PHP-related issues for translation files, not translation content.
lang/sk/dialogs.php (1)
74-74: LGTM!The PHP structure and syntax are correct for this translation entry.
Based on learnings, reviewing only PHP-related issues for translation files, not translation content.
lang/fa/dialogs.php (1)
74-74: LGTM!The PHP structure and syntax are correct for this translation entry.
Based on learnings, reviewing only PHP-related issues for translation files, not translation content.
lang/fr/dialogs.php (1)
74-74: LGTM!The PHP structure and syntax are correct for this translation entry.
Based on learnings, reviewing only PHP-related issues for translation files, not translation content.
lang/ar/dialogs.php (1)
74-74: LGTM!The PHP structure and syntax are correct for this translation entry.
Based on learnings, reviewing only PHP-related issues for translation files, not translation content.
lang/en/dialogs.php (1)
75-75: LGTM!The PHP structure and syntax are correct. The translation key is properly added to support license expiration messaging in the UI.
resources/js/stores/LycheeState.ts (2)
75-75: LGTM!The new state properties
is_pro_enabledandis_se_expiredare correctly initialized and follow the existing pattern in the store.Also applies to: 78-78
148-148: LGTM!The data population for
is_pro_enabledandis_se_expiredis consistent with how other properties are populated from the response data in theload()action.Also applies to: 151-151
app/Http/Resources/GalleryConfigs/InitConfig.php (1)
214-220: LGTM! Pro status properly gated.The logic correctly ensures that
is_pro_enabledis true only when both SE is enabled and the user has Pro status, which provides appropriate layered gating.resources/js/components/settings/General.vue (3)
25-33: LGTM! Expired license handling properly integrated.The Fieldset visibility logic correctly ensures that SE settings are shown when a license has expired, even if the user previously hid the call-to-action. The expired license message provides clear feedback to users.
299-299: LGTM! Store ref properly extracted.The
is_se_expiredref is correctly extracted from the Lychee state store usingstoreToRefs, consistent with the other SE-related properties.
378-382: LGTM! License expiration properly cleared on successful registration.The registration success handler correctly resets
is_se_expiredtofalsealong with updating other SE-related state, ensuring the UI reflects the newly valid license.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
resources/js/components/settings/General.vue (1)
27-29: Consider documenting the PrimeVue version affected.The comment notes a PrimeVue issue but doesn't specify which version is affected. Consider adding the version number or a reference to help future maintainers determine when this workaround can be removed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Http/Resources/GalleryConfigs/InitConfig.php(3 hunks)lang/no/dialogs.php(1 hunks)lang/ru/dialogs.php(1 hunks)lang/zh_TW/dialogs.php(1 hunks)resources/js/components/settings/General.vue(3 hunks)resources/js/lychee.d.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/js/lychee.d.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/ru/dialogs.phplang/zh_TW/dialogs.phplang/no/dialogs.phpapp/Http/Resources/GalleryConfigs/InitConfig.php
**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.vue: Use TypeScript in composition API for Vue3 and use PrimeVue for UI components
Do not use await async calls in Vue3; use .then() instead
In Vue3 components, the order should be: , then <script lang="ts">, then <style>Files:
resources/js/components/settings/General.vue**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function functionName() {} syntax instead of const function = () => {}
Files:
resources/js/components/settings/General.vue**/Http/Resources/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Resource classes should extend from Spatie Data instead of JsonResource
Files:
app/Http/Resources/GalleryConfigs/InitConfig.php🧠 Learnings (3)
📚 Learning: 2025-08-22T06:11:18.329Z
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.Applied to files:
lang/no/dialogs.php📚 Learning: 2025-11-25T21:26:03.890Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-25T21:26:03.890Z Learning: Applies to **/*.php : Only use booleans in if statements, not integers or stringsApplied to files:
app/Http/Resources/GalleryConfigs/InitConfig.php📚 Learning: 2025-11-25T21:26:03.890Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-25T21:26:03.890Z Learning: Applies to **/*.php : Use strict comparison (===) instead of loose comparison (==)Applied to files:
app/Http/Resources/GalleryConfigs/InitConfig.php🧬 Code graph analysis (1)
app/Http/Resources/GalleryConfigs/InitConfig.php (1)
app/Models/Configs.php (2)
Configs(65-369)getValueAsString(256-259)⏰ 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). (19)
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
🔇 Additional comments (7)
lang/no/dialogs.php (1)
74-74: LGTM!The PHP syntax and array structure are correct. The formatting properly places each array key on its own line, consistent with the rest of the file.
Based on learnings, only PHP-related issues are reviewed for translation files in this project.
app/Http/Resources/GalleryConfigs/InitConfig.php (3)
88-88: LGTM! New properties follow conventions.The new
is_pro_enabledandis_se_expiredboolean properties are properly typed and follow snake_case naming conventions.Also applies to: 94-94
216-216: LGTM! Pro status logic is correctly implemented.The code properly retrieves the Pro status from the Verify service and combines it with the SE enabled flag to determine
is_pro_enabled. The logic ensures Pro features are only enabled when both SE is enabled and the Pro tier is active.Also applies to: 220-220
230-230: LGTM! Previous issue resolved.The explicit boolean comparison
!== ''properly addresses the coding guideline requiring only booleans in conditional expressions. The logic correctly identifies an expired license when a license key exists but SE is not enabled.resources/js/components/settings/General.vue (3)
25-25: LGTM! Expired license visibility logic is correct.The updated condition properly ensures the SE fieldset displays when a license has expired, even if the user previously chose to hide SE promotional content. This provides important visibility for license expiration.
Note: The
legend="Lychee SE"attribute appears redundant given the template slot override below, but this may be intentional as a fallback.
31-31: LGTM! Expired license warning is properly placed.The conditional paragraph effectively alerts users to license expiration with appropriate styling and prominent placement at the top of the SE section.
297-297: LGTM! Store integration is properly implemented.The
is_se_expiredstate is correctly:
- Destructured from the Pinia store using
storeToRefsfor reactivity (line 297)- Reset to
falseon successful license registration (line 378)The registration flow properly updates all related SE state flags.
Also applies to: 378-378
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php (1)
56-74: Consider data safety in down() migration.The
down()method makesphoto_idnon-nullable again (line 65), which could fail if there are already size variants withphoto_id = null(i.e., if photos were deleted after this migration ran). Consider adding a check or cleanup step to handle existing null values before attempting to restore the non-nullable constraint.🔎 Suggested approach to handle existing null values:
public function down(): void { // Reverse size_variants foreign key changes Schema::table('size_variants', function (Blueprint $table) { $table->dropForeign(['photo_id']); }); + // Clean up orphaned size variants before making photo_id non-nullable + DB::table('size_variants')->whereNull('photo_id')->delete(); + // Make photo_id non-nullable again Schema::table('size_variants', function (Blueprint $table) { $table->char(self::PHOTO_ID, self::RANDOM_ID_LENGTH)->nullable(false)->change(); }); // Re-add original foreign key without onDelete Schema::table('size_variants', function (Blueprint $table) { $table->foreign(self::PHOTO_ID) ->references('id') ->on('photos'); }); }Note: This would delete orphaned size variants, which may not be desirable if they're still referenced by order_items. Based on learnings, this might be intentionally destructive.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Actions/Photo/Delete.php(3 hunks)app/Models/SizeVariant.php(1 hunks)database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php(1 hunks)tests/Webshop/DeletePurchasedPhotoTest.php(1 hunks)
🧰 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
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:
app/Models/SizeVariant.phpapp/Actions/Photo/Delete.phptests/Webshop/DeletePurchasedPhotoTest.phpdatabase/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Webshop/DeletePurchasedPhotoTest.php
🧠 Learnings (10)
📚 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/Delete.phptests/Webshop/DeletePurchasedPhotoTest.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/Actions/Photo/Delete.phptests/Webshop/DeletePurchasedPhotoTest.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:
app/Actions/Photo/Delete.phpdatabase/migrations/2025_12_18_101617_preserve_size_variants_for_orders.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/Actions/Photo/Delete.phptests/Webshop/DeletePurchasedPhotoTest.php
📚 Learning: 2025-09-18T13:37:32.687Z
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.
Applied to files:
app/Actions/Photo/Delete.phptests/Webshop/DeletePurchasedPhotoTest.php
📚 Learning: 2025-09-13T11:30:21.765Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php:237-239
Timestamp: 2025-09-13T11:30:21.765Z
Learning: In tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php, directly modifying the shared $this->test_order fixture in loops is an intentional design choice, not something that needs to be refactored to create fresh orders for each iteration.
Applied to files:
tests/Webshop/DeletePurchasedPhotoTest.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/Webshop/DeletePurchasedPhotoTest.php
📚 Learning: 2025-08-20T20:32:03.676Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:106-108
Timestamp: 2025-08-20T20:32:03.676Z
Learning: In the Lychee codebase, ildyria intentionally uses destructive migration patterns where up() calls down() to ensure clean state, as indicated by "Working as intended" and "No mercy" comments in migrations like database/migrations/2025_08_19_160144_create_renamer_rules_table.php.
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
📚 Learning: 2025-08-24T14:33:37.747Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3644
File: database/migrations/2025_08_24_122020_fix_timeline_config.php:25-29
Timestamp: 2025-08-24T14:33:37.747Z
Learning: In the Lychee codebase, ildyria prefers not to add null checks in migrations when they can guarantee that database keys exist, as indicated by their "Key exists, don't worry about it" response to null-safety suggestions in database/migrations/2025_08_24_122020_fix_timeline_config.php.
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
📚 Learning: 2025-08-20T20:32:36.930Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:133-134
Timestamp: 2025-08-20T20:32:36.930Z
Learning: In the Lychee codebase, ildyria intentionally designs migrations to be destructive where up() calls down() first to ensure a clean state before inserting data. This approach makes migrations idempotent through destruction and recreation rather than through upsert operations, as confirmed by the "Working as intended" response to suggestions about using insertOrIgnore for config seeding.
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
🧬 Code graph analysis (1)
tests/Webshop/DeletePurchasedPhotoTest.php (5)
app/Models/OrderItem.php (1)
OrderItem(58-190)tests/Feature_v2/Base/BaseApiWithDataTest.php (1)
BaseApiWithDataTest(93-257)app/Models/Photo.php (1)
size_variants(207-210)app/Models/Extensions/SizeVariants.php (1)
getOriginal(138-141)tests/Traits/CatchFailures.php (1)
assertNoContent(133-136)
🔇 Additional comments (7)
app/Models/SizeVariant.php (1)
37-38: LGTM! PHPDoc correctly updated to reflect nullable types.The documentation changes accurately reflect the migration that makes
photo_idnullable, allowing size variants to persist after their parent photo is deleted (withphoto_idset to null).app/Actions/Photo/Delete.php (2)
233-236: LGTM! Size variants in orders are correctly excluded from file deletion.The
leftJoinwithorder_itemsand subsequentwhereNull('oi.id')filter correctly prevents collection of size variants that are referenced by order items, ensuring purchased content remains accessible to customers.
314-318: LGTM! DB deletion correctly safeguards size variants in orders.The
whereNotExistssubqueries on both deletion paths (by photo and by album) ensure size variants referenced inorder_itemsare preserved, maintaining referential integrity for purchased content.Also applies to: 330-334
tests/Webshop/DeletePurchasedPhotoTest.php (4)
42-65: LGTM! Test setup correctly configured for Pro edition.The test class properly uses the
RequireProtrait and sets up necessary test data with appropriate factory methods. The setup and teardown methods ensure clean test state.
83-128: LGTM! Test setup and execution are correctly implemented.The test properly:
- Loads size variants and creates a complete order with associated order item
- Verifies initial database state before deletion
- Executes deletion as admin with proper authorization
- Uses appropriate factory methods and assertions
130-157: LGTM! Assertions correctly verify preservation of order integrity.The test properly verifies that after photo deletion:
- The photo is removed from the database
- The order remains CLOSED and accessible
- The order item maintains its size_variant_id reference
- The size variant persists in the database
- Content remains accessible via content_url
This validates the core functionality of preserving purchased content.
175-253: LGTM! Comprehensive test coverage for album deletion scenario.The test effectively verifies that deleting an album (which cascades to delete photos) still preserves:
- Order status and accessibility
- Order item references
- Size variant data
- Content accessibility via content_url
The parallel structure with
testDeletePurchasedPhotoprovides thorough coverage of both deletion paths.
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Http/Resources/Rights/AlbumRightsResource.php (2)
32-32: Pre-existing typo in property name.The property name
can_pasword_protecthas a typo (should becan_password_protect). While this is not introduced by this PR, it should be corrected in a future change. Note that fixing this will require coordinating updates to the frontend TypeScript types and any code referencing this property.
63-83: Verify the permission upgrade and its breaking change impact.The change from
is_supporter()tois_pro()at line 78 upgrades the permission level required to make albums purchasable. Theis_pro()method exists in the LycheeVerify package and is properly implemented. The logic structure is sound with correct guard clauses and aligns with the system's permission hierarchy where pro is a stricter requirement than supporter status.This represents a confirmed breaking change: existing supporter-level users without pro status will lose the ability to make albums purchasable. Ensure this behavior change is intentional and that existing supporters are appropriately handled (migrated, notified, or policy is acceptable as-is).
🧹 Nitpick comments (8)
app/Actions/Shop/Gateway/CaptureFailedResponse.php (1)
11-13: Consider adding a docblock to clarify the class purpose.While the inheritance from
OrderFailedResponseis clear, a brief docblock would help explain when this specific response type is used versus the baseOrderFailedResponse.Suggested docblock
+/** + * Response for PayPal order capture failures. + * + * Used when an approved PayPal order fails during the capture/complete stage, + * as distinct from failures during initial order creation. + */ class CaptureFailedResponse extends OrderFailedResponse { }app/Actions/Shop/Gateway/CapturedResponse.php (1)
11-13: Consider adding a docblock to clarify the class purpose.While the inheritance from
OrderCreatedResponseis clear, a brief docblock would help explain when this specific response type is used versus the baseOrderCreatedResponse.Suggested docblock
+/** + * Response for successful PayPal order captures. + * + * Used when an approved PayPal order is successfully captured/completed, + * as distinct from the initial order creation response. + */ class CapturedResponse extends OrderCreatedResponse { }app/Actions/Shop/CheckoutService.php (1)
83-87: Naming inconsistency:$requestholds a response for PaypalGateway.For
PaypalGateway, thepurchase()method returns anOrderCreatedResponsedirectly (which already is the response), but the variable is named$request. The subsequent->send()call works becauseOrderCreatedResponse::send()returns$this, but this creates a confusing code path where$requestis actually a response object.Consider renaming to
$purchaseResultor adding a clarifying comment to indicate this dual-purpose behavior.app/Http/Controllers/Shop/CheckoutController.php (1)
147-155: Use$successvariable for consistency.Line 149 duplicates the status check from line 135. Consider using the already-computed
$successvariable for consistency and to avoid redundant comparisons.🔎 Proposed fix
if ($request->provider_type() === OmnipayProviderType::PAYPAL) { return new CheckoutResource( - is_success: $order->status === PaymentStatusType::COMPLETED, + is_success: $success, complete_url: $complete_url, redirect_url: $redirect_url, message: $message, order: OrderResource::fromModel($order), ); }app/Actions/Shop/Gateway/OrderFailedResponse.php (1)
17-24: Consider omitting empty parentheses whendebug_idis missing.When
debug_idis not present, the message ends with" ()"which looks awkward. You could conditionally include the debug ID portion.🔎 Proposed fix
public function __construct( array $details, ) { $error_details = $details['details'][0] ?? null; - $this->message = $error_details !== null - ? ($error_details['issue'] . ' ' . $error_details['description'] . ' (' . ($details['debug_id'] ?? '') . ')') : - ($details['error'] ?? 'Unknown error'); + if ($error_details !== null) { + $debug_suffix = isset($details['debug_id']) ? ' (' . $details['debug_id'] . ')' : ''; + $this->message = $error_details['issue'] . ' ' . $error_details['description'] . $debug_suffix; + } else { + $this->message = $details['error'] ?? 'Unknown error'; + } }Note: This would require updating the test expectation at line 162 in
OrderFailedResponseTest.php.tests/Webshop/Gateway/PaypalGatewayTest.php (1)
166-278: Reduce code duplication in mock setup.The mock setup for
OrderandOrderItemis repeated across multiple test methods. Consider extracting common setup logic into helper methods to improve maintainability and reduce duplication.Example approach
private function createMockOrder(int $amountCents, array $items = []): Order { $order = \Mockery::mock(Order::class)->makePartial(); $order->shouldAllowMockingProtectedMethods(); $order->amount_cents = new Money($amountCents, new Currency('USD')); $order->items = new Collection($items); return $order; } private function createMockOrderItem( string $title, int $priceCents, ?int $purchasableId, PurchasableSizeVariantType $sizeVariant, PurchasableLicenseType $licenseType ): OrderItem { $item = \Mockery::mock(OrderItem::class)->makePartial(); $item->shouldAllowMockingProtectedMethods(); $item->title = $title; $item->price_cents = new Money($priceCents, new Currency('USD')); $item->purchasable_id = $purchasableId; $item->size_variant_type = $sizeVariant; $item->license_type = $licenseType; return $item; }app/Actions/Shop/Gateway/OrderCreatedResponse.php (2)
20-23: Use a more specific exception type for unimplemented methods.Throwing generic
\Exceptionfor unimplemented interface methods reduces type safety and makes it harder for callers to handle these specific cases. Consider using a more specific exception type likeLogicExceptionor creating a customNotImplementedException.🔎 Proposed fix
public function getRequest() { - throw new \Exception('Not implemented'); + throw new \LogicException('Not implemented'); } // ... similar changes for getMessage(), getCode(), getData()Or create a custom exception:
// In app/Exceptions/NotImplementedException.php class NotImplementedException extends \LogicException { public function __construct(string $method) { parent::__construct("Method {$method} is not implemented"); } } // Then use: throw new NotImplementedException(__METHOD__);Also applies to: 40-43, 45-48, 55-58
13-63: Consider adding docblocks to explain interface contract.The class implements
ResponseInterfacebut leaves several methods unimplemented. Adding docblocks would clarify which methods are intentionally not implemented and why, improving maintainability.Example documentation
+ /** + * Get the original request. + * + * Not implemented for this response type. + * + * @throws \LogicException Always throws as this method is not implemented + */ public function getRequest() { throw new \Exception('Not implemented'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.env.example(1 hunks)app/Actions/Shop/CheckoutService.php(7 hunks)app/Actions/Shop/Gateway/CaptureFailedResponse.php(1 hunks)app/Actions/Shop/Gateway/CapturedResponse.php(1 hunks)app/Actions/Shop/Gateway/OrderCreatedResponse.php(1 hunks)app/Actions/Shop/Gateway/OrderFailedResponse.php(1 hunks)app/Actions/Shop/Gateway/PaypalGateway.php(1 hunks)app/Actions/Shop/Gateway/README.md(1 hunks)app/Enum/OmnipayProviderType.php(2 hunks)app/Factories/OmnipayFactory.php(2 hunks)app/Http/Controllers/Shop/CheckoutController.php(2 hunks)app/Http/Resources/Rights/AlbumRightsResource.php(1 hunks)app/Http/Resources/Rights/ModulesRightsResource.php(1 hunks)app/Http/Resources/Shop/CheckoutOptionResource.php(2 hunks)composer.json(1 hunks)config/omnipay.php(1 hunks)docs/backend/Shop-implementation.md(8 hunks)docs/backend/Shop-integration.md(3 hunks)tests/Webshop/Checkout/CheckoutCreateSessionControllerTest.php(1 hunks)tests/Webshop/Checkout/CheckoutOfflineControllerTest.php(0 hunks)tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.php(2 hunks)tests/Webshop/Gateway/CaptureFailedResponseTest.php(1 hunks)tests/Webshop/Gateway/CapturedResponseTest.php(1 hunks)tests/Webshop/Gateway/OrderCreatedResponseTest.php(1 hunks)tests/Webshop/Gateway/OrderFailedResponseTest.php(1 hunks)tests/Webshop/Gateway/PaypalGatewayTest.php(1 hunks)tests/Webshop/OrderManagement/OrderListingAccessTest.php(5 hunks)
💤 Files with no reviewable changes (1)
- tests/Webshop/Checkout/CheckoutOfflineControllerTest.php
✅ Files skipped from review due to trivial changes (1)
- app/Actions/Shop/Gateway/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Webshop/OrderManagement/OrderListingAccessTest.php
- composer.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.phpapp/Http/Resources/Shop/CheckoutOptionResource.phpapp/Actions/Shop/Gateway/CapturedResponse.phpapp/Actions/Shop/Gateway/CaptureFailedResponse.phptests/Webshop/Gateway/CapturedResponseTest.phpapp/Http/Controllers/Shop/CheckoutController.phpapp/Http/Resources/Rights/AlbumRightsResource.phptests/Webshop/Gateway/OrderFailedResponseTest.phptests/Webshop/Gateway/OrderCreatedResponseTest.phpapp/Actions/Shop/CheckoutService.phpapp/Enum/OmnipayProviderType.phpapp/Http/Resources/Rights/ModulesRightsResource.phptests/Webshop/Gateway/PaypalGatewayTest.phptests/Webshop/Gateway/CaptureFailedResponseTest.phpapp/Actions/Shop/Gateway/PaypalGateway.phpapp/Factories/OmnipayFactory.phpapp/Actions/Shop/Gateway/OrderCreatedResponse.phpconfig/omnipay.phptests/Webshop/Checkout/CheckoutCreateSessionControllerTest.phpapp/Actions/Shop/Gateway/OrderFailedResponse.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.phptests/Webshop/Gateway/CapturedResponseTest.phptests/Webshop/Gateway/OrderFailedResponseTest.phptests/Webshop/Gateway/OrderCreatedResponseTest.phptests/Webshop/Gateway/PaypalGatewayTest.phptests/Webshop/Gateway/CaptureFailedResponseTest.phptests/Webshop/Checkout/CheckoutCreateSessionControllerTest.php
**/Http/Resources/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Resource classes should extend from Spatie Data instead of JsonResource
Files:
app/Http/Resources/Shop/CheckoutOptionResource.phpapp/Http/Resources/Rights/AlbumRightsResource.phpapp/Http/Resources/Rights/ModulesRightsResource.php
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: Use Markdown format for documentation
At the bottom of documentation files, add a horizontal rule followed by "Last updated: [date of the update]"
Files:
docs/backend/Shop-integration.mddocs/backend/Shop-implementation.md
🧠 Learnings (14)
📚 Learning: 2025-09-13T11:30:21.765Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php:237-239
Timestamp: 2025-09-13T11:30:21.765Z
Learning: In tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php, directly modifying the shared $this->test_order fixture in loops is an intentional design choice, not something that needs to be refactored to create fresh orders for each iteration.
Applied to files:
tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.phptests/Webshop/Gateway/CapturedResponseTest.phptests/Webshop/Gateway/OrderFailedResponseTest.phptests/Webshop/Gateway/OrderCreatedResponseTest.phptests/Webshop/Gateway/PaypalGatewayTest.phptests/Webshop/Checkout/CheckoutCreateSessionControllerTest.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:
docs/backend/Shop-integration.mdapp/Http/Resources/Rights/AlbumRightsResource.phpdocs/backend/Shop-implementation.md
📚 Learning: 2025-11-25T21:26:03.890Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T21:26:03.890Z
Learning: Applies to **/*.md : At the bottom of documentation files, add a horizontal rule followed by "*Last updated: [date of the update]*"
Applied to files:
docs/backend/Shop-integration.md
📚 Learning: 2025-08-14T20:54:52.579Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3619
File: app/Rules/README.md:227-231
Timestamp: 2025-08-14T20:54:52.579Z
Learning: The user ildyria prefers to keep "Last updated" footers in italic format (*Last updated: [date]*) in documentation files, even if it triggers markdownlint MD036 warnings. This is a stylistic choice for the Lychee project.
Applied to files:
docs/backend/Shop-integration.md
📚 Learning: 2025-11-17T11:36:52.888Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Actions/Shop/CheckoutService.php:70-72
Timestamp: 2025-11-17T11:36:52.888Z
Learning: For Mollie and Stripe payment providers in the CheckoutService, the payment flow uses redirects rather than POST callbacks. When the user completes payment, they are redirected back to Lychee with query parameters in the URL (not POST data). The purchase parameters stored in the session are used with completePurchase() to verify the payment, and any necessary callback data comes from the query parameters in the redirect URL, which Omnipay handles automatically.
Applied to files:
app/Http/Controllers/Shop/CheckoutController.phpapp/Actions/Shop/Gateway/PaypalGateway.php
📚 Learning: 2025-09-28T12:48:30.559Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3721
File: resources/js/views/gallery-panels/Timeline.vue:392-0
Timestamp: 2025-09-28T12:48:30.559Z
Learning: In the Lychee frontend, delete operations at the root/timeline level are gated by `rootRights.can_edit` rather than a separate `can_delete` permission. The `RootAlbumRightsResource` only exposes `can_edit`, `can_upload`, `can_see_live_metrics`, and `can_import_from_server` properties. Album-level rights have `can_delete`, but timeline/root contexts use `can_edit` for delete permissions.
Applied to files:
app/Http/Resources/Rights/AlbumRightsResource.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/Webshop/Gateway/PaypalGatewayTest.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:
docs/backend/Shop-implementation.md
📚 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:
docs/backend/Shop-implementation.md
📚 Learning: 2025-09-18T13:37:32.687Z
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.
Applied to files:
docs/backend/Shop-implementation.md
📚 Learning: 2025-09-13T14:47:30.798Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: database/factories/PurchasableFactory.php:103-109
Timestamp: 2025-09-13T14:47:30.798Z
Learning: Photos and albums have a Many-Many relationship in Lychee, meaning a photo can belong to multiple albums. There is no single photo->album_id property. When creating purchasables for photos, both photo_id and album_id must be specified to indicate which photo-album combination the purchasable applies to.
Applied to files:
docs/backend/Shop-implementation.md
📚 Learning: 2025-09-21T20:09:31.762Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: database/migrations/2025_08_27_203030_create_purchasable.php:35-39
Timestamp: 2025-09-21T20:09:31.762Z
Learning: In the Lychee codebase, ildyria has decided to ignore the unique constraint issue with UNIQUE(album_id, photo_id) in the purchasables table where NULL values in MySQL/MariaDB don't enforce uniqueness as expected. This is acceptable for their multi-database support strategy (sqlite, pgsql, mariadb) and likely handled at the application level.
Applied to files:
docs/backend/Shop-implementation.md
📚 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:
docs/backend/Shop-implementation.md
📚 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(), the SQL query already includes ->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:
docs/backend/Shop-implementation.md
🧬 Code graph analysis (16)
tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.php (1)
resources/js/lychee.d.ts (1)
OmnipayProviderType(106-106)
app/Actions/Shop/Gateway/CapturedResponse.php (1)
app/Actions/Shop/Gateway/OrderCreatedResponse.php (1)
OrderCreatedResponse(13-64)
app/Actions/Shop/Gateway/CaptureFailedResponse.php (1)
app/Actions/Shop/Gateway/OrderFailedResponse.php (1)
OrderFailedResponse(13-70)
tests/Webshop/Gateway/CapturedResponseTest.php (2)
app/Actions/Shop/Gateway/CapturedResponse.php (1)
CapturedResponse(11-13)app/Actions/Shop/Gateway/OrderCreatedResponse.php (6)
OrderCreatedResponse(13-64)isSuccessful(25-28)isRedirect(30-33)isCancelled(35-38)getTransactionReference(50-53)send(60-63)
app/Http/Controllers/Shop/CheckoutController.php (3)
resources/js/lychee.d.ts (4)
OmnipayProviderType(106-106)CheckoutResource(911-918)PaymentStatusType(108-108)OrderResource(950-964)app/Http/Requests/Checkout/FinalizeRequest.php (3)
FinalizeRequest(29-83)basket(74-77)provider_type(79-82)app/Actions/Shop/CheckoutService.php (1)
handlePaymentReturn(180-207)
tests/Webshop/Gateway/OrderFailedResponseTest.php (1)
app/Actions/Shop/Gateway/OrderFailedResponse.php (10)
OrderFailedResponse(13-70)getMessage(46-49)isSuccessful(31-34)isRedirect(36-39)isCancelled(41-44)send(66-69)getRequest(26-29)getCode(51-54)getTransactionReference(56-59)getData(61-64)
tests/Webshop/Gateway/OrderCreatedResponseTest.php (1)
app/Actions/Shop/Gateway/OrderCreatedResponse.php (10)
OrderCreatedResponse(13-64)isSuccessful(25-28)isRedirect(30-33)isCancelled(35-38)getTransactionReference(50-53)send(60-63)getRequest(20-23)getMessage(40-43)getCode(45-48)getData(55-58)
app/Actions/Shop/CheckoutService.php (3)
app/Actions/Shop/Gateway/OrderCreatedResponse.php (1)
OrderCreatedResponse(13-64)app/Actions/Shop/Gateway/PaypalGateway.php (3)
PaypalGateway(70-417)purchase(280-309)getOrderDetails(201-240)app/Factories/OmnipayFactory.php (1)
create_gateway(29-39)
app/Enum/OmnipayProviderType.php (1)
resources/js/lychee.d.ts (1)
OmnipayProviderType(106-106)
tests/Webshop/Gateway/PaypalGatewayTest.php (6)
app/Actions/Shop/Gateway/CapturedResponse.php (1)
CapturedResponse(11-13)app/Actions/Shop/Gateway/CaptureFailedResponse.php (1)
CaptureFailedResponse(11-13)app/Actions/Shop/Gateway/OrderCreatedResponse.php (1)
OrderCreatedResponse(13-64)app/Actions/Shop/Gateway/OrderFailedResponse.php (1)
OrderFailedResponse(13-70)app/Actions/Shop/Gateway/PaypalGateway.php (3)
PaypalGateway(70-417)initialize(154-171)getOrderDetails(201-240)app/Exceptions/Internal/LycheeLogicException.php (1)
LycheeLogicException(13-19)
tests/Webshop/Gateway/CaptureFailedResponseTest.php (3)
app/Actions/Shop/Gateway/CaptureFailedResponse.php (1)
CaptureFailedResponse(11-13)app/Actions/Shop/Gateway/OrderFailedResponse.php (6)
OrderFailedResponse(13-70)getMessage(46-49)isSuccessful(31-34)isRedirect(36-39)isCancelled(41-44)send(66-69)app/Actions/Shop/Gateway/OrderCreatedResponse.php (5)
getMessage(40-43)isSuccessful(25-28)isRedirect(30-33)isCancelled(35-38)send(60-63)
app/Actions/Shop/Gateway/PaypalGateway.php (5)
app/Services/MoneyService.php (2)
MoneyService(22-98)toDecimal(91-97)app/Actions/Shop/Gateway/OrderCreatedResponse.php (3)
getCode(45-48)OrderCreatedResponse(13-64)getMessage(40-43)app/Actions/Shop/Gateway/OrderFailedResponse.php (3)
getCode(51-54)OrderFailedResponse(13-70)getMessage(46-49)app/Actions/Shop/Gateway/CapturedResponse.php (1)
CapturedResponse(11-13)app/Actions/Shop/Gateway/CaptureFailedResponse.php (1)
CaptureFailedResponse(11-13)
app/Factories/OmnipayFactory.php (2)
app/Actions/Shop/Gateway/PaypalGateway.php (1)
PaypalGateway(70-417)resources/js/lychee.d.ts (1)
OmnipayProviderType(106-106)
app/Actions/Shop/Gateway/OrderCreatedResponse.php (1)
app/Actions/Shop/Gateway/OrderFailedResponse.php (10)
__construct(17-24)getRequest(26-29)isSuccessful(31-34)isRedirect(36-39)isCancelled(41-44)getMessage(46-49)getCode(51-54)getTransactionReference(56-59)getData(61-64)send(66-69)
tests/Webshop/Checkout/CheckoutCreateSessionControllerTest.php (1)
resources/js/lychee.d.ts (1)
OmnipayProviderType(106-106)
app/Actions/Shop/Gateway/OrderFailedResponse.php (1)
app/Actions/Shop/Gateway/OrderCreatedResponse.php (10)
__construct(15-18)getRequest(20-23)isSuccessful(25-28)isRedirect(30-33)isCancelled(35-38)getMessage(40-43)getCode(45-48)getTransactionReference(50-53)getData(55-58)send(60-63)
🪛 LanguageTool
docs/backend/Shop-integration.md
[uncategorized] ~72-~72: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e variants ### Manual Fulfillment - FULL Size Variants: Require photographer to man...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (15)
app/Http/Resources/Rights/ModulesRightsResource.php (1)
174-185: The change correctly gates webshop behind Pro edition licensing. The differentiation from other methods is intentional:verify->check()validates any valid license (used for watermarker/renamer), whileverify->is_pro()specifically checks Pro edition (used for webshop and other Pro-only features throughout the codebase). Code follows all guidelines.app/Http/Resources/Shop/CheckoutOptionResource.php (1)
29-29: LGTM! PayPal client ID addition follows existing pattern.The new
paypal_client_idproperty and its initialization are consistent with the existing Mollie and Stripe configuration patterns.Also applies to: 46-46
docs/backend/Shop-implementation.md (1)
12-20: Excellent documentation of fulfillment system and data model updates.The comprehensive documentation of the Order Fulfillment System, including workflows, state transitions, and maintenance tasks, is clear and thorough. The data model clarifications for
album_idconstraints, size variant references, and fulfillment fields provide valuable context for developers working with the shop integration.Also applies to: 53-60, 131-148, 201-244
tests/Webshop/Checkout/CheckoutCreateSessionControllerTest.php (1)
228-256: LGTM! Provider update aligns with PayPal consolidation.The update from
OmnipayProviderType::PAYPAL_EXPRESStoOmnipayProviderType::PAYPALcorrectly reflects the provider enum simplification in this PR.tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.php (1)
154-189: LGTM! PayPal provider update and test expectations are correct.The changes correctly:
- Replace
PAYPAL_EXPRESSwithPAYPALto align with the provider consolidation- Update test expectations to treat
PAYPALas a functioning gateway (likeDUMMY) that returns 200 OKThe distinction between implemented providers (DUMMY, PAYPAL) returning 200 and unimplemented providers (STRIPE) returning 501 is appropriate.
app/Factories/OmnipayFactory.php (1)
11-11: LGTM! Clean routing of PayPal to custom gateway.The
matchexpression cleanly routes the PayPal provider to the customPaypalGatewayimplementation while maintaining the standard Omnipay flow for other providers. This approach is maintainable and follows modern PHP patterns.Also applies to: 31-34
.env.example (1)
262-264: LGTM! Environment configuration simplified for new PayPal integration.The cleanup removes deprecated PayPal API credentials (username/password/signature) in favor of the modern PayPal Server SDK approach using client ID and secret. This aligns with the new
PaypalGatewayimplementation.tests/Webshop/Gateway/CapturedResponseTest.php (1)
1-125: Well-structured test coverage for CapturedResponse.The tests comprehensively cover the
CapturedResponseclass behavior, including inheritance fromOrderCreatedResponse, status methods, transaction reference handling, and method chaining. The test structure follows established conventions.tests/Webshop/Gateway/OrderCreatedResponseTest.php (1)
1-164: Comprehensive test coverage for OrderCreatedResponse.The tests thoroughly validate the
OrderCreatedResponseclass including:
- Successful order creation behavior (isSuccessful, isRedirect, isCancelled)
- Transaction reference handling
- Method chaining via
send()- Exception behavior for unimplemented interface methods
app/Actions/Shop/CheckoutService.php (2)
221-224: Verify:return_urlandcancel_urlare unused for PayPal.For
PaypalGateway, the method short-circuits and callsgetOrderDetails($order), ignoring the$return_urland$cancel_urlparameters. This may be intentional if PayPal's JavaScript SDK handles the return flow client-side, but please confirm this is the expected behavior.
11-12: LGTM: New imports for PayPal gateway support.The added imports for
OrderCreatedResponse,PaypalGateway, andGatewayInterfaceare correctly placed and necessary for the new PayPal integration.Also applies to: 23-23
tests/Webshop/Gateway/CaptureFailedResponseTest.php (1)
1-190: Thorough test coverage for CaptureFailedResponse.The tests comprehensively cover:
- Various error detail formats (structured PayPal errors, simple errors, unknown errors)
- Message formatting including debug_id
- Inheritance behavior from
OrderFailedResponse- All status methods (isSuccessful, isRedirect, isCancelled)
- Method chaining via
send()app/Http/Controllers/Shop/CheckoutController.php (1)
130-161: PayPal-specific flow for finalize method looks good.The implementation correctly differentiates between PayPal (returning
CheckoutResourcefor frontend handling) and other providers (using redirects). This aligns with PayPal's JavaScript SDK flow where the frontend manages the completion experience. The use of strict comparisons (===) follows coding guidelines.tests/Webshop/Gateway/OrderFailedResponseTest.php (1)
31-232: LGTM!Comprehensive test coverage for
OrderFailedResponseincluding constructor variations, status methods, message formatting edge cases, and exception behavior for not-implemented methods.config/omnipay.php (1)
39-45: LGTM!Clean consolidation of PayPal configuration into a single entry with proper environment variable usage. The docblocks provide good clarity on each gateway's purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Actions/Shop/Gateway/PaypalGateway.php (1)
151-169: ThetestModeparameter fromgetDefaultParameters()is unused.The
initialize()method definestestModeas a parameter ingetDefaultParameters()(line 127), but line 165 reads fromconfig('omnipay.testMode', false)instead of$parameters['testMode']. This means passing thetestModeparameter toinitialize()has no effect.If the intention is to use the global config value, consider removing
testModefromgetDefaultParameters()to avoid confusion. Otherwise, check$parameters['testMode']first before falling back to config.🔎 Proposed fix (if parameter should be used)
->environment( - config('omnipay.testMode', false) === true ? Environment::SANDBOX : Environment::PRODUCTION) + ($parameters['testMode'] ?? config('omnipay.testMode', false)) === true ? Environment::SANDBOX : Environment::PRODUCTION) ->build();
🧹 Nitpick comments (1)
app/Actions/Shop/Gateway/PaypalGateway.php (1)
380-388: Addissetguard before accessing array element.The code checks
is_array($capture['details'])but directly accesses$capture['details'][0]without verifying the array is non-empty. This could trigger an undefined array key notice if the details array is empty.🔎 Proposed fix
- if (is_array($capture) && is_array($capture['details']) && $capture['details'][0]->issue === 'INSTRUMENT_DECLINED') { + if (is_array($capture) && isset($capture['details'][0]) && $capture['details'][0]->issue === 'INSTRUMENT_DECLINED') { return new CaptureFailedResponse([
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
app/Actions/Shop/Gateway/PaypalGateway.php(1 hunks)docs/backend/Shop-implementation.md(8 hunks)docs/backend/Shop-integration.md(3 hunks)phpunit.ci.xml(1 hunks)phpunit.pgsql.xml(1 hunks)phpunit.xml(1 hunks)tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.php(1 hunks)tests/Webshop/Gateway/PaypalGatewayTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Webshop/Checkout/CheckoutProcessPaymentControllerTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
app/Actions/Shop/Gateway/PaypalGateway.phptests/Webshop/Gateway/PaypalGatewayTest.php
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: Use Markdown format for documentation
At the bottom of documentation files, add a horizontal rule followed by "Last updated: [date of the update]"
Files:
docs/backend/Shop-implementation.mddocs/backend/Shop-integration.md
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Webshop/Gateway/PaypalGatewayTest.php
🧠 Learnings (14)
📚 Learning: 2025-11-17T11:36:52.888Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Actions/Shop/CheckoutService.php:70-72
Timestamp: 2025-11-17T11:36:52.888Z
Learning: For Mollie and Stripe payment providers in the CheckoutService, the payment flow uses redirects rather than POST callbacks. When the user completes payment, they are redirected back to Lychee with query parameters in the URL (not POST data). The purchase parameters stored in the session are used with completePurchase() to verify the payment, and any necessary callback data comes from the query parameters in the redirect URL, which Omnipay handles automatically.
Applied to files:
app/Actions/Shop/Gateway/PaypalGateway.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:
docs/backend/Shop-implementation.mddocs/backend/Shop-integration.md
📚 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:
docs/backend/Shop-implementation.md
📚 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:
docs/backend/Shop-implementation.md
📚 Learning: 2025-09-18T13:37:32.687Z
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.
Applied to files:
docs/backend/Shop-implementation.md
📚 Learning: 2025-11-25T21:26:03.890Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T21:26:03.890Z
Learning: Applies to **/*.md : At the bottom of documentation files, add a horizontal rule followed by "*Last updated: [date of the update]*"
Applied to files:
docs/backend/Shop-implementation.mddocs/backend/Shop-integration.md
📚 Learning: 2025-10-26T19:15:58.151Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3765
File: version.md:1-1
Timestamp: 2025-10-26T19:15:58.151Z
Learning: The `version.md` file should NOT include the documentation footer (HR line followed by "*Last updated: [date]*") that is typically added to other Markdown files in the Lychee project. This file only contains the version number.
Applied to files:
docs/backend/Shop-implementation.md
📚 Learning: 2025-08-14T20:54:52.579Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3619
File: app/Rules/README.md:227-231
Timestamp: 2025-08-14T20:54:52.579Z
Learning: The user ildyria prefers to keep "Last updated" footers in italic format (*Last updated: [date]*) in documentation files, even if it triggers markdownlint MD036 warnings. This is a stylistic choice for the Lychee project.
Applied to files:
docs/backend/Shop-implementation.mddocs/backend/Shop-integration.md
📚 Learning: 2025-09-13T14:47:30.798Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: database/factories/PurchasableFactory.php:103-109
Timestamp: 2025-09-13T14:47:30.798Z
Learning: Photos and albums have a Many-Many relationship in Lychee, meaning a photo can belong to multiple albums. There is no single photo->album_id property. When creating purchasables for photos, both photo_id and album_id must be specified to indicate which photo-album combination the purchasable applies to.
Applied to files:
docs/backend/Shop-implementation.md
📚 Learning: 2025-09-21T20:09:31.762Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: database/migrations/2025_08_27_203030_create_purchasable.php:35-39
Timestamp: 2025-09-21T20:09:31.762Z
Learning: In the Lychee codebase, ildyria has decided to ignore the unique constraint issue with UNIQUE(album_id, photo_id) in the purchasables table where NULL values in MySQL/MariaDB don't enforce uniqueness as expected. This is acceptable for their multi-database support strategy (sqlite, pgsql, mariadb) and likely handled at the application level.
Applied to files:
docs/backend/Shop-implementation.md
📚 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:
docs/backend/Shop-implementation.md
📚 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(), the SQL query already includes ->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:
docs/backend/Shop-implementation.md
📚 Learning: 2025-09-13T11:30:21.765Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php:237-239
Timestamp: 2025-09-13T11:30:21.765Z
Learning: In tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php, directly modifying the shared $this->test_order fixture in loops is an intentional design choice, not something that needs to be refactored to create fresh orders for each iteration.
Applied to files:
tests/Webshop/Gateway/PaypalGatewayTest.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/Webshop/Gateway/PaypalGatewayTest.php
🪛 LanguageTool
docs/backend/Shop-integration.md
[uncategorized] ~72-~72: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e variants ### Manual Fulfillment - FULL Size Variants: Require photographer to man...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (13)
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
🔇 Additional comments (10)
docs/backend/Shop-integration.md (2)
11-91: LGTM - Comprehensive fulfillment documentation added.The documentation updates clearly explain the basket/order terminology, fulfillment workflow, size variant linking, order status lifecycle, and maintenance tasks. The additions align well with the expanded OrderService API and provide clear guidance for developers implementing or maintaining the shop integration.
209-209: LGTM - Date corrected.The last updated date now correctly shows December 19, 2025.
docs/backend/Shop-implementation.md (3)
12-244: LGTM - Comprehensive implementation documentation.The updates thoroughly document the database schema changes (album_id NOT NULL, unique constraints, size_variant_id, download_link), the expanded OrderService API, and the complete Order Fulfillment System with clear workflow descriptions and status transitions.
514-514: LGTM - Provider enum updated.The provider validation now correctly references PAYPAL instead of PAYPAL_EXPRESS, aligning with the consolidated PayPal gateway implementation.
621-623: LGTM - Date and termination corrected.The document now has a single, properly formatted termination section with the correct date (December 19, 2025).
phpunit.xml (1)
52-53: LGTM - PayPal credentials added to test configuration.The addition of PAYPAL_CLIENT_ID and PAYPAL_SECRET environment variables aligns with the new PayPal gateway integration and follows the existing pattern for payment provider credentials.
phpunit.ci.xml (1)
48-49: LGTM - PayPal credentials added to CI test configuration.Consistent with the phpunit.xml changes, enabling PayPal gateway tests in the CI environment.
phpunit.pgsql.xml (1)
55-56: LGTM - PayPal credentials added to PostgreSQL test configuration.Consistent with other test configurations, ensuring PayPal gateway tests run correctly with PostgreSQL.
app/Actions/Shop/Gateway/PaypalGateway.php (1)
70-307: LGTM - Well-structured PayPal gateway implementation.The gateway provides a clean Omnipay-compatible interface for PayPal Server SDK integration with comprehensive documentation, proper error handling, and detailed order/item mapping. The two-step payment flow (create order → capture) is correctly implemented.
tests/Webshop/Gateway/PaypalGatewayTest.php (1)
1-368: LGTM - Comprehensive PayPal gateway tests.The test suite provides thorough coverage of the PaypalGateway implementation, including interface compliance, initialization scenarios, order detail structure validation, and edge cases (deleted purchasables, multiple items, uninitialized client). All previously flagged issues with incomplete mock setup have been addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php (2)
237-279: Variable naming should use snake_case.Per coding guidelines, variable names should be in
snake_case. This applies to all PayPal test methods.🔎 Proposed fix
- $mockCheckoutService = \Mockery::mock(CheckoutService::class); + $mock_checkout_service = \Mockery::mock(CheckoutService::class); // Set up order in processing state with PayPal provider $this->test_order->status = PaymentStatusType::PROCESSING; $this->test_order->provider = OmnipayProviderType::PAYPAL; $this->test_order->save(); // Mock handlePaymentReturn to update order status to COMPLETED - $mockCheckoutService->shouldReceive('handlePaymentReturn') + $mock_checkout_service->shouldReceive('handlePaymentReturn') ->once() ->andReturnUsing(function ($order, $provider) { $order->status = PaymentStatusType::COMPLETED; $order->save(); return $order; }); // Bind the mock to the service container - $this->app->instance(CheckoutService::class, $mockCheckoutService); + $this->app->instance(CheckoutService::class, $mock_checkout_service);
296-302: Misleading comment: not returning a fresh instance.The comment states "Return a fresh instance to ensure attributes are loaded" but the code simply returns
$this->test_orderwithout callingfresh(). This is inconsistent with the cancelled test (line 351) which does callfresh().🔎 Proposed fix
// Mock handlePaymentReturn to return order with PROCESSING status (failed payment) $mockCheckoutService->shouldReceive('handlePaymentReturn') ->once() ->andReturnUsing(function ($order, $provider) { - // Return a fresh instance to ensure attributes are loaded - return $this->test_order; + // Return the order unchanged (simulates failed payment) + return $order; });app/Actions/Shop/Gateway/PaypalGateway.php (1)
391-400: Consider adding isset() check for defensive programming.While
is_array($capture['details'])ensures the key exists and is an array, it doesn't guarantee index0exists. Addingisset($capture['details'][0])would prevent potential undefined index notices if PayPal returns an empty details array.🔎 Suggested improvement
-if (is_array($capture) && is_array($capture['details']) && $capture['details'][0]->issue === 'INSTRUMENT_DECLINED') { +if (is_array($capture) && isset($capture['details'][0]) && is_object($capture['details'][0]) && $capture['details'][0]->issue === 'INSTRUMENT_DECLINED') {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Actions/Shop/Gateway/PaypalGateway.php(1 hunks)tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php(2 hunks)tests/Webshop/Gateway/PaypalGatewayTest.php(1 hunks)
🧰 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
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:
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.phptests/Webshop/Gateway/PaypalGatewayTest.phpapp/Actions/Shop/Gateway/PaypalGateway.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.phptests/Webshop/Gateway/PaypalGatewayTest.php
🧠 Learnings (5)
📚 Learning: 2025-09-13T11:30:21.765Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php:237-239
Timestamp: 2025-09-13T11:30:21.765Z
Learning: In tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php, directly modifying the shared $this->test_order fixture in loops is an intentional design choice, not something that needs to be refactored to create fresh orders for each iteration.
Applied to files:
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.phptests/Webshop/Gateway/PaypalGatewayTest.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:
tests/Webshop/Gateway/PaypalGatewayTest.php
📚 Learning: 2025-10-03T10:58:59.321Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3732
File: app/Http/Resources/Shop/OrderResource.php:62-64
Timestamp: 2025-10-03T10:58:59.321Z
Learning: In app/Http/Resources/Shop/OrderResource.php, the can_process_payment flag is intentionally set to false in order listings (via OrderService::getAll()) because items are explicitly excluded with without(['items']). This is by design: listings are lightweight, and the actual payment capability is only computed when fetching individual orders with full item data loaded.
Applied to files:
tests/Webshop/Gateway/PaypalGatewayTest.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/Webshop/Gateway/PaypalGatewayTest.php
📚 Learning: 2025-11-17T11:36:52.888Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Actions/Shop/CheckoutService.php:70-72
Timestamp: 2025-11-17T11:36:52.888Z
Learning: For Mollie and Stripe payment providers in the CheckoutService, the payment flow uses redirects rather than POST callbacks. When the user completes payment, they are redirected back to Lychee with query parameters in the URL (not POST data). The purchase parameters stored in the session are used with completePurchase() to verify the payment, and any necessary callback data comes from the query parameters in the redirect URL, which Omnipay handles automatically.
Applied to files:
app/Actions/Shop/Gateway/PaypalGateway.php
🧬 Code graph analysis (2)
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php (1)
app/Actions/Shop/CheckoutService.php (1)
CheckoutService(32-250)
app/Actions/Shop/Gateway/PaypalGateway.php (7)
app/Models/Order.php (2)
Order(65-313)items(121-124)app/Models/OrderItem.php (1)
OrderItem(58-190)app/Services/MoneyService.php (2)
MoneyService(22-98)toDecimal(91-97)app/Actions/Shop/Gateway/OrderCreatedResponse.php (3)
getCode(45-48)OrderCreatedResponse(13-64)getMessage(40-43)app/Actions/Shop/Gateway/OrderFailedResponse.php (3)
getCode(51-54)OrderFailedResponse(13-70)getMessage(46-49)app/Actions/Shop/Gateway/CapturedResponse.php (1)
CapturedResponse(11-13)app/Actions/Shop/Gateway/CaptureFailedResponse.php (1)
CaptureFailedResponse(11-13)
⏰ 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). (7)
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ JS front-end / Node 20
- GitHub Check: 2️⃣ PHP 8.3 - License Check
- GitHub Check: 2️⃣ PHP 8.3 - PHPStan
🔇 Additional comments (11)
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php (4)
21-21: LGTM!Import correctly added for the CheckoutService used in the new mock-based tests.
333-376: LGTM!The cancelled payment test correctly updates the order status to CANCELLED and uses
fresh()to ensure the returned instance has the latest attributes. The assertions properly verify both the JSON response and database state.
411-414: Content-Type header assertion may be fragile.Laravel often returns
application/json; charset=utf-8rather than justapplication/json. Consider using a partial match or Laravel's built-inassertJson()which already validates the response is JSON.🔎 Proposed fix
// For PayPal, verify it returns JSON content type, not a redirect $this->assertOk($response); - $response->assertHeader('Content-Type', 'application/json'); + $this->assertStringContainsString('application/json', $response->headers->get('Content-Type'));Alternatively, you can remove this assertion entirely since
assertJsonStructurebelow already validates the response is valid JSON.
434-467: LGTM!The non-PayPal redirect test provides good contrast to the PayPal JSON tests, verifying that traditional providers still return redirects. The
tearDownmethod properly cleans up Mockery after each test.app/Actions/Shop/Gateway/PaypalGateway.php (3)
162-180: LGTM! Environment selection now works correctly.The previous critical issue with hardcoded
Environment::SANDBOXhas been properly resolved. The gateway now reads fromconfig('omnipay.testMode', false)to determine the environment, ensuring production deployments use PayPal's live API.
210-249: Excellent Money handling and order transformation logic.The method correctly:
- Uses
MoneyService::toDecimal()to convert Money objects to PayPal-compatible decimal strings- Handles deleted purchasables gracefully with
purchasable_id ?? 'No-longer-existing'- Constructs descriptive SKUs for line items
Based on coding guidelines, monetary values are properly handled using the moneyphp/money library without using floats.
289-318: LGTM! Error handling improved.The previous critical issue with undefined
$orderin the catch block has been correctly resolved. Error logging now uses$e->getMessage()which is always available, preventing potential undefined variable errors.tests/Webshop/Gateway/PaypalGatewayTest.php (4)
63-159: Comprehensive initialization test coverage.The tests thoroughly cover:
- Interface compliance
- Gateway metadata (name, short name)
- Default parameter structure
- Multiple initialization scenarios including missing credentials
219-257: LGTM! Mock setup is now complete.The previous critical issue with incomplete mock setup has been fully resolved. Both
$orderItem1and$orderItem2are now properly initialized with all required properties and added to$order->items, ensuring the test correctly exercises the multiple-items scenario.
264-326: LGTM! Test setups are now complete and correct.Both
testGetOrderDetailsSkuFormatandtestGetOrderDetailsWithDeletedPurchasablenow have complete mock setups:
- All order item properties are properly initialized
- Items are correctly added to
$order->items- The deleted purchasable test properly sets
purchasable_idtonullThese tests now effectively validate the intended behaviors.
366-651: Excellent test coverage for purchase and capture flows.The tests comprehensively cover:
- Exception handling when client is not initialized
- Success paths returning appropriate response objects
- Failure scenarios (invalid status, API exceptions)
- Specific PayPal error cases like
INSTRUMENT_DECLINED- Proper mock setup with PayPal SDK classes
The error details mock (lines 587-590) correctly uses an object structure matching the gateway's PHPDoc expectations.
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 (2)
app/Http/Controllers/Shop/CheckoutController.php(2 hunks)tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php(3 hunks)
🧰 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
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:
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.phpapp/Http/Controllers/Shop/CheckoutController.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php
🧠 Learnings (2)
📚 Learning: 2025-09-13T11:30:21.765Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php:237-239
Timestamp: 2025-09-13T11:30:21.765Z
Learning: In tests/Feature_v2/Shop/CheckoutCreateSessionControllerTest.php, directly modifying the shared $this->test_order fixture in loops is an intentional design choice, not something that needs to be refactored to create fresh orders for each iteration.
Applied to files:
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php
📚 Learning: 2025-11-17T11:36:52.888Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3810
File: app/Actions/Shop/CheckoutService.php:70-72
Timestamp: 2025-11-17T11:36:52.888Z
Learning: For Mollie and Stripe payment providers in the CheckoutService, the payment flow uses redirects rather than POST callbacks. When the user completes payment, they are redirected back to Lychee with query parameters in the URL (not POST data). The purchase parameters stored in the session are used with completePurchase() to verify the payment, and any necessary callback data comes from the query parameters in the redirect URL, which Omnipay handles automatically.
Applied to files:
app/Http/Controllers/Shop/CheckoutController.php
🧬 Code graph analysis (2)
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php (1)
app/Actions/Shop/CheckoutService.php (1)
CheckoutService(32-250)
app/Http/Controllers/Shop/CheckoutController.php (10)
resources/js/lychee.d.ts (4)
OmnipayProviderType(106-106)CheckoutResource(911-918)PaymentStatusType(108-108)OrderResource(950-964)app/Http/Requests/Checkout/FinalizeRequest.php (3)
FinalizeRequest(29-83)basket(74-77)provider_type(79-82)app/Actions/Shop/CheckoutService.php (1)
handlePaymentReturn(180-207)app/Http/Requests/Checkout/CancelRequest.php (2)
basket(67-70)CancelRequest(26-71)app/Http/Requests/Basket/GetBasketRequest.php (1)
basket(23-26)app/Http/Requests/Traits/HasBasketTrait.php (1)
basket(24-27)app/Events/OrderCompleted.php (1)
OrderCompleted(23-37)app/Models/Configs.php (2)
Configs(65-369)getValueAsBool(284-287)app/Http/Resources/Shop/OrderItemResource.php (1)
fromModel(39-56)app/Http/Resources/Shop/OrderResource.php (1)
fromModel(48-72)
⏰ 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). (18)
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- ImageProcessing
- 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 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
🔇 Additional comments (1)
tests/Webshop/Checkout/CheckoutFinalizeOrCancelControllerTest.php (1)
384-424: No changes needed. The test expectations are correct.The CheckoutResource class uses Spatie LaravelData's
calculateResponseStatus()method to automatically set the HTTP status code based on theis_successfield. When is_success is false, the response status is set to 400 (HTTP_BAD_REQUEST), and when true, it returns 200 (HTTP_OK). The controller correctly passesis_successas$order->status === PaymentStatusType::COMPLETED, which is false for both failed and cancelled PayPal payments. No explicit status code setting is required in the controller.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/3-reference/shop-implementation.md (1)
578-587: Remove duplicate explanation section.The "Why These Conditions Exist:" section at lines 578–587 duplicates the earlier explanation at lines 578–581. This creates unnecessary redundancy in the documentation.
🔎 Proposed fix
**Why These Conditions Exist:** - -1. **Status Check**: Prevents processing completed/cancelled orders -2. **Email Requirement for FULL Variants**: FULL size variants need special export processing and must be delivered via email -3. **User/Email Requirement**: Ensures there's a way to contact the customer for order fulfillment -4. **Provider Requirement**: Cannot process payment without knowing which payment gateway to use
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
app/Http/Controllers/Shop/CheckoutController.phpcomposer.jsondocs/specs/3-reference/shop-implementation.mddocs/specs/4-architecture/shop-architecture.md
🚧 Files skipped from review as they are similar to previous changes (2)
- composer.json
- app/Http/Controllers/Shop/CheckoutController.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: Use Markdown format for documentation
At the bottom of documentation files, add an hr line followed by "Last updated: [date of the update]"
Files:
docs/specs/4-architecture/shop-architecture.mddocs/specs/3-reference/shop-implementation.md
🧠 Learnings (8)
📚 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:
docs/specs/4-architecture/shop-architecture.mddocs/specs/3-reference/shop-implementation.md
📚 Learning: 2025-09-13T14:47:30.798Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: database/factories/PurchasableFactory.php:103-109
Timestamp: 2025-09-13T14:47:30.798Z
Learning: Photos and albums have a Many-Many relationship in Lychee, meaning a photo can belong to multiple albums. There is no single photo->album_id property. When creating purchasables for photos, both photo_id and album_id must be specified to indicate which photo-album combination the purchasable applies to.
Applied to files:
docs/specs/3-reference/shop-implementation.md
📚 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:
docs/specs/3-reference/shop-implementation.md
📚 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:
docs/specs/3-reference/shop-implementation.md
📚 Learning: 2025-09-21T20:09:31.762Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: database/migrations/2025_08_27_203030_create_purchasable.php:35-39
Timestamp: 2025-09-21T20:09:31.762Z
Learning: In the Lychee codebase, ildyria has decided to ignore the unique constraint issue with UNIQUE(album_id, photo_id) in the purchasables table where NULL values in MySQL/MariaDB don't enforce uniqueness as expected. This is acceptable for their multi-database support strategy (sqlite, pgsql, mariadb) and likely handled at the application level.
Applied to files:
docs/specs/3-reference/shop-implementation.md
📚 Learning: 2025-09-18T13:37:32.687Z
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.
Applied to files:
docs/specs/3-reference/shop-implementation.md
📚 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:
docs/specs/3-reference/shop-implementation.md
📚 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(), the SQL query already includes ->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:
docs/specs/3-reference/shop-implementation.md
🪛 LanguageTool
docs/specs/4-architecture/shop-architecture.md
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e variants ### Manual Fulfillment - FULL Size Variants: Require photographer to man...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (18)
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Install
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- ImageProcessing
- GitHub Check: 2️⃣ PHP 8.3 - Code Style errors
- GitHub Check: 2️⃣ PHP 8.3 - PHPStan
ildyria
changed the title
ildyria
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.