Merge pull request #2381 from ministryofjustice/check-your-claim-page…

…-further-fixes

Patch autosave associations to support conditional validations

app/controllers/external_users/claims_controller.rb

@@ -17,7 +17,7 @@ class ResourceClassNotDefined < StandardError; end
 
   before_action :set_and_authorize_claim, only: %i[show edit update unarchive clone_rejected destroy summary
                                                    confirmation show_message_controls messages disc_evidence]
-  before_action :set_form_step, only: %i[edit]
+  before_action :set_form_step, only: %i[edit update]
   before_action :redirect_unless_editable, only: %i[edit update]
   before_action :generate_form_id, only: %i[new edit]
   before_action :initialize_submodel_counts
@@ -134,6 +134,7 @@ def unarchive
 
   def new
     @claim = resource_klass.new
+    @claim.form_step = params[:step] || @claim.submission_stages.first
     authorize! :new, @claim
     load_offences_and_case_types
     build_nested_resources
@@ -154,6 +155,7 @@ def edit
 
   def create
     @claim = resource_klass.new(params_with_external_user_and_creator)
+    @claim.form_step ||= @claim.submission_stages.first
     authorize! :create, @claim
     result = if submitting_to_laa?
                Claims::CreateClaim.call(@claim)
@@ -266,8 +268,9 @@ def set_and_authorize_claim
 
   def set_form_step
     return unless @claim
-    return unless params[:step].present?
-    @claim.form_step = params[:step]
+    @claim.form_step = params[:step] ||
+                       params.key?(:claim) && claim_params[:form_step] ||
+                       @claim.submission_stages.first
   end
 
   def claim_params

app/models/claim/advocate_claim.rb

@@ -66,8 +66,18 @@ module Claim
   class AdvocateClaim < BaseClaim
     route_key_name 'advocates_claim'
 
-    has_many :basic_fees, foreign_key: :claim_id, class_name: 'Fee::BasicFee', dependent: :destroy, inverse_of: :claim
-    has_many :fixed_fees, foreign_key: :claim_id, class_name: 'Fee::FixedFee', dependent: :destroy, inverse_of: :claim
+    has_many :basic_fees,
+             foreign_key: :claim_id,
+             class_name: 'Fee::BasicFee',
+             dependent: :destroy,
+             inverse_of: :claim,
+             validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :basic_fees }
+    has_many :fixed_fees,
+             foreign_key: :claim_id,
+             class_name: 'Fee::FixedFee',
+             dependent: :destroy,
+             inverse_of: :claim,
+             validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :fixed_fees }
 
     accepts_nested_attributes_for :basic_fees, reject_if: all_blank_or_zero, allow_destroy: true
     accepts_nested_attributes_for :fixed_fees, reject_if: all_blank_or_zero, allow_destroy: true

app/models/claim/advocate_interim_claim.rb

@@ -6,7 +6,12 @@ class AdvocateInterimClaim < BaseClaim
                    unless: proc { |c| c.disable_for_state_transition.eql?(:all) }
     validates_with ::Claim::AdvocateInterimClaimSubModelValidator
 
-    has_one :warrant_fee, foreign_key: :claim_id, class_name: 'Fee::WarrantFee', dependent: :destroy, inverse_of: :claim
+    has_one :warrant_fee,
+            foreign_key: :claim_id,
+            class_name: 'Fee::WarrantFee',
+            dependent: :destroy,
+            inverse_of: :claim,
+            validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :interim_fees }
 
     accepts_nested_attributes_for :warrant_fee, allow_destroy: false
 

app/models/claim/base_claim.rb

@@ -683,7 +683,6 @@ def build_associations
 
     def default_values
       self.source ||= 'web'
-      self.form_step ||= submission_stages.first
     end
 
     def default_case_transferred_from_another_court

app/models/claim/interim_claim.rb

@@ -70,7 +70,12 @@ class InterimClaim < BaseClaim
     validates_with ::Claim::LitigatorSupplierNumberValidator
     validates_with ::Claim::InterimClaimSubModelValidator
 
-    has_one :interim_fee, foreign_key: :claim_id, class_name: 'Fee::InterimFee', dependent: :destroy, inverse_of: :claim
+    has_one :interim_fee,
+            foreign_key: :claim_id,
+            class_name: 'Fee::InterimFee',
+            dependent: :destroy,
+            inverse_of: :claim,
+            validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :interim_fees }
 
     accepts_nested_attributes_for :interim_fee, reject_if: :all_blank, allow_destroy: false
 

