Add separate page for resending signup email for another user #2559
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
And use the almost identical `User#invited_but_not_yet_accepted?` method instead. The latter is more idiomatic, because it uses the usual predicate method naming and it uses `Object#present?` vs `Object#nil?`. Since `User#invitation_sent_at` is backed by a datetime column, I think it's safe to assume `!invitation_sent_at.nil?` is equivalent to `invitation_sent_at.present?` in all cases. Also `User#invited_but_not_yet_accepted?` was already used in more places so it makes sense to continue to use it. In the commits where `User#invited_but_not_accepted` [1] or `User#invited_but_not_yet_accepted` [2] were introduced, I can't see any evidence the distinction was important - I think the duplication was just an oversight. [1]: 6fc0b1f#diff-9802ca3c9c4cf89904fd44bc114e35ebdf2c5dd3d5b645491e2b253e1afef29bR43-R45 [2]: 808cb62#diff-9802ca3c9c4cf89904fd44bc114e35ebdf2c5dd3d5b645491e2b253e1afef29bR82-R84
And add test coverage in `UserPolicyTest`. Hopefully this makes the purpose of the policy predicate method clearer. Note that tests in the `InvitationsControllerTest` were already testing the behaviour and so I'm confident that the change to `InvitationsController#resend` hasn't broken anything.
Since `UserPolicy#resend_invitation?` is currently aliased to `UserPolicy#edit?` this doesn't make any practical difference, but since we've defined `UserPolicy#resend_invitation?` I think it makes sense to add this condition in case the policy implementation changes in the future.
floehopper
force-pushed
the
split-out-resending-signup-email-from-edit-user-page
branch
from
110c06d to
269fd7d
Compare
chrislo
approved these changes
Nov 29, 2023
Trello: https://trello.com/c/K3oa6wAj 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 one. The new `app/views/users/invitation_resends/edit.html.erb` template is based on other similar pages which we've recently extracted, e.g. `app/views/users/two_step_verification_resets/edit.html.erb` where there aren't any form fields; just a button. Previously the "Resend signup email" button on the "edit user" page submitted a POST request directly to `InvitationsController#resend`. Now I've moved `InvitationsController#resend` to `Users::InvitationResendsController#update` and a link on the "edit user" page takes you to a new page which has a "Resend signup email" button which submits a request to that new action. I did consider adding an action to display the new page to the existing `InvitationsController`. However, the latter is already significantly changing the default implementation of `Devise::InvitationsController` and I thought it would be better to move `InvitationsController#resend` into the new controller. I think this also simplifies the invitation-related routes a bit. There weren't any tests in `UsersControllerTest` for this behaviour, so I had to write `Users::InvitationResendsControllerTest` from scratch, but I based it closely on other similar controller tests. Luckily there was one integration test (`test/integration/inviting_users_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::InvitationResendsControllerTest` 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#resend_invitation?`. Although this is a bit ugly, since `UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a pragmatic option. Since the `Users::InvitationResendsController#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 "Resend signup email" page only really makes sense in the context of a user who has been invited but hasn't yet accepted the invitation 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 who has already accepted their invitation.
This is a bit orthogonal to the current PR. However, it seemed odd/inconsistent that previously we recorded an account invitation event when a user was first invited, but not if we resent the invitation. It might be worth considering adding a new event type for this to distinguish re-sending from the initial send, but I decided it wasn't worthwhile right now.
This is a bit orthogonal to the current PR. However, it seemed odd/inconsistent that previously we passed a value for `invited_by` to `User.invite!` when the user was first invited, but we did not pass a value for `invited_by` to `User#invite!` (i.e. the instance method) when we resent an invitation. In fact it turns out that not passing `invited_by` means that `User#invited_by` gets set to `nil`. Thus not only did it not overwrite the initial value with an appropriate new value, the original value was lost, which seems like the worst of all worlds!
This copy was copied directly from the "edit user" page (where it currently still exists albeit in separate paragraphs). As @chrislo pointed out, now that it's in a single paragraph the double "yet" didn't read well. I'm sure Etain will review this copy soon, but for now this seems good enough.
floehopper
force-pushed
the
split-out-resending-signup-email-from-edit-user-page
branch
from
269fd7d to
a6e562c
Compare
floehopper
deleted the
split-out-resending-signup-email-from-edit-user-page
branch
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/K3oa6wAj
This splits out resending a signup email for another user 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 "resend signup email" page uses the GOV.UK Design System, but the "edit user" page still does not. c.f. #2497, #2509, #2537, #2545 & #2547.
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 "resend signup email" page. This page includes some explanatory text and a button to actually resend the signup email for another user.
Rather than further complicating the existing
InvitationsController, I've movedInvitationsController#resend->Users::InvitationResendingsController#update. SinceInvitationsControllerwas already significantly customizing the default behaviour ofDevise::InvitationsController, I think this makes a lot of sense. See the commit titled "Extract page for resending invitation for a user" for more details.I've also tidied up a couple of things along the way. Notably we now record an event in the activity log when we resend a signup email and we keep track of which admin user resent an invitation. See individual commit notes for details.
"Resend signup email" link on "edit user" page
New "resend signup email" page