Merge pull request #2442 from alphagov/preselect-default-supported-pe…

…rmissions-when-creating-new-users

Preselect default supported permissions when creating new users

app/controllers/batch_invitation_permissions_controller.rb

@@ -11,7 +11,6 @@ def new; end
 
   def create
     @batch_invitation.supported_permission_ids = params[:user][:supported_permission_ids] if params[:user]
-    grant_default_permissions(@batch_invitation)
 
     @batch_invitation.save!
 
@@ -36,10 +35,4 @@ def prevent_updating
       redirect_to batch_invitation_path(@batch_invitation)
     end
   end
-
-  def grant_default_permissions(batch_invitation)
-    SupportedPermission.default.each do |default_permission|
-      batch_invitation.grant_permission(default_permission)
-    end
-  end
 end

app/controllers/invitations_controller.rb

@@ -13,7 +13,9 @@ class InvitationsController < Devise::InvitationsController
 
   def new
     authorize User
-    super
+
+    self.resource = User.with_default_permissions
+    render :new
   end
 
   def create
@@ -24,7 +26,6 @@ def create
 
     self.resource = resource_class.invite!(all_params, current_inviter)
     if resource.errors.empty?
-      grant_default_permissions(resource)
       EventLog.record_account_invitation(resource, current_user)
       set_flash_message :notice, :send_instructions, email: resource.email
       respond_with resource, location: after_invite_path_for(resource)
@@ -67,12 +68,6 @@ def after_invite_path_for(_)
     end
   end
 
-  def grant_default_permissions(user)
-    SupportedPermission.default.each do |default_permission|
-      user.grant_permission(default_permission)
-    end
-  end
-
   def organisation(params)
     Organisation.find_by(id: params[:organisation_id])
   end

app/models/user.rb

@@ -122,6 +122,10 @@ def self.with_2sv_statuses(scope_names)
     relation
   end
 
+  def self.with_default_permissions
+    new(supported_permissions: SupportedPermission.default)
+  end
+
   def require_2sv?
     return require_2sv unless organisation
 
@@ -211,7 +215,11 @@ def grant_application_permissions(application, supported_permission_names)
   end
 
   def grant_permission(supported_permission)
-    application_permissions.where(supported_permission_id: supported_permission.id).first_or_create!
+    if persisted?
+      application_permissions.where(supported_permission_id: supported_permission.id).first_or_create!
+    else
+      supported_permissions << supported_permission unless supported_permissions.include?(supported_permission)
+    end
   end
 
   # This overrides `Devise::Recoverable` behavior.

app/views/batch_invitation_permissions/new.html.erb

@@ -20,13 +20,13 @@
 
 <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %>
   <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %>
-    <% options = options_for_permission_option_select(application:) %>
+    <% options = options_for_permission_option_select(application:, user: User.with_default_permissions) %>
     <%= render("govuk_publishing_components/components/option_select", {
       title: application.name,
       key: "user[supported_permission_ids]",
       options_container_id: "user_application_#{application.id}_supported_permissions",
       show_filter: options.length > 4,
-      closed_on_load: true,
+      closed_on_load: options.map {|o| o[:checked] }.none?,
       options:,
     }) %>
   <% end %>

test/controllers/batch_invitation_permissions_controller_test.rb

@@ -66,6 +66,17 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase
         assert_select ".gem-c-option-select[data-filter-element]"
       end
     end
+
+    should "render form checkbox inputs with default permissions checked" do
+      application = create(:application)
+      permission = create(:supported_permission, default: true, application:)
+
+      get :new, params: { batch_invitation_id: @batch_invitation.id }
+
+      assert_select "form" do
+        assert_select "input[type='checkbox'][checked='checked'][name='user[supported_permission_ids][]'][value='#{permission.to_param}']"
+      end
+    end
   end
 
   context "POST create" do
@@ -79,29 +90,13 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase
       assert_redirected_to "/batch_invitations/#{@batch_invitation.id}"
     end
 
-    should "grant selected permissions and default permissions to BatchInvitation" do
-      support_app = create(:application, name: "Support")
-      support_app.signin_permission.update!(default: true)
-
+    should "grant selected permissions to BatchInvitation" do
       post :create, params: {
         batch_invitation_id: @batch_invitation.id,
         user: { supported_permission_ids: [@app.signin_permission.id] },
       }
 
-      assert_equal [@app.signin_permission, support_app.signin_permission],
-                   @batch_invitation.supported_permissions
-    end
-
-    context "with no permissions selected" do
-      should "still grant default permissions to BatchInvitation" do
-        support_app = create(:application, name: "Support")
-        support_app.signin_permission.update!(default: true)
-
-        post :create, params: { batch_invitation_id: @batch_invitation.id }
-
-        assert_equal [support_app.signin_permission],
-                     @batch_invitation.supported_permissions
-      end
+      assert_equal [@app.signin_permission], @batch_invitation.supported_permissions
     end
 
     should "send an email to signon-alerts" do

