Skip to content

Add cart process-order-email endpoint - #318

Closed
PrestaEdit wants to merge 2 commits into
PrestaShop:devfrom
PrestaEdit:add-cart-process-order-email
Closed

PrestaEdit wants to merge 2 commits into
PrestaShop:devfrom
PrestaEdit:add-cart-process-order-email

Conversation

@PrestaEdit

Copy link
Copy Markdown
Contributor
Questions Answers
Branch? dev
Description? Fills the SendProcessOrderEmailCommand gap: PUT /carts/{cartId}/process-order-emails (scope order_write) to send the process-order email to the cart's customer.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Related to PrestaShop/PrestaShop#39630
How to test? PUT /carts/{id}/process-order-emails (client granted order_write) emails the customer. Covered by CartProcessOrderEmailEndpointTest (which disables real mail sending so the assertion does not depend on an SMTP server).
Possible impacts? Sends the process-order email to the cart customer.

Fill the SendProcessOrderEmailCommand gap: PUT /carts/{cartId}/process-order-emails
(scope order_write) to send the process-order email to the cart customer. The test
disables real mail sending (PS_MAIL_METHOD) so Mail::send() succeeds without SMTP.

Related to PrestaShop/PrestaShop#39630

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@mattgoud mattgoud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CartConstraintException is missing from exceptionToStatus. The command builds new CartId($cartId), which throws CartConstraintException (extends CartException, not OrderException) for an invalid id such as 0 — and 0 is reachable since the \d+ requirement matches it. It currently falls through to a 500 instead of 422. Please add CartConstraintException::class => Response::HTTP_UNPROCESSABLE_ENTITY.

Also missing a testInvalid* (422) test per the module testing conventions.

@ps-jarvis ps-jarvis moved this from Ready for review to Waiting for author in PR Dashboard Jul 13, 2026
- Add CartConstraintException => HTTP_UNPROCESSABLE_ENTITY to
  exceptionToStatus. cartId=0 satisfies the URI \d+ requirement but
  `new CartId(0)` throws CartConstraintException, which used to fall
  through to a 500. It now surfaces as 422.
- Add testSendProcessOrderEmailWithInvalidCartIdReturns422 covering
  that case.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@PrestaEdit

Copy link
Copy Markdown
Contributor Author

Thanks @mattgoud — addressed in the last push.

  • Added CartConstraintException::class => Response::HTTP_UNPROCESSABLE_ENTITY to exceptionToStatus on the resource, exactly as flagged. Since \d+ in the URI matches 0, new CartId(0) used to bubble up as a 500; it now returns 422 cleanly.
  • Added testSendProcessOrderEmailWithInvalidCartIdReturns422 — PUTs /carts/0/process-order-emails and asserts 422.

Ready for another look.

@mattgoud mattgoud added the Need AI review Trigger: Request an AI pre-review from Claude label Jul 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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

📋 Summary of changes

This PR exposes SendProcessOrderEmailCommand from the PrestaShop Core as a PUT /carts/{cartId}/process-order-emails endpoint, gated by the order_write scope. The endpoint sends the process-order email to the cart's customer and returns no body (204 No Content). A single resource class and one integration test class are added; no existing files are modified.

⏱️ Estimated review time

5–8 minutes — the change is small (two new files, no mappings, no localized fields), but there is a noteworthy architectural question about the HTTP verb choice and a few test gaps to validate.

🎯 Scope

  • Exposed operations: PUT (action endpoint, no response body)
  • CQRS entity: SendProcessOrderEmailCommand (Order domain, Core)
  • Integration test: yes (partial — see issues below)
🧱 API Platform / CQRS architecture compliance

HTTP verb / operation attribute

CQRSUpdate maps to PUT, which carries idempotency semantics by HTTP definition. Sending an email is not idempotent: two identical PUT /carts/42/process-order-emails calls send two emails. CQRSCreate (POST) with output: false would return 204 just as cleanly and aligns with RFC 9110 semantics for non-idempotent actions. Consider switching to CQRSCreate / POST, or at least document the deliberate choice of PUT.

URI template

/carts/{cartId}/process-order-emails follows the plural, lowercase, kebab-case convention. ✅

Identifier / DTO property

public int $cartId with #[ApiProperty(identifier: true)] — correct. ✅

Command mapping

