Let managers reset user's password #1370

iagirre · 2018-03-15T14:27:20Z

References

This PR fullfills and closes consuldemocracy#2500

Objectives

Let the manager change the users' password in two ways:

Send an email to the registered email so that the user can be able to change the password by themselves.
Change the password using a form. The manager can write it in plain text or generate a random value. When the password is saved, a button to print is enabled.

Visual Changes

Reset password button
User with email
User without email

-Random password generator and show/hide password

Password saved and print password page
How password is printed

Notes

The "Reset password" button (next to "Change user") is placed with a float. When the PR #1363 merges with this one it should be placed next to "Edit account", as expected.

MariaCheca · 2018-03-16T17:16:37Z

app/assets/javascripts/managers.js.coffee

+ $('#user_password').val(password)
+
+ $(".show-password").on "click", (event) ->
+ console.log("hey")


MariaCheca · 2018-03-16T17:17:54Z

app/assets/javascripts/managers.js.coffee

+ if $("#user_password").is("input[type='password']")
+ $('#user_password').prop 'type', 'text'
+ else
+ $('#user_password').prop 'type', 'password'


If an object is going to be used more than twice it's a good practice to store it into a local variable.

MariaCheca · 2018-03-16T17:20:15Z

config/locales/es/management.yml

+ password: Contraseña
+ send_email: Enviar email de reseteo de contraseña
+ reset_email_send: Email enviado correctamente.
+ reseted: ¡Contraseña reseteada correctamente!


No exclamations, please 😬

iagirre · 2018-03-19T08:36:37Z

I made the requested changes and changed the UI a little bit.

This is the new form when the user has an email:

And this is the form when the user doesn't have an email:

bertocq · 2018-03-19T09:45:02Z

config/locales/es/management.yml

@@ -5,13 +5,26 @@ es:
 unverified_user: Solo se pueden editar cuentas de usuarios verificados
 show:
 title: Cuenta de usuario
+ edit:
+ title: 'Editar cuenta de usuario: Resetear contraseña'


I think reestablecer would be a better choice than Resetear. Same at "send_email", "reseted" and "reset_password" strings

bertocq · 2018-03-19T09:49:08Z

spec/features/management/account_spec.rb

+ logout
+
+ sent_token = /.*reset_password_token=(.*)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1]


I wouldn't test here that the reset password form works, that's already tested at https://github.com/AyuntamientoMadrid/consul/blob/master/spec/features/users_auth_spec.rb#L333-L353 instead the objective of this scenario is to check that the email is sent

bertocq · 2018-03-19T09:53:27Z

spec/features/management/account_spec.rb

+ expect(page).to have_content 'Password reseted successfully'
+ expect(page).to have_css("a[href='javascript:window.print();']", text: 'Print password')
+ expect(page).to have_css("div.for-print-only", text: /Password:/, visible: false)


Are we really checking here that the print functionallity works? Or just that the links are present?

Maybe it would be better to use the "manually set password" scenario to be able to correctly check that when printing we're showing the same password that the manager typed

I changed that spec, and now the test sets a password "manualy" and then checks that the new password is in the page.
However, I didn't merge two specs together (the one that sets the password manually and this one), because in my opinion both of them check different things: one checks that the password has been correctly set and the user has no problems to log in after that; the other one checks that after saving the password, the admin has the option to print it (looking for the button) and the written password is present only for printing (I couldn't find any way to check the print dialog 😕 )

Yes, I expressed myself poorly. I didn't meant to merge both scenarios (the Manager changes the password by hand (writen by them) and The password is printed, but to use a manually written password on the The password is printed scenario to be able to check it in the printed view (as you correctly did 👏 )

The tests that will check the featute is working well added. There are four test: 1. Checks that the emails are been send to the user. The test looks for the link in there and takes the token. Visits the page and changes the password. 2 and 3. Both change the password by hand. One uses a password written by the manager, whilst the other uses the 'Generate random password' option. Both tests check that the user can sign in with the new passwords. 4. Checks that the password can be printed when it is saved.

The controllers, models and routes changes has been added.

A new button has been added to the management account page. When it is clicked, the reset password options are shown. If the user has an email, a button to send a reset password email appears. If there is no email, a form to reset the password by hand appears. When the password has been changed by hand, it can be printed using a button in the screen.

bertocq · 2018-03-21T08:51:50Z

@iagirre it's always a good idea to comment on the PR so we can notice requested changes has been applied 👍 we don't cycle through all pending changes PR's refreshing to see if there are changes.

bertocq · 2018-03-21T20:42:22Z

The existing user experience for the scenario when the user has an email is confusing:

As the "Save" and "Send reset password email" are "final actions" that will leave the current view, but have different colors and positions (bottom seems like final, right like an additional step)

I'll try here (instead of messing with the issue even more) to clarify what's expected There are two excluding paths/options:

Reset by email
Reset manually

Similar to what I've said at #1363 (comment)

The "Change user" link should be in the bottom left corner of the grey square, as a red link (check the issue first mockup)
Although the "Reset Password" button was requested as it appears (ignoring a similar feature done by raul) it should be removed from the grey square.
A new submenu "Reset Password via Email/Restablecer contraseña via email" should be added under the "Edit user account" menu. And the associated view will only display the current "Send reset password email" button that is already been developed.

