Merge the OrderReturn endpoints into one domain PR

[PrestaEdit]

Questions	Answers
Branch?	dev
Description?	Consolidates the three pending OrderReturn PRs into one domain PR, and gives two of them the behavioural tests they shipped without
Type?	new feature
BC breaks?	no
Deprecations?	no
Fixed ticket?	Related to PrestaShop/PrestaShop#39630
How to test?	See below
Sponsor company	PrestaEdit

What this PR does

Merges #371, #372 and #386 into one PR, following the mutualisation done on the Product domain in #410.

Endpoints

Method	URI	CQRS	Scope
DELETE	/order-returns/{orderReturnId}	DeleteOrderReturnCommand	order_return_write
DELETE	/order-returns/bulk-delete	BulkDeleteOrderReturnsCommand	order_return_write
GET	/order-returns/{orderReturnId}/products	GetOrderReturnProducts	order_return_read
DELETE	/order-returns/{orderReturnId}/products/{orderDetailId}	DeleteProductFromOrderReturnCommand	order_return_write
DELETE	/order-returns/{orderReturnId}/products/bulk-delete	BulkDeleteProductsFromOrderReturnCommand	order_return_write

Two of the three had no behavioural test

#372 and #386 shipped with a getProtectedEndpoints() declaration and nothing else — scope protection covered, behaviour not. That was hard to avoid one PR at a time: #372's products listing is the only read side of #386's bulk delete, and #371's delete had nothing to check against either, so it fell back to SELECT COUNT(*) FROM ps_order_return.

One OrderReturnActionsEndpointTest now covers the five endpoints:

Endpoint	Asserted through
products listing	the complete row structure and the seeded order detail ids
delete one product	the listing, which no longer contains it
bulk delete products	the listing, now empty
delete an order return	GET /order-returns/{orderReturnId} answering 404
bulk delete order returns	the same GET, for each id

On fixtures: order returns are created by the customer from the front office. The OrderReturn domain exposes only DeleteOrderReturnCommand, BulkDeleteOrderReturnsCommand and UpdateOrderReturnStateCommand — there is no way to create one through the Admin API, so the seeding stays SQL. It is now a single documented helper, and it also creates the order_return_detail rows the product endpoints need, which is what makes those tests possible at all.

How to test

GET    /order-returns/{id}/products               -> 200, the returned lines
DELETE /order-returns/{id}/products/{detailId}    -> 204, then the listing drops it
DELETE /order-returns/{id}/products/bulk-delete   -> 204, then the listing is empty
DELETE /order-returns/{id}                        -> 204, then the GET answers 404
DELETE /order-returns/bulk-delete                 -> 204, then each GET answers 404

Covered by OrderReturnActionsEndpointTest.

Supersedes

• Add OrderReturn delete + bulk-delete Admin API endpoints #371
• Add OrderReturn products Admin API endpoints (list + delete) #372
• Add BulkDeleteProductsFromOrderReturn Admin API endpoint #386

All three will be closed once the CI is green here.

[github-actions]

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

📋 Summary of changes

This PR adds five new Admin API endpoints covering the OrderReturn domain: DELETE /order-returns/{orderReturnId}, DELETE /order-returns/bulk-delete, GET /order-returns/{orderReturnId}/products, DELETE /order-returns/{orderReturnId}/products/{orderDetailId}, and DELETE /order-returns/{orderReturnId}/products/bulk-delete. All new operations delegate to Core 9.2+ CQRS classes (DeleteOrderReturnCommand, BulkDeleteOrderReturnsCommand, GetOrderReturnProducts, DeleteProductFromOrderReturnCommand, BulkDeleteProductsFromOrderReturnCommand). The PR also corrects two pre-existing gaps in OrderReturn.php — missing validationContext on CQRSPartialUpdate and an un-grouped #[Assert\NotNull] that was wrongly triggered on DELETE — and adds a single integrated test class (OrderReturnActionsEndpointTest) covering all five endpoints.

⏱️ Estimated review time

15–20 minutes — five new resource classes plus one test class, a pre-9.2 PHPStan suppression block, and two targeted changes to an existing resource. Main work is verifying Core 9.2 field names against the collection DTO.

🎯 Scope

• Exposed operations: GET collection (products), DELETE single, DELETE bulk (×2)
• CQRS entity: OrderReturn + OrderReturnProduct sub-resource
• Integration test: yes — OrderReturnActionsEndpointTest covers all five endpoints

