Break BatchInvitation creation into two steps

The existing batch invitations controller handles uploading the CSV of users and setting a default organisation, and it delegates managing permissions to the new batch invitation permissions controller. The BatchInvitation and its BatchInvitationUsers will still be persisted once this first form is submitted, but the jobs, emails, etc won't be kicked-off (the batch invitation won't be "in progress") until the permissions form has been submitted.
alphagov · Sep 8, 2023 · cd381e3 · cd381e3
1 parent 5c5b0cb
commit cd381e3
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 57 deletions.
diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb
@@ -1,10 +1,8 @@
 require "csv"
 
 class BatchInvitationsController < ApplicationController
-  include UserPermissionsControllerMethods
   before_action :authenticate_user!
 
-  helper_method :applications_and_permissions
   helper_method :recent_batch_invitations
 
   layout "admin_layout", only: %w[show]
@@ -16,8 +14,6 @@ def new
 
   def create
     @batch_invitation = BatchInvitation.new(user: current_user, organisation_id: params[:batch_invitation][:organisation_id])
-    @batch_invitation.supported_permission_ids = params[:user][:supported_permission_ids] if params[:user]
-    grant_default_permissions(@batch_invitation)
     authorize @batch_invitation
 
     unless file_uploaded?
@@ -64,9 +60,7 @@ def create
 
     @batch_invitation.save!
 
-    @batch_invitation.enqueue
-    flash[:notice] = "Scheduled invitation of #{@batch_invitation.batch_invitation_users.count} users"
-    redirect_to batch_invitation_path(@batch_invitation)
+    redirect_to new_batch_invitation_permissions_path(@batch_invitation)
   end
 
   def show
@@ -91,12 +85,6 @@ def file_uploaded?
     end
   end
 
-  def grant_default_permissions(batch_invitation)
-    SupportedPermission.default.each do |default_permission|
-      batch_invitation.grant_permission(default_permission)
-    end
-  end
-
   def batch_users_error_message(batch_user)
     e = batch_user.errors.first
 

diff --git a/app/views/batch_invitations/new.html.erb b/app/views/batch_invitations/new.html.erb
@@ -66,10 +66,6 @@ Winston Churchill,winston@example.com
       <% end %>
     </div>
 
-    <h2>Permissions for the users</h2>
-
-    <%= render partial: "shared/user_permissions", locals: { user_object: User.new } %>
-
-    <%= f.submit "Create users and send emails", :class => 'btn btn-success' %>
+    <%= f.submit "Manage permissions for new users", :class => 'btn btn-success' %>
   <% end %>
 </div>
diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb
@@ -43,12 +43,11 @@ def users_csv(filename = "users.csv")
 
   context "POST create" do
     should "create a BatchInvitation and BatchInvitationUsers" do
-      app = create(:application)
-      post :create, params: { batch_invitation: { user_names_and_emails: users_csv }, user: { supported_permission_ids: [app.signin_permission.id] } }
+      create(:application)
+      post :create, params: { batch_invitation: { user_names_and_emails: users_csv } }
 
       bi = BatchInvitation.last
       assert_not_nil bi
-      assert_equal [app.signin_permission], bi.supported_permissions
       expected_names_and_emails = [["Arthur Dent", "a@hhg.com"], ["Tricia McMillan", "t@hhg.com"]]
       assert_equal(
         expected_names_and_emails,
@@ -57,9 +56,7 @@ def users_csv(filename = "users.csv")
     end
 
     should "store the organisation to invite users to" do
-      post :create,
-           params: { user: { supported_permission_ids: [] },
-                     batch_invitation: { user_names_and_emails: users_csv, organisation_id: 3 } }
+      post :create, params: { batch_invitation: { user_names_and_emails: users_csv, organisation_id: 3 } }
 
       bi = BatchInvitation.last
 
@@ -70,8 +67,7 @@ def users_csv(filename = "users.csv")
     should "store organisation info from the uploaded CSV when logged in as an admin" do
       @user.update!(role: Roles::Admin.role_name)
       post :create,
-           params: { user: { supported_permission_ids: [] },
-                     batch_invitation: { user_names_and_emails: users_csv("users_with_orgs.csv"), organisation_id: 3 } }
+           params: { batch_invitation: { user_names_and_emails: users_csv("users_with_orgs.csv"), organisation_id: 3 } }
 
       bi = BatchInvitation.last
 
@@ -85,8 +81,7 @@ def users_csv(filename = "users.csv")
     should "store organisation info from the uploaded CSV when logged in as a superadmin" do
       @user.update!(role: Roles::Superadmin.role_name)
       post :create,
-           params: { user: { supported_permission_ids: [] },
-                     batch_invitation: { user_names_and_emails: users_csv("users_with_orgs.csv"), organisation_id: 3 } }
+           params: { batch_invitation: { user_names_and_emails: users_csv("users_with_orgs.csv"), organisation_id: 3 } }
 
       bi = BatchInvitation.last
 
@@ -97,34 +92,15 @@ def users_csv(filename = "users.csv")
       assert_equal "cabinet-office", bi.batch_invitation_users[2].organisation_slug
     end
 
-    should "queue a job to do the processing" do
-      assert_enqueued_jobs 2 do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv }, user: { supported_permission_ids: [] } }
-      end
-    end
-
-    should "send an email to signon-alerts" do
-      perform_enqueued_jobs do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv }, user: { supported_permission_ids: [] } }
-
-        email = ActionMailer::Base.deliveries.detect do |m|
-          m.to.any? { |to| to =~ /signon-alerts@.*\.gov\.uk/ }
-        end
-        assert_not_nil email
-        assert_equal "[SIGNON] #{@user.name} created a batch of 2 users in development", email.subject
-      end
-    end
-
-    should "redirect to the batch invitation page and show a flash message" do
-      post :create, params: { batch_invitation: { user_names_and_emails: users_csv }, user: { supported_permission_ids: [] } }
+    should "redirect to the batch invitation permissions page and show a flash message" do
+      post :create, params: { batch_invitation: { user_names_and_emails: users_csv } }
 
-      assert_match(/Scheduled invitation of 2 users/i, flash[:notice])
-      assert_redirected_to "/batch_invitations/#{BatchInvitation.last.id}"
+      assert_redirected_to "/batch_invitations/#{BatchInvitation.last.id}/permissions/new"
     end
 
     context "no file uploaded" do
       should "redisplay the form and show a flash message" do
-        post :create, params: { batch_invitation: { user_names_and_emails: nil }, user: { supported_permission_ids: [] } }
+        post :create, params: { batch_invitation: { user_names_and_emails: nil } }
 
         assert_template :new
         assert_match(/You must upload a file/i, flash[:alert])
@@ -133,7 +109,7 @@ def users_csv(filename = "users.csv")
 
     context "the CSV contains one or more email addresses that aren't valid" do
       should "redisplay the form and show a flash message" do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("users_with_non_valid_emails.csv") }, user: { supported_permission_ids: [] } }
+        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("users_with_non_valid_emails.csv") } }
 
         assert_template :new
         assert_match(/One or more emails were invalid/i, flash[:alert])
@@ -142,7 +118,7 @@ def users_csv(filename = "users.csv")
 
     context "the CSV has all the fields, but not in the expected order" do
       should "process the fields by name" do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("reversed_users.csv") }, user: { supported_permission_ids: [] } }
+        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("reversed_users.csv") } }
 
         bi = BatchInvitation.last
         assert_not_nil bi.batch_invitation_users.find_by(email: "a@hhg.com")
@@ -152,7 +128,7 @@ def users_csv(filename = "users.csv")
 
     context "the CSV has no data rows" do
       should "redisplay the form and show a flash message" do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("empty_users.csv") }, user: { supported_permission_ids: [] } }
+        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("empty_users.csv") } }
 
         assert_template :new
         assert_match(/no rows/i, flash[:alert])
@@ -161,7 +137,7 @@ def users_csv(filename = "users.csv")
 
     context "the CSV format is invalid" do
       should "redisplay the form and show a flash message" do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("invalid_users.csv") }, user: { supported_permission_ids: [] } }
+        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("invalid_users.csv") } }
 
         assert_template :new
         assert_match(/Couldn't understand that file/i, flash[:alert])
@@ -170,7 +146,7 @@ def users_csv(filename = "users.csv")
 
     context "the CSV has no headers?" do
       should "redisplay the form and show a flash message" do
-        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("no_headers_users.csv") }, user: { supported_permission_ids: [] } }
+        post :create, params: { batch_invitation: { user_names_and_emails: users_csv("no_headers_users.csv") } }
 
         assert_template :new
         assert_match(/must have headers/i, flash[:alert])

diff --git a/test/integration/batch_inviting_users_test.rb b/test/integration/batch_inviting_users_test.rb
@@ -88,6 +88,8 @@ class BatchInvitingUsersTest < ActionDispatch::IntegrationTest
       visit new_batch_invitation_path
       path = Rails.root.join("test/fixtures/users.csv")
       attach_file("Choose a CSV file of users with names and email addresses", path)
+      click_button "Manage permissions for new users"
+
       uncheck "Has access to #{support_app.name}?"
       check "Has access to #{@application.name}?"
       unselect "reader", from: "Permissions for #{@application.name}"
@@ -124,11 +126,12 @@ def perform_batch_invite_with_user(user, application, organisation:, fixture_fil
       visit new_batch_invitation_path
       path = Rails.root.join("test/fixtures", fixture_file)
       attach_file("Choose a CSV file of users with names and email addresses", path)
-      check "Has access to #{application.name}?"
       if organisation
         select organisation.name, from: "Organisation"
       end
+      click_button "Manage permissions for new users"
 
+      check "Has access to #{application.name}?"
       click_button "Create users and send emails"
 
       assert_response_contains("Creating a batch of users")