diff --git a/app/assets/javascripts/discourse/app/controllers/share-topic.js b/app/assets/javascripts/discourse/app/controllers/share-topic.js index 46a09006ea9..5f9766af2c4 100644 --- a/app/assets/javascripts/discourse/app/controllers/share-topic.js +++ b/app/assets/javascripts/discourse/app/controllers/share-topic.js @@ -38,13 +38,12 @@ export default Controller.extend( return; } - Category.reloadBySlugPath(this.model.slug).then((result) => { - const groups = result.category.group_permissions.mapBy("group_name"); - if (groups && !groups.any((group) => group === "everyone")) { + Category.fetchVisibleGroups(this.model.id).then((result) => { + if (result.groups.length > 0) { this.flash( I18n.t("topic.share.restricted_groups", { - count: groups.length, - groupNames: groups.join(", "), + count: result.groups.length, + groupNames: result.groups.join(", "), }), "warning" ); diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index cab4fea6421..329077f284b 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -515,6 +515,10 @@ Category.reopenClass({ return category; }, + fetchVisibleGroups(id) { + return ajax(`/c/${id}/visible_groups.json`); + }, + reloadById(id) { return ajax(`/c/${id}/show.json`); }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js index e028b05ab12..a22ec248ca8 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/share-topic-test.js @@ -14,8 +14,16 @@ acceptance("Share and Invite modal", function (needs) { needs.user(); needs.pretender((server, helper) => { - server.get("/c/feature/find_by_slug.json", () => - helper.response(200, CategoryFixtures["/c/1/show.json"]) + server.get(`/c/2481/visible_groups.json`, () => + helper.response(200, { + groups: ["group_name_1", "group_name_2"], + }) + ); + + server.get(`/c/2/visible_groups.json`, () => + helper.response(200, { + groups: [], + }) ); }); @@ -30,6 +38,7 @@ acceptance("Share and Invite modal", function (needs) { await click("#topic-footer-button-share-and-invite"); assert.ok(exists(".share-topic-modal"), "it shows the modal"); + assert.notOk( exists("#modal-alert.alert-warning"), "it does not show the alert with restricted groups" @@ -73,8 +82,8 @@ acceptance("Share and Invite modal", function (needs) { assert.strictEqual( query("#modal-alert.alert-warning").innerText, I18n.t("topic.share.restricted_groups", { - count: 1, - groupNames: "moderators", + count: 2, + groupNames: "group_name_1, group_name_2", }), "it shows correct restricted group name" ); @@ -85,12 +94,6 @@ acceptance("Share and Invite modal - mobile", function (needs) { needs.user(); needs.mobileView(); - needs.pretender((server, helper) => { - server.get("/c/feature/find_by_slug.json", () => - helper.response(200, CategoryFixtures["/c/1/show.json"]) - ); - }); - test("Topic footer mobile button", async function (assert) { await visit("/t/internationalization-localization/280"); diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js index d73ebf1fc31..80970d7a596 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js @@ -26,6 +26,12 @@ import CategoryFixtures from "discourse/tests/fixtures/category-fixtures"; acceptance("Topic", function (needs) { needs.user(); needs.pretender((server, helper) => { + server.get("/c/2/visible_groups.json", () => + helper.response(200, { + groups: [], + }) + ); + server.get("/c/feature/find_by_slug.json", () => { return helper.response(200, CategoryFixtures["/c/1/show.json"]); }); diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 204c6193d47..163734cf812 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -2,9 +2,9 @@ class CategoriesController < ApplicationController - requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug] + requires_login except: [:index, :categories_and_latest, :categories_and_top, :show, :redirect, :find_by_slug, :visible_groups] - before_action :fetch_category, only: [:show, :update, :destroy] + before_action :fetch_category, only: [:show, :update, :destroy, :visible_groups] before_action :initialize_staff_action_logger, only: [:create, :update, :destroy] skip_before_action :check_xhr, only: [:index, :categories_and_latest, :categories_and_top, :redirect] @@ -120,6 +120,7 @@ class CategoriesController < ApplicationController if Category.topic_create_allowed(guardian).where(id: @category.id).exists? @category.permission = CategoryGroup.permission_types[:full] end + render_serialized(@category, CategorySerializer) end @@ -252,6 +253,11 @@ class CategoriesController < ApplicationController render_serialized(@category, CategorySerializer) end + def visible_groups + @guardian.ensure_can_see!(@category) + render json: success_json.merge(groups: @category.groups.merge(Group.visible_groups(current_user)).pluck("name")) + end + private def self.topics_per_page @@ -371,6 +377,7 @@ class CategoriesController < ApplicationController def fetch_category @category = Category.find_by_slug(params[:id]) || Category.find_by(id: params[:id].to_i) + raise Discourse::NotFound if @category.blank? end def initialize_staff_action_logger diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb index 7ff76772d4c..b7ab43b5806 100644 --- a/app/serializers/category_serializer.rb +++ b/app/serializers/category_serializer.rb @@ -53,6 +53,10 @@ class CategorySerializer < SiteCategorySerializer end end + def include_group_permissions? + scope&.can_edit?(object) + end + def include_available_groups? scope && scope.can_edit?(object) end diff --git a/config/routes.rb b/config/routes.rb index 75066b99c9b..acbb0c700cb 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -722,6 +722,7 @@ Discourse::Application.routes.draw do get "categories_and_top" => "categories#categories_and_top" get "c/:id/show" => "categories#show" + get "c/:id/visible_groups" => "categories#visible_groups" get "c/*category_slug/find_by_slug" => "categories#find_by_slug" get "c/*category_slug/edit(/:tab)" => "categories#find_by_slug", constraints: { format: 'html' } diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 790a7ab2741..f64858e4e48 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -733,4 +733,61 @@ describe CategoriesController do end end end + + describe '#visible_groups' do + fab!(:public_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:public], name: 'aaa') } + fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') } + fab!(:user_only_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc') } + + it 'responds with 404 when id param is invalid' do + get "/c/-9999/visible_groups.json" + + expect(response.status).to eq(404) + end + + it "responds with 403 when category is restricted to the current user" do + category.set_permissions(private_group.name => :full) + category.save! + + get "/c/#{category.id}/visible_groups.json" + + expect(response.status).to eq(403) + end + + it "returns the names of the groups that are visible to an admin" do + sign_in(admin) + + category.set_permissions( + "everyone" => :readonly, + private_group.name => :full, + public_group.name => :full, + user_only_group.name => :full, + ) + + category.save! + + get "/c/#{category.id}/visible_groups.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["groups"]).to eq([public_group.name, private_group.name, user_only_group.name]) + end + + it "returns the names of the groups that are visible to a user and excludes the everyone group" do + sign_in(user) + + category.set_permissions( + "everyone" => :readonly, + private_group.name => :full, + public_group.name => :full, + user_only_group.name => :full, + ) + + category.save! + + get "/c/#{category.id}/visible_groups.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["groups"]).to eq([public_group.name]) + end + end end diff --git a/spec/serializers/category_serializer_spec.rb b/spec/serializers/category_serializer_spec.rb index 9d34d31f2cf..024ae88b58e 100644 --- a/spec/serializers/category_serializer_spec.rb +++ b/spec/serializers/category_serializer_spec.rb @@ -43,76 +43,51 @@ describe CategorySerializer do end describe '#group_permissions' do - context "category without group permissions configured" do - it "returns the right category group permissions for an anon user" do - json = described_class.new(category, scope: Guardian.new, root: false).as_json + fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') } - expect(json[:group_permissions]).to eq([ - { permission_type: CategoryGroup.permission_types[:full], group_name: Group[:everyone]&.name } - ]) + fab!(:user_group) do + Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g| + g.add(user) end end - context "category with group permissions configured" do - fab!(:private_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:staff], name: 'bbb') } + before do + group.update!(name: 'aaa') - fab!(:user_group) do - Fabricate(:group, visibility_level: Group.visibility_levels[:members], name: 'ccc').tap do |g| - g.add(user) - end - end + category.set_permissions( + :everyone => :readonly, + group.name => :readonly, + user_group.name => :full, + private_group.name => :full, + ) - before do - group.update!(name: 'aaa') + category.save! + end - category.set_permissions( - :everyone => :readonly, - group.name => :readonly, - user_group.name => :full, - private_group.name => :full, - ) + it "does not include the attribute for an anon user" do + json = described_class.new(category, scope: Guardian.new, root: false).as_json - category.save! - end + expect(json[:group_permissions]).to eq(nil) + end - it "returns the right category group permissions for an anon user" do - json = described_class.new(category, scope: Guardian.new, root: false).as_json + it "does not include the attribute for a regular user" do + json = described_class.new(category, scope: Guardian.new(user), root: false).as_json - expect(json[:group_permissions]).to eq([ - { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name }, - ]) - end + expect(json[:group_permissions]).to eq(nil) + end - it "returns the right category group permissions for a regular user ordered by ascending group name" do - json = described_class.new(category, scope: Guardian.new(user), root: false).as_json + it "returns the right category group permissions for a user that can edit the category" do + SiteSetting.moderators_manage_categories_and_groups = true + user.update!(moderator: true) - expect(json[:group_permissions]).to eq([ - { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name }, - { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name }, - ]) - end + json = described_class.new(category, scope: Guardian.new(user), root: false).as_json - it "returns the right category group permission for a staff user ordered by ascending group name" do - json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json - - expect(json[:group_permissions]).to eq([ - { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name }, - { permission_type: CategoryGroup.permission_types[:full], group_name: private_group.name }, - { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name }, - { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' }, - ]) - end - - it "returns the group permissions for everyone group too" do - category.set_permissions(everyone: :readonly) - category.save! - - json = described_class.new(category, scope: Guardian.new(admin), root: false).as_json - - expect(json[:group_permissions]).to eq([ - { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' }, - ]) - end + expect(json[:group_permissions]).to eq([ + { permission_type: CategoryGroup.permission_types[:readonly], group_name: group.name }, + { permission_type: CategoryGroup.permission_types[:full], group_name: private_group.name }, + { permission_type: CategoryGroup.permission_types[:full], group_name: user_group.name }, + { permission_type: CategoryGroup.permission_types[:readonly], group_name: 'everyone' }, + ]) end end