Skip to content

Commit

Permalink
Fix redirects in UsersController actions
Browse files Browse the repository at this point in the history
It's a bit hard to follow the git history of this controller, because it
seems to have begun its life as a non-admin controller with actions
allowing users to change their own user, but then in this commit [1] a
bunch of functionality from an admin controller was merged into it.

When we moved the users index page to use the Design System, we removed
some actions from that page [2,3].

More recently we've split out a lot (all?) of the non-admin
functionality into controllers under the `Account` namespace [4].

I'm pretty sure the redirects to `root_path` date back to a time when
the action was called in the context of a user updating their own user.
However, these three actions (`unlock_access`, `resend_email_change` &
`cancel_email_change`) are now only every called in the context of the
page rendered by `UsersController#edit`, so it doesn't make any sense to
me to redirect to the `root_path`. Instead I've changed them to redirect
to the `users_path` which seems consistent with the `update` action.

The redirects using `redirect_back_or_to` seem to relate to a time when
the action was called in multiple contexts: either non-admin vs admin;
or "user index" page vs "edit user" page. The idea is to redirect back
to the page the user came from so the the user interaction makes sense
in both contexts. Note that the path passed to this
`redirect_back_or_to` method is only a fallback if the previous page
cannot be determined.

I've tried to standardize on using the "users index" page (i.e.
`users_path`) as the success path and the "edit user" page (i.e.
`edit_user_path`) as the failure path (i.e. the page where the form
errors are typically displayed). This feels like it gives a better user
experience and makes the code less suprising.

[1]: b730700
[2]: 62eb2f6
[3]: 2197050
[4]: #2377
  • Loading branch information
floehopper committed Nov 9, 2023
1 parent 62a58f0 commit 3bade58
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 6 deletions.
8 changes: 4 additions & 4 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ def unlock
EventLog.record_event(@user, EventLog::MANUAL_ACCOUNT_UNLOCK, initiator: current_user, ip_address: user_ip_address)
@user.unlock_access!
flash[:notice] = "Unlocked #{@user.email}"
redirect_back_or_to(root_path)
redirect_to users_path
end

def resend_email_change
@user.resend_confirmation_instructions
if @user.errors.empty?
redirect_to root_path, notice: "Successfully resent email change email to #{@user.unconfirmed_email}"
redirect_to users_path, notice: "Successfully resent email change email to #{@user.unconfirmed_email}"
else
redirect_to edit_user_path(@user), alert: "Failed to send email change email"
end
Expand All @@ -66,7 +66,7 @@ def resend_email_change
def cancel_email_change
@user.cancel_email_change!

redirect_back_or_to(root_path)
redirect_to users_path
end

def event_logs
Expand All @@ -78,7 +78,7 @@ def reset_two_step_verification
@user.reset_2sv!(current_user)
UserMailer.two_step_reset(@user).deliver_later

redirect_to :root, notice: "Reset 2-step verification for #{@user.email}"
redirect_to users_path, notice: "Reset 2-step verification for #{@user.email}"
end

def require_2sv; end
Expand Down
4 changes: 2 additions & 2 deletions test/controllers/users_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -814,9 +814,9 @@ class UsersControllerTest < ActionController::TestCase
assert_nil @another_user.confirmation_token
end

should "redirect to the edit user admin page" do
should "redirect to the users page" do
delete :cancel_email_change, params: { id: @another_user.id }
assert_redirected_to edit_user_path(@another_user)
assert_redirected_to users_path
end
end
end
Expand Down

0 comments on commit 3bade58

Please sign in to comment.