app/models/claim/litigator_claim.rb

@@ -70,13 +70,19 @@ class LitigatorClaim < BaseClaim
     validates_with ::Claim::LitigatorSupplierNumberValidator, on: :create
     validates_with ::Claim::LitigatorClaimSubModelValidator
 
-    has_one :fixed_fee, foreign_key: :claim_id, class_name: 'Fee::FixedFee', dependent: :destroy, inverse_of: :claim
+    has_one :fixed_fee,
+            foreign_key: :claim_id,
+            class_name: 'Fee::FixedFee',
+            dependent: :destroy,
+            inverse_of: :claim,
+            validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :fixed_fees }
     has_one :warrant_fee, foreign_key: :claim_id, class_name: 'Fee::WarrantFee', dependent: :destroy, inverse_of: :claim
     has_one :graduated_fee,
             foreign_key: :claim_id,
             class_name: 'Fee::GraduatedFee',
             dependent: :destroy,
-            inverse_of: :claim
+            inverse_of: :claim,
+            validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :graduated_fees }
 
     accepts_nested_attributes_for :fixed_fee, reject_if: :all_blank, allow_destroy: false
     accepts_nested_attributes_for :warrant_fee, reject_if: :all_blank, allow_destroy: false

app/models/claim/transfer_claim.rb

@@ -66,8 +66,20 @@ module Claim
   class TransferClaim < BaseClaim
     route_key_name 'litigators_transfer_claim'
 
-    has_one :transfer_detail, foreign_key: :claim_id, class_name: Claim::TransferDetail, dependent: :destroy
-    has_one :transfer_fee, foreign_key: :claim_id, class_name: Fee::TransferFee, dependent: :destroy, inverse_of: :claim
+    has_one :transfer_detail,
+            foreign_key: :claim_id,
+            class_name: Claim::TransferDetail,
+            dependent: :destroy,
+            inverse_of: :claim,
+            validate: proc { |claim|
+              claim.from_api? || claim.form_step.nil? || claim.form_step == :transfer_fee_details
+            }
+    has_one :transfer_fee,
+            foreign_key: :claim_id,
+            class_name: Fee::TransferFee,
+            dependent: :destroy,
+            inverse_of: :claim,
+            validate: proc { |claim| claim.from_api? || claim.form_step.nil? || claim.form_step == :transfer_fees }
 
     accepts_nested_attributes_for :transfer_detail, reject_if: :all_blank, allow_destroy: false
     accepts_nested_attributes_for :transfer_fee, reject_if: :all_blank, allow_destroy: false

app/presenters/claim/base_claim_presenter.rb

@@ -20,10 +20,6 @@ def show_sidebar?
         transfer_fee_details].include? claim.current_step
   end
 
-  def can_be_saved_as_draft?
-    claim.draft? && !%i[case_details transfer_fee_details].include?(claim.current_step)
-  end
-
   # NOTE: this is an interim solution for what probably should be
   # some sort of DSL to describe what fields are required for a given section
   # for that section to be considered completed

app/validators/claim/base_claim_sub_model_validator.rb

@@ -19,10 +19,14 @@ def validate(record)
 
   private
 
+  def validate_all_steps?(record)
+    record.from_api? || record.form_step.nil?
+  end
+
   def associations_for_has_many_validations(record)
     # NOTE: keeping existent validation for API purposes
     # The form validations just validate the fields for the current step
-    return (has_many_association_names_for_steps[record.form_step] || []) unless record.from_api?
+    return (has_many_association_names_for_steps[record.form_step] || []) unless validate_all_steps?(record)
     has_many_association_names_for_steps.select do |k, _v|
       record.submission_current_flow.map(&:to_sym).include?(k)
     end.values.flatten
@@ -42,7 +46,7 @@ def validate_presence_of_association(association_name, options = {})
   def associations_for_has_one_validations(record)
     # NOTE: keeping existent validation for API purposes
     # The form validations just validate the fields for the current step
-    return (has_one_association_names_for_steps[record.form_step] || []) unless record.from_api?
+    return (has_one_association_names_for_steps[record.form_step] || []) unless validate_all_steps?(record)
     has_one_association_names_for_steps.select do |k, _v|
       record.submission_current_flow.map(&:to_sym).include?(k)
     end.values.flatten

app/validators/claim/base_claim_validator.rb

@@ -8,7 +8,8 @@ def self.mandatory_fields
   def step_fields_for_validation
     # NOTE: keeping existent validation for API purposes
     # The form validations just validate the fields for the current step
