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

[Pay on 6/18] When setting an unacceptable password the error message doesn't fit user's expectation #1517

Closed
3 tasks
anthony-hull opened this issue Feb 19, 2021 · 48 comments · Fixed by #3190
Closed
3 tasks
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@anthony-hull
Copy link
Contributor

anthony-hull commented Feb 19, 2021

Platform - version:
Web

Action Performed (reproducible steps):
trigger password reset
make sure you're logged out
navigate to here: http://expensify.cash/#/setpassword/[code here]
make sure you have credentials.login key set to your email address in local storage
send a weak password

Expected Result:
An error that makes sense to the user's action and context

Requirements to be shown:

  • Update the login page to include the password requirements. Otherwise, users can get stuck without ever knowing why.
  • do front end validation so that we don't make the API call to set the password if the user's entry does not meet the requirements.
  • if front end validation passes but there's some other error from the API, we should show that error (today we just spin forever).

Password must include:
• a minimum of 8 characters
• 1 number
• 1 lowercase character
• 1 uppercase character

Actual Result:
message is:

Passwords provided are not acceptable

However to a user's perspective they have only entered one password. This error doesn't make sense to me from that perspective.

Notes/Photos/Videos:

image

Logs - JS/Android/iOS (if applicable):
{"code":666,"jsonCode":402,"type":"Expensify\Libs\Error\ExpError","UUID":"03579dd0-476e-4865-b90b-371af0881280","message":"Passwords provided are not acceptable","title":"Bad password","data":[],"htmlMessage":"","requestID":"08592ec61d000042694e958000000001"}

@marcaaron marcaaron added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 31, 2021
@MelvinBot
Copy link

