Users can add people with relevant convictions (#104)

* Users can add people with relevant convictions

https://eaflood.atlassian.net/browse/WC-108

If a user answers yes to "Has anyone involved in managing your organisation been convicted of an environmental offence in the past 12 months?" they should be prompted to provide the details of the person with the offence (but not the details of the offence).

This should be a blank form as any data from the previous registration is out of date.

* Distinguish between 'key' and 'relevant' KeyPeople

The KeyPeople class actually covers two types of people:

- people with the 'key' person_type, added on the key people form
- people with the 'relevant' person_type, added on the relevant conviction details form

For what I assume are historical reasons in the existing database, all of a registration's people are stored together as KeyPeople embedded objects, with the only distinguishing factor being the 'person_type' attribute. However, as far as the frontend is concerned, these are two separate lists. We need to make sure we don't modify key people when changing relevant people, and the reverse. For example, checking the number of key people which a registration has (eg, a sole trader can only have one) should not also count 'relevant' people.

This change adds some methods to registration-y objects to get lists of 'key people' or 'relevant people' only. It also makes some modifications to the existing key people form to make sure we leave relevant people alone when adding, listing or deleting key people.

And, of course, we really needed some tests for all this.

* Set up form for conviction details

The forms for key people and conviction details are very similar - the only difference is the additional "position" field for the conviction details form. So to start out with, we're mostly duplicating key people and making sure the tests pass.

Once we have the form and test suite in place, we can start reducing duplication between the two (and improving some of the naming).

* Remove duplication from KeyPeople and ConvictionDetails controllers

These both now inherit from a PersonFormController class, which inherits from the FormController.

This allows them to share behaviour to add another person or delete a person.

* Remove duplication from person-y form models

There was also a lot of duplication between KeyPeopleForm and ConvictionDetailsForm. This change adds a PersonForm which both classes inherit from.

Where the PersonForm calls methods which are only defined in the subclasses, we raise a NotImplementedError. This is so that in the event we ever implement a third subclass, these errors will notify us to add those methods to the new subclass. They should never be triggered now as we don't create any instances of PersonForm itself.

* Remove duplication from people form views

Create some shared partials for the name and date fields.

Also fix a validation issue for the 'position' field.

* Change minimum age from 17 to 16

Updated the minimum age for a person with relevant convictions after consulting with the product owner.

app/controllers/conviction_details_forms_controller.rb

@@ -1,9 +1,13 @@
-class ConvictionDetailsFormsController < FormsController
+class ConvictionDetailsFormsController < PersonFormsController
   def new
     super(ConvictionDetailsForm, "conviction_details_form")
   end
 
   def create
     super(ConvictionDetailsForm, "conviction_details_form")
   end
+
+  def delete_person
+    super(ConvictionDetailsForm, "conviction_details_form")
+  end
 end

app/controllers/key_people_forms_controller.rb

@@ -1,37 +1,13 @@
-class KeyPeopleFormsController < FormsController
+class KeyPeopleFormsController < PersonFormsController
   def new
     super(KeyPeopleForm, "key_people_form")
   end
 
   def create
-    if params[:commit] == I18n.t("key_people_forms.new.add_person_link")
-      submit_and_add_another
-    else
-      super(KeyPeopleForm, "key_people_form")
-    end
-  end
-
-  def submit_and_add_another
-    return unless set_up_form(KeyPeopleForm, "key_people_form", params["key_people_form"][:reg_identifier])
-
-    respond_to do |format|
-      if @key_people_form.submit(params["key_people_form"])
-        format.html { redirect_to_correct_form }
-      else
-        format.html { render :new }
-      end
-    end
+    super(KeyPeopleForm, "key_people_form")
   end
 
   def delete_person
-    return unless set_up_form(KeyPeopleForm, "key_people_form", params[:reg_identifier])
-
-    respond_to do |format|
-      # Check if there are any matches first, to avoid a Mongoid error
-      people_with_id = @transient_registration.keyPeople.where(id: params[:id])
-      people_with_id.first.delete if people_with_id.any?
-
-      format.html { redirect_to_correct_form }
-    end
+    super(KeyPeopleForm, "key_people_form")
   end
 end

app/controllers/person_forms_controller.rb

@@ -0,0 +1,35 @@
+class PersonFormsController < FormsController
+  def create(form_class, form)
+    if params[:commit] == I18n.t("#{form}s.new.add_person_link")
+      submit_and_add_another(form_class, form)
+    else
+      super(form_class, form)
+    end
+  end
+
+  def submit_and_add_another(form_class, form)
+    return unless set_up_form(form_class, form, params[form][:reg_identifier])
+
+    form_instance_variable = instance_variable_get("@#{form}")
+
+    respond_to do |format|
+      if form_instance_variable.submit(params[form])
+        format.html { redirect_to_correct_form }
+      else
+        format.html { render :new }
+      end
+    end
+  end
+
+  def delete_person(form_class, form)
+    return unless set_up_form(form_class, form, params[:reg_identifier])
+
+    respond_to do |format|
+      # Check if there are any matches first, to avoid a Mongoid error
+      people_with_id = @transient_registration.keyPeople.where(id: params[:id])
+      people_with_id.first.delete if people_with_id.any?
+
+      format.html { redirect_to_correct_form }
+    end
+  end
+end

app/forms/conviction_details_form.rb

@@ -1,17 +1,38 @@
-class ConvictionDetailsForm < BaseForm
-  # TODO: Define accessible attributes, eg attr_accessor :field
+class ConvictionDetailsForm < PersonForm
+  attr_accessor :position
 
-  def initialize(transient_registration)
-    super
-    # TODO: Define params to get from transient_registration, eg self.field = @transient_registration.field
+  def position?
+    true
   end
 
-  def submit(params)
-    # Assign the params for validation and pass them to the BaseForm method for updating
-    # TODO: Define allowed params, eg self.field = params[:field]
-    # TODO: Include attributes to update in the attributes hash, eg { field: field }
-    attributes = {}
+  def maximum_people_in_type
+    nil
+  end
+
+  def minimum_people_in_type
+    1
+  end
+
+  private
+
+  def person_type
+    "relevant"
+  end
+
+  # Adding the new person directly to @transient_registration.keyPeople immediately updates the object,
+  # regardless of validation. So instead we copy all existing people into a new array and modify that.
+  def list_of_people_to_keep
+    people = []
+
+    @transient_registration.keyPeople.each do |person|
+      # We need to copy the person before adding to the array to avoid a 'conflicting modifications' Mongo error (10151)
+      people << person.clone
+    end
+
+    people
+  end
 
-    super(attributes, params[:reg_identifier])
+  def age_cutoff_date
+    (Date.today - 16.years) + 1.day
   end
 end

app/forms/key_people_form.rb

@@ -1,114 +1,59 @@
-class KeyPeopleForm < BaseForm
+class KeyPeopleForm < PersonForm
   attr_accessor :business_type
-  attr_accessor :first_name, :last_name, :dob_day, :dob_month, :dob_year, :key_person, :date_of_birth
 
   def initialize(transient_registration)
     super
     # We only use this for the correct microcopy
     self.business_type = @transient_registration.business_type
 
     # If there's only one key person, we can pre-fill the fields so users can easily edit them
-    prefill_form if can_only_have_one_key_person? && @transient_registration.keyPeople.present?
+    prefill_form if can_only_have_one_person_in_type? && @transient_registration.key_people.present?
   end
 
-  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]
-    process_date_fields(params)
-
-    self.key_person = add_key_person
-    self.date_of_birth = key_person.date_of_birth
-
-    attributes = if fields_have_content?
-                   { keyPeople: all_key_people }
-                 else
-                   {}
-                 end
-
-    super(attributes, params[:reg_identifier])
-  end
-
-  validates_with KeyPeopleValidator
-  validate :old_enough?
-
-  def maximum_key_people
+  def maximum_people_in_type
     return unless business_type.present?
     key_people_limits[business_type.to_sym][:maximum]
   end
 
