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

Password reset flow #147

Merged
merged 15 commits into from
Sep 6, 2019
Merged

Password reset flow #147

merged 15 commits into from
Sep 6, 2019

Conversation

dominik-zeglen
Copy link
Contributor

@dominik-zeglen dominik-zeglen commented Sep 2, 2019

I want to merge this change because it adds password resetting.
Resolves #139

Screenshots

Screenshot 2019-09-03 at 15 45 35

Screenshot 2019-09-03 at 15 45 44

Screenshot 2019-09-03 at 15 46 03

Pull Request Checklist

  1. All visible strings are translated with proper context.
  2. All data-formatting is locale-aware (dates, numbers, and so on).
  3. Translated strings are extracted to .pot file.
  4. Number of API calls is optimized.
  5. The changes are tested.
  6. Type definitions are up to date.
  7. Changes are mentioned in the changelog.

Sorry, something went wrong.

@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #147 into master will decrease coverage by 0.35%.
The diff coverage is 56.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   74.41%   74.05%   -0.36%     
==========================================
  Files         507      522      +15     
  Lines        5065     5176     +111     
  Branches      867      875       +8     
==========================================
+ Hits         3769     3833      +64     
- Misses       1156     1202      +46     
- Partials      140      141       +1
Impacted Files Coverage Δ
src/storybook/stories/auth/LoginPage.tsx 83.33% <ø> (ø) ⬆️
src/intl.ts 100% <ø> (ø) ⬆️
src/mutations.tsx 11.11% <0%> (ø)
src/queries.tsx 6.06% <0%> (+0.17%) ⬆️
...onents/NewPasswordPage/NewPasswordPage.stories.tsx 100% <100%> (ø)
...components/ResetPasswordPage/ResetPasswordPage.tsx 100% <100%> (ø)
src/storybook/config.js 100% <100%> (ø) ⬆️
src/auth/components/LoginPage/LoginPage.tsx 100% <100%> (+10%) ⬆️
...etPasswordSuccessPage/ResetPasswordSuccessPage.tsx 100% <100%> (ø)
...rdSuccessPage/ResetPasswordSuccessPage.stories.tsx 100% <100%> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17cba3b...1dae9a7. Read the comment docs.

@maarcingebala
Copy link
Member

Few issues:

  • There is no error handling in the "reset-password" view. Some errors that I've encountered so far:

    • in the email field - if the given email doesn't exist
    • in the redirectUrl field when the server isn't configured to accept given URL we'll get an error like "localhost:9000 this is not valid storefront address.". While this error doesn't make sense for the user, we should catch this error and display some message anyway e.g. "Failed to send the email, please contact the administrator or try again later."
  • In the "new-password" view the "New password" input should get the focus first, currently the "Confirm password" inputs gets it.

  • Joining URL is broken - http://localhost:9000//new-password/ - do we use any lib for joining URL or we do it manually?

@dominik-zeglen dominik-zeglen merged commit 0f4bdab into master Sep 6, 2019
@maarcingebala maarcingebala deleted the add/reset-password branch January 9, 2020 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add views to set/reset password
2 participants