-
Notifications
You must be signed in to change notification settings - Fork 16
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
Let managers reset user's password #1370
Let managers reset user's password #1370
Conversation
$('#user_password').val(password) | ||
|
||
$(".show-password").on "click", (event) -> | ||
console.log("hey") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops
if $("#user_password").is("input[type='password']") | ||
$('#user_password').prop 'type', 'text' | ||
else | ||
$('#user_password').prop 'type', 'password' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an object is going to be used more than twice it's a good practice to store it into a local variable.
config/locales/es/management.yml
Outdated
password: Contraseña | ||
send_email: Enviar email de reseteo de contraseña | ||
reset_email_send: Email enviado correctamente. | ||
reseted: ¡Contraseña reseteada correctamente! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exclamations, please 😬
282b1c0
to
96cea02
Compare
config/locales/es/management.yml
Outdated
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think reestablecer
would be a better choice than Resetear
. Same at "send_email", "reseted" and "reset_password" strings
logout | ||
|
||
sent_token = /.*reset_password_token=(.*)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😕 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
96cea02
to
55ed507
Compare
@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. |
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:
Similar to what I've said at #1363 (comment)
|
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 🙏 |
9e679f6
to
1d10cef
Compare
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.
1d10cef
to
324ffee
Compare
[1] About the weird password fill behaviour that I reported...
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
References
This PR fullfills and closes consuldemocracy#2500
Objectives
Let the manager change the users' password in two ways:
Visual Changes
Reset password button
![password01](https://user-images.githubusercontent.com/31625251/37462178-4853bf82-2851-11e8-95ad-35a2261393e1.png)
User with email
![password01_1](https://user-images.githubusercontent.com/31625251/37462195-5c920be8-2851-11e8-9ce4-f40837478901.png)
User without email
![password02](https://user-images.githubusercontent.com/31625251/37462203-6378311c-2851-11e8-9a75-e93fe9ddf1a8.png)
-Random password generator and show/hide password
![password02_2](https://user-images.githubusercontent.com/31625251/37462218-72cb32cc-2851-11e8-82ff-88a75c5e86af.gif)
Password saved and print password page
![password03](https://user-images.githubusercontent.com/31625251/37462226-7cf3aacc-2851-11e8-847f-caaac0780f55.png)
How password is printed
![password04](https://user-images.githubusercontent.com/31625251/37462509-7d6d277a-2852-11e8-8763-28a855a75461.png)
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.