From 0c492eccbf7e5d5bbfc6257a15c7ecd960723d13 Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Thu, 7 Sep 2023 18:08:00 +0100
Subject: [PATCH 01/12] Add test coverage for helper method

I'm planning on introducing a new "status" to BatchInvitations and
I believe this helper method is where that concept currently lives.

I've avoided the temptation to test every possible scenario
exhaustively. Before I looked this closely, though, I'd imagined that
this status and BatchInvitation#outcome mapped 1:1, and that
BatchInvitation#outcome and BatchInvitationUser#outcome were related.
I hope that these tests are clear enough to demonstrate that neither of
those assumptions are true.
---
 test/helpers/batch_invitations_helper_test.rb | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/test/helpers/batch_invitations_helper_test.rb b/test/helpers/batch_invitations_helper_test.rb
index 32d87109b..217260c5d 100644
--- a/test/helpers/batch_invitations_helper_test.rb
+++ b/test/helpers/batch_invitations_helper_test.rb
@@ -1,6 +1,38 @@
 require "test_helper"
 
 class BatchInvitationsHelperTest < ActionView::TestCase
+  context "#batch_invite_status_message" do
+    should "state number of users processed so far when still in progress" do
+      batch_invitation = create(:batch_invitation, outcome: nil)
+      create(:batch_invitation_user, outcome: "failed", batch_invitation:)
+      create(:batch_invitation_user, outcome: "skipped", batch_invitation:)
+      create(:batch_invitation_user, outcome: "success", batch_invitation:)
+      create(:batch_invitation_user, outcome: nil, batch_invitation:)
+
+      assert_equal "In progress. 3 of 4 users processed.",
+                   batch_invite_status_message(batch_invitation)
+    end
+
+    should "state number of users processed when all were successful" do
+      batch_invitation = create(:batch_invitation, outcome: "success")
+      create(:batch_invitation_user, outcome: "skipped", batch_invitation:)
+      create(:batch_invitation_user, outcome: "success", batch_invitation:)
+
+      assert_equal "2 users processed.",
+                   batch_invite_status_message(batch_invitation)
+    end
+
+    should "state number of failures if any users have failed to process" do
+      batch_invitation = create(:batch_invitation, outcome: "success")
+      create(:batch_invitation_user, outcome: "failed", batch_invitation:)
+      create(:batch_invitation_user, outcome: "skipped", batch_invitation:)
+      create(:batch_invitation_user, outcome: "success", batch_invitation:)
+
+      assert_equal "1 error out of 3 users processed.",
+                   batch_invite_status_message(batch_invitation)
+    end
+  end
+
   context "#batch_invite_organisation_for_user" do
     context "when the batch invitation user raises an invalid slug error when asked for organisation_id" do
       setup do

From 98eb7f31f2253399de43b1d05d6bb0abc0134f6e Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Fri, 8 Sep 2023 17:47:45 +0100
Subject: [PATCH 02/12] Non-completed BatchInvitation can't be successful

I think this change removes an order dependency between
`#all_successful?` and BatchInvitation's other state predicate methods.

We use `#all_successful?` as a proxy for "was the batch invitation
processed successfully?" I don't think it makes sense to say that
a non-completed (potentially in progress or just never started) batch
invitation has been successful purely based on the fact that none of its
batch invitation users have (so far) been marked as "failed".
---
 app/models/batch_invitation.rb       |  2 +-
 test/models/batch_invitation_test.rb | 33 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/app/models/batch_invitation.rb b/app/models/batch_invitation.rb
index 3fb2112ea..47299a778 100644
--- a/app/models/batch_invitation.rb
+++ b/app/models/batch_invitation.rb
@@ -17,7 +17,7 @@ def in_progress?
   end
 
   def all_successful?
-    batch_invitation_users.failed.count.zero?
+    !outcome.nil? && batch_invitation_users.failed.count.zero?
   end
 
   def enqueue
diff --git a/test/models/batch_invitation_test.rb b/test/models/batch_invitation_test.rb
index 08a4dfa2f..f3dc5cf5c 100644
--- a/test/models/batch_invitation_test.rb
+++ b/test/models/batch_invitation_test.rb
@@ -26,6 +26,39 @@ class BatchInvitationTest < ActiveSupport::TestCase
     @bi.save!
   end
 
