Tag groups can belong to groups (#10854)

app/assets/javascripts/discourse/app/components/tag-groups-form.js

@@ -4,32 +4,109 @@ import { isEmpty } from "@ember/utils";
 import Component from "@ember/component";
 import { bufferedProperty } from "discourse/mixins/buffered-content";
 import PermissionType from "discourse/models/permission-type";
+import Group from "discourse/models/group";
 import bootbox from "bootbox";
 
 export default Component.extend(bufferedProperty("model"), {
   tagName: "",
+  allGroups: null,
 
-  @discourseComputed("buffered.isSaving", "buffered.name", "buffered.tag_names")
+  init() {
-  savingDisabled(isSaving, name, tagNames) {
+    this._super(...arguments);
-    return isSaving || isEmpty(name) || isEmpty(tagNames);
+    this.setGroupOptions();
+  },
+
+  setGroupOptions() {
+    Group.findAll().then((groups) => {
+      this.set("allGroups", groups);
+    });
+  },
+
+  @discourseComputed(
+    "buffered.isSaving",
+    "buffered.name",
+    "buffered.tag_names",
+    "buffered.permissions"
+  )
+  savingDisabled(isSaving, name, tagNames, permissions) {
+    return (
+      isSaving ||
+      isEmpty(name) ||
+      isEmpty(tagNames) ||
+      (!this.everyoneSelected(permissions) &&
+        isEmpty(this.selectedGroupNames(permissions)))
+    );
+  },
+
+  @discourseComputed("buffered.permissions")
+  showPrivateChooser(permissions) {
+    if (!permissions) {
+      return true;
+    }
+
+    return permissions.everyone !== PermissionType.READONLY;
+  },
+
+  @discourseComputed("buffered.permissions", "allGroups")
+  selectedGroupIds(permissions, allGroups) {
+    if (!permissions || !allGroups) {
+      return [];
+    }
+
+    const selectedGroupNames = Object.keys(permissions);
+    let groupIds = [];
+    allGroups.forEach((group) => {
+      if (selectedGroupNames.includes(group.name)) {
+        groupIds.push(group.id);
+      }
+    });
+
+    return groupIds;
+  },
+
+  everyoneSelected(permissions) {
+    if (!permissions) {
+      return true;
+    }
+
+    return permissions.everyone === PermissionType.FULL;
+  },
+
+  selectedGroupNames(permissions) {
+    if (!permissions) {
+      return [];
+    }
+
+    return Object.keys(permissions).filter((name) => name !== "everyone");
   },
 
   actions: {
-    setPermissions(permissionName) {
+    setPermissionsType(permissionName) {
+      let updatedPermissions = Object.assign(
+        {},
+        this.buffered.get("permissions")
+      );
+
       if (permissionName === "private") {
-        this.buffered.set("permissions", {
+        delete updatedPermissions.everyone;
-          staff: PermissionType.FULL,
-        });
       } else if (permissionName === "visible") {
-        this.buffered.set("permissions", {
+        updatedPermissions.everyone = PermissionType.READONLY;
-          staff: PermissionType.FULL,
-          everyone: PermissionType.READONLY,
-        });
       } else {
-        this.buffered.set("permissions", {
+        updatedPermissions.everyone = PermissionType.FULL;
-          everyone: PermissionType.FULL,
-        });
       }
+
+      this.buffered.set("permissions", updatedPermissions);
+    },
+
+    setPermissionsGroups(groupIds) {
+      let permissions = {};
+      this.allGroups.forEach((group) => {
+        if (groupIds.includes(group.id)) {
+          permissions[group.name] = PermissionType.FULL;
+        }
+      });
+
+      this.buffered.set("permissions", permissions);
     },
 
     save() {
@@ -41,6 +118,14 @@ export default Component.extend(bufferedProperty("model"), {
         "permissions"
       );
 
+      // If 'everyone' is set to full, we can remove any groups.
+      if (
+        !attrs.permissions ||
+        attrs.permissions.everyone === PermissionType.FULL
+      ) {
+        attrs.permissions = { everyone: PermissionType.FULL };
+      }
+
       this.model.save(attrs).then(() => {
         this.commitBuffer();
 

app/assets/javascripts/discourse/app/templates/components/tag-groups-form.hbs

@@ -38,7 +38,7 @@
         value="public"
         id="public-permission"
         selection=buffered.permissionName
-        onChange=(action "setPermissions")}}
+        onChange=(action "setPermissionsType")}}
 
       <label class="radio" for="public-permission">
         {{i18n "tagging.groups.everyone_can_use"}}
@@ -51,11 +51,20 @@
         value="visible"
         id="visible-permission"
         selection=buffered.permissionName
-        onChange=(action "setPermissions")}}
+        onChange=(action "setPermissionsType")}}
 
       <label class="radio" for="visible-permission">
-        {{i18n "tagging.groups.usable_only_by_staff"}}
+        {{i18n "tagging.groups.usable_only_by_groups"}}
       </label>
+
+      <div class="group-access-control {{if showPrivateChooser "hidden"}}">
+        {{group-chooser
+          content=allGroups
+          value=selectedGroupIds
+          labelProperty="name"
+          onChange=(action "setPermissionsGroups")
+        }}
+      </div>
     </div>
     <div>
       {{radio-button
@@ -64,12 +73,20 @@
         value="private"
         id="private-permission"
         selection=buffered.permissionName
-        onChange=(action "setPermissions")}}
+        onChange=(action "setPermissionsType")}}
 
       <label class="radio" for="private-permission">
