Extract edit user organisation page

Trello: https://trello.com/c/fA6dRVSm

c.f. this commit [1] where we did the same for user name.

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
editing the different user attribute and this is the next one.

The new `app/views/users/organisations/edit.html.erb` template in this
commit is closely based on the relevant bits of
`app/views/users/edit.html.erb` &
`app/views/users/_form_fields.html.erb`. I haven't yet changed the
template to use the Design System. I plan to do that in subsequent
commits.

The new `Users::OrganisationsController` is closely based on the
relevant bits of code from `UsersController`, even though some of it is
probably overkill in the new context, e.g. the use of `UserUpdate` &
`UserParameterSanitiser`. However, I thought it was worth keeping this
step as small as possible.

I'm reusing `UserPolicy#edit?` & `UserPolicy#update?` for the
authorization in the new `Users::OrganisationsController`, because that
still seems to make sense. However, note that we also make use of the
existing unconventional `UserPolicy#assign_organisations?` predicate
method which does not correspond to a controller action. I suspect
there's some simplification we could do here to make this more
idiomatic, but I've left that for now.

I've tried to add tests in `Users::OrganisationsControllerTest` to
capture the behaviour implied by how things worked when they were in
`UsersController`. It's not obvious that all of this was previously
captured in `UsersControllerTest` or `UserUpdateTest`. Most of these
tests are in the context of a Admin user doing the editing, because only
Admins & Superadmins can change a user's organisation (see
`UserPolicy#assign_organisations?`).

