From b7b61d4b56392fd665ca359eed87b3f6e80a4493 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Fri, 10 Dec 2021 10:32:15 -0300 Subject: [PATCH] FEATURE: A notification consolidation plan for keeping the latest one. (#15249) We previously used ConsolidateNotifications with a threshold of 1 to re-use an existing notification and bump it to the top instead of creating a new one. It produces some jumpiness in the user notification list, and it relies on updating the `created_at` attribute, which is a bit hacky. As a better alternative, we're introducing a new plan that deletes all the previous versions of the notification, then creates a new one. --- .../consolidate_notifications.rb | 37 +++---------- .../notifications/consolidation_plan.rb | 36 +++++++++++++ .../notifications/consolidation_planner.rb | 25 ++------- .../delete_previous_notifications.rb | 54 +++++++++++++++++++ lib/plugin/instance.rb | 5 +- spec/jobs/dashboard_stats_spec.rb | 17 ------ spec/services/post_alerter_spec.rb | 15 ++---- 7 files changed, 106 insertions(+), 83 deletions(-) create mode 100644 app/services/notifications/consolidation_plan.rb create mode 100644 app/services/notifications/delete_previous_notifications.rb diff --git a/app/services/notifications/consolidate_notifications.rb b/app/services/notifications/consolidate_notifications.rb index 93da4a6af3d..7ffd8315981 100644 --- a/app/services/notifications/consolidate_notifications.rb +++ b/app/services/notifications/consolidate_notifications.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Represents a rule to consolidate a specific notification. +# Consolidate notifications based on a threshold and a time window. # # 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. @@ -14,7 +14,6 @@ # - 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. -# - bump_notification: Bump the consolidated notification to the top after updating it. # # Need to call #set_precondition to configure this: # @@ -25,8 +24,8 @@ # - 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:, bump_notification: true) + class ConsolidateNotifications < ConsolidationPlan + def initialize(from:, to:, consolidation_window: nil, unconsolidated_query_blk: nil, consolidated_query_blk: nil, threshold:) @from = from @to = to @threshold = threshold @@ -38,18 +37,6 @@ module Notifications @bump_notification = bump_notification 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 @@ -76,11 +63,6 @@ module Notifications :unconsolidated_query_blk, :consolidation_window, :bump_notification ) - 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) @@ -96,17 +78,11 @@ module Notifications # Hack: We don't want to cache the old data if we're about to update it. consolidated.instance_variable_set(:@data_hash, nil) - attrs = { + consolidated.update!( data: data_hash.to_json, read: false, updated_at: timestamp, - } - - # Updating created_at may seem wrong, but it's the only way of bumping the notification. - # We cannot order by updated_at because marking them as read will move them to the top. - attrs[:created_at] = timestamp if bump_notification - - consolidated.update!(attrs) + ) consolidated end @@ -146,8 +122,7 @@ module Notifications end def user_notifications(notification, type) - notifications = notification.user.notifications - .where(notification_type: type) + notifications = super(notification, type) if consolidation_window.present? notifications = notifications.where('created_at > ?', consolidation_window.ago) diff --git a/app/services/notifications/consolidation_plan.rb b/app/services/notifications/consolidation_plan.rb new file mode 100644 index 00000000000..cb459b15cae --- /dev/null +++ b/app/services/notifications/consolidation_plan.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Notifications + class ConsolidationPlan + 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) + raise NotImplementedError + end + + def consolidate_or_save!(_notification) + raise NotImplementedError + end + + protected + + def consolidated_data(notification) + return notification.data_hash if @set_data_blk.nil? + @set_data_blk.call(notification) + end + + def user_notifications(notification, type) + notification.user.notifications.where(notification_type: type) + end + end +end diff --git a/app/services/notifications/consolidation_planner.rb b/app/services/notifications/consolidation_planner.rb index 684dcb80e6f..69dc5c1b0d3 100644 --- a/app/services/notifications/consolidation_planner.rb +++ b/app/services/notifications/consolidation_planner.rb @@ -12,7 +12,7 @@ module Notifications private def plan_for(notification) - consolidation_plans = [liked, dashboard_problems_pm, group_message_summary, group_membership] + consolidation_plans = [liked, group_message_summary, group_membership] consolidation_plans.concat(DiscoursePluginRegistry.notification_consolidation_plans) consolidation_plans.detect { |plan| plan.can_consolidate_data?(notification) } @@ -67,31 +67,14 @@ module Notifications 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 + DeletePreviousNotifications.new( + type: Notification.types[:group_message_summary], + previous_query_blk: filtered_by_data_attribute('group_id') ).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]) diff --git a/app/services/notifications/delete_previous_notifications.rb b/app/services/notifications/delete_previous_notifications.rb new file mode 100644 index 00000000000..15ddf0e8d63 --- /dev/null +++ b/app/services/notifications/delete_previous_notifications.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +# Create a new notification while deleting previous versions of it. +# +# Constructor arguments: +# +# - type: The notification type. e.g. `Notification.types[:private_message]` +# - previous_query_blk: A block with the query we'll use to find previous notifications. +# +# 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 DeletePreviousNotifications < ConsolidationPlan + def initialize(type:, previous_query_blk:) + @type = type + @previous_query_blk = previous_query_blk + end + + def can_consolidate_data?(notification) + return false if notification.notification_type != type + + @data = consolidated_data(notification) + + precondition_blk.nil? || precondition_blk.call(notification.data_hash) + end + + def consolidate_or_save!(notification) + @data ||= consolidated_data(notification) + return unless can_consolidate_data?(notification) + + notifications = user_notifications(notification, type) + if previous_query_blk.present? + notifications = previous_query_blk.call(notifications, data) + end + + Notification.transaction do + notifications.destroy_all + notification.save! + end + + notification + end + + private + + attr_reader :type, :data, :precondition_blk, :previous_query_blk + end +end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 902aaa4a00b..ba526f4631e 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -986,10 +986,11 @@ class Plugin::Instance # # 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 + # - Threshold and time window consolidation plan: https://github.com/discourse/discourse/blob/main/app/services/notifications/consolidate_notifications.rb + # - Create a new notification and delete previous versions plan: https://github.com/discourse/discourse/blob/main/app/services/notifications/delete_previous_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 + raise ArgumentError.new("Not a consolidation plan") if !plan.class.ancestors.include?(Notifications::ConsolidationPlan) DiscoursePluginRegistry.register_notification_consolidation_plan(plan, self) end diff --git a/spec/jobs/dashboard_stats_spec.rb b/spec/jobs/dashboard_stats_spec.rb index 3ffb3300eae..e4b2fdafc38 100644 --- a/spec/jobs/dashboard_stats_spec.rb +++ b/spec/jobs/dashboard_stats_spec.rb @@ -33,23 +33,6 @@ 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/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index cd248049771..335ae626a79 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -108,19 +108,11 @@ describe PostAlerter do TopicUser.change(user2.id, pm.id, notification_level: TopicUser.notification_levels[:tracking]) PostAlerter.post_created(op) - group_summary_notification = Notification.where( + starting_count = Notification.where( user_id: user2.id, notification_type: Notification.types[:group_message_summary] - ).last - starting_count = group_summary_notification.data_hash[:inbox_count] + ).pluck("data::json ->> 'inbox_count'").last.to_i - # Create another notification to ensure summary is correctly bumped - user2_post = Fabricate(:post, topic: pm, user: user2) - PostAlerter.new.create_notification( - user2, Notification.types[:liked], user2_post, user_id: pm.user, display_username: pm.user.username - ) - - Notification.where(user: user2).update_all('read = true') 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]) @@ -129,8 +121,7 @@ describe PostAlerter do PostAlerter.post_created(another_post) end.first.data - expect(Notification.recent_report(user2, 1).first.notification_type).to eq(Notification.types[:group_message_summary]) - expect(message_data.dig(:last_notification, :notification, :id)).to eq(group_summary_notification.id) + expect(Notification.where(user: user2).count).to eq(1) expect(message_data.dig(:last_notification, :notification, :data, :inbox_count)).to eq(starting_count + 1) expect(message_data[:unread_notifications]).to eq(1) end