Merge pull request #4373 from nhsuk/validate-child-names

Validate child names on import

Author: misaka

app/lib/apostrophe_normaliser.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module ApostropheNormaliser
+  def self.call(value)
+    pattern = <<-PATTERN
+      (
+        \u0060   # Grave accent, we have one instance of this already so far
+        | \u2019 # Right single quotation mark, the preferred Unicode apostrophe
+                 # but not supported by PDS
+        | \u02BC # Modifier letter apostrophe, also not supported by PDS
+      )
+    PATTERN
+
+    value&.gsub(Regexp.new(pattern, Regexp::EXTENDED), "'")
+  end
+end

app/models/patient_import_row.rb

@@ -4,17 +4,35 @@ class PatientImportRow
   include ActiveModel::Model
 
   MAX_FIELD_LENGTH = 300
+  VALID_NAME_REGEX = Regexp.new(<<~REGEXP, Regexp::EXTENDED).freeze
+    ^[
+      \\w            # ASCII alphanumeric characters
+      \u00C0-\u00D6  # Latin 1 Supplement letters
+      \u00D8-\u00F6  # Latin 1 Supplement letters
+      \u00F8-\u017F  # Latin 1 Supplement & Latin Extended A letters
+      \u0020         # Space
+      \u0027         # Apostrophe
+      \u0060         # Grave accent, will be normalised to apostrophe
+      \u2019         # Preferred Unicode apostrophe, will be normalised to apostrophe
+      \u02BC         # Modifier apostrophe, will be normalised to apostrophe
+      \u002E         # Full stop
+    -]+$ # Hyphen has to come at the very end, even for an extended regexp
+  REGEXP
 
   validate :validate_date_of_birth,
            :validate_existing_patients,
            :validate_first_name,
+           :validate_preferred_first_name,
            :validate_gender_code,
            :validate_last_name,
+           :validate_preferred_last_name,
            :validate_nhs_number,
            :validate_parent_1_email,
+           :validate_parent_1_name,
            :validate_parent_1_phone,
            :validate_parent_1_relationship,
            :validate_parent_2_email,
+           :validate_parent_2_name,
            :validate_parent_2_phone,
            :validate_parent_2_relationship,
            :validate_year_group
@@ -65,14 +83,14 @@ def to_parents
       if parent_1_exists?
         {
           email: parent_1_email_value,
-          full_name: parent_1_name&.to_s,
+          full_name: parent_1_name_value,
           phone: parent_1_phone_value
         }
       end,
       if parent_2_exists?
         {
           email: parent_2_email_value,
-          full_name: parent_2_name&.to_s,
+          full_name: parent_2_name_value,
           phone: parent_2_phone_value
         }
       end
@@ -182,20 +200,22 @@ def import_attributes
       address_town: address_town&.to_s,
       birth_academic_year: birth_academic_year_value,
       date_of_birth: date_of_birth.to_date,
-      family_name: last_name.to_s,
+      family_name: ApostropheNormaliser.call(last_name.to_s),
       gender_code: gender_code_value,
-      given_name: first_name.to_s,
+      given_name: ApostropheNormaliser.call(first_name.to_s),
       nhs_number: nhs_number_value,
-      preferred_family_name: preferred_last_name&.to_s,
-      preferred_given_name: preferred_first_name&.to_s,
+      preferred_family_name:
+        ApostropheNormaliser.call(preferred_last_name&.to_s),
+      preferred_given_name:
+        ApostropheNormaliser.call(preferred_first_name&.to_s),
       registration: registration&.to_s,
       registration_academic_year:
     }.compact
   end
 
   def parent_1_import_attributes
     {
-      full_name: parent_1_name&.to_s,
+      full_name: parent_1_name_value,
       email: parent_1_email_value,
       phone: parent_1_phone_value,
       relationship: parent_1_relationship&.to_s
@@ -204,7 +224,7 @@ def parent_1_import_attributes
 
   def parent_2_import_attributes
     {
-      full_name: parent_2_name&.to_s,
+      full_name: parent_2_name_value,
       email: parent_2_email_value,
       phone: parent_2_phone_value,
       relationship: parent_2_relationship&.to_s
@@ -356,10 +376,18 @@ def gender_code_value
     gender_code&.to_s&.downcase&.gsub(" ", "_")
   end
 
+  def parent_1_name_value
+    ApostropheNormaliser.call(parent_1_name&.to_s)
+  end
+
   def parent_1_email_value
     parent_1_email&.to_s&.downcase
   end
 
+  def parent_2_name_value
+    ApostropheNormaliser.call(parent_2_name&.to_s)
+  end
+
   def parent_2_email_value
     parent_2_email&.to_s&.downcase
   end
@@ -405,6 +433,8 @@ def validate_first_name
         first_name.header,
         "is greater than #{MAX_FIELD_LENGTH} characters long"
       )
+    elsif !first_name.to_s.match?(VALID_NAME_REGEX)
+      errors.add(first_name.header, "includes invalid character(s)")
     end
   end
 
@@ -415,6 +445,18 @@ def validate_gender_code
     end
   end
 
+  def validate_preferred_first_name
+    return if preferred_first_name.blank?
+    if preferred_first_name.to_s.length > MAX_FIELD_LENGTH
+      errors.add(
+        preferred_first_name.header,
+        "is greater than #{MAX_FIELD_LENGTH} characters long"
+      )
+    elsif !preferred_first_name.to_s.match?(VALID_NAME_REGEX)
+      errors.add(preferred_first_name.header, "includes invalid character(s)")
+    end
+  end
+
   def validate_last_name
     if last_name.nil?
       errors.add(:base, "<code>CHILD_LAST_NAME</code> is missing")
@@ -425,6 +467,20 @@ def validate_last_name
         last_name.header,
         "is greater than #{MAX_FIELD_LENGTH} characters long"
       )
