FEATURE: Send notification emails when users leave do not disturb mode (#11643)

app/controllers/do_not_disturb_controller.rb

@@ -25,6 +25,9 @@ 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
     render json: success_json
   end
 

app/jobs/scheduled/process_shelved_notifications.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+module Jobs
+  class ProcessShelvedNotifications < ::Jobs::Scheduled
+    every 5.minutes
+
+    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
+        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
+
+      DB.exec("DELETE FROM do_not_disturb_timings WHERE ends_at < :now", now: now)
+    end
+  end
+end

app/models/notification.rb

@@ -10,6 +10,7 @@ class Notification < ActiveRecord::Base
   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') }
@@ -282,8 +283,10 @@ class Notification < ActiveRecord::Base
   end
 
   def send_email
-    return if skip_send_email || user.do_not_disturb? # TODO: 'shelve' emails rather than skipping them entirely
-    NotificationEmailer.process_notification(self)
+    if skip_send_email
+      return update(processed: true)
+    end
+    NotificationEmailer.process_notification(self) unless user.do_not_disturb?
   end
 
 end

app/models/user.rb

@@ -1379,6 +1379,10 @@ class User < ActiveRecord::Base
     do_not_disturb_timings.where('starts_at <= ? AND ends_at > ?', now, now)
   end
 
+  def do_not_disturb_until
+    active_do_not_disturb_timings.maximum(:ends_at)
+  end
+
   protected
 
   def badge_grant

app/serializers/current_user_serializer.rb

@@ -238,8 +238,4 @@ class CurrentUserSerializer < BasicUserSerializer
   def featured_topic
     object.user_profile.featured_topic
   end
-
-  def do_not_disturb_until
-    object.active_do_not_disturb_timings.maximum(:ends_at)
-  end
 end

app/services/notification_emailer.rb

@@ -3,10 +3,11 @@
 class NotificationEmailer
 
   class EmailUser
-    attr_reader :notification
+    attr_reader :notification, :no_delay
 
-    def initialize(notification)
+    def initialize(notification, no_delay: false)
       @notification = notification
+      @no_delay = no_delay
     end
 
     def group_mentioned
@@ -98,11 +99,11 @@ class NotificationEmailer
     end
 
     def default_delay
-      SiteSetting.email_time_window_mins.minutes
+      no_delay ? 0 : SiteSetting.email_time_window_mins.minutes
     end
 
     def private_delay
-      SiteSetting.personal_email_time_window_seconds
+      no_delay ? 0 : SiteSetting.personal_email_time_window_seconds
     end
 
     def post_type
@@ -123,10 +124,11 @@ class NotificationEmailer
     @disabled = false
   end
 
-  def self.process_notification(notification)
+  def self.process_notification(notification, no_delay: false)
+    notification.update(processed: true)
     return if @disabled
 
-    email_user   = EmailUser.new(notification)
+    email_user   = EmailUser.new(notification, no_delay: no_delay)
     email_method = Notification.types[notification.notification_type]
 
     email_user.public_send(email_method) if email_user.respond_to? email_method

db/migrate/20210105165605_add_processed_to_notifications.rb

@@ -0,0 +1,14 @@
+# 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
+  end
+end

spec/jobs/process_shelved_notifications_spec.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Jobs::ProcessShelvedNotifications do
+  fab!(:user) { Fabricate(:user) }
+  let(:post) { Fabricate(:post) }
+
+  it "removes all past do not disturb timings" do
+    future = Fabricate(:do_not_disturb_timing, ends_at: 1.day.from_now)
+    past = Fabricate(:do_not_disturb_timing, starts_at: 2.day.ago, ends_at: 1.minute.ago)
+
+    expect {
+      subject.execute({})
+    }.to change { DoNotDisturbTiming.count }.by (-1)
+    expect(DoNotDisturbTiming.find_by(id: future.id)).to eq(future)
+    expect(DoNotDisturbTiming.find_by(id: past.id)).to eq(nil)
+  end
+
+  it "does not process unprocessed 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)
+    subject.execute({})
+    expect(notification.reload.processed).to eq(false)
+  end
+
+  it "processes unprocessed 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)
+    subject.execute({})
+    expect(notification.reload.processed).to eq(true)
+  end
+end

spec/models/notification_spec.rb

@@ -495,6 +495,7 @@ 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
@@ -506,6 +507,17 @@ describe Notification do
 
         expect(Notification.last.data_hash[:count]).to eq(5)
       end
+
+      it 'consolidates membership requests with "processed" false if user is in DND' do
+        user.do_not_disturb_timings.create(starts_at: Time.now, ends_at: 3.days.from_now)
+
+        create_membership_request_notification
+        create_membership_request_notification
+
+        notification = Notification.last
+        expect(notification.notification_type).to eq(Notification.types[:membership_request_consolidated])
+        expect(notification.processed).to eq(false)
+      end
     end
   end
 
@@ -530,4 +542,19 @@ describe Notification do
       expect(Notification.where(user_id: user.id).pluck(:id)).to contain_exactly(notification4.id, notification3.id)
     end
   end
+
+  describe "processed" do
+    fab!(:user) { Fabricate(:user) }
+
+    it "is false after creation when the user is in do not disturb" 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)
+    end
+
+    it "is true after creation when the user isn't in do not disturb" 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)
+    end
+  end
 end