test/controllers/invitations_controller_test.rb

@@ -61,6 +61,17 @@ class InvitationsControllerTest < ActionController::TestCase
         end
       end
 
+      should "render form checkbox inputs with some default permissions checked" do
+        application = create(:application)
+        permission = create(:supported_permission, default: true, application:)
+
+        get :new
+
+        assert_select "form" do
+          assert_select "input[type='checkbox'][checked='checked'][name='user[supported_permission_ids][]'][value='#{permission.to_param}']"
+        end
+      end
+
       should "render filter for option-select component when app has more than 4 permissions" do
         application = create(:application)
         4.times { create(:supported_permission, application:) }
@@ -169,18 +180,6 @@ class InvitationsControllerTest < ActionController::TestCase
         assert_equal [permission], invitee.supported_permissions
       end
 
-      should "save invitee with default supported permissions" do
-        default_permission = create(:supported_permission, default: true)
-        non_default_permission = create(:supported_permission, default: false)
-
-        post :create, params: {
-          user: { name: "invitee", email: "invitee@gov.uk", supported_permission_ids: [non_default_permission.to_param] },
-        }
-
-        invitee = User.last
-        assert_same_elements [default_permission, non_default_permission], invitee.supported_permissions
-      end
-
       should "send invitation to invitee from inviter" do
         invitee = create(:user)
         User.expects(:invite!).with(anything, @inviter).returns(invitee)

test/integration/batch_inviting_users_test.rb

@@ -97,32 +97,6 @@ class BatchInvitingUsersTest < ActionDispatch::IntegrationTest
     end
   end
 
-  should "ensure that batch invited users get default permissions even when not checked in UI" do
-    create(:supported_permission, application: @application, name: "reader", default: true)
-    support_app = create(:application, name: "support", with_supported_permissions: [SupportedPermission::SIGNIN_NAME])
-    support_app.signin_permission.update!(default: true)
-    user = create(:admin_user)
-
-    visit root_path
-    signin_with(user)
-
-    perform_enqueued_jobs do
-      visit new_batch_invitation_path
-      path = Rails.root.join("test/fixtures/users.csv")
-      attach_file("Upload a CSV file", path)
-      click_button "Manage permissions for new users"
-
-      uncheck "Has access to #{support_app.name}?"
-      check "Has access to #{@application.name}?"
-      uncheck "reader"
-      click_button "Create users and send emails"
-
-      invited_user = User.find_by(email: "fred@example.com")
-      assert invited_user.has_access_to?(support_app)
-      assert invited_user.permissions_for(@application).include? "reader"
-    end
-  end
-
   context "when the organisation mandates 2sv" do
     setup do
       @user = create(:superadmin_user)

test/models/user_test.rb

@@ -512,6 +512,58 @@ def setup
     assert_equal old_encrypted_password, u.encrypted_password, "Changed password"
   end
 
+  context "#grant_permission" do
+    context "where the user does not have the permission" do
+      should "add the new permission"  do
+        user = create(:user)
+        application = create(:application)
+        supported_permission = create(:supported_permission, application:)
+
+        user.grant_permission(supported_permission)
+
+        assert user.has_permission?(supported_permission)
+      end
+    end
+
+    context "where the user has the permission" do
+      should "use the existing permission" do
+        user = create(:user)
+        application = create(:application)
+        supported_permission = create(:supported_permission, application:)
+        user.supported_permissions << supported_permission
+
+        user.grant_permission(supported_permission)
+
+        assert user.has_permission?(supported_permission)
+        assert user.supported_permissions.include?(supported_permission)
+      end
+    end
+
+    context "where the user is a new user" do
+      should "grant the permission" do
+        user = build(:user)
+        application = create(:application)
+        supported_permission = create(:supported_permission, application:)
+
+        user.grant_permission(supported_permission)
+
+        assert user.has_permission?(supported_permission)
+      end
+
+      should "prevent the permission being granted twice" do
+        user = build(:user)
+        application = create(:application)
+        supported_permission = create(:supported_permission, application:)
+
+        user.grant_permission(supported_permission)
+        user.grant_permission(supported_permission)
+        user.save!
+
+        user.has_permission?(supported_permission)
+      end
+    end
+  end
+
   test "can grant signin permission to allow user to access the app" do
     app = create(:application)
     user = create(:user)
@@ -1109,6 +1161,17 @@ def setup
     end
   end
 
+  context ".with_default_permissions" do
+    should "return a new user with default permissions added" do
+      application = create(:application)
+      create(:supported_permission, default: true, application:)
+
+      user = User.with_default_permissions
+
+      assert 1, user.supported_permissions.size
+    end
+  end
+
   def authenticate_access(user, app)
     Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id)
   end