Tweak key people validation handling and heading (#95)

These changes are following the QA review.

The bulk of the changes are about how we handle validation messages. This commit reduces the number of validation errors which a user can get at one time:

- Previously a totally blank form would trigger about 6 different messages - now it should just be one
- Date validation errors have also been simplified so if all fields are missing, we only display a single message
- We also only check that the complete date is a valid date once we've validated the individual fields to reduce the number of errors users get
- Date fields which don't contain valid integers are no longer converted to 0, which gives us better messages too

Also changed the heading text for overseas users to match the wireframe.

app/forms/key_people_form.rb

@@ -12,9 +12,7 @@ def submit(params)
     # Assign the params for validation and pass them to the BaseForm method for updating
     self.first_name = params[:first_name]
     self.last_name = params[:last_name]
-    self.dob_day = params[:dob_day].to_i
-    self.dob_month = params[:dob_month].to_i
-    self.dob_year = params[:dob_year].to_i
+    process_date_fields(params)
 
     self.key_person = add_key_person
     self.date_of_birth = key_person.date_of_birth
@@ -24,13 +22,23 @@ def submit(params)
     super(attributes, params[:reg_identifier])
   end
 
-  validates :first_name, presence: true, length: { maximum: 35 }
-  validates :last_name, presence: true, length: { maximum: 35 }
+  validates_with KeyPeopleValidator
   validate :old_enough?
-  validates_with DateOfBirthValidator
 
   private
 
+  # If we can make the date fields positive integers, save those integers
+  # Otherwise, save as nil
+  def process_date_fields(params)
+    day = params[:dob_day].to_i
+    month = params[:dob_month].to_i
+    year = params[:dob_year].to_i
+
+    self.dob_day = day if day.positive?
+    self.dob_month = month if month.positive?
+    self.dob_year = year if year.positive?
+  end
+
   def add_key_person
     KeyPerson.new(first_name: first_name,
                   last_name: last_name,

app/models/key_person.rb

@@ -26,6 +26,8 @@ def set_date_of_birth
       errors.add(:date_of_birth, :invalid_date)
     rescue ArgumentError
       errors.add(:date_of_birth, :invalid_date)
+    rescue TypeError
+      errors.add(:date_of_birth, :invalid_date)
     end
   end
 end

app/validators/date_of_birth_validator.rb

@@ -1,19 +1,39 @@
 class DateOfBirthValidator < ActiveModel::Validator
   def validate(record)
-    fields = { day: record.dob_day, month: record.dob_month, year: record.dob_year }
+    # Make sure we have any date fields to validate before proceeding
+    return false if all_fields_empty?(record)
 
-    fields.each do |type, field|
-      validate_field(record, type, field)
-    end
+    # Next, check the date fields one at a time
+    return false unless individual_fields_valid?(record)
 
+    # If individual fields are OK, check the validity of the date as a whole
     dob_is_a_date?(record)
   end
 
   private
 
-  def validate_field(record, type, field)
-    return unless field_present?(record, type, field)
-    return unless field_is_integer?(record, type, field)
+  def all_fields_empty?(record)
+    fields = [record.dob_day, record.dob_month, record.dob_year].compact
+    return false if fields.any?
+    record.errors.add(:date_of_birth, :not_a_date)
+    true
+  end
+
+  def individual_fields_valid?(record)
+    all_fields_valid = true
+
+    fields = { day: record.dob_day, month: record.dob_month, year: record.dob_year }
+    fields.each do |type, field|
+      next if field_is_valid?(record, type, field)
+      all_fields_valid = false
+    end
+
+    all_fields_valid
+  end
+
+  def field_is_valid?(record, type, field)
+    return false unless field_present?(record, type, field)
+    return false unless field_is_integer?(record, type, field)
     field_is_in_correct_range?(record, type, field)
   end
 

app/validators/key_people_validator.rb

@@ -0,0 +1,41 @@
+class KeyPeopleValidator < ActiveModel::Validator
+  def validate(record)
+    return false unless fields_have_content?(record)
+    validate_first_name(record)
+    validate_last_name(record)
+    DateOfBirthValidator.new.validate(record)
+  end
+
+  private
+
+  def fields_have_content?(record)
+    fields = [record.first_name, record.last_name, record.dob_day, record.dob_month, record.dob_year]
+    fields.each do |field|
+      return true if field.present? && field.to_s.length.positive?
+    end
+    record.errors.add(:base, :not_enough_key_people)
+    false
+  end
+
+  def validate_first_name(record)
+    return unless field_is_present?(record, :first_name)
+    field_is_not_too_long?(record, :first_name, 35)
+  end
+
+  def validate_last_name(record)
+    return unless field_is_present?(record, :last_name)
+    field_is_not_too_long?(record, :last_name, 35)
+  end
+
+  def field_is_present?(record, field)
+    return true if record.send(field).present?
+    record.errors.add(field, :blank)
+    false
+  end
+
+  def field_is_not_too_long?(record, field, length)
+    return true if record.send(field).length < length
+    record.errors.add(field, :too_long)
+    false
+  end
+end

config/locales/forms/key_people_forms/en.yml

@@ -6,7 +6,7 @@ en:
       heading_limitedLiabilityPartnership: Partner details
       heading_overseas: Business owner details
       heading_partnership: Partner details
-      heading_soleTrader: Business owner details
+      heading_soleTrader: Business or organisation owner details
       description_1_localAuthority: Provide the name and date of birth for each chief executive of this local authority or public body.
       description_1_limitedCompany: Provide the name and date of birth for each director of this business.
       description_1_limitedLiabilityPartnership: Provide the name and date of birth for each partner of this business.
@@ -27,6 +27,7 @@ en:
     errors:
       models:
         key_people_form:
+          not_enough_key_people: "You must add the details of at least one person"
           attributes:
             first_name:
               blank: "You must enter a first name"

spec/forms/key_people_forms_spec.rb

@@ -226,6 +226,18 @@
         end
       end
 
+      context "when all the date of birth fields are empty" do
+        before(:each) do
+          key_people_form.dob_day = ""
+          key_people_form.dob_month = ""
+          key_people_form.dob_year = ""
+        end
+
+        it "is not valid" do
+          expect(key_people_form).to_not be_valid
+        end
+      end
+
       context "when a date of birth is not a valid date" do
         before(:each) do
           key_people_form.date_of_birth = nil