Add order discount endpoint

[PrestaEdit]

Questions	Answers
Branch?	dev
Description?	Fills the AddCartRuleToOrderCommand gap: PUT /orders/{orderId}/discounts (scope order_write) to add a discount to an order (cartRuleName, cartRuleType = percent/amount/free_shipping, value, optional orderInvoiceId).
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	Related to PrestaShop/PrestaShop#39630
How to test?	PUT /orders/{id}/discounts with { "cartRuleName": "...", "cartRuleType": "amount", "value": "1" } (client granted order_write) adds the discount. Covered by OrderDiscountEndpointTest.
Possible impacts?	Adds a cart rule (discount) to the order and recalculates its totals.

[mattgoud]

This wraps an Add…Command but exposes it as CQRSUpdate (PUT) on a plural collection URI /orders/{orderId}/discounts. The operation is non-idempotent (calling it twice adds two discounts), so per the module conventions it should be CQRSCreate (POST).

Also missing a testInvalid* (422) test — empty cartRuleName or a null value for an amount/percent discount both throw OrderConstraintException (422) and would make good negative cases.

[PrestaEdit]

Thanks @mattgoud — addressed in the last push.

• Switched CQRSUpdate (PUT) → CQRSCreate (POST) on the same URI, so the operation now matches its non-idempotent semantics (calling twice adds two discounts). Kept output: false since there's no CQRSQuery for a single order-cart-rule to return.
• Added #[Assert\NotBlank] on $cartRuleName and #[Assert\Choice(choices: ['percent', 'amount', 'free_shipping'])] on $cartRuleType so malformed payloads are rejected at the validator layer with the standard 422 payload.
• Test: switched testAddCartRuleToOrder to createItem (POST) with HTTP_CREATED; updated getProtectedEndpoints to POST; added testAddCartRuleWithEmptyName asserting 422 via assertValidationErrors on cartRuleName.

Ready for another look.

[mattgoud]

Thanks for switching to CQRSCreate (POST). CI is red now though: the happy-path testAddCartRuleToOrder asserts HTTP_CREATED (201), but with output: false the endpoint returns 204 No Content, so the assertion fails on all 8 integration matrix jobs. Either assert HTTP_NO_CONTENT (204), or, if you want the created state echoed back, drop output: false and wire a CQRSQuery.

[github-actions]

🤖 Claude AI Pre-Review — Automated analysis. Does not replace human review.

📋 Summary of changes

Adds a new POST /orders/{orderId}/discounts endpoint that wraps AddCartRuleToOrderCommand to apply a cart-rule-based discount to an existing order. The endpoint accepts cartRuleName, cartRuleType (percent / amount / free_shipping), an optional value, and an optional orderInvoiceId. No response body is returned (204 No Content). A matching OrderDiscountEndpointTest covers the happy path and one validation error case.

⏱️ Estimated review time

5–8 minutes — small surface (one resource class, one test), but several convention gaps need attention.

🎯 Scope

• Exposed operations: POST
• CQRS entity: AddCartRuleToOrderCommand
• Integration test: partial (happy path + one validation case; coverage gaps noted below)

🧱 API Platform / CQRS architecture compliance

CQRSCreate — missing validationContext ⛔

CQRSCreate is missing the required validationContext: ['groups' => ['Default', 'Create']] attribute (CONTEXT.md: "Use validationContext: ['groups' => ['Default', 'Create']] on CQRSCreate"). Without it the Create validation group is never applied, which means any field-level constraint that uses the Create group will be silently ignored.

new CQRSCreate(
    uriTemplate: '/orders/{orderId}/discounts',
    requirements: ['orderId' => '\d+'],
    output: false,
    CQRSCommand: AddCartRuleToOrderCommand::class,
    scopes: ['order_write'],
    validationContext: ['groups' => ['Default', 'Create']],  // ← missing
),

$cartRuleType — missing #[Assert\NotBlank]

$cartRuleType is non-nullable (string, no default) and only carries #[Assert\Choice]. If the field is omitted from the request body, API Platform leaves the property uninitialized and the CQRS layer will encounter an Error. Adding #[Assert\NotBlank] makes the missing-field case explicit and returns a proper 422 instead of a 500.

exceptionToStatus — broad OrderException catch

OrderException::class => Response::HTTP_UNPROCESSABLE_ENTITY is very broad — it catches all order exceptions not listed above it, including ones that should perhaps be 404 or a different code. If a narrower OrderConstraintException exists in the Core, prefer mapping that to 422 and leaving OrderException unregistered (or as a 500). At minimum, document why the broad mapping is intentional.