-  def minimum_key_people
+  def minimum_people_in_type
     # Business type should always be set, but use 1 as the default, just in case
     return 1 unless business_type.present?
     key_people_limits[business_type.to_sym][:minimum]
   end
 
-  def number_of_existing_key_people
-    @transient_registration.keyPeople.count
-  end
-
-  def can_only_have_one_key_person?
-    return false unless maximum_key_people.present?
-    maximum_key_people == 1
-  end
-
-  def enough_key_people?
-    return false if number_of_existing_key_people < minimum_key_people
-    true
-  end
-
-  def fields_have_content?
-    fields = [first_name, last_name, dob_day, dob_month, dob_year]
-    fields.each do |field|
-      return true if field.present? && field.to_s.length.positive?
-    end
-    false
-  end
-
   private
 
-  def prefill_form
-    self.first_name = @transient_registration.keyPeople.first.first_name
-    self.last_name = @transient_registration.keyPeople.first.last_name
-    self.dob_day = @transient_registration.keyPeople.first.dob_day
-    self.dob_month = @transient_registration.keyPeople.first.dob_month
-    self.dob_year = @transient_registration.keyPeople.first.dob_year
-  end
-
-  def process_date_fields(params)
-    self.dob_day = format_date_field_value(params[:dob_day])
-    self.dob_month = format_date_field_value(params[:dob_month])
-    self.dob_year = format_date_field_value(params[:dob_year])
-  end
-
-  # If we can make the date fields positive integers, use those integers
-  # Otherwise, return nil
-  def format_date_field_value(value)
-    # If this isn't a valid integer, .to_i returns 0
-    integer_value = value.to_i
-    return integer_value if integer_value.positive?
+  def person_type
+    "key"
   end
 
