CLDC-531: Part 2 - Validate API created case logs (#45)

* Validate API created case logs

* Validate record completion

* Set complete or in progress status based on field presence validation

* Make autogenerated fields a named constant

* Test the whole validation

* Add some hated code comments to explain some things

* Be stricter about the params we allow mass updates on

app/controllers/case_logs_controller.rb

@@ -1,6 +1,6 @@
 class CaseLogsController < ApplicationController
-  skip_before_action :verify_authenticity_token, only: [:create], if: :json_request?
+  skip_before_action :verify_authenticity_token, if: :json_create_request?
-  before_action :authenticate, only: [:create], if: :json_request?
+  before_action :authenticate, if: :json_create_request?
 
   def index
     @submitted_case_logs = CaseLog.where(status: 1)
@@ -8,10 +8,16 @@ class CaseLogsController < ApplicationController
   end
 
   def create
-    @case_log = CaseLog.create!(create_params)
+    case_log = CaseLog.create(create_params)
     respond_to do |format|
-      format.html { redirect_to @case_log }
+      format.html { redirect_to case_log }
-      format.json { render json: @case_log }
+      format.json do
+        if case_log.persisted?
+          render json: case_log, status: :created
+        else
+          render json: { errors: case_log.errors.full_messages }, status: :unprocessable_entity
+        end
+      end
     end
   end
 
@@ -73,8 +79,8 @@ private
     end
   end
 
-  def json_request?
+  def json_create_request?
-    request.format.json?
+    (request["action"] == "create") && request.format.json?
   end
 
   def authenticate
@@ -84,6 +90,6 @@ private
   def create_params
     return {} unless params[:case_log]
 
-    params.require(:case_log).permit(CaseLog.new.attributes.keys)
+    params.require(:case_log).permit(CaseLog.editable_fields)
   end
 end

app/models/case_log.rb

@@ -3,35 +3,52 @@ class CaseLogValidator < ActiveModel::Validator
   # this is how the metaprogramming of the method name being
   # call in the validate method works.
 
-  def validate_tenant_code(record)
-    if record.tenant_code.blank?
-      record.errors.add :tenant_code, "Tenant code can't be blank"
-    end
-  end
-
   def validate_tenant_age(record)
-    if record.tenant_age.blank?
+    if record.tenant_age && !/^[1-9][0-9]?$|^120$/.match?(record.tenant_age.to_s)
-      record.errors.add :tenant_age, "Tenant age can't be blank"
+      record.errors.add :tenant_age, "must be between 0 and 120"
-    elsif !/^[1-9][0-9]?$|^100$/.match?(record.tenant_age.to_s)
-      record.errors.add :tenant_age, "Tenant age must be between 0 and 100"
     end
   end
 
   def validate(record)
+    # If we've come from the form UI we only want to validate the specific fields
+    # that have just been submitted. If we're submitting a log via API or Bulk Upload
+    # we want to validate all data fields.
     question_to_validate = options[:previous_page]
-    if respond_to?("validate_#{question_to_validate}")
+    if question_to_validate && respond_to?("validate_#{question_to_validate}")
       public_send("validate_#{question_to_validate}", record)
+    else
+      # This assumes that all methods in this class other than this one are
+      # validations to be run
+      validation_methods = public_methods(false) - [__callee__]
+      validation_methods.each { |meth| public_send(meth, record) }
     end
   end
 end
 
 class CaseLog < ApplicationRecord
   validate :instance_validations
+  before_save :update_status!
+
   attr_writer :previous_page
 
   enum status: { "in progress" => 0, "submitted" => 1 }
 
+  AUTOGENERATED_FIELDS = %w[status created_at updated_at id].freeze
+
   def instance_validations
     validates_with CaseLogValidator, ({ previous_page: @previous_page } || {})
   end
+
+  def update_status!
+    self.status = (all_fields_completed? && errors.empty? ? "submitted" : "in progress")
+  end
+
+  def all_fields_completed?
+    mandatory_fields = attributes.except(*AUTOGENERATED_FIELDS)
+    mandatory_fields.none? { |_key, val| val.nil? }
+  end
+
+  def self.editable_fields
+    attribute_names - AUTOGENERATED_FIELDS
+  end
 end

db/migrate/20211013113607_remove_redundant_field.rb

@@ -0,0 +1,9 @@
+class RemoveRedundantField < ActiveRecord::Migration[6.1]
+  def up
+    remove_column :case_logs, :prior_homelessness
+  end
+
+  def down
+    add_column :case_logs, :prior_homelessness, :string
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_10_11_115946) do
+ActiveRecord::Schema.define(version: 2021_10_13_113607) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -25,7 +25,6 @@ ActiveRecord::Schema.define(version: 2021_10_11_115946) do
     t.string "tenant_ethnic_group"
     t.string "tenant_nationality"
     t.string "previous_housing_situation"