No CQRSCommandMapping is declared, so the framework will try to inject $cartId directly into SendProcessOrderEmailCommand::__construct(). This works only if the constructor's parameter name matches cartId exactly. Please verify the Core command's constructor signature; if the parameter is named differently (e.g., $id) a COMMAND_MAPPING is required.

Scopes

order_write is syntactically correct. Semantically, a cart-domain action using an order scope is a design choice — acceptable given there is no cart_write scope — but worth noting for documentation.

Exception handling

All five mapped exceptions appear relevant and use the correct HTTP status codes (404 / 422). ✅

No custom normalizer/processor

None added. ✅

No Value Objects

$cartId is typed int. ✅

Multi-shop

Cart entity is not shop-associated in the DTO sense; shopIds is correctly absent. ✅

💡 Improvement suggestions
  1. HTTP verb: Switch to CQRSCreate / POST if the team agrees that sending an email is a non-idempotent action. This is the most impactful design question in the PR.

  2. Test: use updateItem() helper: ApiTestCase::updateItem() exists for PUT requests and accepts null data. Using it instead of requestApi() directly keeps tests consistent with the rest of the suite.

  3. Test: add a 404 case: CartNotFoundException is in exceptionToStatus but never exercised. Add a test that calls the endpoint with a valid-format but non-existent cart ID (e.g., 999999) and asserts HTTP_NOT_FOUND.

  4. Test: verify error response shape in the 422 case: assertValidationErrors() (or at minimum checking response['violations']) should accompany the 422 assertion so the error contract is tested, not just the status code.

  5. Test: restore DB state: createCustomerCart() inserts a ps_cart row that is never cleaned up. Add DatabaseDump::restoreTables(['cart', 'cart_product']) (or the tables your fixture touches) to setUpBeforeClass / tearDownAfterClass so the test is hermetic.

  6. Redundant createApiClient call: getBearerToken() already creates an API client on the fly when one does not exist (per ApiTestCase internals). The explicit self::createApiClient(['order_write']) in setUpBeforeClass is unnecessary unless there is a specific reason to pre-warm it.

✅ 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 — N/A

Operations & scopes

  • Correct operation attribute per HTTP method
  • Scope format: order_write is valid — ⚠️ see note above on idempotency / verb choice

API Resource properties

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

CQRS mapping

  • QUERY_MAPPING direction — N/A (no GET / no response body)
  • CQRSCommandMapping direction — N/A (no body fields); ⚠️ verify constructor param name in Core
  • CQRSQuery present — N/A (output: false)
  • No SerializedName

Forbidden practices (CI-enforced)

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

Exception handling & validation

  • ConstraintException → 422, NotFoundException → 404
  • Correct validationContext groups — N/A (no body fields)

Multi-shop

  • shopIds absent (entity not shop-associated) ✅

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

Integration test

  • Extends ApiTestCase
  • @depends chain — N/A (single action, not a CRUD chain)
  • testInvalid* with assertValidationErrors — status code only, error shape not verified
  • getProtectedEndpoints() lists all URIs
  • DatabaseDump::restoreTables() covers all affected tables — missing; createCustomerCart() leaves a row behind
  • declare(strict_types=1) present
  • Missing 404 test (non-existent cart ID)

@github-actions github-actions Bot added AI reviewed Status: Claude AI has already pre-reviewed this PR and removed Need AI review Trigger: Request an AI pre-review from Claude labels Jul 15, 2026

@mattgoud mattgoud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, CartConstraintException is now mapped to 422 and the negative test is in. LGTM.

@mattgoud mattgoud added Waiting for QA Status: Action required, Waiting for test feedback and removed Waiting for author labels Jul 15, 2026
@ps-jarvis ps-jarvis moved this from Waiting for author to To be tested in PR Dashboard Jul 15, 2026
@PrestaEdit
PrestaEdit marked this pull request as draft August 20, 2026 08:21
@PrestaEdit

Copy link
Copy Markdown
Contributor Author

Superseded by #424, which merges this endpoint with the rest of the domain, following the mutualisation asked for by the core team (one PR per domain rather than one per endpoint).

The consolidated PR is green and lists what changed for this endpoint on the way in. Closing here to keep the review in one place.

@PrestaEdit PrestaEdit closed this Aug 20, 2026
@github-project-automation github-project-automation Bot moved this from To be tested to Closed in PR Dashboard Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI reviewed Status: Claude AI has already pre-reviewed this PR Waiting for QA Status: Action required, Waiting for test feedback

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants