From 43613009d66b2eb94cb456d9542b24593b001553 Mon Sep 17 00:00:00 2001
From: Colin Darie <colin@darie.eu>
Date: Thu, 2 Mar 2023 19:01:14 +0100
Subject: [PATCH] fix(job): Cron::WeeklyOverviewJob simplified and respect
 dolist API rate limiting

---
 app/jobs/cron/weekly_overview_job.rb       | 15 +++++++++------
 spec/jobs/cron/weekly_overview_job_spec.rb | 20 +++++++++++---------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/app/jobs/cron/weekly_overview_job.rb b/app/jobs/cron/weekly_overview_job.rb
index bc009a1b4..f8857d759 100644
--- a/app/jobs/cron/weekly_overview_job.rb
+++ b/app/jobs/cron/weekly_overview_job.rb
@@ -1,13 +1,16 @@
 class Cron::WeeklyOverviewJob < Cron::CronJob
   self.schedule_expression = "every monday at 7 am"
 
-  def perform(*args)
+  def perform
     # Feature flipped to avoid mails in staging due to unprocessed dossier
-    if Rails.application.config.ds_weekly_overview
-      Instructeur.all
-        .map { |instructeur| [instructeur, instructeur.last_week_overview] }
-        .reject { |_, overview| overview.nil? }
-        .each { |instructeur, _| InstructeurMailer.last_week_overview(instructeur).deliver_later }
+    return unless Rails.application.config.ds_weekly_overview
+
+    Instructeur.find_each do |instructeur|
+      # NOTE: it's not exactly accurate because rate limit is not shared between jobs processes
+      Dolist::API.sleep_until_limit_reset if Dolist::API.near_rate_limit?
+
+      # mailer won't send anything if overview if empty
+      InstructeurMailer.last_week_overview(instructeur)&.deliver_later
     end
   end
 end
diff --git a/spec/jobs/cron/weekly_overview_job_spec.rb b/spec/jobs/cron/weekly_overview_job_spec.rb
index df62333d9..e6fd7e254 100644
--- a/spec/jobs/cron/weekly_overview_job_spec.rb
+++ b/spec/jobs/cron/weekly_overview_job_spec.rb
@@ -2,7 +2,6 @@ RSpec.describe Cron::WeeklyOverviewJob, type: :job do
   describe 'perform' do
     let!(:instructeur) { create(:instructeur) }
     let(:overview) { double('overview') }
-    let(:mailer_double) { double('mailer', deliver_later: true) }
 
     context 'if the feature is enabled' do
       before do
@@ -12,11 +11,14 @@ RSpec.describe Cron::WeeklyOverviewJob, type: :job do
         Rails.application.config.ds_weekly_overview = false
       end
 
+      subject(:run_job) { Cron::WeeklyOverviewJob.new.perform }
+      # See also spec/mailers/instructeur_mailer_spec.rb
+
       context 'with one instructeur with one overview' do
+        let(:mailer_double) { double('mailer', deliver_later: true) }
         before do
-          expect_any_instance_of(Instructeur).to receive(:last_week_overview).and_return(overview)
           allow(InstructeurMailer).to receive(:last_week_overview).and_return(mailer_double)
-          Cron::WeeklyOverviewJob.new.perform
+          run_job
         end
 
         it { expect(InstructeurMailer).to have_received(:last_week_overview).with(instructeur) }
@@ -25,22 +27,22 @@ RSpec.describe Cron::WeeklyOverviewJob, type: :job do
 
       context 'with one instructeur with no overviews' do
         before do
-          expect_any_instance_of(Instructeur).to receive(:last_week_overview).and_return(nil)
-          allow(InstructeurMailer).to receive(:last_week_overview)
-          Cron::WeeklyOverviewJob.new.perform
+          allow(InstructeurMailer).to receive(:last_week_overview).and_return(nil)
+          run_job
         end
 
-        it { expect(InstructeurMailer).not_to have_received(:last_week_overview) }
+        it { expect(InstructeurMailer).to have_received(:last_week_overview).with(instructeur) }
+        it { expect { run_job }.not_to raise_error(NoMethodError) }
       end
     end
 
     context 'if the feature is disabled' do
       before do
-        allow(Instructeur).to receive(:all)
+        allow(Instructeur).to receive(:find_each)
         Cron::WeeklyOverviewJob.new.perform
       end
 
-      it { expect(Instructeur).not_to receive(:all) }
+      it { expect(Instructeur).not_to receive(:find_each) }
     end
   end
 end