diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb index ac3f275b5..fba1609f6 100644 --- a/app/controllers/batch_invitations_controller.rb +++ b/app/controllers/batch_invitations_controller.rb @@ -15,7 +15,7 @@ def create authorize @batch_invitation unless file_uploaded? - flash[:alert] = "You must upload a file" + flash.now[:alert] = "You must upload a file" render :new return end @@ -23,16 +23,16 @@ def create begin csv = CSV.parse(params[:batch_invitation][:user_names_and_emails].read, headers: true) rescue CSV::MalformedCSVError => e - flash[:alert] = "Couldn't understand that file: #{e.message}" + flash.now[:alert] = "Couldn't understand that file: #{e.message}" render :new return end if csv.empty? - flash[:alert] = "CSV had no rows." + flash.now[:alert] = "CSV had no rows." render :new return elsif %w[Name Email].any? { |required_header| csv.headers.exclude?(required_header) } - flash[:alert] = "CSV must have headers including 'Name' and 'Email'" + flash.now[:alert] = "CSV must have headers including 'Name' and 'Email'" render :new return end @@ -47,7 +47,7 @@ def create batch_user = BatchInvitationUser.new(batch_user_args) unless batch_user.valid? - flash[:alert] = batch_users_error_message(batch_user) + flash.now[:alert] = batch_users_error_message(batch_user) return render :new end diff --git a/app/models/batch_invitation_user.rb b/app/models/batch_invitation_user.rb index 32a312481..ad88f349e 100644 --- a/app/models/batch_invitation_user.rb +++ b/app/models/batch_invitation_user.rb @@ -7,6 +7,8 @@ class BatchInvitationUser < ApplicationRecord validates :outcome, inclusion: { in: [nil, "success", "failed", "skipped"] } before_save :strip_whitespace_from_name + before_validation :strip_whitespace_from_email + before_save :strip_whitespace_from_organisation_slug scope :processed, -> { where.not(outcome: nil) } scope :unprocessed, -> { where(outcome: nil) } @@ -99,4 +101,12 @@ def sanitise_attributes_for_inviting_user_role(raw_attributes, inviting_user) def strip_whitespace_from_name name.strip! end + + def strip_whitespace_from_email + email&.strip! + end + + def strip_whitespace_from_organisation_slug + organisation_slug&.strip! + end end diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb index 81f9b41e6..9f043554f 100644 --- a/test/controllers/batch_invitations_controller_test.rb +++ b/test/controllers/batch_invitations_controller_test.rb @@ -89,7 +89,7 @@ def users_csv(filename = "users.csv") post :create, params: { batch_invitation: { user_names_and_emails: nil } } assert_template :new - assert_match(/You must upload a file/i, flash[:alert]) + assert_match(/You must upload a file/i, flash.now[:alert]) end end @@ -98,7 +98,7 @@ def users_csv(filename = "users.csv") 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]) + assert_match(/One or more emails were invalid/i, flash.now[:alert]) end end @@ -117,7 +117,7 @@ def users_csv(filename = "users.csv") post :create, params: { batch_invitation: { user_names_and_emails: users_csv("empty_users.csv") } } assert_template :new - assert_match(/no rows/i, flash[:alert]) + assert_match(/no rows/i, flash.now[:alert]) end end @@ -126,7 +126,7 @@ def users_csv(filename = "users.csv") 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]) + assert_match(/Couldn't understand that file/i, flash.now[:alert]) end end @@ -135,7 +135,7 @@ def users_csv(filename = "users.csv") 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]) + assert_match(/must have headers/i, flash.now[:alert]) end end end diff --git a/test/integration/batch_inviting_users_test.rb b/test/integration/batch_inviting_users_test.rb index dcbe1cc79..9cfab9834 100644 --- a/test/integration/batch_inviting_users_test.rb +++ b/test/integration/batch_inviting_users_test.rb @@ -25,6 +25,28 @@ class BatchInvitingUsersTest < ActionDispatch::IntegrationTest assert_user_created_and_invited("lara@example.com", @application, organisation: @cabinet_office) assert_user_not_created("emma@example.com") end + + should "only display flash alert once on validation error" do + visit root_path + signin_with(@user) + + visit new_batch_invitation_path + click_button "Manage permissions for new users" + + assert_response_contains "You must upload a file" + + path = Rails.root.join("test/fixtures/users.csv") + attach_file("Upload a CSV file", path) + click_button "Manage permissions for new users" + + batch_invitation = BatchInvitation.last + assert batch_invitation.present? + + invited_user = batch_invitation.batch_invitation_users.last + assert_equal "fred@example.com", invited_user.email + + refute_response_contains "You must upload a file" + end end context "for admin users" do diff --git a/test/models/batch_invitation_user_test.rb b/test/models/batch_invitation_user_test.rb index b1e3476e9..8f6604899 100644 --- a/test/models/batch_invitation_user_test.rb +++ b/test/models/batch_invitation_user_test.rb @@ -7,22 +7,41 @@ class BatchInvitationUserTest < ActiveSupport::TestCase assert_equal "Ailean Millard", user.name end + + should "strip unwanted whitespace from email before validating" do + user = build(:batch_invitation_user, email: " foo@example.com ") + user.valid? + + assert_equal "foo@example.com", user.email + end + + should "strip unwanted whitespace from organisation_slug before persisting" do + user = create(:batch_invitation_user, organisation_slug: " cabinet-office ") + + assert_equal "cabinet-office", user.organisation_slug + end end context "validations" do - should "validate email address" do + should "validate presence of email address" do + user = build(:batch_invitation_user, email: nil) + + assert_not user.valid? + assert_includes user.errors[:email], "can't be blank" + end + + should "validate format of email address" do user = build(:batch_invitation_user, email: "@gov.uk") assert_not user.valid? - assert_equal ["is invalid"], user.errors[:email] + assert_includes user.errors[:email], "is invalid" end should "prevent user being created with a known non-government email address" do user = build(:batch_invitation_user, email: "piers.quinn@yahoo.co.uk") assert_not user.valid? - assert_equal ["not accepted. Please enter a workplace email to continue."], - user.errors[:email] + assert_includes user.errors[:email], "not accepted. Please enter a workplace email to continue." end should "not allow user to be updated with a known non-government email address" do