FIX: tags in a 'visible by everyone but usable only by staff' group weren't visible by everyone

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

@@ -1,71 +1,62 @@
-import { ajax } from 'discourse/lib/ajax';
+import { ajax } from "discourse/lib/ajax";
-import RestModel from 'discourse/models/rest';
+import RestModel from "discourse/models/rest";
-import computed from 'ember-addons/ember-computed-decorators';
+import computed from "ember-addons/ember-computed-decorators";
-import PermissionType from 'discourse/models/permission-type';
+import PermissionType from "discourse/models/permission-type";
 
-const TagGroup = RestModel.extend({
+export default RestModel.extend({
-  @computed('name', 'tag_names')
+  @computed("name", "tag_names", "saving")
-  disableSave() {
+  disableSave(name, tagNames, saving) {
-    return Ember.isEmpty(this.get('name')) || Ember.isEmpty(this.get('tag_names')) || this.get('saving');
+    return saving || Ember.isEmpty(name) || Ember.isEmpty(tagNames);
   },
 
-  @computed('permissions')
+  @computed("permissions")
   permissionName: {
     get(permissions) {
-      if (!permissions) return 'public';
+      if (!permissions) return "public";
 
-      if (permissions['everyone'] === PermissionType.FULL) {
+      if (permissions["everyone"] === PermissionType.FULL) {
-        return 'public';
+        return "public";
-      } else if (permissions['everyone'] === PermissionType.READONLY) {
+      } else if (permissions["everyone"] === PermissionType.READONLY) {
-        return 'visible';
+        return "visible";
       } else {
-        return 'private';
+        return "private";
       }
     },
 
     set(value) {
-      if (value === 'private') {
+      if (value === "private") {
-        this.set('permissions', {'staff': PermissionType.FULL});
+        this.set("permissions", { "staff": PermissionType.FULL });
-      } else if (value === 'visible') {
+      } else if (value === "visible") {
-        this.set('permissions', {'staff': PermissionType.FULL, 'everyone': PermissionType.READONLY});
+        this.set("permissions", { "staff": PermissionType.FULL, "everyone": PermissionType.READONLY });
       } else {
-        this.set('permissions', {'everyone': PermissionType.FULL});
+        this.set("permissions", { "everyone": PermissionType.FULL });
       }
     }
   },
 
   save() {
-    let url = "/tag_groups";
+    this.set("savingStatus", I18n.t("saving"));
-    const self = this,
+    this.set("saving", true);
-          isNew = this.get('id') === 'new';
-    if (!isNew) {
-      url = "/tag_groups/" + this.get('id');
-    }
 
-    this.set('savingStatus', I18n.t('saving'));
+    const isNew = this.get("id") === "new";
-    this.set('saving', true);
+    const url = isNew ? "/tag_groups" : `/tag_groups/${this.get("id")}`;
+    const data = this.getProperties("name", "tag_names", "parent_tag_name", "one_per_topic", "permissions");
 
     return ajax(url, {
-      data: {
+      data,
-        name: this.get('name'),
+      type: isNew ? "POST" : "PUT"
-        tag_names: this.get('tag_names'),
+    }).then(result => {
-        parent_tag_name: this.get('parent_tag_name') ? this.get('parent_tag_name') : undefined,
+      if (result.tag_group && result.tag_group.id) {
-        one_per_topic: this.get('one_per_topic'),
+        this.set("id", result.tag_group.id);
-        permissions: this.get('permissions')
-      },
-      type: isNew ? 'POST' : 'PUT'
-    }).then(function(result) {
-      if(result.tag_group && result.tag_group.id) {
-        self.set('id', result.tag_group.id);
       }
-      self.set('savingStatus', I18n.t('saved'));
+    }).finally(() => {
-      self.set('saving', false);
+      this.set("savingStatus", I18n.t("saved"));
+      this.set("saving", false);
     });
   },
 
   destroy() {
-    return ajax("/tag_groups/" + this.get('id'), {type: "DELETE"});
+    return ajax(`/tag_groups/${this.get("id")}`, { type: "DELETE" });
   }
 });
 
-export default TagGroup;

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

@@ -30,15 +30,15 @@
 
   <section class="group-visibility">
     <div>
-      {{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="public" id="public-permission" selection=model.permissionName onChange="changed"}}
       <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}}
+      {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="visible" id="visible-permission" selection=model.permissionName onChange="changed"}}
       <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}}
+      {{radio-button class="tag-permissions-choice" name="tag-permissions-choice" value="private" id="private-permission" selection=model.permissionName onChange="changed"}}
       <label class="radio" for="private-permission">{{i18n 'tagging.groups.visible_only_to_staff'}}</label>
     </div>
   </section>

app/controllers/tag_groups_controller.rb

@@ -33,7 +33,7 @@ class TagGroupsController < ApplicationController
     if @tag_group.save
       render_serialized(@tag_group, TagGroupSerializer)
     else
-      return render_json_error(@tag_group)
+      render_json_error(@tag_group)
     end
   end
 
@@ -52,8 +52,7 @@ class TagGroupsController < ApplicationController
 
   def search
     matches = if params[:q].present?
-      term = params[:q].strip.downcase
+      TagGroup.where('lower(name) ILIKE ?', "%#{params[:q].strip}%")
-      TagGroup.where('lower(name) like ?', "%#{term}%")
     else
       TagGroup.all
     end
@@ -82,7 +81,7 @@ class TagGroupsController < ApplicationController
         :one_per_topic,
         tag_names: [],
         parent_tag_name: [],
-        permissions: [*permissions&.keys]
+        permissions: permissions&.keys,
       )
       result[:tag_names] ||= []
       result[:parent_tag_name] ||= []

app/controllers/tags_controller.rb

@@ -32,27 +32,26 @@ class TagsController < ::ApplicationController
       end
 
       format.json do
-        ungrouped_tags = Tag.where("tags.id NOT IN (select tag_id from tag_group_memberships)")
+        show_all_tags = guardian.can_admin_tags? && guardian.is_admin?
-
-        # show all the tags to admins
-        unless guardian.can_admin_tags? && guardian.is_admin?
-          ungrouped_tags = ungrouped_tags.where("tags.topic_count > 0")
-        end
 
         if SiteSetting.tags_listed_by_group
-          grouped_tag_counts = TagGroup.allowed(guardian).order('name ASC').includes(:tags).map do |tag_group|
+          ungrouped_tags = Tag.where("tags.id NOT IN (SELECT tag_id FROM tag_group_memberships)")
+          ungrouped_tags = ungrouped_tags.where("tags.topic_count > 0") unless show_all_tags
+
+          grouped_tag_counts = TagGroup.visible(guardian).order('name ASC').includes(:tags).map do |tag_group|
             { id: tag_group.id, name: tag_group.name, tags: self.class.tag_counts_json(tag_group.tags) }
           end
 
           render json: {
-            tags: self.class.tag_counts_json(ungrouped_tags), # tags that don't belong to a group
+            tags: self.class.tag_counts_json(ungrouped_tags),
             extras: { tag_groups: grouped_tag_counts }
           }
         else
-          unrestricted_tags = DiscourseTagging.filter_visible(ungrouped_tags, guardian)
+          tags = show_all_tags ? Tag.all : Tag.where("tags.topic_count > 0")
+          unrestricted_tags = DiscourseTagging.filter_visible(tags, guardian)
 
-          categories = Category.where("id in (select category_id from category_tags)")
+          categories = Category.where("id IN (SELECT category_id FROM category_tags)")
-            .where("id in (?)", guardian.allowed_category_ids)
+            .where("id IN (?)", guardian.allowed_category_ids)
             .includes(:tags)
 
           category_tag_counts = categories.map do |c|

app/models/tag_group.rb

@@ -39,9 +39,6 @@ class TagGroup < ActiveRecord::Base
     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
@@ -65,27 +62,21 @@ class TagGroup < ActiveRecord::Base
     end
   end
 
-  def self.allowed(guardian)
+  def self.visible(guardian)
     if guardian.is_staff?
       TagGroup
     else
-      category_permissions_filter_sql = <<~SQL
+      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))
+          id IN (SELECT tag_group_id FROM category_tag_groups WHERE category_id IN (?))
-        AND id IN (
+        ) OR (
-          SELECT tag_group_id
+          id NOT IN (SELECT tag_group_id FROM category_tag_groups)
-          FROM tag_group_permissions
+          AND
-          WHERE group_id = ?
+          id IN (SELECT tag_group_id FROM tag_group_permissions WHERE group_id = ?)
-            AND permission_type = ?
         )
       SQL
 
-      TagGroup.where(
+      TagGroup.where(filter_sql, guardian.allowed_category_ids, Group::AUTO_GROUPS[:everyone])
-        category_permissions_filter_sql,
-        guardian.allowed_category_ids,
-        Group::AUTO_GROUPS[:everyone],
-        TagGroupPermission.permission_types[:full]
-      )
     end
   end
 end

spec/models/tag_group_spec.rb

@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 describe TagGroup do
-  describe '#allowed' do
+  describe '#visible' do
     let(:user1) { Fabricate(:user) }
     let(:user2) { Fabricate(:user) }
     let(:admin) { Fabricate(:admin) }
@@ -9,6 +9,10 @@ describe TagGroup do
 
     let(:group) { Fabricate(:group) }
 
+    let!(:everyone_tag_group) { Fabricate(:tag_group, name: 'Visible & usable by everyone', tag_names: ['foo-bar']) }
+    let!(:visible_tag_group) { Fabricate(:tag_group, name: 'Visible by everyone, usable by staff', tag_names: ['foo']) }
+    let!(:staff_only_tag_group) { Fabricate(:tag_group, name: 'Staff only', tag_names: ['bar']) }
+
     let!(:public_tag_group) { Fabricate(:tag_group, name: 'Public', tag_names: ['public1']) }
     let!(:private_tag_group) { Fabricate(:tag_group, name: 'Private', tag_names: ['privatetag1']) }
     let!(:staff_tag_group) { Fabricate(:tag_group, name: 'Staff Talk', tag_names: ['stafftag1']) }
@@ -16,26 +20,56 @@ describe TagGroup do
 
     let!(:public_category) { Fabricate(:category, name: 'Public Category') }
     let!(:private_category) { Fabricate(:private_category, group: group) }
-    let(:staff_category) { Fabricate(:category, name: 'Secret') }
+    let!(:staff_category) { Fabricate(:category, name: 'Secret') }
+
+    let(:everyone) { Group::AUTO_GROUPS[:everyone] }
+    let(:staff) { Group::AUTO_GROUPS[:staff] }
+
+    let(:full) { TagGroupPermission.permission_types[:full] }
+    let(:readonly) { TagGroupPermission.permission_types[:readonly] }
 
     before do
       group.add(user2)
       group.save!
+
       staff_category.set_permissions(admins: :full)
       staff_category.save!
+
       private_category.set_permissions(staff: :full, group => :full)
       private_category.save!
+
       public_category.allowed_tag_groups = [public_tag_group.name]
       private_category.allowed_tag_groups = [private_tag_group.name]
       staff_category.allowed_tag_groups = [staff_tag_group.name]
+
+      everyone_tag_group.permissions = [[everyone, full]]
+      everyone_tag_group.save!
+
+      visible_tag_group.permissions = [[everyone, readonly], [staff, full]]
+      visible_tag_group.save!
+
+      staff_only_tag_group.permissions = [[staff, full]]
+      staff_only_tag_group.save!
     end
 
-    it "returns correct groups based on category permissions" do
+    it "returns correct groups based on category & tag group permissions" do
-      expect(TagGroup.allowed(Guardian.new(admin)).pluck(:name)).to match_array(TagGroup.pluck(:name))
+      expect(TagGroup.visible(Guardian.new(admin)).pluck(:name)).to match_array(TagGroup.pluck(:name))
-      expect(TagGroup.allowed(Guardian.new(moderator)).pluck(:name)).to match_array(TagGroup.pluck(:name))
+      expect(TagGroup.visible(Guardian.new(moderator)).pluck(:name)).to match_array(TagGroup.pluck(:name))
-      expect(TagGroup.allowed(Guardian.new(user2)).pluck(:name)).to match_array([public_tag_group.name, unrestricted_tag_group.name, private_tag_group.name])
+
-      expect(TagGroup.allowed(Guardian.new(user1)).pluck(:name)).to match_array([public_tag_group.name, unrestricted_tag_group.name])
+      expect(TagGroup.visible(Guardian.new(user2)).pluck(:name)).to match_array([
-      expect(TagGroup.allowed(Guardian.new(nil)).pluck(:name)).to match_array([public_tag_group.name, unrestricted_tag_group.name])
+        public_tag_group.name, unrestricted_tag_group.name, private_tag_group.name,
+        everyone_tag_group.name, visible_tag_group.name,
+      ])
+
+      expect(TagGroup.visible(Guardian.new(user1)).pluck(:name)).to match_array([
+        public_tag_group.name, unrestricted_tag_group.name, everyone_tag_group.name,
+        visible_tag_group.name,
+      ])
+
+      expect(TagGroup.visible(Guardian.new(nil)).pluck(:name)).to match_array([
+        public_tag_group.name, unrestricted_tag_group.name, everyone_tag_group.name,
+        visible_tag_group.name,
+      ])
     end
   end
 end