-        {{i18n "tagging.groups.visible_only_to_staff"}}
+        {{i18n "tagging.groups.visible_only_to_groups"}}
       </label>
     </div>
+
+    <div class="group-access-control {{unless showPrivateChooser "hidden"}}">
+      {{group-chooser
+        content=allGroups
+        value=selectedGroupIds
+        labelProperty="name"
+        onChange=(action "setPermissionsGroups")}}
+    </div>
   </section>
 
   {{d-button

app/assets/javascripts/discourse/tests/acceptance/tag-groups-test.js

@@ -18,6 +18,19 @@ acceptance("Tag Groups", {
         },
       });
     });
+
+    server.get("/groups/search.json", () => {
+      return helper.response([
+        {
+          id: 88,
+          name: "tl1",
+        },
+        {
+          id: 89,
+          name: "tl2",
+        },
+      ]);
+    });
   },
 });
 
@@ -36,3 +49,26 @@ test("tag groups can be saved and deleted", async (assert) => {
   await click(".tag-chooser .choice:first");
   assert.ok(!find(".tag-group-content .btn.btn-danger")[0].disabled);
 });
+
+QUnit.test(
+  "tag groups can have multiple groups added to them",
+  async (assert) => {
+    const tags = selectKit(".tag-chooser");
+    const groups = selectKit(".group-chooser");
+
+    await visit("/tag_groups");
+    await click(".content-list .btn");
+
+    await fillIn(".tag-group-content h1 input", "test tag group");
+    await tags.expand();
+    await tags.selectRowByValue("monkey");
+
+    await click("#private-permission");
+    assert.ok(find(".tag-group-content .btn.btn-default:disabled").length);
+
+    await groups.expand();
+    await groups.selectRowByIndex(1);
+    await groups.selectRowByIndex(0);
+    assert.ok(!find(".tag-group-content .btn.btn-default")[0].disabled);
+  }
+);

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

@@ -256,6 +256,10 @@ header .discourse-tag {
     h1 input {
       width: 100%;
     }
+    .group-access-control {
+      margin-left: 44px;
+      margin-bottom: 10px;
+    }
   }
   .group-tags-list .tag-chooser {
     width: 100%;

app/controllers/tag_groups_controller.rb

@@ -82,19 +82,13 @@ class TagGroupsController < ApplicationController
     tag_group = params.delete(:tag_group)
     params.merge!(tag_group.permit!) if tag_group
 
-    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,
+      permissions: {}
     )
 
     result[:tag_names] ||= []

config/locales/client.en.yml

@@ -3409,8 +3409,8 @@ en:
         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"
+        usable_only_by_groups: "Tags are visible to everyone, but only the following groups can use them"
-        visible_only_to_staff: "Tags are visible only to staff"
+        visible_only_to_groups: "Tags are visible only to the following groups"
 
       topics:
         none:

config/locales/server.en.yml

@@ -4564,8 +4564,8 @@ en:
 
   tags:
     title: "Tags"
-    staff_tag_disallowed: 'The tag "%{tag}" may only be applied by staff.'
+    restricted_tag_disallowed: 'You cannot apply the tag "%{tag}".'
-    staff_tag_remove_disallowed: 'The tag "%{tag}" may only be removed by staff.'
+    restricted_tag_remove_disallowed: 'You cannot remove the tag "%{tag}".'
     minimum_required_tags:
       one: "You must select at least %{count} tag."
       other: "You must select at least %{count} tags."

