Skip to content

Merge the two OrderState resource PRs into one - #417

Open
PrestaEdit wants to merge 5 commits into
PrestaShop:devfrom
PrestaEdit:domain/order-state
Open

PrestaEdit wants to merge 5 commits into
PrestaShop:devfrom
PrestaEdit:domain/order-state

Conversation

@PrestaEdit

Copy link
Copy Markdown
Contributor
Questions Answers
Branch? dev
Description? Consolidates the two conflicting OrderState PRs into a single domain resource, keeping the best of each, with full-structure assertions
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 #226 and #260 into one PR, following the mutualisation done on the Product domain in #410.

These two PRs were not complementary — they were two different implementations of the same resource. Both created src/ApiPlatform/Resources/OrderState/OrderState.php and tests/Integration/ApiPlatform/OrderStateEndpointTest.php, plus a bulk-delete class differing only by its name (BulkDeleteOrderState vs BulkDeleteOrderStates). Whichever had been merged first, the other would have conflicted.

Endpoints

Method URI CQRS Scope
GET /order-states/{orderStateId} GetOrderStateForEditing order_state_read
GET /order-states prestashop.core.grid.data_provider.order_states order_state_read
POST /order-states AddOrderStateCommand + GetOrderStateForEditing order_state_write
PATCH /order-states/{orderStateId} EditOrderStateCommand + GetOrderStateForEditing order_state_write
DELETE /order-states/{orderStateId} DeleteOrderStateCommand order_state_write
DELETE /order-states/bulk-delete BulkDeleteOrderStateCommand order_state_write

How the two versions were reconciled

#260 is the base, since it was the more advanced of the two. What #226 had and #260 did not has been grafted back in:

Kept from What
#260 POST replaying GetOrderStateForEditing, so the create returns the entity instead of just {orderStateId}
#260 DefaultLanguage constraints on names (the core validator for localized required fields) rather than a plain NotBlank
#260 DuplicateOrderStateNameException → 422
#260 the paginated list endpoint (OrderStateList)
#226 the read-only deleted property — EditableOrderState::isDeleted() was exposed by neither the resource nor the tests in #260
#226 normalizationContext: ['skip_null_values' => false]
#226 read: false on the partial update, consistent with the other resources declaring a CQRSQuery
#226 the soft-delete assertions, see below

Fixed along the way

The bulk-delete exception mapping was wrong in both PRs. BulkDeleteOrderStateHandler catches every OrderStateException (including "not found") per id, collects the failures and rethrows a single BulkDeleteOrderStateException. So OrderStateNotFoundException => 404 (#226) could never fire, and OrderStateException => 404 (#260) turned a partial failure into a 404. It is now BulkDeleteOrderStateException => 422.

BulkDeleteOrderStateException does not implement BulkCommandExceptionInterface, so the multi-status normalizer does not apply here and a plain exceptionToStatus mapping is the right thing.

Tests

One OrderStateEndpointTest instead of the two conflicting ones. It already built every fixture through the API in #260; what changed:

  • testAddOrderState, testGetOrderState and testPartialUpdateOrderState now compare the complete entity with a single assertEquals instead of checking a handful of keys, which is what actually pins the read/write symmetry of the POST and the PATCH;
  • the soft-delete assertions from Add OrderState resource #226 are back: after a delete (single or bulk) the order state is still readable through the GET with deleted: true, and no longer appears in the listing. Add OrderState Admin API endpoints (CRUD + list + bulk delete) #260's test only checked the listing.

How to test

POST   /order-states                  -> 200, the whole entity, not just the id
GET    /order-states/{id}             -> 200, identical to the POST response
PATCH  /order-states/{id}             -> 200, the updated entity
GET    /order-states                  -> 200, paginated list
DELETE /order-states/{id}             -> 204, then GET returns deleted: true
DELETE /order-states/bulk-delete      -> 204, then each GET returns deleted: true

Covered by OrderStateEndpointTest.

Supersedes

Both will be closed once the CI is green here.

PrestaEdit and others added 5 commits August 20, 2026 10:33
Add the Admin API endpoints for the OrderState domain (order statuses):

- POST   /order-states                  (AddOrderStateCommand)
- GET    /order-states/{orderStateId}   (GetOrderStateForEditing)
- PATCH  /order-states/{orderStateId}   (EditOrderStateCommand)
- DELETE /order-states/{orderStateId}   (DeleteOrderStateCommand)
- DELETE /order-states/bulk-delete      (BulkDeleteOrderStateCommand)

Exposes the order state data fields (localized names + email templates, color
and the behaviour flags: loggable, invoice, hidden, sendEmail, pdfInvoice,
pdfDelivery, shipped, paid, delivery). As with Contact/OrderReturnState the
create and update commands expect different field names (localizedNames/
localizedTemplates vs name/template), handled by distinct command mappings.
The optional state icon upload is intentionally left out of this first version.

Adds an integration test covering the full CRUD + bulk delete, and the
order_state_read / order_state_write scopes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two issues from the first version:
- GetOrderStateForEditing exposes isSendEmailEnabled() and isDeleted(), so the
  query result keys are "sendEmailEnabled" / "deleted" — added the missing
  mappings to QUERY_MAPPING and an "isDeleted" property to the resource.
- Order states are soft-deleted, so the record is still readable after a
  delete; the tests now assert isDeleted=true instead of a 404.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rector's boolean property naming rule requires 'deleted' rather than
'isDeleted'; with that name the query result field maps by identity, so the
extra QUERY_MAPPING entry is dropped. Tests updated accordingly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Consolidates PrestaShop#226 and PrestaShop#260, which both created
src/ApiPlatform/Resources/OrderState/OrderState.php and
tests/Integration/ApiPlatform/OrderStateEndpointTest.php with different content.

Takes PrestaShop#260 as the base (it is the more complete one) and grafts what only PrestaShop#226 had:

- PrestaShop#260 wins on: POST replaying GetOrderStateForEditing so the create returns the
  entity, the DefaultLanguage constraints on the localized names, the
  DuplicateOrderStateNameException mapping and the paginated list endpoint
- PrestaShop#226 wins on: the read-only "deleted" property (EditableOrderState::isDeleted()),
  skip_null_values => false, read: false on the partial update, and the soft-delete
  assertions its test had and PrestaShop#260's had lost
- one bulk class instead of BulkDeleteOrderState + BulkDeleteOrderStates, and its
  exception mapping is fixed: the handler swallows every OrderStateException and
  rethrows BulkDeleteOrderStateException with the failed ids, so neither
  OrderStateNotFoundException => 404 (PrestaShop#226) nor OrderStateException => 404 (PrestaShop#260)
  could ever fire. It is now BulkDeleteOrderStateException => 422
- the create, get and update assertions compare the complete entity instead of
  checking a few keys, so a missing or extra field fails the test

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

2 participants