-    return (self.class.fields_for_steps[@record.form_step] || []) unless @record.from_api?
+    return (self.class.fields_for_steps[@record.form_step] || []) unless @record.from_api? || @record.form_step.nil?
+    return self.class.fields_for_steps.values.flatten if !@record.from_api? && @record.form_step.nil?
     self.class.fields_for_steps.select do |k, _v|
       @record.submission_current_flow.map(&:to_sym).include?(k)
     end.values.flatten

app/views/external_users/claims/buttons/_continue.html.haml

@@ -3,5 +3,4 @@
     - commit = claim.next_step? ? 'commit_continue' : 'commit_submit_claim'
     = f.submit t('buttons.continue'), name: commit, class: 'button left btn-spacer-20'
 
-    - if claim.can_be_saved_as_draft?
-      = f.submit t('buttons.save_a_draft'), name: 'commit_save_draft', class: 'button-gray-3 left'
+    = f.submit t('buttons.save_a_draft'), name: 'commit_save_draft', class: 'button-gray-3 left'

config/initializers/01_autosave_association_patch.rb

@@ -0,0 +1,38 @@
+# NOTE: This is a MAJOR shoehorn patch to get around the convoluted way the validations in this application work.
+# Its main intent is to solve the problem of having validations for a claim per step/stage and because of the fact
+# that Rails autosaves and validates associations that have been defined as with 'accept_nested_attributes_for'.
+# What that causes is that for a given step/stage that does not require a given association to be validated, as long as
+# the association itself exists, Rails will automatically save and validate it which is not the expected behaviour here.
+#
+# The initial approach was to define the association with the option validate: Proc.new { |claim| claim.condition_to_validate? }
+# Unfortunately, the association definition support Procs or lambdas but does not evaluate them, just checks if the value is not false.
+#
+# The next approach was to define the association with the option validate: false and have an explicity validation for the association
+# conditional to the step/stage it belongs:
+#
+# validates_associated :association_name, if: Proc.new { |claim| claim.condition_to_validate? }
+#
+# This approach works as intended with a caveat: the errors produced are not attached to the parent object anymore (as they were with the
+# default autosave/validaton set in the association definition)
+#
+# So, that gets us here, patching the autosave association to check if the validate option is a proc, and if so evaluated it to determine
+# if the associated records needs to be validated or not.
+#
+# WARNING: This patch will likely cause any further Rails updates/upgrades to break, so do keep that in mind!!!
+#
+# This should probably be re-thinked going forward, so a cleaner/less disruptive solution is choosen instead.
+# This is not a recommended approach to take!!!, but rather a last resort approach to get around a very unsual behaviour (saving invalid records into the database).
+
+module ActiveRecord
+  module AutosaveAssociation
+    private
+      alias old_association_valid? association_valid?
+
+      def association_valid?(reflection, record, index=nil)
+        if reflection.options[:validate].is_a?(Proc)
+          return true unless reflection.options[:validate].call(self)
+        end
+        old_association_valid?(reflection, record, index)
+      end
+  end
+end

features/step_definitions/authorise_claim_steps.rb

@@ -1,5 +1,12 @@
 Given(/^there is a claim allocated to the case worker with case number '(.*?)'$/) do |case_number|
-  @claim = create(:allocated_claim, external_user: @advocate, case_number: case_number)
+  # TODO: this does not create a valid submitted claim
+  # creates some sort of claim with some information that happens
+  # to be in the submitted state
+  # To go around this I'm preserving old functionality for the factory by
+  # setting the form_step.
+  # Going forward this should be properly fixed with an actual factory that is valid
+  # for a submitted claim
+  @claim = create(:allocated_claim, external_user: @advocate, case_number: case_number, form_step: :case_details)
   @case_worker.claims << @claim
 end
 

features/step_definitions/claim_allocation_steps.rb

@@ -66,7 +66,14 @@
   @claims = []
 
   case_numbers.each do |case_number|
-    @claims << create(:submitted_claim, case_number: case_number)
+    # TODO: this does not create a valid submitted claim
+    # creates some sort of claim with some information that happens
+    # to be in the submitted state
+    # To go around this I'm preserving old functionality for the factory by
+    # setting the form_step.
+    # Going forward this should be properly fixed with an actual factory that is valid
+    # for a submitted claim
+    @claims << create(:submitted_claim, case_number: case_number, form_step: :case_details)
   end
 end