From 4601f3be7e0b7a8c642e8d7295195725060bf436 Mon Sep 17 00:00:00 2001 From: Mark VanLandingham Date: Thu, 7 Jan 2021 10:49:49 -0600 Subject: [PATCH] FEATURE: Send notification emails when users leave do not disturb mode (#11643) --- app/controllers/do_not_disturb_controller.rb | 3 + .../process_shelved_notifications.rb | 29 +++ app/models/notification.rb | 7 +- app/models/user.rb | 4 + app/serializers/current_user_serializer.rb | 4 - app/services/notification_emailer.rb | 14 +- ...05165605_add_processed_to_notifications.rb | 14 ++ .../process_shelved_notifications_spec.rb | 37 ++++ spec/models/notification_spec.rb | 27 +++ .../do_not_disturb_controller_spec.rb | 11 + spec/services/notification_emailer_spec.rb | 203 +++++++++--------- 11 files changed, 245 insertions(+), 108 deletions(-) create mode 100644 app/jobs/scheduled/process_shelved_notifications.rb create mode 100644 db/migrate/20210105165605_add_processed_to_notifications.rb create mode 100644 spec/jobs/process_shelved_notifications_spec.rb diff --git a/app/controllers/do_not_disturb_controller.rb b/app/controllers/do_not_disturb_controller.rb index cd22eb033f5..3f43246844d 100644 --- a/app/controllers/do_not_disturb_controller.rb +++ b/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 diff --git a/app/jobs/scheduled/process_shelved_notifications.rb b/app/jobs/scheduled/process_shelved_notifications.rb new file mode 100644 index 00000000000..38470a18d31 --- /dev/null +++ b/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 diff --git a/app/models/notification.rb b/app/models/notification.rb index 3fd94b7335c..9fbc6afc423 100644 --- a/app/models/notification.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 279d11a4fbf..6b26110a32b 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 4b7c1bcf6fe..3ec597a7db6 100644 --- a/app/serializers/current_user_serializer.rb +++ b/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 diff --git a/app/services/notification_emailer.rb b/app/services/notification_emailer.rb index db51246e3db..a8b2406ca3f 100644 --- a/app/services/notification_emailer.rb +++ b/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 diff --git a/db/migrate/20210105165605_add_processed_to_notifications.rb b/db/migrate/20210105165605_add_processed_to_notifications.rb new file mode 100644 index 00000000000..8c560f2de7e --- /dev/null +++ b/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 diff --git a/spec/jobs/process_shelved_notifications_spec.rb b/spec/jobs/process_shelved_notifications_spec.rb new file mode 100644 index 00000000000..dd44c1ca6c0 --- /dev/null +++ b/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 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index e87bff865c4..eab1cb4aa2e 100644 --- a/spec/models/notification_spec.rb +++ b/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 diff --git a/spec/requests/do_not_disturb_controller_spec.rb b/spec/requests/do_not_disturb_controller_spec.rb index 9893daafb0c..21c6f7a3741 100644 --- a/spec/requests/do_not_disturb_controller_spec.rb +++ b/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 diff --git a/spec/services/notification_emailer_spec.rb b/spec/services/notification_emailer_spec.rb index c986f725185..69f622172f3 100644 --- a/spec/services/notification_emailer_spec.rb +++ b/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,98 +140,109 @@ describe NotificationEmailer do end - context 'user_mentioned' do - let(:type) { :user_mentioned } - let(:delay) { SiteSetting.email_time_window_mins.minutes } - let!(:notification) { create_notification(:mentioned) } + [true, false].each do |no_delay| - include_examples "enqueue_public" + 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) } - it "enqueue a delayed job for users that are online" do - notification.user.last_seen_at = 1.minute.ago + include_examples "enqueue_public" - expect_enqueued_with( - job: :user_email, - args: NotificationEmailer::EmailUser.notification_params(notification, type), - at: Time.zone.now + delay - ) do - NotificationEmailer.process_notification(notification) + it "enqueue a delayed job for users that are online" do + notification.user.last_seen_at = 1.minute.ago + + expect_enqueued_with( + job: :user_email, + args: NotificationEmailer::EmailUser.notification_params(notification, type), + at: Time.zone.now + delay + ) do + NotificationEmailer.process_notification(notification) + end end + end - 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) } - context 'user_replied' do - let(:type) { :user_replied } - let(:delay) { SiteSetting.email_time_window_mins.minutes } - let!(:notification) { create_notification(:replied) } - - include_examples "enqueue_public" - end - - context 'user_quoted' do - let(:type) { :user_quoted } - let(:delay) { SiteSetting.email_time_window_mins.minutes } - let!(:notification) { create_notification(:quoted) } - - include_examples "enqueue_public" - end - - context 'user_linked' do - let(:type) { :user_linked } - let(:delay) { SiteSetting.email_time_window_mins.minutes } - let!(:notification) { create_notification(:linked) } - - include_examples "enqueue_public" - end - - context 'user_posted' do - let(:type) { :user_posted } - let(:delay) { SiteSetting.email_time_window_mins.minutes } - let!(:notification) { create_notification(:posted) } - - include_examples "enqueue_public" - end - - context 'user_private_message' do - let(:type) { :user_private_message } - let(:delay) { SiteSetting.personal_email_time_window_seconds } - let!(:notification) { create_notification(:private_message) } - - include_examples "enqueue_private" - - it "doesn't enqueue a job for a small action" do - notification.data_hash["original_post_type"] = Post.types[:small_action] - - expect_not_enqueued_with(job: :user_email, args: { type: type }) do - NotificationEmailer.process_notification(notification) - end + include_examples "enqueue_public" 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) } + + include_examples "enqueue_public" + 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) } + + include_examples "enqueue_public" + 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) } + + include_examples "enqueue_public" + 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) } + + include_examples "enqueue_private" + + it "doesn't enqueue a job for a small action" do + notification.data_hash["original_post_type"] = Post.types[:small_action] + + expect_not_enqueued_with(job: :user_email, args: { type: type }) do + NotificationEmailer.process_notification(notification) + end + end + + 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) } + + include_examples "enqueue_public" + 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) } + + include_examples "enqueue_public" + 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 - - context 'user_invited_to_private_message' do - let(:type) { :user_invited_to_private_message } - let(:delay) { SiteSetting.personal_email_time_window_seconds } - let!(:notification) { create_notification(:invited_to_private_message) } - - include_examples "enqueue_public" - end - - context 'user_invited_to_topic' do - let(:type) { :user_invited_to_topic } - let(:delay) { SiteSetting.personal_email_time_window_seconds } - let!(:notification) { create_notification(:invited_to_topic) } - - include_examples "enqueue_public" - end - - context 'watching the first post' do - 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