FEATURE: move staff tags setting to tag group settings

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

@@ -1,6 +1,7 @@
 import { ajax } from 'discourse/lib/ajax';
 import RestModel from 'discourse/models/rest';
 import computed from 'ember-addons/ember-computed-decorators';
+import PermissionType from 'discourse/models/permission-type';
 
 const TagGroup = RestModel.extend({
   @computed('name', 'tag_names')
@@ -8,6 +9,31 @@ const TagGroup = RestModel.extend({
     return Ember.isEmpty(this.get('name')) || Ember.isEmpty(this.get('tag_names')) || this.get('saving');
   },
 
+  @computed('permissions')
+  permissionName: {
+    get(permissions) {
+      if (!permissions) return 'public';
+
+      if (permissions['everyone'] === PermissionType.FULL) {
+        return 'public';
+      } else if (permissions['everyone'] === PermissionType.READONLY) {
+        return 'visible';
+      } else {
+        return 'private';
+      }
+    },
+
+    set(value) {
+      if (value === 'private') {
+        this.set('permissions', {'staff': PermissionType.FULL});
+      } else if (value === 'visible') {
+        this.set('permissions', {'staff': PermissionType.FULL, 'everyone': PermissionType.READONLY});
+      } else {
+        this.set('permissions', {'everyone': PermissionType.FULL});
+      }
+    }
+  },
+
   save() {
     let url = "/tag_groups";
     const self = this,
@@ -25,7 +51,7 @@ const TagGroup = RestModel.extend({
         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'),
-        permissions: this.get('visible_only_to_staff') ? {"staff": "1"} : {"everyone": "1"}
+        permissions: this.get('permissions')
       },
       type: isNew ? 'POST' : 'PUT'
     }).then(function(result) {

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

@@ -29,10 +29,18 @@
   </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>
+    <div>
+      {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="public" id="public-permission" selection=model.permissionName}}
+      <label class="radio" for="public-permission">{{i18n 'tagging.groups.everyone_can_use'}}</label>
+    </div>
+    <div>
+      {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="visible" id="visible-permission" selection=model.permissionName}}
+      <label class="radio" for="visible-permission">{{i18n 'tagging.groups.usable_only_by_staff'}}</label>
+    </div>
+    <div>
+      {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="private" id="private-permission" selection=model.permissionName}}
+      <label class="radio" for="private-permission">{{i18n 'tagging.groups.visible_only_to_staff'}}</label>
+    </div>
   </section>
 
   <button {{action "save"}} disabled={{model.disableSave}} class='btn'>{{i18n 'tagging.groups.save'}}</button>

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

@@ -237,6 +237,9 @@ header .discourse-tag {color: $tag-color }
     ul {
       margin-bottom: 10px;
     }
+    .btn {
+      margin-left: 10px;
+    }
   }
   .tag-group-content {
     width: 75%;
@@ -249,11 +252,13 @@ header .discourse-tag {color: $tag-color }
       display: inline-block;
       margin-right: 10px;
     }
+    .btn {
+      margin-right: 10px;
+    }
   }
   .group-tags-list .tag-chooser {
     width: 100%;
   }
-  .btn {margin-left: 10px;}
   .saving {
     margin-left: 20px;
   }

app/models/category_group.rb

@@ -5,7 +5,7 @@ class CategoryGroup < ActiveRecord::Base
   delegate :name, to: :group, prefix: true
 
   def self.permission_types
-    @permission_types ||= Enum.new(:full, :create_post, :readonly)
+    @permission_types ||= Enum.new(full: 1, create_post: 2, readonly: 3)
   end
 
 end

app/models/tag.rb

@@ -49,7 +49,7 @@ class Tag < ActiveRecord::Base
 
     return [] if scope_category_ids.empty?
 
-    filter_sql = guardian&.is_staff? ? '' : (' AND ' + DiscourseTagging.filter_visible_sql)
+    filter_sql = guardian&.is_staff? ? '' : " AND tags.id NOT IN (#{DiscourseTagging.hidden_tags_query.select(:id).to_sql})"
 
     tag_names_with_counts = Tag.exec_sql <<~SQL
       SELECT tags.name as tag_name, SUM(stats.topic_count) AS sum_topic_count

app/models/tag_group.rb

@@ -9,6 +9,7 @@ class TagGroup < ActiveRecord::Base
 
   belongs_to :parent_tag, class_name: 'Tag'
 
+  before_create :init_permissions
   before_save :apply_permissions
 
   attr_accessor :permissions
@@ -45,6 +46,15 @@ class TagGroup < ActiveRecord::Base
     end
   end
 
