From f5cc748986a872bb245c5c15bbef5af249d655fb Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Thu, 10 Aug 2023 12:04:27 +0800 Subject: [PATCH] FIX: Fix reminder frequency not getting sent daily when early by a few seconds/minutes (#496) Add a buffer in determining whether to remind a user about assignments --- app/jobs/scheduled/enqueue_reminders.rb | 4 +- spec/jobs/scheduled/enqueue_reminders_spec.rb | 87 +++++++++++-------- 2 files changed, 54 insertions(+), 37 deletions(-) diff --git a/app/jobs/scheduled/enqueue_reminders.rb b/app/jobs/scheduled/enqueue_reminders.rb index c5cb59ed..13aea395 100644 --- 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 index 4733101d..5dec1080 100644 --- 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)