feat: reset password feature #756
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/shopware-pwa/shopware-pwa-docs/ilc6mk9te |
|
@mmularski Is "No matching customer for the email..." message returned from Shopware 6 API or it is somehow validated on Shopware PWA end? A common practice is to always return a successful resetting message in a way "If your user account really exists then check your email for the link to reset a password.". No matter if an account exists or not. |
|
It is Shopware API response. |
There was a problem hiding this comment.
Very nice start! Just one thing with checking storefrontUrl in params.
Also, tests are not passing so I cannot see coverage. Please see page for this build: https://app.circleci.com/pipelines/github/DivanteLtd/shopware-pwa/731/workflows/bf914308-47a4-4933-81c0-b1eaafa66940/jobs/747
You should be able to run yarn test locally to see if tests are passing and coverage result :)
Also, I agree with @rmakara and prepared #764 Shopware Task for this, as it's the API part to fix.
@mmularski what are you displaying if operation ends successfully?
| export async function resetPassword( | ||
| params: CustomerResetPasswordParam | ||
| ): Promise<void> { | ||
| if (!params.storefrontUrl) { |
There was a problem hiding this comment.
in case of params being null we should prevent error.
| if (!params.storefrontUrl) { | |
| if (!params?.storefrontUrl) { |
more about: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#optional-chaining
This will require to add unit test invoking method with no params like
const result = await resetPassword(null as any);There was a problem hiding this comment.
- I have added optional chaining here but it makes a problem with branch coverage due to
Optional chaining does not count as branch for coverage istanbuljs/istanbuljs#516
Just added a workaround with /* istanbul ignore next */ FYI :)
This will require to add unit test invoking method with no params like
constresult = await resetPassword(null as any);
I do not get this. null as any will cause TypeError due to inconsistency with CustomerResetPasswordParam or I just do not understand something? :D
- After a successful operation, modal just closes and email is sending. I was modeling on login operation.
There was a problem hiding this comment.
Here's what I meant with this test, turned out it has to be another if statement. What we needed to test:
- invocation with no parameter provided
- invocation without any parameters provided
please take a look at this commit: 29bc328
Why should we test for 2? Because library compiles to JS and someone might unintentionally ignore param for method. We need to be resistant to this kind of behaviour:)
| mockedApiClient.resetPassword.mockImplementationOnce(async () => | ||
| Promise.resolve(undefined) | ||
| ); |
There was a problem hiding this comment.
this is perfectly okay, more educational improvement here you can do
https://jestjs.io/docs/en/mock-function-api#mockfnmockreturnvalueoncevalue
and in case of mocking rejections like in line 513
https://jestjs.io/docs/en/mock-function-api#mockfnmockrejectedvalueoncevalue
There was a problem hiding this comment.
Some good hints. Thank you :)
…wa into feat-reset-password
After successful invocation (no API error) please hide email input and button, the display below message and let the user close the modal by himself. When he'll get this feedback on the action he knows what happened and what should he expect next :) After this, PR is ready as everything else looks 👌 Message:
|
|
Deployment failed with the following error: |
|
@patzick fixed. Now after a successful operation, modal looks like below
|
Changes
Closes #667
Checklist