-    t.integer "prior_homelessness"
     t.string "armed_forces"
     t.string "tenant_economic_status"
     t.integer "household_number_of_other_members"

spec/features/case_log_spec.rb

@@ -290,9 +290,9 @@ RSpec.describe "Test Features" do
         expect(page).to have_selector("#case-log-tenant-age-field-error")
       end
 
-      it " of greater than 100 it shows validation" do
+      it " of greater than 120 it shows validation" do
         visit("/case_logs/#{id}/tenant_age")
-        fill_in_number_question(empty_case_log.id, "tenant_age", 120)
+        fill_in_number_question(empty_case_log.id, "tenant_age", 121)
         expect(page).to have_selector("#error-summary-title")
         expect(page).to have_selector("#case-log-tenant-age-error")
         expect(page).to have_selector("#case-log-tenant-age-field-error")

spec/fixtures/complete_case_log.json

@@ -0,0 +1,117 @@
+{
+  "case_log":
+    {
+      "tenant_code": "T657",
+      "tenant_age": 35,
+      "tenant_gender": "Female",
+      "tenant_ethnic_group": "White: English/Scottish/Welsh/Northern Irish/British",
+      "tenant_nationality": "UK national resident in UK",
+      "previous_housing_situation": "Private sector tenancy",
+      "armed_forces": "Yes - a regular",
+      "tenant_economic_status": "Full-time - 30 hours or more",
+      "household_number_of_other_members": 7,
+      "person_2_relationship": "Partner",
+      "person_2_age": 32,
+      "person_2_gender": "Male",
+      "person_2_economic_status": "Not seeking work",
+      "person_3_relationship": "Child - includes young adult and grown-up",
+      "person_3_age": 12,
+      "person_3_gender": "Male",
+      "person_3_economic_status": "Child under 16",
+      "person_4_relationship": "Child - includes young adult and grown-up",
+      "person_4_age": 12,
+      "person_4_gender": "Female",
+      "person_4_economic_status": "Child under 16",
+      "person_5_relationship": "Child - includes young adult and grown-up",
+      "person_5_age": 10,
+      "person_5_gender": "Non-binary",
+      "person_5_economic_status": "Child under 16",
+      "person_6_relationship": "Child - includes young adult and grown-up",
+      "person_6_age": 5,
+      "person_6_gender": "Prefer not to say",
+      "person_6_economic_status": "Child under 16",
+      "person_7_relationship": "Child - includes young adult and grown-up",
+      "person_7_age": 5,
+      "person_7_gender": "Prefer not to say",
+      "person_7_economic_status": "Child under 16",
+      "person_8_relationship": "Child - includes young adult and grown-up",
+      "person_8_age": 2,
+      "person_8_gender": "Prefer not to say",
+      "person_8_economic_status": "Child under 16",
+      "homelessness": "No",
+      "reason_for_leaving_last_settled_home": "Other problems with neighbours",
+      "benefit_cap_spare_room_subsidy": "No",
+      "armed_forces_active": "No",
+      "armed_forces_injured": "No",
+      "armed_forces_partner": "No",
+      "medical_conditions": "Yes",
+      "pregnancy": "No",
+      "accessibility_requirements": "No",
+      "condition_effects": "dummy",
+      "tenancy_code": "BZ757",
+      "tenancy_start_date": "12/03/2019",
+      "starter_tenancy": "No",
+      "fixed_term_tenancy": "No",
+      "tenancy_type": "Fixed term – Secure",
+      "letting_type": "Affordable Rent - General Needs",
+      "letting_provider": "This landlord",
+      "property_location": "Barnet",
+      "previous_postcode": "NW1 5TY",
+      "property_relet": "No",
+      "property_vacancy_reason": "Relet - tenant abandoned property",
+      "property_reference": "P9876",
+      "property_unit_type": "House",
+      "property_building_type": "dummy",
+      "property_number_of_bedrooms": 3,
+      "property_void_date": "03/11/2019",
+      "property_major_repairs": "Yes",
+      "property_major_repairs_date": "05/05/2020",
+      "property_number_of_times_relet": 2,
+      "property_wheelchair_accessible": true,
+      "net_income": 1000,
+      "net_income_frequency": "Monthly",
+      "net_income_uc_proportion": "Some",
+      "housing_benefit": "Universal Credit with housing element, but not Housing Benefit",
+      "rent_frequency": "Weekly",
+      "basic_rent": 200,
+      "service_charge": 50,
+      "personal_service_charge": 40,
+      "support_charge": 35,
+      "total_charge": 325,
+      "outstanding_amount": "Yes",
+      "time_lived_in_la": "1 to 2 years",
+      "time_on_la_waiting_list": "Less than 1 year",
+      "previous_la": "Ashford",
+      "property_postcode": "SE2 6RT",
+      "reasonable_preference": "Yes",
+      "reasonable_preference_reason": "dummy",
+      "cbl_letting": true,
+      "chr_letting": false,
+      "cap_letting": false,
+      "outstanding_rent_or_charges": 25,
+      "other_reason_for_leaving_last_settled_home": "Other reason",
+      "accessibility_requirements_fully_wheelchair_accessible_housing": true,
+      "accessibility_requirements_wheelchair_access_to_essential_rooms": false,
+      "accessibility_requirements_level_access_housing": false,
+      "accessibility_requirements_other_disability_requirements": false,
+      "accessibility_requirements_no_disability_requirements": false,
+      "accessibility_requirements_do_not_know": false,
+      "accessibility_requirements_prefer_not_to_say": false,
+      "condition_effects_vision": false,
+      "condition_effects_hearing": true,
+      "condition_effects_mobility": false,
+      "condition_effects_dexterity": false,
+      "condition_effects_stamina": false,
+      "condition_effects_learning": false,
+      "condition_effects_memory": false,
+      "condition_effects_mental_health": false,
+      "condition_effects_social_or_behavioral": false,
+      "condition_effects_other": false,
+      "condition_effects_prefer_not_to_say": false,
+      "reasonable_preference_reason_homeless": false,
+      "reasonable_preference_reason_unsatisfactory_housing": false,
+      "reasonable_preference_reason_medical_grounds": false,
+      "reasonable_preference_reason_avoid_hardship": false,
+      "reasonable_preference_reason_do_not_know": true
+    }
+  }

