Skip to content
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

[API] Adjust reset password requests #12420

Merged
merged 1 commit into from Mar 5, 2021
Merged

Conversation

lchrusciel
Copy link
Member

@lchrusciel lchrusciel commented Mar 4, 2021

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Continuation of #12391

@lchrusciel lchrusciel requested a review from a team as a code owner March 4, 2021 17:06
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Mar 4, 2021
@@ -15,15 +15,15 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="https://api-platform.com/schema/metadata https://api-platform.com/schema/metadata/metadata-2.0.xsd"
>
<resource class="Sylius\Bundle\ApiBundle\Command\ResetPassword" shortName="ResetPassword">
<resource class="Sylius\Bundle\ApiBundle\Command\ResetPassword" shortName="ResetPasswordRequest">
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave as it was, now shortName reflects only specific (request) action, as before it reflected for resource/action that were for reset password

Copy link
Member

Choose a reason for hiding this comment

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

I would be also for changing the command name, now it is quite confusing

@@ -61,7 +61,7 @@ public function iWantToLogIn(): void
*/
public function iWantToResetPassword(): void
{
$this->request = Request::create('shop', 'request-reset-password', 'Bearer');
$this->request = Request::create('shop', 'reset-password-requests', 'Bearer');
Copy link
Contributor

Choose a reason for hiding this comment

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

Well I would call here also @GSadee as, first version was reset-password-request, then it was suggested to change to reqest-reset-password and PATCH reset-password/{token} now it is changed to reset-password-requests (plural form), and from what I see in our task there was suggested to use singular reset-password-request

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I got a little lost in what these endpoints should look like, the current form is consistent with endpoints like apply-coupon or change-quantity, but these ones should be based on the API resource, so this is a good change.

@GSadee GSadee merged commit 711950d into Sylius:master Mar 5, 2021
@GSadee
Copy link
Member

GSadee commented Mar 5, 2021

Thank you, Łukasz! 🥇

@lchrusciel lchrusciel deleted the reset-password branch March 5, 2021 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants