Add cart send-to-customer endpoint

[PrestaEdit]

Questions	Answers
Branch?	dev
Description?	Fills the SendCartToCustomerCommand gap: PUT /carts/{cartId}/emails (scope cart_write) to send the cart to its customer by email.
Type?	improvement
BC breaks?	no
Deprecations?	no
Related ticket?	Related to PrestaShop/PrestaShop#39630
Fixed ticket?	fixed PrestaShop/PrestaShop#41969
How to test?	PUT /carts/{id}/emails (client granted cart_write) emails the cart to the customer. Covered by CartEmailEndpointTest (which disables real mail sending so the assertion does not depend on an SMTP server).
Possible impacts?	Sends an email to the cart customer.

[mattgoud]

Thanks for this. The endpoint is correct and functional and follows the module conventions well:

• SendCartToCustomerCommand::__construct($cartId) maps cleanly from the cartId identifier, no CQRSCommandMapping needed, and the green integration test confirms the wiring.
• The exception mapping is correctly ordered: CartNotFoundException => 404 before CartException => 422 (required since CartNotFoundException extends CartException).
• read: false + output: false + allowEmptyBody: true is the right combo for a no-input/no-output action returning 204, and disabling PS_MAIL_METHOD in the test avoids an SMTP dependency.

A few changes I would like before merging:

1. Add declare(strict_types=1); to CartEmail.php. The test file already has it, and it is the project standard for every PHP file.
2. Cover the not-found case in the test. A PUT /carts/{nonExistentId}/emails asserting 404 would exercise the CartNotFoundException => 404 mapping, which is the most subtle part of the resource and is currently untested. This is the main one for me.
3. Use a non-empty cart in createCustomerCart() (add a product) so the test is more representative of a real send-to-customer flow.

Happy to re-review once these are in.

[github-actions]

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

📋 Summary of changes

This PR exposes SendCartToCustomerCommand as a new PUT /carts/{cartId}/emails endpoint (scope cart_write). It adds a single resource class CartEmail that triggers the command with no request body and no response body (204 No Content), plus an integration test CartEmailEndpointTest that disables real mail sending for CI safety.

⏱️ Estimated review time

3–5 minutes — the resource class is very small; the test has a couple of issues worth examining closely.

🎯 Scope

• Exposed operations: PUT (action — sends email)
• CQRS entity: SendCartToCustomerCommand
• Integration test: yes (partial — missing invalid-ID coverage and DB cleanup)

🧱 API Platform / CQRS architecture compliance

Operation attribute choice — acceptable but worth a note

CQRSUpdate wraps a PUT and fires a command, which is the best available attribute for an action that has no response body and no read phase. SendCartToCustomerCommand is not strictly an "edit" command, but with read: false, output: false, and allowEmptyBody: true the semantics are correct for an action endpoint. No dedicated "CQRSAction" attribute exists in the framework, so this is the right pragmatic choice.

URI — /carts/{cartId}/emails is plural, lowercase, kebab-case, and follows parent-path sub-resource convention. ✅

exceptionToStatus

• CartNotFoundException → 404 ✅
• CartException → 422 ✅

Identifier property — $cartId with #[ApiProperty(identifier: true)] is correct. ✅

No CQRSCommandMapping needed — {cartId} in the URI template is the only parameter; the framework auto-maps it to the command constructor parameter of the same name. ✅

No CQRSQuery needed — output: false means the endpoint returns 204 and there is nothing to re-read. ✅

No custom normalizer or processor — ✅

No Value Objects in properties — ✅

💡 Improvement suggestions

1. Missing test coverage for non-existent cartId (404)

CONTEXT.md requires a testInvalid… method. For this endpoint the relevant invalid case is a non-existent cart ID, which should yield 404 (via CartNotFoundException). Add:

public function testSendCartToNonExistentCart(): void
{
    $this->requestApi(
        'PUT',
        '/carts/99999/emails',
        null,
        ['cart_write'],
        Response::HTTP_NOT_FOUND
    );
}

2. No DB cleanup for the cart created in createCustomerCart()

createCustomerCart() calls $cart->add(), inserting a row into the cart table. There is no DatabaseDump::restoreTables() call, so this row persists for subsequent test runs. Add to setUpBeforeClass / tearDownAfterClass:

DatabaseDump::restoreTables(['cart']);

3. requestApi instead of updateItem helper

CONTEXT.md says to use requestApi only as a last resort. updateItem('/carts/{id}/emails', [], ['cart_write'], Response::HTTP_NO_CONTENT) does the same thing through the canonical helper. Switch to updateItem for consistency.

4. createApiClient call is unnecessary

self::createApiClient(['cart_write']) in setUpBeforeClass is redundant — getBearerToken (used by the helpers internally) already creates an API client on the fly per scope set. Remove the explicit call.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case (/carts/{cartId}/emails)
• Identifier uses domain name + Id suffix (cartId)
• Sub-resources follow parent path (/carts/{cartId}/emails)
• Bulk operation URI — N/A

Operations & scopes

• Correct operation attribute per HTTP method (CQRSUpdate → PUT)
• Scope format: cart_write ✅

API Resource properties

• All properties strictly typed (public int $cartId)
• Naming conventions respected
• #[ApiProperty(identifier: true)] on ID property
• #[LocalizedValue] / #[DefaultLanguage] — N/A (no localized fields)

CQRS mapping

• QUERY_MAPPING direction — N/A (no query)
• CQRSCommandMapping direction — N/A (URI param auto-mapped)
• CQRSQuery present on CQRSCreate/CQRSPartialUpdate — N/A
• No SerializedName ✅

Forbidden practices (CI-enforced)

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

Exception handling & validation

• CartException → 422, CartNotFoundException → 404 ✅
• validationContext — N/A (no body fields, no validation groups needed)

Multi-shop

• shopIds — N/A (cart entity has no shop-association DTO property needed here)

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

Integration test

• Extends ApiTestCase
• @depends chain — N/A (single test method)
• Asserts all fields — N/A (204 No Content, no response body)
• testInvalid* method — missing (no coverage for non-existent cartId → 404)
• getProtectedEndpoints() lists the URI ✅
• DatabaseDump::restoreTables() — missing (cart row inserted in test is never cleaned up)
• declare(strict_types=1) present ✅

[PrestaEdit]

Thanks for the thorough review @mattgoud! Addressed all three nits in dc08b06:

• Added declare(strict_types=1); to the CartEmail resource.
• Added testSendUnknownCartReturnsNotFound — PUTs to a non-existent cartId and asserts 404, exercising the CartNotFoundException => 404 mapping.
• The test cart now holds a product, so it's a more representative send-to-customer case.

[mattgoud]

Perfect, all three addressed in dc08b06 and CI is green across the matrix:

• declare(strict_types=1); added to CartEmail.php.
• testSendUnknownCartReturnsNotFound uses MAX(id_cart)+1 for a guaranteed non-existent id and asserts 404, so the CartNotFoundException => 404 mapping is now exercised end-to-end.
• createCustomerCart() now adds a simple, active, in-stock product (nicely filtered out of products with combinations), which is more representative of a real send-to-customer flow.

Thanks for the quick turnaround. LGTM 👍

[github-actions]

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

📋 Summary of changes

This PR adds a single PUT /carts/{cartId}/emails endpoint backed by SendCartToCustomerCommand, letting API clients trigger a "send cart to customer" email without writing to the database. The endpoint uses CQRSUpdate with output: false and allowEmptyBody: true, returning 204 No Content. An integration test (CartEmailEndpointTest) covers the happy path, the unknown-cart 404, and scope enforcement — without actually sending an email (mail sending is disabled).

⏱️ Estimated review time

10–15 minutes — small diff (2 files, ~170 lines), simple action-only endpoint with no CQRS mapping needed.

🎯 Scope

• Exposed operations: PUT (action trigger, no response body)
• CQRS entity: SendCartToCustomerCommand
• Integration test: yes (partial — see notes)

🧱 API Platform / CQRS architecture compliance

URI endpoint name — moderate concern

The established pattern in this module for action sub-resources is a verb or hyphenated action phrase in the URI suffix. For example:

