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