FEATURE: option for tags in a tag group to be visible only to staff

app/assets/javascripts/discourse/models/tag-group.js.es6

@@ -24,7 +24,8 @@ const TagGroup = RestModel.extend({
         name: this.get('name'),
         tag_names: this.get('tag_names'),
         parent_tag_name: this.get('parent_tag_name') ? this.get('parent_tag_name') : undefined,
-        one_per_topic: this.get('one_per_topic')
+        one_per_topic: this.get('one_per_topic'),
+        permissions: this.get('visible_only_to_staff') ? {"staff": "1"} : {"everyone": "1"}
       },
       type: isNew ? 'POST' : 'PUT'
     }).then(function(result) {

app/assets/javascripts/discourse/templates/tag-groups-show.hbs

@@ -28,6 +28,13 @@
     </label>
   </section>
 
+  <section class="group-visibility">
+    <label>
+      {{input type="checkbox" checked=model.visible_only_to_staff name="visible_only_to_staff"}}
+      {{i18n 'tagging.groups.visible_only_to_staff'}}
+    </label>
+  </section>
+
   <button {{action "save"}} disabled={{model.disableSave}} class='btn'>{{i18n 'tagging.groups.save'}}</button>
   <button {{action "destroy"}} disabled={{model.disableSave}} class='btn btn-danger'>{{d-icon "trash-o"}} {{i18n 'tagging.groups.delete'}}</button>
   <span class="saving {{unless model.savingStatus 'hidden'}}">{{model.savingStatus}}</span>

app/controllers/tag_groups_controller.rb

@@ -70,7 +70,20 @@ class TagGroupsController < ApplicationController
     end
 
     def tag_groups_params
-      result = params.permit(:id, :name, :one_per_topic, tag_names: [], parent_tag_name: [])
+      if permissions = params[:permissions]
+        permissions.each do |k, v|
+          permissions[k] = v.to_i
+        end
+      end
+
+      result = params.permit(
+        :id,
+        :name,
+        :one_per_topic,
+        tag_names: [],
+        parent_tag_name: [],
+        permissions: [*permissions&.keys]
+      )
       result[:tag_names] ||= []
       result[:parent_tag_name] ||= []
       result[:one_per_topic] = (params[:one_per_topic] == "true")

app/controllers/tags_controller.rb

@@ -44,7 +44,10 @@ class TagsController < ::ApplicationController
             extras: { tag_groups: grouped_tag_counts }
           }
         else
-          unrestricted_tags = Tag.where("tags.id NOT IN (select tag_id from category_tags) AND tags.topic_count > 0")
+          unrestricted_tags = DiscourseTagging.filter_visible(
+            Tag.where("tags.id NOT IN (select tag_id from category_tags) AND tags.topic_count > 0"),
+            guardian
+          )
 
           categories = Category.where("id in (select category_id from category_tags)")
             .where("id in (?)", guardian.allowed_category_ids)

app/models/category.rb

@@ -321,6 +321,30 @@ SQL
     end
   end
 
+  def self.resolve_permissions(permissions)
+    read_restricted = true
+
+    everyone = Group::AUTO_GROUPS[:everyone]
+    full = CategoryGroup.permission_types[:full]
+
+    mapped = permissions.map do |group, permission|
+      group_id = Group.group_id_from_param(group)
+      permission = CategoryGroup.permission_types[permission] unless permission.is_a?(Integer)
+
+      [group_id, permission]
+    end
+
+    mapped.each do |group, permission|
+      if group == everyone && permission == full
+        return [false, []]
+      end
+
+      read_restricted = false if group == everyone
+    end
+
+    [read_restricted, mapped]
+  end
+
   def allowed_tags=(tag_names_arg)
     DiscourseTagging.add_or_create_tags_by_name(self, tag_names_arg, unlimited: true)
   end
@@ -379,33 +403,6 @@ SQL
     self.update_attributes(latest_topic_id: latest_topic_id, latest_post_id: latest_post_id)
   end
 
-  def self.resolve_permissions(permissions)
-    read_restricted = true
-
-    everyone = Group::AUTO_GROUPS[:everyone]
-    full = CategoryGroup.permission_types[:full]
-
-    mapped = permissions.map do |group, permission|
-      group = group.id if group.is_a?(Group)
-
-      # subtle, using Group[] ensures the group exists in the DB
-      group = Group[group.to_sym].id unless group.is_a?(Integer)
-      permission = CategoryGroup.permission_types[permission] unless permission.is_a?(Integer)
-
-      [group, permission]
-    end
-
-    mapped.each do |group, permission|
-      if group == everyone && permission == full
-        return [false, []]
-      end
-
-      read_restricted = false if group == everyone
-    end
-
-    [read_restricted, mapped]
-  end
-
   def self.query_parent_category(parent_slug)
     self.where(slug: parent_slug, parent_category_id: nil).pluck(:id).first ||
     self.where(id: parent_slug.to_i).pluck(:id).first

app/models/group.rb

@@ -434,6 +434,15 @@ class Group < ActiveRecord::Base
     end
   end
 
+  # given something that might be a group name, id, or record, return the group id
+  def self.group_id_from_param(group_param)
+    return group_param.id if group_param.is_a?(Group)
+    return group_param if group_param.is_a?(Integer)
+
+    # subtle, using Group[] ensures the group exists in the DB
+    Group[group_param.to_sym].id
+  end
+
   def self.builtin
     Enum.new(:moderators, :admins, :trust_level_1, :trust_level_2)
   end

app/models/tag.rb

@@ -46,11 +46,14 @@ class Tag < ActiveRecord::Base
 
     return [] if scope_category_ids.empty?
 
+    filter_sql = guardian&.is_staff? ? '' : (' AND ' + DiscourseTagging.filter_visible_sql)
+
     tag_names_with_counts = Tag.exec_sql <<~SQL
       SELECT tags.name as tag_name, SUM(stats.topic_count) AS sum_topic_count
         FROM category_tag_stats stats
   INNER JOIN tags ON stats.tag_id = tags.id AND stats.topic_count > 0
        WHERE stats.category_id in (#{scope_category_ids.join(',')})
+       #{filter_sql}
     GROUP BY tags.name
     ORDER BY sum_topic_count DESC, tag_name ASC
        LIMIT #{limit}

app/models/tag_group.rb

@@ -5,9 +5,14 @@ class TagGroup < ActiveRecord::Base
   has_many :tags, through: :tag_group_memberships
   has_many :category_tag_groups, dependent: :destroy
   has_many :categories, through: :category_tag_groups
+  has_many :tag_group_permissions, dependent: :destroy
 
   belongs_to :parent_tag, class_name: 'Tag'
 
+  before_save :apply_permissions
+
+  attr_accessor :permissions
+
   def tag_names=(tag_names_arg)
     DiscourseTagging.add_or_create_tags_by_name(self, tag_names_arg, unlimited: true)
   end
@@ -22,13 +27,47 @@ class TagGroup < ActiveRecord::Base
     end
   end
 
+  def permissions=(permissions)
+    @permissions = TagGroup.resolve_permissions(permissions)
+  end
+
+  def self.resolve_permissions(permissions)
+    everyone_group_id = Group::AUTO_GROUPS[:everyone]
+    full = TagGroupPermission.permission_types[:full]
+
+    mapped = permissions.map do |group, permission|
+      group_id = Group.group_id_from_param(group)
+      permission = TagGroupPermission.permission_types[permission] unless permission.is_a?(Integer)
+
+      return [] if group_id == everyone_group_id && permission == full
+
+      [group_id, permission]
+    end
+  end
+
+  def apply_permissions
+    if @permissions
+      tag_group_permissions.destroy_all
+      @permissions.each do |group_id, permission_type|
+        tag_group_permissions.build(group_id: group_id, permission_type: permission_type)
+      end
+      @permissions = nil
+    end
+  end
+
+  def visible_only_to_staff
+    # currently only "everyone" and "staff" groups are supported
+    tag_group_permissions.count > 0
+  end
+
   def self.allowed(guardian)
     if guardian.is_staff?
       TagGroup
     else
       category_permissions_filter = <<~SQL
-        id IN ( SELECT tag_group_id FROM category_tag_groups WHERE category_id IN (?))
-        OR id NOT IN (SELECT tag_group_id FROM category_tag_groups)
+        (id IN ( SELECT tag_group_id FROM category_tag_groups WHERE category_id IN (?))
+        OR id NOT IN (SELECT tag_group_id FROM category_tag_groups))
+        AND id NOT IN (SELECT tag_group_id FROM tag_group_permissions)
       SQL
 
       TagGroup.where(category_permissions_filter, guardian.allowed_category_ids)

app/models/tag_group_permission.rb

@@ -0,0 +1,9 @@
+# Who can see and use tags belonging to a tag group.
+class TagGroupPermission < ActiveRecord::Base
+  belongs_to :tag_group
+  belongs_to :group
+
+  def self.permission_types
+    @permission_types ||= Enum.new(full: 1) #, see: 2
+  end
+end

app/serializers/concerns/topic_tags_mixin.rb

@@ -9,7 +9,7 @@ module TopicTagsMixin
 
   def tags
     # Calling method `pluck` along with `includes` causing N+1 queries
-    topic.tags.map(&:name)
+    DiscourseTagging.filter_visible(topic.tags, scope).map(&:name)
   end
 
   def topic

app/serializers/post_revision_serializer.rb

@@ -160,7 +160,11 @@ class PostRevisionSerializer < ApplicationSerializer
   end
 
   def tags_changes
-    { previous: previous["tags"], current: current["tags"] }
+    changes = {
+      previous: filter_visible_tags(previous["tags"]),
+      current: filter_visible_tags(current["tags"])
+    }
+    changes[:previous] == changes[:current] ? nil : changes
   end
 
   def include_tags_changes?
@@ -250,4 +254,9 @@ class PostRevisionSerializer < ApplicationSerializer
       object.user || Discourse.system_user
     end
 
+    def filter_visible_tags(tags)
+      @hidden_tag_names ||= DiscourseTagging.hidden_tag_names(scope)
+      tags.is_a?(Array) ? (tags - @hidden_tag_names) : tags
+    end
+
 end

app/serializers/tag_group_serializer.rb

@@ -1,5 +1,5 @@
 class TagGroupSerializer < ApplicationSerializer
-  attributes :id, :name, :tag_names, :parent_tag_name, :one_per_topic
+  attributes :id, :name, :tag_names, :parent_tag_name, :one_per_topic, :visible_only_to_staff
 
   def tag_names
     object.tags.map(&:name).sort

config/locales/client.en.yml

@@ -2662,6 +2662,7 @@ en:
         save: "Save"
         delete: "Delete"
         confirm_delete: "Are you sure you want to delete this tag group?"
+        visible_only_to_staff: "Tags are visible only to staff"
 
       topics:
         none:

db/migrate/20180323154826_create_tag_group_permissions.rb

@@ -0,0 +1,10 @@
+class CreateTagGroupPermissions < ActiveRecord::Migration[5.1]
+  def change
+    create_table :tag_group_permissions do |t|
+      t.references :tag_group,  null: false
+      t.references :group, null: false
+      t.integer :permission_type, default: 1, null: false
+      t.timestamps null: false
+    end
+  end
+end

lib/discourse_tagging.rb

@@ -76,7 +76,6 @@ module DiscourseTagging
       term.gsub!("_", "\\_")
       term = clean_tag(term)
       query = query.where('tags.name like ?', "%#{term}%")
-      # query = query.where('tags.id NOT IN (?)', selected_tag_ids) unless selected_tag_ids.empty?
     end
 
     # Filters for category-specific tags:
@@ -152,7 +151,36 @@ module DiscourseTagging
       end
     end
 
+    if guardian.nil? || guardian.is_staff?
       query
+    else
+      filter_visible(query, guardian)
+    end
+  end
+
+  def self.filter_visible(query, guardian = nil)
+    if !guardian&.is_staff? && TagGroupPermission.where(group_id: Group::AUTO_GROUPS[:staff]).exists?
+      query.where(filter_visible_sql)
+    else
+      query
+    end
+  end
+
+  def self.filter_visible_sql
+    @filter_visible_sql ||= <<~SQL
+      tags.id NOT IN (
+        SELECT tgm.tag_id
+          FROM tag_group_memberships tgm
+    INNER JOIN tag_group_permissions tgp
+            ON tgp.tag_group_id = tgm.tag_group_id
+           AND tgp.group_id = #{Group::AUTO_GROUPS[:staff]})
+    SQL
+  end
+
+  def self.hidden_tag_names(guardian = nil)
+    return [] if guardian&.is_staff? || !TagGroupPermission.where(group_id: Group::AUTO_GROUPS[:staff]).exists?
+    tag_group_ids = TagGroupPermission.where(group_id: Group::AUTO_GROUPS[:staff]).pluck(:tag_group_id)
+    Tag.includes(:tag_groups).where('tag_group_id in (?)', tag_group_ids).pluck(:name)
   end
 
   def self.clean_tag(tag)

spec/components/discourse_tagging_spec.rb

@@ -7,11 +7,12 @@ require 'discourse_tagging'
 
 describe DiscourseTagging do
 
+  let(:admin) { Fabricate(:admin) }
   let(:user)  { Fabricate(:user) }
 
-  let!(:tag1) { Fabricate(:tag, name: "tag1") }
-  let!(:tag2) { Fabricate(:tag, name: "tag2") }
-  let!(:tag3) { Fabricate(:tag, name: "tag3") }
+  let!(:tag1) { Fabricate(:tag, name: "fun") }
+  let!(:tag2) { Fabricate(:tag, name: "fun2") }
+  let!(:tag3) { Fabricate(:tag, name: "fun3") }
 
   before do
     SiteSetting.tagging_enabled = true
@@ -25,7 +26,7 @@ describe DiscourseTagging do
         tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user),
           selected_tags: [tag2.name],
           for_input: true,
-          term: 'tag'
+          term: 'fun'
         ).map(&:name)
         expect(tags).to contain_exactly(tag1.name, tag3.name)
       end
@@ -37,6 +38,41 @@ describe DiscourseTagging do
         ).map(&:name)
         expect(tags).to contain_exactly(tag1.name, tag3.name)
       end
