From aa4cba04b1ebe16819e8adfec7a55dbbdc623d2f Mon Sep 17 00:00:00 2001
From: Thomas Leese <thomas@leese.io>
Date: Wed, 12 Jun 2024 08:30:37 +0100
Subject: [PATCH 1/5] Add Expirable#expires?

This adds a convenience method which returns true if the object should
expire at some point (i.e. it's got an expires_at set).
---
 app/jobs/expire_requestable_job.rb | 7 ++-----
 app/models/concerns/expirable.rb   | 7 +++++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/jobs/expire_requestable_job.rb b/app/jobs/expire_requestable_job.rb
index 08ec97ca1f..713bc2d40a 100644
--- a/app/jobs/expire_requestable_job.rb
+++ b/app/jobs/expire_requestable_job.rb
@@ -2,12 +2,9 @@
 
 class ExpireRequestableJob < ApplicationJob
   def perform(requestable)
-    if requestable.received? || requestable.expired? ||
-         !requestable.requested? || requestable.expires_at.nil?
-      return
-    end
+    return if requestable.received? || requestable.expired?
 
-    if Time.zone.now > requestable.expires_at
+    if requestable.expires? && Time.zone.now > requestable.expires_at
       ExpireRequestable.call(requestable:, user: "Expirer")
     end
   end
diff --git a/app/models/concerns/expirable.rb b/app/models/concerns/expirable.rb
index 6b6fa64280..57b09bfdec 100644
--- a/app/models/concerns/expirable.rb
+++ b/app/models/concerns/expirable.rb
@@ -9,9 +9,12 @@ def expires_at
     requested_at + expires_after
   end
 
+  def expires?
+    expires_at != nil
+  end
+
   def days_until_expired
-    return nil if expires_at.nil?
-    (expires_at.to_date - Time.zone.today).to_i
+    (expires_at.to_date - Time.zone.today).to_i if expires?
   end
 
   def expired!

From 65a989f2c06f2e33b7b5e86732d06876bb8ea0c9 Mon Sep 17 00:00:00 2001
From: Thomas Leese <thomas@leese.io>
Date: Wed, 12 Jun 2024 08:31:29 +0100
Subject: [PATCH 2/5] Add Expirable scopes

This adds an expired and not_expired scope on the expirable objects
which we can use to better filter the objects, and make it more
readable.
---
 app/jobs/expire_requestables_job.rb | 7 ++++---
 app/models/concerns/expirable.rb    | 5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/app/jobs/expire_requestables_job.rb b/app/jobs/expire_requestables_job.rb
index 482c1d62df..5a3f0e2e4b 100644
--- a/app/jobs/expire_requestables_job.rb
+++ b/app/jobs/expire_requestables_job.rb
@@ -5,9 +5,10 @@ def perform(requestable_class_name)
     requestable_class_name
       .constantize
       .requested
-      .where(expired_at: nil, received_at: nil)
+      .not_received
+      .not_expired
       .find_each do |requestable|
-        ExpireRequestableJob.perform_later(requestable)
-      end
+      ExpireRequestableJob.perform_later(requestable)
+    end
   end
 end
diff --git a/app/models/concerns/expirable.rb b/app/models/concerns/expirable.rb
index 57b09bfdec..7e9b0b3cdf 100644
--- a/app/models/concerns/expirable.rb
+++ b/app/models/concerns/expirable.rb
@@ -3,6 +3,11 @@
 module Expirable
   extend ActiveSupport::Concern
 
+  included do
+    scope :expired, -> { where.not(expired_at: nil) }
+    scope :not_expired, -> { where(expired_at: nil) }
+  end
+
   def expires_at
     return nil if requested_at.nil? || expires_after.nil?
 

From a937c8f8f996d26c9e32cf6206a915f2fd1398dd Mon Sep 17 00:00:00 2001
From: Thomas Leese <thomas@leese.io>
Date: Wed, 12 Jun 2024 08:42:57 +0100
Subject: [PATCH 3/5] Update expiration job schedule

This gives more time to each expiration requestable type to ensure
they're finished before moving on to the next one.
---
 config/schedule.yml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/config/schedule.yml b/config/schedule.yml
index 72bd01b40b..e7eedf1f91 100644
--- a/config/schedule.yml
+++ b/config/schedule.yml
@@ -16,22 +16,22 @@ expire_consent_requests:
   args: ["ConsentRequest"]
 
 expire_further_information_requests:
-  cron: "10 2 * * *"
+  cron: "12 2 * * *"
   class: ExpireRequestablesJob
   args: ["FurtherInformationRequest"]
 
 expire_professional_standing_requests:
-  cron: "20 2 * * *"
+  cron: "24 2 * * *"
   class: ExpireRequestablesJob
   args: ["ProfessionalStandingRequest"]
 
 expire_qualification_requests:
-  cron: "30 2 * * *"
+  cron: "36 2 * * *"
   class: ExpireRequestablesJob
   args: ["QualificationRequest"]
 
 expire_reference_requests:
-  cron: "40 2 * * *"
+  cron: "48 2 * * *"
   class: ExpireRequestablesJob
   args: ["ReferenceRequest"]
 

From 7720aad0254ca2df76190558d56464293f711bc7 Mon Sep 17 00:00:00 2001
From: Thomas Leese <thomas@leese.io>
Date: Wed, 12 Jun 2024 08:44:05 +0100
Subject: [PATCH 4/5] Ensure received requestables aren't expirable

When received the `expires_at` value becomes `nil` and `expires?` will
return `false`. This is to ensure that we don't accidentally expire it
when it shouldn't be.
---
 app/models/application_form.rb            | 4 ++++
 app/models/concerns/expirable.rb          | 5 +++--
 app/models/further_information_request.rb | 9 ++++++---
 app/models/reference_request.rb           | 7 +++++--
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/app/models/application_form.rb b/app/models/application_form.rb
index f425f82bfd..8d7860bbb3 100644
--- a/app/models/application_form.rb
+++ b/app/models/application_form.rb
@@ -320,6 +320,10 @@ def requested_at
     created_at
   end
 
+  def received_at
+    submitted_at
+  end
+
   def expires_after
     submitted? ? nil : 6.months
   end
diff --git a/app/models/concerns/expirable.rb b/app/models/concerns/expirable.rb
index 7e9b0b3cdf..df5e517be7 100644
--- a/app/models/concerns/expirable.rb
+++ b/app/models/concerns/expirable.rb
@@ -9,7 +9,7 @@ module Expirable
   end
 
   def expires_at
-    return nil if requested_at.nil? || expires_after.nil?
+    return nil if received_at || requested_at.nil? || expires_after.nil?
 
     requested_at + expires_after
   end
@@ -19,7 +19,8 @@ def expires?
   end
 
   def days_until_expired
-    (expires_at.to_date - Time.zone.today).to_i if expires?
+    @days_until_expired ||=
+      expires? ? (expires_at.to_date - Time.zone.today).to_i : nil
   end
 
   def expired!
diff --git a/app/models/further_information_request.rb b/app/models/further_information_request.rb
index 612ffc0ceb..c1506e4ec4 100644
--- a/app/models/further_information_request.rb
+++ b/app/models/further_information_request.rb
@@ -41,9 +41,12 @@ class FurtherInformationRequest < ApplicationRecord
   FOUR_WEEK_COUNTRY_CODES = %w[AU CA GI NZ US].freeze
 
   def should_send_reminder_email?(_name, number_of_reminders_sent)
-    (days_until_expired <= 14 && number_of_reminders_sent.zero?) ||
-      (days_until_expired <= 7 && number_of_reminders_sent == 1) ||
-      (days_until_expired <= 2 && number_of_reminders_sent == 2)
+    days_until_expired &&
+      (
+        (days_until_expired <= 14 && number_of_reminders_sent.zero?) ||
+          (days_until_expired <= 7 && number_of_reminders_sent == 1) ||
+          (days_until_expired <= 2 && number_of_reminders_sent == 2)
+      )
   end
 
   def send_reminder_email(_name, _number_of_reminders_sent)
diff --git a/app/models/reference_request.rb b/app/models/reference_request.rb
index d2fcc51dd6..f1f896c58e 100644
--- a/app/models/reference_request.rb
+++ b/app/models/reference_request.rb
@@ -95,8 +95,11 @@ def responses_given?
   end
 
   def should_send_reminder_email?(_name, number_of_reminders_sent)
-    (days_until_expired <= 28 && number_of_reminders_sent.zero?) ||
-      (days_until_expired <= 14 && number_of_reminders_sent == 1)
+    days_until_expired &&
+      (
+        (days_until_expired <= 28 && number_of_reminders_sent.zero?) ||
+          (days_until_expired <= 14 && number_of_reminders_sent == 1)
+      )
   end
 
   def send_reminder_email(_name, number_of_reminders_sent)

From acf8145b52a949a45e7770ae5b1d334924348b6b Mon Sep 17 00:00:00 2001
From: Thomas Leese <thomas@leese.io>
Date: Wed, 12 Jun 2024 08:46:04 +0100
Subject: [PATCH 5/5] Expire requestable synchronously

This fixes an issue where if more than one requestable happened to
expire at the same time, there is a race condition between when the
status and action required at fields are updated, leading to incorrect
application statuses.
---
 app/jobs/expire_requestable_job.rb            |  11 --
 app/jobs/expire_requestables_job.rb           |   6 +-
 spec/jobs/expire_requestable_job_spec.rb      | 167 ------------------
 ...pec.rb => expire_requestables_job_spec.rb} |  28 +--
 4 files changed, 22 insertions(+), 190 deletions(-)
 delete mode 100644 app/jobs/expire_requestable_job.rb
 delete mode 100644 spec/jobs/expire_requestable_job_spec.rb
 rename spec/jobs/{expire_requestables_spec.rb => expire_requestables_job_spec.rb} (66%)

