diff --git a/app/assets/javascripts/discourse/models/tag-group.js.es6 b/app/assets/javascripts/discourse/models/tag-group.js.es6 index f433038abbe..020f2b99018 100644 --- a/app/assets/javascripts/discourse/models/tag-group.js.es6 +++ b/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) { diff --git a/app/assets/javascripts/discourse/templates/tag-groups-show.hbs b/app/assets/javascripts/discourse/templates/tag-groups-show.hbs index 55dea80a7c4..80f4efc3b39 100644 --- a/app/assets/javascripts/discourse/templates/tag-groups-show.hbs +++ b/app/assets/javascripts/discourse/templates/tag-groups-show.hbs @@ -29,10 +29,18 @@
- +
+ {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="public" id="public-permission" selection=model.permissionName}} + +
+
+ {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="visible" id="visible-permission" selection=model.permissionName}} + +
+
+ {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="private" id="private-permission" selection=model.permissionName}} + +
diff --git a/app/assets/stylesheets/common/base/tagging.scss b/app/assets/stylesheets/common/base/tagging.scss index a114c9bdee0..d459ea4f52e 100644 --- a/app/assets/stylesheets/common/base/tagging.scss +++ b/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; } diff --git a/app/models/category_group.rb b/app/models/category_group.rb index 849fcba0260..ca7a8ec8500 100644 --- a/app/models/category_group.rb +++ b/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 diff --git a/app/models/tag.rb b/app/models/tag.rb index 3182d62635f..8fcba761387 100644 --- a/app/models/tag.rb +++ b/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 diff --git a/app/models/tag_group.rb b/app/models/tag_group.rb index 2ed60e07801..cc556a2a907 100644 --- a/app/models/tag_group.rb +++ b/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 diff --git a/app/models/tag_group_permission.rb b/app/models/tag_group_permission.rb index bec35735653..f2a27c1c872 100644 --- a/app/models/tag_group_permission.rb +++ b/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 diff --git a/app/serializers/post_revision_serializer.rb b/app/serializers/post_revision_serializer.rb index 0e0348cb39b..e8c7e9f16ee 100644 --- a/app/serializers/post_revision_serializer.rb +++ b/app/serializers/post_revision_serializer.rb @@ -255,8 +255,12 @@ class PostRevisionSerializer < ApplicationSerializer end def filter_visible_tags(tags) - @hidden_tag_names ||= DiscourseTagging.hidden_tag_names(scope) - tags.is_a?(Array) ? (tags - @hidden_tag_names) : tags + if tags.is_a?(Array) && tags.size > 0 + @hidden_tag_names ||= DiscourseTagging.hidden_tag_names(scope) + tags - @hidden_tag_names + else + tags + end end end diff --git a/app/serializers/tag_group_serializer.rb b/app/serializers/tag_group_serializer.rb index e0a4734f246..47db578ce82 100644 --- a/app/serializers/tag_group_serializer.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 89ff718d4c9..fbeba4d7024 100644 --- a/config/locales/client.en.yml +++ b/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: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index bbc6fe7ac32..adf8b240a78 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/config/site_settings.yml b/config/site_settings.yml index de79862c76d..e12a9819370 100644 --- a/config/site_settings.yml +++ b/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: diff --git a/db/migrate/20180420141134_remove_staff_tags_setting.rb b/db/migrate/20180420141134_remove_staff_tags_setting.rb new file mode 100644 index 00000000000..4b473cb7adb --- /dev/null +++ b/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 diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index b991595f0d5..98426e2ed7c 100644 --- a/lib/discourse_tagging.rb +++ b/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? diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb index 51a636fca26..fb1ce805460 100644 --- a/spec/components/discourse_tagging_spec.rb +++ b/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 diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index 77837631329..083bac94f4a 100644 --- a/spec/components/post_revisor_spec.rb +++ b/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 diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index ed54be8c1da..1e5c1a723e7 100644 --- a/spec/components/search_spec.rb +++ b/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 diff --git a/spec/components/topic_creator_spec.rb b/spec/components/topic_creator_spec.rb index bb9586bfdb4..e56da86f49e 100644 --- a/spec/components/topic_creator_spec.rb +++ b/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 diff --git a/spec/models/tag_user_spec.rb b/spec/models/tag_user_spec.rb index b5b9dabacb8..9a506419001 100644 --- a/spec/models/tag_user_spec.rb +++ b/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) diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index 0a74e2df384..a137b1e12d3 100644 --- a/spec/support/helpers.rb +++ b/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