Merge pull request #2657 from alphagov/refactor_build_user_update_params

Refactor build user update params

app/controllers/account/permissions_controller.rb

@@ -18,7 +18,13 @@ def edit
   def update
     authorize [:account, @application], :edit_permissions?
 
-    UserUpdate.new(current_user, build_user_update_params, current_user, user_ip_address).call
+    supported_permission_ids = UserUpdatePermissionBuilder.new(
+      user: current_user,
+      updatable_permission_ids: @permissions.pluck(:id),
+      selected_permission_ids: update_params[:supported_permission_ids].map(&:to_i),
+    ).build
+
+    UserUpdate.new(current_user, { supported_permission_ids: }, current_user, user_ip_address).call
 
     flash[:application_id] = @application.id
     redirect_to account_applications_path
@@ -37,14 +43,4 @@ def set_application
   def set_permissions
     @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
   end
-
-  def build_user_update_params
-    permissions_user_has = current_user.supported_permissions.pluck(:id)
-    updatable_permissions_for_this_app = @permissions.pluck(:id)
-    selected_permissions = update_params[:supported_permission_ids].map(&:to_i)
-    permissions_to_add = updatable_permissions_for_this_app.intersection(selected_permissions)
-    permissions_to_remove = updatable_permissions_for_this_app.difference(selected_permissions)
-
-    { supported_permission_ids: (permissions_user_has + permissions_to_add - permissions_to_remove).sort }
-  end
 end

app/controllers/api_users/permissions_controller.rb

@@ -11,7 +11,13 @@ def edit
   def update
     authorize @api_user
 
-    UserUpdate.new(@api_user, build_user_update_params, current_user, user_ip_address).call
+    supported_permission_ids = UserUpdatePermissionBuilder.new(
+      user: @api_user,
+      updatable_permission_ids: @permissions.pluck(:id),
+      selected_permission_ids: update_params[:supported_permission_ids].map(&:to_i),
+    ).build
+
+    UserUpdate.new(@api_user, { supported_permission_ids: }, current_user, user_ip_address).call
 
     flash[:application_id] = @application.id
     redirect_to api_user_applications_path(@api_user)
@@ -34,14 +40,4 @@ def set_application
   def set_permissions
     @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
   end
-
-  def build_user_update_params
-    permissions_user_has = @api_user.supported_permissions.pluck(:id)
-    updatable_permissions_for_this_app = @permissions.pluck(:id)
-    selected_permissions = update_params[:supported_permission_ids].map(&:to_i)
-    permissions_to_add = updatable_permissions_for_this_app.intersection(selected_permissions)
-    permissions_to_remove = updatable_permissions_for_this_app.difference(selected_permissions)
-
-    { supported_permission_ids: (permissions_user_has + permissions_to_add - permissions_to_remove).sort }
-  end
 end

app/controllers/users/permissions_controller.rb

@@ -19,7 +19,13 @@ def edit
   def update
     authorize UserApplicationPermission.for(@user, @application)
 
-    UserUpdate.new(@user, build_user_update_params, current_user, user_ip_address).call
+    supported_permission_ids = UserUpdatePermissionBuilder.new(
+      user: @user,
+      updatable_permission_ids: @permissions.pluck(:id),
+      selected_permission_ids: update_params[:supported_permission_ids].map(&:to_i),
+    ).build
+
+    UserUpdate.new(@user, { supported_permission_ids: }, current_user, user_ip_address).call
 
     flash[:application_id] = @application.id
     redirect_to user_applications_path(@user)
@@ -42,14 +48,4 @@ def set_application
   def set_permissions
     @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
   end
-
-  def build_user_update_params
-    permissions_user_has = @user.supported_permissions.pluck(:id)
-    updatable_permissions_for_this_app = @permissions.pluck(:id)
-    selected_permissions = update_params[:supported_permission_ids].map(&:to_i)
-    permissions_to_add = updatable_permissions_for_this_app.intersection(selected_permissions)
-    permissions_to_remove = updatable_permissions_for_this_app.difference(selected_permissions)
-
-    { supported_permission_ids: (permissions_user_has + permissions_to_add - permissions_to_remove).sort }
-  end
 end

app/models/user_update_permission_builder.rb

@@ -0,0 +1,15 @@
+class UserUpdatePermissionBuilder
+  def initialize(user:, updatable_permission_ids:, selected_permission_ids:)
+    @user = user
+    @updatable_permission_ids = updatable_permission_ids
+    @selected_permission_ids = selected_permission_ids
+  end
+
+  def build
+    permissions_user_has = @user.supported_permissions.pluck(:id)
+    permissions_to_add = @updatable_permission_ids.intersection(@selected_permission_ids)
+    permissions_to_remove = @updatable_permission_ids.difference(@selected_permission_ids)
+
+    (permissions_user_has + permissions_to_add - permissions_to_remove).sort
+  end
+end

