diff --git a/app/models/notification.rb b/app/models/notification.rb index 4c0419f9e78..fc0e60805b0 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -5,7 +5,6 @@ class Notification < ActiveRecord::Base belongs_to :topic MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS = 24 - MEMBERSHIP_REQUEST_CONSOLIDATION_THRESHOLD = 3 validates_presence_of :data validates_presence_of :notification_type @@ -15,10 +14,25 @@ class Notification < ActiveRecord::Base scope :visible , lambda { joins('LEFT JOIN topics ON notifications.topic_id = topics.id') .where('topics.id IS NULL OR topics.deleted_at IS NULL') } - scope :filter_by_display_username_and_type, ->(username, notification_type) { - where("data::json ->> 'display_username' = ?", username) - .where(notification_type: notification_type) - .order(created_at: :desc) + scope :filter_by_consolidation_data, ->(notification_type, data) { + notifications = where(notification_type: notification_type) + + case notification_type + when types[:liked], types[:liked_consolidated] + key = "display_username" + consolidation_window = SiteSetting.likes_notification_consolidation_window_mins.minutes.ago + when types[:private_message] + key = "topic_title" + consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago + when types[:membership_request_consolidated] + key = "group_name" + consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago + end + + notifications = notifications.where("created_at > ? AND data::json ->> '#{key}' = ?", consolidation_window, data[key.to_sym]) if data[key&.to_sym].present? + notifications = notifications.where("data::json ->> 'username2' IS NULL") if notification_type == types[:liked] + + notifications } attr_accessor :skip_send_email @@ -27,7 +41,7 @@ class Notification < ActiveRecord::Base after_commit(on: :create) do DiscourseEvent.trigger(:notification_created, self) - send_email unless consolidate_membership_requests + send_email unless NotificationConsolidator.new(self).consolidate! end def self.ensure_consistency! @@ -230,70 +244,6 @@ class Notification < ActiveRecord::Base NotificationEmailer.process_notification(self) if !skip_send_email end - private - - def consolidate_membership_requests - return unless unread_pm? - - post_id = data_hash[:original_post_id] - return if post_id.blank? - - custom_field = PostCustomField.select(:value).find_by(post_id: post_id, name: "requested_group_id") - return if custom_field.blank? - - group_id = custom_field.value.to_i - group_name = Group.select(:name).find_by(id: group_id)&.name - return if group_name.blank? - - consolidation_window = MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours.ago - timestamp = Time.zone.now - unread = user.notifications.unread - - consolidated_notification = unread - .where("created_at > ? AND data::json ->> 'group_name' = ?", consolidation_window, group_name) - .find_by(notification_type: Notification.types[:membership_request_consolidated]) - - if consolidated_notification.present? - data = consolidated_notification.data_hash - data["count"] += 1 - - Notification.transaction do - consolidated_notification.update!( - data: data.to_json, - read: false, - updated_at: timestamp - ) - - destroy! - end - - return true - end - - notifications = unread - .where(notification_type: Notification.types[:private_message]) - .where("created_at > ? AND data::json ->> 'topic_title' = ?", consolidation_window, data_hash[:topic_title]) - - return if notifications.count < MEMBERSHIP_REQUEST_CONSOLIDATION_THRESHOLD - - Notification.transaction do - Notification.create!( - notification_type: Notification.types[:membership_request_consolidated], - user_id: user_id, - data: { - group_name: group_name, - count: notifications.count - }.to_json, - updated_at: timestamp, - created_at: timestamp - ) - - notifications.destroy_all - end - - true - end - end # == Schema Information diff --git a/app/services/notification_consolidator.rb b/app/services/notification_consolidator.rb new file mode 100644 index 00000000000..9ba3dc0ba47 --- /dev/null +++ b/app/services/notification_consolidator.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +class NotificationConsolidator + attr_reader :notification, :notification_type, :consolidation_type, :data + + def initialize(notification) + @notification = notification + @notification_type = notification.notification_type + @data = notification.data_hash + + if notification_type == Notification.types[:liked] + @consolidation_type = Notification.types[:liked_consolidated] + @data[:username] = @data[:display_username] + elsif notification_type == Notification.types[:private_message] + post_id = @data[:original_post_id] + return if post_id.blank? + + custom_field = PostCustomField.select(:value).find_by(post_id: post_id, name: "requested_group_id") + return if custom_field.blank? + + group_id = custom_field.value.to_i + group_name = Group.select(:name).find_by(id: group_id)&.name + return if group_name.blank? + + @consolidation_type = Notification.types[:membership_request_consolidated] + @data[:group_name] = group_name + end + end + + def consolidate! + return if SiteSetting.notification_consolidation_threshold.zero? || consolidation_type.blank? + + update_consolidated_notification! || create_consolidated_notification! + end + + def update_consolidated_notification! + consolidated_notification = user_notifications.filter_by_consolidation_data(consolidation_type, data).first + return if consolidated_notification.blank? + + data_hash = consolidated_notification.data_hash + data_hash["count"] += 1 + + Notification.transaction do + consolidated_notification.update!( + data: data_hash.to_json, + read: false, + updated_at: timestamp + ) + notification.destroy! + end + + consolidated_notification + end + + def create_consolidated_notification! + notifications = user_notifications.unread.filter_by_consolidation_data(notification_type, data) + return if notifications.count <= SiteSetting.notification_consolidation_threshold + + consolidated_notification = nil + + Notification.transaction do + timestamp = notifications.last.created_at + data[:count] = notifications.count + + consolidated_notification = Notification.create!( + notification_type: consolidation_type, + user_id: notification.user_id, + data: data.to_json, + updated_at: timestamp, + created_at: timestamp + ) + + notifications.destroy_all + end + + consolidated_notification + end + + private + + def user_notifications + notification.user.notifications + end + + def timestamp + @timestamp ||= Time.zone.now + end +end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 7194e0ec696..f769649e53a 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -331,28 +331,19 @@ class PostAlerter notification_data = {} - if is_liked - if existing_notification_of_same_type && - existing_notification_of_same_type.created_at > 1.day.ago && - ( - user.user_option.like_notification_frequency == - UserOption.like_notification_frequency_type[:always] - ) + if is_liked && + existing_notification_of_same_type && + existing_notification_of_same_type.created_at > 1.day.ago && + ( + user.user_option.like_notification_frequency == + UserOption.like_notification_frequency_type[:always] + ) - data = existing_notification_of_same_type.data_hash - notification_data["username2"] = data["display_username"] - notification_data["count"] = (data["count"] || 1).to_i + 1 - # don't use destroy so we don't trigger a notification count refresh - Notification.where(id: existing_notification_of_same_type.id).destroy_all - elsif !SiteSetting.likes_notification_consolidation_threshold.zero? - notification = consolidate_liked_notifications( - user, - post, - opts[:display_username] - ) - - return notification if notification - end + data = existing_notification_of_same_type.data_hash + notification_data["username2"] = data["display_username"] + notification_data["count"] = (data["count"] || 1).to_i + 1 + # don't use destroy so we don't trigger a notification count refresh + Notification.where(id: existing_notification_of_same_type.id).destroy_all end collapsed = false @@ -625,82 +616,4 @@ class PostAlerter def warn_if_not_sidekiq Rails.logger.warn("PostAlerter.#{caller_locations(1, 1)[0].label} was called outside of sidekiq") unless Sidekiq.server? end - - private - - def consolidate_liked_notifications(user, post, username) - user_notifications = user.notifications - - consolidation_window = - SiteSetting.likes_notification_consolidation_window_mins.minutes.ago - - liked_by_user_notifications = - user_notifications - .filter_by_display_username_and_type( - username, Notification.types[:liked] - ) - .where( - "created_at > ? AND data::json ->> 'username2' IS NULL", - consolidation_window - ) - - user_liked_consolidated_notification = - user_notifications - .filter_by_display_username_and_type( - username, Notification.types[:liked_consolidated] - ) - .where("created_at > ?", consolidation_window) - .first - - if user_liked_consolidated_notification - return update_consolidated_liked_notification_count!( - user_liked_consolidated_notification - ) - elsif ( - liked_by_user_notifications.count >= - SiteSetting.likes_notification_consolidation_threshold - ) - return create_consolidated_liked_notification!( - liked_by_user_notifications, - post, - username - ) - end - end - - def update_consolidated_liked_notification_count!(notification) - data = notification.data_hash - data["count"] += 1 - - notification.update!( - data: data.to_json, - read: false - ) - - notification - end - - def create_consolidated_liked_notification!(notifications, post, username) - notification = nil - - Notification.transaction do - timestamp = notifications.last.created_at - - notification = Notification.create!( - notification_type: Notification.types[:liked_consolidated], - user_id: post.user_id, - data: { - username: username, - display_username: username, - count: notifications.count + 1 - }.to_json, - updated_at: timestamp, - created_at: timestamp - ) - - notifications.each(&:destroy!) - end - - notification - end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 85dc5239576..1a89507cd53 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1988,9 +1988,9 @@ en: disable_system_edit_notifications: "Disables edit notifications by the system user when 'download_remote_images_to_local' is active." - likes_notification_consolidation_threshold: "Number of liked notifications received before the notifications are consolidated into a single one. Set to 0 to disable. The window can be configured via `SiteSetting.likes_notification_consolidation_window_mins`." + notification_consolidation_threshold: "Number of liked or membership request notifications received before the notifications are consolidated into a single one. Set to 0 to disable." - likes_notification_consolidation_window_mins: "Duration in minutes where liked notifications are consolidated into a single notification once the threshold has been reached. The threshold can be configured via `SiteSetting.likes_notification_consolidation_threshold`." + likes_notification_consolidation_window_mins: "Duration in minutes where liked notifications are consolidated into a single notification once the threshold has been reached. The threshold can be configured via `SiteSetting.notification_consolidation_threshold`." automatically_unpin_topics: "Automatically unpin topics when the user reaches the bottom." diff --git a/config/site_settings.yml b/config/site_settings.yml index 9d1f045f0c7..b4ef1349cb7 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1857,7 +1857,7 @@ uncategorized: disable_system_edit_notifications: true - likes_notification_consolidation_threshold: + notification_consolidation_threshold: default: 3 min: 0 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index a78d6e8ed6c..eabbbc68b5e 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -258,7 +258,7 @@ describe Notification do end end - describe '.filter_by_display_username_and_type' do + describe '.filter_by_consolidation_data' do let(:post) { Fabricate(:post) } fab!(:user) { Fabricate(:user) } @@ -267,8 +267,8 @@ describe Notification do end it 'should return the right notifications' do - expect(Notification.filter_by_display_username_and_type( - user.username_lower, Notification.types[:liked] + expect(Notification.filter_by_consolidation_data( + Notification.types[:liked], display_username: user.username_lower )).to eq([]) expect do @@ -280,8 +280,8 @@ describe Notification do PostActionCreator.like(user, post) end.to change { Notification.count }.by(2) - expect(Notification.filter_by_display_username_and_type( - user.username_lower, Notification.types[:liked] + expect(Notification.filter_by_consolidation_data( + Notification.types[:liked], display_username: user.username_lower )).to contain_exactly( Notification.find_by(notification_type: Notification.types[:liked]) ) @@ -376,7 +376,7 @@ describe Notification do before do PostCustomField.create!(post_id: post.id, name: "requested_group_id", value: group.id) - create_membership_request_notification + 2.times { create_membership_request_notification } end it 'should consolidate membership requests to a new notification' do @@ -391,12 +391,12 @@ describe Notification do data = notification.data_hash expect(data[:group_name]).to eq(group.name) - expect(data[:count]).to eq(3) + expect(data[:count]).to eq(4) notification = create_membership_request_notification expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect(Notification.last.data_hash[:count]).to eq(4) + expect(Notification.last.data_hash[:count]).to eq(5) end end end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 3413cb273a9..c4c85d985f1 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -263,13 +263,13 @@ describe PostAction do fab!(:likee) { Fabricate(:user) } it "can be disabled" do - SiteSetting.likes_notification_consolidation_threshold = 0 + SiteSetting.notification_consolidation_threshold = 0 expect do PostActionCreator.like(liker, Fabricate(:post, user: likee)) end.to change { likee.reload.notifications.count }.by(1) - SiteSetting.likes_notification_consolidation_threshold = 1 + SiteSetting.notification_consolidation_threshold = 1 expect do PostActionCreator.like(liker, Fabricate(:post, user: likee)) @@ -285,7 +285,7 @@ describe PostAction do end it 'should consolidate likes notification when the threshold is reached' do - SiteSetting.likes_notification_consolidation_threshold = 2 + SiteSetting.notification_consolidation_threshold = 2 expect do 3.times do @@ -353,7 +353,7 @@ describe PostAction do end it 'should consolidate liked notifications when threshold is reached' do - SiteSetting.likes_notification_consolidation_threshold = 2 + SiteSetting.notification_consolidation_threshold = 2 post = Fabricate(:post, user: likee)