+  context "#all_successful?" do
+    should "be false when at least one BatchInvitationUser has failed" do
+      @bi.update_column(:outcome, "success")
+      @user_a.update_column(:outcome, "failed")
+
+      assert_not @bi.all_successful?
+    end
+
+    should "be true when no BatchInvitationUsers have failed" do
+      @bi.update_column(:outcome, "success")
+      @user_a.update_column(:outcome, "success")
+      @user_b.update_column(:outcome, "success")
+
+      assert @bi.all_successful?
+    end
+
+    should "be true even if outcome is 'fail' as long as no BatchInvitationUsers have failed" do
+      @bi.update_column(:outcome, "fail")
+      @user_a.update_column(:outcome, "success")
+      @user_b.update_column(:outcome, "success")
+
+      assert @bi.all_successful?
+    end
+
+    should "be false when BatchInvitation is still in progress" do
+      @bi.update_column(:outcome, nil)
+      @user_a.update_column(:outcome, "success")
+      @user_b.update_column(:outcome, "success")
+
+      assert_not @bi.all_successful?
+    end
+  end
+
   context "perform" do
     should "create the users and assign them permissions" do
       @bi.reload.perform

From a8f0fd05a21601e24ecf27235e7a115e594851ee Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Thu, 7 Sep 2023 09:59:44 +0100
Subject: [PATCH 03/12] BatchInvitation#has_permissions?

We're planning to break BatchInvitation creation into two separate
steps:
  1. Upload user info
  2. Manage permissions

One way of making that work requires us to know whether a given batch
invitation has already had permissions assigned to it or not
---
 app/models/batch_invitation.rb       |  4 ++++
 test/models/batch_invitation_test.rb | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/app/models/batch_invitation.rb b/app/models/batch_invitation.rb
index 47299a778..b5e7b5b74 100644
--- a/app/models/batch_invitation.rb
+++ b/app/models/batch_invitation.rb
@@ -12,6 +12,10 @@ class BatchInvitation < ApplicationRecord
   validates :outcome, inclusion: { in: [nil, "success", "fail"] }
   validates :user_id, presence: true
 
+  def has_permissions?
+    batch_invitation_application_permissions.exists?
+  end
+
   def in_progress?
     outcome.nil?
   end
diff --git a/test/models/batch_invitation_test.rb b/test/models/batch_invitation_test.rb
index f3dc5cf5c..c7b35cd54 100644
--- a/test/models/batch_invitation_test.rb
+++ b/test/models/batch_invitation_test.rb
@@ -26,6 +26,20 @@ class BatchInvitationTest < ActiveSupport::TestCase
     @bi.save!
   end
 
+  context "#has_permissions?" do
+    should "be false when BatchInvitation has no batch_invitation_application_permissions" do
+      invitation = create(:batch_invitation)
+
+      assert_not invitation.has_permissions?
+    end
+
+    should "be true when BatchInvitation has any batch_invitation_application_permissions at all" do
+      invitation = create(:batch_invitation, supported_permissions: [@app.signin_permission])
+
+      assert invitation.has_permissions?
+    end
+  end
+
   context "#all_successful?" do
     should "be false when at least one BatchInvitationUser has failed" do
       @bi.update_column(:outcome, "success")

From e75600190cc41d3d8fc654d0102d25ad7d9818ac Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Fri, 8 Sep 2023 16:08:28 +0100
Subject: [PATCH 04/12] BatchInvitation needs permissions to be inprogress

This change makes the use of the `#has_permissions?` and `#in_progress?`
predicates independent of one another (i.e. they don't always need to be
called together, in the correct order) and it protects batch invitations
in a very specific state from being misidentified.

We're going to break the batch invitation creation process into two
parts, the second part being the selection of the batch invitation's
permissions. A batch invitation won't technically be in-progress (i.e.
inviting users and sending out emails) until its permissions have been
selected. In theory, a user could abandon a half-finished batch
invitation, leaving it created in the database but without permissions.