A second new submenu "Reset Password Manually" should be added under the "Edit user account" menu. And the associated view will only display the password reset form that has already been developed. Also when arriving to this form the password field should not be already filled with a value (to avoid confusing the user/manger into thinking that it was the current user password), the same applies to the "Back" button from the "Print password view", as it returns to the form and 12345678 password appears in the input ... instead of the one that we manually set
Random passwords don't need to have strange characters like gyy5!27e, its better a large one that an "impossible to read by humans" one https://xkcd.com/936/. If we are forced to generate a random value in JS, just use alphanumeric characters (with upper/lower case letters if you want more combinations) and at least 10 or 12 chars instead of 8

iagirre · 2018-03-22T11:25:25Z

I've just made the push to update this PR. I added the submenu with the two options to change the password.

I also changed the random password generator so that it doesn't use characters like $ or !, but 12 alphanumeric chars with some capital letters (I maintained the same char group without ambiguous characters like 1, l or I).

NOTE: I know that the 'Change user' link is still unchanged, but @raul-fuentes is modifing the same files and he did it, so when both PR are merged the link will appear red and in the bottom of the box (I said this in order to avoid conflics when both PR will be merged, but If I should also change the link, let me know and I will).

NOTE 2: In my computer the form to change the password always appears unfilled 🤔 so I couldn't prove if the solution given works correctly. Let me know if not 🙏

A submenu has been added to the side menu's 'Edit user account' option. This submenu has two options: - Reset password via email: an email is send so that the user can change their password by themselves. - Reset password manually: the manager has to write the password manually (or generate a random one). The random password generator has also been changed so that the passwords generated don't contain characters like $ or !. It uses some capital letters, some other lower case letters and some numbers. Ambiguous characters like 1, l, I has been removed.

The back button when the user changes the password (in the print password page) redirects to the edit manually page. The routes to access those pages has been added.

The specs has been updated accordingly with the new UI.

bertocq · 2018-03-22T21:55:47Z

[1] About the weird password fill behaviour that I reported...

Also when arriving to this form the password field should not be already filled with a value (to avoid confusing the user/manger into thinking that it was the current user password), the same applies to the "Back" button from the "Print password view", as it returns to the form and 12345678 password appears in the input ... instead of the one that we manually set

I realized it was a chrome plugin I have (enabled also for private sessions) that auto-fills password fields 🙈 my bad!

[2] Sorry but spec/features/management/account_spec.rb:15 spec is failing. I guess raul's fix will do the trick https://github.com/AyuntamientoMadrid/consul/pull/1363/files#diff-7f5afe0b6447090de432c81501a61459R14 we can just assume by merging both branches it will be solved 👍

Thanks for the changes in the menus, I like that there's a dropdown for the new submenus 👏 and also for the neat idea (I maintained the same char group without ambiguous characters like 1, l or I). on the random password generation.

bertocq

👍

MariaCheca suggested changes Mar 16, 2018

View reviewed changes

iagirre force-pushed the 2500-let-managers-reset-users-password branch 2 times, most recently from 282b1c0 to 96cea02 Compare March 19, 2018 08:34

bertocq reviewed Mar 19, 2018

View reviewed changes

bertocq suggested changes Mar 19, 2018

View reviewed changes

iagirre added 3 commits March 19, 2018 12:04

Add the backend functionality for the feature

a29d43f

The controllers, models and routes changes has been added.

iagirre force-pushed the 2500-let-managers-reset-users-password branch from 96cea02 to 55ed507 Compare March 19, 2018 11:07

iagirre force-pushed the 2500-let-managers-reset-users-password branch from 9e679f6 to 1d10cef Compare March 22, 2018 11:26

iagirre added 3 commits March 22, 2018 15:18

Update backed to work with new UI

87b23f3

The back button when the user changes the password (in the print password page) redirects to the edit manually page. The routes to access those pages has been added.

Update specs to work with the new UI

324ffee

The specs has been updated accordingly with the new UI.

iagirre force-pushed the 2500-let-managers-reset-users-password branch from 1d10cef to 324ffee Compare March 22, 2018 14:18

raul-fuentes mentioned this pull request Mar 22, 2018

Added method for managers to update a user's email #1363

Open

bertocq approved these changes Mar 22, 2018

View reviewed changes

bertocq merged commit ceeb110 into AyuntamientoMadrid:master Mar 22, 2018

iagirre mentioned this pull request Mar 23, 2018

Let managers reset user's password consuldemocracy/consuldemocracy#2548

Merged

aitbw deleted the 2500-let-managers-reset-users-password branch April 5, 2018 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let managers reset user's password #1370

Let managers reset user's password #1370

iagirre commented Mar 15, 2018

MariaCheca Mar 16, 2018

MariaCheca Mar 16, 2018

MariaCheca Mar 16, 2018

iagirre commented Mar 19, 2018

bertocq Mar 19, 2018

bertocq Mar 19, 2018

bertocq Mar 19, 2018

iagirre Mar 19, 2018

bertocq Mar 19, 2018

bertocq commented Mar 21, 2018

bertocq commented Mar 21, 2018

iagirre commented Mar 22, 2018

bertocq commented Mar 22, 2018

bertocq left a comment

		logout

		sent_token = /.reset_password_token=(.)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1]

Let managers reset user's password #1370

Let managers reset user's password #1370

Conversation

iagirre commented Mar 15, 2018

References

Objectives

Visual Changes

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iagirre commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertocq commented Mar 21, 2018

bertocq commented Mar 21, 2018

iagirre commented Mar 22, 2018

bertocq commented Mar 22, 2018

bertocq left a comment

Choose a reason for hiding this comment