/zones/{zoneId}/toggle-status   ← CQRSUpdate + output:false, same pattern
/modules/{name}/reset           ← CQRSPartialUpdate + allowEmptyBody:true

This PR uses the noun emails:

PUT /carts/{cartId}/emails

The word emails reads as a collection endpoint (a GET would list emails, etc.), not as an imperative trigger. A more self-describing name like /carts/{cartId}/send-to-customer would align with the verb-suffix convention and match the underlying command name.

HTTP semantics — minor concern

CQRSUpdate maps to PUT, which is conventionally idempotent. Sending an email is not idempotent — a second identical request fires a second email to the customer. CQRSCreate (POST) would be more semantically correct for a side-effectful trigger. That said, Zone.php already uses CQRSUpdate + output:false for /toggle-status, so this is a grey area in the module's existing practice. Worth a conscious decision by the maintainers.

allowEmptyBody: true — ✓

Valid and used in several existing resources (Zone.php, ResetModule.php, ShowcaseCard.php, etc.).

Exception mapping — ✓

CartNotFoundException::class => Response::HTTP_NOT_FOUND,
CartException::class => Response::HTTP_UNPROCESSABLE_ENTITY,

Correct order (specific before parent), respects the 404/422 rule from CONTEXT.md.

No CQRSCommandMapping — ✓

The only DTO property is the identifier $cartId. If SendCartToCustomerCommand's constructor parameter is also named cartId, the framework auto-resolves it without an explicit mapping. Verify against the Core command signature.

No custom normalizers/processors — ✓

declare(strict_types=1) — ✓

License header — ✓

💡 Improvement suggestions

1. Rename the URI suffix: Change emails → send-to-customer (or another verb phrase) so the endpoint name is unambiguous and matches the command it wraps.

2. DatabaseDump::restoreTables missing in the test: createCustomerCart() inserts a row into ps_cart (and ps_cart_product). Without a DatabaseDump::restoreTables(['ps_cart', 'ps_cart_product']) call in setUpBeforeClass / tearDownAfterClass, that data persists and may interfere with other test classes that query MAX(id_cart) or similar.

3. createApiClient explicit call: CONTEXT.md notes that getBearerToken already creates an API client on the fly; explicitly calling self::createApiClient(['cart_write']) in setUpBeforeClass is typically only needed for special cases. Remove it unless there is a specific reason.

4. Consider updateItem vs requestApi: The test calls requestApi('PUT', ...) directly, which the guidelines designate as a last resort. If updateItem($url, null, $scopes) works with a null body when allowEmptyBody: true is set, it would be the idiomatic helper. If it doesn't, a short note in the test explaining why requestApi is used here would help reviewers.

5. No testInvalid* / assertValidationErrors: The guidelines require a testInvalid… method for every endpoint. For this particular command, triggering a 422 may require a cart with no customer attached. If no 422 scenario is reachable from the API (e.g. SendCartToCustomerCommand cannot throw CartException), add a brief comment explaining why the test is omitted rather than silently skipping it.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case (/carts/{cartId}/emails)
• Identifier uses domain name + Id suffix (cartId)
• Sub-resources follow parent path (/carts/{cartId}/...)
• URI action suffix uses an action verb — emails reads as a collection noun, not an action trigger (see architecture notes above)

Operations & scopes

• Correct operation attribute per HTTP method (CQRSUpdate → PUT)
• Scope format correct: cart_write

API Resource properties

• All properties strictly typed (public int $cartId)
• Naming conventions respected (no is prefix, no localized prefix)
• #[ApiProperty(identifier: true)] on $cartId
• No localized fields (N/A)

CQRS mapping

• No explicit mapping needed — only identifier property, auto-resolved
• CQRSQuery not required — output: false returns 204 No Content
• No SerializedName used

Forbidden practices (CI-enforced)

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

Exception handling & validation

• CartNotFoundException → 404 ✓
• CartException (parent) → 422 ✓, correct order
• No validationContext — acceptable here since no request body is validated; verify that SendCartToCustomerCommand genuinely cannot throw a constraint violation reachable via this endpoint

