Merge pull request #1707 from DFE-Digital/use-old-value-new-value

Stop using old_state and new_state

app/components/timeline_entry/component.rb

@@ -46,14 +46,16 @@ def status_changed_vars
         old_status:
           render(
             StatusTag::Component.new(
-              status: timeline_event.old_state,
+              status:
+                timeline_event.old_state.presence || timeline_event.old_value,
               class_context: "timeline-event",
             ),
           ).strip,
         new_status:
           render(
             StatusTag::Component.new(
-              status: timeline_event.new_state,
+              status:
+                timeline_event.new_state.presence || timeline_event.new_value,
               class_context: "timeline-event",
             ),
           ).strip,

app/lib/application_form_status_updater.rb

@@ -29,8 +29,8 @@ def call
         application_form.update!(status:)
         create_timeline_event(
           event_type: "status_changed",
-          old_state: old_status,
-          new_state: status,
+          old_value: old_status,
+          new_value: status,
         )
       end
 

app/models/timeline_event.rb

@@ -90,15 +90,6 @@ class TimelineEvent < ApplicationRecord
             absence: true,
             unless: -> { assessor_assigned? || reviewer_assigned? }
 
-  validates :old_state,
-            :new_state,
-            presence: true,
-            if: -> { status_changed? || assessment_section_recorded? }
-  validates :old_state,
-            :new_state,
-            absence: true,
-            unless: -> { status_changed? || assessment_section_recorded? }
-
   validates :assessment_section,
             presence: true,
             if: :assessment_section_recorded?
@@ -154,15 +145,15 @@ class TimelineEvent < ApplicationRecord
             :new_value,
             presence: true,
             if: -> do
-              action_required_by_changed? || information_changed? ||
-                stage_changed?
+              action_required_by_changed? || assessment_section_recorded? ||
+                information_changed? || stage_changed? || status_changed?
             end
   validates :old_value,
             :new_value,
             absence: true,
             unless: -> do
-              action_required_by_changed? || information_changed? ||
-                stage_changed?
+              action_required_by_changed? || assessment_section_recorded? ||
+                information_changed? || stage_changed? || status_changed?
             end
 
   validates :column_name, presence: true, if: :information_changed?

app/services/update_assessment_section.rb

@@ -11,7 +11,7 @@ def initialize(assessment_section:, user:, params:)
   end
 
   def call
-    old_state = assessment_section.status
+    old_status = assessment_section.status
 
     ActiveRecord::Base.transaction do
       selected_keys = selected_failure_reasons.keys
@@ -36,7 +36,7 @@ def call
       next false unless assessment_section.update(params)
 
       update_application_form_assessor
-      create_timeline_event(old_state:)
+      create_timeline_event(old_status:)
       update_assessment_started_at
       update_application_form_state
 
@@ -61,17 +61,17 @@ def update_application_form_assessor
     end
   end
 
-  def create_timeline_event(old_state:)
-    new_state = assessment_section.status
-    return if old_state == new_state
+  def create_timeline_event(old_status:)
+    new_status = assessment_section.status
+    return if old_status == new_status
 
     TimelineEvent.create!(
       creator: user,
       event_type: :assessment_section_recorded,
       assessment_section:,
       application_form:,
-      old_state:,
-      new_state:,
+      old_value: old_status,
+      new_value: new_status,
     )
   end
 

lib/tasks/timeline_events.rake

@@ -1,8 +1,15 @@
 namespace :timeline_events do
-  desc "Migrate state_changed events"
-  task migrate_state_changed: :environment do
-    TimelineEvent.where(event_type: "state_changed").update_all(
-      event_type: "status_changed",
-    )
+  desc "Migrate old_state/new_state"
+  task migrate_old_new_state: :environment do
+    TimelineEvent
+      .where(event_type: %w[status_changed assessment_section_recorded])
+      .each do |timeline_event|
+        timeline_event.update!(
+          old_value: timeline_event.old_state,
+          new_value: timeline_event.new_state,
+          old_state: "",
+          new_state: "",
+        )
+      end
   end
 end

spec/components/timeline_entry_spec.rb

@@ -56,20 +56,20 @@
       create(
         :timeline_event,
         :status_changed,
-        old_state: "submitted",
-        new_state: "awarded",
+        old_value: "submitted",
+        new_value: "awarded",
       )
     end
-    let(:old_state) do
-      I18n.t("components.status_tag.#{timeline_event.old_state}")
+    let(:old_status_tag) do
+      I18n.t("components.status_tag.#{timeline_event.old_value}")
     end
-    let(:new_state) do
-      I18n.t("components.status_tag.#{timeline_event.new_state}")
+    let(:new_status_tag) do
+      I18n.t("components.status_tag.#{timeline_event.new_value}")
     end
 
     it "describes the event" do
       expect(component.text.squish).to include(
-        "Status changed from #{old_state} to #{new_state}",
+        "Status changed from #{old_status_tag} to #{new_status_tag}",
       )
     end
 
