From 0744d242c6c066eb467c9df3aa6df3b6a8dd7b54 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 6 Jul 2023 11:27:23 +1000 Subject: [PATCH] FIX: post alerter notification when topic directly watched (#22433) In previous PR https://github.com/discourse/discourse/pull/22340 bug was introduced. Notifications were blocked when, even if topic was watched directly. New query is taking TopicUser into consideration. In addition, in user interface, when `watched_precedence_over_muted` is not set, then value from SiteSetting should be displayed. --- .../app/controllers/preferences/tracking.js | 11 ++++++--- .../user-preferences-tracking-test.js | 16 +++++++++++++ app/services/post_alerter.rb | 2 ++ spec/services/post_alerter_spec.rb | 24 +++++++++++++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js b/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js index 2f515b94253..a39a922dd9b 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js @@ -108,12 +108,17 @@ export default class extends Controller { "model.muted_tags.[]" ) get showMutePrecedenceSetting() { - return ( + const show = (this.model.watchedCategories?.length > 0 && this.model.muted_tags?.length > 0) || (this.model.watched_tags?.length > 0 && - this.model.mutedCategories?.length > 0) - ); + this.model.mutedCategories?.length > 0); + + if (show && this.model.user_option.watched_precedence_over_muted === null) { + this.model.user_option.watched_precedence_over_muted = + this.siteSettings.watched_precedence_over_muted; + } + return show; } @computed( diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-tracking-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-tracking-test.js index 176516da4e3..68b8f980ed4 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-tracking-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-tracking-test.js @@ -196,6 +196,22 @@ acceptance("User Preferences - Tracking", function (needs) { }); }); + test("additional precedence option when one category is watched and tag is muted", async function (assert) { + this.siteSettings.tagging_enabled = true; + + await visit("/u/eviltrout/preferences/tracking"); + + const mutedTagsSelector = selectKit( + ".tracking-controls__muted-tags .tag-chooser" + ); + + assert.dom(".user-preferences__watched-precedence-over-muted").doesNotExist; + await mutedTagsSelector.expand(); + await mutedTagsSelector.selectRowByValue("dog"); + + assert.dom(".user-preferences__watched-precedence-over-muted").exists(); + }); + test("tracking category which is set to regular notification level for user when mute_all_categories_by_default site setting is disabled", async function (assert) { this.siteSettings.tagging_enabled = false; diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 25a2b982d82..dfd31db3bee 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -272,6 +272,8 @@ class PostAlerter SELECT user_id FROM category_users WHERE category_id = #{topic.category_id.to_i} AND notification_level = #{CategoryUser.notification_levels[:muted]} UNION SELECT user_id FROM tag_users tu JOIN topic_tags tt ON tt.tag_id = tu.tag_id AND tt.topic_id = #{topic.id} AND tu.notification_level = #{TagUser.notification_levels[:muted]} + EXCEPT + SELECT user_id FROM topic_users tus WHERE tus.topic_id = #{topic.id} AND tus.notification_level = #{TopicUser.notification_levels[:watching]} SQL User .where("id IN (#{user_ids_sql})") diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index b6d6a634969..9cfb9c1350f 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1905,10 +1905,22 @@ RSpec.describe PostAlerter do fab!(:topic_with_muted_category_and_watched_tag) do Fabricate(:topic, category: muted_category, tags: [watched_tag]) end + fab!(:directly_watched_topic) do + Fabricate(:topic, category: muted_category, tags: [muted_tag]) + end + fab!(:topic_user) do + Fabricate( + :topic_user, + topic: directly_watched_topic, + user: user, + notification_level: TopicUser.notification_levels[:watching], + ) + end fab!(:topic_with_watched_category) { Fabricate(:topic, category: category) } fab!(:post) { Fabricate(:post, topic: topic_with_muted_tag_and_watched_category) } fab!(:post_2) { Fabricate(:post, topic: topic_with_muted_category_and_watched_tag) } fab!(:post_3) { Fabricate(:post, topic: topic_with_watched_category) } + fab!(:post_4) { Fabricate(:post, topic: directly_watched_topic) } before do CategoryUser.set_notification_level_for_category( @@ -1926,6 +1938,9 @@ RSpec.describe PostAlerter do expect { PostAlerter.post_created(topic_with_muted_category_and_watched_tag.posts.first) }.to change { Notification.count }.by(1) + expect { PostAlerter.post_created(directly_watched_topic.posts.first) }.to change { + Notification.count + }.by(1) end it "respects user option even if watched_precedence_over_muted site setting is true" do @@ -1937,6 +1952,9 @@ RSpec.describe PostAlerter do expect { PostAlerter.post_created(topic_with_muted_category_and_watched_tag.posts.first) }.not_to change { Notification.count } + expect { PostAlerter.post_created(directly_watched_topic.posts.first) }.to change { + Notification.count + }.by(1) end it "does not add notification when watched_precedence_over_muted setting is false" do @@ -1950,6 +1968,9 @@ RSpec.describe PostAlerter do expect { PostAlerter.post_created(topic_with_watched_category.posts.first) }.to change { Notification.count }.by(1) + expect { PostAlerter.post_created(directly_watched_topic.posts.first) }.to change { + Notification.count + }.by(1) end it "respects user option even if watched_precedence_over_muted site setting is false" do @@ -1961,6 +1982,9 @@ RSpec.describe PostAlerter do expect { PostAlerter.post_created(topic_with_muted_category_and_watched_tag.posts.first) }.to change { Notification.count }.by(1) + expect { PostAlerter.post_created(directly_watched_topic.posts.first) }.to change { + Notification.count + }.by(1) end end