spec/models/case_log_spec.rb

@@ -1,4 +1,17 @@
 require "rails_helper"
 
 RSpec.describe Form, type: :model do
+  describe "#new" do
+    it "validates age is a number" do
+      expect { CaseLog.create!(tenant_age: "random") }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it "validates age is under 120" do
+      expect { CaseLog.create!(tenant_age: 121) }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it "validates age is over 0" do
+      expect { CaseLog.create!(tenant_age: 0) }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+  end
 end

spec/requests/case_log_controller_spec.rb

@@ -11,6 +11,8 @@ RSpec.describe CaseLogsController, type: :request do
       ActionController::HttpAuthentication::Basic
                           .encode_credentials(api_username, api_password)
     end
+    let(:in_progress) { "in progress" }
+    let(:submitted) { "submitted" }
 
     let(:headers) do
       {
@@ -51,6 +53,34 @@ RSpec.describe CaseLogsController, type: :request do
       expect(json_response["property_postcode"]).to eq(property_postcode)
     end
 
+    context "invalid json params" do
+      let(:tenant_age) { 2000 }
+
+      it "validates case log parameters" do
+        json_response = JSON.parse(response.body)
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(json_response["errors"]).to eq(["Tenant age must be between 0 and 120"])
+      end
+    end
+
+    context "partial case log submission" do
+      it "marks the record as in_progress" do
+        json_response = JSON.parse(response.body)
+        expect(json_response["status"]).to eq(in_progress)
+      end
+    end
+
+    context "complete case log submission" do
+      let(:params) do
+        JSON.parse(File.open("spec/fixtures/complete_case_log.json").read)
+      end
+
+      it "marks the record as submitted" do
+        json_response = JSON.parse(response.body)
+        expect(json_response["status"]).to eq(submitted)
+      end
+    end
+
     context "request with invalid credentials" do
       let(:basic_credentials) do
         ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops")