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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove password confirmation field #11388

Merged
merged 18 commits into from Sep 10, 2023
Merged

Remove password confirmation field #11388

merged 18 commits into from Sep 10, 2023

Conversation

andreslucena
Copy link
Member

@andreslucena andreslucena commented Jul 26, 2023 •
edited

馃帺 What? Why?

One year ago, we received a PR to disable the password confirmation field and have a "show password" button, to bring a more modern experience, solve problems like people having to add the same password twice correctly and so on.

馃搶 Related Issues

Testing

Try to set up the password on:

  • User sign-up
  • User log in
  • User log in (modal)
  • Forgot password
  • Admin/forced password change
  • Account/password change
  • Set password on invitation

馃摲 Screenshots

User sign-up

Screenshot of the user sign-up form

User log in

Screenshot of the user log in form

User log in (modal)

Screenshot of the user log in modal form

Forgot password

Screenshot of the forgot password form

Admin/forced password change

Screenshot of the Admin/forced password change form

Account/password change

Screenshot of the Account/password change form

Set password on invitation

Screenshot of the "Set password on invitation" form

鈾ワ笍 Thank you!

@andreslucena andreslucena marked this pull request as ready for review July 26, 2023 08:43
@andreslucena andreslucena marked this pull request as draft July 26, 2023 08:43
@andreslucena andreslucena self-assigned this Jul 26, 2023
@andreslucena andreslucena force-pushed the feature/one-password-field branch 4 times, most recently from 7445340 to fa58250 Compare July 28, 2023 15:26
@andreslucena andreslucena force-pushed the feature/one-password-field branch 2 times, most recently from b175d76 to b01314a Compare August 23, 2023 16:04
@andreslucena andreslucena added target: user-experience module: core type: change PRs that implement a change for an existing feature labels Aug 23, 2023
@andreslucena andreslucena added this to the 0.28.0 milestone Aug 23, 2023
@andreslucena andreslucena marked this pull request as ready for review August 28, 2023 06:48
@andreslucena andreslucena mentioned this pull request Aug 29, 2023
Closed
@andreslucena andreslucena requested a review from a team August 29, 2023 08:56
alecslupu
alecslupu reviewed Aug 29, 2023
View reviewed changes
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

I have tested most of the happy paths (user clicks to signup, user is being invited to admin).

While checking i could see that we may have another place where the password confirmation may not be needed. I wonder if system admin needs to carry the password validation ...

OFC is not needed to be done in this PR.

Still pending more intensive user review from my end.

decidim-core/lib/devise/models/decidim_validatable.rb Outdated Show resolved Hide resolved
@andreslucena @alecslupu
Clean-up password_required? method
6e6c4eb 
Suggested by code review

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
@andreslucena
Copy link
Member Author

I have tested most of the happy paths (user clicks to signup, user is being invited to admin).

Great! We should be special careful with the help text (that is still there after submitting) and also the error messages (that kicks in correctly and so on).

While checking i could see that we may have another place where the password confirmation may not be needed. I wonder if system admin needs to carry the password validation ...

OFC is not needed to be done in this PR.

I actually had doubts regarding this one. On one hand, you know that I'm a fan of making it consistent all the time, but in this particular case one of the inputs for setting up this password is the CLI. That's hidden too, and I don't think that we should implement this there. You can check it out with bin/rails decidim_system:create_admin. I should have mentioned it as you can't read my mind (yet).

@alecslupu
Copy link
Contributor

alecslupu commented Sep 10, 2023 •
edited

Form testing session

  • User sign-up
  • User log in
  • User log in (modal)
  • Forgot password
  • Admin/forced password change
  • Account/password change
  • Set password on invitation

alecslupu
alecslupu approved these changes Sep 10, 2023
View reviewed changes
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

LGTM

@alecslupu alecslupu merged commit 1f4e94d into develop Sep 10, 2023
93 checks passed
@alecslupu alecslupu deleted the feature/one-password-field branch September 10, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecslupu alecslupu alecslupu approved these changes

Assignees

@alecslupu alecslupu

Labels
module: core target: user-experience type: change PRs that implement a change for an existing feature
Projects
Maintainers' backlog
Archived in project
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andreslucena @alecslupu