REFACTOR: New spec tests and code improvement

app/models/tag.rb

@@ -28,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 AND topics.archetype != "private_message"
+        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/serializers/concerns/topic_tags_mixin.rb

@@ -4,10 +4,14 @@ module TopicTagsMixin
   end
 
   def include_tags?
-    SiteSetting.tagging_enabled && (!object.private_message? || scope.can_tag_pms?)
+    scope.can_see_tags?(topic)
   end
 
   def tags
-    object.tags.pluck(:name)
+    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"] && (!topic.private_message? || scope.can_tag_pms?)
+    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)]
-        end
+        latest_modifications[field.to_s] = [topic.send(field)] if topic.respond_to?(field)
       end
 
-      if SiteSetting.topic_featured_link_enabled
-        latest_modifications["featured_link"] = [post.topic.featured_link]
-      end
-
-      if SiteSetting.tagging_enabled && (!topic.private_message? || scope.can_tag_pms?)
-        latest_modifications["tags"] = [post.topic.tags.map(&:name)]
-      end
+      latest_modifications["featured_link"] = [post.topic.featured_link] if SiteSetting.topic_featured_link_enabled
+      latest_modifications["tags"] = [topic.tags.pluck(:name)] if scope.can_see_tags?(topic)
 
       post_revisions << PostRevision.new(
         number: post_revisions.last.number + 1,

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 && (!object.topic.private_message? || scope.can_tag_pms?)
-  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

lib/guardian.rb

@@ -129,10 +129,14 @@ class Guardian
   alias :can_see_flags? :can_moderate?
   alias :can_close? :can_moderate?
 
-  def can_tag?(obj)
-    return false unless obj && obj.is_a?(Topic)
+  def can_tag?(topic)
+    return false if topic.blank?
 
-    obj.private_message? ? can_tag_pms? : can_tag_topics?
+    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)

lib/topic_query.rb

@@ -271,9 +271,8 @@ class TopicQuery
 
   def list_private_messages_tag(user)
     list = private_messages_for(user, :all)
-    tag_id = Tag.where('name ilike ?', @options[:tags][0]).pluck(:id).first
-    list = list.joins("JOIN topic_tags tt ON tt.topic_id = topics.id AND
-                      tt.tag_id = #{tag_id}")
+    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
 

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,14 +10,15 @@ 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
@@ -94,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

@@ -38,4 +38,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

@@ -67,17 +67,17 @@ describe TopicViewSerializer do
     end
   end
 
-  let(:user) { Fabricate(:user) }
-  let(:moderator) { Fabricate(:moderator) }
-  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
-
   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
@@ -89,21 +89,19 @@ describe TopicViewSerializer do
     end
 
     it "should include the tag for staff users" do
-      json = serialize_topic(pm, moderator)
-      expect(json[:tags]).to eq([tag.name])
-
-      json = serialize_topic(pm, Fabricate(:admin))
-      expect(json[:tags]).to eq([tag.name])
+      [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
 
-      json = serialize_topic(pm, moderator)
-      expect(json[:tags]).to eq(nil)
-
-      json = serialize_topic(pm, Fabricate(:admin))
-      expect(json[:tags]).to eq(nil)
+      [moderator, admin].each do |user|
+        json = serialize_topic(pm, user)
+        expect(json[:tags]).to eq(nil)
+      end
     end
   end
 end