Merge pull request #5610 from discourse/pm-tags

FEATURE: Allow staffs to tag PMs

app/assets/javascripts/discourse/controllers/composer.js.es6

@@ -140,7 +140,8 @@ export default Ember.Controller.extend({
     return !this.site.mobileView &&
             this.site.get('can_tag_topics') &&
             canEditTitle &&
-            !creatingPrivateMessage;
+            !creatingPrivateMessage &&
+            (!this.get('model.topic.isPrivateMessage') || this.site.get('can_tag_pms'));
   },
 
   @computed('model.whisper', 'model.unlistTopic')

app/assets/javascripts/discourse/controllers/topic.js.es6

@@ -104,7 +104,7 @@ export default Ember.Controller.extend(BufferedContent, {
 
   @computed('model.isPrivateMessage')
   canEditTags(isPrivateMessage) {
-    return !isPrivateMessage && this.site.get('can_tag_topics');
+    return this.site.get('can_tag_topics') && (!isPrivateMessage || this.site.get('can_tag_pms'));
   },
 
   actions: {

app/assets/javascripts/discourse/lib/render-tag.js.es6

@@ -3,7 +3,12 @@ export default function renderTag(tag, params) {
   tag = Handlebars.Utils.escapeExpression(tag);
   const classes = ['tag-' + tag, 'discourse-tag'];
   const tagName = params.tagName || "a";
-  const href = (tagName === "a" && !params.noHref) ? " href='" + Discourse.getURL("/tags/" + tag) + "' " : "";
+  let path;
+  if (tagName === "a" && !params.noHref) {
+    const current_user = Discourse.User.current();
+    path = params.isPrivateMessage ? `/u/${current_user.username}/messages/tag/${tag}` : `/tags/${tag}`;
+  }
+  const href = path ? ` href='${Discourse.getURL(path)}' ` : "";
 
   if (Discourse.SiteSettings.tag_style || params.style) {
     classes.push(params.style || Discourse.SiteSettings.tag_style);

app/assets/javascripts/discourse/lib/render-tags.js.es6

@@ -20,6 +20,7 @@ export function addTagsHtmlCallback(callback, options) {
 export default function(topic, params){
   let tags = topic.tags;
   let buffer = "";
+  const isPrivateMessage = topic.get('isPrivateMessage');
 
   if (params && params.mode === "list") {
     tags = topic.get("visibleListTags");
@@ -43,7 +44,7 @@ export default function(topic, params){
     buffer = "<div class='discourse-tags'>";
     if (tags) {
       for(let i=0; i<tags.length; i++){
-        buffer += renderTag(tags[i]) + ' ';
+        buffer += renderTag(tags[i], { isPrivateMessage }) + ' ';
       }
     }
 

app/assets/javascripts/discourse/routes/app-route-map.js.es6

@@ -96,6 +96,7 @@ export default function() {
       this.route('archive');
       this.route('group', { path: 'group/:name'});
       this.route('groupArchive', { path: 'group/:name/archive'});
+      this.route('tag', { path: 'tag/:id'});
     });
 
     this.route('preferences', { resetNamespace: true }, function() {

app/assets/javascripts/discourse/routes/user-private-messages-tag.js.es6

@@ -0,0 +1,10 @@
+import createPMRoute from "discourse/routes/build-private-messages-route";
+
+export default createPMRoute('tags', 'private-messages-tags').extend({
+    model(params) {
+      const username = this.modelFor("user").get("username_lower");
+      return this.store.findFiltered("topicList", {
+        filter: `topics/private-messages-tag/${username}/${params.id}`
+      });
+    }
+});

app/assets/javascripts/discourse/templates/components/topic-category.hbs

@@ -1,13 +1,13 @@
-{{#if topic.category.parentCategory}}
+{{#unless topic.isPrivateMessage}}
-  {{bound-category-link topic.category.parentCategory}}
+  {{#if topic.category.parentCategory}}
-{{/if}}
+    {{bound-category-link topic.category.parentCategory}}
-{{bound-category-link topic.category hideParent=true}}
+  {{/if}}
+  {{bound-category-link topic.category hideParent=true}}
+{{/unless}}
 <div class="topic-header-extra">
   {{#if siteSettings.tagging_enabled}}
     <div class="list-tags">
-      {{#each topic.tags as |t|}}
+      {{discourse-tags topic mode="list"}}
-        {{discourse-tag t}}
-      {{/each}}
     </div>
   {{/if}}
   {{#if siteSettings.topic_featured_link_enabled}}

app/assets/javascripts/discourse/templates/topic.hbs

@@ -63,9 +63,7 @@
             {{/if}}
           </h1>
 
-          {{#unless model.isPrivateMessage}}
+          {{topic-category topic=model class="topic-category"}}
-            {{topic-category topic=model class="topic-category"}}
-          {{/unless}}
         {{/if}}
       {{/topic-title}}
     {{/if}}

app/assets/stylesheets/common/base/compose.scss

@@ -189,7 +189,7 @@
   .category-input {
     display: flex;
     flex: 1 0 35%;
-    margin: 0 0 5px 10px;
+    margin: 0 5px 5px 10px;
     @media screen and (max-width: 955px) {
       flex: 1 0 100%;
       margin-left: 0;
@@ -223,7 +223,7 @@
 
   .mini-tag-chooser {
     flex: 1 1 25%;
-    margin: 0 0 5px 5px;
+    margin: 0 0 5px 0;
     background: $secondary;
     @media all and (max-width: 900px) {
       margin: 0;

app/assets/stylesheets/common/base/topic.scss

@@ -122,6 +122,10 @@ a.badge-category {
   }
 }
 
+.archetype-private_message #topic-title .edit-topic-title .tag-chooser {
+  margin-left: 19px;
+}
+
 .private_message {
   #topic-title {
     .edit-topic-title {

app/controllers/list_controller.rb

@@ -151,6 +151,7 @@ class ListController < ApplicationController
     private_messages_archive
     private_messages_group
     private_messages_group_archive
+    private_messages_tag
   }.each do |action|
     generate_message_route(action)
   end
@@ -333,6 +334,7 @@ class ListController < ApplicationController
   def build_topic_list_options
     options = {}
     params[:page] = params[:page].to_i rescue 1
+    params[:tags] = [params[:tag_id]] if params[:tag_id].present? && guardian.can_tag_pms?
 
     TopicQuery.public_valid_options.each do |key|
       options[key] = params[key]

app/models/tag.rb

@@ -16,21 +16,6 @@ class Tag < ActiveRecord::Base
 
   after_save :index_search
 
-  COUNT_ARG = "topics.id"
-
-  # Apply more activerecord filters to the tags_by_count_query, and then
-  # fetch the result with .count(Tag::COUNT_ARG).
-  #
-  # e.g., Tag.tags_by_count_query.where("topics.category_id = ?", category.id).count(Tag::COUNT_ARG)
-  def self.tags_by_count_query(opts = {})
-    q = Tag.joins("LEFT JOIN topic_tags ON tags.id = topic_tags.tag_id")
-      .joins("LEFT JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL")
-      .group("tags.id, tags.name")
-      .order('count_topics_id DESC')
-    q = q.limit(opts[:limit]) if opts[:limit]
-    q
-  end
-
   def self.ensure_consistency!
     update_topic_counts # topic_count counter cache can miscount
   end
@@ -43,7 +28,7 @@ class Tag < ActiveRecord::Base
         SELECT COUNT(topics.id) AS topic_count, tags.id AS tag_id
         FROM tags
         LEFT JOIN topic_tags ON tags.id = topic_tags.tag_id
-        LEFT JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL
+        LEFT JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL AND topics.archetype != 'private_message'
         GROUP BY tags.id
       ) x
       WHERE x.tag_id = t.id

app/models/topic_tag.rb

@@ -1,22 +1,28 @@
 class TopicTag < ActiveRecord::Base
   belongs_to :topic
-  belongs_to :tag, counter_cache: "topic_count"
+  belongs_to :tag
 
   after_create do
-    if topic&.category_id
+    if topic && topic.archetype != Archetype.private_message
-      if stat = CategoryTagStat.where(tag_id: tag_id, category_id: topic.category_id).first
+      tag.increment!(:topic_count)
-        stat.increment!(:topic_count)
+
-      else
+      if topic.category_id
-        CategoryTagStat.create(tag_id: tag_id, category_id: topic.category_id, topic_count: 1)
+        if stat = CategoryTagStat.where(tag_id: tag_id, category_id: topic.category_id).first
+          stat.increment!(:topic_count)
+        else
+          CategoryTagStat.create(tag_id: tag_id, category_id: topic.category_id, topic_count: 1)
+        end
       end
     end
   end
 
   after_destroy do
-    if topic&.category_id
+    if topic && topic.archetype != Archetype.private_message
-      if stat = CategoryTagStat.where(tag_id: tag_id, category: topic.category_id).first
+      if topic.category_id && stat = CategoryTagStat.where(tag_id: tag_id, category: topic.category_id).first
         stat.topic_count == 1 ? stat.destroy : stat.decrement!(:topic_count)
       end
+
+      tag.decrement!(:topic_count)
     end
   end
 end

app/serializers/concerns/topic_tags_mixin.rb

@@ -0,0 +1,17 @@
+module TopicTagsMixin
+  def self.included(klass)
+    klass.attributes :tags
+  end
+
+  def include_tags?
+    scope.can_see_tags?(topic)
+  end
+
+  def tags
+    topic.tags.pluck(:name)
+  end
+
+  def topic
+    object.is_a?(Topic) ? object : object.topic
+  end
+end

app/serializers/post_revision_serializer.rb

@@ -164,7 +164,7 @@ class PostRevisionSerializer < ApplicationSerializer
   end
 
   def include_tags_changes?
-    SiteSetting.tagging_enabled && previous["tags"] != current["tags"]
+    scope.can_see_tags?(topic) && previous["tags"] != current["tags"]
   end
 
   protected
@@ -197,18 +197,11 @@ class PostRevisionSerializer < ApplicationSerializer
 
       # Retrieve any `tracked_topic_fields`
       PostRevisor.tracked_topic_fields.each_key do |field|
-        if topic.respond_to?(field)
+        latest_modifications[field.to_s] = [topic.send(field)] if topic.respond_to?(field)
-          latest_modifications[field.to_s] = [topic.send(field)]
-        end
       end
 
-      if SiteSetting.topic_featured_link_enabled
+      latest_modifications["featured_link"] = [post.topic.featured_link] if SiteSetting.topic_featured_link_enabled
-        latest_modifications["featured_link"] = [post.topic.featured_link]
+      latest_modifications["tags"] = [topic.tags.pluck(:name)] if scope.can_see_tags?(topic)
-      end
-
-      if SiteSetting.tagging_enabled
-        latest_modifications["tags"] = [post.topic.tags.map(&:name)]
-      end
 
       post_revisions << PostRevision.new(
         number: post_revisions.last.number + 1,

app/serializers/search_topic_list_item_serializer.rb

@@ -1,12 +1,5 @@
 class SearchTopicListItemSerializer < ListableTopicSerializer
-  attributes :tags,
+  include TopicTagsMixin
-    :category_id
 
-  def include_tags?
+  attributes :category_id
-    SiteSetting.tagging_enabled
-  end
-
-  def tags
-    object.tags.map(&:name)
-  end
 end

app/serializers/site_serializer.rb

@@ -21,6 +21,7 @@ class SiteSerializer < ApplicationSerializer
              :topic_flag_types,
              :can_create_tag,
              :can_tag_topics,
+             :can_tag_pms,
              :tags_filter_regexp,
              :top_tags,
              :wizard_required,
@@ -106,11 +107,15 @@ class SiteSerializer < ApplicationSerializer
   end
 
   def can_create_tag
-    SiteSetting.tagging_enabled && scope.can_create_tag?
+    scope.can_create_tag?
   end
 
   def can_tag_topics
-    SiteSetting.tagging_enabled && scope.can_tag_topics?
+    scope.can_tag_topics?
+  end
+
+  def can_tag_pms
+    scope.can_tag_pms?
   end
 
   def include_tags_filter_regexp?

app/serializers/suggested_topic_serializer.rb

@@ -1,4 +1,5 @@
 class SuggestedTopicSerializer < ListableTopicSerializer
+  include TopicTagsMixin
 
   # need to embed so we have users
   # front page json gets away without embedding
@@ -7,21 +8,13 @@ class SuggestedTopicSerializer < ListableTopicSerializer
     has_one :user, serializer: BasicUserSerializer, embed: :objects
   end
 
-  attributes :archetype, :like_count, :views, :category_id, :tags, :featured_link, :featured_link_root_domain
+  attributes :archetype, :like_count, :views, :category_id, :featured_link, :featured_link_root_domain
   has_many :posters, serializer: SuggestedPosterSerializer, embed: :objects
 
   def posters
     object.posters || []
   end
 
-  def include_tags?
-    SiteSetting.tagging_enabled
-  end
-
-  def tags
-    object.tags.map(&:name)
-  end
-
   def include_featured_link?
     SiteSetting.topic_featured_link_enabled
   end

app/serializers/topic_list_item_serializer.rb

@@ -1,4 +1,5 @@
 class TopicListItemSerializer < ListableTopicSerializer
+  include TopicTagsMixin
 
   attributes :views,
              :like_count,
@@ -10,7 +11,6 @@ class TopicListItemSerializer < ListableTopicSerializer
              :pinned_globally,
              :bookmarked_post_numbers,
              :liked_post_numbers,
-             :tags,
              :featured_link,
              :featured_link_root_domain
 
@@ -66,14 +66,6 @@ class TopicListItemSerializer < ListableTopicSerializer
     object.association(:first_post).loaded?
   end
 
-  def include_tags?
-    SiteSetting.tagging_enabled
-  end
-
-  def tags
-    object.tags.map(&:name)
-  end
-
   def include_featured_link?
     SiteSetting.topic_featured_link_enabled
   end

app/serializers/topic_view_serializer.rb

@@ -4,6 +4,7 @@ require_dependency 'new_post_manager'
 class TopicViewSerializer < ApplicationSerializer
   include PostStreamSerializerMixin
   include SuggestedTopicsMixin
+  include TopicTagsMixin
   include ApplicationHelper
 
   def self.attributes_from_topic(*list)
@@ -60,7 +61,6 @@ class TopicViewSerializer < ApplicationSerializer
              :chunk_size,
              :bookmarked,
              :message_archived,
-             :tags,
              :topic_timer,
              :private_topic_timer,
              :unicode_title,
@@ -238,10 +238,6 @@ class TopicViewSerializer < ApplicationSerializer
     scope.is_staff? && NewPostManager.queue_enabled?
   end
 
-  def include_tags?
-    SiteSetting.tagging_enabled
-  end
-
   def topic_timer
     TopicTimerSerializer.new(object.topic.public_topic_timer, root: false)
   end
@@ -255,10 +251,6 @@ class TopicViewSerializer < ApplicationSerializer
     TopicTimerSerializer.new(timer, root: false)
   end
 
-  def tags
-    object.topic.tags.map(&:name)
-  end
-
   def include_featured_link?
     SiteSetting.topic_featured_link_enabled
   end

config/locales/server.en.yml

@@ -1618,6 +1618,7 @@ en:
     tags_listed_by_group: "List tags by tag group on the Tags page (/tags)."
     tag_style: "Visual style for tag badges."
     staff_tags: "A list of tags that can only be applied by staff members"
+    allow_staff_to_tag_pms: "Allow staff members to tag any personal message"
     min_trust_level_to_tag_topics: "Minimum trust level required to tag topics"
     suppress_overlapping_tags_in_list: "If tags match exact words in topic titles, don't show the tag"
     remove_muted_tags_from_latest: "Don't show topics tagged with muted tags in the latest topic list."

config/routes.rb

@@ -367,6 +367,7 @@ Discourse::Application.routes.draw do
     get "#{root_path}/:username/messages/:filter" => "user_actions#private_messages", constraints: { username: RouteFormat.username }
     get "#{root_path}/:username/messages/group/:group_name" => "user_actions#private_messages", constraints: { username: RouteFormat.username, group_name: RouteFormat.username }
     get "#{root_path}/:username/messages/group/:group_name/archive" => "user_actions#private_messages", constraints: { username: RouteFormat.username, group_name: RouteFormat.username }
+    get "#{root_path}/:username/messages/tag/:tag_id" => "user_actions#private_messages", constraints: StaffConstraint.new
     get "#{root_path}/:username.json" => "users#show", constraints: { username: RouteFormat.username }, defaults: { format: :json }
     get({ "#{root_path}/:username" => "users#show", constraints: { username: RouteFormat.username, format: /(json|html)/ } }.merge(index == 1 ? { as: 'user' } : {}))
     put "#{root_path}/:username" => "users#update", constraints: { username: RouteFormat.username }, defaults: { format: :json }
@@ -597,20 +598,20 @@ Discourse::Application.routes.draw do
   resources :similar_topics
 
   get "topics/feature_stats"
-  get "topics/created-by/:username" => "list#topics_by", as: "topics_by", constraints: { username: RouteFormat.username }
-  get "topics/private-messages/:username" => "list#private_messages", as: "topics_private_messages", constraints: { username: RouteFormat.username }
-  get "topics/private-messages-sent/:username" => "list#private_messages_sent", as: "topics_private_messages_sent", constraints: { username: RouteFormat.username }
-  get "topics/private-messages-archive/:username" => "list#private_messages_archive", as: "topics_private_messages_archive", constraints: { username: RouteFormat.username }
-  get "topics/private-messages-unread/:username" => "list#private_messages_unread", as: "topics_private_messages_unread", constraints: { username: RouteFormat.username }
-  get "topics/private-messages-group/:username/:group_name.json" => "list#private_messages_group", as: "topics_private_messages_group", constraints: {
-    username: RouteFormat.username,
-    group_name: RouteFormat.username
-  }
 
-  get "topics/private-messages-group/:username/:group_name/archive.json" => "list#private_messages_group_archive", as: "topics_private_messages_group_archive", constraints: {
+  scope "/topics", username: RouteFormat.username do
-    username: RouteFormat.username,
+    get "created-by/:username" => "list#topics_by", as: "topics_by"
-    group_name: RouteFormat.username
+    get "private-messages/:username" => "list#private_messages", as: "topics_private_messages"
-  }
+    get "private-messages-sent/:username" => "list#private_messages_sent", as: "topics_private_messages_sent"
+    get "private-messages-archive/:username" => "list#private_messages_archive", as: "topics_private_messages_archive"
+    get "private-messages-unread/:username" => "list#private_messages_unread", as: "topics_private_messages_unread"
+    get "private-messages-tag/:username/:tag_id.json" => "list#private_messages_tag", as: "topics_private_messages_tag", constraints: StaffConstraint.new
+
+    scope "/private-messages-group/:username", group_name: RouteFormat.username do
+      get ":group_name.json" => "list#private_messages_group", as: "topics_private_messages_group"
+      get ":group_name/archive.json" => "list#private_messages_group_archive", as: "topics_private_messages_group_archive"
+    end
+  end
 
   get 'embed/comments' => 'embed#comments'
   get 'embed/count' => 'embed#count'

config/site_settings.yml

@@ -1571,6 +1571,8 @@ tags:
     type: list
     client: true
     default: ''
+  allow_staff_to_tag_pms:
+    default: false
   suppress_overlapping_tags_in_list:
     default: false
     client: true

lib/discourse_tagging.rb

@@ -4,10 +4,10 @@ module DiscourseTagging
   TAGS_FILTER_REGEXP = /[\/\?#\[\]@!\$&'\(\)\*\+,;=\.%\\`^\s|\{\}"<>]+/ # /?#[]@!$&'()*+,;=.%\`^|{}"<>
 
   def self.tag_topic_by_names(topic, guardian, tag_names_arg, append: false)
-    if SiteSetting.tagging_enabled
+    if guardian.can_tag?(topic)
       tag_names = DiscourseTagging.tags_for_saving(tag_names_arg, guardian) || []
 
-      old_tag_names = topic.tags.map(&:name) || []
+      old_tag_names = topic.tags.pluck(:name) || []
       new_tag_names = tag_names - old_tag_names
       removed_tag_names = old_tag_names - tag_names
 

lib/guardian.rb

@@ -129,6 +129,16 @@ class Guardian
   alias :can_see_flags? :can_moderate?
   alias :can_close? :can_moderate?
 
+  def can_tag?(topic)
+    return false if topic.blank?
+
+    topic.private_message? ? can_tag_pms? : can_tag_topics?
+  end
+
+  def can_see_tags?(topic)
+    SiteSetting.tagging_enabled && topic.present? && (!topic.private_message? || can_tag_pms?)
+  end
+
   def can_send_activation_email?(user)
     user && is_staff? && !SiteSetting.must_approve_users?
   end

lib/guardian/tag_guardian.rb

@@ -5,7 +5,11 @@ module TagGuardian
   end
 
   def can_tag_topics?
-    user && user.has_trust_level?(SiteSetting.min_trust_level_to_tag_topics.to_i)
+    user && SiteSetting.tagging_enabled && user.has_trust_level?(SiteSetting.min_trust_level_to_tag_topics.to_i)
+  end
+
+  def can_tag_pms?
+    is_staff? && SiteSetting.tagging_enabled && SiteSetting.allow_staff_to_tag_pms
   end
 
   def can_admin_tags?

lib/topic_query.rb

@@ -269,6 +269,13 @@ class TopicQuery
     create_list(:private_messages, {}, list)
   end
 
+  def list_private_messages_tag(user)
+    list = private_messages_for(user, :all)
+    list = list.joins("JOIN topic_tags tt ON tt.topic_id = topics.id
+                      JOIN tags t ON t.id = tt.tag_id AND t.name = '#{@options[:tags][0]}'")
+    create_list(:private_messages, {}, list)
+  end
+
   def list_category_topic_ids(category)
     query = default_results(category: category.id)
     pinned_ids = query.where('pinned_at IS NOT NULL AND category_id = ?', category.id).limit(nil).order('pinned_at DESC').pluck(:id)

spec/components/topic_query_spec.rb

@@ -823,6 +823,19 @@ describe TopicQuery do
             expect(suggested_topics).to eq([private_group_topic.id, private_message.id])
           end
         end
+
+        context "by tag filter" do
+          let(:tag) { Fabricate(:tag) }
+          let!(:user) { group_user }
+
+          it 'should return only tagged topics' do
+            Fabricate(:topic_tag, topic: private_message, tag: tag)
+            Fabricate(:topic_tag, topic: private_group_topic)
+
+            expect(TopicQuery.new(user, tags: [tag.name]).list_private_messages_tag(user).topics).to eq([private_message])
+          end
+
+        end
       end
 
       context 'with some existing topics' do

spec/fabricators/topic_tag_fabricator.rb

@@ -0,0 +1,4 @@
+Fabricator(:topic_tag) do
+  tag
+  topic
+end

spec/models/tag_spec.rb

@@ -10,63 +10,19 @@ describe Tag do
     end
   end
 
+  let(:tag) { Fabricate(:tag) }
+  let(:topic) { Fabricate(:topic, tags: [tag]) }
+
   before do
     SiteSetting.tagging_enabled = true
     SiteSetting.min_trust_level_to_tag_topics = 0
   end
 
   it "can delete tags on deleted topics" do
-    tag = Fabricate(:tag)
-    topic = Fabricate(:topic, tags: [tag])
     topic.trash!
     expect { tag.destroy }.to change { Tag.count }.by(-1)
   end
 
-  describe '#tags_by_count_query' do
-    it "returns empty hash if nothing is tagged" do
-      expect(described_class.tags_by_count_query.count(Tag::COUNT_ARG)).to eq({})
-    end
-
-    context "with some tagged topics" do
-      before do
-        @topics = []
-        3.times { @topics << Fabricate(:topic) }
-        make_some_tags(count: 2)
-        @topics[0].tags << @tags[0]
-        @topics[0].tags << @tags[1]
-        @topics[1].tags << @tags[0]
-      end
-
-      it "returns tag names with topic counts in a hash" do
-        counts = described_class.tags_by_count_query.count(Tag::COUNT_ARG)
-        expect(counts[@tags[0].name]).to eq(2)
-        expect(counts[@tags[1].name]).to eq(1)
-      end
-
-      it "can be used to filter before doing the count" do
-        counts = described_class.tags_by_count_query.where("topics.id = ?", @topics[1].id).count(Tag::COUNT_ARG)
-        expect(counts).to eq(@tags[0].name => 1)
-      end
-
-      it "returns unused tags too" do
-        unused = Fabricate(:tag)
-        counts = described_class.tags_by_count_query.count(Tag::COUNT_ARG)
-        expect(counts[unused.name]).to eq(0)
-      end
-
-      it "doesn't include deleted topics in counts" do
-        deleted_topic_tag = Fabricate(:tag)
-        delete_topic = Fabricate(:topic)
-        post = Fabricate(:post, topic: delete_topic, user: delete_topic.user)
-        delete_topic.tags << deleted_topic_tag
-        PostDestroyer.new(Fabricate(:admin), post).destroy
-
-        counts = described_class.tags_by_count_query.count(Tag::COUNT_ARG)
-        expect(counts[deleted_topic_tag.name]).to eq(0)
-      end
-    end
-  end
-
   describe '#top_tags' do
     it "returns nothing if nothing has been tagged" do
       make_some_tags(tag_a_topic: false)
@@ -139,4 +95,14 @@ describe Tag do
       end
     end
   end
+
+  context "topic counts" do
+    it "should exclude private message topics" do
+      topic
+      Fabricate(:private_message_topic, tags: [tag])
+      described_class.ensure_consistency!
+      tag.reload
+      expect(tag.topic_count).to eq(1)
+    end
+  end
 end

spec/models/topic_tag_spec.rb

@@ -0,0 +1,47 @@
+require 'rails_helper'
+
+describe TopicTag do
+
+  let(:topic) { Fabricate(:topic) }
+  let(:tag) { Fabricate(:tag) }
+  let(:topic_tag) { Fabricate(:topic_tag, topic: topic, tag: tag) }
+
+  context '#after_create' do
+
+    it "tag topic_count should be increased" do
+      expect {
+        topic_tag
+      }.to change(tag, :topic_count).by(1)
+    end
+
+    it "tag topic_count should not be increased" do
+      topic.archetype = Archetype.private_message
+
+      expect {
+        topic_tag
+      }.to change(tag, :topic_count).by(0)
+    end
+
+  end
+
+  context '#after_destroy' do
+
+    it "tag topic_count should be decreased" do
+      topic_tag
+      expect {
+        topic_tag.destroy
+      }.to change(tag, :topic_count).by(-1)
+    end
+
+    it "tag topic_count should not be decreased" do
+      topic.archetype = Archetype.private_message
+      topic_tag
+
+      expect {
+        topic_tag.destroy
+      }.to change(tag, :topic_count).by(0)
+    end
+
+  end
+
+end

spec/requests/list_controller_spec.rb

@@ -65,4 +65,32 @@ RSpec.describe ListController do
       )
     end
   end
+
+  describe "filter private messages by tag" do
+    let(:user) { Fabricate(:user) }
+    let(:moderator) { Fabricate(:moderator) }
+    let(:admin) { Fabricate(:admin) }
+    let(:tag) { Fabricate(:tag) }
+    let(:private_message) { Fabricate(:private_message_topic) }
+
+    before do
+      SiteSetting.tagging_enabled = true
+      SiteSetting.allow_staff_to_tag_pms = true
+      Fabricate(:topic_tag, tag: tag, topic: private_message)
+    end
+
+    it 'should fail for non-staff users' do
+      sign_in(user)
+      get "/topics/private-messages-tag/#{user.username}/#{tag.name}.json"
+      expect(response.status).to eq(404)
+    end
+
+    it 'should be success for staff users' do
+      [moderator, admin].each do |user|
+        sign_in(user)
+        get "/topics/private-messages-tag/#{user.username}/#{tag.name}.json"
+        expect(response).to be_success
+      end
+    end
+  end
 end

spec/serializers/topic_view_serializer_spec.rb

@@ -1,6 +1,11 @@
 require 'rails_helper'
 
 describe TopicViewSerializer do
+  def serialize_topic(topic, user)
+    topic_view = TopicView.new(topic.id, user)
+    described_class.new(topic_view, scope: Guardian.new(user), root: false).as_json
+  end
+
   let(:topic) { Fabricate(:topic) }
   let(:user) { Fabricate(:user) }
 
@@ -12,8 +17,7 @@ describe TopicViewSerializer do
         topic.update!(featured_link: featured_link)
         SiteSetting.topic_featured_link_enabled = false
 
-        topic_view = TopicView.new(topic.id, user)
+        json = serialize_topic(topic, user)
-        json = described_class.new(topic_view, scope: Guardian.new(user), root: false).as_json
 
         expect(json[:featured_link]).to eq(nil)
         expect(json[:featured_link_root_domain]).to eq(nil)
@@ -24,8 +28,7 @@ describe TopicViewSerializer do
       it 'should return the right attributes' do
         topic.update!(featured_link: featured_link)
 
-        topic_view = TopicView.new(topic.id, user)
+        json = serialize_topic(topic, user)
-        json = described_class.new(topic_view, scope: Guardian.new(user), root: false).as_json
 
         expect(json[:featured_link]).to eq(featured_link)
         expect(json[:featured_link_root_domain]).to eq('discourse.org')
@@ -42,8 +45,7 @@ describe TopicViewSerializer do
 
     describe 'when loading last chunk' do
       it 'should include suggested topics' do
-        topic_view = TopicView.new(topic.id, user)
+        json = serialize_topic(topic, user)
-        json = described_class.new(topic_view, scope: Guardian.new(user), root: false).as_json
 
         expect(json[:suggested_topics].first.id).to eq(topic2.id)
       end
@@ -64,4 +66,42 @@ describe TopicViewSerializer do
       end
     end
   end
+
+  describe 'when tags added to private message topics' do
+    let(:moderator) { Fabricate(:moderator) }
+    let(:admin) { Fabricate(:admin) }
+    let(:tag) { Fabricate(:tag) }
+    let(:pm) do
+      Fabricate(:private_message_topic, tags: [tag], topic_allowed_users: [
+        Fabricate.build(:topic_allowed_user, user: moderator),
+        Fabricate.build(:topic_allowed_user, user: user)
+      ])
+    end
+
+    before do
+      SiteSetting.tagging_enabled = true
+      SiteSetting.allow_staff_to_tag_pms = true
+    end
+
+    it "should not include the tag for normal users" do
+      json = serialize_topic(pm, user)
+      expect(json[:tags]).to eq(nil)
+    end
+
+    it "should include the tag for staff users" do
+      [moderator, admin].each do |user|
+        json = serialize_topic(pm, user)
+        expect(json[:tags]).to eq([tag.name])
+      end
+    end
+
+    it "should not include the tag if pm tags disabled" do
+      SiteSetting.allow_staff_to_tag_pms = false
+
+      [moderator, admin].each do |user|
+        json = serialize_topic(pm, user)
+        expect(json[:tags]).to eq(nil)
+      end
+    end
+  end
 end