test/controllers/account/permissions_controller_test.rb

@@ -124,33 +124,6 @@ class Account::PermissionsControllerTest < ActionController::TestCase
       assert_redirected_to "/users/sign_in"
     end
 
-    should "replace the users permissions with new ones" do
-      application = create(:application, with_supported_permissions: %w[new old])
-      user = create(:admin_user, with_permissions: { application => ["old", SupportedPermission::SIGNIN_NAME] })
-      sign_in user
-
-      new_permission = application.supported_permissions.find_by(name: "new")
-
-      patch :update, params: { application_id: application.id, application: { supported_permission_ids: [new_permission.id] } }
-
-      assert_equal %w[new signin], user.permissions_for(application)
-      assert_redirected_to account_applications_path
-    end
-
-    should "retain permissions for other apps" do
-      other_application = create(:application, with_supported_permissions: %w[other])
-      application = create(:application, with_supported_permissions: %w[new old])
-      user = create(:admin_user, with_permissions: { application => ["old", SupportedPermission::SIGNIN_NAME], other_application => %w[other] })
-      sign_in user
-
-      new_permission = application.supported_permissions.find_by(name: "new")
-
-      patch :update, params: { application_id: application.id, application: { supported_permission_ids: [new_permission.id] } }
-
-      assert_equal %w[new signin], user.permissions_for(application)
-      assert_equal %w[other], user.permissions_for(other_application)
-    end
-
     should "prevent permissions being added for apps that the current user does not have access to" do
       application1 = create(:application)
       application2 = create(:application, with_supported_permissions: %w[app2-permission])

test/controllers/api_users/permissions_controller_test.rb

@@ -123,26 +123,6 @@ class ApiUsers::PermissionsControllerTest < ActionController::TestCase
       assert_not_authenticated
     end
 
-    should "replace the users permissions with new ones" do
-      application = create(:application, with_supported_permissions: %w[new old])
-      api_user = create(:api_user, with_permissions: { application => %w[old] })
-      create(:access_token, application:, resource_owner_id: api_user.id)
-
-      current_user = create(:superadmin_user)
-      sign_in current_user
-
-      stub_policy current_user, api_user, update?: true
-
-      new_permission = application.supported_permissions.find_by(name: "new")
-
-      expected_params = { supported_permission_ids: [new_permission.id] }
-      user_update = stub("user-update").responds_like_instance_of(UserUpdate)
-      user_update.expects(:call)
-      UserUpdate.stubs(:new).with(api_user, expected_params, current_user, anything).returns(user_update)
-
-      patch :update, params: { api_user_id: api_user, application_id: application, application: { supported_permission_ids: [new_permission.id] } }
-    end
-
     should "redirect once the permissions have been updated" do
       application = create(:application, with_supported_permissions: %w[new old])
       api_user = create(:api_user, with_permissions: { application => %w[old] })
@@ -160,28 +140,6 @@ class ApiUsers::PermissionsControllerTest < ActionController::TestCase
       assert_redirected_to api_user_applications_path(api_user)
     end
 
-    should "retain permissions for other apps" do
-      other_application = create(:application, with_supported_permissions: %w[other])
-      application = create(:application, with_supported_permissions: %w[new old])
-      api_user = create(:api_user, with_permissions: { application => %w[old], other_application => %w[other] })
-      create(:access_token, application:, resource_owner_id: api_user.id)
-
-      current_user = create(:superadmin_user)
-      sign_in current_user
-
-      stub_policy current_user, api_user, update?: true
-
-      new_permission = application.supported_permissions.find_by(name: "new")
-      other_permission = other_application.supported_permissions.find_by(name: "other")
-
-      expected_params = { supported_permission_ids: [other_permission.id, new_permission.id].sort }
-      user_update = stub("user-update").responds_like_instance_of(UserUpdate)
-      user_update.expects(:call)
-      UserUpdate.stubs(:new).with(api_user, expected_params, current_user, anything).returns(user_update)
-
-      patch :update, params: { api_user_id: api_user, application_id: application, application: { supported_permission_ids: [new_permission.id] } }
-    end
-
     should "prevent permissions being added for apps that the current user does not have access to" do
       organisation = create(:organisation)
 

test/controllers/users/permissions_controller_test.rb

