Skip to content

Remove password confirmation field#11388

Merged
alecslupu merged 18 commits intodevelopfrom
feature/one-password-field
Sep 10, 2023
Merged

Remove password confirmation field#11388
alecslupu merged 18 commits intodevelopfrom
feature/one-password-field

Conversation

@andreslucena
Copy link
Copy Markdown
Member

@andreslucena andreslucena commented Jul 26, 2023 •
edited
Loading

🎩 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
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 28, 2023
View reviewed changes
@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 force-pushed the feature/one-password-field branch from 3d91f1f to deefbbb Compare August 28, 2023 05:55
@andreslucena andreslucena force-pushed the feature/one-password-field branch from deefbbb to b617b76 Compare August 28, 2023 05:58
@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
Copy Markdown
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.

@andreslucena @alecslupu
Clean-up password_required? method
6e6c4eb 
Suggested by code review

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

andreslucena commented Aug 30, 2023

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
Copy Markdown
Contributor

alecslupu commented Sep 10, 2023 •
edited
Loading

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
Copy Markdown
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
@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

1 more reviewer

@alecslupu alecslupu alecslupu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alecslupu alecslupu

Labels

module: core target: user-experience type: change PRs that implement a change for an existing feature

Projects

No open projects
Maintainers' backlog
Archived in project

Milestone

0.28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@andreslucena @alecslupu @github-advanced-security