+    elsif !last_name.to_s.match?(VALID_NAME_REGEX)
+      errors.add(last_name.header, "includes invalid character(s)")
+    end
+  end
+
+  def validate_preferred_last_name
+    return if preferred_last_name.blank?
+    if preferred_last_name.to_s.length > MAX_FIELD_LENGTH
+      errors.add(
+        preferred_last_name.header,
+        "is greater than #{MAX_FIELD_LENGTH} characters long"
+      )
+    elsif !preferred_last_name.to_s.match?(VALID_NAME_REGEX)
+      errors.add(preferred_last_name.header, "includes invalid character(s)")
     end
   end
 
@@ -438,6 +494,19 @@ def validate_nhs_number
     ).validate_each(self, nhs_number.header, nhs_number_value)
   end
 
+  def validate_parent_1_name
+    return if parent_1_name.blank?
+
+    if parent_1_name.to_s.length > MAX_FIELD_LENGTH
+      errors.add(
+        parent_1_name.header,
+        "is greater than #{MAX_FIELD_LENGTH} characters long"
+      )
+    elsif !parent_1_name.to_s.match?(VALID_NAME_REGEX)
+      errors.add(parent_1_name.header, "includes invalid character(s)")
+    end
+  end
+
   def validate_parent_1_email
     return if parent_1_email.blank?
 
@@ -465,6 +534,19 @@ def validate_parent_1_relationship
     end
   end
 
+  def validate_parent_2_name
+    return if parent_2_name.blank?
+
+    if parent_2_name.to_s.length > MAX_FIELD_LENGTH
+      errors.add(
+        parent_2_name.header,
+        "is greater than #{MAX_FIELD_LENGTH} characters long"
+      )
+    elsif !parent_2_name.to_s.match?(VALID_NAME_REGEX)
+      errors.add(parent_2_name.header, "includes invalid character(s)")
+    end
+  end
+
   def validate_parent_2_email
     return if parent_2_email.blank?
 

spec/fixtures/cohort_import/invalid_fields.csv

@@ -18,3 +18,5 @@ invalid,John Smith,Father,john@example.com,07412345678,,,,,Jimmy,Smith,Jim,2010-
 123456,John Smith,Father,john@example.com,07412345678,,,,,Jimmy,Smith,Jim,invalid,10 Downing Street,,London,SW1A 1AA,9990000018
 123456,John Smith,Father,john@example.com,07412345678,,,,,Jimmy,Smith,Jim,2010-01-01,10 Downing Street,,London,invalid,9990000018
 123456,John Smith,Father,john@example.com,07412345678,,,,,Jimmy,Smith,Jim,2010-01-01,10 Downing Street,,London,SW1A 1AA,invalid
+123456,John Smith,Father,john@example.com,07412345678,,,,,J£mmy,Smith,Jim,2010-01-01,10 Downing Street,,London,SW1A 1AA,9990000018
+123456,John Smith,Father,john@example.com,07412345678,,,,,Jimmy,Sm©th,Jim,2010-01-01,10 Downing Street,,London,SW1A 1AA,9990000018

spec/models/cohort_import_row_spec.rb

@@ -123,6 +123,82 @@
         )
       end
     end