spec/requests/do_not_disturb_controller_spec.rb

@@ -42,5 +42,16 @@ describe DoNotDisturbController do
         expect(user.do_not_disturb_timings.last.ends_at.to_i).to eq(Time.new(2020, 11, 24, 23, 59, 59).utc.to_i)
       end
     end
+
+    describe "#destroy" do
+      it "process 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)
+        delete "/do-not-disturb.json"
+        expect(notification.reload.processed).to eq(true)
+      end
+    end
   end
 end

spec/services/notification_emailer_spec.rb

@@ -28,9 +28,9 @@ describe NotificationEmailer do
       expect_enqueued_with(
         job: :user_email,
         args: NotificationEmailer::EmailUser.notification_params(notification, type),
-        at: Time.zone.now + delay
+        at: no_delay ? Time.zone.now : Time.zone.now + delay
       ) do
-        NotificationEmailer.process_notification(notification)
+        NotificationEmailer.process_notification(notification, no_delay: no_delay)
       end
     end
 
@@ -39,7 +39,7 @@ describe NotificationEmailer do
 
       it "doesn't enqueue a job" do
         expect_not_enqueued_with(job: :user_email, args: { type: type }) do
-          NotificationEmailer.process_notification(notification)
+          NotificationEmailer.process_notification(notification, no_delay: no_delay)
         end
       end
 
@@ -51,15 +51,15 @@ describe NotificationEmailer do
             job: :user_email,
             args: { type: type }
           ) do
-            NotificationEmailer.process_notification(notification)
+            NotificationEmailer.process_notification(notification, no_delay: no_delay)
           end
         else
           expect_enqueued_with(
             job: :user_email,
             args: NotificationEmailer::EmailUser.notification_params(notification, type),
-            at: Time.zone.now + delay
+            at: no_delay ? Time.zone.now : Time.zone.now + delay
           ) do
-            NotificationEmailer.process_notification(notification)
+            NotificationEmailer.process_notification(notification, no_delay: no_delay)
           end
         end
       end
@@ -73,15 +73,15 @@ describe NotificationEmailer do
             job: :user_email,
             args: { type: type }
           ) do
-            NotificationEmailer.process_notification(notification)
+            NotificationEmailer.process_notification(notification, no_delay: no_delay)
           end
         else
           expect_enqueued_with(
             job: :user_email,
             args: NotificationEmailer::EmailUser.notification_params(notification, type),
-            at: Time.zone.now + delay
+            at: no_delay ? Time.zone.now : Time.zone.now + delay
           ) do
