Merge pull request #2297 from alphagov/give-batch-uploading-users-mor…

…e-information

Give batch uploading users more information when an email address in their batch is invalid

app/controllers/batch_invitations_controller.rb

@@ -43,8 +43,6 @@ def create
       return
     end
 
-    @batch_invitation.save!
-
     csv.each do |row|
       batch_user_args = {
         batch_invitation: @batch_invitation,
@@ -54,8 +52,18 @@ def create
       if policy(@batch_invitation).assign_organisation_from_csv?
         batch_user_args[:organisation_slug] = row["Organisation"]
       end
-      BatchInvitationUser.create!(batch_user_args)
+      batch_user = BatchInvitationUser.new(batch_user_args)
+
+      unless batch_user.valid?
+        flash[:alert] = batch_users_error_message(batch_user)
+        return render :new
+      end
+
+      @batch_invitation.batch_invitation_users << batch_user
     end
+
+    @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)
@@ -88,4 +96,14 @@ def grant_default_permissions(batch_invitation)
       batch_invitation.grant_permission(default_permission)
     end
   end
+
+  def batch_users_error_message(batch_user)
+    e = batch_user.errors.first
+
+    if e.attribute == :email
+      "One or more emails were invalid"
+    else
+      e.full_message
+    end
+  end
 end

app/models/batch_invitation_user.rb

@@ -1,6 +1,8 @@
 class BatchInvitationUser < ApplicationRecord
   belongs_to :batch_invitation
 
+  validates :email, presence: true, format: { with: Devise.email_regexp }
+
   validates :outcome, inclusion: { in: [nil, "success", "failed", "skipped"] }
 
   before_save :strip_whitespace_from_name

test/controllers/batch_invitations_controller_test.rb

@@ -131,6 +131,15 @@ def users_csv(filename = "users.csv")
       end
     end
 
+    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: [] } }
+
+        assert_template :new
+        assert_match(/One or more emails were invalid/i, flash[:alert])
+      end
+    end
+
     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: [] } }

test/controllers/fixtures/users_with_non_valid_emails.csv

@@ -0,0 +1,5 @@
+Name,Email
+Raphael Gupta,raphael@example.gov.uk
+Bolormaa Śniegowski,@bolo
+Flora Gao,f.gao@example.gov.uk
+Aureliusz Clemente,aureliusz@examplegovuk

test/models/batch_invitation_user_test.rb

@@ -9,6 +9,15 @@ class BatchInvitationUserTest < ActiveSupport::TestCase
     end
   end
 
+  context "validations" do
+    should "validate email address" do
+      user = build(:batch_invitation_user, email: "@gov.uk")
+
+      assert_not user.valid?
+      assert_equal ["is invalid"], user.errors[:email]
+    end
+  end
+
   context "invite" do
     setup do
       @inviting_user = create(:admin_user)
@@ -78,7 +87,8 @@ class BatchInvitationUserTest < ActiveSupport::TestCase
 
     context "the user could not be saved (eg email is blank)" do
       should "record it as a failure" do
-        user = create(:batch_invitation_user, batch_invitation: @batch_invitation, email: nil)
+        user = create(:batch_invitation_user, batch_invitation: @batch_invitation)
+        user.email = nil
         user.invite(@inviting_user, [])
 
         assert_equal "failed", user.reload.outcome
@@ -87,7 +97,8 @@ class BatchInvitationUserTest < ActiveSupport::TestCase
       should "log the error" do
         GovukError.expects(:notify).once
 
-        user = create(:batch_invitation_user, batch_invitation: @batch_invitation, email: nil)
+        user = create(:batch_invitation_user, batch_invitation: @batch_invitation)
+        user.email = nil
         user.invite(@inviting_user, [])
       end
     end