Rather than creating a combinatorial explosion of tests in
`Users::OrganisationsControllerTest` relating to whether a user with all
the different roles can edit another user with all the different roles,
I've resorted to stubbing `UserPolicy.new` and relevant predicate
methods on the `UserPolicy` instance. Although this is a bit ugly, since
`UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a
pragmatic option.

[1]: 899a8a1

app/controllers/users/organisations_controller.rb

@@ -0,0 +1,43 @@
+class Users::OrganisationsController < ApplicationController
+  before_action :authenticate_user!
+  before_action :load_user
+  before_action :authorize_user
+  before_action :redirect_to_account_page_if_acting_on_own_user, only: %i[edit]
+
+  def edit; end
+
+  def update
+    updater = UserUpdate.new(@user, user_params, current_user, user_ip_address)
+    if updater.call
+      redirect_to edit_user_path(@user), notice: "Updated user #{@user.email} successfully"
+    else
+      render :edit
+    end
+  end
+
+private
+
+  def load_user
+    @user = User.find(params[:user_id])
+  end
+
+  def authorize_user
+    authorize(@user)
+    authorize(@user, :assign_organisations?)
+  end
+
+  def user_params
+    UserParameterSanitiser.new(
+      user_params: permitted_user_params,
+      current_user_role: current_user.role.to_sym,
+    ).sanitise
+  end
+
+  def permitted_user_params
+    @permitted_user_params ||= params.require(:user).permit(:organisation_id).to_h
+  end
+
+  def redirect_to_account_page_if_acting_on_own_user
+    redirect_to edit_account_organisation_path if current_user == @user
+  end
+end

app/controllers/users_controller.rb

@@ -36,8 +36,6 @@ def edit
   end
 
   def update
-    raise Pundit::NotAuthorizedError if params[:user][:organisation_id].present? && !policy(@user).assign_organisations?
-
     updater = UserUpdate.new(@user, user_params, current_user, user_ip_address)
     if updater.call
       redirect_to users_path, notice: "Updated user #{@user.email} successfully"
@@ -112,7 +110,7 @@ def user_params
   end
 
   def permitted_user_params
-    @permitted_user_params ||= params.require(:user).permit(:user, :organisation_id, :require_2sv, :skip_update_user_permissions, supported_permission_ids: []).to_h
+    @permitted_user_params ||= params.require(:user).permit(:user, :require_2sv, :skip_update_user_permissions, supported_permission_ids: []).to_h
   end
 
   def filter_params

app/views/users/_form_fields.html.erb

@@ -21,6 +21,15 @@
   <% end %>
 </p>
 
+<p>
+  <strong>Organisation:</strong> <%= @user.organisation.present? ? @user.organisation.name : Organisation::NONE %>
+  <% if policy(User).assign_organisations? %>
+    <%= link_to edit_user_organisation_path(@user) do %>
+      Change<span class="invisible"> organisation</span>
+    <% end %>
+  <% end %>
+</p>
+
 <% if policy(@user).mandate_2sv? %>
   <dl>
     <dt>Account security</dt>
@@ -69,13 +78,6 @@
   </dl>
 <% end %>
 
-<% if policy(User).assign_organisations? %>
-  <p class="form-group">
-    <%= f.label :organisation_id, "Organisation" %><br />
-    <%= f.select :organisation_id, organisation_options(f), organisation_select_options, { class: "chosen-select form-control", 'data-module' => 'chosen' } %>
-  </p>
-<% end %>
-
 <h2 class="add-vertical-margins"> <%= "Editable " if (current_user.publishing_manager? ) %>Permissions</h2>
 <%= render partial: "shared/user_permissions", locals: { user_object: f.object }%>
 

app/views/users/organisations/edit.html.erb

@@ -0,0 +1,24 @@
+<% content_for :title, "Edit [#{@user.name}]" %>
+
+<h1>Edit &ldquo;<%= @user.name %>&rdquo;</h1>
+
+<%= form_for @user, :url => user_organisation_path(@user), :html => {:class => 'well add-top-margin'} do |f| %>
+  <% if @user.errors.count > 0 %>
+    <div class="govuk-error-summary alert alert-danger" aria-labelledby="error-summary-title" role="alert" tabindex="-1" data-module="error-summary">
+      <div class="govuk-error-summary__body">
+        <ul class="govuk-list govuk-error-summary__list">
+          <% @user.errors.full_messages.each do |message| %>
+            <%= content_tag :li, message %>
+          <% end %>
+        </ul>
+      </div>
+    </div>
+  <% end %>
+
+  <p class="form-group">
+    <%= f.label :organisation_id, "Organisation" %><br />
+    <%= f.select :organisation_id, organisation_options(f), organisation_select_options, { class: "chosen-select form-control", 'data-module' => 'chosen' } %>
+  </p>
+
+  <%= f.submit :class => 'btn btn-primary' %>
+<% end %>

config/routes.rb

@@ -49,6 +49,7 @@
       delete :cancel_email_change
     end
     resource :role, only: %i[edit update], controller: "users/roles"
+    resource :organisation, only: %i[edit update], controller: "users/organisations"
   end
   get "user", to: "oauth_users#show"
 

test/controllers/users/organisations_controller_test.rb

@@ -0,0 +1,243 @@
+require "test_helper"
+
+class Users::OrganisationsControllerTest < ActionController::TestCase
+  attr_reader :organisation
+
+  setup do
+    @organisation = create(:organisation)
+  end
+
+  context "GET edit" do
+    context "signed in as Admin user" do
+      setup do
+        @admin = create(:admin_user)
+        sign_in(@admin)
+      end
+
+      should "display form with organisation_id field" do
+        user = create(:user, organisation:)
+
+        get :edit, params: { user_id: user }
+
+        assert_template :edit
+        assert_select "form[action='#{user_organisation_path(user)}']" do
+          assert_select "select[name='user[organisation_id]'] option", value: organisation.id
+          assert_select "input[type='submit']"
+        end
+      end
+
+      should "authorize access if UserPolicy#edit? and UserPolicy#assign_organisations? return true" do
+        user = create(:user)
+
+        user_policy = stub_everything("user-policy", edit?: true, assign_organisations?: true)
+        UserPolicy.stubs(:new).returns(user_policy)
+
+        get :edit, params: { user_id: user }
+
+        assert_response :success
+      end
+
+      should "not authorize access if UserPolicy#edit? returns false" do
+        user = create(:user)
+
+        user_policy = stub_everything("user-policy", edit?: false, assign_organisations?: true)
+        UserPolicy.stubs(:new).returns(user_policy)
+
+        get :edit, params: { user_id: user }
+
+        assert_not_authorised
+      end
+
+      should "not authorize access if UserPolicy#assign_organisations? returns false" do
+        user = create(:user)
+
+        user_policy = stub_everything("user-policy", edit?: true, assign_organisations?: false)
+        UserPolicy.stubs(:new).returns(user_policy)
+
+        get :edit, params: { user_id: user }
+
+        assert_not_authorised
+      end
+
+      should "redirect to account edit organisation page if admin is acting on their own user" do
+        get :edit, params: { user_id: @admin }
+
+        assert_redirected_to edit_account_organisation_path
+      end
+    end
+
+    context "signed in as Normal user" do
+      setup do
+        sign_in(create(:user))
+      end
+
+      should "not be authorized" do
+        user = create(:user)
+
+        get :edit, params: { user_id: user }
+
+        assert_not_authorised
+      end
+    end
+
+    context "not signed in" do
+      should "not be allowed access" do
+        user = create(:user)
+
+        get :edit, params: { user_id: user }
+
+        assert_not_authenticated
+      end
+    end
+  end
+
+  context "PUT update" do
+    attr_reader :another_organisation
+
+    setup do
+      @another_organisation = create(:organisation)
+    end
+
+    context "signed in as Admin user" do
+      setup do
+        @admin = create(:admin_user)
+        sign_in(@admin)
+      end
+
+      should "update user organisation" do
+        user = create(:user, organisation:)
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_equal another_organisation, user.reload.organisation
+      end
+
+      should "record account updated & organisation change events" do
+        user = create(:user, organisation:)
+
+        @controller.stubs(:user_ip_address).returns("1.1.1.1")
+
+        EventLog.expects(:record_event).with(
+          user,
+          EventLog::ACCOUNT_UPDATED,
+          initiator: @admin,
+          ip_address: "1.1.1.1",
+        )
+
+        EventLog.expects(:record_organisation_change).with(
+          user,
+          organisation.name,
+          another_organisation.name,
+          @admin,
+        )
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+      end
+
+      should "should not record organisation change event if organisation has not changed" do
+        user = create(:user, organisation:)
+
+        EventLog.expects(:record_organisation_change).never
+
+        put :update, params: { user_id: user, user: { organisation_id: organisation } }
+      end
+
+      should "push changes out to apps" do
+        user = create(:user, organisation:)
+
+        PermissionUpdater.expects(:perform_on).with(user).once
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+      end
+
+      should "redirect to user page and display success notice" do
+        user = create(:user, organisation:, email: "user@gov.uk")
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_redirected_to edit_user_path(user)
+        assert_equal "Updated user user@gov.uk successfully", flash[:notice]
+      end
+
+      should "update user organisation if UserPolicy#update? and UserPolicy#assign_organisations? return true" do
+        user = create(:user, organisation:)
+
+        user_policy = stub_everything("user-policy", update?: true, assign_organisations?: true)
+        UserPolicy.stubs(:new).returns(user_policy)
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_equal another_organisation, user.reload.organisation
+      end
+
+      should "not update user organisation if UserPolicy#update? returns false" do
+        user = create(:user, organisation:)
+
+        user_policy = stub_everything("user-policy", update?: false, assign_organisations?: true)
+        UserPolicy.stubs(:new).returns(user_policy)
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_equal organisation, user.reload.organisation
+        assert_not_authorised
+      end
+
+      should "not update user organisation if UserPolicy#assign_organisations? returns false" do
+        user = create(:user, organisation:)
+
+        user_policy = stub_everything("user-policy", update?: true, assign_role?: false)
+        UserPolicy.stubs(:new).returns(user_policy)
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_equal organisation, user.reload.organisation
+        assert_not_authorised
+      end
+
+      should "redisplay form if organisation is not valid" do
+        user = create(:organisation_admin_user, organisation:)
+
+        put :update, params: { user_id: user, user: { organisation_id: nil } }
+
+        assert_template :edit
+        assert_select "form[action='#{user_organisation_path(user)}']" do
+          assert_select "select[name='user[organisation_id]'] option", value: organisation.id
+        end
+      end
+
+      should "display errors if organisation is not valid" do
+        user = create(:organisation_admin_user, organisation:)
+
+        put :update, params: { user_id: user, user: { organisation_id: nil } }
+
+        assert_select ".govuk-error-summary" do
+          assert_select "li", text: "Organisation can't be 'None' for Organisation Admin"
+        end
+      end
+    end
+
+    context "signed in as Normal user" do
+      setup do
+        sign_in(create(:user))
+      end
+
+      should "not be authorized" do
+        user = create(:user, organisation:)
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_not_authorised
+      end
+    end
+
+    context "not signed in" do
+      should "not be allowed access" do
+        user = create(:user, organisation:)
+
+        put :update, params: { user_id: user, user: { organisation_id: another_organisation } }
+
+        assert_not_authenticated
+      end
+    end
+  end
+end