+
+      context 'with tags visible only to staff' do
+        let(:hidden_tag) { Fabricate(:tag) }
+        let!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+
+        it 'should return all tags to staff' do
+          tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(admin)).to_a
+          expect(tags).to contain_exactly(tag1, tag2, tag3, hidden_tag)
+          expect(tags.size).to eq(4)
+        end
+
+        it 'should not return hidden tag to non-staff' do
+          tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user)).to_a
+          expect(tags).to contain_exactly(tag1, tag2, tag3)
+          expect(tags.size).to eq(3)
+        end
+      end
+    end
+  end
+
+  describe 'filter_visible' do
+    let(:hidden_tag) { Fabricate(:tag) }
+    let!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+    let(:topic) { Fabricate(:topic, tags: [tag1, tag2, tag3, hidden_tag]) }
+
+    it 'returns all tags to staff' do
+      tags = DiscourseTagging.filter_visible(topic.tags, Guardian.new(admin))
+      expect(tags.size).to eq(4)
+      expect(tags).to contain_exactly(tag1, tag2, tag3, hidden_tag)
+    end
+
+    it 'does not return hidden tags to non-staff' do
+      tags = DiscourseTagging.filter_visible(topic.tags, Guardian.new(user))
+      expect(tags.size).to eq(3)
+      expect(tags).to contain_exactly(tag1, tag2, tag3)
     end
   end
 end

