From 94750c81faf0d8a3b7fb0f019fbd176cd96af39c Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Fri, 11 Mar 2022 11:57:47 +0100 Subject: [PATCH] FIX: Update group inbox notifications on archive/unarchive (#16152) --- app/jobs/regular/group_pm_alert.rb | 2 +- app/jobs/regular/group_pm_update_summary.rb | 22 ++++++++++ app/models/group_archived_message.rb | 12 ++++++ app/services/post_alerter.rb | 37 +++++++++------- spec/services/post_alerter_spec.rb | 48 ++++++++++++++++++++- 5 files changed, 103 insertions(+), 18 deletions(-) create mode 100644 app/jobs/regular/group_pm_update_summary.rb diff --git a/app/jobs/regular/group_pm_alert.rb b/app/jobs/regular/group_pm_alert.rb index a1058bb733d..530560e16d7 100644 --- a/app/jobs/regular/group_pm_alert.rb +++ b/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 = { diff --git a/app/jobs/regular/group_pm_update_summary.rb b/app/jobs/regular/group_pm_update_summary.rb new file mode 100644 index 00000000000..cb6c5a61f10 --- /dev/null +++ b/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 diff --git a/app/models/group_archived_message.rb b/app/models/group_archived_message.rb index ea5eb6a9abb..72d71af025c 100644 --- a/app/models/group_archived_message.rb +++ b/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) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 6f4af85d081..ba091af0a0a 100644 --- a/app/services/post_alerter.rb +++ b/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) diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 0ab5048dbad..ebcfd4ef51c 100644 --- a/spec/services/post_alerter_spec.rb +++ b/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)