Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 fc1fd1b416
This commit is contained in:
Alan Guo Xiang Tan 2021-09-10 09:20:50 +08:00 committed by GitHub
parent 2215cc2547
commit bc23dcd30b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 117 additions and 114 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
app/assets/javascripts/discourse/app/models/private-message-topic-tracking-state.js
View File

@ -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);

64
app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js
View File

@ -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",

14
app/controllers/topics_controller.rb
View File

@ -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

12
app/models/group_archived_message.rb
View File

@ -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

23
app/models/private_message_topic_tracking_state.rb
View File

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

7
app/models/user_archived_message.rb
View File

@ -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

2
lib/post_creator.rb
View File

@ -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

4
lib/topics_bulk_action.rb
View File

@ -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

20
spec/components/post_creator_spec.rb
View File

@ -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

20
spec/models/private_message_topic_tracking_state_spec.rb
View File

@ -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

16
spec/models/user_archived_message_spec.rb
View File

@ -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

39
spec/requests/topics_controller_spec.rb
View File

@ -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