spec/models/tag_spec.rb

@@ -94,6 +94,24 @@ describe Tag do
         expect(described_class.top_tags.sort).to eq([@tags[0].name, @tags[1].name, @tags[2].name].sort)
       end
     end
+
+    context "with hidden tags" do
+      let(:hidden_tag) { Fabricate(:tag, name: 'hidden') }
+      let!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+      let!(:topic2) { Fabricate(:topic, tags: [tag, hidden_tag]) }
+
+      it "returns all tags to staff" do
+        expect(Tag.top_tags(guardian: Guardian.new(Fabricate(:admin)))).to include(hidden_tag.name)
+      end
+
+      it "doesn't return hidden tags to anon" do
+        expect(Tag.top_tags).to_not include(hidden_tag.name)
+      end
+
+      it "doesn't return hidden tags to non-staff" do
+        expect(Tag.top_tags(guardian: Guardian.new(Fabricate(:user)))).to_not include(hidden_tag.name)
+      end
+    end
   end
 
   describe '#pm_tags' do

spec/serializers/post_revision_serializer_spec.rb

@@ -0,0 +1,53 @@
+require 'rails_helper'
+
+describe PostRevisionSerializer do
+  let(:post) { Fabricate(:post, version: 2) }
+
+  context 'hidden tags' do
+    let(:public_tag) { Fabricate(:tag, name: 'public') }
+    let(:public_tag2) { Fabricate(:tag, name: 'visible') }
+    let(:hidden_tag) { Fabricate(:tag, name: 'hidden') }
+    let(:hidden_tag2) { Fabricate(:tag, name: 'secret') }
+
+    let(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name, hidden_tag2.name]) }
+
+    let(:post_revision) do
+      Fabricate(:post_revision,
+        post: post,
+        modifications: { 'tags' => [['public', 'hidden'], ['visible', 'hidden']] }
+      )
+    end
+
+    let(:post_revision2) do
+      Fabricate(:post_revision,
+        post: post,
+        modifications: { 'tags' => [['visible', 'hidden', 'secret'], ['visible', 'hidden']] }
+      )
+    end
+
+    before do
+      SiteSetting.tagging_enabled = true
+      staff_tag_group
+      post.topic.tags = [public_tag2, hidden_tag]
+    end
+
+    it 'returns all tag changes to staff' do
+      json = PostRevisionSerializer.new(post_revision, scope: Guardian.new(Fabricate(:admin)), root: false).as_json
+      expect(json[:tags_changes][:previous]).to include(public_tag.name)
+      expect(json[:tags_changes][:previous]).to include(hidden_tag.name)
+      expect(json[:tags_changes][:current]).to include(public_tag2.name)
+      expect(json[:tags_changes][:current]).to include(hidden_tag.name)
+    end
+
+    it 'does not return hidden tags to non-staff' do
+      json = PostRevisionSerializer.new(post_revision, scope: Guardian.new(Fabricate(:user)), root: false).as_json
+      expect(json[:tags_changes][:previous]).to eq([public_tag.name])
+      expect(json[:tags_changes][:current]).to eq([public_tag2.name])
+    end
+
+    it 'does not show tag modificiatons if changes are not visible to the user' do
+      json = PostRevisionSerializer.new(post_revision2, scope: Guardian.new(Fabricate(:user)), root: false).as_json
+      expect(json[:tags_changes]).to_not be_present
+    end
+  end
+end

