From 5e97a9bfb7844f673d0ba64efbcb2f2fe53e0ba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 16 May 2018 09:48:19 +0200 Subject: [PATCH] FIX: tags in a 'visible by everyone but usable only by staff' group weren't visible by everyone --- .../discourse/models/tag-group.js.es6 | 77 ++++++++----------- .../discourse/templates/tag-groups-show.hbs | 6 +- app/controllers/tag_groups_controller.rb | 7 +- app/controllers/tags_controller.rb | 21 +++-- app/models/tag_group.rb | 27 +++---- spec/models/tag_group_spec.rb | 50 ++++++++++-- 6 files changed, 101 insertions(+), 87 deletions(-) diff --git a/app/assets/javascripts/discourse/models/tag-group.js.es6 b/app/assets/javascripts/discourse/models/tag-group.js.es6 index 020f2b99018..ea29f414123 100644 --- a/app/assets/javascripts/discourse/models/tag-group.js.es6 +++ b/app/assets/javascripts/discourse/models/tag-group.js.es6 @@ -1,71 +1,62 @@ -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'; +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') - disableSave() { - return Ember.isEmpty(this.get('name')) || Ember.isEmpty(this.get('tag_names')) || this.get('saving'); +export default RestModel.extend({ + @computed("name", "tag_names", "saving") + disableSave(name, tagNames, 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) { - return 'public'; - } else if (permissions['everyone'] === PermissionType.READONLY) { - return 'visible'; + if (permissions["everyone"] === PermissionType.FULL) { + return "public"; + } else if (permissions["everyone"] === PermissionType.READONLY) { + return "visible"; } else { - return 'private'; + 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}); + 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}); + this.set("permissions", { "everyone": PermissionType.FULL }); } } }, save() { - let url = "/tag_groups"; - const self = this, - isNew = this.get('id') === 'new'; - if (!isNew) { - url = "/tag_groups/" + this.get('id'); - } + this.set("savingStatus", I18n.t("saving")); + this.set("saving", true); - this.set('savingStatus', I18n.t('saving')); - this.set('saving', true); + const isNew = this.get("id") === "new"; + 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: { - 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'), - 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); + data, + type: isNew ? "POST" : "PUT" + }).then(result => { + if (result.tag_group && result.tag_group.id) { + this.set("id", result.tag_group.id); } - self.set('savingStatus', I18n.t('saved')); - self.set('saving', false); + }).finally(() => { + 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; diff --git a/app/assets/javascripts/discourse/templates/tag-groups-show.hbs b/app/assets/javascripts/discourse/templates/tag-groups-show.hbs index 80f4efc3b39..60f2f48bcd4 100644 --- a/app/assets/javascripts/discourse/templates/tag-groups-show.hbs +++ b/app/assets/javascripts/discourse/templates/tag-groups-show.hbs @@ -30,15 +30,15 @@
- {{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"}}
- {{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"}}
- {{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"}}
diff --git a/app/controllers/tag_groups_controller.rb b/app/controllers/tag_groups_controller.rb index 069f73830ab..65748f83780 100644 --- a/app/controllers/tag_groups_controller.rb +++ b/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) like ?', "%#{term}%") + TagGroup.where('lower(name) ILIKE ?', "%#{params[:q].strip}%") 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] ||= [] diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 0d67cb80770..67c4ee54d36 100644 --- a/app/controllers/tags_controller.rb +++ b/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 the tags to admins - unless guardian.can_admin_tags? && guardian.is_admin? - ungrouped_tags = ungrouped_tags.where("tags.topic_count > 0") - end + show_all_tags = guardian.can_admin_tags? && guardian.is_admin? 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)") - .where("id in (?)", guardian.allowed_category_ids) + categories = Category.where("id IN (SELECT category_id FROM category_tags)") + .where("id IN (?)", guardian.allowed_category_ids) .includes(:tags) category_tag_counts = categories.map do |c| diff --git a/app/models/tag_group.rb b/app/models/tag_group.rb index cc556a2a907..5a5a74af02f 100644 --- a/app/models/tag_group.rb +++ b/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 - (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 IN ( - SELECT tag_group_id - FROM tag_group_permissions - WHERE group_id = ? - AND permission_type = ? + 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 IN (SELECT tag_group_id FROM tag_group_permissions WHERE group_id = ?) ) SQL - TagGroup.where( - category_permissions_filter_sql, - guardian.allowed_category_ids, - Group::AUTO_GROUPS[:everyone], - TagGroupPermission.permission_types[:full] - ) + TagGroup.where(filter_sql, guardian.allowed_category_ids, Group::AUTO_GROUPS[:everyone]) end end end diff --git a/spec/models/tag_group_spec.rb b/spec/models/tag_group_spec.rb index 8dc4d45373a..b2fcd20c9d7 100644 --- a/spec/models/tag_group_spec.rb +++ b/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 - expect(TagGroup.allowed(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.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.allowed(Guardian.new(nil)).pluck(:name)).to match_array([public_tag_group.name, unrestricted_tag_group.name]) + it "returns correct groups based on category & tag group permissions" do + expect(TagGroup.visible(Guardian.new(admin)).pluck(:name)).to match_array(TagGroup.pluck(:name)) + expect(TagGroup.visible(Guardian.new(moderator)).pluck(:name)).to match_array(TagGroup.pluck(:name)) + + expect(TagGroup.visible(Guardian.new(user2)).pluck(:name)).to match_array([ + 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