Add separate page for unlocking another user's account #2561
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrislo
approved these changes
Nov 30, 2023
Since `UserPolicy#unlock?` is currently aliased to `UserPolicy#edit?` this doesn't make any practical difference, but since we've defined `UserPolicy#unlock?` I think it makes sense to add this condition in case the policy implementation changes in the future.
Trello: https://trello.com/c/P1w2E6MO This is a step on the journey to move the edit user page to use the GOV.UK Design System. The new design calls for separate pages for different operations on a user and this is the next (and possibly last!) one. The new `app/views/users/unlockings/edit.html.erb` template is based on other similar pages which we've recently extracted, e.g. `app/views/users/invitation_resendings/edit.html.erb` where there aren't any form fields; just a button. Previously the "Unlock account" button on the "edit user" page submitted a POST request directly to `UsersController#unlock`. Now I've moved `UsersController#unlock` to `Users::UnlockingsController#update` and a link on the "edit user" page takes you to a new page which has a "Unlock account" button which submits a request to that new action. There weren't any tests in `UsersControllerTest` for this behaviour, so I had to write `Users::UnlockingsControllerTest` from scratch, but I based it closely on other similar controller tests. Luckily there was one integration test (`test/integration/user_locking_test.rb`) covering the behaviour which I've been able to fixup to work with the new UI. Rather than creating a combinatorial explosion of tests in `Users::UnlockingsControllerTest` relating to whether a user with all the different roles can resend an invitation to another user with all the different roles, I've resorted to stubbing `UserPolicy.new` and `UserPolicy#unlock?`. Although this is a bit ugly, since `UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a pragmatic option. Since the `Users::UnlockingsController#update` action calls a bang method on the instance of `User` and doesn't check for errors, there's no need to include an error summary component in the page. The copy on the new "unlock account" page only really makes sense in the context of a user whose account has been locked and indeed the link to this page will only be displayed on the "edit user" page in this scenario, so I've added a before action to redirect back to the "edit user" if someone tries to view the page for a user whose account is not locked. Note that I've used "unlockings" vs "unlocks" in the routes & controller name to avoid clashing with the existing Devise-generated routes for a user to unlock their own account, even though we don't include links to this in the unlock instructions email, so they are currently unused. It might make more sense to see if we can disable them and then rename the routes & controller added in this commit, but I'm going to leave that for now.
This method is provided by Devise's Lockable module and reads its value from the Devise configuration. The value is returned as an `ActiveSupport::Duration` and so it can be converted into a human-readable string by calling `ActiveSupport::Duration#inspect`. Thus these changes make the copy resilient to changes in that configuration and avoid a small amount of duplication. Note that these changes won't currently change what is displayed, because `Devise.unlock_in` is currently configured to be `1.hour`.
While this is a bit orthogonal to the current PR, I thought it was worth bringing these usages into line with the changes I made in the previous commit. This method is provided by Devise's Lockable module and reads its value from the Devise configuration. The value is returned as an `ActiveSupport::Duration` and so it can be converted into a human-readable string by calling `ActiveSupport::Duration#inspect`. I think it's clearer to use `User.unlock_in` vs `Devise.unlock_in`, because the `Devise::Lockable` module is included in the `User` model. I think using `ActiveSupport::Duration#inspect` rather than reconstructing the duration string as per the previous implementation of `EventLog::LOCKED_DURATION` is a lot simpler and easier to read. Note that these changes won't currently change what is displayed, because `Devise.unlock_in` is currently configured to be `1.hour`.
The implementation of `Users::UnlockingsController#update` was carried over from `UsersController#unlock` and I thought the order of the method calls was a bit odd. I've now added a controller test to highlight the problem and fixed it by reordering the method calls in the action.
floehopper
force-pushed
the
split-out-unlock-account-from-edit-user-page
branch
from
9896c40 to
a15ae9c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/uZD2I9dj & https://trello.com/c/P1w2E6MO
This splits out unlocking another user's account into a separate page. The latest design for the "edit user" page calls for the splitting out of a bunch of user operations into separate pages like this. The new "unlock account" page uses the GOV.UK Design System, but the "edit user" page still does not. c.f. #2497, #2509, #2537, #2545, #2547 & #2559.
Previously this functionality was implemented by a button on the "edit user" page, but now the button has been replaced by a link to a new "unlock account" page. This new page includes some explanatory text and a button to actually unlock the user's account.
I've also changed the "1 hour" in the explanatory text to be derived from the Devise configuration rather than being hard-coded and fixed a bug whereby a "manual account unlock" event was recorded whether or not the unlocking succeeded.
I've used "unlockings" vs "unlocks" in the routes & controller name to avoid clashing with the existing Devise-generated routes for a user to unlock their own account, even though we don't include links to this in the unlock instructions email, so they are currently unused. It might make more sense to see if we can disable the Devise-generated routes and then rename the routes & controller added in this PR but I'm going to leave that for now.
"Unlock account" link on "edit user" page
New "unlock account" page