@@ -87,7 +87,7 @@
       create(
         :timeline_event,
         :assessment_section_recorded,
-        new_state: "completed",
+        new_value: "completed",
         assessment_section:,
       )
     end

spec/factories/application_forms.rb

@@ -190,8 +190,8 @@
           :status_changed,
           application_form:,
           creator: application_form.teacher,
-          old_state: "draft",
-          new_state: "submitted",
+          old_value: "draft",
+          new_value: "submitted",
         )
       end
     end

spec/factories/timeline_events.rb

@@ -68,8 +68,8 @@
 
     trait :status_changed do
       event_type { "status_changed" }
-      old_state { ApplicationForm.statuses.keys.sample }
-      new_state { ApplicationForm.statuses.keys.sample }
+      old_value { ApplicationForm.statuses.keys.sample }
+      new_value { ApplicationForm.statuses.keys.sample }
     end
 
     trait :assessment_section_recorded do
@@ -82,8 +82,8 @@
           assessment: build(:assessment, application_form:),
         )
       end
-      old_state { %i[not_started invalid completed].sample }
-      new_state { %i[not_started invalid completed].sample }
+      old_value { %i[not_started invalid completed].sample }
+      new_value { %i[not_started invalid completed].sample }
     end
 
     trait :note_created do

spec/lib/application_form_status_updater_spec.rb

@@ -72,8 +72,8 @@
         :status_changed,
         creator: user,
         application_form:,
-        old_state: "draft",
-        new_state: new_status,
+        old_value: "draft",
+        new_value: new_status,
       )
     end
   end

spec/models/timeline_event_spec.rb

@@ -100,8 +100,6 @@
       before { timeline_event.event_type = :assessor_assigned }
 
       it { is_expected.to_not validate_presence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -123,8 +121,6 @@
       before { timeline_event.event_type = :reviewer_assigned }
 
       it { is_expected.to_not validate_presence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -146,8 +142,6 @@
       before { timeline_event.event_type = :status_changed }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_presence_of(:old_state) }
-      it { is_expected.to validate_presence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -161,16 +155,14 @@
       it { is_expected.to validate_absence_of(:requestable_type) }
       it { is_expected.to validate_absence_of(:work_history_id) }
       it { is_expected.to validate_absence_of(:column_name) }
-      it { is_expected.to validate_absence_of(:old_value) }
-      it { is_expected.to validate_absence_of(:new_value) }
+      it { is_expected.to validate_presence_of(:old_value) }
+      it { is_expected.to validate_presence_of(:new_value) }
     end
 
     context "with an assessment section recorded event type" do
       before { timeline_event.event_type = :assessment_section_recorded }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_presence_of(:old_state) }
-      it { is_expected.to validate_presence_of(:new_state) }
       it { is_expected.to validate_presence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -184,16 +176,14 @@
       it { is_expected.to validate_absence_of(:requestable_type) }
       it { is_expected.to validate_absence_of(:work_history_id) }
       it { is_expected.to validate_absence_of(:column_name) }
-      it { is_expected.to validate_absence_of(:old_value) }
-      it { is_expected.to validate_absence_of(:new_value) }
+      it { is_expected.to validate_presence_of(:old_value) }
+      it { is_expected.to validate_presence_of(:new_value) }
     end
 
     context "with a note created event type" do
       before { timeline_event.event_type = :note_created }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_presence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -215,8 +205,6 @@
       before { timeline_event.event_type = :email_sent }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_presence_of(:mailer_class_name) }
@@ -238,8 +226,6 @@
       before { timeline_event.event_type = :age_range_subjects_verified }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -261,8 +247,6 @@
       before { timeline_event.event_type = :requestable_received }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -294,8 +278,6 @@
       before { timeline_event.event_type = :requestable_received }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -327,8 +309,6 @@
       before { timeline_event.event_type = :requestable_expired }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -360,8 +340,6 @@
       before { timeline_event.event_type = :requestable_assessed }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -393,8 +371,6 @@
       before { timeline_event.event_type = :action_required_by_changed }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }
@@ -416,8 +392,6 @@
       before { timeline_event.event_type = :stage_changed }
 
       it { is_expected.to validate_absence_of(:assignee) }
-      it { is_expected.to validate_absence_of(:old_state) }
-      it { is_expected.to validate_absence_of(:new_state) }
       it { is_expected.to validate_absence_of(:assessment_section) }
       it { is_expected.to validate_absence_of(:note) }
       it { is_expected.to validate_absence_of(:mailer_class_name) }

spec/services/submit_application_form_spec.rb

@@ -64,8 +64,8 @@
       :status_changed,
       creator: user,
       application_form:,
-      old_state: "draft",
-      new_state: "submitted",
+      old_value: "draft",
+      new_value: "submitted",
     )
   end