Since it's already true now that a batch invitation isn't in-progress
until its permissions have been selected, this change in logic doesn't
change existing behaviour.
---
 app/models/batch_invitation.rb                |  2 +-
 .../batch_invitations_controller_test.rb      |  4 ++--
 test/factories/batch_invitation.rb            | 15 ++++++++++++++
 test/helpers/batch_invitations_helper_test.rb |  2 +-
 test/models/batch_invitation_test.rb          | 20 +++++++++++++++++++
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/app/models/batch_invitation.rb b/app/models/batch_invitation.rb
index b5e7b5b74..29cc916f1 100644
--- a/app/models/batch_invitation.rb
+++ b/app/models/batch_invitation.rb
@@ -17,7 +17,7 @@ def has_permissions?
   end
 
   def in_progress?
-    outcome.nil?
+    outcome.nil? && has_permissions?
   end
 
   def all_successful?
diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb
index 1d9ca2a24..317123a20 100644
--- a/test/controllers/batch_invitations_controller_test.rb
+++ b/test/controllers/batch_invitations_controller_test.rb
@@ -21,7 +21,7 @@ def users_csv(filename = "users.csv")
 
     context "some batches created recently" do
       setup do
-        @bi = create(:batch_invitation)
+        @bi = create(:batch_invitation, :in_progress)
         create(:batch_invitation_user, batch_invitation: @bi)
       end
 
@@ -180,7 +180,7 @@ def users_csv(filename = "users.csv")
 
   context "GET show" do
     setup do
-      @bi = create(:batch_invitation)
+      @bi = create(:batch_invitation, :in_progress)
       @user1 = create(:batch_invitation_user, name: "A", email: "a@m.com", batch_invitation: @bi)
       @user2 = create(:batch_invitation_user, name: "B", email: "b@m.com", batch_invitation: @bi)
     end
diff --git a/test/factories/batch_invitation.rb b/test/factories/batch_invitation.rb
index 9b00d1dbd..db6ed6a83 100644
--- a/test/factories/batch_invitation.rb
+++ b/test/factories/batch_invitation.rb
@@ -4,5 +4,20 @@
     trait :with_organisation do
       association :organisation, factory: :organisation
     end
+
+    trait :in_progress do
+      outcome { nil }
+
+      has_permissions
+    end
+
+    trait :has_permissions do
+      after(:create) do |batch_invitation|
+        unless batch_invitation.has_permissions?
+          batch_invitation.supported_permissions << create(:supported_permission)
+          batch_invitation.save!
+        end
+      end
+    end
   end
 end
diff --git a/test/helpers/batch_invitations_helper_test.rb b/test/helpers/batch_invitations_helper_test.rb
index 217260c5d..565091faa 100644
--- a/test/helpers/batch_invitations_helper_test.rb
+++ b/test/helpers/batch_invitations_helper_test.rb
@@ -3,7 +3,7 @@
 class BatchInvitationsHelperTest < ActionView::TestCase
   context "#batch_invite_status_message" do
     should "state number of users processed so far when still in progress" do
-      batch_invitation = create(:batch_invitation, outcome: nil)
+      batch_invitation = create(:batch_invitation, :in_progress)
       create(:batch_invitation_user, outcome: "failed", batch_invitation:)
       create(:batch_invitation_user, outcome: "skipped", batch_invitation:)
       create(:batch_invitation_user, outcome: "success", batch_invitation:)
diff --git a/test/models/batch_invitation_test.rb b/test/models/batch_invitation_test.rb
index c7b35cd54..f211d9f20 100644
--- a/test/models/batch_invitation_test.rb
+++ b/test/models/batch_invitation_test.rb
@@ -40,6 +40,26 @@ class BatchInvitationTest < ActiveSupport::TestCase
     end
   end
 
+  context "#in_progress?" do
+    should "be false when BatchInvitation has an outcome" do
+      @bi.update_column(:outcome, "success")
+
+      assert_not @bi.in_progress?
+    end
+
+    should "be true when BatchInvitation does not have an outcome yet" do
+      @bi.update_column(:outcome, nil)
+
+      assert @bi.in_progress?
+    end
+
+    should "be false when BatchInvitation does not have any permissions yet" do
+      invitation = create(:batch_invitation, outcome: nil)
+
+      assert_not invitation.in_progress?
+    end
+  end
+
   context "#all_successful?" do
     should "be false when at least one BatchInvitationUser has failed" do
       @bi.update_column(:outcome, "success")

