From 16be95e2759f83e2d423d8437183d60ceefc2b69 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 22 May 2024 13:32:19 +0100 Subject: [PATCH] Refactor AssessAssessmentSection This removes the need to return true or false from the service, since we're not using this value anywhere anyway. --- .../updates_english_language_status.rb | 18 +- app/services/assess_assessment_section.rb | 52 +++-- .../assess_assessment_section_spec.rb | 192 ++++++++---------- 3 files changed, 115 insertions(+), 147 deletions(-) diff --git a/app/forms/concerns/assessor_interface/updates_english_language_status.rb b/app/forms/concerns/assessor_interface/updates_english_language_status.rb index 267fbb7604..b1b234e403 100644 --- a/app/forms/concerns/assessor_interface/updates_english_language_status.rb +++ b/app/forms/concerns/assessor_interface/updates_english_language_status.rb @@ -36,14 +36,16 @@ def english_language_section(assessment) private def save_english_language_status - return true unless update_english_language_status? - - AssessAssessmentSection.call( - assessment_section: - self.class.english_language_section(assessment_section.assessment), - user:, - passed: english_language_section_passed, - ) + if update_english_language_status? + AssessAssessmentSection.call( + assessment_section: + self.class.english_language_section(assessment_section.assessment), + user:, + passed: english_language_section_passed, + ) + end + + true end def update_english_language_status? diff --git a/app/services/assess_assessment_section.rb b/app/services/assess_assessment_section.rb index e84557a717..c58a44e51b 100644 --- a/app/services/assess_assessment_section.rb +++ b/app/services/assess_assessment_section.rb @@ -19,35 +19,12 @@ def call old_status = assessment_section.status ActiveRecord::Base.transaction do - selected_keys = selected_failure_reasons.keys - - assessment_section - .selected_failure_reasons - .where.not(key: selected_keys) - .destroy_all - - selected_failure_reasons.each do |key, assessor_feedback| - failure_reason = - assessment_section.selected_failure_reasons.find_or_initialize_by( - key:, - ) - failure_reason.update!(assessor_feedback: assessor_feedback[:notes]) - next unless assessor_feedback[:work_histories] - failure_reason.update!( - work_histories: assessor_feedback[:work_histories], - ) - end - - unless assessment_section.update(passed:, assessed_at: Time.zone.now) - next false - end - + update_selected_failure_reasons + update_passed_and_assessed_at update_application_form_assessor create_timeline_event(old_status:) update_assessment_started_at update_application_form_state - - true end end @@ -58,6 +35,28 @@ def call delegate :assessment, to: :assessment_section delegate :application_form, to: :assessment + def update_selected_failure_reasons + selected_keys = selected_failure_reasons.keys + + assessment_section + .selected_failure_reasons + .where.not(key: selected_keys) + .destroy_all + + selected_failure_reasons.each do |key, assessor_feedback| + failure_reason = + assessment_section.selected_failure_reasons.find_or_initialize_by(key:) + + failure_reason.update!(assessor_feedback: assessor_feedback[:notes]) + next unless assessor_feedback[:work_histories] + failure_reason.update!(work_histories: assessor_feedback[:work_histories]) + end + end + + def update_passed_and_assessed_at + assessment_section.update!(passed:, assessed_at: Time.zone.now) + end + def update_application_form_assessor if application_form.assessor.nil? AssignApplicationFormAssessor.call( @@ -83,8 +82,7 @@ def create_timeline_event(old_status:) end def update_assessment_started_at - return if assessment.started_at - assessment.update!(started_at: Time.zone.now) + assessment.update!(started_at: Time.zone.now) if assessment.started_at.nil? end def update_application_form_state diff --git a/spec/services/assess_assessment_section_spec.rb b/spec/services/assess_assessment_section_spec.rb index 2b8fdc1515..07bf45ffe1 100644 --- a/spec/services/assess_assessment_section_spec.rb +++ b/spec/services/assess_assessment_section_spec.rb @@ -25,144 +25,112 @@ ) end - context "when the update is successful" do - it { is_expected.to be true } + it "sets the status" do + expect { call }.to change(assessment_section, :status).from( + "not_started", + ).to("rejected") + end - it "sets the status" do - expect { call }.to change(assessment_section, :status).from( - "not_started", - ).to("rejected") - end + it "sets the assessed at date" do + expect { travel_to(Date.new(2020, 1, 1)) { call } }.to change( + assessment_section, + :assessed_at, + ).from(nil).to(Date.new(2020, 1, 1)) + end - it "sets the assessed at date" do - expect { travel_to(Date.new(2020, 1, 1)) { call } }.to change( - assessment_section, - :assessed_at, - ).from(nil).to(Date.new(2020, 1, 1)) - end + it "records a timeline event" do + expect { call }.to have_recorded_timeline_event( + :assessment_section_recorded, + ) + end - it "records a timeline event" do - expect { call }.to have_recorded_timeline_event( - :assessment_section_recorded, - ) - end + it "creates the assessment failure reason records" do + expect { call }.to change { + SelectedFailureReason.where( + assessment_section:, + key: selected_failure_reason_key, + ).count + }.by(1) + end - it "creates the assessment failure reason records" do - expect { call }.to change { - SelectedFailureReason.where( - assessment_section:, + context "when the failure reason already exists" do + context "when the feedback has been updated" do + before do + assessment_section.selected_failure_reasons.create( key: selected_failure_reason_key, - ).count - }.by(1) - end + assessor_feedback: "I need updating", + ) + end - context "when the failure reason already exists" do - context "when the feedback has been updated" do - before do - assessment_section.selected_failure_reasons.create( - key: selected_failure_reason_key, - assessor_feedback: "I need updating", - ) - end - - it "doesn't create a new assessment failure reason record" do - expect { call }.not_to( - change do - SelectedFailureReason.where( - assessment_section:, - key: selected_failure_reason_key, - ).count - end, - ) - end - - it "updates the existing record" do - expect { call }.to change { - SelectedFailureReason.find_by( + it "doesn't create a new assessment failure reason record" do + expect { call }.not_to( + change do + SelectedFailureReason.where( + assessment_section:, key: selected_failure_reason_key, - ).assessor_feedback - }.to(selected_failure_reason_assessor_feedback[:notes]) - end + ).count + end, + ) end - context "when the failure reason is no longer selected" do - let(:different_key) do - ( - assessment_section.failure_reasons - - Array(selected_failure_reason_key) - ).sample - end - - before do - assessment_section.selected_failure_reasons.create( - key: different_key, - assessor_feedback: "I need deleting", - ) - end - - it "deletes the now unselected failure reason" do - expect { call }.to change { - SelectedFailureReason.where(key: different_key).count - }.by(-1) - end + it "updates the existing record" do + expect { call }.to change { + SelectedFailureReason.find_by( + key: selected_failure_reason_key, + ).assessor_feedback + }.to(selected_failure_reason_assessor_feedback[:notes]) end end - it "changes the assessor" do - expect { call }.to change { application_form.assessor }.from(nil).to(user) - end - - context "with an existing assessor" do - before { application_form.assessor = create(:staff) } - - it "doesn't change the assessor" do - expect { call }.to_not(change { application_form.assessor }) + context "when the failure reason is no longer selected" do + let(:different_key) do + ( + assessment_section.failure_reasons - + Array(selected_failure_reason_key) + ).sample end - end - it "changes the application form state" do - expect { call }.to change { application_form.statuses }.from( - %w[assessment_not_started], - ).to(%w[assessment_in_progress]) - end - - it "changes the assessment started at" do - expect { call }.to change(assessment, :started_at).from(nil) - end - - context "with an existing assessment started at" do - before { assessment.update!(started_at: Date.new(2021, 1, 1)) } + before do + assessment_section.selected_failure_reasons.create( + key: different_key, + assessor_feedback: "I need deleting", + ) + end - it "doesn't change the assessor" do - expect { call }.to_not change(assessment, :started_at) + it "deletes the now unselected failure reason" do + expect { call }.to change { + SelectedFailureReason.where(key: different_key).count + }.by(-1) end end end - context "when the update fails" do - before { allow(assessment_section).to receive(:update).and_return(false) } - - it { is_expected.to be false } + it "changes the assessor" do + expect { call }.to change { application_form.assessor }.from(nil).to(user) + end - it "doesn't record a timeline event" do - expect { call }.to_not have_recorded_timeline_event( - :assessment_section_recorded, - ) - end + context "with an existing assessor" do + before { application_form.assessor = create(:staff) } it "doesn't change the assessor" do - expect { call }.to_not change(application_form, :assessor) + expect { call }.to_not(change { application_form.assessor }) end + end - it "doesn't change the application form stage" do - expect { call }.to_not change(application_form, :stage) - end + it "changes the application form state" do + expect { call }.to change { application_form.statuses }.from( + %w[assessment_not_started], + ).to(%w[assessment_in_progress]) + end - it "doesn't change the application form statuses" do - expect { call }.to_not change(application_form, :statuses) - end + it "changes the assessment started at" do + expect { call }.to change(assessment, :started_at).from(nil) + end - it "doesn't change the assessment started at" do + context "with an existing assessment started at" do + before { assessment.update!(started_at: Date.new(2021, 1, 1)) } + + it "doesn't change the assessor" do expect { call }.to_not change(assessment, :started_at) end end