diff --git a/app/jobs/notification/remove_old_notification_job.rb b/app/jobs/notification/remove_old_notification_job.rb index 770461744..d7ec9d42f 100644 --- a/app/jobs/notification/remove_old_notification_job.rb +++ b/app/jobs/notification/remove_old_notification_job.rb @@ -1,8 +1,54 @@ class Notification::RemoveOldNotificationJob < ApplicationJob - queue_as :low + queue_as :purgable + + NOTIFICATION_LIMIT = 300 + OLD_NOTIFICATION_THRESHOLD = 1.month def perform - Notification.where('created_at < ?', 1.month.ago) - .find_each(batch_size: 1000, &:delete) + remove_old_notifications + trim_user_notifications + end + + private + + def remove_old_notifications + Notification.where('created_at < ?', OLD_NOTIFICATION_THRESHOLD.ago) + .delete_all + end + + def trim_user_notifications + # Find users with more than NOTIFICATION_LIMIT notifications + user_ids_exceeding_limit.each do |user_id| + trim_notifications_for_user(user_id) + end + end + + def user_ids_exceeding_limit + Notification.group(:user_id) + .having('COUNT(*) > ?', NOTIFICATION_LIMIT) + .pluck(:user_id) + end + + def trim_notifications_for_user(user_id) + # Find the cutoff notification (the 301st when we want to keep top 300) + # Order by created_at DESC, then id DESC for deterministic ordering + cutoff = Notification.where(user_id: user_id) + .order(created_at: :desc, id: :desc) + .offset(NOTIFICATION_LIMIT) + .limit(1) + .pick(:created_at, :id) + + return unless cutoff + + cutoff_time, cutoff_id = cutoff + + # Delete notifications older than cutoff, or same timestamp but lower/equal ID + # Since we order by id DESC, higher IDs are kept (come first), lower IDs deleted + # This avoids race conditions: notifications created after finding the cutoff + # will have timestamps > cutoff_time and won't be incorrectly deleted + Notification.where(user_id: user_id) + .where('created_at < ? OR (created_at = ? AND id <= ?)', + cutoff_time, cutoff_time, cutoff_id) + .delete_all end end diff --git a/app/jobs/trigger_scheduled_items_job.rb b/app/jobs/trigger_scheduled_items_job.rb index 15b0011c8..0ec368c54 100644 --- a/app/jobs/trigger_scheduled_items_job.rb +++ b/app/jobs/trigger_scheduled_items_job.rb @@ -19,9 +19,6 @@ class TriggerScheduledItemsJob < ApplicationJob # Job to sync whatsapp templates Channels::Whatsapp::TemplatesSyncSchedulerJob.perform_later - - # Job to clear notifications which are older than 1 month - Notification::RemoveOldNotificationJob.perform_later end end diff --git a/config/schedule.yml b/config/schedule.yml index c039719fa..cae9b94c8 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -59,3 +59,10 @@ periodic_assignment_job: cron: '*/30 * * * *' class: 'AutoAssignment::PeriodicAssignmentJob' queue: scheduled_jobs + +# executed daily at 2230 UTC +# removes old notifications (>1 month) and trims to 300 per user +remove_old_notification_job: + cron: '30 22 * * *' + class: 'Notification::RemoveOldNotificationJob' + queue: purgable diff --git a/spec/jobs/notification/remove_old_notification_job_spec.rb b/spec/jobs/notification/remove_old_notification_job_spec.rb index 1605d2da6..be3f8d035 100644 --- a/spec/jobs/notification/remove_old_notification_job_spec.rb +++ b/spec/jobs/notification/remove_old_notification_job_spec.rb @@ -1,23 +1,65 @@ require 'rails_helper' RSpec.describe Notification::RemoveOldNotificationJob do - let(:user) { create(:user) } - let(:conversation) { create(:conversation) } + let(:account) { create(:account) } + let(:user) { create(:user, account: account) } + let(:conversation) { create(:conversation, account: account) } it 'enqueues the job' do expect do described_class.perform_later end.to have_enqueued_job(described_class) - .on_queue('low') + .on_queue('purgable') end - it 'removes old notifications which are older than 1 month' do - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 2.months.ago) - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 1.month.ago) - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 1.day.ago) - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 1.hour.ago) + describe 'removing old notifications' do + it 'removes notifications older than 1 month' do + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 2.months.ago) + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 1.month.ago) + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 1.day.ago) + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 1.hour.ago) - described_class.perform_now - expect(Notification.count).to eq(2) + described_class.perform_now + expect(Notification.count).to eq(2) + end + end + + describe 'trimming user notifications' do + it 'does not delete notifications when user has fewer than 300' do + create_list(:notification, 50, user: user, account: account, primary_actor: conversation) + + expect { described_class.perform_now }.not_to(change(Notification, :count)) + end + + it 'trims to 300 notifications per user keeping the most recent' do + old_notifications = create_list(:notification, 50, user: user, account: account, primary_actor: conversation, + created_at: 2.days.ago) + recent_notifications = create_list(:notification, 300, user: user, account: account, primary_actor: conversation, + created_at: 1.hour.ago) + + described_class.perform_now + + expect(Notification.where(user_id: user.id).count).to eq(300) + expect(Notification.where(id: old_notifications.map(&:id))).to be_empty + expect(Notification.where(id: recent_notifications.map(&:id)).count).to eq(300) + end + end + + describe 'combined functionality' do + it 'removes old notifications and trims user notifications in one job' do + # User with old and excess notifications + create_list(:notification, 100, user: user, account: account, primary_actor: conversation, created_at: 2.months.ago) + create_list(:notification, 250, user: user, account: account, primary_actor: conversation, created_at: 1.day.ago) + + described_class.perform_now + + # All old notifications removed, remaining trimmed to 300 + expect(Notification.where(user_id: user.id).count).to eq(250) + expect(Notification.where('created_at < ?', 1.month.ago)).to be_empty + end end end diff --git a/spec/jobs/trigger_scheduled_items_job_spec.rb b/spec/jobs/trigger_scheduled_items_job_spec.rb index 4409e7ee0..6ccb41916 100644 --- a/spec/jobs/trigger_scheduled_items_job_spec.rb +++ b/spec/jobs/trigger_scheduled_items_job_spec.rb @@ -30,11 +30,6 @@ RSpec.describe TriggerScheduledItemsJob do described_class.perform_now end - it 'triggers Notification::RemoveOldNotificationJob' do - expect(Notification::RemoveOldNotificationJob).to receive(:perform_later).once - described_class.perform_now - end - context 'when unexecuted Scheduled campaign jobs' do let!(:twilio_sms) { create(:channel_twilio_sms, account: account) } let!(:twilio_inbox) { create(:inbox, channel: twilio_sms, account: account) }