🧱 API Platform / CQRS architecture compliance

✅ What is correct

OrderReturn.php fixes — Adding validationContext: ['groups' => ['Default', 'Update']] to the existing CQRSPartialUpdate was a missing requirement per CONTEXT.md. Scoping #[Assert\NotNull(groups: ['Update'])] is the right solution to prevent the constraint from firing on DELETE (API Platform validates the resource on all operations sharing the same class). Both changes are correct.

exceptionToStatus coverage — CannotDeleteLastProductFromOrderReturnException → 422 is correctly mapped in both OrderReturnProduct and BulkOrderReturnProducts. OrderReturnConstraintException → 422 and OrderReturnNotFoundException → 404 are present on all resources that need them (see open item below for BulkOrderReturns).

Scopes — order_return_read / order_return_write follow the {entity_snake_case}_{action} pattern correctly. Every operation has at least one scope.

URI conventions — All paths are plural, lowercase, kebab-case; parent path correctly prefixes sub-resource paths; {orderReturnId} and {orderDetailId} use the {domain}Id convention; bulk URIs use the bulk- prefix.

minVersion: '9.2.0' — Consistently applied across all new operations, with matching PHPStan suppression blocks for the 9.0 and 9.1 config files.

No forbidden patterns — No custom normalizers, processors, or Value Object properties detected.

---

⚠️ Items requiring human verification

1. BulkOrderReturns.php — missing allowEmptyBody: false

The canonical BulkAttributeGroups.php sets allowEmptyBody: false on its CQRSDelete operation as a defensive gate against empty payloads. Neither BulkOrderReturns nor BulkOrderReturnProducts includes this flag. Assert\NotBlank on the body property is a second line of defence, but whether API Platform runs Symfony validation on a CQRSDelete body without allowEmptyBody: false should be confirmed.

2. BulkOrderReturns.php — exceptionToStatus maps only NotFoundException

Only OrderReturnNotFoundException → 404 is registered. If BulkDeleteOrderReturnsCommand can throw OrderReturnConstraintException, those would surface as 500s. Please verify against the Core command and add the mapping if needed.

3. OrderReturnProductList.php — unverifiable CQRSQueryMapping

No CQRSQueryMapping is declared. The DTO exposes orderDetailId, customizationId, reference, productName, quantity, customization. For these to be populated correctly without an explicit mapping, the Core's GetOrderReturnProducts query result must return fields with exactly those camelCase names. The test asserts the keys, which is a useful cross-check, but a human reviewer should open the Core 9.2 class and confirm the field names directly.

4. OrderReturnProduct.php — missing #[ApiProperty(identifier: true)] on $orderDetailId

CONTEXT.md: "The DTO property exposed as the identifier must match the URI parameter and be marked #[ApiProperty(identifier: true)]." For the DELETE sub-resource at …/products/{orderDetailId}, the leaf identifier $orderDetailId should bear this annotation. Please verify whether API Platform's route resolution requires it here (it does for single-entity resources; behaviour on sub-resource DELETE-only classes may differ).

5. BulkOrderReturnProducts.$stagedProductRows — bulk naming deviation

CONTEXT.md specifies the bulk property should be named {pluralDomain}Ids (e.g. attributeGroupIds). $stagedProductRows deviates because the compound key shape (order_detail_id + customization_id) cannot be reduced to a plain ID array, so the deviation is justified — but it is worth a conscious acknowledgement in the review thread.

💡 Improvement suggestions

testDeleteProductFromOrderReturn / testDeleteOrderReturn / testBulkDeleteOrderReturns — these use requestApi('DELETE', ...) where deleteItem($url, $scopes) (or deleteItem($url, $scopes, Response::HTTP_NO_CONTENT)) would be the idiomatic helper per CONTEXT.md. The result is equivalent but inconsistent with the preferred test API.

No @depends chain — The test methods seed independently rather than chaining via @depends. This is the right call given there is no create API for order returns (SQL seeding is the only option), but a brief comment in the class explaining why the chain was skipped would help future contributors.

testBulkDeleteEveryProductFromOrderReturnIsRefused — This test correctly expects 422 but does not call assertValidationErrors. That is intentional (the 422 comes from a domain exception, not from Symfony field validation), but it is worth noting: the response body shape differs from a validation error response, and a future test author might expect assertValidationErrors to be used here. A comment noting "domain exception, not validation error" would prevent confusion.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case
• Identifier uses domain name + Id suffix
• Sub-resources follow parent path
• Bulk operation URI uses bulk- prefix and plural Ids parameter — BulkOrderReturns.$orderReturnIds ✅; BulkOrderReturnProducts.$stagedProductRows deviates (justified by compound key, see above)