@@ -205,27 +205,6 @@ class Users::PermissionsControllerTest < ActionController::TestCase
       assert_redirected_to "/users/sign_in"
     end
 
-    should "replace the users permissions with new ones" do
-      application = create(:application, with_supported_permissions: %w[new old])
-      user = create(:user, with_permissions: { application => %w[old] })
-      user.grant_application_signin_permission(application)
-
-      current_user = create(:admin_user)
-      sign_in current_user
-
-      permission = stub_user_application_permission(user, application)
-      stub_policy current_user, permission, update?: true
-
-      new_permission = application.supported_permissions.find_by(name: "new")
-
-      expected_params = { supported_permission_ids: [new_permission.id, application.signin_permission.id].sort }
-      user_update = stub("user-update").responds_like_instance_of(UserUpdate)
-      user_update.expects(:call)
-      UserUpdate.stubs(:new).with(user, expected_params, current_user, anything).returns(user_update)
-
-      patch :update, params: { user_id: user, application_id: application.id, application: { supported_permission_ids: [new_permission.id] } }
-    end
-
     should "redirect once the permissions have been updated" do
       application = create(:application, with_supported_permissions: %w[new old])
       user = create(:user, with_permissions: { application => %w[old] })
@@ -244,29 +223,6 @@ class Users::PermissionsControllerTest < ActionController::TestCase
       assert_redirected_to user_applications_path(user)
     end
 
-    should "retain permissions for other apps" do
-      other_application = create(:application, with_supported_permissions: %w[other])
-      application = create(:application, with_supported_permissions: %w[new old])
-      user = create(:user, with_permissions: { application => %w[old], other_application => %w[other] })
-      user.grant_application_signin_permission(application)
-
-      current_user = create(:admin_user)
-      sign_in current_user
-
-      permission = stub_user_application_permission(user, application)
-      stub_policy current_user, permission, update?: true
-
-      new_permission = application.supported_permissions.find_by(name: "new")
-      other_permission = other_application.supported_permissions.find_by(name: "other")
-
-      expected_params = { supported_permission_ids: [other_permission.id, new_permission.id, application.signin_permission.id].sort }
-      user_update = stub("user-update").responds_like_instance_of(UserUpdate)
-      user_update.expects(:call)
-      UserUpdate.stubs(:new).with(user, expected_params, current_user, anything).returns(user_update)
-
-      patch :update, params: { user_id: user, application_id: application.id, application: { supported_permission_ids: [new_permission.id] } }
-    end
-
     should "prevent permissions being added for apps that the current user does not have access to" do
       organisation = create(:organisation)
 

test/models/user_update_permission_builder_test.rb

@@ -0,0 +1,62 @@
+require "test_helper"
+
+class UserUpdatePermissionBuilderTest < ActiveSupport::TestCase
+  def setup
+    application = create(:application)
+    create(:supported_permission, application:, name: "perm-1")
+    @user = create(:user, with_permissions: { application => %w[perm-1] })
+    @existing_permission_id = @user.supported_permissions.first.id
+  end
+
+  context "#build" do
+    should "should return users existing permission if not updatable and not selected" do
+      builder = UserUpdatePermissionBuilder.new(
+        user: @user,
+        updatable_permission_ids: [],
+        selected_permission_ids: [],
+      )
+
+      assert_equal [@existing_permission_id], builder.build
+    end
+
+    should "should remove users existing permission if updatable and not selected" do
+      builder = UserUpdatePermissionBuilder.new(
+        user: @user,
+        updatable_permission_ids: [@existing_permission_id],
+        selected_permission_ids: [],
+      )
+
+      assert_equal [], builder.build
+    end
+
+    should "should add new permission if updatable and selected" do
+      builder = UserUpdatePermissionBuilder.new(
+        user: @user,
+        updatable_permission_ids: [1],
+        selected_permission_ids: [1],
+      )
+
+      assert_equal [1, @existing_permission_id].sort, builder.build
+    end
+
+    should "should not add new permission if updatable and not selected" do
+      builder = UserUpdatePermissionBuilder.new(
+        user: @user,
+        updatable_permission_ids: [1],
+        selected_permission_ids: [],
+      )
+
+      assert_equal [@existing_permission_id], builder.build
+    end
+
+    should "should not add new permission if not updatable" do
+      builder = UserUpdatePermissionBuilder.new(
+        user: @user,
+        updatable_permission_ids: [1],
+        selected_permission_ids: [2],
+      )
+
+      assert_equal [@existing_permission_id], builder.build
+    end
+  end
+end