FIX: correct tracking when mute all categories (#11441)

Currently, we have a solution for muted topics. Basically, when a post is created first we send a `muted` message to users who muted that specific topic:

https://github.com/discourse/discourse/blob/master/app/models/topic_tracking_state.rb#L91

Later, topic tracking state filters if the topic is muted or not before update state:

https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/app/models/topic-tracking-state.js#L58:L67

That solution works quite well.

I wanted to extend it to handle `mute all categories by default` setting as well.

In that case, we should only inform the user about new topic/post when they explicitly want to.

If that setting is enabled, we would send "unmuted" message to a user who watches specific category, topic or tag. In all other cases, don't inform user about new topic as all categories are muted by default.

Meta: https://meta.discourse.org/t/threads-muted-by-mute-all-by-default-are-showing-up-as-new-but-not-visible/168324
This commit is contained in:
Krzysztof Kotlarek 2020-12-10 16:49:05 +11:00 committed by GitHub
parent eb60fc86dc
commit da2a61e36c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 123 additions and 15 deletions

View File

@ -55,17 +55,24 @@ const TopicTrackingState = EmberObject.extend({
const tracker = this; const tracker = this;
const process = (data) => { const process = (data) => {
if (data.message_type === "muted") { if (["muted", "unmuted"].includes(data.message_type)) {
tracker.trackMutedTopic(data.topic_id); tracker.trackMutedOrUnmutedTopic(data);
return; return;
} }
tracker.pruneOldMutedTopics(); tracker.pruneOldMutedAndUnmutedTopics();
if (tracker.isMutedTopic(data.topic_id)) { if (tracker.isMutedTopic(data.topic_id)) {
return; return;
} }
if (
this.siteSettings.mute_all_categories_by_default &&
!tracker.isUnmutedTopic(data.topic_id)
) {
return;
}
if (data.message_type === "delete") { if (data.message_type === "delete") {
tracker.removeTopic(data.topic_id); tracker.removeTopic(data.topic_id);
tracker.incrementMessageCount(); tracker.incrementMessageCount();
@ -166,26 +173,47 @@ const TopicTrackingState = EmberObject.extend({
return (this.currentUser && this.currentUser.muted_topics) || []; return (this.currentUser && this.currentUser.muted_topics) || [];
}, },
trackMutedTopic(topicId) { unmutedTopics() {
let mutedTopics = this.mutedTopics().concat({ return (this.currentUser && this.currentUser.unmuted_topics) || [];
topicId: topicId,
createdAt: Date.now(),
});
this.currentUser && this.currentUser.set("muted_topics", mutedTopics);
}, },
pruneOldMutedTopics() { trackMutedOrUnmutedTopic(data) {
let topics, key;
if (data.message_type === "muted") {
key = "muted_topics";
topics = this.mutedTopics();
} else {
key = "unmuted_topics";
topics = this.unmutedTopics();
}
topics = topics.concat({
topicId: data.topic_id,
createdAt: Date.now(),
});
this.currentUser && this.currentUser.set(key, topics);
},
pruneOldMutedAndUnmutedTopics() {
const now = Date.now(); const now = Date.now();
let mutedTopics = this.mutedTopics().filter( let mutedTopics = this.mutedTopics().filter(
(mutedTopic) => now - mutedTopic.createdAt < 60000 (mutedTopic) => now - mutedTopic.createdAt < 60000
); );
this.currentUser && this.currentUser.set("muted_topics", mutedTopics); let unmutedTopics = this.unmutedTopics().filter(
(unmutedTopic) => now - unmutedTopic.createdAt < 60000
);
this.currentUser &&
this.currentUser.set("muted_topics", mutedTopics) &&
this.currentUser.set("unmuted_topics", unmutedTopics);
}, },
isMutedTopic(topicId) { isMutedTopic(topicId) {
return !!this.mutedTopics().findBy("topicId", topicId); return !!this.mutedTopics().findBy("topicId", topicId);
}, },
isUnmutedTopic(topicId) {
return !!this.unmutedTopics().findBy("topicId", topicId);
},
updateSeen(topicId, highestSeen) { updateSeen(topicId, highestSeen) {
if (!topicId || !highestSeen) { if (!topicId || !highestSeen) {
return; return;

View File

@ -327,7 +327,7 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
assert.equal(state.countNew(4), 0); assert.equal(state.countNew(4), 0);
}); });
test("mute topic", function (assert) { test("mute and unmute topic", function (assert) {
let currentUser = User.create({ let currentUser = User.create({
username: "chuck", username: "chuck",
muted_category_ids: [], muted_category_ids: [],
@ -335,14 +335,19 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
const state = TopicTrackingState.create({ currentUser }); const state = TopicTrackingState.create({ currentUser });
state.trackMutedTopic(1); state.trackMutedOrUnmutedTopic({ topic_id: 1, message_type: "muted" });
assert.equal(currentUser.muted_topics[0].topicId, 1); assert.equal(currentUser.muted_topics[0].topicId, 1);
state.pruneOldMutedTopics(); state.trackMutedOrUnmutedTopic({ topic_id: 2, message_type: "unmuted" });
assert.equal(currentUser.unmuted_topics[0].topicId, 2);
state.pruneOldMutedAndUnmutedTopics();
assert.equal(state.isMutedTopic(1), true); assert.equal(state.isMutedTopic(1), true);
assert.equal(state.isUnmutedTopic(2), true);
this.clock.tick(60000); this.clock.tick(60000);
state.pruneOldMutedTopics(); state.pruneOldMutedAndUnmutedTopics();
assert.equal(state.isMutedTopic(1), false); assert.equal(state.isMutedTopic(1), false);
assert.equal(state.isUnmutedTopic(2), false);
}); });
}); });

View File

@ -12,6 +12,7 @@ class TopicTrackingState
UNREAD_MESSAGE_TYPE = "unread" UNREAD_MESSAGE_TYPE = "unread"
LATEST_MESSAGE_TYPE = "latest" LATEST_MESSAGE_TYPE = "latest"
MUTED_MESSAGE_TYPE = "muted" MUTED_MESSAGE_TYPE = "muted"
UNMUTED_MESSAGE_TYPE = "unmuted"
attr_accessor :user_id, attr_accessor :user_id,
:topic_id, :topic_id,
@ -104,6 +105,25 @@ class TopicTrackingState
MessageBus.publish("/latest", message.as_json, user_ids: user_ids) MessageBus.publish("/latest", message.as_json, user_ids: user_ids)
end end
def self.publish_unmuted(topic)
return if !SiteSetting.mute_all_categories_by_default
user_ids = User
.joins(DB.sql_fragment("LEFT JOIN category_users ON category_users.user_id = users.id AND category_users.category_id = :category_id", category_id: topic.category_id))
.joins(DB.sql_fragment("LEFT JOIN topic_users ON topic_users.user_id = users.id AND topic_users.topic_id = :topic_id", topic_id: topic.id))
.joins("LEFT JOIN tag_users ON tag_users.user_id = users.id AND tag_users.tag_id IN (#{topic.tag_ids.join(",").presence || 'NULL'})")
.where("category_users.notification_level > 0 OR topic_users.notification_level > 0 OR tag_users.notification_level > 0")
.where("users.last_seen_at > ?", 7.days.ago)
.order("users.last_seen_at DESC")
.limit(100)
.pluck(:id)
return if user_ids.blank?
message = {
topic_id: topic.id,
message_type: UNMUTED_MESSAGE_TYPE,
}
MessageBus.publish("/latest", message.as_json, user_ids: user_ids)
end
def self.publish_unread(post) def self.publish_unread(post)
return unless post.topic.regular? return unless post.topic.regular?
# TODO at high scale we are going to have to defer this, # TODO at high scale we are going to have to defer this,

View File

@ -57,6 +57,7 @@ class PostJobsEnqueuer
end end
def after_post_create def after_post_create
TopicTrackingState.publish_unmuted(@post.topic)
if @post.post_number > 1 if @post.post_number > 1
TopicTrackingState.publish_muted(@post.topic) TopicTrackingState.publish_muted(@post.topic)
TopicTrackingState.publish_unread(@post) TopicTrackingState.publish_unread(@post)

View File

@ -537,6 +537,7 @@ class PostRevisor
return if bypass_bump? || !is_last_post? return if bypass_bump? || !is_last_post?
@topic.update_column(:bumped_at, Time.now) @topic.update_column(:bumped_at, Time.now)
TopicTrackingState.publish_muted(@topic) TopicTrackingState.publish_muted(@topic)
TopicTrackingState.publish_unmuted(@topic)
TopicTrackingState.publish_latest(@topic) TopicTrackingState.publish_latest(@topic)
end end

View File

@ -112,6 +112,59 @@ describe TopicTrackingState do
end end
end end
describe '#publish_unmuted' do
let(:user) do
Fabricate(:user, last_seen_at: Date.today)
end
let(:second_user) do
Fabricate(:user, last_seen_at: Date.today)
end
let(:third_user) do
Fabricate(:user, last_seen_at: Date.today)
end
let(:post) do
create_post(user: user)
end
it 'can correctly publish unmuted' do
Fabricate(:topic_tag, topic: topic)
SiteSetting.mute_all_categories_by_default = true
TopicUser.find_by(topic: topic, user: post.user).update(notification_level: 1)
CategoryUser.create!(category: topic.category, user: second_user, notification_level: 1)
TagUser.create!(tag: topic.tags.first, user: third_user, notification_level: 1)
TagUser.create!(tag: topic.tags.first, user: Fabricate(:user), notification_level: 0)
messages = MessageBus.track_publish("/latest") do
TopicTrackingState.publish_unmuted(topic)
end
unmuted_message = messages.find { |message| message.data["message_type"] == "unmuted" }
expect(unmuted_message.user_ids.sort).to eq([user.id, second_user.id, third_user.id].sort)
expect(unmuted_message.data["topic_id"]).to eq(topic.id)
expect(unmuted_message.data["message_type"]).to eq(described_class::UNMUTED_MESSAGE_TYPE)
end
it 'should not publish any message when notification level is not muted' do
SiteSetting.mute_all_categories_by_default = true
TopicUser.find_by(topic: topic, user: post.user).update(notification_level: 0)
messages = MessageBus.track_publish("/latest") do
TopicTrackingState.publish_unmuted(topic)
end
unmuted_messages = messages.select { |message| message.data["message_type"] == "unmuted" }
expect(unmuted_messages).to eq([])
end
it 'should not publish any message when the user was not seen in the last 7 days' do
TopicUser.find_by(topic: topic, user: post.user).update(notification_level: 1)
post.user.update(last_seen_at: 8.days.ago)
messages = MessageBus.track_publish("/latest") do
TopicTrackingState.publish_unmuted(topic)
end
unmuted_messages = messages.select { |message| message.data["message_type"] == "unmuted" }
expect(unmuted_messages).to eq([])
end
end
describe '#publish_private_message' do describe '#publish_private_message' do
fab!(:admin) { Fabricate(:admin) } fab!(:admin) { Fabricate(:admin) }