diff --git a/app/jobs/expire_requestable_job.rb b/app/jobs/expire_requestable_job.rb
deleted file mode 100644
index 713bc2d40a..0000000000
--- a/app/jobs/expire_requestable_job.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-# frozen_string_literal: true
-
-class ExpireRequestableJob < ApplicationJob
-  def perform(requestable)
-    return if requestable.received? || requestable.expired?
-
-    if requestable.expires? && Time.zone.now > requestable.expires_at
-      ExpireRequestable.call(requestable:, user: "Expirer")
-    end
-  end
-end
diff --git a/app/jobs/expire_requestables_job.rb b/app/jobs/expire_requestables_job.rb
index 5a3f0e2e4b..b5bf077053 100644
--- a/app/jobs/expire_requestables_job.rb
+++ b/app/jobs/expire_requestables_job.rb
@@ -7,8 +7,10 @@ def perform(requestable_class_name)
       .requested
       .not_received
       .not_expired
-      .find_each do |requestable|
-      ExpireRequestableJob.perform_later(requestable)
+      .each do |requestable|
+      if requestable.expires? && Time.zone.now > requestable.expires_at
+        ExpireRequestable.call(requestable:, user: "Expirer")
+      end
     end
   end
 end
diff --git a/spec/jobs/expire_requestable_job_spec.rb b/spec/jobs/expire_requestable_job_spec.rb
deleted file mode 100644
index 28ce70ff0e..0000000000
--- a/spec/jobs/expire_requestable_job_spec.rb
+++ /dev/null
@@ -1,167 +0,0 @@
-# frozen_string_literal: true
-
-require "rails_helper"
-
-RSpec.describe ExpireRequestableJob do
-  describe "#perform" do
-    let(:region) { create(:region, :in_country, country_code: "FR") }
-    let(:application_form) { create(:application_form, :submitted, region:) }
-    let(:assessment) { create(:assessment, application_form:) }
-
-    subject(:perform) { described_class.new.perform(requestable) }
-
-    shared_examples_for "not expired requestable" do
-      it "doesn't expire" do
-        expect(ExpireRequestable).to_not receive(:call).with(
-          requestable:,
-          user: a_kind_of(String),
-        )
-        perform
-      end
-    end
-
-    shared_examples_for "expired requestable" do
-      it "expires" do
-        expect(ExpireRequestable).to receive(:call).with(
-          requestable:,
-          user: a_kind_of(String),
-        )
-        perform
-      end
-    end
-
-    context "with requested FI request" do
-      let(:requestable) do
-        create(:further_information_request, requested_at:, assessment:)
-      end
-
-      context "when less than six weeks old" do
-        let(:requested_at) { (6.weeks - 2.hours).ago }
-        it_behaves_like "not expired requestable"
-      end
-
-      context "when it is more than six weeks old" do
-        let(:requested_at) { (6.weeks + 2.hours).ago }
-        it_behaves_like "expired requestable"
-      end
-
-      context "when the applicant is from a country with a 4 week expiry" do
-        let(:application_form) do
-          create(:application_form, :submitted, :old_regulations, region:)
-        end
-
-        # Australia, Canada, Gibraltar, New Zealand, US
-        %w[AU CA GI NZ US].each do |country_code|
-          context "from country_code #{country_code}" do
-            let(:region) { create(:region, :in_country, country_code:) }
-
-            context "when it is less than four weeks old" do
-              let(:requested_at) { (4.weeks - 1.hour).ago }
-              it_behaves_like "not expired requestable"
-            end
-
-            context "when it is more than four weeks old from #{country_code}" do
-              let(:requested_at) { (4.weeks + 1.hour).ago }
-              it_behaves_like "expired requestable"
-            end
-          end
-        end
-      end
-    end
-
-    context "with any received FI request" do
-      let(:requestable) do
-        create(:received_further_information_request, requested_at: 1.year.ago)
-      end
-
-      it_behaves_like "not expired requestable"
-    end
-
-    context "with any expired FI request" do
-      let(:requestable) do
-        create(:further_information_request, :expired, requested_at: 1.year.ago)
-      end
-
-      it_behaves_like "not expired requestable"
-    end
-
-    context "with a requested professional standing request" do
-      before do
-        application_form.update!(
-          teaching_authority_provides_written_statement: true,
-        )
-      end
-
-      let(:requestable) do
-        create(:professional_standing_request, requested_at:, assessment:)
-      end
-
-      context "when less than 36 weeks old" do
-        let(:requested_at) { (36.weeks - 1.hour).ago }
-
-        it_behaves_like "not expired requestable"
-      end
-
-      context "when it is more than 36 weeks old" do
-        let(:requested_at) { (36.weeks + 1.hour).ago }
-
-        it_behaves_like "expired requestable"
-      end
-    end
-
-    context "with any received professional standing request" do
-      let(:requestable) do
-        create(
-          :received_professional_standing_request,
-          requested_at: 1.year.ago,
-        )
-      end
-
-      it_behaves_like "not expired requestable"
-    end
-
-    context "with any expired professional standing request" do
-      let(:requestable) do
-        create(
-          :professional_standing_request,
-          :expired,
-          requested_at: 1.year.ago,
-        )
-      end
-
-      it_behaves_like "not expired requestable"
-    end
-
-    context "with a requested reference request" do
-      let(:requestable) { create(:reference_request, requested_at:) }
-
-      context "when less than six weeks old" do
-        let(:requested_at) { (6.weeks - 1.hour).ago }
-
-        it_behaves_like "not expired requestable"
-      end
-
-      context "when it is more than six weeks old" do
-        let(:requested_at) { (6.weeks + 1.hour).ago }
-
-        it_behaves_like "expired requestable"
-      end
-    end
-
-    context "with any received reference request" do
-      let(:requestable) do
-        create(:received_reference_request, requested_at: 1.year.ago)
-      end
-
-      it_behaves_like "not expired requestable"
-    end
-
-    context "with any expired reference request" do
-      let(:requestable) do
-        create(:reference_request, :expired, requested_at: 1.year.ago)
-      end
-
-      it_behaves_like "not expired requestable"
-    end
-  end
-end
diff --git a/spec/jobs/expire_requestables_spec.rb b/spec/jobs/expire_requestables_job_spec.rb
similarity index 66%
rename from spec/jobs/expire_requestables_spec.rb
rename to spec/jobs/expire_requestables_job_spec.rb
index 7b8f5c0c25..05b82a554b 100644
--- a/spec/jobs/expire_requestables_spec.rb
+++ b/spec/jobs/expire_requestables_job_spec.rb
@@ -7,28 +7,36 @@
     describe "#perform" do
       subject(:perform) { described_class.new.perform(class_name) }
 