lib/discourse_tagging.rb

@@ -23,31 +23,47 @@ module DiscourseTagging
         end
       end
 
+      # tags currently on the topic
       old_tag_names = topic.tags.pluck(:name) || []
+      # tags we're trying to add to the topic
       new_tag_names = tag_names - old_tag_names
+      # tag names being removed from the topic
       removed_tag_names = old_tag_names - tag_names
 
-      # Protect staff-only tags
+      # tag names which are visible, but not usable, by *some users*
-      unless guardian.is_staff?
+      readonly_tags = DiscourseTagging.readonly_tag_names(guardian)
-        all_staff_tags = DiscourseTagging.staff_tag_names
+      # tags names which are not visibile or usuable by *some users*
-        hidden_tags = DiscourseTagging.hidden_tag_names
+      hidden_tags = DiscourseTagging.hidden_tag_names(guardian)
 
-        staff_tags = new_tag_names & all_staff_tags
+      # tag names which ARE permitted by *this user*
-        staff_tags += new_tag_names & hidden_tags
+      permitted_tags = DiscourseTagging.permitted_tag_names(guardian)
-        if staff_tags.present?
-          topic.errors.add(:base, I18n.t("tags.staff_tag_disallowed", tag: staff_tags.join(" ")))
-          return false
-        end
 
-        staff_tags = removed_tag_names & all_staff_tags
+      # If this user has explicit permission to use certain tags,
-        if staff_tags.present?
+      # we need to ensure those tags are removed from the list of
-          topic.errors.add(:base, I18n.t("tags.staff_tag_remove_disallowed", tag: staff_tags.join(" ")))
+      # restricted tags
-          return false
+      if permitted_tags.present?
-        end
+        readonly_tags = readonly_tags - permitted_tags
-
+        hidden_tags = hidden_tags - permitted_tags
-        tag_names += removed_tag_names & hidden_tags
       end
 
+      # visible, but not usable, tags this user is trying to use
+      disallowed_tags = new_tag_names & readonly_tags
+      # hidden tags this user is trying to use
+      disallowed_tags += new_tag_names & hidden_tags
+
+      if disallowed_tags.present?
+        topic.errors.add(:base, I18n.t("tags.restricted_tag_disallowed", tag: disallowed_tags.join(" ")))
+        return false
+      end
+
+      removed_readonly_tags = removed_tag_names & readonly_tags
+      if removed_readonly_tags.present?
+        topic.errors.add(:base, I18n.t("tags.restricted_tag_remove_disallowed", tag: removed_readonly_tags.join(" ")))
+        return false
+      end
+
+      tag_names += removed_tag_names & hidden_tags
+
       category = topic.category
       tag_names = tag_names + old_tag_names if append
 
@@ -182,8 +198,7 @@ module DiscourseTagging
       INNER JOIN tag_group_memberships tgm ON tgm.tag_id = t.id /*and_name_like*/
       INNER JOIN tag_groups tg ON tg.id = tgm.tag_group_id
       INNER JOIN tag_group_permissions tgp
-      ON tg.id = tgp.tag_group_id
+      ON tg.id = tgp.tag_group_id /*and_group_ids*/
-      AND tgp.group_id = #{Group::AUTO_GROUPS[:everyone]}
       AND tgp.permission_type = #{TagGroupPermission.permission_types[:full]}
     )
   SQL
@@ -217,6 +232,8 @@ module DiscourseTagging
     sql = +"WITH #{TAG_GROUP_RESTRICTIONS_SQL}, #{CATEGORY_RESTRICTIONS_SQL}"
     if (opts[:for_input] || opts[:for_topic]) && filter_for_non_staff
       sql << ", #{PERMITTED_TAGS_SQL} "
+      builder_params[:group_ids] = permitted_group_ids(guardian)
+      sql.gsub!("/*and_group_ids*/", "AND group_id IN (:group_ids)")
     end
 
     outer_join = category.nil? || category.allow_global_tags || !category_has_restricted_tags
@@ -296,12 +313,23 @@ module DiscourseTagging
     end
 
     if filter_for_non_staff
