FIX: Fix reminder frequency not getting sent daily when early by a fe…

…w seconds/minutes (#496) Add a buffer in determining whether to remind a user about assignments
discourse · Aug 10, 2023 · f5cc748 · f5cc748
1 parent a0bd071
commit f5cc748
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 37 deletions.
diff --git a/app/jobs/scheduled/enqueue_reminders.rb b/app/jobs/scheduled/enqueue_reminders.rb
@@ -20,6 +20,8 @@ def allowed_group_ids
       Group.assign_allowed_groups.pluck(:id).join(",")
     end
 
+    REMINDER_BUFFER_MINUTES = 10
+
     def user_ids
       global_frequency = SiteSetting.remind_assigns_frequency
       frequency =
@@ -46,7 +48,7 @@ def user_ids
         AND #{frequency} > 0
         AND (
           last_reminder.value IS NULL OR
-          last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency})
+          last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) + ('1 MINUTE'::INTERVAL * #{REMINDER_BUFFER_MINUTES})
         )
         AND assignments.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency})
         AND assignments.assigned_to_type = 'User'

diff --git a/spec/jobs/scheduled/enqueue_reminders_spec.rb b/spec/jobs/scheduled/enqueue_reminders_spec.rb
@@ -3,12 +3,13 @@
 require "rails_helper"
 
 RSpec.describe Jobs::EnqueueReminders do
-  let(:assign_allowed_group) { Group.find_by(name: "staff") }
-  let(:user) { Fabricate(:user, groups: [assign_allowed_group]) }
+  fab!(:assign_allowed_group) { Fabricate(:group) }
+  fab!(:user) { Fabricate(:user, groups: [assign_allowed_group]) }
 
   before do
     SiteSetting.remind_assigns_frequency = RemindAssignsFrequencySiteSettings::MONTHLY_MINUTES
     SiteSetting.assign_enabled = true
+    SiteSetting.assign_allowed_on_groups = "#{assign_allowed_group.id}"
   end
 
   describe "#execute" do
@@ -36,55 +37,69 @@
       assert_reminders_enqueued(0)
     end
 
-    it "does not enqueue a reminder if it's too soon" do
-      user.upsert_custom_fields(PendingAssignsReminder::REMINDED_AT => 2.days.ago)
-      assign_multiple_tasks_to(user)
+    it "doesn't count assigns from deleted topics" do
+      deleted_post = Fabricate(:post)
+      assign_one_task_to(user, post: deleted_post)
+      (PendingAssignsReminder::REMINDER_THRESHOLD - 1).times { assign_one_task_to(user) }
+
+      deleted_post.topic.trash!
 
       assert_reminders_enqueued(0)
     end
 
-    it "enqueues a reminder if the user was reminded more than a month ago" do
-      user.upsert_custom_fields(PendingAssignsReminder::REMINDED_AT => 31.days.ago)
-      assign_multiple_tasks_to(user)
+    describe "assignment frequency" do
+      it "enqueues a reminder if the user reminder frequency is 1 day and the last reminded at is almost 1 day" do
+        user.custom_fields[
+          PendingAssignsReminder::REMINDERS_FREQUENCY
+        ] = RemindAssignsFrequencySiteSettings::DAILY_MINUTES
+        user.custom_fields[PendingAssignsReminder::REMINDED_AT] = 1.days.ago
+        user.save
 
-      assert_reminders_enqueued(1)
-    end
+        assign_multiple_tasks_to(user, assigned_on: 1.day.ago - 1.minute)
 
-    it "does not enqueue reminders if the remind frequency is set to never" do
-      SiteSetting.remind_assigns_frequency = 0
-      assign_multiple_tasks_to(user)
+        assert_reminders_enqueued(1)
+      end
 
-      assert_reminders_enqueued(0)
-    end
+      it "does not enqueue a reminder if it's too soon" do
+        user.upsert_custom_fields(PendingAssignsReminder::REMINDED_AT => 1.days.ago)
+        assign_multiple_tasks_to(user)
 
-    it "does not enqueue reminders if the topic was just assigned to the user" do
-      just_assigned = DateTime.now
-      assign_multiple_tasks_to(user, assigned_on: just_assigned)
+        assert_reminders_enqueued(0)
+      end
 
-      assert_reminders_enqueued(0)
-    end
+      it "enqueues a reminder if the user was reminded more than a month ago" do
+        user.upsert_custom_fields(PendingAssignsReminder::REMINDED_AT => 31.days.ago)
+        assign_multiple_tasks_to(user)
 
-    it "enqueues a reminder when the user overrides the global frequency" do
-      SiteSetting.remind_assigns_frequency = 0
-      user.custom_fields.merge!(
-        PendingAssignsReminder::REMINDERS_FREQUENCY =>
-          RemindAssignsFrequencySiteSettings::DAILY_MINUTES,
-      )
-      user.save_custom_fields
+        assert_reminders_enqueued(1)
+      end
 
-      assign_multiple_tasks_to(user)
+      it "does not enqueue reminders if the remind frequency is set to never" do
+        SiteSetting.remind_assigns_frequency = 0
+        assign_multiple_tasks_to(user)
 
-      assert_reminders_enqueued(1)
-    end
+        assert_reminders_enqueued(0)
+      end
 
-    it "doesn't count assigns from deleted topics" do
-      deleted_post = Fabricate(:post)
-      assign_one_task_to(user, post: deleted_post)
-      (PendingAssignsReminder::REMINDER_THRESHOLD - 1).times { assign_one_task_to(user) }
+      it "does not enqueue reminders if the topic was just assigned to the user" do
+        just_assigned = DateTime.now
+        assign_multiple_tasks_to(user, assigned_on: just_assigned)
 
-      deleted_post.topic.trash!
+        assert_reminders_enqueued(0)
+      end
 
-      assert_reminders_enqueued(0)
+      it "enqueues a reminder when the user overrides the global frequency" do
+        SiteSetting.remind_assigns_frequency = 0
+        user.custom_fields.merge!(
+          PendingAssignsReminder::REMINDERS_FREQUENCY =>
+            RemindAssignsFrequencySiteSettings::DAILY_MINUTES,
+        )
+        user.save_custom_fields
+
+        assign_multiple_tasks_to(user)
+
+        assert_reminders_enqueued(1)
+      end
     end
 
     def assert_reminders_enqueued(expected_amount)