-            NotificationEmailer.process_notification(notification)
+            NotificationEmailer.process_notification(notification, no_delay: no_delay)
           end
         end
       end
@@ -96,7 +96,7 @@ describe NotificationEmailer do
 
       it "doesn't enqueue a job" do
         expect_not_enqueued_with(job: :user_email, args: { type: type }) do
-          NotificationEmailer.process_notification(notification)
+          NotificationEmailer.process_notification(notification, no_delay: no_delay)
         end
       end
     end
@@ -107,7 +107,7 @@ describe NotificationEmailer do
         Post.any_instance.expects(:post_type).returns(Post.types[:small_action])
 
         expect_not_enqueued_with(job: :user_email, args: { type: type }) do
-          NotificationEmailer.process_notification(notification)
+          NotificationEmailer.process_notification(notification, no_delay: no_delay)
         end
       end
 
@@ -122,7 +122,7 @@ describe NotificationEmailer do
       notification.user.user_option.update_columns(email_level: UserOption.email_level_types[:never])
 
       expect_not_enqueued_with(job: :user_email, args: { type: type }) do
-        NotificationEmailer.process_notification(notification)
+        NotificationEmailer.process_notification(notification, no_delay: no_delay)
       end
     end
   end
@@ -140,7 +140,10 @@ describe NotificationEmailer do
 
   end
 
+  [true, false].each do |no_delay|
+
     context 'user_mentioned' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_mentioned }
       let(:delay) { SiteSetting.email_time_window_mins.minutes }
       let!(:notification) { create_notification(:mentioned) }
@@ -162,6 +165,7 @@ describe NotificationEmailer do
     end
 
     context 'user_replied' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_replied }
       let(:delay) { SiteSetting.email_time_window_mins.minutes }
       let!(:notification) { create_notification(:replied) }
@@ -170,6 +174,7 @@ describe NotificationEmailer do
     end
 
     context 'user_quoted' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_quoted }
       let(:delay) { SiteSetting.email_time_window_mins.minutes }
       let!(:notification) { create_notification(:quoted) }
@@ -178,6 +183,7 @@ describe NotificationEmailer do
     end
 
     context 'user_linked' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_linked }
       let(:delay) { SiteSetting.email_time_window_mins.minutes }
       let!(:notification) { create_notification(:linked) }
@@ -186,6 +192,7 @@ describe NotificationEmailer do
     end
 
     context 'user_posted' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_posted }
       let(:delay) { SiteSetting.email_time_window_mins.minutes }
       let!(:notification) { create_notification(:posted) }
@@ -194,6 +201,7 @@ describe NotificationEmailer do
     end
 
     context 'user_private_message' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_private_message }
       let(:delay) { SiteSetting.personal_email_time_window_seconds }
       let!(:notification) { create_notification(:private_message) }
@@ -211,6 +219,7 @@ describe NotificationEmailer do
     end
 
     context 'user_invited_to_private_message' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_invited_to_private_message }
       let(:delay) { SiteSetting.personal_email_time_window_seconds }
       let!(:notification) { create_notification(:invited_to_private_message) }
@@ -219,6 +228,7 @@ describe NotificationEmailer do
     end
 
     context 'user_invited_to_topic' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_invited_to_topic }
       let(:delay) { SiteSetting.personal_email_time_window_seconds }
       let!(:notification) { create_notification(:invited_to_topic) }
@@ -227,11 +237,12 @@ describe NotificationEmailer do
     end
 
     context 'watching the first post' do
+      let(:no_delay) { no_delay }
       let(:type) { :user_watching_first_post }
       let(:delay) { SiteSetting.email_time_window_mins.minutes }
       let!(:notification) { create_notification(:watching_first_post) }
 
       include_examples "enqueue_public"
     end
-
+  end
 end