Add all combination endpoints - #102
Jeremie-Kiwik wants to merge 8 commits into
Conversation
|
nice PR ! whaou ! 15 endpoints |
|
Hello @Jeremie-Kiwik Thank you for this PR. There are a few errors in the integration tests, looks like some typos in the paths, could you take a look? |
|
@kpodemski : ah sorry, I thought those were internal errors in the deployment of the test environments. I didn’t really look into it any further. |
|
@Jeremie-Kiwik there is a conflict on your PR. |
b23ee33 to
7aefe3d
Compare
|
Hello @Jeremie-Kiwik Thanks for fixing the conflicts, the last step is making sure to have the CI 🟢. Thank you 🙏🏻 |
|
Hello @Jeremie-Kiwik Just a quick heads-up: reviews on pending Admin API PRs will start in the coming days. We first took some time to clarify and unify the Admin API contribution rules and ADR expectations. With that work done, the team will now review existing PRs based on those updates. Please note that, based on the updated standards described here: some aspects of this PR currently do not meet the requirements. For this reason, I've added the Invalid label for now. Someone from the team will take a closer look at the PR and provide concrete suggestions on how it can be adjusted to align with the new guidelines. Thanks for your patience. Feedback will follow directly on the PR. |
|
Hi, |
|
@Jeremie-Kiwik you sill have conflict, sorry. Could you fix it please? |
|
@nicosomb : done. I also ran your new IA context to dig deeper and find some flaws. It should be robust / clean, now. |
|
@Jeremie-Kiwik the PR can't be reviewed in the current state, a merge probably went wrong and many commit from the original branch are now in your history. Can you please rebase? |
abc10d6 to
761e2ed
Compare
|
@Quetzacoalt91 is it better now? |
|
Yep that's better, thanks |
mattgoud
left a comment
mattgoud
left a comment
There was a problem hiding this comment.
thanks for the PR @Jeremie-Kiwik 🙏 reviewing as part of the sheriff rotation. solid work overall — the 15 CQRS command/query mappings all check out, the requirements: ['productId' => '\d+'] added to Product.php are exactly what's needed to disambiguate /products/combinations/... from /products/{productId}, every new resource has its \d+ requirements, scopes/strict_types/exception mappings are consistent, the tests use restoreAllTables(), and the validation unit test is a nice touch. a few things before merge:
1. this renames endpoints that already shipped (v0.3.0 → v0.7.0) — please flag it
the PR restructures released routes:
GET /products/{productId}/combination-ids→/products/{productId}/combinations/idsPOST /products/{productId}/generate-combinations→POST /products/{productId}/combinations- generate body:
groupedAttributes(array of objects) →groupedAttributeIds(map)
that's a breaking change to the public API surface. it's targeting dev and the module is pre-1.0 / the Admin API is still experimental, so it's allowed — but the description has BC breaks: no and there's no CHANGELOG / migration note. could you flag the BC break in the description and add a note for consumers? and it'd be good to get a maintainer (cc the API team) to explicitly sign off on the rename rather than slip it in silently.
2. /images/clears — rename before it ships
the URI /products/combinations/{combinationId}/images/clears reads oddly ("clears" plural) and uses PATCH for a destructive clear-all. cheaper to get right now than after release — consider DELETE .../images or .../images/clear. (see inline)
3. test lost the product_read scope
createApiClient(['product_write']) dropped the product_read the old setup pre-created. not a failure (tokens auto-provision per scope), just a perf/consistency regression — worth restoring both.
nice PR — main ask is just making the BC story explicit. 👍
(verified non-issues while reviewing: the BulkProductCombinations mapping is fine — NormalizationMapper is additive so combinationIds passes through by name; the ?? [] in the generate serializer can't silently generate nothing since NotBlank catches it; the bootstrap require_once is a legit PHPUnit load-order workaround.)
| #[ApiProperty(openapiContext: ['type' => 'object', 'additionalProperties' => ['type' => 'array', 'items' => ['type' => 'integer']], 'example' => ['1' => [2, 3], '2' => [10, 14]]])] | ||
| #[Assert\NotBlank(groups: ['Create'])] | ||
| #[Assert\Type('array', groups: ['Create'])] | ||
| public array $groupedAttributeIds; |
There was a problem hiding this comment.
this is a breaking change vs the released endpoint: the old GenerateCombinations took groupedAttributes as an array of {attributeGroupId, attributeIds} objects, this now takes groupedAttributeIds as a map. fine for a pre-1.0 / experimental API, but please flag the BC break in the PR description + a migration note, since it shipped in v0.3.0–v0.7.0.
| #[ApiResource( | ||
| operations: [ | ||
| new CQRSPartialUpdate( | ||
| uriTemplate: '/products/combinations/{combinationId}/images/clears', |
There was a problem hiding this comment.
/images/clears is an awkward name ("clears" plural) and PATCH is a weak verb for a destructive clear-all. since this hasn't shipped yet, cheaper to fix now — DELETE /products/combinations/{combinationId}/images (or .../images/clear) would read better.
| self::$attributeData[$attribute->name[1]] = (int) $attribute->id; | ||
| } | ||
| } | ||
| self::createApiClient(['product_write']); |
There was a problem hiding this comment.
this dropped the product_read scope the old setup pre-created — the suite still passes (tokens auto-provision per requested scope) but it re-creates an API client on the first read call. worth restoring ['product_write', 'product_read'] for the pre-warm optimization.
📋 Summary of changesThis PR adds a comprehensive suite of combination endpoints: single-combination GET/PATCH/DELETE, paginated list, ID collection, generate (POST), bulk-delete, stock update + stock movements, suppliers read/write, image set + image clear, and two search operations. It replaces five older files ( ⏱️ Estimated review time60–90 minutes — large surface (15+ files), multiple operation types, non-trivial test rewrites. 🎯 Scope
🧱 API Platform / CQRS architecture compliance🔴 Hard Blocker — Custom normalizer still present
🔴 Likely Bug —
|
github-actions
Bot
added
AI reviewed
|
@mattgoud hello, I didn't forget you. I'll do the changes this week, sorry for the delay 🙏. |
|
@mattgoud: it should be ok now! |
|
Heads-up for cross-referencing: a subset of the combination endpoints proposed here has since been merged into
The write side of this PR is not in |
|
Heads up on a small overlap: #410 (centralization of the pending Product-domain endpoints) now exposes Everything else in this PR (combination update/delete/bulk-delete, images, suppliers, stock movements, etc.) remains uncovered and very much wanted. When rebasing, you can drop the
Thanks for the contribution! |
FIX phpstan: remove custom provider + normalize query params Rewrite unit tests without Symfony\Component\Validator (only available in the global PS vendor, not in the module test env) Remove custom processors + more robust code (use Claude AI Pre-Review recommandations) End of Claude proposal changes Last Claude review
… standardize the generate-combinations response envelope
3c285b1 to
8d10730
Compare
|
OK, not an easy fix 😅 As #121 is already merged, a chunk of what this PR originally added is redundant, and #410 (still open) is about to cover another piece of it. Trimmed the PR down accordingly, per PrestaEdit's and @jolelievre's comments above (with Claude's support, I must say) Here we go: Removed — already merged in
|
mattgoud
left a comment
mattgoud
left a comment
There was a problem hiding this comment.
thanks for the trim @Jeremie-Kiwik 🙏 and sorry for the silence on my side, i owed you a reply since june.
first, closing my old review: all 3 points are done ✅ BC break disclosed in the description (and mostly moot now that those routes went out with the prune), /images/clears is now DELETE .../images, and product_read is back in the test setup.
the reduced scope looks right to me: what's left is exactly the part neither #121 nor #410 covers.
on CI: the red PHPStan (9.0.3) is an infra flake, the prestashop/statsforecast zip came down corrupted during composer install. a re-run should clear it, nothing to fix on your side.
a few things on the new state:
1. combinationSuppliers payload is snake_case
the request body exposes supplier_id / currency_id / price_tax_excluded / product_supplier_id. that's UpdateCombinationSuppliersCommand's internal array shape leaking into the public API, everything else we expose is camelCase. the mapper handles this with index placeholders, SearchAlias.php:110-122 is the precedent. (see inline)
2. no validation on the inner supplier items (returns 500)
#[Assert\NotBlank] only guards the outer array. setCombinationSuppliers() reads $productSupplier['supplier_id'] unguarded and hands it to CombinationSupplierAssociation::__construct(int $combinationId, int $supplierId, ...), so {"combinationSuppliers":[{"reference":"x"}]} gives undefined-key + TypeError = 500 instead of 422. needs a nested Assert\All/Assert\Collection or a real nested DTO. (see inline)
3. merge ProductCombinationImagesClear into ProductCombinationImages
both operate on /products/combinations/{combinationId}/images, and the convention here is one class per resource carrying several operations: ProductCategory.php does exactly that (CQRSCreate + CQRSDelete on the same URI), ProductImage.php has 3. drops a file. (see inline)
4. nit: stock-movements param defaults are strings
schema: ['type' => 'integer', 'default' => '5'], the default should be an int, otherwise the generated openapi schema contradicts its own type. (see inline)
rest reads clean, mappings all check out, and the test rework (mutated-tables restore + real stock delta before asserting movements) is a nice improvement 👍
| ], | ||
| CQRSCommandMapping: [ | ||
| '[_context][uriVariables][combinationId]' => '[combinationId]', | ||
| '[combinationSuppliers]' => '[combinationSuppliers]', |
There was a problem hiding this comment.
the payload keys here come straight from UpdateCombinationSuppliersCommand's raw array (supplier_id, currency_id, reference, price_tax_excluded, product_supplier_id), so an internal shape ends up in the public API while everything else we expose is camelCase.
the mapper supports index placeholders, so you can keep the DTO camelCase and translate on the way in:
CQRSCommandMapping: [
'[_context][uriVariables][combinationId]' => '[combinationId]',
'[combinationSuppliers][@index][supplierId]' => '[combinationSuppliers][@index][supplier_id]',
'[combinationSuppliers][@index][currencyId]' => '[combinationSuppliers][@index][currency_id]',
'[combinationSuppliers][@index][reference]' => '[combinationSuppliers][@index][reference]',
'[combinationSuppliers][@index][priceTaxExcluded]' => '[combinationSuppliers][@index][price_tax_excluded]',
'[combinationSuppliers][@index][productSupplierId]' => '[combinationSuppliers][@index][product_supplier_id]',
],src/ApiPlatform/Resources/SearchAlias/SearchAlias.php:110-122 is the in-repo precedent for this. the openapi schema and the how-to-test body below would need the same rename.
| ], | ||
| ])] | ||
| #[Assert\NotBlank(groups: ['Update'])] | ||
| public ?array $combinationSuppliers = null; |
There was a problem hiding this comment.
NotBlank only guards the outer array, nothing validates the items. UpdateCombinationSuppliersCommand::setCombinationSuppliers() reads the keys unguarded:
new CombinationSupplierAssociation(
$this->combinationId->getValue(),
$productSupplier['supplier_id'],
...and the VO signature is __construct(int $combinationId, int $supplierId, ?int $productSupplierId = null). so PATCH {"combinationSuppliers": [{"reference": "x"}]} gives an undefined-key warning then a TypeError, i.e. a 500 where the API should answer 422.
an Assert\All + Assert\Collection on the required keys (or a proper nested DTO) would cover it. worth a test case too.
There was a problem hiding this comment.
OK, I implemented it. Note: I had to duplicate snake + camel keys, as after normalization both are present (same behavior than in src/ApiPlatform/Resources/SearchAlias/SearchAlias.php)
The result would be:
#[Assert\All(constraints: [
new Assert\Collection(
fields: [
'supplierId' => [new Assert\NotBlank(), new Assert\Type('integer')],
// Add supplier_id because after normalization both supplierId and supplier_id are present
'supplier_id' => new Assert\Optional(new Assert\Type('integer')),(Another option would be to add a allowExtraFields: true, but this would accept any unknown value, so I prefer to be deterministic)
Is it OK for you like this ?
|
|
||
| #[ApiResource( | ||
| operations: [ | ||
| new CQRSDelete( |
There was a problem hiding this comment.
this operates on the same URI as ProductCombinationImages (/products/combinations/{combinationId}/images), so it can live in that class as a second operation rather than in its own file. output: false and the exception map are per-operation, so nothing is lost.
precedent: ProductCategory.php carries CQRSCreate + CQRSDelete on /products/{productId}/categories, and ProductImage.php groups get/update/delete on /products/images/{imageId}.
| parameters: [ | ||
| 'limit' => new QueryParameter( | ||
| key: 'limit', | ||
| schema: ['type' => 'integer', 'default' => '5'], |
There was a problem hiding this comment.
nit: 'default' => '5' (and '0' just below) are strings in a schema declared 'type' => 'integer', so the generated openapi contradicts itself. 'default' => 5 / 'default' => 0.
11 participants
EDIT 3 (2026-06-24) — Breaking change disclosure
Flagging explicitly that this PR is a BC breaks. Renames/reshapes some routes and fields that were already released (v0.3.0 → v0.7.0). (thanks @mattgoud for pointing this)
Routes renamed:
GET /products/{productId}/combination-ids→GET /products/{productId}/combinations/idsPOST /products/{productId}/generate-combinations→POST /products/{productId}/combinationsGenerate combinations — request body changed:
groupedAttributes(array of{attributeGroupId, attributeIds}objects) →groupedAttributeIds(map{attributeGroupId: [attributeIds]})Generate combinations — response body changed:
{ "newCombinationIds": [1, 2, 3] }{ "items": [...], "totalItems": N }Combination IDs list — response shape changed:
{ "combinationIds": [1, 2, 3] }[{ "combinationId": 1 }, { "combinationId": 2 }, ...]Combinations list — field renames (on top of the envelope change already disclosed in EDIT 2): the DTO now exposes the underlying Core field names directly instead of translating them:
name→combinationNameattributes→attributesInformationimpactOnPriceTaxExcluded→impactOnPriceSingle combination (get/update) — fields no longer exposed compared to the previous released version:
impactOnPriceTaxIncluded,impactOnUnitPriceTaxIncluded,ecotaxTaxIncluded,productTaxRate,productPriceTaxExcluded,productEcotaxTaxExcluded,coverThumbnailUrl, and the root-levelquantity. This is a scope reduction for this first version (tax-included variants and parent-product price info can be recomputed from the tax-excluded values and the existingProductendpoint); happy to restore any of these if needed.Delete route changed:
PATCH /products/combinations/{combinationId}/images/clears→DELETE /products/combinations/{combinationId}/imagesEDIT 2 (2026-05-12)
I ran the AI Context analysis tool (#188) and
applied the main proposals it raised:
ProductCombinationsList: replaced theCQRSGetwrapper DTO (productId/combinations/totalCombinationsCount) with a properCQRSPaginateoperation returning a standard paginatedenvelope (
items/totalItems). Fixed URI variable resolution by declaring explicituriVariables: ['productId' => new Link(identifiers: ['productId'])]and adding a hidden$productIdproperty on the DTO — without this, API Platform auto-generates theuriVariablesmap from the
identifier: truefield (combinationId), which doesn't match the{productId}path segment and causes a 404.
ProductCombinationIdList: removed the erroneousidentifier: trueon$combinationId(same URI-variable mismatch issue as above).
ProductCombinationStock: removed dead$quantitywrite-only property (output: falseendpoint, unused).
ProductCombinationSuppliers: added missing$combinationSupplierswrite property with@NotBlankvalidation and the correspondingCQRSCommandMappingentry.ProductCombinationsAssociationSearch: movedexceptionToStatusfrom the operation levelto the
#[ApiResource]class level (correct placement per convention).GenerateCombinationsSerializer: added missingdeclare(strict_types=1).stock-movements, so the assertion isalways meaningful rather than conditional on pre-existing data.
The How-to-test section below has been updated to reflect the new paginated response format for
step 4.
EDIT
Changed endpoints to match the new ADR convention
#109
Endpoints summary
GET /products/combinations/{combinationId}
PATCH /products/combinations/{combinationId}
DELETE /products/combinations/{combinationId}
POST /products/{productId}/combinations
GET /products/{productId}/combinations/ids
GET /products/{productId}/combinations
DELETE /products/{productId}/combinations/bulk-delete
PATCH /products/combinations/{combinationId}/stocks
GET /products/combinations/{combinationId}/stock-movements
PATCH /products/combinations/{combinationId}/suppliers
GET /products/combinations/{combinationId}/suppliers
PATCH /products/combinations/{combinationId}/images
DELETE /products/combinations/{combinationId}/images
GET /products/{productId}/combinations/search
GET /products/combinations/associations/search
How to test
Create an API Client with these scopes:
product_read&product_writeRequest an access token
0. Create a product
POST/admin-api/products{ "productType": "combinations", "names": { "fr-FR": "Test combinated product" } }Get the product ID in the response. We will use it as
{productId}for other calls1. Generate product combinations
(
GenerateProductCombinationsCommandcommand)Will add 4 combinations, with:
attribute group 'Size' (
id_attribute_group=1)id_attribute=2)id_attribute=3)attribute group 'Color' (
id_attribute_group=2)id_attribute=10)id_attribute=14)Method:
POSTURI:
/admin-api/products/{productId}/combinationsBody:
{ "groupedAttributeIds": { "1": [2, 3], "2": [10, 14] } }HTTP Code:
201HTTP Body: standard paginated envelope —
itemscontaining 4 combinations, andtotalItemsPlease note the 4
combinationId, we will use them below. Let's call them{combinationId_1}, ...,{combinationId_4}, in ascending order.You should see the combinations in back office too.
2. Get combinations IDs
(
GetCombinationIdscommand)GET/admin-api/products/{productId}/combinations/idsHTTP Code:
200HTTP Body: array of
{ "combinationId": number }with our 4 Ids3. Update a combination
(
UpdateCombinationCommandcommand)PATCH/admin-api/products/combinations/{combinationId_1}{ "default": true, "reference": "REF-001", "gtin": "3519690900332", "isbn": "978-3-16-148410-0", "mpn": "MPN-123", "upc": "72527273070", "impactOnWeight": 0.15, "impactOnPrice": 1.99, "ecoTax": 0.2, "impactOnUnitPrice": 0.3, "wholesalePrice": 12.5, "minimalQuantity": 2, "lowStockThreshold": 5, "availableDate": "2025-12-31T00:00:00+00:00", "availableNowLabels": { "fr-FR": "En stock", "en-GB": "In stock" }, "availableLaterLabels": { "fr-FR": "Bientôt", "en-GB": "Later" } }HTTP Code:
200HTTP Body: updated combination
4. Get all combinations (paginated list)
(
GetEditableCombinationsListcommand)GET/admin-api/products/{productId}/combinationslimit,offset,orderBy,orderWayHTTP Code:
200HTTP Body (standard paginated envelope):
{ "items": [ { "combinationId": 42, "combinationName": "Size: M - Color: Red", "reference": "REF-001", "default": true, "impactOnPrice": 1.99, "quantity": 0, "imageUrl": "", "ecoTax": 0.2, "attributesInformation": [ { "attributeGroupId": 1, "attributeGroupName": "Size", "attributeId": 2, "attributeName": "M" }, { "attributeGroupId": 2, "attributeGroupName": "Color", "attributeId": 10, "attributeName": "Red" } ] } ], "totalItems": 4 }Pagination example:
/admin-api/products/{productId}/combinations?limit=2&offset=05. Get single combination detail
(
GetCombinationForEditingcommand)GET/admin-api/products/combinations/{combinationId_1}HTTP Code:
200HTTP Body: full combination details (pricing, labels, dates, identifiers)
6. Delete a single combination
(
DeleteCombinationCommandcommand)Let's delete the 'L - size' / 'Red - color' combination:
DELETE/admin-api/products/combinations/{combinationId_3}HTTP Code:
204HTTP Body: none
7. Bulk delete combinations
(
BulkDeleteCombinationCommandcommand)Let's delete now all the blue combinations:
(change {combinationId_x} in the body with real values)
DELETE/admin-api/products/{productId}/combinations/bulk-delete{ "combinationIds": [{combinationId_2}, {combinationId_4}] }HTTP Code:
204HTTP Body: none
8. Update combination stock
(
UpdateCombinationStockAvailableCommandcommand)Fixed quantity
PATCH/admin-api/products/combinations/{combinationId_1}/stocks{ "location": "somewhere", "fixedQuantity": 42 }HTTP Code:
204HTTP Body: none
HTTP Code:
422On back-office, you should see a quantity of
42for the combination.Delta quantity
PATCH/admin-api/products/combinations/{combinationId_1}/stocks{ "deltaQuantity": -10 }Same as fixed quantity
On back-office, you should see a quantity of
32for the combination.9. Get stock movements
(
GetCombinationStockMovementscommand)GET/admin-api/products/combinations/{combinationId_1}/stock-movements?limit=5HTTP Code:
200HTTP Body: array of movements (type, dates, ids, deltaQuantity, employeeName)
10. Update suppliers for a combination
(
UpdateCombinationSuppliersCommandcommand)Before testing, suppliers must be linked to the product.
The endpoint to associate suppliers with a product has not been implemented yet. It must be done manually from the back office:
0)Here we go: update information.
PATCH/admin-api/products/combinations/{combinationId_1}/suppliers{ "combinationSuppliers": [ { "supplier_id": 1, "currency_id": 1, "reference": "SUP-REF-001", "price_tax_excluded": "10.50" }, { "supplier_id": 2, "currency_id": 1, "reference": "SUP-REF-002", "price_tax_excluded": "20.00" } ] }HTTP Code:
204HTTP Body: none
Use the GET endpoint below to verify the updated suppliers list.
11. Get suppliers associated to a combination
(
GetCombinationSupplierscommand)GET/admin-api/products/combinations/{combinationId_1}/suppliersHTTP Code:
200HTTP Body: array of suppliers (productSupplierId, productId, supplierId, supplierName, reference, priceTaxExcluded, currencyId, combinationId)
12. Associate images to a combination
(
SetCombinationImagesCommandcommand)First add 3 random images to the product
{productId}, on the back office. We will then add 2 of them to the combinationGet the associated
id_imagein DB. We will use them as{imageId_1},{imageId_2}and{imageId_3}.If you don't have access to the DB, reload the page then inspect the DOM of the image to get the data-id
You will have something like:
Your id is
27PATCH/admin-api/products/combinations/{combinationId_1}/images(replace
{imageId_x}with the real values){ "imageIds": [{imageId_1}, {imageId_3}] }HTTP Code:
200HTTP Body: updated combination (at least combinationId, imageIds)
On back-office, you should see that 2 images are linked to the combination (with a black border around the images)
13. Remove all images from a combination
(
RemoveAllCombinationImagesCommandcommand)DELETE/admin-api/products/combinations/{combinationId}/imagesHTTP Code:
204HTTP Body: none
On back-office, images are no more linked to the combination (no black border around them)
14. Search combinations (scoped to a product)
(
SearchProductCombinationscommand)Will search combinations with for example, a given string in attributes
GET/admin-api/products/{productId}/combinations/search?phrase=rouge&limit=5Note: The search strings are localized. So if you have an English PS, please test
search?phrase=redinstead, like this:/admin-api/products/{productId}/combinations/search?phrase=red&limit=5HTTP Code:
200HTTP Body:
{ "productId": {productId}, "combinations": [ { "combinationId": x, "combinationName": "xxxx" } ] }If you search for a non-existent string,
combinationsshould be empty.15. Search combinations for association (global search)
(
SearchCombinationsForAssociationcommand)Important note: this search DOES NOT search in attribute names; it searches product/combination name and references (like ref, ean13, upc, mpn, isbn, supplier_reference...)
GET/admin-api/products/combinations/associations/search?phrase=REF&limit=5HTTP Code:
200HTTP Body: array of
{ productId, combinationId, name, reference, imageUrl }Or an empty array if nothing is found