DEV: Route PM only tags to PM tags show route (#17870)

Previously, PM only tags were being routed to the public topic list with
the tag added as a filter. However, the public topic list does not fetch
PMs and hence PM only tags did not provide any value when added to the
Sidebar. This commit changes that by allowing the client to
differentiate PM only tag and thus routes the link to the PM tags show
route.

Counts for PM only tags section links are not supported as of this
commit and will be added in a follow up commit.

app/assets/javascripts/discourse/app/components/sidebar/tags-section.js

@@ -1,15 +1,17 @@
 import I18n from "I18n";
 
 import { cached } from "@glimmer/tracking";
+import Component from "@glimmer/component";
 import { inject as service } from "@ember/service";
 import { action } from "@ember/object";
 
-import Component from "@glimmer/component";
 import TagSectionLink from "discourse/lib/sidebar/tags-section/tag-section-link";
+import PMTagSectionLink from "discourse/lib/sidebar/tags-section/pm-tag-section-link";
 
 export default class SidebarTagsSection extends Component {
   @service router;
   @service topicTrackingState;
+  @service pmTopicTrackingState;
   @service currentUser;
 
   constructor() {
@@ -17,7 +19,9 @@ export default class SidebarTagsSection extends Component {
 
     this.callbackId = this.topicTrackingState.onStateChange(() => {
       this.sectionLinks.forEach((sectionLink) => {
-        sectionLink.refreshCounts();
+        if (sectionLink.refreshCounts) {
+          sectionLink.refreshCounts();
+        }
       });
     });
   }
@@ -30,13 +34,22 @@ export default class SidebarTagsSection extends Component {
   get sectionLinks() {
     const links = [];
 
-    for (const tagName of this.currentUser.sidebarTagNames) {
+    for (const tag of this.currentUser.sidebarTags) {
-      links.push(
+      if (tag.pm_only) {
-        new TagSectionLink({
+        links.push(
-          tagName,
+          new PMTagSectionLink({
-          topicTrackingState: this.topicTrackingState,
+            tag,
-        })
+            currentUser: this.currentUser,
-      );
+          })
+        );
+      } else {
+        links.push(
+          new TagSectionLink({
+            tag,
+            topicTrackingState: this.topicTrackingState,
+          })
+        );
+      }
     }
 
     return links;

app/assets/javascripts/discourse/app/controllers/preferences/sidebar.js

@@ -12,24 +12,29 @@ export default class extends Controller {
   @action
   save() {
     const initialSidebarCategoryIds = this.model.sidebarCategoryIds;
-    const initialSidebarTagNames = this.model.sidebarTagNames;
-
-    this.model.set("sidebar_tag_names", this.selectedSidebarTagNames);
 
     this.model.set(
       "sidebarCategoryIds",
       this.selectedSidebarCategories.mapBy("id")
     );
 
+    this.model.set("sidebar_tag_names", this.selectedSidebarTagNames);
+
     this.model
       .save()
-      .then(() => {
+      .then((result) => {
+        if (result.user.sidebar_tags) {
+          this.model.set("sidebar_tags", result.user.sidebar_tags);
+        }
+
         this.saved = true;
       })
       .catch((error) => {
         this.model.set("sidebarCategoryIds", initialSidebarCategoryIds);
-        this.model.set("sidebar_tag_names", initialSidebarTagNames);
         popupAjaxError(error);
+      })
+      .finally(() => {
+        this.model.set("sidebar_tag_names", []);
       });
   }
 }

app/assets/javascripts/discourse/app/lib/sidebar/tags-section/pm-tag-section-link.js

@@ -0,0 +1,22 @@
+export default class PMTagSectionLink {
+  constructor({ tag, currentUser }) {
+    this.tag = tag;
+    this.currentUser = currentUser;
+  }
+
+  get name() {
+    return this.tag.name;
+  }
+
+  get models() {
+    return [this.currentUser, this.tag.name];
+  }
+
+  get route() {
+    return "userPrivateMessages.tagsShow";
+  }
+
+  get text() {
+    return this.tag.name;
+  }
+}

app/assets/javascripts/discourse/app/lib/sidebar/tags-section/tag-section-link.js

@@ -8,8 +8,8 @@ export default class TagSectionLink {
   @tracked totalUnread = 0;
   @tracked totalNew = 0;
 
-  constructor({ tagName, topicTrackingState }) {
+  constructor({ tag, topicTrackingState }) {
-    this.tagName = tagName;
+    this.tagName = tag.name;
     this.topicTrackingState = topicTrackingState;
     this.refreshCounts();
   }
@@ -31,18 +31,24 @@ export default class TagSectionLink {
     return this.tagName;
   }
 
-  get model() {
+  get models() {
-    return this.tagName;
+    return [this.tagName];
+  }
+
+  get route() {
+    if (this.totalUnread > 0) {
+      return "tag.showUnread";
+    } else if (this.totalNew > 0) {
+      return "tag.showNew";
+    } else {
+      return "tag.show";
+    }
   }
 
   get currentWhen() {
     return "tag.show tag.showNew tag.showUnread tag.showTop";
   }
 
-  get route() {
-    return "tag.show";
-  }
-
   get text() {
     return this.tagName;
   }
@@ -58,14 +64,4 @@ export default class TagSectionLink {
       });
     }
   }
-
-  get route() {
-    if (this.totalUnread > 0) {
-      return "tag.showUnread";
-    } else if (this.totalNew > 0) {
-      return "tag.showNew";
-    } else {
-      return "tag.show";
-    }
-  }
 }

app/assets/javascripts/discourse/app/models/user.js

@@ -1,7 +1,7 @@
 import EmberObject, { computed, get, getProperties } from "@ember/object";
 import cookie, { removeCookie } from "discourse/lib/cookie";
 import { defaultHomepage, escapeExpression } from "discourse/lib/utilities";
-import { alias, equal, filterBy, gt, or } from "@ember/object/computed";
+import { alias, equal, filterBy, gt, mapBy, or } from "@ember/object/computed";
 import getURL, { getURLWithCDN } from "discourse-common/lib/get-url";
 import { A } from "@ember/array";
 import Badge from "discourse/models/badge";
@@ -314,15 +314,23 @@ const User = RestModel.extend({
 
   sidebarCategoryIds: alias("sidebar_category_ids"),
 
-  @discourseComputed("sidebar_tag_names.[]")
+  @discourseComputed("sidebar_tags.[]")
-  sidebarTagNames(sidebarTagNames) {
+  sidebarTags(sidebarTags) {
-    if (!sidebarTagNames || sidebarTagNames.length === 0) {
+    if (!sidebarTags || sidebarTags.length === 0) {
       return [];
     }
 
-    return sidebarTagNames;
+    if (this.siteSettings.tags_sort_alphabetically) {
+      return sidebarTags.sort((a, b) => {
+        return a.name.localeCompare(b);
+      });
+    } else {
+      return sidebarTags;
+    }
   },
 
+  sidebarTagNames: mapBy("sidebarTags", "name"),
+
   @discourseComputed("sidebar_category_ids.[]")
   sidebarCategories(sidebarCategoryIds) {
     if (!sidebarCategoryIds || sidebarCategoryIds.length === 0) {
@@ -446,6 +454,7 @@ const User = RestModel.extend({
         );
         User.current().setProperties(userProps);
         this.setProperties(updatedState);
+        return result;
       })
       .finally(() => {
         this.set("isSaving", false);

app/assets/javascripts/discourse/app/templates/components/sidebar/tags-section.hbs

@@ -16,7 +16,7 @@
         @content={{sectionLink.text}}
         @currentWhen={{sectionLink.currentWhen}}
         @badgeText={{sectionLink.badgeText}}
-        @model={{sectionLink.model}}>
+        @models={{sectionLink.models}} >
       </Sidebar::SectionLink>
     {{/each}}
   {{else}}

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

@@ -57,7 +57,7 @@ acceptance(
 acceptance("Sidebar - Categories Section", function (needs) {
   needs.user({
     sidebar_category_ids: [],
-    sidebar_tag_names: [],
+    sidebar_tags: [],
   });
 
   needs.settings({

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

@@ -42,7 +42,18 @@ acceptance("Sidebar - Tags section", function (needs) {
     tracked_tags: ["tag1"],
     watched_tags: ["tag2", "tag3"],
     watching_first_post_tags: [],
-    sidebar_tag_names: ["tag1", "tag2", "tag3"],
+    sidebar_tags: [
+      { name: "tag1", pm_only: false },
+      { name: "tag2", pm_only: false },
+      {
+        name: "tag3",
+        pm_only: false,
+      },
+      {
+        name: "tag4",
+        pm_only: true,
+      },
+    ],
   });
 
   needs.pretender((server, helper) => {
@@ -52,6 +63,20 @@ acceptance("Sidebar - Tags section", function (needs) {
       });
     });
 
+    server.get("/topics/private-messages-tags/:username/:tagId", () => {
+      const topics = [
+        { id: 1, posters: [] },
+        { id: 2, posters: [] },
+        { id: 3, posters: [] },
+      ];
+
+      return helper.response({
+        topic_list: {
+          topics,
+        },
+      });
+    });
+
     ["latest", "top", "new", "unread"].forEach((type) => {
       server.get(`/tag/:tagId/l/${type}.json`, () => {
         return helper.response(
@@ -85,7 +110,7 @@ acceptance("Sidebar - Tags section", function (needs) {
 
   test("section content when user has not added any tags", async function (assert) {
     updateCurrentUser({
-      sidebar_tag_names: [],
+      sidebar_tags: [],
     });
 
     await visit("/");
@@ -106,8 +131,8 @@ acceptance("Sidebar - Tags section", function (needs) {
 
     assert.strictEqual(
       count(".sidebar-section-tags .sidebar-section-link"),
-      3,
+      4,
-      "3 section links under the section"
+      "4 section links under the section"
     );
 
     assert.strictEqual(
@@ -167,6 +192,29 @@ acceptance("Sidebar - Tags section", function (needs) {
     );
   });
 
+  test("private message tag section links for user", async function (assert) {
+    await visit("/");
+
+    await click(".sidebar-section-link-tag4");
+
+    assert.strictEqual(
+      currentURL(),
+      "/u/eviltrout/messages/tags/tag4",
+      "it should transition to user's private message tag4 tag page"
+    );
+
+    assert.strictEqual(
+      count(".sidebar-section-tags .sidebar-section-link.active"),
+      1,
+      "only one link is marked as active"
+    );
+
+    assert.ok(
+      exists(`.sidebar-section-link-tag4.active`),
+      "the tag4 section link is marked as active"
+    );
+  });
+
   test("visiting tag discovery top route", async function (assert) {
     await visit(`/tag/tag1/l/top`);
 

app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js

@@ -12,7 +12,7 @@ import selectKit from "discourse/tests/helpers/select-kit-helper";
 acceptance("User Preferences - Sidebar", function (needs) {
   needs.user({
     sidebar_category_ids: [],
-    sidebar_tag_names: [],
+    sidebar_tags: [],
   });
 
   needs.settings({
@@ -39,7 +39,14 @@ acceptance("User Preferences - Sidebar", function (needs) {
         // This request format will cause an error
         return helper.response(400, {});
       } else {
-        return helper.response({ user: {} });
+        return helper.response({
+          user: {
+            sidebar_tags: [
+              { name: "monkey", pm_only: false },
+              { name: "gazelle", pm_only: false },
+            ],
+          },
+        });
       }
     });
   });
@@ -121,7 +128,7 @@ acceptance("User Preferences - Sidebar", function (needs) {
   });
 
   test("user encountering error when adding tags to sidebar", async function (assert) {
-    updateCurrentUser({ sidebar_tag_names: ["monkey"] });
+    updateCurrentUser({ sidebar_tags: [{ name: "monkey", pm_only: false }] });
 
     await visit("/");
 

app/serializers/concerns/user_sidebar_tags_mixin.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module UserSidebarTagsMixin
+  def self.included(base)
+    base.has_many :sidebar_tags, serializer: Sidebar::TagSerializer, embed: :objects
+  end
+
+  def include_sidebar_tags?
+    SiteSetting.enable_experimental_sidebar_hamburger && SiteSetting.tagging_enabled
+  end
+end

app/serializers/current_user_serializer.rb

@@ -2,6 +2,7 @@
 
 class CurrentUserSerializer < BasicUserSerializer
   include UserTagNotificationsMixin
+  include UserSidebarTagsMixin
 
   attributes :name,
              :unread_notifications,
@@ -75,7 +76,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :pending_posts_count,
              :status,
              :sidebar_category_ids,
-             :sidebar_tag_names,
+             :sidebar_tags,
              :likes_notifications_disabled,
              :grouped_unread_high_priority_notifications,
              :redesigned_user_menu_enabled
@@ -315,14 +316,6 @@ class CurrentUserSerializer < BasicUserSerializer
     SiteSetting.enable_experimental_sidebar_hamburger
   end
 
-  def sidebar_tag_names
-    object.sidebar_tags.pluck(:name)
-  end
-
-  def include_sidebar_tag_names?
-    include_sidebar_category_ids? && SiteSetting.tagging_enabled
-  end
-
   def include_status?
     SiteSetting.enable_user_status && object.has_status?
   end

app/serializers/sidebar/tag_serializer.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Sidebar
+  class TagSerializer < ::ApplicationSerializer
+    attributes :name, :pm_only
+
+    def pm_only
+      object.topic_count == 0 && object.pm_topic_count > 0
+    end
+  end
+end

app/serializers/user_card_serializer.rb

@@ -16,7 +16,11 @@ class UserCardSerializer < BasicUserSerializer
     attributes(*attrs)
     attrs.each do |attr|
       define_method "include_#{attr}?" do
-        can_edit
+        if defined?(super)
+          super() && can_edit
+        else
+          can_edit
+        end
       end
     end
   end

app/serializers/user_serializer.rb

@@ -2,6 +2,7 @@
 
 class UserSerializer < UserCardSerializer
   include UserTagNotificationsMixin
+  include UserSidebarTagsMixin
 
   attributes :bio_raw,
              :bio_cooked,
@@ -62,7 +63,8 @@ class UserSerializer < UserCardSerializer
                      :user_api_keys,
                      :user_auth_tokens,
                      :user_notification_schedule,
-                     :use_logo_small_as_avatar
+                     :use_logo_small_as_avatar,
+                     :sidebar_tags
 
   untrusted_attributes :bio_raw,
                        :bio_cooked,

spec/serializers/current_user_serializer_spec.rb

@@ -220,45 +220,39 @@ RSpec.describe CurrentUserSerializer do
     end
   end
 
-  describe '#sidebar_tag_names' do
+  describe '#sidebar_tags' do
     fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user) }
     fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user) }
 
-    it "is not included when SiteSeting.enable_experimental_sidebar_hamburger is false" do
-      SiteSetting.enable_experimental_sidebar_hamburger = false
-
-      json = serializer.as_json
-
-      expect(json[:sidebar_tag_names]).to eq(nil)
-    end
-
-    it "is not included when SiteSeting.tagging_enabled is false" do
-      SiteSetting.enable_experimental_sidebar_hamburger = true
-      SiteSetting.tagging_enabled = false
-
-      json = serializer.as_json
-
-      expect(json[:sidebar_tag_names]).to eq(nil)
-    end
-
     it "is not included when experimental sidebar has not been enabled" do
       SiteSetting.enable_experimental_sidebar_hamburger = false
       SiteSetting.tagging_enabled = true
 
       json = serializer.as_json
 
-      expect(json[:sidebar_tag_names]).to eq(nil)
+      expect(json[:sidebar_tags]).to eq(nil)
     end
 
-    it "is present when experimental sidebar has been enabled" do
+    it "is not included when tagging has not been enabled" do
       SiteSetting.enable_experimental_sidebar_hamburger = true
-      SiteSetting.tagging_enabled = true
+      SiteSetting.tagging_enabled = false
 
       json = serializer.as_json
 
-      expect(json[:sidebar_tag_names]).to contain_exactly(
+      expect(json[:sidebar_tags]).to eq(nil)
-        tag_sidebar_section_link.linkable.name,
+    end
-        tag_sidebar_section_link_2.linkable.name
+
+    it "is present when experimental sidebar and tagging has been enabled" do
+      SiteSetting.enable_experimental_sidebar_hamburger = true
+      SiteSetting.tagging_enabled = true
+
+      tag_sidebar_section_link_2.linkable.update!(pm_topic_count: 5, topic_count: 0)
+
+      json = serializer.as_json
+
+      expect(json[:sidebar_tags]).to contain_exactly(
+        { name: tag_sidebar_section_link.linkable.name, pm_only: false },
+        { name: tag_sidebar_section_link_2.linkable.name, pm_only: true }
       )
     end
   end

spec/serializers/user_serializer_spec.rb

@@ -376,4 +376,52 @@ RSpec.describe UserSerializer do
       expect(json[:user_api_keys][2][:id]).to eq(user_api_key_2.id)
     end
   end
+
+  describe '#sidebar_tags' do
+    fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user) }
+    fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user) }
+
+    context 'when viewing self' do
+      subject(:json) { UserSerializer.new(user, scope: Guardian.new(user), root: false).as_json }
+
+      it "is not included when SiteSeting.enable_experimental_sidebar_hamburger is false" do
+        SiteSetting.enable_experimental_sidebar_hamburger = false
+        SiteSetting.tagging_enabled = true
+
+        expect(json[:sidebar_tags]).to eq(nil)
+      end
+
+      it "is not included when SiteSeting.tagging_enabled is false" do
+        SiteSetting.enable_experimental_sidebar_hamburger = true
+        SiteSetting.tagging_enabled = false
+
+        expect(json[:sidebar_tags]).to eq(nil)
+      end
+
+      it "is present when experimental sidebar and tagging has been enabled" do
+        SiteSetting.enable_experimental_sidebar_hamburger = true
+        SiteSetting.tagging_enabled = true
+
+        tag_sidebar_section_link_2.linkable.update!(pm_topic_count: 5, topic_count: 0)
+
+        expect(json[:sidebar_tags]).to contain_exactly(
+          { name: tag_sidebar_section_link.linkable.name, pm_only: false },
+          { name: tag_sidebar_section_link_2.linkable.name, pm_only: true }
+        )
+      end
+    end
+
+    context 'when viewing another user' do
+      fab!(:user2) { Fabricate(:user) }
+
+      subject(:json) { UserSerializer.new(user, scope: Guardian.new(user2), root: false).as_json }
+
+      it "is not present even when experimental sidebar and tagging has been enabled" do
+        SiteSetting.enable_experimental_sidebar_hamburger = true
+        SiteSetting.tagging_enabled = true
+
+        expect(json[:sidebar_tags]).to eq(nil)
+      end
+    end
+  end
 end