From c6fd0b1f112a5d490d6de916afb8bcdb70fbf063 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 21 Sep 2023 15:36:43 +0100 Subject: [PATCH 1/4] Only display batch invitation errors once Trello: https://trello.com/c/aO4nVCZT When setting flash messages prior to *rendering* vs redirecting, it's important to use `flash.now` vs `flash` to avoid the error potentially appearing on a later page thus confusing the user. Although I've changed the controller test to assert against `flash.now` instead of `flash` this doesn't seem to force me to make the change in the controller itself. So I've ended up adding an integration test to force me to make the change. --- .../batch_invitations_controller.rb | 10 ++++----- .../batch_invitations_controller_test.rb | 10 ++++----- test/integration/batch_inviting_users_test.rb | 22 +++++++++++++++++++ 3 files changed, 32 insertions(+), 10 deletions(-) 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/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 From 60033ae47f04c9f13dc787e2f8249e03e377d3b0 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 21 Sep 2023 15:49:23 +0100 Subject: [PATCH 2/4] Normalize BatchInvitationUser#email before validation Trello: https://trello.com/c/aO4nVCZT Strip leading & trailing whitespace from BatchInvitationUser#email before validation to avoid unnecessary validation errors confusing users. Note that unlike the strip_whitespace_from_name method, we need to call this from a before_validation callback rather than a before_save callback, because otherwise the email validation can fail due to the presence of the whitespace. As a general principle, I think we should probably implement this whitespace-stripping normalization in a before_validation callback, but I'm going to leave that to whoever picks up this other Trello card [1]. [1]: https://trello.com/c/4QiHXdtE --- app/models/batch_invitation_user.rb | 5 +++++ test/models/batch_invitation_user_test.rb | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/app/models/batch_invitation_user.rb b/app/models/batch_invitation_user.rb index 32a312481..123c417e6 100644 --- a/app/models/batch_invitation_user.rb +++ b/app/models/batch_invitation_user.rb @@ -7,6 +7,7 @@ class BatchInvitationUser < ApplicationRecord validates :outcome, inclusion: { in: [nil, "success", "failed", "skipped"] } before_save :strip_whitespace_from_name + before_validation :strip_whitespace_from_email scope :processed, -> { where.not(outcome: nil) } scope :unprocessed, -> { where(outcome: nil) } @@ -99,4 +100,8 @@ 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 end diff --git a/test/models/batch_invitation_user_test.rb b/test/models/batch_invitation_user_test.rb index b1e3476e9..e95618572 100644 --- a/test/models/batch_invitation_user_test.rb +++ b/test/models/batch_invitation_user_test.rb @@ -7,6 +7,13 @@ 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 end context "validations" do From b8513bba320f1c032025d8998d2dd71b4e3aa50b Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 21 Sep 2023 16:05:08 +0100 Subject: [PATCH 3/4] Normalize BatchInvitationUser#organisation_slug before save Trello: https://trello.com/c/aO4nVCZT Strip leading & trailing whitespace from BatchInvitationUser#organisation_slug before save to avoid unnecessary failed organisaion lookups. I decided to add this normalization even though we aren't aware of failed organisation lookups happening, because the call to `Organisation.find_by(slug: organisation_slug)` in `BatchInvitationUser#organisation_from_slug` wouldn't find an organisation if an organisation_slug had e.g. leading whitespace. Since there's no validation on organisation_slug, I've elected to trigger strip_whitespace_from_organisation_slug from a before_save callback c.f. BatchInvitationUser#email. Also the lack of organisation_slug presence validation means that strip_whitespace_from_organisation_slug can't rely on the organisation_slug not being nil, so we have to use the safe navigation operator (&.) in this method. As a general principle, I think we should probably implement this whitespace-stripping normalization in a before_validation callback, but I'm going to leave that to whoever picks up this other Trello card [1]. [1]: https://trello.com/c/4QiHXdtE --- app/models/batch_invitation_user.rb | 5 +++++ test/models/batch_invitation_user_test.rb | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/app/models/batch_invitation_user.rb b/app/models/batch_invitation_user.rb index 123c417e6..c25a00755 100644 --- a/app/models/batch_invitation_user.rb +++ b/app/models/batch_invitation_user.rb @@ -8,6 +8,7 @@ class BatchInvitationUser < ApplicationRecord 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) } @@ -104,4 +105,8 @@ def strip_whitespace_from_name def strip_whitespace_from_email email.strip! end + + def strip_whitespace_from_organisation_slug + organisation_slug&.strip! + end end diff --git a/test/models/batch_invitation_user_test.rb b/test/models/batch_invitation_user_test.rb index e95618572..36f9b3f3a 100644 --- a/test/models/batch_invitation_user_test.rb +++ b/test/models/batch_invitation_user_test.rb @@ -14,6 +14,12 @@ class BatchInvitationUserTest < ActiveSupport::TestCase 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 From 4f57c62098c2c13526d3053a9f4486e7ed75c8a4 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 21 Sep 2023 16:25:51 +0100 Subject: [PATCH 4/4] Fix normalization of BatchInvitationUser#email Trello: https://trello.com/c/aO4nVCZT Having implemented strip_whitespace_from_organisation_slug I realised I'd probably missed a scenario for strip_whitespace_from_email. And sure enough there was no test coverage for the case when BatchInvitationUser#email is nil. In this commit I've improved the test coverage for email validation which for one thing explicitly forces the presence validation to be enabled for the email attribute, and for another, it forces me to use the safe navigation operator in strip_whitespace_from_email. This is because the latter is called from a before_validation callback and so email could be nil at this point. --- app/models/batch_invitation_user.rb | 2 +- test/models/batch_invitation_user_test.rb | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/models/batch_invitation_user.rb b/app/models/batch_invitation_user.rb index c25a00755..ad88f349e 100644 --- a/app/models/batch_invitation_user.rb +++ b/app/models/batch_invitation_user.rb @@ -103,7 +103,7 @@ def strip_whitespace_from_name end def strip_whitespace_from_email - email.strip! + email&.strip! end def strip_whitespace_from_organisation_slug diff --git a/test/models/batch_invitation_user_test.rb b/test/models/batch_invitation_user_test.rb index 36f9b3f3a..8f6604899 100644 --- a/test/models/batch_invitation_user_test.rb +++ b/test/models/batch_invitation_user_test.rb @@ -23,19 +23,25 @@ class BatchInvitationUserTest < ActiveSupport::TestCase 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