From bc23dcd30b1106bfd9a2ebeff4a7add21b23a353 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 10 Sep 2021 09:20:50 +0800 Subject: [PATCH] FIX: Don't publish PM archive events to acting user. (#14291) When a user archives a personal message, they are redirected back to the inbox and will refresh the list of the topics for the given filter. Publishing an event to the user results in an incorrect incoming message because the list of topics has already been refreshed. This does mean that if a user has two tabs opened, the non-active tab will not receive the incoming message but at this point we do not think the technical trade-offs are worth it to support this feature. We basically have to somehow exclude a client from an incoming message which is not easy to do. Follow-up to fc1fd1b41689694b3916dc4e10605eb9b8bb89b7 --- .../private-message-topic-tracking-state.js | 10 +-- .../acceptance/user-private-messages-test.js | 64 ++++++------------- app/controllers/topics_controller.rb | 14 +++- app/models/group_archived_message.rb | 12 ++-- .../private_message_topic_tracking_state.rb | 23 +++---- app/models/user_archived_message.rb | 7 -- lib/post_creator.rb | 2 +- lib/topics_bulk_action.rb | 4 +- spec/components/post_creator_spec.rb | 20 ++++++ ...ivate_message_topic_tracking_state_spec.rb | 20 ++---- spec/models/user_archived_message_spec.rb | 16 +---- spec/requests/topics_controller_spec.rb | 39 +++++++++++ 12 files changed, 117 insertions(+), 114 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/private-message-topic-tracking-state.js b/app/assets/javascripts/discourse/app/models/private-message-topic-tracking-state.js index c8a8d0fcb4..a7fafaeb27 100644 --- a/app/assets/javascripts/discourse/app/models/private-message-topic-tracking-state.js +++ b/app/assets/javascripts/discourse/app/models/private-message-topic-tracking-state.js @@ -168,18 +168,12 @@ const PrivateMessageTopicTrackingState = EmberObject.extend({ this._notifyIncoming(message.topic_id); } - break; - case "archive": - if ( - [INBOX_FILTER, ARCHIVE_FILTER].includes(this.filter) && - ["user", "all"].includes(this.inbox) - ) { - this._notifyIncoming(message.topic_id); - } break; case "group_archive": if ( [INBOX_FILTER, ARCHIVE_FILTER].includes(this.filter) && + (!message.payload.acting_user_id || + message.payload.acting_user_id !== this.currentUser.id) && (this.inbox === "all" || this._displayMessageForGroupInbox(message)) ) { this._notifyIncoming(message.topic_id); diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js index 5157fa71e7..29372f6822 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js @@ -235,16 +235,6 @@ acceptance( ); }; - const publishArchiveToMessageBus = function (opts) { - publishToMessageBus( - `/private-message-topic-tracking-state/user/${opts.userId || 5}`, - { - topic_id: opts.topicId, - message_type: "archive", - } - ); - }; - const publishGroupArchiveToMessageBus = function (opts) { publishToMessageBus( `/private-message-topic-tracking-state/group/${opts.groupIds[0]}`, @@ -253,6 +243,7 @@ acceptance( message_type: "group_archive", payload: { group_ids: opts.groupIds, + acting_user_id: opts.actingUserId, }, } ); @@ -289,42 +280,6 @@ acceptance( ); }; - test("incoming archive message on all and archive filter", async function (assert) { - for (const url of [ - "/u/charlie/messages", - "/u/charlie/messages/archive", - "/u/charlie/messages/personal", - "/u/charlie/messages/personal/archive", - ]) { - await visit(url); - - publishArchiveToMessageBus({ topicId: 1 }); - - await visit(url); // wait for re-render - - assert.ok( - exists(".show-mores"), - `${url} displays the topic incoming info` - ); - } - - for (const url of [ - "/u/charlie/messages/group/awesome_group/archive", - "/u/charlie/messages/group/awesome_group", - ]) { - await visit(url); - - publishArchiveToMessageBus({ topicId: 1 }); - - await visit(url); // wait for re-render - - assert.ok( - !exists(".show-mores"), - `${url} does not display the topic incoming info` - ); - } - }); - test("incoming read message on unread filter", async function (assert) { await visit("/u/charlie/messages/unread"); @@ -335,6 +290,23 @@ acceptance( assert.ok(exists(".show-mores"), `displays the topic incoming info`); }); + test("incoming group archive message acted by current user", async function (assert) { + await visit("/u/charlie/messages"); + + publishGroupArchiveToMessageBus({ + groupIds: [14], + topicId: 1, + actingUserId: 5, + }); + + await visit("/u/charlie/messages"); // wait for re-render + + assert.ok( + !exists(".show-mores"), + `does not display the topic incoming info` + ); + }); + test("incoming group archive message on all and archive filter", async function (assert) { for (const url of [ "/u/charlie/messages", diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 1f9ab77c30..710632bae0 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -548,12 +548,22 @@ class TopicsController < ApplicationController if group_ids.present? allowed_groups = topic.allowed_groups .where('topic_allowed_groups.group_id IN (?)', group_ids).pluck(:id) + allowed_groups.each do |id| if archive - GroupArchivedMessage.archive!(id, topic) + GroupArchivedMessage.archive!( + id, + topic, + acting_user_id: current_user.id + ) + group_id = id else - GroupArchivedMessage.move_to_inbox!(id, topic) + GroupArchivedMessage.move_to_inbox!( + id, + topic, + acting_user_id: current_user.id + ) end end end diff --git a/app/models/group_archived_message.rb b/app/models/group_archived_message.rb index 3b481c86f1..ea5eb6a9ab 100644 --- a/app/models/group_archived_message.rb +++ b/app/models/group_archived_message.rb @@ -9,7 +9,7 @@ class GroupArchivedMessage < ActiveRecord::Base destroyed = GroupArchivedMessage.where(group_id: group_id, topic_id: topic_id).destroy_all trigger(:move_to_inbox, group_id, topic_id) MessageBus.publish("/topic/#{topic_id}", { type: "move_to_inbox" }, group_ids: [group_id]) - publish_topic_tracking_state(topic, 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? end @@ -19,7 +19,7 @@ class GroupArchivedMessage < ActiveRecord::Base GroupArchivedMessage.create!(group_id: group_id, topic_id: topic_id) trigger(:archive_message, group_id, topic_id) MessageBus.publish("/topic/#{topic_id}", { type: "archived" }, group_ids: [group_id]) - publish_topic_tracking_state(topic, 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? end @@ -39,8 +39,12 @@ class GroupArchivedMessage < ActiveRecord::Base end private_class_method :set_imap_sync - def self.publish_topic_tracking_state(topic, group_id) - PrivateMessageTopicTrackingState.publish_group_archived(topic, group_id) + def self.publish_topic_tracking_state(topic, group_id, acting_user_id = nil) + PrivateMessageTopicTrackingState.publish_group_archived( + topic: topic, + group_id: group_id, + acting_user_id: acting_user_id + ) end private_class_method :publish_topic_tracking_state end diff --git a/app/models/private_message_topic_tracking_state.rb b/app/models/private_message_topic_tracking_state.rb index 0c33e97b2d..74ad8d5f12 100644 --- a/app/models/private_message_topic_tracking_state.rb +++ b/app/models/private_message_topic_tracking_state.rb @@ -21,7 +21,6 @@ class PrivateMessageTopicTrackingState NEW_MESSAGE_TYPE = "new_topic" UNREAD_MESSAGE_TYPE = "unread" READ_MESSAGE_TYPE = "read" - ARCHIVE_MESSAGE_TYPE = "archive" GROUP_ARCHIVE_MESSAGE_TYPE = "group_archive" def self.report(user) @@ -163,29 +162,23 @@ class PrivateMessageTopicTrackingState end end - def self.publish_group_archived(topic, group_id) + def self.publish_group_archived(topic:, group_id:, acting_user_id: nil) return unless topic.private_message? message = { message_type: GROUP_ARCHIVE_MESSAGE_TYPE, topic_id: topic.id, payload: { - group_ids: [group_id] + group_ids: [group_id], + acting_user_id: acting_user_id } }.as_json - MessageBus.publish(self.group_channel(group_id), message, group_ids: [group_id]) - end - - def self.publish_user_archived(topic, user_id) - return unless topic.private_message? - - message = { - message_type: ARCHIVE_MESSAGE_TYPE, - topic_id: topic.id, - }.as_json - - MessageBus.publish(self.user_channel(user_id), message, user_ids: [user_id]) + MessageBus.publish( + self.group_channel(group_id), + message, + group_ids: [group_id] + ) end def self.publish_read(topic_id, last_read_post_number, user, notification_level = nil) diff --git a/app/models/user_archived_message.rb b/app/models/user_archived_message.rb index f5ee006dfb..2410f50e2e 100644 --- a/app/models/user_archived_message.rb +++ b/app/models/user_archived_message.rb @@ -16,7 +16,6 @@ class UserArchivedMessage < ActiveRecord::Base UserArchivedMessage.where(user_id: user_id, topic_id: topic_id).destroy_all trigger(:move_to_inbox, user_id, topic_id) MessageBus.publish("/topic/#{topic_id}", { type: "move_to_inbox" }, user_ids: [user_id]) - publish_topic_tracking_state(topic, user_id) end def self.archive!(user_id, topic) @@ -25,7 +24,6 @@ class UserArchivedMessage < ActiveRecord::Base UserArchivedMessage.create!(user_id: user_id, topic_id: topic_id) trigger(:archive_message, user_id, topic_id) MessageBus.publish("/topic/#{topic_id}", { type: "archived" }, user_ids: [user_id]) - publish_topic_tracking_state(topic, user_id) end def self.trigger(event, user_id, topic_id) @@ -35,11 +33,6 @@ class UserArchivedMessage < ActiveRecord::Base DiscourseEvent.trigger(event, user: user, topic: topic) end end - - def self.publish_topic_tracking_state(topic, user_id) - PrivateMessageTopicTrackingState.publish_user_archived(topic, user_id) - end - private_class_method :publish_topic_tracking_state end # == Schema Information diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 69a94bdc80..3bedd2a1d1 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -480,7 +480,7 @@ class PostCreator end GroupArchivedMessage.where(topic_id: @topic.id).pluck(:group_id).each do |group_id| - GroupArchivedMessage.move_to_inbox!(group_id, @topic) + GroupArchivedMessage.move_to_inbox!(group_id, @topic, acting_user_id: @user.id) end end diff --git a/lib/topics_bulk_action.rb b/lib/topics_bulk_action.rb index e0a82daa4c..f38f4b17ce 100644 --- a/lib/topics_bulk_action.rb +++ b/lib/topics_bulk_action.rb @@ -47,7 +47,7 @@ class TopicsBulkAction topics.each do |t| if guardian.can_see?(t) && t.private_message? if group - GroupArchivedMessage.move_to_inbox!(group.id, t) + GroupArchivedMessage.move_to_inbox!(group.id, t, acting_user_id: @user.id) else UserArchivedMessage.move_to_inbox!(@user.id, t) end @@ -60,7 +60,7 @@ class TopicsBulkAction topics.each do |t| if guardian.can_see?(t) && t.private_message? if group - GroupArchivedMessage.archive!(group.id, t) + GroupArchivedMessage.archive!(group.id, t, acting_user_id: @user.id) else UserArchivedMessage.archive!(@user.id, t) end diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 165162953e..bf677b39df 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -1134,6 +1134,26 @@ describe PostCreator do expect(target_user1.notifications.count).to eq(1) expect(target_user2.notifications.count).to eq(1) + + GroupArchivedMessage.create!(group: group, topic: post.topic) + + message = MessageBus.track_publish( + PrivateMessageTopicTrackingState.group_channel(group.id) + ) do + PostCreator.create!(user, + raw: "this is a reply to the group message", + topic_id: post.topic_id + ) + end.first + + expect(message.data["message_type"]).to eq( + PrivateMessageTopicTrackingState::GROUP_ARCHIVE_MESSAGE_TYPE + ) + + expect(message.data["payload"]["acting_user_id"]).to eq(user.id) + + expect(GroupArchivedMessage.exists?(group: group, topic: post.topic)) + .to eq(false) end end diff --git a/spec/models/private_message_topic_tracking_state_spec.rb b/spec/models/private_message_topic_tracking_state_spec.rb index f4aa96f323..f0fdb380a9 100644 --- a/spec/models/private_message_topic_tracking_state_spec.rb +++ b/spec/models/private_message_topic_tracking_state_spec.rb @@ -168,26 +168,17 @@ describe PrivateMessageTopicTrackingState do end end - describe '.publish_user_archived' do - it 'should publish the right message_bus message' do - message = MessageBus.track_publish(described_class.user_channel(user.id)) do - described_class.publish_user_archived(private_message, user.id) - end.first - - data = message.data - - expect(data['topic_id']).to eq(private_message.id) - expect(data['message_type']).to eq(described_class::ARCHIVE_MESSAGE_TYPE) - end - end - describe '.publish_group_archived' do it 'should publish the right message_bus message' do user_3 = Fabricate(:user) group.add(user_3) messages = MessageBus.track_publish do - described_class.publish_group_archived(group_message, group.id) + described_class.publish_group_archived( + topic: group_message, + group_id: group.id, + acting_user_id: user_3.id + ) end expect(messages.map(&:channel)).to contain_exactly( @@ -201,6 +192,7 @@ describe PrivateMessageTopicTrackingState do expect(data['message_type']).to eq(described_class::GROUP_ARCHIVE_MESSAGE_TYPE) expect(data['topic_id']).to eq(group_message.id) expect(data['payload']['group_ids']).to contain_exactly(group.id) + expect(data['payload']['acting_user_id']).to eq(user_3.id) end end diff --git a/spec/models/user_archived_message_spec.rb b/spec/models/user_archived_message_spec.rb index dcfd7743e1..8a2f5a0795 100644 --- a/spec/models/user_archived_message_spec.rb +++ b/spec/models/user_archived_message_spec.rb @@ -20,11 +20,7 @@ describe UserArchivedMessage do UserArchivedMessage.archive!(user.id, private_message) expect do - messages = MessageBus.track_publish(PrivateMessageTopicTrackingState.user_channel(user.id)) do - UserArchivedMessage.move_to_inbox!(user.id, private_message) - end - - expect(messages.present?).to eq(true) + UserArchivedMessage.move_to_inbox!(user.id, private_message) end.to change { private_message.message_archived?(user) }.from(true).to(false) end @@ -40,14 +36,4 @@ describe UserArchivedMessage do end end - describe '.archive' do - it 'archives message correctly' do - messages = MessageBus.track_publish(PrivateMessageTopicTrackingState.user_channel(user.id)) do - UserArchivedMessage.archive!(user.id, private_message) - end - - expect(messages.present?).to eq(true) - end - end - end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 6cd81254d6..f904694a6b 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -4458,4 +4458,43 @@ RSpec.describe TopicsController do .to eq(true) end end + + describe '#archive_message' do + fab!(:group) do + Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]).tap do |g| + g.add(user) + end + end + + fab!(:group_message) do + create_post( + user: user, + target_group_names: [group.name], + archetype: Archetype.private_message + ).topic + end + + it 'should be able to archive a private message' do + sign_in(user) + + message = MessageBus.track_publish( + PrivateMessageTopicTrackingState.group_channel(group.id) + ) do + + put "/t/#{group_message.id}/archive-message.json" + + expect(response.status).to eq(200) + end.first + + expect(message.data["message_type"]).to eq( + PrivateMessageTopicTrackingState::GROUP_ARCHIVE_MESSAGE_TYPE + ) + + expect(message.data["payload"]["acting_user_id"]).to eq(user.id) + + body = response.parsed_body + + expect(body["group_name"]).to eq(group.name) + end + end end