Merge the Customer endpoints into one domain PR - #428
PrestaEdit wants to merge 10 commits into
Conversation
📋 Summary of changesThis PR consolidates four previously separate Customer PRs into one domain merge. It adds four new endpoints: ⏱️ Estimated review time10–15 minutes — four small resource classes, one test class extension, one new test class; the main complexity is verifying field alignment for the two collection endpoints. 🎯 Scope
🧱 API Platform / CQRS architecture complianceCustomerPrivateNote.php — missing
|
mattgoud
left a comment
There was a problem hiding this comment.
Reviewed against a local install with the branch mounted and every endpoint called. The error mapping is the best of the three Shop/ImageType/Customer merges I have looked at today: 422 on the transformation of a non-guest, 404 on an unknown customer id for all four endpoints, and the Assert\NotNull does fire on the private note. One blocking point, and a few things worth tightening.
Blocking: two routes are reserved and answer nothing
_api_/customer_private_notes/{customerId}{._format}_get GET /customer_private_notes/{customerId}.{_format}
_api_/transform_guest_to_customers/{customerId}{._format}_get GET /transform_guest_to_customers/{customerId}.{_format}
_controller: api_platform.action.not_exposed()
GET /admin-api/customer_private_notes/2
404 {"detail":"This route does not aim to be called.","class":"ApiPlatform\\Metadata\\Exception\\NotExposedHttpException"}
CustomerPrivateNote and TransformGuestToCustomer both carry #[ApiProperty(identifier: true)] while exposing only a PATCH and a PUT. With no item GET declared, API Platform registers one itself so the resource stays addressable, and it answers that 404. Two public URIs end up reserved for nothing, and they are snake_case on top of it, which the URI convention of this repository does not allow anywhere else.
Removing the attribute from both classes is enough here, and I checked that it costs nothing:
routes after removal only /customers/{customerId}/private-notes and /customers/{customerId}/transform-to-customers remain
PATCH /customers/2/private-notes 204 and ps_customer.note really holds the new value
PUT /customers/2/transform-to-customers 422 still, on a customer who is not a guest
PATCH /customers/999999/private-notes 404 still
GET /customer_private_notes/2 404 and now it is a real "no route", not a reserved one
Worth noting for next time: this is not the same fix as the one on #422. There the property is named id, which API Platform treats as the identifier by convention, so it needed an explicit identifier: false. Here the property is $customerId, so dropping the attribute is enough.
Worth fixing
Two ways to say "no private note", two different answers.
PATCH /customers/2/private-notes {} 422 privateNote: This value should not be null.
PATCH /customers/2/private-notes {"privateNote":null} 400 The type of the "privateNote" attribute ... must be string, NULL given
Omitting the field gives the clean validation error, sending an explicit null hits the denormalizer first and surfaces its message. Typing the property ?string and keeping the NotNull would route both through the same 422.
Two semantics an API consumer will misread, and neither is visible from the payload.
totalPaid is total_paid_real, the amount actually received, not the order total. On the demo fixtures it reads "€0.00" on every order while ps_orders.total_paid holds 61.80, 169.90 and so on. That is what GetCustomerOrdersHandler passes and it matches the BO, so nothing to change in the behaviour, but a caller reading totalPaid will assume it is the order total.
/customers/{id}/carts excludes carts that already became an order, because the core calls Cart::getCustomerCarts($id, false). On a customer with five carts, all five converted, the endpoint answers []. That is also the real reason the test can only ever cover the empty case, which is worth writing down next to it.
A one-line docblock on each of the two properties is enough.
requirements: ['customerId' => '\d+'] is on CustomerPrivateNote, TransformGuestToCustomer, Customer and CustomerDetails, but not on CustomerCart and CustomerOrder.
Tests
There is no testInvalid* in either class. One asserting 422 on PATCH {} would lock in the behaviour I verified above, and a second on the null payload would lock in whichever answer you settle on for it.
On the automated pre-review
Its main ask was to verify the field alignment against the core, which it could not do. Done, and it is exact on all nine properties, names and types:
CartSummary::getCartId(): int,getCreationDate(): string,getTotalPrice(): stringOrderSummary::getOrderId(): int,getOrderPlacedDate(): string,getPaymentMethodName(): string,getOrderStatus(): string,getOrderProductsCount(): int,getTotalPaid(): string
customerId is not an orphan either, the live response carries it on every row.
Three of its other points I would set aside:
DateTimeImmutablefor the two date properties: no. The core returns them already formatted as strings, sostringis the correct type. The format is2026-09-18 09:44:06rather than ISO-8601, but that comes from the core and changing it here would not be faithful.transform-to-customersinApiResourceUriTemplateRector::SKIPPED_KEYWORDS: not needed, the route registers exactly as declared and the Rector job is green.- The missing
validationContext: real, but it changes nothing today, and the report says so itself.Assert\NotNullcarries no group, so it fires on thePATCHregardless, which the 422 above confirms.
Expose SetPrivateNoteAboutCustomerCommand through
PATCH /customers/{customerId}/private-note, in a dedicated resource class so
the rich Customer resource is left untouched. The body carries the privateNote
string (mapped by matching field name).
Adds an integration test and a scopes entry.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Expose two customer read endpoints:
- GET /customers/{customerId}/orders (GetCustomerOrders) -> order summaries
- GET /customers/{customerId}/carts (GetCustomerCarts) -> cart summaries
The query results (arrays of summary DTOs) are mapped through the resource via
[_queryResult], like the ShowcaseCard endpoint.
Adds integration tests and reuses the customer_read scope.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PUT /customers/{customerId}/transform-to-customers (TransformGuestToCustomerCommand)
turns a guest customer into a registered one. Added as a standalone resource class
and a separate test to avoid colliding with the in-progress Customer PRs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Expose GetCustomerForAddressCreation as GET /customers/address-creation-info ?customerEmail=... (scope customer_read): return the minimal customer info the address creation flow needs (customerId, firstName, lastName, company). Experimental combination of CQRSGet + QueryParameter (previously only exercised on CQRSGetCollection). Related to PrestaShop/PrestaShop#39630 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Consolidates PrestaShop#218, PrestaShop#225, PrestaShop#243 and PrestaShop#342. PrestaShop#218 and PrestaShop#225 both wrote CustomerEndpointTest.php and conflict on cherry-pick, which is the usual sign that they belonged in one PR. PrestaShop#342 is dropped rather than merged. It exposed GetCustomerForAddressCreation as GET /customers/address-creation-infos?customerEmail=, but that query is already listed in EXCLUDED_CQRS_CLASSES on dev with the USELESS_DUPLICATE reason — and the exclusion is right: GET /customers/search matches on email among other fields and returns idCustomer, firstname, lastname and company, a strict superset of the four fields the dropped endpoint returned. The exclusion entry already exists, so this PR adds nothing for it. The private-note test now asserts its work: the note is the privateNote of the generalInformation of GET /customers/{customerId}/details, so the write finally has a read side to check against. It previously asserted the 204 and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GetCustomerOrders and GetCustomerCarts both return a list of row objects, not a
scalar. QueryResultSerializerTrait only wraps a query result behind the
"_queryResult" key when the result is a scalar, so the
['[_queryResult]' => '[orders]'] / '[carts]' mappings the source PRs relied on
never fired and the responses came back with customerId alone.
Turn both into CQRSGetCollection operations describing one row, which is the
idiomatic shape for /customers/{customerId}/orders and /carts anyway, and rename
the resource classes to the singular accordingly.
The identifier and the requirements kept the two collection routes from being registered at all, so both endpoints answered 404. ProductImageList declares the same shape — a sub-collection under a parent id — with neither, and it works.
bf694c4 to
525b87a
Compare
- Drop the identifier attribute on CustomerPrivateNote and
TransformGuestToCustomer, which made API Platform reserve
/customer_private_notes/{id} and /transform_guest_to_customers/{id}
as not-exposed routes
- Type privateNote as ?string so an explicit null gets the same 422
as an omitted field instead of a 400
- Add the customerId requirement on the orders and carts collections
- Document totalPaid (total_paid_real) and the carts filter
- Cover the invalid private note payloads
525b87a to
9d23e65
Compare
|
Pre-QA OK I pushed the review fixes on this branch myself, so this is a pre-QA: same process as a QA, but I am not setting the QA label. Another QA has to sign off. Tested on: core No linked issue: the How to test section of the description was concrete enough to test directly. Compared statesBoth states come from this branch. Without the fix = the four resource classes put back to their state before the review commit ( Result
The missing The two semantics documented in the review commit hold on the demo data:
Automated tests
Before merge
Raw transcriptsWithout the fix: With the fix: |
|
Pre-QA, Swagger evidence (follow-up to the pre-QA comment above) Replayed through the back-office Swagger UI ( The 4 endpoints, with the fix
Read back with What changes between the two statesWithout the fix = the four resource classes put back to their state before the review commit, cache deleted, same page and authorization.
Every other call answers the same in both states, as listed in the table of the comment above. The two reserved routes ( The curl block was hidden before capturing, response headers are cropped out, and the BO |
|
thanks @PrestaEdit for merging the Customer PRs into one, and for writing down why #342 was dropped. I took the last round of my review myself to move this forward: rebased on 9d23e65 covers every point of my review:
evidence is in the two pre-QA comments above. since I wrote the fix, I'm dismissing my review instead of approving, so this needs another reviewer. |
addressed in 9d23e65, see the comment above. dismissing instead of approving since I pushed the fix myself.
- Map CustomerConstraintException to 422 on the orders, carts and transform endpoints: customer id 0 passes the route requirement and was answering 500 - Apply the CleanHtml constraint of the back-office note form - Singular transform-to-customer URI, skipped by the Rector rule - validationContext on the transform operation - Test the carts endpoint on a real cart, the 404 and the invalid id of the transform endpoint, and the invalid id of orders and carts - Rename tearDownBeforeClass, which PHPUnit never called, so the customer tables are really restored






What this PR does
Merges #218, #225, #243 and #342 into one PR, following the mutualisation done on the Product domain in #410.
Endpoints added
/customers/{customerId}/private-notesSetPrivateNoteAboutCustomerCommandcustomer_write/customers/{customerId}/ordersGetCustomerOrderscustomer_read/customers/{customerId}/cartsGetCustomerCartscustomer_read/customers/{customerId}/transform-to-customersTransformGuestToCustomerCommandcustomer_write#218 and #225 both wrote
tests/Integration/ApiPlatform/CustomerEndpointTest.phpand conflict on cherry-pick — the usual sign that they belonged in one PR.#342 is dropped, not merged
It exposed
GetCustomerForAddressCreationasGET /customers/address-creation-infos?customerEmail=, returning{customerId, firstName, lastName, company}.That query is already listed in
EXCLUDED_CQRS_CLASSESondev, with theUSELESS_DUPLICATEreason — the team had recorded the decision not to expose it. The exclusion holds up:GET /customers/searchtakesphrases[]that match "first name, last name, email, company name and id" and returnsidCustomer,firstname,lastname,companyand more — a strict superset of the four fields, reachable by the same email lookup.Since the exclusion entry already exists on
dev, this PR does not need to add anything for it; the resource and its test are simply not carried over.Tests
The private-note test now asserts its work. It used to check the
204and stop there, because the write had no read side inside its own PR:GET /customers/{customerId}/details— already ondev— returns the note asgeneralInformation.privateNote, so the test now reads it back.CustomerTransformGuestEndpointTestis kept as its own class: it already builds every fixture throughPOST /customersand asserts throughGET /customers/{id}, and it needs acustomer/customer_groupreset that the main suite does not want.How to test
Covered by
CustomerEndpointTestandCustomerTransformGuestEndpointTest.Supersedes
and closes #342 as a duplicate (its query is already excluded on
dev).All four will be closed once the CI is green here.