diff --git a/app/controllers/do_not_disturb_controller.rb b/app/controllers/do_not_disturb_controller.rb index 3f43246844d..db24c7167c6 100644 --- a/app/controllers/do_not_disturb_controller.rb +++ b/app/controllers/do_not_disturb_controller.rb @@ -25,9 +25,8 @@ class DoNotDisturbController < ApplicationController def destroy current_user.active_do_not_disturb_timings.destroy_all current_user.publish_do_not_disturb(ends_at: nil) - current_user.notifications.unprocessed.each do |notification| - NotificationEmailer.process_notification(notification, no_delay: true) - end + current_user.shelved_notifications.each(&:process) + current_user.shelved_notifications.destroy_all render json: success_json end diff --git a/app/jobs/scheduled/process_shelved_notifications.rb b/app/jobs/scheduled/process_shelved_notifications.rb index 38470a18d31..d734e51e814 100644 --- a/app/jobs/scheduled/process_shelved_notifications.rb +++ b/app/jobs/scheduled/process_shelved_notifications.rb @@ -6,22 +6,17 @@ module Jobs def execute(args) sql = <<~SQL - SELECT n.id FROM notifications AS n - INNER JOIN do_not_disturb_timings AS dndt ON n.user_id = dndt.user_id - WHERE n.processed = false + SELECT sn.id FROM shelved_notifications as sn + INNER JOIN notifications AS notification ON sn.notification_id = notification.id + INNER JOIN do_not_disturb_timings AS dndt ON notification.user_id = dndt.user_id AND dndt.ends_at <= :now SQL now = Time.zone.now - notification_ids = DB.query_single(sql, now: now) - - Notification.where(id: notification_ids).each do |notification| - begin - NotificationEmailer.process_notification(notification, no_delay: true) - rescue - Rails.logger.warn("Failed to process notification with ID #{notification.id}") - end - end + shelved_notification_ids = DB.query_single(sql, now: now) + shelved_notifications = ShelvedNotification.where(id: shelved_notification_ids) + shelved_notifications.each(&:process) + shelved_notifications.destroy_all DB.exec("DELETE FROM do_not_disturb_timings WHERE ends_at < :now", now: now) end diff --git a/app/models/notification.rb b/app/models/notification.rb index 9fbc6afc423..b5a07a32e21 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -4,13 +4,14 @@ class Notification < ActiveRecord::Base belongs_to :user belongs_to :topic + has_one :shelved_notification + MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS = 24 validates_presence_of :data validates_presence_of :notification_type scope :unread, lambda { where(read: false) } - scope :unprocessed, lambda { where(processed: false) } scope :recent, lambda { |n = nil| n ||= 10; order('notifications.created_at desc').limit(n) } scope :visible , lambda { joins('LEFT JOIN topics ON notifications.topic_id = topics.id') .where('topics.id IS NULL OR topics.deleted_at IS NULL') } @@ -283,10 +284,11 @@ class Notification < ActiveRecord::Base end def send_email - if skip_send_email - return update(processed: true) - end - NotificationEmailer.process_notification(self) unless user.do_not_disturb? + return if skip_send_email + + user.do_not_disturb? ? + ShelvedNotification.create(notification_id: self.id) : + NotificationEmailer.process_notification(self) end end diff --git a/app/models/shelved_notification.rb b/app/models/shelved_notification.rb new file mode 100644 index 00000000000..79e4bee2496 --- /dev/null +++ b/app/models/shelved_notification.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class ShelvedNotification < ActiveRecord::Base + belongs_to :notification + + def process + NotificationEmailer.process_notification(notification, no_delay: true) + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 8b6f58d13cb..315eac92d82 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1391,6 +1391,10 @@ class User < ActiveRecord::Base active_do_not_disturb_timings.maximum(:ends_at) end + def shelved_notifications + ShelvedNotification.joins(:notification).where("notifications.user_id = ?", self.id) + end + protected def badge_grant diff --git a/app/services/notification_emailer.rb b/app/services/notification_emailer.rb index a8b2406ca3f..21e31e5ebb7 100644 --- a/app/services/notification_emailer.rb +++ b/app/services/notification_emailer.rb @@ -125,7 +125,6 @@ class NotificationEmailer end def self.process_notification(notification, no_delay: false) - notification.update(processed: true) return if @disabled email_user = EmailUser.new(notification, no_delay: no_delay) diff --git a/db/migrate/20210105165605_add_processed_to_notifications.rb b/db/migrate/20210105165605_add_processed_to_notifications.rb index 8c560f2de7e..769d1275467 100644 --- a/db/migrate/20210105165605_add_processed_to_notifications.rb +++ b/db/migrate/20210105165605_add_processed_to_notifications.rb @@ -1,14 +1,6 @@ # frozen_string_literal: true class AddProcessedToNotifications < ActiveRecord::Migration[6.0] - def up - add_column :notifications, :processed, :boolean, default: false - execute "UPDATE notifications SET processed = true" - change_column_null(:notifications, :processed, false) - add_index :notifications, [:processed], unique: false - end - - def down - remove_column :notifications, :processed + def change end end diff --git a/db/migrate/20210126222142_create_shelved_notifications.rb b/db/migrate/20210126222142_create_shelved_notifications.rb new file mode 100644 index 00000000000..2af5b2e86cb --- /dev/null +++ b/db/migrate/20210126222142_create_shelved_notifications.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true +class CreateShelvedNotifications < ActiveRecord::Migration[6.0] + def change + create_table :shelved_notifications do |t| + t.integer :notification_id, null: false + end + add_index :shelved_notifications, [:notification_id] + end +end diff --git a/db/post_migrate/20210127140730_undo_add_processed_to_notifications.rb b/db/post_migrate/20210127140730_undo_add_processed_to_notifications.rb new file mode 100644 index 00000000000..2904760628b --- /dev/null +++ b/db/post_migrate/20210127140730_undo_add_processed_to_notifications.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class UndoAddProcessedToNotifications < ActiveRecord::Migration[6.0] + def up + execute "ALTER TABLE notifications DROP COLUMN IF EXISTS processed" + end +end diff --git a/spec/jobs/process_shelved_notifications_spec.rb b/spec/jobs/process_shelved_notifications_spec.rb index dd44c1ca6c0..eca634330c7 100644 --- a/spec/jobs/process_shelved_notifications_spec.rb +++ b/spec/jobs/process_shelved_notifications_spec.rb @@ -17,21 +17,21 @@ describe Jobs::ProcessShelvedNotifications do expect(DoNotDisturbTiming.find_by(id: past.id)).to eq(nil) end - it "does not process unprocessed notifications when the user is in DND" do + it "does not process shelved_notifications when the user is in DND" do user.do_not_disturb_timings.create(starts_at: 2.days.ago, ends_at: 2.days.from_now) notification = Notification.create(read: false, user_id: user.id, topic_id: 2, post_number: 1, data: '{}', notification_type: 1) - expect(notification.reload.processed).to eq(false) + expect(notification.shelved_notification).to be_present subject.execute({}) - expect(notification.reload.processed).to eq(false) + expect(notification.shelved_notification).to be_present end - it "processes unprocessed notifications when the user leaves DND" do + it "processes and destroys shelved_notifications when the user leaves DND" do user.do_not_disturb_timings.create(starts_at: 2.days.ago, ends_at: 2.days.from_now) notification = Notification.create(read: false, user_id: user.id, topic_id: 2, post_number: 1, data: '{}', notification_type: 1) user.do_not_disturb_timings.last.update(ends_at: 1.days.ago) - expect(notification.reload.processed).to eq(false) + expect(notification.shelved_notification).to be_present subject.execute({}) - expect(notification.reload.processed).to eq(true) + expect { notification.shelved_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) end end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index eab1cb4aa2e..138ca3d76e2 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -495,7 +495,6 @@ describe Notification do expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound) notification = Notification.last - expect(notification.processed).to eq(true) expect(notification.notification_type).to eq(Notification.types[:membership_request_consolidated]) data = notification.data_hash @@ -516,7 +515,7 @@ describe Notification do notification = Notification.last expect(notification.notification_type).to eq(Notification.types[:membership_request_consolidated]) - expect(notification.processed).to eq(false) + expect(notification.shelved_notification).to be_present end end end @@ -543,18 +542,18 @@ describe Notification do end end - describe "processed" do + describe "do not disturb" do fab!(:user) { Fabricate(:user) } - it "is false after creation when the user is in do not disturb" do + it "creates a shelved_notification record when created while user is in DND" do user.do_not_disturb_timings.create(starts_at: Time.now, ends_at: 3.days.from_now) notification = Notification.create(read: false, user_id: user.id, topic_id: 2, post_number: 1, data: '{}', notification_type: 1) - expect(notification.processed).to be(false) + expect(notification.shelved_notification).to be_present end - it "is true after creation when the user isn't in do not disturb" do + it "doesn't create a shelved_notification record when created while user is isn't DND" do notification = Notification.create(read: false, user_id: user.id, topic_id: 2, post_number: 1, data: '{}', notification_type: 1) - expect(notification.processed).to be(true) + expect(notification.shelved_notification).to be_nil end end end diff --git a/spec/requests/do_not_disturb_controller_spec.rb b/spec/requests/do_not_disturb_controller_spec.rb index 21c6f7a3741..1f3f65b3fe5 100644 --- a/spec/requests/do_not_disturb_controller_spec.rb +++ b/spec/requests/do_not_disturb_controller_spec.rb @@ -44,13 +44,14 @@ describe DoNotDisturbController do end describe "#destroy" do - it "process notifications that came in during DND" do + it "process shelved notifications that came in during DND" do user.do_not_disturb_timings.create(starts_at: 2.days.ago, ends_at: 2.days.from_now) notification = Notification.create(read: false, user_id: user.id, topic_id: 2, post_number: 1, data: '{}', notification_type: 1) - expect(notification.processed).to eq(false) + expect(notification.shelved_notification).to be_present delete "/do-not-disturb.json" - expect(notification.reload.processed).to eq(true) + expect { notification.shelved_notification.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(user.do_not_disturb?).to eq(false) end end end