FIX: Remove tags from experimental sidebar on notification level changed (#17083)

As part of this commit, a bug where updating a tag's notification level on the server side does not update the state of the user's tag notification levels on the client side is fixed too.

app/assets/javascripts/discourse/app/controllers/tag-show.js

@@ -181,14 +181,15 @@ export default DiscoverySortableController.extend(
       this.tagNotification
         .update({ notification_level: notificationLevel })
         .then((response) => {
-          this.currentUser.set(
+          const payload = response.responseJson;
-            "muted_tag_ids",
+
-            this.currentUser.calculateMutedIds(
+          this.currentUser.setProperties({
-              notificationLevel,
+            watched_tags: payload.watched_tags,
-              response.responseJson.tag_id,
+            watching_first_post_tags: payload.watching_first_post_tags,
-              "muted_tag_ids"
+            tracked_tags: payload.tracked_tags,
-            )
+            muted_tags: payload.muted_tags,
-          );
+            regular_tags: payload.regular_tags,
+          });
         });
     },
   }

app/assets/javascripts/discourse/app/models/topic-tracking-state.js

@@ -32,15 +32,15 @@ function isUnseen(topic) {
   return !topic.is_seen;
 }
 
-function hasMutedTags(topicTagIds, mutedTagIds, siteSettings) {
+function hasMutedTags(topicTags, mutedTags, siteSettings) {
-  if (!mutedTagIds || !topicTagIds) {
+  if (!mutedTags || !topicTags) {
     return false;
   }
   return (
     (siteSettings.remove_muted_tags_from_latest === "always" &&
-      topicTagIds.any((tagId) => mutedTagIds.includes(tagId))) ||
+      topicTags.any((topicTag) => mutedTags.includes(topicTag))) ||
     (siteSettings.remove_muted_tags_from_latest === "only_muted" &&
-      topicTagIds.every((tagId) => mutedTagIds.includes(tagId)))
+      topicTags.every((topicTag) => mutedTags.includes(topicTag)))
   );
 }
 
@@ -876,10 +876,9 @@ const TopicTrackingState = EmberObject.extend({
     }
 
     if (["new_topic", "latest"].includes(data.message_type)) {
-      const mutedTagIds = User.currentProp("muted_tag_ids");
+      const mutedTags = User.currentProp("muted_tags");
-      if (
+
-        hasMutedTags(data.payload.topic_tag_ids, mutedTagIds, this.siteSettings)
+      if (hasMutedTags(data.payload.tags, mutedTags, this.siteSettings)) {
-      ) {
         return;
       }
     }

app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js

@@ -14,6 +14,8 @@ import {
 import { isLegacyEmber } from "discourse-common/config/environment";
 import discoveryFixture from "discourse/tests/fixtures/discovery-fixtures";
 import { cloneJSON } from "discourse-common/lib/object";
+import selectKit from "discourse/tests/helpers/select-kit-helper";
+import { NotificationLevels } from "discourse/lib/notification-levels";
 
 acceptance("Sidebar - Tags section - tagging disabled", function (needs) {
   needs.settings({
@@ -62,6 +64,16 @@ acceptance("Sidebar - Tags section", function (needs) {
         );
       });
     });
+
+    server.put("/tag/:tagId/notifications", (request) => {
+      return helper.response({
+        watched_tags: [],
+        watching_first_post_tags: [],
+        regular_tags: [request.params.tagId],
+        tracked_tags: [],
+        muted_tags: [],
+      });
+    });
   });
 
   conditionalTest(
@@ -370,4 +382,25 @@ acceptance("Sidebar - Tags section", function (needs) {
       );
     }
   );
+
+  conditionalTest(
+    "updating tags notification levels",
+    !isLegacyEmber(),
+    async function (assert) {
+      await visit(`/tag/tag1/l/unread`);
+
+      const notificationLevelsDropdown = selectKit(".notifications-button");
+
+      await notificationLevelsDropdown.expand();
+
+      await notificationLevelsDropdown.selectRowByValue(
+        NotificationLevels.REGULAR
+      );
+
+      assert.ok(
+        !exists(".sidebar-section-tags .sidebar-section-link-tag1"),
+        "tag1 section link is removed from sidebar"
+      );
+    }
+  );
 });