Command mapping — no issues

AddCartRuleToOrderCommand's constructor parameters ($orderId, $cartRuleName, $cartRuleType, $value, $orderInvoiceId) match the DTO property names exactly. No CQRSCommandMapping is needed. ✅

$value typing — acceptable

The command accepts ?string for $value and converts it to DecimalNumber internally. Using ?string on the DTO is therefore correct here — no DecimalNumber property needed. ✅

URI, scopes, identifier — correct

/orders/{orderId}/discounts is plural/kebab-case; {orderId} matches $orderId marked #[ApiProperty(identifier: true)]; scope order_write follows the {entity_snake_case}_write pattern. ✅

PR description says PUT but code uses POST

The PR description's "How to test" section reads PUT /orders/{id}/discounts, but the implementation correctly uses CQRSCreate (POST). The description should be corrected.

💡 Improvement suggestions

Test teardown: replace manual SQL with DatabaseDump::restoreTables()

tearDownAfterClass manually deletes from order_cart_rule by discount name. CONTEXT.md requires using DatabaseDump::restoreTables([...]) for all affected tables. The current approach is fragile (misses related table rows such as order_discount_tax, order_cart_rule, and the order totals) and diverges from the project convention:

public static function tearDownAfterClass(): void
{
    DatabaseDump::restoreTables(['orders', 'order_cart_rule', 'order_discount_tax']);
    parent::tearDownAfterClass();
}

Test method naming: rename to testInvalid*

testAddCartRuleWithEmptyName should follow the testInvalid… naming pattern (CONTEXT.md: "Always include a testInvalid… method"). Suggested rename: testInvalidCartRuleName.

Add a test for invalid cartRuleType

Only empty cartRuleName is tested for validation. Adding a case for an out-of-range cartRuleType (e.g. 'discount') would verify #[Assert\Choice] is wired correctly.

@depends chain

testAddCartRuleWithEmptyName and testAddCartRuleToOrder both independently fetch the latest order ID. If they were chained with @depends, the second test would reuse the orderId from the first and avoid a second raw-SQL round-trip.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case — /orders/{orderId}/discounts ✅
• Identifier uses domain name + Id suffix — orderId ✅
• Sub-resources follow parent path ✅
• Bulk operation URI — N/A

Operations & scopes

• Correct operation attribute per HTTP method — CQRSCreate for POST ✅
• Scope format: order_write ✅

API Resource properties

• All properties strictly typed, scalars/arrays only ✅
• Naming conventions respected ✅
• #[ApiProperty(identifier: true)] on $orderId ✅
• #[LocalizedValue] / #[DefaultLanguage] — N/A (no localized fields)

CQRS mapping

• QUERY_MAPPING — N/A (POST only)
• CQRSCommandMapping — not needed, property names match command parameters exactly ✅
• CQRSQuery on CQRSCreate — output: false / 204 intentional ✅
• No SerializedName ✅

Forbidden practices (CI-enforced)

• No custom normalizers or processors ✅
• No Value Objects in properties ✅

Exception handling & validation

• OrderNotFoundException → 404 ✅
• InvalidCartRuleDiscountValueException → 422 ✅
• validationContext: ['groups' => ['Default', 'Create']] missing on CQRSCreate ⛔

Multi-shop

• No shopIds needed — adding a discount to an identified order does not require a shop constraint ✅

Listing field alignment — N/A (no list endpoint)

Integration test

• Extends ApiTestCase ✅
• testInvalid* naming — method is named testAddCartRuleWithEmptyName, not testInvalid… ⚠️
• getProtectedEndpoints() lists the POST URI ✅
• DatabaseDump::restoreTables() — missing; teardown uses raw SQL delete instead ⚠️
• declare(strict_types=1) present ✅

[mattgoud]

Two small robustness/convention points (from the AI pre-review, verified):

1. $cartRuleType is required but only has #[Assert\Choice(...)], no #[Assert\NotBlank]. Since the property is a non-nullable string with no default, omitting it from the request risks an uninitialized-typed-property error (500) instead of a clean 422. Adding #[Assert\NotBlank] gives the proper validation error.
2. Consider validationContext: ['groups' => ['Default', 'Create']] on the CQRSCreate to match the module convention (and future-proof any Create-group constraints). Low impact today since your current constraints are in the Default group, but it's the documented pattern.

[mattgoud]

Thanks, validationContext ['Default','Create'] and Assert\NotBlank on cartRuleType are in. LGTM.