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

Hide the password field when the Notification checkbox is checked on the user account creation form #4266

Closed
herbdool opened this issue Jan 10, 2020 · 17 comments · Fixed by backdrop/backdrop#3040

Comments

@herbdool
Copy link

herbdool commented Jan 10, 2020

It seems even for experienced Backdrop admins the account creation form is somewhat confusing.
(Why a password field, notification checkbox easily missed...)

Some suggestions for improvement were already made here in comments.


Original report:

Description of the need
Currently when an admin adds a user account, the admin has to create a password. It would be better to force the user to change their password when they log in for the first time.

Proposed solution
Have a config option to require a password change.

Inspired by https://www.drupal.org/project/force_password_change but don't need most of the stuff in that module in core. Just the one feature.


PR by @indigoxela: backdrop/backdrop#3040

@indigoxela
Copy link
Member

indigoxela commented Jan 11, 2020

If an admin creates an account and checks "Notify user of new account", this new user gets a one-time login link in a mail.

Note the explanation in that mail:

This link can only be used once to log in and will lead you to a page where
you can set your password. 

The user doesn't get the password directly (which would be a security problem), but gets a hashed link to set his/her own password.
If I get your description right, this is what you're after.

To my understanding the admin has to set something in that field, because the database column is set to "not null".

This handling (exactly the same as in D7) has disadvantages:

  1. The admin sets a value that is never used unless in the database, which might be confusing
  2. If an admin forgets to check "Notify user of ..." the user will never know about the account, getting a notification out requires to create a new account.

I think we could improve this UX for admins.

@herbdool
Copy link
Author

Thanks for the explanation. I don't think I really connected the dots that if the user was notified that they would get a one-time login link even if I had set a password. So looks like we just need to improve the help text.

Something like: "By selecting to notify user of the new account they will receive a one-time login link and be lead to a page where they can set their new password."

@olafgrabienski
Copy link

By selecting to notify user of the new account they will receive a one-time login link and be lead to a page where they can set their new password.

  • We can drop the part "By selecting to notify user of the new account". That information is already in the options's name ("Notify user of new account").
  • Let's mention email as notification medium.

So, here's a new suggestion for the help text:

The user will receive an email with a one-time login link which leads to a page where they can set their new password.

@olafgrabienski
Copy link

If an admin forgets to check "Notify user of ..." the user will never know about the account ...

@indigoxela Good point! At least, I forget quite often to check the "Notify" option. In my opinion, it's easy to miss the option in the UI because the form element doesn't have a label:

screen-backdrop-new-user-account-1

Here's an alternative with a Notification label. I guess it's better visible, isn't it?

screen-backdrop-new-user-account-2

@indigoxela
Copy link
Member

Both suggestions are an improvement for sure (help text and extra "Notification" label).

Maybe we could go further: "Notify user..." checked by default?

Or maybe more like, for example, Nextcloud does it: IF the admin sets a password, no notification mail goes out, IF the admin doesn't set a password, a notification with the one-time link goes out automatically (and the password value is some random string silently added to the field on submit).

Would that change be too big?

@klonos
Copy link
Member

klonos commented Jan 14, 2020

Here's an alternative with a Notification label. I guess it's better visible, isn't it?

Yes, it is better in this case, but the underlying issue is basically #4272, and ideally we will solve that globally, instead of per-case as here.

The issue with single checkboxes in forms ('#type' => 'checkbox') is that their #title property is the actual checkbox label; whereas in multiple checkboxes ('#type' => 'checkboxes') each individual checkbox label is specified in their #options property, leaving #title to be used as a label for the entire group of checkboxes.

Another idea is to move this checkbox outside the fieldset, above the submit button:

Screen Shot 2020-01-14 at 2 32 37 pm

But...

@indigoxela
Copy link
Member

Another idea is to move this checkbox outside the fieldset

I think, that's a good idea, as - to my understanding - the notification isn't an account setting like status and roles are.

What about combining suggestions? Form item outside the fieldset AND an extra label for better visibility AND (possibly) checkbox checked by default. And, of course, a better help text.

@olafgrabienski
Copy link

Another idea is to move this checkbox outside the fieldset

What about combining suggestions? Form item outside the fieldset AND an extra label for better visibility AND (possibly) checkbox checked by default. And, of course, a better help text.

Sounds good! I've got an idea based on your suggestions:

  • Move the "Notify user" checkbox outside the fieldset
  • Place it below the E-mail address input
  • Check the "Notify user" box by default
  • Hide the Password input in this case
  • Show the Password input only if the "Notify user" box gets unchecked
  • And, of course, better help text ;-)

Here's a quick mockup (without extra label and help text for the moment):

Default, password input hidden:
screen-backdrop-new-account-suggestion-a

Notify user box unchecked, password input visible:
screen-backdrop-new-account-suggestion-b

@indigoxela indigoxela changed the title Require users to change password if an admin created their account Better admin UX for account creation form Jan 14, 2020
@herbdool
Copy link
Author

I like @olafgrabienski's suggestion the best. Much better workflow to have the checkbox toggle the password field.

@indigoxela
Copy link
Member

indigoxela commented Jan 17, 2020

As we seem to have consensus, that @olafgrabienski's mockup really nailed it, I've started to work on a PR.

It still needs work. Fixing tests, of course and maybe some changes re help texts.
I have to admit, I'm already happy with the UI.

@indigoxela
Copy link
Member

My PR is ready for testing and review.

Feedback is welcome.

@olafgrabienski
Copy link

@indigoxela I've tested the PR using the sandbox site - works well, and the UI looks really much more clear!

In the help text, I'd suggest to remove the "new" before "password":

The user will receive an email with a one-time login link which leads to a page where they can set their new password.

@indigoxela
Copy link
Member

I'd suggest to remove the "new" before "password":

Agreed, that "new" is somewhat pointless. PR has been updated.

@olafgrabienski
Copy link

PR has been updated.

Thank you! Tested again, works fine!

@jenlampton
Copy link
Member

There's one tiny docblock update needed for this PR, otherwise RTBC!

The docblock for user_pass_required_validate() currently reads as follows:

/**
 * Set a default value if one-time login link goes out,
 * set a form error if not and password value is empty.
 */

I'd like to recommend it be something like this:

/**
 * Element validate handler for the password field.
 *
 * Set a form error when the password field is empty and the the notify option
 * is not set. If the notify option is set, set a default password.
 */

@jenlampton
Copy link
Member

Adding the good first issue label since this final change should be a quick one.

@quicksketch
Copy link
Member

Thanks folks! Great work coming up with a solution that actually reduces the number of fields and makes more sense at the same time! It bothers me that Backdrop required you set a password when making new accounts, just asking for an insecure password so it could be verbalized or sent in manually in an email. This is muuuuch better without requiring a password.

Merged backdrop/backdrop#3040 into 1.x. As this is a UI change (though minor) I'm going not going to backport it into 1.15.x. This will be in 1.16.0 May 15. Thanks!

@jenlampton jenlampton changed the title Better admin UX for setting passwords on account creation form Hide the password field when the Notification checkbox is checked on the user account creation form May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants