-
Notifications
You must be signed in to change notification settings - Fork 25
Product category endpoints #104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
e08f39d to
b29f62f
Compare
70f37ab to
01a81dd
Compare
|
While the PR is in draft, I am taking the liberty of removing the review request. |
Oh yes, you're right, I hadn't thought of that 🙏 |
|
To display the list of categories in the response, I created I implemented this control by checking the Thank you! |
| // Enables the ProductCategoryOutputNormalizer | ||
| // for serializing the response of CQRS category endpoints. | ||
| 'use_product_category_normalizer' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options for normalization shouldn't be in this field which is a pure PrestaShop custom field It should be in the normalization context instead
But I think we should be able to handle this use case without relying on a normalizer, but first I have one question: should this endpoint return the full product? or only the part with the categories?
Ideally, we should keep it simple and only return the list of associated categories, the thing is we don't have such a query and the API is only meant to be a bridge to the existing CQRS classes, because of this limiation the list of associated categoruies can only be fetch via GetProductForEditing
Which means the categories should be displayed in the return of this endpoint, but to do that this whole API resource should have the same fields as the PrestaShop\Module\APIResources\ApiPlatform\Resources\Product\Product resource, so it should probably extend it which can be a bit overkill
OR we can decide that this update endpoint doesn't return data and simply indicates if it succeeded by returning a no response 204, and to check the modified content we can use /products/1, if so then the missing mapping needs to be added on PrestaShop\Module\APIResources\ApiPlatform\Resources\Product\Product
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use mapping to handle the categories properly in this PR #116
But it required another modification in the core so that I can rename the categoryId properly We could have kept the id in the sub object, but it doesn't really respect the expected convention (IDs should be prefixed with their associated resource), so I preferred to do this POC to see if we can handle it as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think we should be able to handle this use case without relying on a normalizer, but first I have one question: should this endpoint return the full product? or only the part with the categories?
I believe the response should only include the part with the categories.
Which means the categories should be displayed in the return of this endpoint, but to do that this whole API resource should have the same fields as the PrestaShop\Module\APIResources\ApiPlatform\Resources\Product\Product resource, so it should probably extend it which can be a bit overkill
Yes, I think it's excessive too ;-)
OR we can decide that this update endpoint doesn't return data and simply indicates if it succeeded by returning a no response 204, and to check the modified content we can use /products/1, if so then the missing mapping needs to be added on PrestaShop\Module\APIResources\ApiPlatform\Resources\Product\Product
It's the first solution I implemented, then I added a commit with the creation of the normalizer (this was following our conversation on Slack).
I tried to use mapping to handle the categories properly in this PR #116
But it required another modification in the core so that I can rename the categoryId properly We could have kept the id in the sub object, but it doesn't really respect the expected convention (IDs should be prefixed with their associated resource), so I preferred to do this POC to see if we can handle it as expected
I couldn't understand this point, maybe I didn't translate it well or I'm having trouble following your reasoning 😅
| } | ||
| } | ||
|
|
||
| public function testAssignProductToCategory(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add these endpoints earlier in the workflow, you can pass it the $productId this way you can test these changes on the newly created product instead of hard-coded ID 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, I should make the test testAssignProductToCategory() dependent on the test testPartialUpdateProduct() in order to receive the product id as input, right?
But the category id would still be hardcoded in the method?
|
@Codencode one test is red |
@nicosomb, yes I know, I asked @jolelievre because there's an issue between the documentation and the Rector Dry Run. He will decide how to proceed. Thanks for the heads-up 😃 |
|
Hi @Codencode for this one you can update |
c70e27d to
ced1c41
Compare
|
Hi @jolelievre, I've made the change, I hope I understood correctly 😄. Thanks! |
ced1c41 to
dbd4bf3
Compare
|
Hello @Codencode There's a conflict in |
dbd4bf3 to
5717560
Compare
|
Hello @kpodemski Thank you! |
|
Hello @Codencode 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. |
|
Hello @kpodemski, Thanks! |
How To Test - Product Category Endpoints
Create an API Client with these scopes:
product_writeQuery 1 - Assign Product to Category
Method: POST
URI:
/products/{productId}/assign-to-categoryExample:
/products/1/assign-to-categoryBody
{ "categoryId": 3 }Response
{ "productId": 1, "categories": [ { "id": 3, "name": "Clothes", "displayName": "Clothes" } ] }Query 2 - Set Associated Product Categories
Method: POST
URI:
/products/{productId}/categoriesExample:
/products/1/categoriesBody
{ "categoryIds": [3, 4, 5], "defaultCategoryId": 5 }Response
{ "productId": 1, "defaultCategoryId": 5, "categories": [ { "id": 3, "name": "Clothes", "displayName": "Clothes" }, { "id": 4, "name": "Men", "displayName": "Men" }, { "id": 5, "name": "Women", "displayName": "Women" } ] }Query 3 - Remove All Associated Product Categories
Method: DELETE
URI:
/products/{productId}/categoriesExample:
/product/1/categoriesBody
Nessun body richiesto.
Response