Remove some test duplication

We test these two scenarios multiple times in three different
controller tests. Now the underlying logic has been extracted to
UserUpdatePermissionBuilder I think they can be replaced with simpler
and more comprehensive tests of `UserUpdatePermissionBuilder#build`.

Note the remaining tests of the update method in the three permissions
controllers are still useful - they effectively test that the correct
values are set by the `set_application` and `set_permissions` methods
on those controllers.

I considered refactoring these tests to `expect` that
`UserUpdatePermissionBuilder` is called correctly, but decided to keep
things as they are for now. This change still feels like an
improvement.

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