From 2d2d9e9abc81086e0d1673100ae0522b80c5e9c0 Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Fri, 8 Sep 2023 18:05:40 +0100
Subject: [PATCH 05/12] State for BatchInvitation without permissions

There's no way currently, even after we've broken the batch invitation
from across two pages, that a BatchInvitation without any permissions
will appear on a page that wants to describe its state. But I don't
find it unlikely that this could change without notice, so I'm adding
this with future us in mind
---
 app/helpers/batch_invitations_helper.rb       |  2 ++
 test/helpers/batch_invitations_helper_test.rb | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/app/helpers/batch_invitations_helper.rb b/app/helpers/batch_invitations_helper.rb
index 9f49e651d..90cfd7c3b 100644
--- a/app/helpers/batch_invitations_helper.rb
+++ b/app/helpers/batch_invitations_helper.rb
@@ -7,6 +7,8 @@ def batch_invite_status_message(batch_invitation)
         "users processed."
     elsif batch_invitation.all_successful?
       "#{batch_invitation.batch_invitation_users.count} users processed."
+    elsif !batch_invitation.has_permissions?
+      "Batch invitation doesn't have any permissions yet."
     else
       "#{pluralize(batch_invitation.batch_invitation_users.failed.count, 'error')} out of " \
         "#{batch_invitation.batch_invitation_users.count} " \
diff --git a/test/helpers/batch_invitations_helper_test.rb b/test/helpers/batch_invitations_helper_test.rb
index 565091faa..f7190cde5 100644
--- a/test/helpers/batch_invitations_helper_test.rb
+++ b/test/helpers/batch_invitations_helper_test.rb
@@ -14,7 +14,11 @@ class BatchInvitationsHelperTest < ActionView::TestCase
     end
 
     should "state number of users processed when all were successful" do
-      batch_invitation = create(:batch_invitation, outcome: "success")
+      batch_invitation = create(
+        :batch_invitation,
+        :has_permissions,
+        outcome: "success",
+      )
       create(:batch_invitation_user, outcome: "skipped", batch_invitation:)
       create(:batch_invitation_user, outcome: "success", batch_invitation:)
 
@@ -23,7 +27,11 @@ class BatchInvitationsHelperTest < ActionView::TestCase
     end
 
     should "state number of failures if any users have failed to process" do
-      batch_invitation = create(:batch_invitation, outcome: "success")
+      batch_invitation = create(
+        :batch_invitation,
+        :has_permissions,
+        outcome: "success",
+      )
       create(:batch_invitation_user, outcome: "failed", batch_invitation:)
       create(:batch_invitation_user, outcome: "skipped", batch_invitation:)
       create(:batch_invitation_user, outcome: "success", batch_invitation:)
@@ -31,6 +39,13 @@ class BatchInvitationsHelperTest < ActionView::TestCase
       assert_equal "1 error out of 3 users processed.",
                    batch_invite_status_message(batch_invitation)
     end
+
+    should "explain the problem for a batch invitation that has no permissions" do
+      batch_invitation = create(:batch_invitation)
+
+      assert_equal "Batch invitation doesn't have any permissions yet.",
+                   batch_invite_status_message(batch_invitation)
+    end
   end
 
   context "#batch_invite_organisation_for_user" do

From 5141e008c093c7bea2e55b63d70cfa12d49c750e Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Fri, 8 Sep 2023 17:25:39 +0100
Subject: [PATCH 06/12] Group tests in `context`

---
 .../batch_invitations_controller_test.rb      | 56 ++++++++++---------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb
index 317123a20..2edfe802f 100644
--- a/test/controllers/batch_invitations_controller_test.rb
+++ b/test/controllers/batch_invitations_controller_test.rb
@@ -179,40 +179,44 @@ def users_csv(filename = "users.csv")
   end
 
   context "GET show" do
-    setup do
-      @bi = create(:batch_invitation, :in_progress)
-      @user1 = create(:batch_invitation_user, name: "A", email: "a@m.com", batch_invitation: @bi)
-      @user2 = create(:batch_invitation_user, name: "B", email: "b@m.com", batch_invitation: @bi)
-    end
+    context "processing in progress" do
+      setup do
+        @bi = create(:batch_invitation, :in_progress)
+        @user1 = create(:batch_invitation_user, name: "A", email: "a@m.com", batch_invitation: @bi)
+        @user2 = create(:batch_invitation_user, name: "B", email: "b@m.com", batch_invitation: @bi)
+      end
 
-    should "list the users being created" do
-      get :show, params: { id: @bi.id }
-      assert_select "table tbody tr", 2
-      assert_select "table td", "a@m.com"
-      assert_select "table td", "b@m.com"
-    end
+      should "list the users being created" do
+        get :show, params: { id: @bi.id }
+        assert_select "table tbody tr", 2
+        assert_select "table td", "a@m.com"
+        assert_select "table td", "b@m.com"
+      end
 
-    should "include a meta refresh" do
-      get :show, params: { id: @bi.id }
-      assert_select 'head meta[http-equiv=refresh][content="3"]'
-    end
+      should "include a meta refresh" do
+        get :show, params: { id: @bi.id }
+        assert_select 'head meta[http-equiv=refresh][content="3"]'
+      end
 
-    should "show the state of the processing" do
-      @user1.update_column(:outcome, "failed")
-      get :show, params: { id: @bi.id }
-      assert_select "section.gem-c-notice", /In progress/i
-      assert_select "section.gem-c-notice", /1 of 2 users processed/i
-    end
+      should "show the state of the processing" do
+        @user1.update_column(:outcome, "failed")
+        get :show, params: { id: @bi.id }
+        assert_select "section.gem-c-notice", /In progress/i
+        assert_select "section.gem-c-notice", /1 of 2 users processed/i
+      end
 
-    should "show the outcome for each user" do
-      @user1.update_column(:outcome, "failed")
-      get :show, params: { id: @bi.id }
-      assert_select "td", /Failed/i
+      should "show the outcome for each user" do
+        @user1.update_column(:outcome, "failed")
+        get :show, params: { id: @bi.id }
+        assert_select "td", /Failed/i
+      end
     end
 
     context "processing complete" do
       setup do
-        @bi.update_column(:outcome, "success")
+        @bi = create(:batch_invitation, outcome: "success")
+        create(:batch_invitation_user, name: "A", email: "a@m.com", batch_invitation: @bi)
+        create(:batch_invitation_user, name: "B", email: "b@m.com", batch_invitation: @bi)
         get :show, params: { id: @bi.id }
       end
 

From c007777d1167080ac23edce4b7a1cfe5314d8812 Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Fri, 8 Sep 2023 18:12:57 +0100
Subject: [PATCH 07/12] Test batch_invitations#show without permissions

A test documenting the behaviour when a user visits the #show page for
a BatchInvitation that doesn't have any permissions (it just happened to
already work this way)
---
 .../batch_invitations_controller_test.rb       | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb
index 2edfe802f..a4a4a2791 100644
--- a/test/controllers/batch_invitations_controller_test.rb
+++ b/test/controllers/batch_invitations_controller_test.rb
@@ -228,5 +228,23 @@ def users_csv(filename = "users.csv")
         assert_select "head meta[http-equiv=refresh]", count: 0
       end
     end
+
+    context "batch invitation doesn't have any permissions yet" do
+      setup do
+        @bi = create(:batch_invitation)
+        create(:batch_invitation_user, name: "A", email: "a@m.com", batch_invitation: @bi)
+        create(:batch_invitation_user, name: "B", email: "b@m.com", batch_invitation: @bi)
+        get :show, params: { id: @bi.id }
+      end
+
+      should "explain the problem with the batch invitation" do
+        assert_select "div.gem-c-error-alert",
+                      /Batch invitation doesn't have any permissions yet./
+      end
+
+      should "not include the meta refresh" do
+        assert_select "head meta[http-equiv=refresh]", count: 0
+      end
+    end
   end
 end

From 86a7bfe0eb5d9f9cb828abae0e5d28a3e4475bb1 Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Thu, 7 Sep 2023 16:26:05 +0100
Subject: [PATCH 08/12] Introduce batch_invitation_permissions resource

This is mostly copy-pasted from BatchInvitationController's existing
permissions-handling code. We're en route to breaking the batch
invitation creation process into a two step form and this is going to be
the second step.
---
 ...batch_invitation_permissions_controller.rb | 37 +++++++++
 .../batch_invitation_permissions/new.html.erb | 13 +++
 config/routes.rb                              |  7 +-
 ..._invitation_permissions_controller_test.rb | 83 +++++++++++++++++++
 4 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 app/controllers/batch_invitation_permissions_controller.rb
 create mode 100644 app/views/batch_invitation_permissions/new.html.erb
 create mode 100644 test/controllers/batch_invitation_permissions_controller_test.rb

diff --git a/app/controllers/batch_invitation_permissions_controller.rb b/app/controllers/batch_invitation_permissions_controller.rb
new file mode 100644
index 000000000..8225d1bb3
--- /dev/null
+++ b/app/controllers/batch_invitation_permissions_controller.rb
@@ -0,0 +1,37 @@
+class BatchInvitationPermissionsController < ApplicationController
+  include UserPermissionsControllerMethods
+  before_action :authenticate_user!
+  before_action :load_batch_invitation
+  before_action :authorise_to_manage_permissions
+
+  helper_method :applications_and_permissions
+
+  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!
+
+    @batch_invitation.enqueue
+    flash[:notice] = "Scheduled invitation of #{@batch_invitation.batch_invitation_users.count} users"
+    redirect_to batch_invitation_path(@batch_invitation)
+  end
+
+private
+
+  def load_batch_invitation
+    @batch_invitation = current_user.batch_invitations.find(params[:batch_invitation_id])
+  end
+
+  def authorise_to_manage_permissions
+    authorize @batch_invitation
+  end
+
+  def grant_default_permissions(batch_invitation)
+    SupportedPermission.default.each do |default_permission|
+      batch_invitation.grant_permission(default_permission)
+    end
+  end
+end
diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb
new file mode 100644
index 000000000..346f0a3e1
--- /dev/null
+++ b/app/views/batch_invitation_permissions/new.html.erb
@@ -0,0 +1,13 @@
+<% content_for :title, "Manage permissions for new users" %>
+
+<div class="page-title">
+  <h1>Manage permissions for new users</h1>
+</div>
+
+<div class="well">
+  <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %>
+    <%= render partial: "shared/user_permissions", locals: { user_object: User.new } %>
+
+    <%= f.submit "Create users and send emails", :class => 'btn btn-success' %>
+  <% end %>
+</div>
diff --git a/config/routes.rb b/config/routes.rb
index 6ee78fad9..464d26df1 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -54,7 +54,12 @@
     resources :applications, only: [:index]
   end
 
-  resources :batch_invitations, only: %i[new create show]
+  resources :batch_invitations, only: %i[new create show] do
+    resource :permissions,
+             only: %i[new create],
+             controller: :batch_invitation_permissions
+  end
+
   resources :bulk_grant_permission_sets, only: %i[new create show]
   resources :organisations, only: %i[index edit update]
   resources :suspensions, only: %i[edit update]
diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb
new file mode 100644
index 000000000..714e85104
--- /dev/null
+++ b/test/controllers/batch_invitation_permissions_controller_test.rb
@@ -0,0 +1,83 @@
+require "test_helper"
+
+class BatchInvitationPermissionsControllerTest < ActionController::TestCase
+  include ActiveJob::TestHelper
+
+  setup do
+    @user = create(:admin_user)
+    sign_in @user
+
+    @app = create(:application, name: "Profound Publisher")
+
+    @batch_invitation = create(:batch_invitation, user: @user)
+    create(
+      :batch_invitation_user,
+      name: "Darayavaush Ayers",
+      email: "darayavaush.ayers@department.gov.uk",
+      batch_invitation: @batch_invitation,
+    )
+    create(
+      :batch_invitation_user,
+      name: "Precious Kumar",
+      email: "precious.kumar@department.gov.uk",
+      batch_invitation: @batch_invitation,
+    )
+  end
+
+  context "GET new" do
+    should "allow selection of application permissions to grant to users" do
+      get :new, params: { batch_invitation_id: @batch_invitation.id }
+
+      assert_select "table#editable-permissions" do
+        assert_select "td", "Has access to Profound Publisher?"
+        assert_select "td", "Permissions for Profound Publisher"
+      end
+    end
+  end
+
+  context "POST create" do
+    should "grant selected permissions and 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,
+        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
+    end
+
+    should "send an email to signon-alerts" do
+      perform_enqueued_jobs do
+        post :create, params: { batch_invitation_id: @batch_invitation.id }
+
+        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_id: @batch_invitation.id }
+
+      assert_match(/Scheduled invitation of 2 users/i, flash[:notice])
+      assert_redirected_to "/batch_invitations/#{@batch_invitation.id}"
+    end
+  end
+end

From ea8d58deeb2cb88dc966361d5097b5c16060588a Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Thu, 7 Sep 2023 16:59:08 +0100
Subject: [PATCH 09/12] Pundit policy for batch invitation permissions

Instead of freeloading on the existing BatchInvitation controller action
policies
---
 app/controllers/batch_invitation_permissions_controller.rb | 2 +-
 app/policies/batch_invitation_policy.rb                    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/controllers/batch_invitation_permissions_controller.rb b/app/controllers/batch_invitation_permissions_controller.rb
index 8225d1bb3..80329c46c 100644
--- a/app/controllers/batch_invitation_permissions_controller.rb
+++ b/app/controllers/batch_invitation_permissions_controller.rb
@@ -26,7 +26,7 @@ def load_batch_invitation
   end
 
   def authorise_to_manage_permissions
-    authorize @batch_invitation
+    authorize @batch_invitation, :manage_permissions?
   end
 
   def grant_default_permissions(batch_invitation)
diff --git a/app/policies/batch_invitation_policy.rb b/app/policies/batch_invitation_policy.rb
index d08587d6a..dc8e90d69 100644
--- a/app/policies/batch_invitation_policy.rb
+++ b/app/policies/batch_invitation_policy.rb
@@ -6,6 +6,7 @@ def new?
   end
   alias_method :create?, :new?
   alias_method :show?, :new?
+  alias_method :manage_permissions?, :new?
 
   def assign_organisation_from_csv?
     current_user.govuk_admin?

From 6d1742258d12ffbf23152d4cfa78f41f2772da1a Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Thu, 7 Sep 2023 16:59:41 +0100
Subject: [PATCH 10/12] Prevent updating permissions

This form is the second step of the batch invitation creation process.
Once a batch invitation's permissions have been set, it's considered to
be "in progress" and these details shouldn't be changed.
---
 ...batch_invitation_permissions_controller.rb |  8 ++++++++
 ..._invitation_permissions_controller_test.rb | 20 +++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/app/controllers/batch_invitation_permissions_controller.rb b/app/controllers/batch_invitation_permissions_controller.rb
index 80329c46c..af73635c1 100644
--- a/app/controllers/batch_invitation_permissions_controller.rb
+++ b/app/controllers/batch_invitation_permissions_controller.rb
@@ -3,6 +3,7 @@ class BatchInvitationPermissionsController < ApplicationController
   before_action :authenticate_user!
   before_action :load_batch_invitation
   before_action :authorise_to_manage_permissions
+  before_action :prevent_updating
 
   helper_method :applications_and_permissions
 
@@ -29,6 +30,13 @@ def authorise_to_manage_permissions
     authorize @batch_invitation, :manage_permissions?
   end
 
+  def prevent_updating
+    if @batch_invitation.has_permissions?
+      flash[:alert] = "Permissions have already been set for this batch of users"
+      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)
diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb
index 714e85104..59e82e838 100644
--- a/test/controllers/batch_invitation_permissions_controller_test.rb
+++ b/test/controllers/batch_invitation_permissions_controller_test.rb
@@ -25,6 +25,16 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase
   end
 
   context "GET new" do
+    should "not allow access if batch invitation already has permissions" do
+      @batch_invitation.supported_permission_ids = [@app.signin_permission.id]
+      @batch_invitation.save!
+
+      get :new, params: { batch_invitation_id: @batch_invitation.id }
+
+      assert_match(/Permissions have already been set for this batch of users/, flash[:alert])
+      assert_redirected_to "/batch_invitations/#{@batch_invitation.id}"
+    end
+
     should "allow selection of application permissions to grant to users" do
       get :new, params: { batch_invitation_id: @batch_invitation.id }
 
@@ -36,6 +46,16 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase
   end
 
   context "POST create" do
+    should "not accept submission if batch invitation already has permissions" do
+      @batch_invitation.supported_permission_ids = [@app.signin_permission.id]
+      @batch_invitation.save!
+
+      post :create, params: { batch_invitation_id: @batch_invitation.id }
+
+      assert_match(/Permissions have already been set for this batch of users/, flash[:alert])
+      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)

From f8456e71eda1a10cdf5b10561ee915225bacb7e0 Mon Sep 17 00:00:00 2001
From: Mike Patrick <mike.patrick@digital.cabinet-office.gov.uk>
Date: Thu, 7 Sep 2023 09:44:20 +0100
Subject: [PATCH 11/12] 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.
---
 .../batch_invitations_controller.rb           | 14 +----
 app/views/batch_invitations/new.html.erb      |  6 +--
 .../batch_invitations_controller_test.rb      | 52 +++++--------------
 test/integration/batch_inviting_users_test.rb |  5 +-
 4 files changed, 20 insertions(+), 57 deletions(-)

diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb
index d3f8686e6..b51a54d87 100644
--- 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
index e95b89019..59da3281c 100644
--- 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
index a4a4a2791..d516c80d8 100644
--- 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
index fea5ca3ea..518d1be28 100644
--- 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")

From 37397e3a5765bc6564a62d82bbb8f63bd0d6e5cb Mon Sep 17 00:00:00 2001
From: Chris Lowis <chris.lowis@digital.cabinet-office.gov.uk>
Date: Mon, 11 Sep 2023 16:44:15 +0100
Subject: [PATCH 12/12] Link to edit the permissions for a batch when they're
 not present

This allows a user to complete an unfinished batch (i.e. one where the
user has uploaded a CSV file but not selected any permissions).
---
 app/helpers/batch_invitations_helper.rb       |  8 ++++++++
 app/views/batch_invitations/new.html.erb      |  2 +-
 test/helpers/batch_invitations_helper_test.rb | 14 ++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/app/helpers/batch_invitations_helper.rb b/app/helpers/batch_invitations_helper.rb
index 90cfd7c3b..30778c38a 100644
--- a/app/helpers/batch_invitations_helper.rb
+++ b/app/helpers/batch_invitations_helper.rb
@@ -1,4 +1,12 @@
 module BatchInvitationsHelper
+  def batch_invite_status_link(batch_invitation, &block)
+    if !batch_invitation.has_permissions?
+      link_to(new_batch_invitation_permissions_path(batch_invitation), alt: "Edit this batch's permissions", &block)
+    else
+      link_to(batch_invitation_path(batch_invitation), alt: "View this batch", &block)
+    end
+  end
+
   def batch_invite_status_message(batch_invitation)
     if batch_invitation.in_progress?
       "In progress. " \
diff --git a/app/views/batch_invitations/new.html.erb b/app/views/batch_invitations/new.html.erb
index 59da3281c..6efdc0db3 100644
--- a/app/views/batch_invitations/new.html.erb
+++ b/app/views/batch_invitations/new.html.erb
@@ -17,7 +17,7 @@
       <% recent_batch_invitations.each do |batch_invitation| %>
         <tr>
           <td>
-            <%= link_to(batch_invitation_path(batch_invitation), alt: "View this batch") do %>
+            <%= batch_invite_status_link(batch_invitation) do %>
               <%= batch_invitation.batch_invitation_users.count %> users by <%= batch_invitation.user.name %> at <%= batch_invitation.created_at.to_fs(:govuk_date) %>
             <% end %>
           </td>
diff --git a/test/helpers/batch_invitations_helper_test.rb b/test/helpers/batch_invitations_helper_test.rb
index f7190cde5..9c9b5b132 100644
--- a/test/helpers/batch_invitations_helper_test.rb
+++ b/test/helpers/batch_invitations_helper_test.rb
@@ -93,4 +93,18 @@ class BatchInvitationsHelperTest < ActionView::TestCase
       end
     end
   end
+
+  context "#batch_invite_status_link" do
+    should "link to show the batch when it has permissions" do
+      batch_invitation = create(:batch_invitation, :has_permissions, outcome: "success")
+
+      assert_includes batch_invite_status_link(batch_invitation) {}, batch_invitation_path(batch_invitation)
+    end
+
+    should "link to show edit the permissions when it has no permissions" do
+      batch_invitation = create(:batch_invitation)
+
+      assert_includes batch_invite_status_link(batch_invitation) {}, new_batch_invitation_permissions_path(batch_invitation)
+    end
+  end
 end