DEV: Replace 'processed' column on notifications with new table (#11864)

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
 

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

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

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

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

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)

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

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

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

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

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

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