FIX: Update group inbox notifications on archive/unarchive (#16152)

app/jobs/regular/group_pm_alert.rb

@@ -16,7 +16,7 @@ module Jobs
         "group_users.notification_level = :level",
         level: NotificationLevels.all[:tracking]
       ).find_each do |u|
-        alerter.notify_group_summary(u, post)
+        alerter.notify_group_summary(u, topic)
       end
 
       notification_data = {

app/jobs/regular/group_pm_update_summary.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Jobs
+  class GroupPmUpdateSummary < ::Jobs::Base
+    def execute(args)
+      return unless group = Group.find_by(id: args[:group_id])
+      return unless topic = Topic.find_by(id: args[:topic_id])
+
+      group.set_message_default_notification_levels!(topic, ignore_existing: true)
+
+      alerter = PostAlerter.new
+
+      group.users.where(
+        "group_users.notification_level = :level",
+        level: NotificationLevels.all[:tracking]
+      ).find_each do |u|
+        alerter.notify_group_summary(u, topic, acting_user_id: args[:acting_user_id])
+      end
+
+    end
+  end
+end

app/models/group_archived_message.rb

@@ -11,6 +11,12 @@ class GroupArchivedMessage < ActiveRecord::Base
     MessageBus.publish("/topic/#{topic_id}", { type: "move_to_inbox" }, group_ids: [group_id])
     publish_topic_tracking_state(topic, group_id, opts[:acting_user_id])
     set_imap_sync(topic_id) if !opts[:skip_imap_sync] && destroyed.present?
+    Jobs.enqueue(
+      :group_pm_update_summary,
+      group_id: group_id,
+      topic_id: topic_id,
+      acting_user_id: opts[:acting_user_id]
+    )
   end
 
   def self.archive!(group_id, topic, opts = {})
@@ -21,6 +27,12 @@ class GroupArchivedMessage < ActiveRecord::Base
     MessageBus.publish("/topic/#{topic_id}", { type: "archived" }, group_ids: [group_id])
     publish_topic_tracking_state(topic, group_id, opts[:acting_user_id])
     set_imap_sync(topic_id) if !opts[:skip_imap_sync] && destroyed.blank?
+    Jobs.enqueue(
+      :group_pm_update_summary,
+      group_id: group_id,
+      topic_id: topic_id,
+      acting_user_id: opts[:acting_user_id]
+    )
   end
 
   def self.trigger(event, group_id, topic_id)

app/services/post_alerter.rb

@@ -315,30 +315,37 @@ class PostAlerter
     end
   end
 
-  def notify_group_summary(user, post)
+  def notify_group_summary(user, topic, acting_user_id: nil)
     @group_stats ||= {}
-    stats = (@group_stats[post.topic_id] ||= group_stats(post.topic))
+    stats = (@group_stats[topic.id] ||= group_stats(topic))
     return unless stats
 
-    group_id = post.topic
+    group_id = topic
       .topic_allowed_groups
       .where(group_id: user.groups.pluck(:id))
       .pluck_first(:group_id)
 
     stat = stats.find { |s| s[:group_id] == group_id }
-    return unless stat && stat[:inbox_count] > 0
+    return unless stat
 
     DistributedMutex.synchronize("group_message_notify_#{user.id}") do
-      Notification.consolidate_or_create!(
-        notification_type: Notification.types[:group_message_summary],
-        user_id: user.id,
-        data: {
-          group_id: stat[:group_id],
-          group_name: stat[:group_name],
-          inbox_count: stat[:inbox_count],
-          username: user.username_lower
-        }.to_json
-      )
+      if stat[:inbox_count] > 0
+        Notification.consolidate_or_create!(
+          notification_type: Notification.types[:group_message_summary],
+          user_id: user.id,
+          read: user.id === acting_user_id ? true : false,
+          data: {
+            group_id: stat[:group_id],
+            group_name: stat[:group_name],
+            inbox_count: stat[:inbox_count],
+            username: user.username_lower
+          }.to_json
+        )
+      else
+        Notification.where(user_id: user.id, notification_type: Notification.types[:group_message_summary])
+          .where("data::json ->> 'group_id' = ?", stat[:group_id].to_s)
+          .delete_all
+      end
     end
 
     # TODO decide if it makes sense to also publish a desktop notification
@@ -652,7 +659,7 @@ class PostAlerter
         if is_replying?(user, reply_to_user, quoted_users)
           create_pm_notification(user, post, emails_to_skip_send)
         else
-          notify_group_summary(user, post)
+          notify_group_summary(user, post.topic)
         end
       when TopicUser.notification_levels[:regular]
         if is_replying?(user, reply_to_user, quoted_users)

spec/services/post_alerter_spec.rb

@@ -86,7 +86,7 @@ describe PostAlerter do
     context "group inboxes" do
       fab!(:user1) { Fabricate(:user) }
       fab!(:user2) { Fabricate(:user) }
-      fab!(:group) { Fabricate(:group, users: [user2], name: "TestGroup") }
+      fab!(:group) { Fabricate(:group, users: [user2], name: "TestGroup", default_notification_level: 2) }
       fab!(:pm) { Fabricate(:topic, archetype: 'private_message', category_id: nil, allowed_groups: [group]) }
       fab!(:op) { Fabricate(:post, user: pm.user, topic: pm) }
 
@@ -102,6 +102,7 @@ describe PostAlerter do
       end
 
       it "triggers group summary notification" do
+        Jobs.run_immediately!
         TopicUser.change(user2.id, pm.id, notification_level: TopicUser.notification_levels[:tracking])
 
         PostAlerter.post_created(op)
@@ -112,6 +113,49 @@ describe PostAlerter do
 
         notification_payload = JSON.parse(group_summary_notification.first.data)
         expect(notification_payload["group_name"]).to eq(group.name)
+        expect(notification_payload["inbox_count"]).to eq(1)
+
+        # archiving the only PM clears the group summary notification
+        GroupArchivedMessage.archive!(group.id, pm)
+        expect(Notification.where(user_id: user2.id)).to be_blank
+
+        # moving to inbox the only PM restores the group summary notification
+        GroupArchivedMessage.move_to_inbox!(group.id, pm)
+        group_summary_notification = Notification.where(user_id: user2.id)
+        expect(group_summary_notification.first.notification_type).to eq(Notification.types[:group_message_summary])
+
+        updated_payload = JSON.parse(group_summary_notification.first.data)
+        expect(updated_payload["group_name"]).to eq(group.name)
+        expect(updated_payload["inbox_count"]).to eq(1)
+
+        # adding a second PM updates the count
+        pm2 = Fabricate(:topic, archetype: 'private_message', category_id: nil, allowed_groups: [group])
+        op2 = Fabricate(:post, user: pm2.user, topic: pm2)
+        TopicUser.change(user2.id, pm2.id, notification_level: TopicUser.notification_levels[:tracking])
+
+        PostAlerter.post_created(op2)
+        group_summary_notification = Notification.where(user_id: user2.id)
+        updated_payload = JSON.parse(group_summary_notification.first.data)
+
+        expect(updated_payload["group_name"]).to eq(group.name)
+        expect(updated_payload["inbox_count"]).to eq(2)
+
+        # archiving the second PM quietly updates the group summary count for the acting user
+        GroupArchivedMessage.archive!(group.id, pm2, acting_user_id: user2.id)
+        group_summary_notification = Notification.where(user_id: user2.id)
+        expect(group_summary_notification.first.read).to eq(true)
+        updated_payload = JSON.parse(group_summary_notification.first.data)
+
+        expect(updated_payload["inbox_count"]).to eq(1)
+
+        # moving to inbox the second PM quietly updates the group summary count for the acting user
+        GroupArchivedMessage.move_to_inbox!(group.id, pm2, acting_user_id: user2.id)
+        group_summary_notification = Notification.where(user_id: user2.id)
+        expect(group_summary_notification.first.read).to eq(true)
+        updated_payload = JSON.parse(group_summary_notification.first.data)
+
+        expect(updated_payload["group_name"]).to eq(group.name)
+        expect(updated_payload["inbox_count"]).to eq(2)
       end
 
       it 'updates the consolidated group summary inbox count and bumps the notification' do
@@ -168,7 +212,7 @@ describe PostAlerter do
         }.to change(user1.notifications, :count).by(1)
       end
 
-      it 'nofies a group member if someone quotes their post' do
+      it 'notifies a group member if someone quotes their post' do
         group.add(user1)
 
         post = Fabricate(:post, topic: pm, user: user1)