-      let!(:requested_requestable) { create(:"requested_#{factory_name}") }
+      let!(:requested_requestable) do
+        create(:"requested_#{factory_name}", requested_at: 1.year.ago)
+      end
       let!(:received_requestable) { create(:"received_#{factory_name}") }
       let!(:expired_requestable) do
         create(:"requested_#{factory_name}", :expired)
       end
 
-      it "enqueues a job for each 'requested' #{class_name}s" do
-        expect { perform }.to have_enqueued_job(ExpireRequestableJob).with(
-          requested_requestable,
+      it "enqueues a job for each requested #{class_name}s" do
+        expect(ExpireRequestable).to receive(:call).with(
+          requestable: requested_requestable,
+          user: "Expirer",
         )
+        perform
       end
 
-      it "doesn't enqueue a job for 'received' #{class_name}s" do
-        expect { perform }.to_not have_enqueued_job(ExpireRequestableJob).with(
-          received_requestable,
+      it "doesn't enqueue a job for received #{class_name}s" do
+        expect(ExpireRequestable).to_not receive(:call).with(
+          requestable: received_requestable,
+          user: "Expirer",
         )
+        perform
       end
 
-      it "doesn't enqueue a job for 'expired' #{class_name}s" do
-        expect { perform }.to_not have_enqueued_job(ExpireRequestableJob).with(
-          expired_requestable,
+      it "doesn't enqueue a job for expired #{class_name}s" do
+        expect(ExpireRequestable).to_not receive(:call).with(
+          requestable: expired_requestable,
+          user: "Expirer",
         )
+        perform
       end
     end
   end