Add order remove-discount endpoint

[PrestaEdit]

Questions	Answers
Branch?	dev
Description?	Fills the DeleteCartRuleFromOrderCommand gap: DELETE /orders/{orderId}/discounts/{orderCartRuleId} (scope order_write) to remove a discount from an order.
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	Related to PrestaShop/PrestaShop#39630
How to test?	DELETE /orders/{id}/discounts/{orderCartRuleId} (client granted order_write) removes the order discount. Covered by OrderDiscountRemovalEndpointTest.
Possible impacts?	Removes a cart rule (discount) from the order and recalculates its totals.

[github-actions]

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

📋 Summary of changes

Adds a DELETE /orders/{orderId}/discounts/{orderCartRuleId} endpoint that wraps DeleteCartRuleFromOrderCommand to remove a cart rule (discount) from a specific order. The endpoint is scoped under order_write and is the first resource in the Order/ domain folder. An integration test (OrderDiscountRemovalEndpointTest) is included.

⏱️ Estimated review time

5–8 minutes — small, focused PR (2 new files, no existing code changed). Main review effort is on the dual-identifier pattern and test completeness.

🎯 Scope

• Exposed operations: DELETE
• CQRS entity: DeleteCartRuleFromOrderCommand
• Integration test: yes (partial — missing invalid/not-found scenarios)

🧱 API Platform / CQRS architecture compliance

✅ What looks correct

• URI template /orders/{orderId}/discounts/{orderCartRuleId} is plural, lowercase, kebab-case, and follows the sub-resource nesting convention.
• requirements: ['orderId' => '\d+', 'orderCartRuleId' => '\d+'] correctly validates both path parameters.
• Scope is order_write — correct for a DELETE operation.
• Both identifier properties are strictly typed (int).
• exceptionToStatus is present with OrderNotFoundException → 404. ✅
• License header is correct.
• No custom normalizer, no custom processor, no Value Objects, no SerializedName. ✅

---

⚠️ Concern 1 — orderId should not be #[ApiProperty(identifier: true)]

Both properties carry #[ApiProperty(identifier: true)]:

#[ApiProperty(identifier: true)]
public int $orderId;

#[ApiProperty(identifier: true)]
public int $orderCartRuleId;

Every comparable sub-resource in this module marks only the leaf-resource identifier:

Resource	URI	identifier: true
OrderAddress	/addresses/orders/{orderId}	orderId only
Combination	/products/combinations/{combinationId}	combinationId only
CategoryDelete	/categories/{categoryId}/{mode}	categoryId only

Because id_order_cart_rule is a globally-unique primary key in the DB, orderCartRuleId alone uniquely identifies the resource. orderId is a routing/validation parameter — it should be declared as a plain property (no identifier: true), exactly as OrderAddress.php does with its parent orderId.

Marking two properties as identifiers creates a composite-key resource in API Platform, which diverges from all established patterns in this module.

---

⚠️ Concern 2 — OrderException as catch-all for 422 is overly broad

OrderException::class => Response::HTTP_UNPROCESSABLE_ENTITY,

CONTEXT.md says: "at minimum the entity's …ConstraintException → 422". Using the base OrderException catches every OrderException subclass (including potential future exceptions that warrant other HTTP codes). The canonical pattern (see CategoryDelete.php, OrderAddress.php) is to list a specific …ConstraintException. If DeleteCartRuleFromOrderCommand can throw a dedicated OrderConstraintException, that should be listed instead.

---

⚠️ Concern 3 — No CQRSCommandMapping: constructor parameters must be verified

The resource has no CQRSCommandMapping, so the framework maps URI variables to DeleteCartRuleFromOrderCommand constructor parameters by name. This only works if the command's constructor parameters are named exactly orderId and orderCartRuleId. Please verify against the Core class. If they differ (e.g. $cartRuleId, $id), a mapping is required.

💡 Improvement suggestions

Test: use deleteItem instead of requestApi

CONTEXT.md states requestApi is the "last resort for cases the others don't fit". For a plain DELETE, deleteItem is the correct helper:

// Instead of:
$this->requestApi('DELETE', $url, null, ['order_write'], Response::HTTP_NO_CONTENT);

// Use:
$this->deleteItem($url, ['order_write'], Response::HTTP_NO_CONTENT);

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

The test manually deletes from order_cart_rule in tearDownAfterClass. CONTEXT.md requires DatabaseDump::restoreTables([...]) for proper DB isolation:

public static function setUpBeforeClass(): void
{
    parent::setUpBeforeClass();
    DatabaseDump::restoreTables(['order_cart_rule']);
    // ...
}

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

Manual SQL cleanup is less reliable (it won't run if the test fails mid-way, leaving the DB dirty).

Test: add testNotFound scenario

CONTEXT.md says: "Always include a testInvalid… method covering validation errors". A DELETE endpoint should also test the 404 path:

public function testDeleteNonExistentOrderCartRule(): void
{
    $this->deleteItem('/orders/99999/discounts/99999', ['order_write'], Response::HTTP_NOT_FOUND);
}

Test: createApiClient in setUpBeforeClass is redundant

CONTEXT.md notes that getBearerToken (called internally by requestApi/deleteItem) already creates an API client on-the-fly. The explicit self::createApiClient(['order_write']) call can be removed.

Test: test data setup is fragile

The test fetches "any cart rule" and "the most recent order" from the DB and inserts a order_cart_rule row joining them. There's no guarantee those two records are semantically related. If DeleteCartRuleFromOrderCommand validates the order/cart-rule relationship before deleting, this test may fail in environments where the fixture data doesn't satisfy that constraint. Using dedicated fixture data or a more targeted setup would make the test more robust.

✅ Pre-review checklist

URI & routing

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

Operations & scopes

• Correct operation attribute per HTTP method — CQRSDelete for DELETE ✅
• Scope format — order_write ✅

API Resource properties

• All properties strictly typed (int) ✅
• Naming conventions respected (no is prefix, no localized prefix) ✅
• #[ApiProperty(identifier: true)] on ID property — ⚠️ orderId is also marked identifier: true; only the leaf resource identifier (orderCartRuleId) should be
• #[LocalizedValue] / #[DefaultLanguage] — N/A (no localized fields)

CQRS mapping

• QUERY_MAPPING direction — N/A (no query)
• CQRSCommandMapping — absent; ⚠️ requires verification that DeleteCartRuleFromOrderCommand constructor params are named orderId / orderCartRuleId
• CQRSQuery on CQRSCreate/CQRSPartialUpdate — N/A (DELETE only)
• No SerializedName ✅

Forbidden practices (CI-enforced)

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

Exception handling & validation

• NotFoundException → 404 ✅
• ConstraintException → 422 — ⚠️ uses OrderException (base class) instead of a specific OrderConstraintException
• validationContext — N/A (DELETE only)

Multi-shop

• shopIds — N/A (order discount is not a shop-associated entity in this context)

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

Integration test

• Extends ApiTestCase ✅
• @depends chain — N/A (single meaningful test)
• testInvalid* with assertValidationErrors — ❌ missing; no testNotFound scenario either
• getProtectedEndpoints() lists the endpoint URI ✅
• DatabaseDump::restoreTables() — ❌ missing; manual SQL used instead
• declare(strict_types=1) present in test file ✅

[mattgoud]

The endpoint itself is correct, but it needs a not-found test before it can be approved. exceptionToStatus maps OrderNotFoundException => 404, yet nothing exercises that path. Please add one, e.g.:

public function testDeleteFromNonExistentOrderReturns404(): void
{
    $this->deleteItem('/orders/999999/discounts/999999', ['order_write'], Response::HTTP_NOT_FOUND);
}

While you're in the test, two small alignments with the module conventions: use the deleteItem helper instead of requestApi, and restore state via DatabaseDump::restoreTables(['order_cart_rule']) in setUp/tearDown rather than manual SQL (it also runs when a test fails mid-way).

For the record, you can disregard two points from the AI pre-review:

• OrderException => 422 "too broad" is correct here — DeleteCartRuleFromOrderHandler throws the base OrderException directly (e.g. "Invalid order cart rule provided."), so narrowing to a specific ConstraintException would let those escape as 500.
• The two #[ApiProperty(identifier: true)] are appropriate for a two-path-variable URI (/orders/{orderId}/discounts/{orderCartRuleId}) — both variables must map to the command. The single-identifier precedents it cites only have one id in the path.

Once the 404 test is in, happy to approve.

[mattgoud]

Re-reviewed 👍 404 test added, switched to the deleteItem helper, and state is restored via DatabaseDump::restoreTables(['order_cart_rule']) in setUp/tearDown. LGTM.

Non-blocking nit for before merge: the OrderDiscountRemoval resource is missing declare(strict_types=1) — added it on #319, worth keeping the Order resources consistent.