From 16e01b4b16a2cad2b1da1f8379f8db0d256abdba Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 22 Nov 2024 15:39:52 +0000 Subject: [PATCH] Move error handling Moves the error handling out of the `ClaimStudentLoanDetailsUpdater` and into the job that calls it. When performing an SLC upload, if the `ClaimStudentLoanDetailsUpdater` errors, and swallows the error, then we can get in a scenario where the claim doesn't have student loan details set on it but the claim does have a passing task. As the job uses the presence of the task to determine which claims need their details updating, subsequent SLC data uploads don't set the claim's details. This commit moves the error handling to wrap both the `ClaimStudentLoanDetailsUpdater` and the `AutomatedCheck`, so we still preserve the original intention of allowing the rest of the import to go ahead if one of the claims is throwing an error for some reason. --- app/jobs/student_loan_amount_check_job.rb | 3 + app/jobs/student_loan_plan_check_job.rb | 3 + .../claim_student_loan_details_updater.rb | 3 - .../student_loan_amount_check_job_spec.rb | 164 +++++++++++------- spec/jobs/student_loan_plan_check_job_spec.rb | 31 ++++ ...claim_student_loan_details_updater_spec.rb | 25 --- 6 files changed, 136 insertions(+), 93 deletions(-) diff --git a/app/jobs/student_loan_amount_check_job.rb b/app/jobs/student_loan_amount_check_job.rb index 1855262a05..6aabe721f1 100644 --- a/app/jobs/student_loan_amount_check_job.rb +++ b/app/jobs/student_loan_amount_check_job.rb @@ -6,6 +6,9 @@ def perform claims.each do |claim| ClaimStudentLoanDetailsUpdater.call(claim) AutomatedChecks::ClaimVerifiers::StudentLoanAmount.new(claim:).perform + rescue => e + # If something goes wrong, log the error and continue + Rollbar.error(e) end end diff --git a/app/jobs/student_loan_plan_check_job.rb b/app/jobs/student_loan_plan_check_job.rb index ce6f4ca41b..5f002f8a08 100644 --- a/app/jobs/student_loan_plan_check_job.rb +++ b/app/jobs/student_loan_plan_check_job.rb @@ -12,6 +12,9 @@ def perform claims.each do |claim| ClaimStudentLoanDetailsUpdater.call(claim) AutomatedChecks::ClaimVerifiers::StudentLoanPlan.new(claim:).perform + rescue => e + # If something goes wrong, log the error and continue + Rollbar.error(e) end end diff --git a/app/models/claim_student_loan_details_updater.rb b/app/models/claim_student_loan_details_updater.rb index 254d779ed7..0467b825db 100644 --- a/app/models/claim_student_loan_details_updater.rb +++ b/app/models/claim_student_loan_details_updater.rb @@ -15,9 +15,6 @@ def update_claim_with_latest_data claim.save!(context: :"student-loan") end - rescue => e - # If something goes wrong, log the error and continue - Rollbar.error(e) end private diff --git a/spec/jobs/student_loan_amount_check_job_spec.rb b/spec/jobs/student_loan_amount_check_job_spec.rb index ac77e754b2..12490eabde 100644 --- a/spec/jobs/student_loan_amount_check_job_spec.rb +++ b/spec/jobs/student_loan_amount_check_job_spec.rb @@ -10,100 +10,134 @@ let!(:journey_configuration) { create(:journey_configuration, :student_loans) } describe "#perform" do - before do - allow(ClaimStudentLoanDetailsUpdater).to receive(:call) - end - - shared_examples :skip_check do + context "without error" do before do - allow(AutomatedChecks::ClaimVerifiers::StudentLoanAmount).to receive(:new) + allow(ClaimStudentLoanDetailsUpdater).to receive(:call) end - it "excludes the claim from the check", :aggregate_failures do - expect(ClaimStudentLoanDetailsUpdater).not_to receive(:call).with(claim) - expect(AutomatedChecks::ClaimVerifiers::StudentLoanAmount).not_to receive(:new).with(claim: claim) - perform_job - end - end + shared_examples :skip_check do + before do + allow(AutomatedChecks::ClaimVerifiers::StudentLoanAmount).to receive(:new) + end - context "when the previous student loan amount check was run manually" do - let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: nil, manual: true) } + it "excludes the claim from the check", :aggregate_failures do + expect(ClaimStudentLoanDetailsUpdater).not_to receive(:call).with(claim) + expect(AutomatedChecks::ClaimVerifiers::StudentLoanAmount).not_to receive(:new).with(claim: claim) + perform_job + end + end - include_examples :skip_check - end + context "when the previous student loan amount check was run manually" do + let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: nil, manual: true) } - context "when a claim is not awaiting decision" do - let(:claim_status) { :approved } + include_examples :skip_check + end - include_examples :skip_check - end + context "when a claim is not awaiting decision" do + let(:claim_status) { :approved } - context "when a claim was submitted using the student loan questions" do - before do - claim.update!(submitted_using_slc_data: nil) + include_examples :skip_check end - include_examples :skip_check - end + context "when a claim was submitted using the student loan questions" do + before do + claim.update!(submitted_using_slc_data: nil) + end - context "when the student loan amount check did not run before" do - it "updates the student loan details" do - expect(ClaimStudentLoanDetailsUpdater).to receive(:call).with(claim) - perform_job + include_examples :skip_check end - it "runs the task" do - expect { perform_job } - .to change { claim.reload.notes.count } - .and change { claim.tasks.count } + context "when the student loan amount check did not run before" do + it "updates the student loan details" do + expect(ClaimStudentLoanDetailsUpdater).to receive(:call).with(claim) + perform_job + end + + it "runs the task" do + expect { perform_job } + .to change { claim.reload.notes.count } + .and change { claim.tasks.count } + end end - end - context "when the previous student loan amount check outcome was NO DATA" do - let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: nil, manual: false) } + context "when the previous student loan amount check outcome was NO DATA" do + let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: nil, manual: false) } - it "updates the student loan details" do - expect(ClaimStudentLoanDetailsUpdater).to receive(:call).with(claim) - perform_job - end + it "updates the student loan details" do + expect(ClaimStudentLoanDetailsUpdater).to receive(:call).with(claim) + perform_job + end - it "re-runs the task" do - expect { perform_job } - .to change { claim.reload.notes.count } - .and change { claim.tasks.last.updated_at } - .and not_change { claim.reload.tasks.count } + it "re-runs the task" do + expect { perform_job } + .to change { claim.reload.notes.count } + .and change { claim.tasks.last.updated_at } + .and not_change { claim.reload.tasks.count } + end end - end - context "when the previous student loan amount check outcome was FAILED" do - let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: :none, passed: false, manual: false) } + context "when the previous student loan amount check outcome was FAILED" do + let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: :none, passed: false, manual: false) } - it "does not update the student loan details" do - expect(ClaimStudentLoanDetailsUpdater).not_to receive(:call) - perform_job + it "does not update the student loan details" do + expect(ClaimStudentLoanDetailsUpdater).not_to receive(:call) + perform_job + end + + it "does not re-run the task" do + expect { perform_job } + .to not_change { claim.reload.notes.count } + .and not_change { claim.tasks.last.updated_at } + .and not_change { claim.reload.tasks.count } + end end - it "does not re-run the task" do - expect { perform_job } - .to not_change { claim.reload.notes.count } - .and not_change { claim.tasks.last.updated_at } - .and not_change { claim.reload.tasks.count } + context "when the previous student loan amount check outcome was PASSED" do + let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: :all, manual: false) } + + it "does not update the student loan details" do + expect(ClaimStudentLoanDetailsUpdater).not_to receive(:call) + perform_job + end + + it "does not re-run the task" do + expect { perform_job } + .to not_change { claim.reload.notes.count } + .and not_change { claim.tasks.last.updated_at } + .and not_change { claim.reload.tasks.count } + end end end - context "when the previous student loan amount check outcome was PASSED" do - let!(:previous_task) { create(:task, claim: claim, name: "student_loan_amount", claim_verifier_match: :all, manual: false) } + context "when there's an error" do + let(:exception) { ActiveRecord::RecordInvalid } - it "does not update the student loan details" do - expect(ClaimStudentLoanDetailsUpdater).not_to receive(:call) + before do + create( + :student_loans_data, + claim_reference: claim.reference, + nino: claim.national_insurance_number, + date_of_birth: claim.date_of_birth + ) + allow_any_instance_of(Claim).to receive(:save!) { raise(exception) } + allow(Rollbar).to receive(:error) + end + + it "suppresses the exception" do + expect { perform_job }.not_to raise_error + end + + it "logs the exception" do perform_job + + expect(Rollbar).to have_received(:error).with(exception) end - it "does not re-run the task" do - expect { perform_job } - .to not_change { claim.reload.notes.count } - .and not_change { claim.tasks.last.updated_at } - .and not_change { claim.reload.tasks.count } + it "does not update the student loan details or create a task or note" do + expect { perform_job }.to not_change { claim.student_loan_plan } + .and not_change { claim.eligibility.student_loan_repayment_amount } + .and not_change { claim.tasks.count } + .and not_change { claim.notes.count } end end end diff --git a/spec/jobs/student_loan_plan_check_job_spec.rb b/spec/jobs/student_loan_plan_check_job_spec.rb index 4e6fe5ed43..97accbf41a 100644 --- a/spec/jobs/student_loan_plan_check_job_spec.rb +++ b/spec/jobs/student_loan_plan_check_job_spec.rb @@ -125,5 +125,36 @@ include_examples :student_loan_plan_claim_verifier_not_called end + + context "when an error occurs while updating" do + let(:exception) { ActiveRecord::RecordInvalid } + + before do + create( + :student_loans_data, + claim_reference: claim.reference, + nino: claim.national_insurance_number, + date_of_birth: claim.date_of_birth + ) + allow_any_instance_of(Claim).to receive(:save!) { raise(exception) } + allow(Rollbar).to receive(:error) + end + + it "suppresses the exception" do + expect { perform_job }.not_to raise_error + end + + it "logs the exception" do + perform_job + + expect(Rollbar).to have_received(:error).with(exception) + end + + it "does not update the student loan details or create a task or note" do + expect { perform_job }.to not_change { claim.student_loan_plan } + .and not_change { claim.tasks.count } + .and not_change { claim.notes.count } + end + end end end diff --git a/spec/models/claim_student_loan_details_updater_spec.rb b/spec/models/claim_student_loan_details_updater_spec.rb index c9bde596e8..61e02b57db 100644 --- a/spec/models/claim_student_loan_details_updater_spec.rb +++ b/spec/models/claim_student_loan_details_updater_spec.rb @@ -108,31 +108,6 @@ end end - context "when an error occurs while updating" do - let(:exception) { ActiveRecord::RecordInvalid } - - before do - allow(claim).to receive(:save!) { raise(exception) } - allow(Rollbar).to receive(:error) - end - - it "suppresses the exception" do - expect { call }.not_to raise_error - end - - it "logs the exception" do - call - - expect(Rollbar).to have_received(:error).with(exception) - end - - it "does not update the student loan details" do - expect { call }.to not_change { claim.reload.has_student_loan } - .and not_change { claim.student_loan_plan } - .and not_change { claim.eligibility.student_loan_repayment_amount } - end - end - context "when updating a claim after submission" do let(:claim) { create(:claim, :submitted, :with_no_student_loan, policy:) }