+  def init_permissions
+    unless tag_group_permissions.present? || @permissions
+      tag_group_permissions.build(
+        group_id: Group::AUTO_GROUPS[:everyone],
+        permission_type: TagGroupPermission.permission_types[:full]
+      )
+    end
+  end
+
   def apply_permissions
     if @permissions
       tag_group_permissions.destroy_all
@@ -55,22 +65,27 @@ class TagGroup < ActiveRecord::Base
     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
+      category_permissions_filter_sql = <<~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))
-        AND id NOT IN (SELECT tag_group_id FROM tag_group_permissions)
+        AND id IN (
+          SELECT tag_group_id
+          FROM tag_group_permissions
+          WHERE group_id = ?
+            AND permission_type = ?
+        )
       SQL
 
-      TagGroup.where(category_permissions_filter, guardian.allowed_category_ids)
+      TagGroup.where(
+        category_permissions_filter_sql,
+        guardian.allowed_category_ids,
+        Group::AUTO_GROUPS[:everyone],
+        TagGroupPermission.permission_types[:full]
+      )
     end
   end
 end

app/models/tag_group_permission.rb

@@ -4,7 +4,7 @@ class TagGroupPermission < ActiveRecord::Base
   belongs_to :group
 
   def self.permission_types
-    @permission_types ||= Enum.new(full: 1) #, see: 2
+    @permission_types ||= Enum.new(full: 1, readonly: 3)
   end
 end
 

app/serializers/post_revision_serializer.rb

@@ -255,8 +255,12 @@ class PostRevisionSerializer < ApplicationSerializer
     end
 
     def filter_visible_tags(tags)
+      if tags.is_a?(Array) && tags.size > 0
         @hidden_tag_names ||= DiscourseTagging.hidden_tag_names(scope)
-      tags.is_a?(Array) ? (tags - @hidden_tag_names) : tags
+        tags - @hidden_tag_names
+      else
+        tags
+      end
     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, :visible_only_to_staff
+  attributes :id, :name, :tag_names, :parent_tag_name, :one_per_topic, :permissions
 
   def tag_names
     object.tags.map(&:name).sort
@@ -8,4 +8,17 @@ class TagGroupSerializer < ApplicationSerializer
   def parent_tag_name
     [object.parent_tag.try(:name)].compact
   end
+
+  def permissions
+    @permissions ||= begin
+      h = {}
+      object.tag_group_permissions.joins(:group).includes(:group).order("groups.name").each do |tgp|
+        h[tgp.group.name] = tgp.permission_type
+      end
+      if h.size == 0
+        h['everyone'] = TagGroupPermission.permission_types[:full]
+      end
+      h
+    end
+  end
 end

config/locales/client.en.yml

@@ -2660,6 +2660,8 @@ en:
         save: "Save"
         delete: "Delete"
         confirm_delete: "Are you sure you want to delete this tag group?"
+        everyone_can_use: "Tags can be used by everyone"
+        usable_only_by_staff: "Tags are visible to everyone, but only staff can use them"
         visible_only_to_staff: "Tags are visible only to staff"
 
       topics:

config/locales/server.en.yml

@@ -1674,7 +1674,6 @@ en:
     tags_sort_alphabetically: "Show tags in alphabetical order. Default is to show in order of popularity."
     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"

config/site_settings.yml

@@ -1605,10 +1605,6 @@ tags:
   tags_listed_by_group:
     client: true
     default: false
-  staff_tags:
-    type: list
-    client: true
-    default: ''
   allow_staff_to_tag_pms:
     default: false
   suppress_overlapping_tags_in_list:

db/migrate/20180420141134_remove_staff_tags_setting.rb