-      # remove hidden tags
+      group_ids = permitted_group_ids(guardian)
-      builder.where(<<~SQL, Group::AUTO_GROUPS[:everyone])
+
+      builder.where(<<~SQL, group_ids, group_ids)
         id NOT IN (
-          SELECT tag_id
+          (SELECT tgm.tag_id
-            FROM tag_group_memberships tgm
+           FROM tag_group_permissions tgp
-           WHERE tag_group_id NOT IN (SELECT tag_group_id FROM tag_group_permissions WHERE group_id = ?)
+           INNER JOIN tag_groups tg ON tgp.tag_group_id = tg.id
+           INNER JOIN tag_group_memberships tgm ON tg.id = tgm.tag_group_id
+           WHERE tgp.group_id NOT IN (?))
+
+          EXCEPT
+
+          (SELECT tgm.tag_id
+           FROM tag_group_permissions tgp
+           INNER JOIN tag_groups tg ON tgp.tag_group_id = tg.id
+           INNER JOIN tag_group_memberships tgm ON tg.id = tgm.tag_group_id
+           WHERE tgp.group_id IN (?))
         )
       SQL
     end
@@ -348,16 +376,52 @@ module DiscourseTagging
     guardian&.is_staff? ? [] : hidden_tags_query.pluck(:name)
   end
 
+  # most restrictive level of tag groups
   def self.hidden_tags_query
-    Tag.joins(:tag_groups)
+    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]
       )
+
+    query
   end
 
+  def self.permitted_group_ids(guardian = nil)
+    group_ids = [Group::AUTO_GROUPS[:everyone]]
+
+    if guardian.authenticated?
+      group_ids.concat(guardian.user.groups.pluck(:id))
+    end
+
+    group_ids
+  end
+
+  # read-only tags for this user
+  def self.readonly_tag_names(guardian = nil)
+    return [] if guardian&.is_staff?
+
+    query = Tag.joins(tag_groups: :tag_group_permissions)
+      .where('tag_group_permissions.permission_type = ?',
+        TagGroupPermission.permission_types[:readonly])
+
+    query.pluck(:name)
+  end
+
+  # explicit permissions to use these tags
+  def self.permitted_tag_names(guardian = nil)
+    query = Tag.joins(tag_groups: :tag_group_permissions)
+      .where('tag_group_permissions.group_id IN (?) AND tag_group_permissions.permission_type = ?',
+        permitted_group_ids(guardian),
+        TagGroupPermission.permission_types[:full]
+      )
+
+    query.pluck(:name).uniq
+  end
+
+  # middle level of tag group restrictions
   def self.staff_tag_names
     tag_names = Discourse.cache.read(TAGS_STAFF_CACHE_KEY)
 

spec/components/discourse_tagging_spec.rb

@@ -85,6 +85,38 @@ describe DiscourseTagging do
         end
       end
 
+      context 'with tags visible only to non-admin group' do
+        fab!(:hidden_tag) { Fabricate(:tag) }
+        fab!(:group) { Fabricate(:group, name: "my-group") }
+        let!(:user_tag_group) { Fabricate(:tag_group, permissions: { "my-group" => 1 }, tag_names: [hidden_tag.name]) }
+
+        before do
+          group.add(user)
+        end
+
+        it 'should return all tags to member of group' do
+          tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user)).to_a
+          expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3, hidden_tag]))
+        end
+
+        it 'should allow a tag group to have multiple group permissions' do
+          group2 = Fabricate(:group, name: "another-group")
+          user2 = Fabricate(:user)
+          user3 = Fabricate(:user)
+          group2.add(user2)
+          user_tag_group.update!(permissions: { "my-group" => 1, "another-group" => 1 })
+
+          tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user)).to_a
+          expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3, hidden_tag]))
+
+          tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user2)).to_a
+          expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3, hidden_tag]))
+
+          tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user3)).to_a
+          expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3]))
+        end
+      end
+
       context 'with required tags from tag group' do
         fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2]) }
         fab!(:category) { Fabricate(:category, required_tag_group: tag_group, min_tags_from_required_group: 1) }
@@ -199,7 +231,7 @@ describe DiscourseTagging do
     end
 
     it 'returns staff only tags to everyone' do
-      create_staff_tags(['important'])
+      create_staff_only_tags(['important'])
       staff_tag = Tag.where(name: 'important').first
       topic.tags << staff_tag
       tags = DiscourseTagging.filter_visible(topic.tags, Guardian.new(user))