Operations & scopes

• Correct operation attribute per HTTP method
• Scope format: order_return_read / order_return_write, singular form

API Resource properties

• All properties strictly typed, scalars/arrays only (no Value Objects)
• Naming conventions respected (no is prefix, enabled not active, no localized prefix)
• #[ApiProperty(identifier: true)] on ID property — present on $orderReturnId in OrderReturn.php; missing on $orderDetailId in OrderReturnProduct.php (see above)
• #[LocalizedValue] / #[DefaultLanguage] — not applicable (no localized fields)

CQRS mapping

• QUERY_MAPPING direction — not applicable to CQRSDelete; CQRSGetCollection has no explicit mapping (verify Core field names, see above)
• CQRSCommandMapping direction — not applicable; path params bind by name
• CQRSQuery present on CQRSCreate/CQRSPartialUpdate when full object must be returned — not applicable to new operations; pre-existing CQRSPartialUpdate already had it
• No SerializedName — mappings only

Forbidden practices (CI-enforced)

• No custom normalizers or processors in the module
• No Value Objects in properties

Exception handling & validation

• ConstraintException → 422, NotFoundException → 404 — with caveat: BulkOrderReturns missing OrderReturnConstraintException (see above)
• Correct validationContext groups on Create / Update operations — CQRSPartialUpdate now has ['Default', 'Update'] ✅; CQRSDelete does not need it

Multi-shop

• shopIds absent — OrderReturn is a customer-facing entity (created via front-office RMA flow); no shop association on the DTO is expected. Human reviewer should confirm the Core entity has no shop table association.

Listing field alignment (CQRSGetCollection — OrderReturnProductList)

• DTO properties match fields from the CQRS query result — cannot verify without the Core 9.2 source; human reviewer must open GetOrderReturnProducts and cross-check field names
• No ApiResourceMapping entries needed — only if Core field names already match DTO names exactly
• No orphan DTO property — asserted indirectly by the integration test checking all keys

Integration test

• Extends ApiTestCase
• @depends chain — absent; justified (no create API, SQL seeding required)
• Asserts all response fields (testListOrderReturnProducts checks every key in $products[0])
• Edge-case 422 test (testBulkDeleteEveryProductFromOrderReturnIsRefused)
• getProtectedEndpoints() lists all five URIs
• DatabaseDump::restoreTables(['order_return', 'order_return_detail']) in setUp and tearDown
• declare(strict_types=1) present

[nicosomb]

Two more things worth noting, not really actionable in this PR:

Bulk delete isn't atomic. AbstractBulkCommandHandler::handleBulkAction only catches exceptions matching the type passed to it (DeleteOrderReturnException here), but OrderReturnRepository::delete() throws OrderReturnNotFoundException on a missing id, a sibling exception, not a subtype. Hitting a missing id partway through a batch re-throws immediately and abandons the rest of the array, with no rollback of what was already deleted. This is shared Core behavior, not specific to this PR, and no test here exercises a mixed valid/invalid batch.

Two items from the automated pre-review don't hold up. allowEmptyBody: false being absent on BulkOrderReturns/BulkOrderReturnProducts has no effect, false is already the framework default (CQRSApiSerializer::isEmptyBodyAllowed). And OrderReturnProductList's field mapping is fine as is, every property, including the isCustomization getter mapping to customization, matches GetOrderReturnProducts's result by name.

[nicosomb]

Only OrderReturnNotFoundException is mapped. BulkDeleteOrderReturnsCommand's constructor throws OrderReturnConstraintException for an invalid id (OrderReturnId::assertIsIntegerGreaterThanZero), unmapped here so it falls through to 500. Needs OrderReturnConstraintException mapped to 422.

[nicosomb]

No ApiProperty(identifier: true) on orderDetailId, unlike every other non-bulk single-target resource in the module (CategoryDelete, TaxRule, SearchEngine/DeleteSearchEngine, Address all mark theirs). Worth aligning for consistency.

[nicosomb]

GetOrderReturnProductsHandler builds each row from OrderReturnProductForEditing, which also exposes getCustomizationFields() (the actual customization content, e.g. text/file inputs), not exposed here. The DTO says a line is customized but never what the customization contains.