FIX: Updating a consolidated notification should bump it to the top. (#15199)

In the future, it would be better to have a consolidated_at timestamp instead of updating created_at.

app/services/notifications/consolidate_notifications.rb

@@ -14,6 +14,7 @@
 # - 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,7 +26,7 @@
 
 module Notifications
   class ConsolidateNotifications
-    def initialize(from:, to:, consolidation_window: nil, unconsolidated_query_blk: nil, consolidated_query_blk: nil, threshold:)
+    def initialize(from:, to:, consolidation_window: nil, unconsolidated_query_blk: nil, consolidated_query_blk: nil, threshold:, bump_notification: true)
       @from = from
       @to = to
       @threshold = threshold
@@ -34,6 +35,7 @@ module Notifications
       @unconsolidated_query_blk = unconsolidated_query_blk
       @precondition_blk = nil
       @set_data_blk = nil
+      @bump_notification = bump_notification
     end
 
     def set_precondition(precondition_blk: nil)
@@ -69,7 +71,10 @@ module Notifications
 
     private
 
-    attr_reader :notification, :from, :to, :data, :threshold, :consolidated_query_blk, :unconsolidated_query_blk, :consolidation_window
+    attr_reader(
+      :notification, :from, :to, :data, :threshold, :consolidated_query_blk,
+      :unconsolidated_query_blk, :consolidation_window, :bump_notification
+    )
 
     def consolidated_data(notification)
       return notification.data_hash if @set_data_blk.nil?
@@ -91,11 +96,17 @@ 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)
 
-      consolidated.update!(
+      attrs = {
         data: data_hash.to_json,
         read: false,
-        updated_at: timestamp
-      )
+        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

spec/services/post_alerter_spec.rb

@@ -103,7 +103,8 @@ describe PostAlerter do
         expect(notification_payload["group_name"]).to eq(group.name)
       end
 
-      it 'consolidates group summary notifications by bumping an existing one' do
+      it 'updates the consolidated group summary inbox count and bumps the notification' do
+        user2.update!(last_seen_at: 5.minutes.ago)
         TopicUser.change(user2.id, pm.id, notification_level: TopicUser.notification_levels[:tracking])
         PostAlerter.post_created(op)
 
@@ -113,21 +114,25 @@ describe PostAlerter do
         ).last
         starting_count = group_summary_notification.data_hash[:inbox_count]
 
-        expect(starting_count).to eq(1)
+        # 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])
 
-        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]
+        message_data = MessageBus.track_publish("/notification/#{user2.id}") do
+          PostAlerter.post_created(another_post)
+        end.first.data
 
-        expect(group_summary_notification.id).to eq(consolidated_summary.id)
-        expect(updated_inbox_count).to eq(starting_count + 1)
+        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(message_data.dig(:last_notification, :notification, :data, :inbox_count)).to eq(starting_count + 1)
+        expect(message_data[:unread_notifications]).to eq(1)
       end
     end
   end