app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js

@@ -760,8 +760,10 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) {
       });
 
       test("topics in muted tags do not get added to the state", function (assert) {
-        trackingState.currentUser.set("muted_tag_ids", [44]);
+        trackingState.currentUser.set("muted_tags", ["pending"]);
+
         publishToMessageBus("/new", newTopicPayload);
+
         assert.strictEqual(
           trackingState.findState(222),
           undefined,

app/controllers/tags_controller.rb

@@ -301,7 +301,7 @@ class TagsController < ::ApplicationController
     raise Discourse::NotFound unless tag
     level = params[:tag_notification][:notification_level].to_i
     TagUser.change(current_user.id, tag.id, level)
-    render json: { notification_level: level, tag_id: tag.id }
+    render_serialized(current_user, UserTagNotificationsSerializer, root: false)
   end
 
   def personal_messages

app/serializers/basic_user_serializer.rb

@@ -36,14 +36,4 @@ class BasicUserSerializer < ApplicationSerializer
   def category_user_notification_levels
     @category_user_notification_levels ||= CategoryUser.notification_levels_for(user)
   end
-
-  def tags_with_notification_level(lookup_level)
-    tag_user_notification_levels.select do |id, level|
-      level == TagUser.notification_levels[lookup_level]
-    end.keys
-  end
-
-  def tag_user_notification_levels
-    @tag_user_notification_levels ||= TagUser.notification_levels_for(user)
-  end
 end

app/serializers/concerns/user_tag_notifications_mixin.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module UserTagNotificationsMixin
+  def muted_tags
+    tags_with_notification_level(:muted)
+  end
+
+  def tracked_tags
+    tags_with_notification_level(:tracking)
+  end
+
+  def watching_first_post_tags
+    tags_with_notification_level(:watching_first_post)
+  end
+
+  def watched_tags
+    tags_with_notification_level(:watching)
+  end
+
+  def regular_tags
+    tags_with_notification_level(:regular)
+  end
+
+  def tags_with_notification_level(lookup_level)
+    tag_user_notification_levels.select do |id, level|
+      level == TagUser.notification_levels[lookup_level]
+    end.keys
+  end
+
+  def tag_user_notification_levels
+    @tag_user_notification_levels ||= TagUser.notification_levels_for(user)
+  end
+end

app/serializers/current_user_serializer.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class CurrentUserSerializer < BasicUserSerializer
+  include UserTagNotificationsMixin
 
   attributes :name,
              :unread_notifications,
@@ -33,7 +34,6 @@ class CurrentUserSerializer < BasicUserSerializer
              :tracked_category_ids,
              :watched_first_post_category_ids,
              :watched_category_ids,
-             :muted_tag_ids,
              :watched_tags,
              :watching_first_post_tags,
              :tracked_tags,
@@ -230,32 +230,6 @@ class CurrentUserSerializer < BasicUserSerializer
     categories_with_notification_level(:watching_first_post)
   end
 
-  # this is a weird outlier that is used for topic tracking state which
-  # needs the actual ids, which is why it is duplicated with muted_tags
-  def muted_tag_ids
-    TagUser.lookup(object, :muted).pluck(:tag_id)
-  end
-
-  def muted_tags
-    tags_with_notification_level(:muted)
-  end
-
-  def tracked_tags
-    tags_with_notification_level(:tracking)
-  end
-
-  def watching_first_post_tags
-    tags_with_notification_level(:watching_first_post)
-  end
-
-  def watched_tags
-    tags_with_notification_level(:watching)
-  end
-
-  def regular_tags
-    tags_with_notification_level(:regular)
-  end
-
   def ignored_users
     IgnoredUser.where(user: object.id).joins(:ignored_user).pluck(:username)
   end

app/serializers/user_serializer.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class UserSerializer < UserCardSerializer
+  include UserTagNotificationsMixin
 
   attributes :bio_raw,
              :bio_cooked,
@@ -211,22 +212,6 @@ class UserSerializer < UserCardSerializer
   ###
   ### PRIVATE ATTRIBUTES
   ###
-  def muted_tags
-    tags_with_notification_level(:muted)
-  end
-
-  def tracked_tags
-    tags_with_notification_level(:tracking)
-  end
-
-  def watching_first_post_tags
-    tags_with_notification_level(:watching_first_post)
-  end
-
-  def watched_tags
-    tags_with_notification_level(:watching)
-  end
-
   def muted_category_ids
     categories_with_notification_level(:muted)
   end

app/serializers/user_tag_notifications_serializer.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class UserTagNotificationsSerializer < ApplicationSerializer
+  include UserTagNotificationsMixin
+
+  attributes :watched_tags,
+             :watching_first_post_tags,
+             :tracked_tags,
+             :muted_tags,
+             :regular_tags
+
+  def user
+    object
+  end
+end

spec/requests/tags_controller_spec.rb

@@ -1095,4 +1095,59 @@ describe TagsController do
       end
     end
   end
+
+  describe '#update_notifications' do
+    fab!(:tag) { Fabricate(:tag) }
+
+    before do
+      sign_in(user)
+    end
+
+    it 'returns 404 when tag is not found' do
+      put "/tag/someinvalidtagname/notifications.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    it 'updates the notification level of a tag for a user' do
+      tag_user = TagUser.change(user.id, tag.id, NotificationLevels.all[:muted])
+
+      put "/tag/#{tag.name}/notifications.json", params: {
+        tag_notification: {
+          notification_level: NotificationLevels.all[:tracking]
+        }
+      }
+
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["watched_tags"]).to eq([])
+      expect(response.parsed_body["watching_first_post_tags"]).to eq([])
+      expect(response.parsed_body["tracked_tags"]).to eq([tag.name])
+      expect(response.parsed_body["muted_tags"]).to eq([])
+      expect(response.parsed_body["regular_tags"]).to eq([])
+
+      expect(tag_user.reload.notification_level).to eq(NotificationLevels.all[:tracking])
+    end
+
+    it 'sets the notification level of a tag for a user' do
+      expect do
+        put "/tag/#{tag.name}/notifications.json", params: {
+          tag_notification: {
+            notification_level: NotificationLevels.all[:muted]
+          }
+        }
+
+        expect(response.status).to eq(200)
+
+        expect(response.parsed_body["watched_tags"]).to eq([])
+        expect(response.parsed_body["watching_first_post_tags"]).to eq([])
+        expect(response.parsed_body["tracked_tags"]).to eq([])
+        expect(response.parsed_body["muted_tags"]).to eq([tag.name])
+        expect(response.parsed_body["regular_tags"]).to eq([])
+      end.to change { user.tag_users.count }.by(1)
+
+      tag_user = user.tag_users.last
+
+      expect(tag_user.notification_level).to eq(NotificationLevels.all[:muted])
+    end
+  end
 end

spec/serializers/current_user_serializer_spec.rb

@@ -60,19 +60,21 @@ RSpec.describe CurrentUserSerializer do
     end
   end
 
-  context "#muted_tag_ids" do
+  context "#muted_tag" do
     fab!(:user) { Fabricate(:user) }
     fab!(:tag) { Fabricate(:tag) }
+
     let!(:tag_user) do
-      TagUser.create!(user_id: user.id,
+      TagUser.create!(
-                      notification_level: TagUser.notification_levels[:muted],
+        user_id: user.id,
-                      tag_id: tag.id
+        notification_level: TagUser.notification_levels[:muted],
-                     )
+        tag_id: tag.id
+      )
     end
 
-    it 'include muted tag ids' do
+    it 'includes muted tag names' do
       payload = serializer.as_json
-      expect(payload[:muted_tag_ids]).to eq([tag.id])
+      expect(payload[:muted_tags]).to eq([tag.name])
     end
   end