+
+    context "when the given name has invalid characters" do
+      let(:data) { valid_data.merge("CHILD_FIRST_NAME" => "J£mmy") }
+
+      it "is invalid" do
+        expect(cohort_import_row).to be_invalid
+        expect(cohort_import_row.errors.size).to eq(1)
+        expect(cohort_import_row.errors["CHILD_FIRST_NAME"]).to contain_exactly(
+          "includes invalid character(s)"
+        )
+      end
+    end
+
+    context "when the family name has invalid characters" do
+      let(:data) { valid_data.merge("CHILD_LAST_NAME" => "ᶚmith") }
+
+      it "is invalid" do
+        expect(cohort_import_row).to be_invalid
+        expect(cohort_import_row.errors.size).to eq(1)
+        expect(cohort_import_row.errors["CHILD_LAST_NAME"]).to contain_exactly(
+          "includes invalid character(s)"
+        )
+      end
+    end
+
+    context "when the preferred given name has invalid characters" do
+      let(:data) { valid_data.merge("CHILD_PREFERRED_FIRST_NAME" => "J£m") }
+
+      it "is invalid" do
+        expect(cohort_import_row).to be_invalid
+        expect(cohort_import_row.errors.size).to eq(1)
+        expect(
+          cohort_import_row.errors["CHILD_PREFERRED_FIRST_NAME"]
+        ).to contain_exactly("includes invalid character(s)")
+      end
+    end
+
+    context "when the preferred family name has invalid characters" do
+      let(:data) { valid_data.merge("CHILD_PREFERRED_LAST_NAME" => "ᶚmithy") }
+
+      it "is invalid" do
+        expect(cohort_import_row).to be_invalid
+        expect(cohort_import_row.errors.size).to eq(1)
+        expect(
+          cohort_import_row.errors["CHILD_PREFERRED_LAST_NAME"]
+        ).to contain_exactly("includes invalid character(s)")
+      end
+    end
+
+    context "when the first parent's full name has invalid characters" do
+      let(:data) do
+        valid_data.merge(parent_1_data).merge("PARENT_1_NAME" => "John© Smith")
+      end
+
+      it "is invalid" do
+        expect(cohort_import_row).to be_invalid
+        expect(cohort_import_row.errors.size).to eq(1)
+        expect(cohort_import_row.errors["PARENT_1_NAME"]).to contain_exactly(
+          "includes invalid character(s)"
+        )
+      end
+    end
+
+    context "when the second parent's full name has invalid characters" do
+      let(:data) do
+        valid_data.merge(parent_2_data).merge("PARENT_2_NAME" => "John© Smith")
+      end
+
+      it "is invalid" do
+        expect(cohort_import_row).to be_invalid
+        expect(cohort_import_row.errors.size).to eq(1)
+        expect(cohort_import_row.errors["PARENT_2_NAME"]).to contain_exactly(
+          "includes invalid character(s)"
+        )
+      end
+    end
   end
 
   describe "#to_parents" do
@@ -195,6 +271,23 @@
 
       it { should eq([existing_parent]) }
     end
+
+    context "with fancy apostrophes" do
+      let(:data) do
+        valid_data
+          .merge(parent_1_data)
+          .merge(parent_2_data)
+          .merge(
+            "PARENT_1_NAME" => "Jane OʼReilly",
+            "PARENT_2_NAME" => "Jacob O`Reilly"
+          )
+      end
+
+      it "converts fancy apostrophes to plain apostrophes" do
+        expect(parents.first.full_name).to eq("Jane O'Reilly")
+        expect(parents.second.full_name).to eq("Jacob O'Reilly")
+      end
+    end
   end
 
   describe "#to_patient" do
@@ -747,4 +840,28 @@
       end
     end
   end
+
+  describe "#import_attributes" do
+    context "with fancy apostrophes" do
+      let(:data) do
+        valid_data.merge(
+          {
+            "CHILD_FIRST_NAME" => "Mickʼee",
+            "CHILD_LAST_NAME" => "OʼReilly",
+            "CHILD_PREFERRED_FIRST_NAME" => "Mickʼee",
+            "CHILD_PREFERRED_LAST_NAME" => "OʼReilly"
+          }
+        )
+      end
+
+      it "converts fancy apostrophes to plain apostrophes" do
+        expect(cohort_import_row.import_attributes).to include(
+          given_name: "Mick'ee",
+          family_name: "O'Reilly",
+          preferred_given_name: "Mick'ee",
+          preferred_family_name: "O'Reilly"
+        )
+      end
+    end
+  end
 end

spec/models/cohort_import_spec.rb

@@ -74,6 +74,10 @@
         expect(cohort_import).to be_invalid
         expect(cohort_import.rows).not_to be_empty
       end
+
+      it "is invalid" do
+        expect(cohort_import).not_to be_valid
+      end
     end
 
     describe "with unrecognised fields" do
@@ -400,5 +404,23 @@
         expect { process! }.to change(session.patients, :count).from(0).to(2)
       end
     end
+
+    context "with invalid fields" do
+      before { process! }
+
+      let(:file) { "invalid_fields.csv" }
+
+      describe "error for row with first name having invalid characters" do
+        subject(:child_first_name) { cohort_import.errors[:row_21][0][0] }
+
+        it { should include "includes invalid character(s)" }
+      end
+
+      describe "error for row with last name having invalid characters" do
+        subject(:child_last_name) { cohort_import.errors[:row_22][0][0] }
+
+        it { should include "includes invalid character(s)" }
+      end
+    end
   end
 end