@@ -0,0 +1,52 @@
+class RemoveStaffTagsSetting < ActiveRecord::Migration[5.1]
+  def up
+    execute "INSERT INTO tag_group_permissions
+      (tag_group_id, group_id, permission_type, created_at, updated_at)
+      SELECT id, #{Group::AUTO_GROUPS[:everyone]},
+             #{TagGroupPermission.permission_types[:full]},
+             now(), now()
+        FROM tag_groups
+       WHERE id NOT IN (SELECT tag_group_id FROM tag_group_permissions)"
+
+    result = execute("SELECT value FROM site_settings WHERE name = 'staff_tags'").to_a
+    if result.length > 0
+      if tags = result[0]['value']&.split('|')
+        tag_group = execute(
+          "INSERT INTO tag_groups (name, created_at, updated_at)
+           VALUES ('staff tags', now(), now())
+           RETURNING id"
+        )
+
+        tag_group_id = tag_group[0]['id']
+
+        execute(
+          "INSERT INTO tag_group_permissions
+          (tag_group_id, group_id, permission_type, created_at, updated_at)
+          VALUES
+          (#{tag_group_id}, #{Group::AUTO_GROUPS[:everyone]},
+           #{TagGroupPermission.permission_types[:readonly]}, now(), now()),
+          (#{tag_group_id}, #{Group::AUTO_GROUPS[:staff]},
+           #{TagGroupPermission.permission_types[:full]}, now(), now())"
+        )
+
+        tags.each do |tag_name|
+          tag = execute("SELECT id FROM tags WHERE name = '#{tag_name}'").to_a
+          if tag[0] && tag[0]['id']
+            execute(
+              "INSERT INTO tag_group_memberships
+              (tag_id, tag_group_id, created_at, updated_at)
+              VALUES
+              (#{tag[0]['id']}, #{tag_group_id}, now(), now())"
+            )
+          end
+        end
+      end
+    end
+
+    execute "DELETE FROM site_settings WHERE name = 'staff_tags'"
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/discourse_tagging.rb

@@ -11,18 +11,19 @@ module DiscourseTagging
       new_tag_names = tag_names - old_tag_names
       removed_tag_names = old_tag_names - tag_names
 
-      hidden_tags = DiscourseTagging.hidden_tag_names
-
       # Protect staff-only tags
       unless guardian.is_staff?
-        staff_tags = DiscourseTagging.staff_only_tags(new_tag_names) || []
-        staff_tags += hidden_tags & new_tag_names
+        all_staff_tags = DiscourseTagging.staff_tag_names
+        hidden_tags = DiscourseTagging.hidden_tag_names
+
+        staff_tags = new_tag_names & all_staff_tags
+        staff_tags += new_tag_names & hidden_tags
         if staff_tags.present?
           topic.errors[:base] << I18n.t("tags.staff_tag_disallowed", tag: staff_tags.join(" "))
           return false
         end
 
-        staff_tags = DiscourseTagging.staff_only_tags(removed_tag_names)
+        staff_tags = removed_tag_names & all_staff_tags
         if staff_tags.present?
           topic.errors[:base] << I18n.t("tags.staff_tag_remove_disallowed", tag: staff_tags.join(" "))
           return false
@@ -129,8 +130,8 @@ module DiscourseTagging
 
     if opts[:for_input] || opts[:for_topic]
       unless guardian.nil? || guardian.is_staff?
-        staff_tag_names = SiteSetting.staff_tags.split("|")
-        query = query.where('tags.name NOT IN (?)', staff_tag_names) if staff_tag_names.present?
+        all_staff_tags = DiscourseTagging.staff_tag_names
+        query = query.where('tags.name NOT IN (?)', all_staff_tags) if all_staff_tags.present?
       end
 
       # exclude tag groups that have a parent tag which is missing from selected_tags
@@ -176,28 +177,31 @@ module DiscourseTagging
   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
+    guardian&.is_staff? ? query : query.where("tags.id NOT IN (#{hidden_tags_query.select(:id).to_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)
+    return [] if guardian&.is_staff?
+
+    hidden_tags_query.pluck(:name)
+  end
+
+  def self.hidden_tags_query
+    Tag.joins(:tag_groups)
+      .where('tag_groups.id NOT IN (
+        SELECT tag_group_id
+        FROM tag_group_permissions
+        WHERE group_id = ?)',
+        Group::AUTO_GROUPS[:everyone]
+      )
+  end
+
+  def self.staff_tag_names
+    Tag.joins(tag_groups: :tag_group_permissions)
+      .where('tag_group_permissions.group_id = ? AND tag_group_permissions.permission_type = ?',
+        Group::AUTO_GROUPS[:everyone],
+        TagGroupPermission.permission_types[:readonly]
+      ).pluck(:name)
   end
 
   def self.clean_tag(tag)
@@ -206,17 +210,6 @@ module DiscourseTagging
       .gsub(TAGS_FILTER_REGEXP, '')[0...SiteSetting.max_tag_length]
   end
 
-  def self.staff_only_tags(tags)
-    return nil if tags.nil?
-
-    staff_tags = SiteSetting.staff_tags.split("|")
-
-    tag_diff = tags - staff_tags
-    tag_diff = tags - tag_diff
-
-    tag_diff.present? ? tag_diff : nil
-  end
-
   def self.tags_for_saving(tags_arg, guardian, opts = {})
 
     return [] unless guardian.can_tag_topics? && tags_arg.present?

spec/components/discourse_tagging_spec.rb

@@ -74,6 +74,15 @@ describe DiscourseTagging do
       expect(tags.size).to eq(3)
       expect(tags).to contain_exactly(tag1, tag2, tag3)
     end
+
+    it 'returns staff only tags to everyone' do
+      create_staff_tags(['important'])
+      staff_tag = Tag.where(name: 'important').first
+      topic.tags << staff_tag
+      tags = DiscourseTagging.filter_visible(topic.tags, Guardian.new(user))
+      expect(tags.size).to eq(4)
+      expect(tags).to contain_exactly(tag1, tag2, tag3, staff_tag)
+    end
   end
 
   describe 'tag_topic_by_names' do
@@ -81,7 +90,7 @@ describe DiscourseTagging do
       let(:topic) { Fabricate(:topic) }
 
       before do
-        SiteSetting.staff_tags = "alpha"
+        create_staff_tags(['alpha'])
       end
 
       it "regular users can't add staff-only tags" do

spec/components/post_revisor_spec.rb

@@ -645,14 +645,14 @@ describe PostRevisor do
           end
 
           it "can't add staff-only tags" do
-            SiteSetting.staff_tags = "important"
+            create_staff_tags(['important'])
             result = subject.revise!(Fabricate(:user), raw: "lets totally update the body", tags: ['important', 'stuff'])
             expect(result).to eq(false)
             expect(post.topic.errors.present?).to eq(true)
           end
 
           it "staff can add staff-only tags" do
-            SiteSetting.staff_tags = "important"
+            create_staff_tags(['important'])
             result = subject.revise!(Fabricate(:admin), raw: "lets totally update the body", tags: ['important', 'stuff'])
             expect(result).to eq(true)
             post.reload
@@ -661,9 +661,9 @@ describe PostRevisor do
 
           context "with staff-only tags" do
             before do
-              SiteSetting.staff_tags = "important"
+              create_staff_tags(['important'])
               topic = post.topic
-              topic.tags = [Fabricate(:tag, name: "super"), Fabricate(:tag, name: "important"), Fabricate(:tag, name: "stuff")]
+              topic.tags = [Fabricate(:tag, name: "super"), Tag.where(name: "important").first, Fabricate(:tag, name: "stuff")]
             end
 
             it "staff-only tags can't be removed" do

spec/components/search_spec.rb

@@ -432,11 +432,10 @@ describe Search do
       end
 
       it 'shows staff tags' do
-        staff_tag = Fabricate(:tag, name: "#{tag.name}9")
-        SiteSetting.staff_tags = "#{staff_tag.name}"
+        create_staff_tags(["#{tag.name}9"])
 
-        expect(Search.execute(tag.name, guardian: Guardian.new(Fabricate(:admin))).tags).to contain_exactly(tag, staff_tag)
-        expect(search.tags).to contain_exactly(tag, staff_tag)
+        expect(Search.execute(tag.name, guardian: Guardian.new(Fabricate(:admin))).tags.map(&:name)).to contain_exactly(tag.name, "#{tag.name}9")
+        expect(search.tags.map(&:name)).to contain_exactly(tag.name, "#{tag.name}9")
       end
 
       it 'includes category-restricted tags' do

spec/components/topic_creator_spec.rb

@@ -80,7 +80,7 @@ describe TopicCreator do
 
       context 'staff-only tags' do
         before do
-          SiteSetting.staff_tags = "alpha"
+          create_staff_tags(['alpha'])
         end
 
         it "regular users can't add staff-only tags" do

spec/models/tag_user_spec.rb

@@ -156,7 +156,7 @@ describe TagUser do
         staff = Fabricate(:admin)
         topic = create_post.topic
 
-        SiteSetting.staff_tags = "foo"
+        create_staff_tags(['foo'])
 
         result = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), ["foo"])
         expect(result).to eq(false)

spec/support/helpers.rb

@@ -85,4 +85,21 @@ module Helpers
   def email(email_name)
     fixture_file("emails/#{email_name}.eml")
   end
+
+  def create_staff_tags(tag_names)
+    tag_group = Fabricate(:tag_group, name: 'Staff Tags')
+    TagGroupPermission.create!(
+      tag_group: tag_group,
+      group_id: Group::AUTO_GROUPS[:everyone],
+      permission_type: TagGroupPermission.permission_types[:readonly]
+    )
+    TagGroupPermission.create!(
+      tag_group: tag_group,
+      group_id: Group::AUTO_GROUPS[:staff],
+      permission_type: TagGroupPermission.permission_types[:full]
+    )
+    tag_names.each do |name|
+      tag_group.tags << (Tag.where(name: name).first || Fabricate(:tag, name: name))
+    end
+  end
 end