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.

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)

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

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])

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

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
 

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)

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