-  def add_key_person
-    KeyPerson.new(first_name: first_name,
-                  last_name: last_name,
-                  dob_day: dob_day,
-                  dob_month: dob_month,
-                  dob_year: dob_year,
-                  person_type: "key")
+  def prefill_form
+    self.first_name = @transient_registration.key_people.first.first_name
+    self.last_name = @transient_registration.key_people.first.last_name
+    self.dob_day = @transient_registration.key_people.first.dob_day
+    self.dob_month = @transient_registration.key_people.first.dob_month
+    self.dob_year = @transient_registration.key_people.first.dob_year
   end
 
-  def all_key_people
-    # If there's only one key person allowed, just replace existing data
-    return [key_person] if can_only_have_one_key_person?
-
-    existing_key_people = []
-    # Adding the new key person directly to @transient_registration.keyPeople immediately updates the object,
-    # regardless of validation. So instead we copy the existing key people into a new array and modify that.
-    @transient_registration.keyPeople.each do |person|
-      existing_key_people << person
+  # Adding the new key person directly to @transient_registration.keyPeople immediately updates the object,
+  # regardless of validation. So instead we copy all existing people into a new array and modify that.
+  def list_of_people_to_keep
+    people = []
+
+    # If there's only one key person allowed, we want to discard any existing key people, but keep people with
+    # relevant convictions. Otherwise, we copy all the keyPeople, regardless of type.
+    existing_people = if can_only_have_one_person_in_type?
+                        @transient_registration.relevant_people
+                      else
+                        @transient_registration.keyPeople
+                      end
+
+    existing_people.each do |person|
+      # We need to copy the person before adding to the array to avoid a 'conflicting modifications' Mongo error (10151)
+      people << person.clone
     end
-    existing_key_people << key_person
+
+    people
   end
 
   def key_people_limits
@@ -122,9 +67,7 @@ def key_people_limits
     }
   end
 
-  def old_enough?
-    return false unless date_of_birth.present?
-
+  def age_cutoff_date
     age_limits = {
       limitedCompany: 16.years,
       limitedLiabilityPartnership: 17.years,
@@ -133,12 +76,11 @@ def old_enough?
       partnership: 17.years,
       soleTrader: 17.years
     }
-    age_cutoff_date = (Date.today - age_limits[business_type.to_sym]) + 1.day
 
-    return true if date_of_birth < age_cutoff_date
+    (Date.today - age_limits[business_type.to_sym]) + 1.day
+  end
 
-    error_message = "age_limit_#{business_type}".to_sym
-    errors.add(:date_of_birth, error_message)
-    false
+  def age_limit_error_message
+    "age_limit_#{business_type}".to_sym
   end
 end