Skip to content
This repository was archived by the owner on Dec 12, 2018. It is now read-only.

added password reset#8

Open
nopolabs wants to merge 1 commit intostormpath:masterfrom
nopolabs:add-password-reset
Open

added password reset#8
nopolabs wants to merge 1 commit intostormpath:masterfrom
nopolabs:add-password-reset

Conversation

@nopolabs
Copy link

Added password reset to the sample app.
per issue #6

@robertjd
Copy link
Member

Thanks for the PR! In reviewing it I found some things we need to change:

  • Errors from sendPasswordResetEmail are being thrown. The error should be handled and displayed to the user. If the error’s status code is 404, it means that no account was found for the given email address.
  • We should put the required attribute on the email input field, so that HTML5 form validation will kick in if the form is submitted without an email address
  • From a UX perspective: it doesn’t feel that intuitive to have to put in my email first and then click “forgot password” - I’m used to doing it the other way around. Thoughts @rdegges ?
  • The message “reset email sent” should be spiced up a bit, i.e. “An email with password reset instructions has been sent to foo@bar.com”. We should also change the banner color to be something other than red
  • While not a new issue in this PR, the Stormpath client instance, spClient, is being constructed on every request. It should only be created once and shared by the entire process.

@nopolabs
Copy link
Author

I agree with your first two points.

On the fence with respect to the third, this app is a minimal demonstration of the Stormpath API, not so much of good UX.

I considered including the email address in the "reset email sent" message and decided against it since it could potentially leak an email address to someone who requested a password reset and provided a username.

I think the last point is a good one, this is a common flaw in demo apps and one prone to cut and paste replication.

Ultimately I think that sendPasswordResetEmail should be a method in the passport-stormpath module, but that seemed beyond the scope of what I wanted to do last night ;)

Take what you like.

@hijak
Copy link

hijak commented Sep 28, 2014

ah so close :(

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants