diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb index 379a49921..d3f8686e6 100644 --- a/app/controllers/batch_invitations_controller.rb +++ b/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 diff --git a/app/models/batch_invitation_user.rb b/app/models/batch_invitation_user.rb index 21fe387b3..2fbfef7b9 100644 --- a/app/models/batch_invitation_user.rb +++ b/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 diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb index 05b564ae0..56f7267cf 100644 --- a/test/controllers/batch_invitations_controller_test.rb +++ b/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: [] } } diff --git a/test/controllers/fixtures/users_with_non_valid_emails.csv b/test/controllers/fixtures/users_with_non_valid_emails.csv new file mode 100644 index 000000000..67fd9d3a0 --- /dev/null +++ b/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 diff --git a/test/models/batch_invitation_user_test.rb b/test/models/batch_invitation_user_test.rb index 9e0693ac2..a420c4652 100644 --- a/test/models/batch_invitation_user_test.rb +++ b/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