Refactor AssessAssessmentSection

This removes the need to return true or false from the service, since
we're not using this value anywhere anyway.

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?

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

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