Triggered auto assignment to @koswalto (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 31, 2021
@MelvinBot
Copy link

Triggered auto assignment to @flodnv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@MelvinBot
Copy link

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@koswalto
Copy link

Sorry, went to unassign myself but tricked Melvin in the process

@thienlnam
Copy link
Contributor

Could we get a copy for this please? @Expensify/marketing

Something to indicate that the entered password does not meet the specified requirements

@jamesdeanexpensify
Copy link
Contributor

Ah, I think you're looking to add the "Waiting for copy" label. I'll do that and it should auto-assign one of us from the Marketing Content team. Thanks!

@jamesdeanexpensify
Copy link
Contributor

Oh weiiiiird it's not in the this repo. I'll take this one! Gimme a few.

@jamesdeanexpensify
Copy link
Contributor

What qualifies a password as "not acceptable" on our end? We should basically be telling them two things (1) what happened that caused the error and (2) what they can do to resolve it. Hope this helps, thank you!

@thienlnam
Copy link
Contributor

@jamesdeanexpensify Thanks for taking this!

It looks like our current requirements for a, acceptable password is that

  • Must be greater than or equal to 8 in length
  • Must contain a lowercase letter, an uppercase letter, and a number

@jamesdeanexpensify
Copy link
Contributor

Can it contain special characters, or only alphanumeric?

@sylveawong
Copy link

sylveawong commented Apr 13, 2021

ooh, i opened an issue for this a while back with copy for what we need to say for the reqs/ what we should be checking for:

https://github.com/Expensify/Expensify/issues/149016

(basically, we should: show reqs from that start, then do front end validation so that we don't make the API call to set the password if the user's entry doesn't meet reqs, and then if there's a different error from the API, show the "friendly" API error text)

(reqs being)

Password must include:
• a minimum of 8 characters
• 1 number
• 1 lowercase character
• 1 uppercase character

@thienlnam
Copy link
Contributor

Oh awesome @sylveawong! Thanks for that - I copied the requirements into the issue and we'll just make this the external issue

@thienlnam thienlnam removed their assignment Apr 20, 2021
@mallenexpensify
Copy link
Contributor

@anthony-hull would you like to work on this issue? If so the job in Upwork will pay $250 and their will be a $150 bonus for proposing the issue (you need to fix the issue in order to get the bonus).

@koswalto assigned back to you, once we here back from @anthony-hull, can you create the Upwork job then hire @anthony-hull if he wants the job?

@koswalto
Copy link

I'm not working on contributor issues @mallenexpensify -- I just did my triage so I'm unassigning myself here 👍

@mallenexpensify
Copy link
Contributor

Sorry about that @koswalto , I jumped the gun

@mallenexpensify mallenexpensify self-assigned this Apr 23, 2021
@anthony-hull
Copy link
Contributor Author

anthony-hull commented Apr 27, 2021

Hi team, yes I would like to work on this issue thank you for getting back to me!

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label May 7, 2021
@MelvinBot
Copy link

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@anthony-hull
Copy link
Contributor Author

HOLD on @anthony-hull accepting the issue.

https://www.upwork.com/jobs/~01b9b67d1f16f3c564

Is the job not already created? I've applied to this one.

@mallenexpensify
Copy link
Contributor

Ooops, good catch @anthony-hull , I forgot the Upwork job was already created :ohnothing:
@anthony-hull you should have everything you need now, post in comment if you don't. @thienlnam will review any PRs once they're ready.
@puneetlath since I've already been working on this and the job is created in Upwork, I'll take over your 'External' assignment. ie. once this issue has been closed for 7 days without regressions, I'll be the one paying the job and bonus

@anthony-hull
Copy link
Contributor Author

I'm happy to start.
I haven't been offered the contract in upwork yet, however.

@mallenexpensify
Copy link
Contributor

Hired @anthony-hull in Upwork, you're all set to start!

@mallenexpensify
Copy link
Contributor

Hi @anthony-hull do you have an update to provide? Anything you're waiting on from us?

@anthony-hull
Copy link
Contributor Author

Hey sorry team. I have been ill over the past week. I have everything I need at the moment. I've started work on this and will provide an update later today.

@anthony-hull
Copy link
Contributor Author

changePassword
Today I have experimented with the UX of setting the password.
In the gif you can see the real time feedback to the user as they compose their password.
It is modelled after the update password form on the settings page. When the password requirements are met it changed the stylings green. The password hint appears when focus is on the first input box and stays shows when there is a password entered which doesn't meet the rules.
The second box will tell the user the passwords must match, which disappears when the passwords match.
I will also make the passwords must match hint stay even onBlur when there is a password entered in the second box but it doesn't match.

Tomorrow I will link this up to the logic of allowing the user to send the password when these checks pass.
This component could be re-used on the change password page as well.

Please let me know what you think of the UX/UI.
Would you prefer the hint to show as written above, in a bulleted list?

@thienlnam
Copy link
Contributor

cc @Expensify/design for UX/UI feedback

@shawnborton
Copy link
Contributor

I wonder if a simpler approach would be to always keep the Your password must have XXXX.... hint text under the first password input, and only make it red after someone has left that input and they didn't meet the criteria.

@anthony-hull
Copy link
Contributor Author

sure. What colour should it be when the user has satisfied the requirements?

@thienlnam
Copy link
Contributor

@anthony-hull I imagine that it can stay red until the user hits 'Set Password' and if it is valid then they should move on to the next screen

@anthony-hull
Copy link
Contributor Author

expensift
Here you can see it will stay red now. I forgot to show making the password match the rules but it won't change back from red once set.

@thienlnam
Copy link
Contributor

Ohh I see what you mean - my bad I didn't realize that there was also another error field for confirming the password. In that case I think it makes sense if it goes back to the original color if the requirements were met. I'm going to fallback to @shawnborton for this one though. The changes are looking good though, could you open up a PR and tag me on it? We'll work on any changes from that

@anthony-hull
Copy link
Contributor Author

Yeah sure I'll put a draft PR in

@shawnborton
Copy link
Contributor

Can you make the small hint text under the input use color.textSupporting until it needs to turn red for error validation?

For the second input, I don't think it makes sense to immediately show a "Passwords must match" error - the user hasn't even had a chance to type the password at that point. Can we wait until they are done typing to validate the field, and then show the error if they don't match?

@anthony-hull anthony-hull mentioned this issue May 27, 2021
5 tasks
@anthony-hull
Copy link
Contributor Author

Thanks for the feedback @shawnborton . I've made the changes you suggested.
As @thienlnam suggested I've made a draft PR with the changes in so you can have a play with how the UX works and see what you think.

@anthony-hull
Copy link
Contributor Author

@anthony-hull
Copy link
Contributor Author

I think it would be a good idea to re-use this component for the reset password page to improve the UX of the password rules there as well.

@mallenexpensify mallenexpensify self-assigned this Jun 10, 2021
@mallenexpensify mallenexpensify changed the title When setting an unacceptable password the error message doesn't fit user's expectation [Pay on 6/18] When setting an unacceptable password the error message doesn't fit user's expectation Jun 15, 2021
@mallenexpensify
Copy link
Contributor

Paid in Upwork with bonus for reporting the issue. Thanks @anthony-hull !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.