Skip to content

Conversation

@Kaikina
Copy link

@Kaikina Kaikina commented Dec 3, 2025

Questions Answers
Description? Add partial update for tax rules group on status property (enabled)
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? NA
Sponsor company Evolutive
How to test? Try via the swagger doc to edit the status of an existing tax rule group. Switch it to disabled then enabled.

@ps-jarvis
Copy link

Hello @Kaikina!

This is your first pull request on ps_apiresources repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

@github-project-automation github-project-automation bot moved this to Ready for review in PR Dashboard Dec 3, 2025
@kpodemski
Copy link
Contributor

Hello @Kaikina

As visible in the CI, the name you chose for one of the endpoints is invalid, it didn’t follow requirements from ADR. Could you make sure to fix it?

@ps-jarvis ps-jarvis moved this from Ready for review to Waiting for author in PR Dashboard Dec 15, 2025
Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

Blocking since CI is red, details in the message above.

@Kaikina
Copy link
Author

Kaikina commented Dec 15, 2025

Hello @Kaikina

As visible in the CI, the name you chose for one of the endpoints is invalid, it didn’t follow requirements from ADR. Could you make sure to fix it?

I can change it, but that's kind of a weird issue I think, as the endpoint is not to update several statuses but only one

@kpodemski
Copy link
Contributor

cc @jolelievre

@Kaikina
Copy link
Author

Kaikina commented Dec 15, 2025

I think the problem is that the path corresponding to this CQRS command cannot fully respect the REST approach, as this patch method directs the user to the update of one specific property.
Here update-status does not indicate the name of a resource but rather an action, which is not really RESTfull convenient.
There's already a PATCH endpoint for the TaxRulesGroup Object, so I don't know if this one should not be just simply fully omitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Waiting for author

Development

Successfully merging this pull request may close these issues.

3 participants