@@ -209,17 +241,17 @@ describe DiscourseTagging do
   end
 
   describe 'tag_topic_by_names' do
-    context 'staff-only tags' do
+    context 'visible but restricted tags' do
       fab!(:topic) { Fabricate(:topic) }
 
       before do
-        create_staff_tags(['alpha'])
+        create_staff_only_tags(['alpha'])
       end
 
       it "regular users can't add staff-only tags" do
         valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), ['alpha'])
         expect(valid).to eq(false)
-        expect(topic.errors[:base]&.first).to eq(I18n.t("tags.staff_tag_disallowed", tag: 'alpha'))
+        expect(topic.errors[:base]&.first).to eq(I18n.t("tags.restricted_tag_disallowed", tag: 'alpha'))
       end
 
       it 'staff can add staff-only tags' do
@@ -227,6 +259,29 @@ describe DiscourseTagging do
         expect(valid).to eq(true)
         expect(topic.errors[:base]).to be_empty
       end
+
+      context 'non-staff users in tag group groups' do
+        fab!(:non_staff_group) { Fabricate(:group, name: 'non_staff_group') }
+
+        before do
+          create_limited_tags('Group for Non-Staff', non_staff_group.id, ['alpha'])
+        end
+
+        it 'can use hidden tag if in correct group' do
+          non_staff_group.add(user)
+
+          valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), ['alpha'])
+
+          expect(valid).to eq(true)
+          expect(topic.errors[:base]).to be_empty
+        end
+
+        it 'will return error if user is not in correct group' do
+          user2 = Fabricate(:user)
+          valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user2), ['alpha'])
+          expect(valid).to eq(false)
+        end
+      end
     end
 
     it 'respects category allow_global_tags setting' do

spec/components/post_revisor_spec.rb

@@ -787,14 +787,14 @@ describe PostRevisor do
           end
 
           it "can't add staff-only tags" do
-            create_staff_tags(['important'])
+            create_staff_only_tags(['important'])
             result = subject.revise!(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
-            create_staff_tags(['important'])
+            create_staff_only_tags(['important'])
             result = subject.revise!(admin, raw: "lets totally update the body", tags: ['important', 'stuff'])
             expect(result).to eq(true)
             post.reload
@@ -803,7 +803,7 @@ describe PostRevisor do
 
           context "with staff-only tags" do
             before do
-              create_staff_tags(['important'])
+              create_staff_only_tags(['important'])
               topic = post.topic
               topic.tags = [Fabricate(:tag, name: "super"), Tag.where(name: "important").first, Fabricate(:tag, name: "stuff")]
             end

spec/components/search_spec.rb

@@ -952,7 +952,7 @@ describe Search do
       end
 
       it 'shows staff tags' do
-        create_staff_tags(["#{tag.name}9"])
+        create_staff_only_tags(["#{tag.name}9"])
 
         expect(Search.execute(tag.name, guardian: Guardian.new(admin)).tags.map(&:name)).to eq([tag.name, "#{tag.name}9"])
         expect(search.tags.map(&:name)).to eq([tag.name, "#{tag.name}9"])

spec/components/topic_creator_spec.rb

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

spec/models/tag_user_spec.rb

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

spec/support/helpers.rb

@@ -93,16 +93,20 @@ module Helpers
     fixture_file("emails/#{email_name}.eml")
   end
 
-  def create_staff_tags(tag_names)
+  def create_staff_only_tags(tag_names)
-    tag_group = Fabricate(:tag_group, name: 'Staff Tags')
+    create_limited_tags('Staff Tags', Group::AUTO_GROUPS[:staff], tag_names)
-    TagGroupPermission.create!(
+  end
+
+  def create_limited_tags(tag_group_name, group_id, tag_names)
+    tag_group = Fabricate(:tag_group, name: tag_group_name)
+    TagGroupPermission.where(
       tag_group: tag_group,
       group_id: Group::AUTO_GROUPS[:everyone],
-      permission_type: TagGroupPermission.permission_types[:readonly]
+      permission_type: TagGroupPermission.permission_types[:full]
-    )
+    ).update(permission_type: TagGroupPermission.permission_types[:readonly])
     TagGroupPermission.create!(
       tag_group: tag_group,
-      group_id: Group::AUTO_GROUPS[:staff],
+      group_id: group_id,
       permission_type: TagGroupPermission.permission_types[:full]
     )
     tag_names.each do |name|