From c6c633e041221734fc47bbfc1979fe9bfa09d568 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 19 Apr 2022 11:37:01 +1000 Subject: [PATCH] FIX: Issues with incorrect unread and private message topic tracking state (#16474) This commit fixes two issues at play. The first was introduced in f6c852b (or maybe not introduced but rather revealed). When a user posted a new message in a topic, they received the unread topic tracking state MessageBus message, and the Unread (X) indicator was incremented by one, because with the aforementioned perf commit we "guess" the correct last read post for the user, because we no longer calculate individual users' read status there. This meant that every time a user posted in a topic they tracked, the unread indicator was incremented. To get around this, we can just exclude the user who created the post from the target users of the unread state message. The second issue was related to the private message topic tracking state, and was somewhat similar. Whenever a user created a new private message, the New (X) indicator was incremented, and could not be cleared until the page was refreshed. To solve this, we just don't update the topic state for the user when the new_topic tracking state message comes through if the user who created the topic is the same as the current user. cf. https://meta.discourse.org/t/bottom-of-topic-shows-there-is-1-unread-remaining-when-there-are-actually-0-unread-topics-remaining/220817 --- .../private-message-topic-tracking-state.js | 28 +++- ...ivate-message-topic-tracking-state-test.js | 146 +++++++++++++++++- .../private_message_topic_tracking_state.rb | 12 +- app/models/topic_tracking_state.rb | 3 + spec/fabricators/topic_user_fabricator.rb | 16 ++ spec/lib/post_creator_spec.rb | 6 +- ...ivate_message_topic_tracking_state_spec.rb | 2 + spec/models/topic_tracking_state_spec.rb | 31 +++- 8 files changed, 225 insertions(+), 19 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 43e3ebe651d..5a96b7064f4 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 @@ -107,6 +107,10 @@ const PrivateMessageTopicTrackingState = EmberObject.extend({ this._afterStateChange(); }, + findState(topicId) { + return this.states.get(topicId); + }, + _userChannel() { return `${this.CHANNEL_PREFIX}/user/${this.currentUser.id}`; }, @@ -159,13 +163,14 @@ const PrivateMessageTopicTrackingState = EmberObject.extend({ _processMessage(message) { switch (message.message_type) { case "new_topic": - this._modifyState(message.topic_id, message.payload); - - if ( - [NEW_FILTER, INBOX_FILTER].includes(this.filter) && - this._shouldDisplayMessageForInbox(message) - ) { - this._notifyIncoming(message.topic_id); + if (message.payload.created_by_user_id !== this.currentUser.id) { + this._modifyState(message.topic_id, message.payload); + if ( + [NEW_FILTER, INBOX_FILTER].includes(this.filter) && + this._shouldDisplayMessageForInbox(message) + ) { + this._notifyIncoming(message.topic_id); + } } break; @@ -174,6 +179,13 @@ const PrivateMessageTopicTrackingState = EmberObject.extend({ break; case "unread": + // Note: At some point we may want to make the same peformance optimisation + // here as we did with the other topic tracking state, where we only send + // one 'unread' update to all users, not a more accurate unread update to + // each individual user with their own read state. In this case, we need to + // ignore unread updates which are triggered by the current user. + // + // cf. f6c852bf8e7f4dea519425ba87a114f22f52a8f4 this._modifyState(message.topic_id, message.payload); if ( @@ -235,7 +247,7 @@ const PrivateMessageTopicTrackingState = EmberObject.extend({ }, _modifyState(topicId, data, opts = {}) { - const oldState = this.states.get(topicId); + const oldState = this.findState(topicId); let newState = data; if (oldState && !deepEqual(oldState, newState)) { diff --git a/app/assets/javascripts/discourse/tests/unit/models/private-message-topic-tracking-state-test.js b/app/assets/javascripts/discourse/tests/unit/models/private-message-topic-tracking-state-test.js index 7d5091c3730..cad66d77d04 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/private-message-topic-tracking-state-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/private-message-topic-tracking-state-test.js @@ -1,9 +1,31 @@ import { test } from "qunit"; -import { discourseModule } from "discourse/tests/helpers/qunit-helpers"; +import pretender from "discourse/tests/helpers/create-pretender"; +import { + discourseModule, + publishToMessageBus, +} from "discourse/tests/helpers/qunit-helpers"; import MessageBus from "message-bus-client"; import PrivateMessageTopicTrackingState from "discourse/models/private-message-topic-tracking-state"; import User from "discourse/models/user"; +function setupPretender() { + pretender.get(`/u/test/private-message-topic-tracking-state`, () => { + return [ + 200, + { "Content-Type": "application/json" }, + [ + { + topic_id: 123, + highest_post_number: 12, + last_read_post_number: 12, + notification_level: 3, + group_ids: [], + }, + ], + ]; + }); +} + discourseModule( "Unit | Model | private-message-topic-tracking-state", function (hooks) { @@ -12,7 +34,7 @@ discourseModule( hooks.beforeEach(function () { pmTopicTrackingState = PrivateMessageTopicTrackingState.create({ messageBus: MessageBus, - currentUser: User.create({ id: 1, username: "test" }), + currentUser: User.create({ id: 77889, username: "test" }), }); }); @@ -30,3 +52,123 @@ discourseModule( }); } ); + +discourseModule( + "Unit | Model | private-message-topic-tracking-state | processing new_topic message", + function (hooks) { + let pmTopicTrackingState; + + hooks.beforeEach(function () { + setupPretender(); + pmTopicTrackingState = PrivateMessageTopicTrackingState.create({ + messageBus: MessageBus, + currentUser: User.create({ id: 77889, username: "test" }), + }); + pmTopicTrackingState.startTracking(); + }); + + test("modifies the topic state only if the topic was not created by the current user", function (assert) { + let payload = { + last_read_post_number: null, + highest_post_number: 1, + group_ids: [], + created_by_user_id: 5, + }; + publishToMessageBus("/private-message-topic-tracking-state/user/77889", { + message_type: "new_topic", + topic_id: 4398, + payload, + }); + assert.deepEqual( + pmTopicTrackingState.findState(4398), + payload, + "the new topic created by a different user is loaded into state" + ); + + payload = { + last_read_post_number: null, + highest_post_number: 1, + group_ids: [], + created_by_user_id: 77889, + }; + publishToMessageBus("/private-message-topic-tracking-state/user/77889", { + message_type: "new_topic", + topic_id: 4400, + payload, + }); + assert.deepEqual( + pmTopicTrackingState.findState(4400), + undefined, + "the new topic created by the current user is not loaded into state" + ); + }); + } +); + +discourseModule( + "Unit | Model | private-message-topic-tracking-state | processing unread message", + function (hooks) { + let pmTopicTrackingState; + + hooks.beforeEach(function () { + setupPretender(); + pmTopicTrackingState = PrivateMessageTopicTrackingState.create({ + messageBus: MessageBus, + currentUser: User.create({ id: 77889, username: "test" }), + }); + pmTopicTrackingState.startTracking(); + }); + + test("modifies the last_read_post_number and highest_post_number", function (assert) { + let payload = { + last_read_post_number: 12, + highest_post_number: 13, + notification_level: 3, + group_ids: [], + created_by_user_id: 5, + }; + publishToMessageBus("/private-message-topic-tracking-state/user/77889", { + message_type: "unread", + topic_id: 123, + payload, + }); + + let state = pmTopicTrackingState.findState(123); + assert.deepEqual( + state.highest_post_number, + 13, + "the unread payload triggered by a different user creating a new post updates the state with the correct highest_post_number" + ); + assert.deepEqual( + state.last_read_post_number, + 12, + "the unread payload triggered by a different user creating a new post updates the state with the correct last_read_post_number" + ); + + payload = { + last_read_post_number: 14, + highest_post_number: 14, + notification_level: 3, + group_ids: [], + created_by_user_id: 77889, + }; + publishToMessageBus("/private-message-topic-tracking-state/user/77889", { + message_type: "unread", + topic_id: 123, + payload, + }); + + state = pmTopicTrackingState.findState(123); + assert.deepEqual( + state.highest_post_number, + 14, + "the unread payload triggered by the current user creating a new post updates the state with the correct highest_post_number" + ); + assert.deepEqual( + state.last_read_post_number, + 14, + "the unread payload triggered by the current user creating a new post updates the state with the correct last_read_post_number" + ); + }); + } +); diff --git a/app/models/private_message_topic_tracking_state.rb b/app/models/private_message_topic_tracking_state.rb index 76bdbb8e6f7..66c5003efaf 100644 --- a/app/models/private_message_topic_tracking_state.rb +++ b/app/models/private_message_topic_tracking_state.rb @@ -120,6 +120,12 @@ class PrivateMessageTopicTrackingState .where("gu.group_id IN (?)", group_ids) end + # Note: At some point we may want to make the same peformance optimisation + # here as we did with the other topic tracking state, where we only send + # one 'unread' update to all users, not a more accurate unread update to + # each individual user with their own read state. + # + # cf. f6c852bf8e7f4dea519425ba87a114f22f52a8f4 scope .select([:user_id, :last_read_post_number, :notification_level]) .each do |tu| @@ -137,7 +143,8 @@ class PrivateMessageTopicTrackingState last_read_post_number: tu.last_read_post_number, highest_post_number: post.post_number, notification_level: tu.notification_level, - group_ids: allowed_group_ids + group_ids: allowed_group_ids, + created_by_user_id: post.user_id } } @@ -156,7 +163,8 @@ class PrivateMessageTopicTrackingState payload: { last_read_post_number: nil, highest_post_number: 1, - group_ids: topic.allowed_groups.pluck(:id) + group_ids: topic.allowed_groups.pluck(:id), + created_by_user_id: topic.user_id, } }.as_json diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index d43b054b5b3..1b143b5c2b4 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -142,9 +142,12 @@ class TopicTrackingState tag_ids, tags = post.topic.tags.pluck(:id, :name).transpose end + # We don't need to publish unread to the person who just made the post, + # this is why they are excluded from the initial scope. scope = TopicUser .tracking(post.topic_id) .includes(user: :user_stat) + .where.not(user_id: post.user_id) group_ids = if post.post_type == Post.types[:whisper] diff --git a/spec/fabricators/topic_user_fabricator.rb b/spec/fabricators/topic_user_fabricator.rb index 11cf529a884..c082933f016 100644 --- a/spec/fabricators/topic_user_fabricator.rb +++ b/spec/fabricators/topic_user_fabricator.rb @@ -4,3 +4,19 @@ Fabricator(:topic_user) do user topic end + +Fabricator(:topic_user_tracking, from: :topic_user) do + notification_level { NotificationLevels.topic_levels[:tracking] } +end + +Fabricator(:topic_user_watching, from: :topic_user) do + notification_level { NotificationLevels.topic_levels[:watching] } +end + +Fabricator(:topic_user_regular, from: :topic_user) do + notification_level { NotificationLevels.topic_levels[:regular] } +end + +Fabricator(:topic_user_muted, from: :topic_user) do + notification_level { NotificationLevels.topic_levels[:muted] } +end diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb index 9b32dcb523d..400076faba1 100644 --- a/spec/lib/post_creator_spec.rb +++ b/spec/lib/post_creator_spec.rb @@ -142,15 +142,19 @@ describe PostCreator do admin = Fabricate(:user) admin.grant_admin! + other_admin = Fabricate(:user) + other_admin.grant_admin! cat = Fabricate(:category) cat.set_permissions(admins: :full) cat.save created_post = nil + other_user_tracking_topic = nil messages = MessageBus.track_publish do created_post = PostCreator.new(admin, basic_topic_params.merge(category: cat.id)).create + Fabricate(:topic_user_tracking, topic: created_post.topic, user: other_admin) _reply = PostCreator.new(admin, raw: "this is my test reply 123 testing", topic_id: created_post.topic_id, advance_draft: true).create end @@ -177,7 +181,7 @@ describe PostCreator do ) admin_ids = [Group[:admins].id] - expect(messages.any? { |m| m.group_ids != admin_ids && m.user_ids != [admin.id] }).to eq(false) + expect(messages.any? { |m| m.group_ids != admin_ids && (!m.user_ids.include?(other_admin.id) && !m.user_ids.include?(admin.id)) }).to eq(false) end it 'generates the correct messages for a normal topic' do diff --git a/spec/models/private_message_topic_tracking_state_spec.rb b/spec/models/private_message_topic_tracking_state_spec.rb index 6f400af0dec..98b9ebae1e5 100644 --- a/spec/models/private_message_topic_tracking_state_spec.rb +++ b/spec/models/private_message_topic_tracking_state_spec.rb @@ -138,6 +138,7 @@ describe PrivateMessageTopicTrackingState do expect(data['payload']['last_read_post_number']).to eq(nil) expect(data['payload']['highest_post_number']).to eq(1) expect(data['payload']['group_ids']).to eq([group.id]) + expect(data['payload']['created_by_user_id']).to eq(group_message.user_id) end end @@ -160,6 +161,7 @@ describe PrivateMessageTopicTrackingState do expect(data['topic_id']).to eq(private_message.id) expect(data['payload']['last_read_post_number']).to eq(1) expect(data['payload']['highest_post_number']).to eq(1) + expect(data['payload']['created_by_user_id']).to eq(private_message.first_post.user_id) expect(data['payload']['notification_level']) .to eq(NotificationLevels.all[:watching]) expect(data['payload']['group_ids']).to eq([]) diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index e4288540957..e66a8d5f56e 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -74,6 +74,12 @@ describe TopicTrackingState do end describe '#publish_unread' do + let(:other_user) { Fabricate(:user) } + + before do + Fabricate(:topic_user_watching, topic: topic, user: other_user) + end + it "can correctly publish unread" do message = MessageBus.track_publish("/unread") do TopicTrackingState.publish_unread(post) @@ -81,13 +87,26 @@ describe TopicTrackingState do data = message.data - expect(message.user_ids).to contain_exactly(post.user.id) + expect(message.user_ids).to contain_exactly(other_user.id) expect(message.group_ids).to eq(nil) expect(data["topic_id"]).to eq(topic.id) expect(data["message_type"]).to eq(described_class::UNREAD_MESSAGE_TYPE) expect(data["payload"]["archetype"]).to eq(Archetype.default) end + it "does not publish unread to the user who created the post" do + message = MessageBus.track_publish("/unread") do + TopicTrackingState.publish_unread(post) + end.first + + data = message.data + + expect(message.user_ids).not_to include(post.user_id) + expect(data["topic_id"]).to eq(topic.id) + expect(data["message_type"]).to eq(described_class::UNREAD_MESSAGE_TYPE) + expect(data["payload"]["archetype"]).to eq(Archetype.default) + end + it "is not erroring when user_stat is missing" do post.user.user_stat.destroy! message = MessageBus.track_publish("/unread") do @@ -96,7 +115,7 @@ describe TopicTrackingState do data = message.data - expect(message.user_ids).to contain_exactly(post.user.id) + expect(message.user_ids).to contain_exactly(other_user.id) end it "does not publish whisper post to non-staff users" do @@ -108,13 +127,13 @@ describe TopicTrackingState do expect(messages).to eq([]) - post.user.grant_admin! + other_user.grant_admin! message = MessageBus.track_publish("/unread") do TopicTrackingState.publish_unread(post) end.first - expect(message.user_ids).to contain_exactly(post.user_id) + expect(message.user_ids).to contain_exactly(other_user.id) expect(message.group_ids).to eq(nil) end @@ -130,13 +149,13 @@ describe TopicTrackingState do expect(messages).to eq([]) - group.add(post.user) + group.add(other_user) message = MessageBus.track_publish("/unread") do TopicTrackingState.publish_unread(post) end.first - expect(message.user_ids).to contain_exactly(post.user_id) + expect(message.user_ids).to contain_exactly(other_user.id) expect(message.group_ids).to eq(nil) end