From 1fc06520bd7548665d5ab94d57073c3ae1b79394 Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Tue, 30 Nov 2021 13:36:14 -0300
Subject: [PATCH] REFACTOR: Improve support for consolidating notifications.
(#14904)
* REFACTOR: Improve support for consolidating notifications.
Before this commit, we didn't have a single way of consolidating notifications. For notifications like group summaries, we manually removed old ones before creating a new one. On the other hand, we used an after_create callback for likes and group membership requests, which caused unnecessary work, as we need to delete the record we created to replace it with a consolidated one.
We now have all the consolidation rules centralized in a single place: the consolidation planner class. Other parts of the app looking to create a consolidable notification can do so by calling Notification#consolidate_or_save!, instead of the default Notification#create! method.
Finally, we added two more rules: one for re-using existing group summaries and another for deleting duplicated dashboard problems PMs notifications when the user is tracking the moderator's inbox. Setting the threshold to one forces the planner to apply this rule every time.
I plan to add plugin support for adding custom rules in another PR to keep this one relatively small.
* DEV: Introduces a plugin API for consolidating notifications.
This commit removes the `Notification#filter_by_consolidation_data` scope since plugins could have to define their criteria. The Plan class now receives two blocks, one to query for an already consolidated notification, which we'll try to update, and another to query for existing ones to consolidate.
It also receives a consolidation window, which accepts an ActiveSupport::Duration object, and filter notifications created since that value.
---
app/controllers/notifications_controller.rb | 2 +-
app/models/notification.rb | 32 ++--
app/services/notification_consolidator.rb | 88 ----------
.../consolidate_notifications.rb | 152 ++++++++++++++++++
.../notifications/consolidation_planner.rb | 105 ++++++++++++
app/services/post_alerter.rb | 12 +-
lib/discourse_plugin_registry.rb | 2 +
lib/plugin/instance.rb | 17 ++
spec/components/plugin/instance_spec.rb | 54 +++++++
spec/jobs/dashboard_stats_spec.rb | 17 ++
spec/models/notification_spec.rb | 52 ++----
.../consolidation_planner_spec.rb | 57 +++++++
spec/services/post_alerter_spec.rb | 27 ++++
13 files changed, 455 insertions(+), 162 deletions(-)
delete mode 100644 app/services/notification_consolidator.rb
create mode 100644 app/services/notifications/consolidate_notifications.rb
create mode 100644 app/services/notifications/consolidation_planner.rb
create mode 100644 spec/services/notifications/consolidation_planner_spec.rb
diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index baa9aca3a43..d89917b4f18 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -73,7 +73,7 @@ class NotificationsController < ApplicationController
end
def create
- @notification = Notification.create!(notification_params)
+ @notification = Notification.consolidate_or_create!(notification_params)
render_notification
end
diff --git a/app/models/notification.rb b/app/models/notification.rb
index bf91b350c2b..2b8fd37eb00 100644
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -16,34 +16,13 @@ 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_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
after_commit :refresh_notification_count, on: [:create, :update, :destroy]
+ after_commit :send_email, on: :create
after_commit(on: :create) do
DiscourseEvent.trigger(:notification_created, self)
- send_email unless NotificationConsolidator.new(self).consolidate!
end
before_create do
@@ -52,6 +31,15 @@ class Notification < ActiveRecord::Base
self.high_priority = self.high_priority || Notification.high_priority_types.include?(self.notification_type)
end
+ def self.consolidate_or_create!(notification_params)
+ notification = new(notification_params)
+ consolidation_planner = Notifications::ConsolidationPlanner.new
+
+ consolidated_notification = consolidation_planner.consolidate_or_save!(notification)
+
+ consolidated_notification == :no_plan ? notification.tap(&:save!) : consolidated_notification
+ end
+
def self.purge_old!
return if SiteSetting.max_notifications_per_user == 0
diff --git a/app/services/notification_consolidator.rb b/app/services/notification_consolidator.rb
deleted file mode 100644
index 9ba3dc0ba47..00000000000
--- a/app/services/notification_consolidator.rb
+++ /dev/null
@@ -1,88 +0,0 @@
-# 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/notifications/consolidate_notifications.rb b/app/services/notifications/consolidate_notifications.rb
new file mode 100644
index 00000000000..83d5f7e901d
--- /dev/null
+++ b/app/services/notifications/consolidate_notifications.rb
@@ -0,0 +1,152 @@
+# frozen_string_literal: true
+
+# Represents a rule to consolidate a specific notification.
+#
+# If a consolidated notification already exists, we'll update it instead.
+# If it doesn't and creating a new one would match the threshold, we delete existing ones and create a consolidated one.
+# Otherwise, save the original one.
+#
+# Constructor arguments:
+#
+# - from: The notification type of the unconsolidated notification. e.g. `Notification.types[:private_message]`
+# - to: The type the consolidated notification will have. You can use the same value as from to flatten notifications or bump existing ones.
+# - threshold: If creating a new notification would match this number, we'll destroy existing ones and create a consolidated one. It also accepts a lambda that returns a number.
+# - consolidation_window: Only consolidate notifications created since this value (Pass a ActiveSupport::Duration instance, and we'll call #ago on it).
+# - unconsolidated_query_blk: A block with additional queries to apply when fetching for unconsolidated notifications.
+# - consolidated_query_blk: A block with additional queries to apply when fetching for a consolidated notification.
+#
+# Need to call #set_precondition to configure this:
+#
+# - precondition_blk: A block that receives the mutated data and returns true if we have everything we need to consolidate.
+#
+# Need to call #set_mutations to configure this:
+#
+# - set_data_blk: A block that receives the notification data hash and mutates it, adding additional data needed for consolidation.
+
+module Notifications
+ class ConsolidateNotifications
+ def initialize(from:, to:, consolidation_window: nil, unconsolidated_query_blk: nil, consolidated_query_blk: nil, threshold:)
+ @from = from
+ @to = to
+ @threshold = threshold
+ @consolidation_window = consolidation_window
+ @consolidated_query_blk = consolidated_query_blk
+ @unconsolidated_query_blk = unconsolidated_query_blk
+ @precondition_blk = nil
+ @set_data_blk = nil
+ end
+
+ def set_precondition(precondition_blk: nil)
+ @precondition_blk = precondition_blk
+
+ self
+ end
+
+ def set_mutations(set_data_blk: nil)
+ @set_data_blk = set_data_blk
+
+ self
+ end
+
+ def can_consolidate_data?(notification)
+ return false if get_threshold.zero? || to.blank?
+ return false if notification.notification_type != from
+
+ @data = consolidated_data(notification)
+
+ return true if @precondition_blk.nil?
+ @precondition_blk.call(data)
+ end
+
+ def consolidate_or_save!(notification)
+ @data ||= consolidated_data(notification)
+ return unless can_consolidate_data?(notification)
+
+ update_consolidated_notification!(notification) ||
+ create_consolidated_notification!(notification) ||
+ notification.tap(&:save!)
+ end
+
+ private
+
+ attr_reader :notification, :from, :to, :data, :threshold, :consolidated_query_blk, :unconsolidated_query_blk, :consolidation_window
+
+ def consolidated_data(notification)
+ return notification.data_hash if @set_data_blk.nil?
+ @set_data_blk.call(notification)
+ end
+
+ def update_consolidated_notification!(notification)
+ notifications = user_notifications(notification, to)
+
+ if consolidated_query_blk.present?
+ notifications = consolidated_query_blk.call(notifications, data)
+ end
+ consolidated = notifications.first
+ return if consolidated.blank?
+
+ data_hash = consolidated.data_hash.merge(data)
+ data_hash[:count] += 1 if data_hash[:count].present?
+
+ # Hack: We don't want to cache the old data if we're about to update it.
+ consolidated.instance_variable_set(:@data_hash, nil)
+
+ consolidated.update!(
+ data: data_hash.to_json,
+ read: false,
+ updated_at: timestamp
+ )
+
+ consolidated
+ end
+
+ def create_consolidated_notification!(notification)
+ notifications = user_notifications(notification, from)
+ if unconsolidated_query_blk.present?
+ notifications = unconsolidated_query_blk.call(notifications, data)
+ end
+
+ # Saving the new notification would pass the threshold? Consolidate instead.
+ count_after_saving_notification = notifications.count + 1
+ return if count_after_saving_notification <= get_threshold
+
+ timestamp = notifications.last.created_at
+ data[:count] = count_after_saving_notification
+
+ consolidated = nil
+
+ Notification.transaction do
+ notifications.destroy_all
+
+ consolidated = Notification.create!(
+ notification_type: to,
+ user_id: notification.user_id,
+ data: data.to_json,
+ updated_at: timestamp,
+ created_at: timestamp
+ )
+ end
+
+ consolidated
+ end
+
+ def get_threshold
+ threshold.is_a?(Proc) ? threshold.call : threshold
+ end
+
+ def user_notifications(notification, type)
+ notifications = notification.user.notifications
+ .where(notification_type: type)
+
+ if consolidation_window.present?
+ notifications = notifications.where('created_at > ?', consolidation_window.ago)
+ end
+
+ notifications
+ end
+
+ def timestamp
+ @timestamp ||= Time.zone.now
+ end
+ end
+end
diff --git a/app/services/notifications/consolidation_planner.rb b/app/services/notifications/consolidation_planner.rb
new file mode 100644
index 00000000000..684dcb80e6f
--- /dev/null
+++ b/app/services/notifications/consolidation_planner.rb
@@ -0,0 +1,105 @@
+# frozen_string_literal: true
+
+module Notifications
+ class ConsolidationPlanner
+ def consolidate_or_save!(notification)
+ plan = plan_for(notification)
+ return :no_plan if plan.nil?
+
+ plan.consolidate_or_save!(notification)
+ end
+
+ private
+
+ def plan_for(notification)
+ consolidation_plans = [liked, dashboard_problems_pm, group_message_summary, group_membership]
+ consolidation_plans.concat(DiscoursePluginRegistry.notification_consolidation_plans)
+
+ consolidation_plans.detect { |plan| plan.can_consolidate_data?(notification) }
+ end
+
+ def liked
+ ConsolidateNotifications.new(
+ from: Notification.types[:liked],
+ to: Notification.types[:liked_consolidated],
+ threshold: -> { SiteSetting.notification_consolidation_threshold },
+ consolidation_window: SiteSetting.likes_notification_consolidation_window_mins.minutes,
+ unconsolidated_query_blk: ->(notifications, data) do
+ key = 'display_username'
+ value = data[key.to_sym]
+ filtered = notifications.where("data::json ->> 'username2' IS NULL")
+
+ filtered = filtered.where("data::json ->> '#{key}' = ?", value) if value
+
+ filtered
+ end,
+ consolidated_query_blk: filtered_by_data_attribute('display_username')
+ ).set_mutations(
+ set_data_blk: ->(notification) do
+ data = notification.data_hash
+ data.merge(username: data[:display_username])
+ end
+ )
+ end
+
+ def group_membership
+ ConsolidateNotifications.new(
+ from: Notification.types[:private_message],
+ to: Notification.types[:membership_request_consolidated],
+ threshold: -> { SiteSetting.notification_consolidation_threshold },
+ consolidation_window: Notification::MEMBERSHIP_REQUEST_CONSOLIDATION_WINDOW_HOURS.hours,
+ unconsolidated_query_blk: filtered_by_data_attribute('topic_title'),
+ consolidated_query_blk: filtered_by_data_attribute('group_name')
+ ).set_precondition(
+ precondition_blk: ->(data) { data[:group_name].present? }
+ ).set_mutations(
+ set_data_blk: ->(notification) do
+ data = notification.data_hash
+ post_id = data[:original_post_id]
+ custom_field = PostCustomField.select(:value).find_by(post_id: post_id, name: "requested_group_id")
+ group_id = custom_field&.value
+ group_name = group_id.present? ? Group.select(:name).find_by(id: group_id.to_i)&.name : nil
+
+ data[:group_name] = group_name
+ data
+ end
+ )
+ end
+
+ def group_message_summary
+ ConsolidateNotifications.new(
+ from: Notification.types[:group_message_summary],
+ to: Notification.types[:group_message_summary],
+ unconsolidated_query_blk: filtered_by_data_attribute('group_id'),
+ consolidated_query_blk: filtered_by_data_attribute('group_id'),
+ threshold: 1 # We should always apply this plan to refresh the summary stats
+ ).set_precondition(
+ precondition_blk: ->(data) { data[:group_id].present? }
+ )
+ end
+
+ def dashboard_problems_pm
+ ConsolidateNotifications.new(
+ from: Notification.types[:private_message],
+ to: Notification.types[:private_message],
+ threshold: 1,
+ unconsolidated_query_blk: filtered_by_data_attribute('topic_title'),
+ consolidated_query_blk: filtered_by_data_attribute('topic_title')
+ ).set_precondition(
+ precondition_blk: ->(data) do
+ data[:topic_title] == I18n.t("system_messages.dashboard_problems.subject_template")
+ end
+ )
+ end
+
+ def filtered_by_data_attribute(attribute_name)
+ ->(notifications, data) do
+ if (value = data[attribute_name.to_sym])
+ notifications.where("data::json ->> '#{attribute_name}' = ?", value.to_s)
+ else
+ notifications
+ end
+ end
+ end
+ end
+end
diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 31efae476c2..d311391abaa 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -327,15 +327,9 @@ class PostAlerter
stat = stats.find { |s| s[:group_id] == group_id }
return unless stat && stat[:inbox_count] > 0
- notification_type = Notification.types[:group_message_summary]
-
DistributedMutex.synchronize("group_message_notify_#{user.id}") do
- Notification.where(notification_type: notification_type, user_id: user.id).each do |n|
- n.destroy if n.data_hash[:group_id] == stat[:group_id]
- end
-
- Notification.create(
- notification_type: notification_type,
+ Notification.consolidate_or_create!(
+ notification_type: Notification.types[:group_message_summary],
user_id: user.id,
data: {
group_id: stat[:group_id],
@@ -509,7 +503,7 @@ class PostAlerter
end
# Create the notification
- created = user.notifications.create!(
+ created = user.notifications.consolidate_or_create!(
notification_type: type,
topic_id: post.topic_id,
post_number: post.post_number,
diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb
index 53a05aa4b49..10642a73e57 100644
--- a/lib/discourse_plugin_registry.rb
+++ b/lib/discourse_plugin_registry.rb
@@ -93,6 +93,8 @@ class DiscoursePluginRegistry
define_filtered_register :push_notification_filters
+ define_filtered_register :notification_consolidation_plans
+
def self.register_auth_provider(auth_provider)
self.auth_providers << auth_provider
end
diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb
index e59a6fbc05a..d24e49e3749 100644
--- a/lib/plugin/instance.rb
+++ b/lib/plugin/instance.rb
@@ -976,6 +976,23 @@ class Plugin::Instance
DiscoursePluginRegistry.register_reviewable_score_link({ reason: reason.to_sym, setting: setting_name }, self)
end
+ # If your plugin creates notifications, and you'd like to consolidate/collapse similar ones,
+ # you're in the right place.
+ # This method receives a plan object, which must be an instance of `Notifications::ConsolidateNotifications`.
+ #
+ # Instead of using `Notification#create!`, you should use `Notification#consolidate_or_save!`,
+ # which will automatically pick your plan and apply it, updating an already consolidated notification,
+ # consolidating multiple ones, or creating a regular one.
+ #
+ # The rule object is quite complex. We strongly recommend you write tests to ensure your plugin consolidates notifications correctly.
+ #
+ # - Plan's documentation: https://github.com/discourse/discourse/blob/main/app/services/notifications/consolidate_notifications.rb
+ # - Base plans: https://github.com/discourse/discourse/blob/main/app/services/notifications/consolidation_planner.rb
+ def register_notification_consolidation_plan(plan)
+ raise ArgumentError.new("Not a consolidation plan") if plan.class != Notifications::ConsolidateNotifications
+ DiscoursePluginRegistry.register_notification_consolidation_plan(plan, self)
+ end
+
protected
def self.js_path
diff --git a/spec/components/plugin/instance_spec.rb b/spec/components/plugin/instance_spec.rb
index bdd1c835da3..506ac7e09ed 100644
--- a/spec/components/plugin/instance_spec.rb
+++ b/spec/components/plugin/instance_spec.rb
@@ -668,4 +668,58 @@ describe Plugin::Instance do
Site.categories_callbacks.clear
end
end
+
+ describe '#register_notification_consolidation_plan' do
+ let(:plugin) { Plugin::Instance.new }
+ fab!(:topic) { Fabricate(:topic) }
+
+ after do
+ DiscoursePluginRegistry.reset_register!(:notification_consolidation_plans)
+ end
+
+ it 'fails when the received object is not a consolidation plan' do
+ expect { plugin.register_notification_consolidation_plan(Object.new) }.to raise_error(ArgumentError)
+ end
+
+ it 'registers a consolidation plan and uses it' do
+ plan = Notifications::ConsolidateNotifications.new(
+ from: Notification.types[:code_review_commit_approved],
+ to: Notification.types[:code_review_commit_approved],
+ threshold: 1,
+ consolidation_window: 1.minute,
+ unconsolidated_query_blk: ->(notifications, _data) do
+ notifications.where("(data::json ->> 'consolidated') IS NULL")
+ end,
+ consolidated_query_blk: ->(notifications, _data) do
+ notifications.where("(data::json ->> 'consolidated') IS NOT NULL")
+ end
+ ).set_mutations(
+ set_data_blk: ->(notification) do
+ notification.data_hash.merge(consolidated: true)
+ end
+ )
+
+ plugin.register_notification_consolidation_plan(plan)
+
+ create_notification!
+ create_notification!
+
+ expect(commit_approved_notifications.count).to eq(1)
+ consolidated_notification = commit_approved_notifications.last
+ expect(consolidated_notification.data_hash[:consolidated]).to eq(true)
+ end
+
+ def commit_approved_notifications
+ Notification.where(user: topic.user, notification_type: Notification.types[:code_review_commit_approved])
+ end
+
+ def create_notification!
+ Notification.consolidate_or_create!(
+ notification_type: Notification.types[:code_review_commit_approved],
+ topic_id: topic.id,
+ user: topic.user,
+ data: {}
+ )
+ end
+ end
end
diff --git a/spec/jobs/dashboard_stats_spec.rb b/spec/jobs/dashboard_stats_spec.rb
index e4b2fdafc38..3ffb3300eae 100644
--- a/spec/jobs/dashboard_stats_spec.rb
+++ b/spec/jobs/dashboard_stats_spec.rb
@@ -33,6 +33,23 @@ describe ::Jobs::DashboardStats do
expect(new_topic.title).to eq(old_topic.title)
end
+ it 'consolidates notifications when not tracking admins group' do
+ Discourse.redis.setex(AdminDashboardData.problems_started_key, 14.days.to_i, 3.days.ago)
+ Jobs.run_immediately!
+
+ admin = Fabricate(:admin)
+ Group[:admins].add(admin)
+
+ described_class.new.execute({})
+ clear_recently_sent!
+ new_topic = described_class.new.execute({}).topic
+ notifications = Notification.where(user: admin, notification_type: Notification.types[:private_message])
+
+ expect(notifications.count).to eq(1)
+ from_topic_id = Post.select(:topic_id).find_by(id: notifications.last.data_hash[:original_post_id]).topic_id
+ expect(from_topic_id).to eq(new_topic.id)
+ end
+
it 'duplicates message if previous one has replies' do
Discourse.redis.setex(AdminDashboardData.problems_started_key, 14.days.to_i, 3.days.ago)
expect { described_class.new.execute({}) }.to change { Topic.count }.by(1)
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 2e91a5e7286..a99cde5c04b 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -355,36 +355,6 @@ describe Notification do
end
end
- describe '.filter_by_consolidation_data' do
- let(:post) { Fabricate(:post) }
- fab!(:user) { Fabricate(:user) }
-
- before do
- PostActionNotifier.enable
- end
-
- it 'should return the right notifications' do
- expect(Notification.filter_by_consolidation_data(
- Notification.types[:liked], display_username: user.username_lower
- )).to eq([])
-
- expect do
- PostAlerter.post_created(Fabricate(:basic_reply,
- user: user,
- topic: post.topic
- ))
-
- PostActionCreator.like(user, post)
- end.to change { Notification.count }.by(2)
-
- 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])
- )
- end
- end
-
describe "do not disturb" do
it "calls NotificationEmailer.process_notification when user is not in 'do not disturb'" do
user = Fabricate(:user)
@@ -482,7 +452,7 @@ describe Notification do
fab!(:post) { Fabricate(:post) }
def create_membership_request_notification
- Notification.create(
+ Notification.consolidate_or_create!(
notification_type: Notification.types[:private_message],
user_id: user.id,
data: {
@@ -500,23 +470,21 @@ describe Notification do
end
it 'should consolidate membership requests to a new notification' do
- notification = create_membership_request_notification
- notification.reload
+ original_notification = create_membership_request_notification
+ starting_count = SiteSetting.notification_consolidation_threshold
- notification = create_membership_request_notification
- expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
+ consolidated_notification = create_membership_request_notification
+ expect { original_notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
- notification = Notification.last
- expect(notification.notification_type).to eq(Notification.types[:membership_request_consolidated])
+ expect(consolidated_notification.notification_type).to eq(Notification.types[:membership_request_consolidated])
- data = notification.data_hash
+ data = consolidated_notification.data_hash
expect(data[:group_name]).to eq(group.name)
- expect(data[:count]).to eq(4)
+ expect(data[:count]).to eq(starting_count + 1)
- notification = create_membership_request_notification
- expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
+ updated_consolidated_notification = create_membership_request_notification
- expect(Notification.last.data_hash[:count]).to eq(5)
+ expect(updated_consolidated_notification.data_hash[:count]).to eq(starting_count + 2)
end
it 'consolidates membership requests with "processed" false if user is in DND' do
diff --git a/spec/services/notifications/consolidation_planner_spec.rb b/spec/services/notifications/consolidation_planner_spec.rb
new file mode 100644
index 00000000000..4597c1e5fbd
--- /dev/null
+++ b/spec/services/notifications/consolidation_planner_spec.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Notifications::ConsolidationPlanner do
+ describe '#consolidate_or_save!' do
+ let(:threshold) { 1 }
+ fab!(:user) { Fabricate(:user) }
+ let(:like_user) { 'user1' }
+
+ before { SiteSetting.notification_consolidation_threshold = threshold }
+
+ it "does nothing it haven't passed the consolidation threshold yet" do
+ notification = build_notification(:liked, { display_username: like_user })
+
+ saved_like = subject.consolidate_or_save!(notification)
+
+ expect(saved_like.id).to be_present
+ expect(saved_like.notification_type).to eq(Notification.types[:liked])
+ end
+
+ it 'consolidates multiple notifications into a new one' do
+ first_notification = Fabricate(:notification, user: user, notification_type: Notification.types[:liked], data: { display_username: like_user }.to_json)
+ notification = build_notification(:liked, { display_username: like_user })
+
+ consolidated_like = subject.consolidate_or_save!(notification)
+
+ expect(consolidated_like.id).not_to eq(first_notification.id)
+ expect(consolidated_like.notification_type).to eq(Notification.types[:liked_consolidated])
+ data = JSON.parse(consolidated_like.data)
+ expect(data['count']).to eq(threshold + 1)
+ end
+
+ it 'updates the notification if we already consolidated it' do
+ count = 5
+ Fabricate(:notification,
+ user: user, notification_type: Notification.types[:liked_consolidated],
+ data: { count: count, display_username: like_user }.to_json
+ )
+ notification = build_notification(:liked, { display_username: like_user })
+
+ updated = subject.consolidate_or_save!(notification)
+
+ expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
+ data = JSON.parse(updated.data)
+ expect(data['count']).to eq(count + 1)
+ end
+ end
+
+ def build_notification(type_sym, data)
+ Fabricate.build(:notification, user: user, notification_type: Notification.types[type_sym], data: data.to_json)
+ end
+
+ def plan_for(notification)
+ subject.plan_for(notification)
+ end
+end
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index 65c6714bdd5..edc0a044e6b 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -102,6 +102,33 @@ describe PostAlerter do
notification_payload = JSON.parse(group_summary_notification.first.data)
expect(notification_payload["group_name"]).to eq(group.name)
end
+
+ it 'consolidates group summary notifications by bumping an existing one' do
+ TopicUser.change(user2.id, pm.id, notification_level: TopicUser.notification_levels[:tracking])
+ PostAlerter.post_created(op)
+
+ group_summary_notification = Notification.where(
+ user_id: user2.id,
+ notification_type: Notification.types[:group_message_summary]
+ ).last
+ starting_count = group_summary_notification.data_hash[:inbox_count]
+
+ expect(starting_count).to eq(1)
+
+ another_pm = Fabricate(:topic, archetype: 'private_message', category_id: nil, allowed_groups: [group])
+ another_post = Fabricate(:post, user: another_pm.user, topic: another_pm)
+ TopicUser.change(user2.id, another_pm.id, notification_level: TopicUser.notification_levels[:tracking])
+
+ PostAlerter.post_created(another_post)
+ consolidated_summary = Notification.where(
+ user_id: user2.id,
+ notification_type: Notification.types[:group_message_summary]
+ ).last
+ updated_inbox_count = consolidated_summary.data_hash[:inbox_count]
+
+ expect(group_summary_notification.id).to eq(consolidated_summary.id)
+ expect(updated_inbox_count).to eq(starting_count + 1)
+ end
end
end