spec/serializers/suggested_topic_serializer_spec.rb

@@ -2,11 +2,12 @@ require 'rails_helper'
 
 describe SuggestedTopicSerializer do
   let(:user) { Fabricate(:user) }
+  let(:admin) { Fabricate(:admin) }
 
   describe '#featured_link and #featured_link_root_domain' do
     let(:featured_link) { 'http://meta.discourse.org' }
     let(:topic) { Fabricate(:topic, featured_link: featured_link, category: Fabricate(:category, topic_featured_link_allowed: true)) }
-    subject(:json) { described_class.new(topic, scope: Guardian.new(user), root: false).as_json }
+    subject(:json) { SuggestedTopicSerializer.new(topic, scope: Guardian.new(user), root: false).as_json }
 
     context 'when topic featured link is disable' do
       before do
@@ -32,4 +33,26 @@ describe SuggestedTopicSerializer do
       end
     end
   end
+
+  describe 'hidden tags' do
+    let(:topic) { Fabricate(:topic) }
+    let(:hidden_tag) { Fabricate(:tag, name: 'hidden') }
+    let(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+
+    before do
+      SiteSetting.tagging_enabled = true
+      staff_tag_group
+      topic.tags << hidden_tag
+    end
+
+    it 'returns hidden tag to staff' do
+      json = SuggestedTopicSerializer.new(topic, scope: Guardian.new(admin), root: false).as_json
+      expect(json[:tags]).to eq([hidden_tag.name])
+    end
+
+    it 'does not return hidden tag to non-staff' do
+      json = SuggestedTopicSerializer.new(topic, scope: Guardian.new(user), root: false).as_json
+      expect(json[:tags]).to eq([])
+    end
+  end
 end

spec/serializers/topic_list_item_serializer_spec.rb

@@ -43,4 +43,25 @@ describe TopicListItemSerializer do
       expect(serialized[:featured_link_root_domain]).to eq(nil)
     end
   end
+
+  describe 'hidden tags' do
+    let(:hidden_tag) { Fabricate(:tag, name: 'hidden') }
+    let(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+
+    before do
+      SiteSetting.tagging_enabled = true
+      staff_tag_group
+      topic.tags << hidden_tag
+    end
+
+    it 'returns hidden tag to staff' do
+      json = TopicListItemSerializer.new(topic, scope: Guardian.new(Fabricate(:admin)), root: false).as_json
+      expect(json[:tags]).to eq([hidden_tag.name])
+    end
+
+    it 'does not return hidden tag to non-staff' do
+      json = TopicListItemSerializer.new(topic, scope: Guardian.new(Fabricate(:user)), root: false).as_json
+      expect(json[:tags]).to eq([])
+    end
+  end
 end

spec/serializers/topic_view_serializer_spec.rb

@@ -1,13 +1,14 @@
 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
+  def serialize_topic(topic, user_arg)
+    topic_view = TopicView.new(topic.id, user_arg)
+    described_class.new(topic_view, scope: Guardian.new(user_arg), root: false).as_json
   end
 
   let(:topic) { Fabricate(:topic) }
   let(:user) { Fabricate(:user) }
+  let(:admin) { Fabricate(:admin) }
 
   describe '#featured_link and #featured_link_root_domain' do
     let(:featured_link) { 'http://meta.discourse.org' }
@@ -69,7 +70,6 @@ describe TopicViewSerializer do
 
   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: [
@@ -104,4 +104,25 @@ describe TopicViewSerializer do
       end
     end
   end
+
+  describe 'with hidden tags' do
+    let(:hidden_tag) { Fabricate(:tag, name: 'hidden') }
+    let(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+
+    before do
+      SiteSetting.tagging_enabled = true
+      staff_tag_group
+      topic.tags << hidden_tag
+    end
+
+    it 'returns hidden tag to staff' do
+      json = serialize_topic(topic, admin)
+      expect(json[:tags]).to eq([hidden_tag.name])
+    end
+
+    it 'does not return hidden tag to non-staff' do
+      json = serialize_topic(topic, user)
+      expect(json[:tags]).to eq([])
+    end
+  end
 end