Multi-shop

• No shopIds property — cart is not a directly shop-associated entity in the DTO sense ✓

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

Integration test

• Extends ApiTestCase
• getProtectedEndpoints() lists the PUT URI
• declare(strict_types=1) present
• @depends chain — N/A for a single-action endpoint, but note explicitly
• testInvalid* with assertValidationErrors — absent; add or document why N/A
• DatabaseDump::restoreTables() — missing; ps_cart / ps_cart_product are dirtied by the test
• createApiClient explicit call — not needed per guidelines; remove unless justified

[mattgoud]

Following up on the AI pre-review checklist — not every box is ticked, but they are not all equal, so here is my triage so you know what is worth acting on vs noise:

Worth fixing (the one I would actually like addressed):

• DatabaseDump::restoreTables() is missing. The test inserts into cart (and cart_product via updateQty) but never restores them, so the rows leak into other test classes — including any that read MAX(id_cart), which your own testSendUnknownCartReturnsNotFound relies on. 29 endpoint tests in the module already do this. Adding DatabaseDump::restoreTables(['cart', 'cart_product']); in both setUpBeforeClass and tearDownAfterClass would align it. I missed this in my first pass — good catch by the bot.

Design calls, your/maintainers' decision (not blocking):

• URI naming emails vs a verb suffix, and PUT vs POST. The module leans towards verb suffixes for action sub-resources (/modules/{name}/reset, /zones/{id}/toggle-status), so /carts/{cartId}/send-to-customer would read more clearly, and POST would be more honest about the non-idempotent side effect. That said, Zone already uses CQRSUpdate + output:false for an action, so the current choice is defensible. Up to you.
• testInvalid* / 422: only reachable if the endpoint can surface a CartException (e.g. a cart with no valid customer). Either add one case or drop a one-line comment saying why it is N/A here.

Bot noise — safe to ignore:

• "Remove the explicit createApiClient" — this is actually the dominant convention (36 test classes call it in setUpBeforeClass), so keep it.
• validationContext / @depends — N/A for a no-body single-action endpoint, as the bot itself notes.

My approval stands; only the DB cleanup is something I would genuinely like in before merge.

[mattgoud]

Switching my approval back to request-changes to gate the merge on the one real item from the AI pre-review (my earlier comment stands for the full triage).

Blocking:

• Add DatabaseDump::restoreTables(['cart', 'cart_product']); in both setUpBeforeClass and tearDownAfterClass. The test inserts into cart / cart_product and currently never restores them, leaking rows into other test classes — including any reading MAX(id_cart), which testSendUnknownCartReturnsNotFound itself depends on. 29 endpoint tests in the module already follow this pattern.

Everything else (URI naming, PUT vs POST, testInvalid*) is a non-blocking design call, and the bot's "remove createApiClient" suggestion is wrong (it is the dominant convention). Happy to re-approve as soon as the DB cleanup is in.

[PrestaEdit]

Thanks @mattgoud, appreciate the triage.

• DB cleanup — done in d8e64a8: DatabaseDump::restoreTables(['cart', 'cart_product']); now runs in both setUpBeforeClass and tearDownAfterClass, so the inserted rows no longer leak (and the MAX(id_cart) lookup in the 404 test stays reliable).
• 422 case — added a one-line note in the test (2c71cb9) explaining why it's N/A: the only path to a generic CartException is a mail-send failure or a cart whose customer no longer exists, neither of which can be reliably set up through this endpoint in an integration test.
• URI naming / PUT vs POST — happy to switch to /carts/{cartId}/send-to-customer + POST if you'd prefer, but since Zone already uses the same CQRSUpdate + output:false action pattern I've left it as-is. Your call.

[mattgoud]

Re-approving. Both points are in and CI is green across the full matrix:

• DB cleanup (was blocking): DatabaseDump::restoreTables(['cart', 'cart_product']) added to setUpBeforeClass and tearDownAfterClass (d8e64a8). No more row leakage.
• 422 mapping (non-blocking): documented why the CartException => 422 path is not reachable from an integration test (2c71cb9).

Thanks again for the quick turnarounds. LGTM 👍