From 4f0fe9219541525321f11db2db660f451a00ba30 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 28 Aug 2024 16:11:16 +1000 Subject: [PATCH] UX: Add group link to category permission row (#28560) Makes it easier to reach the group from the category security tab, and moves the trash button to the right to avoid misclicks. Also converts the category permission row to gjs --- .../components/category-permission-row.gjs | 170 ++++++++++++++++++ .../components/category-permission-row.hbs | 25 --- .../app/components/category-permission-row.js | 128 ------------- .../app/components/edit-category-security.hbs | 4 +- .../app/components/edit-category-security.js | 15 ++ .../acceptance/category-edit-security-test.js | 2 +- .../common/base/edit-category.scss | 6 +- ...b => edit_category_form_templates_spec.rb} | 2 +- spec/system/edit_category_security_spec.rb | 61 +++++++ .../components/category_permission_row.rb | 37 ++++ spec/system/page_objects/pages/category.rb | 13 ++ 11 files changed, 306 insertions(+), 157 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/components/category-permission-row.gjs delete mode 100644 app/assets/javascripts/discourse/app/components/category-permission-row.hbs delete mode 100644 app/assets/javascripts/discourse/app/components/category-permission-row.js rename spec/system/{category_edit_spec.rb => edit_category_form_templates_spec.rb} (97%) create mode 100644 spec/system/edit_category_security_spec.rb create mode 100644 spec/system/page_objects/components/category_permission_row.rb diff --git a/app/assets/javascripts/discourse/app/components/category-permission-row.gjs b/app/assets/javascripts/discourse/app/components/category-permission-row.gjs new file mode 100644 index 00000000000..d2b83587ab1 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/category-permission-row.gjs @@ -0,0 +1,170 @@ +import Component from "@glimmer/component"; +import { action } from "@ember/object"; +import { inject as service } from "@ember/service"; +import DButton from "discourse/components/d-button"; +import concatClass from "discourse/helpers/concat-class"; +import PermissionType from "discourse/models/permission-type"; +import getURL from "discourse-common/lib/get-url"; +import I18n from "discourse-i18n"; + +const EVERYONE = "everyone"; + +export default class CategoryPermissionRow extends Component { + @service currentUser; + + get everyonePermissionType() { + return this.args.everyonePermission?.permission_type; + } + + get canReply() { + return ( + this.args.type === PermissionType.CREATE_POST || + this.args.type === PermissionType.FULL + ); + } + + get canCreate() { + return this.args.type === PermissionType.FULL; + } + + get canCreateIcon() { + return this.canCreate ? "check-square" : "far-square"; + } + + get canReplyIcon() { + return this.canReply ? "check-square" : "far-square"; + } + + get replyGrantedClass() { + return this.args.type <= PermissionType.CREATE_POST ? "reply-granted" : ""; + } + + get createGrantedClass() { + return this.args.type === PermissionType.FULL ? "create-granted" : ""; + } + + get isEveryoneGroup() { + return this.args.groupName === EVERYONE; + } + + get replyDisabled() { + // If everyone has create permission then it doesn't make sense to + // be able to remove reply for other groups + if ( + !this.isEveryoneGroup && + this.everyonePermissionType && + this.everyonePermissionType <= PermissionType.CREATE_POST + ) { + return true; + } + return false; + } + + get replyTooltip() { + return this.replyDisabled + ? I18n.t("category.permissions.inherited") + : I18n.t("category.permissions.toggle_reply"); + } + + get createDisabled() { + // If everyone has full permission then it doesn't make sense to + // be able to remove create for other groups + if ( + !this.isEveryoneGroup && + this.everyonePermissionType && + this.everyonePermissionType === PermissionType.FULL + ) { + return true; + } + return false; + } + + get createTooltip() { + return this.createDisabled + ? I18n.t("category.permissions.inherited") + : I18n.t("category.permissions.toggle_full"); + } + + get groupLink() { + return getURL(`/g/${this.args.groupName}`); + } + + @action + removeRow(event) { + event?.preventDefault(); + this.args.category.removePermission(this.args.groupName); + } + + @action + setPermissionReply() { + if (this.args.type <= PermissionType.CREATE_POST) { + this.#updatePermission(PermissionType.READONLY); + } else { + this.#updatePermission(PermissionType.CREATE_POST); + } + } + + @action + setPermissionFull() { + if ( + !this.isEveryoneGroup && + this.everyonePermissionType === PermissionType.FULL + ) { + return; + } + + if (this.args.type === PermissionType.FULL) { + this.#updatePermission(PermissionType.CREATE_POST); + } else { + this.#updatePermission(PermissionType.FULL); + } + } + + #updatePermission(type) { + this.args.category.updatePermission(this.args.groupName, type); + + if (this.isEveryoneGroup) { + this.args.onChangeEveryonePermission(type); + } + } + + +} diff --git a/app/assets/javascripts/discourse/app/components/category-permission-row.hbs b/app/assets/javascripts/discourse/app/components/category-permission-row.hbs deleted file mode 100644 index 0125e1d2644..00000000000 --- a/app/assets/javascripts/discourse/app/components/category-permission-row.hbs +++ /dev/null @@ -1,25 +0,0 @@ - - {{this.group_name}} - - {{d-icon "far-trash-alt"}} - - - - - - - - - \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/category-permission-row.js b/app/assets/javascripts/discourse/app/components/category-permission-row.js deleted file mode 100644 index aa2d0a6b271..00000000000 --- a/app/assets/javascripts/discourse/app/components/category-permission-row.js +++ /dev/null @@ -1,128 +0,0 @@ -import Component from "@ember/component"; -import { action } from "@ember/object"; -import { alias, equal } from "@ember/object/computed"; -import { classNames } from "@ember-decorators/component"; -import { observes } from "@ember-decorators/object"; -import PermissionType from "discourse/models/permission-type"; -import discourseComputed from "discourse-common/utils/decorators"; -import I18n from "discourse-i18n"; - -const EVERYONE = "everyone"; - -@classNames("permission-row", "row-body") -export default class CategoryPermissionRow extends Component { - @equal("type", PermissionType.FULL) canCreate; - @alias("everyonePermission.permission_type") everyonePermissionType; - - @discourseComputed("type") - canReply(value) { - return ( - value === PermissionType.CREATE_POST || value === PermissionType.FULL - ); - } - - @discourseComputed("type") - canReplyIcon() { - return this.canReply ? "check-square" : "far-square"; - } - - @discourseComputed("type") - canCreateIcon() { - return this.canCreate ? "check-square" : "far-square"; - } - - @discourseComputed("type") - replyGranted() { - return this.type <= PermissionType.CREATE_POST ? "reply-granted" : ""; - } - - @discourseComputed("type") - createGranted() { - return this.type === PermissionType.FULL ? "create-granted" : ""; - } - - @observes("everyonePermissionType") - inheritFromEveryone() { - if (this.group_name === EVERYONE) { - return; - } - - // groups cannot have a lesser permission than "everyone" - if (this.everyonePermissionType < this.type) { - this.updatePermission(this.everyonePermissionType); - } - } - - @discourseComputed("everyonePermissionType", "type") - replyDisabled(everyonePermissionType) { - if ( - this.group_name !== EVERYONE && - everyonePermissionType && - everyonePermissionType <= PermissionType.CREATE_POST - ) { - return true; - } - return false; - } - - @discourseComputed("replyDisabled") - replyTooltip(replyDisabled) { - return replyDisabled - ? I18n.t("category.permissions.inherited") - : I18n.t("category.permissions.toggle_reply"); - } - - @discourseComputed("everyonePermissionType", "type") - createDisabled(everyonePermissionType) { - if ( - this.group_name !== EVERYONE && - everyonePermissionType && - everyonePermissionType === PermissionType.FULL - ) { - return true; - } - return false; - } - - @discourseComputed("createDisabled") - createTooltip(createDisabled) { - return createDisabled - ? I18n.t("category.permissions.inherited") - : I18n.t("category.permissions.toggle_full"); - } - - updatePermission(type) { - this.category.updatePermission(this.group_name, type); - } - - @action - removeRow(event) { - event?.preventDefault(); - this.category.removePermission(this.group_name); - } - - @action - setPermissionReply() { - if (this.type <= PermissionType.CREATE_POST) { - this.updatePermission(PermissionType.READONLY); - } else { - this.updatePermission(PermissionType.CREATE_POST); - } - } - - @action - setPermissionFull() { - if ( - this.group_name !== EVERYONE && - this.everyonePermissionType === PermissionType.FULL - ) { - return; - } - - if (this.type === PermissionType.FULL) { - this.updatePermission(PermissionType.CREATE_POST); - } else { - this.updatePermission(PermissionType.FULL); - } - } -} diff --git a/app/assets/javascripts/discourse/app/components/edit-category-security.hbs b/app/assets/javascripts/discourse/app/components/edit-category-security.hbs index 68758f54640..bee8f3658af 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-security.hbs +++ b/app/assets/javascripts/discourse/app/components/edit-category-security.hbs @@ -15,14 +15,16 @@ {{i18n "category.permissions.see"}} {{i18n "category.permissions.reply"}} {{i18n "category.permissions.create"}} + {{#each this.category.permissions as |p|}} {{/each}} diff --git a/app/assets/javascripts/discourse/app/components/edit-category-security.js b/app/assets/javascripts/discourse/app/components/edit-category-security.js index 70a1bf0a741..07617234792 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-security.js +++ b/app/assets/javascripts/discourse/app/components/edit-category-security.js @@ -34,5 +34,20 @@ export default buildCategoryPanel("security", { permission_type: this.minimumPermission, }); }, + + onChangeEveryonePermission(everyonePermissionType) { + this.category.permissions.forEach((permission, idx) => { + if (permission.group_name === "everyone") { + return; + } + + if (everyonePermissionType < permission.permission_type) { + this.category.set( + `permissions.${idx}.permission_type`, + everyonePermissionType + ); + } + }); + }, }, }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/category-edit-security-test.js b/app/assets/javascripts/discourse/tests/acceptance/category-edit-security-test.js index 39d7c772a94..40b133f0d34 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/category-edit-security-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/category-edit-security-test.js @@ -59,7 +59,7 @@ acceptance("Category Edit - Security", function (needs) { const addedRow = [...queryAll(".row-body")].at(-1); assert.strictEqual( - addedRow.querySelector(".group-name-label").innerText, + addedRow.querySelector(".group-name-link").innerText, "staff" ); assert.strictEqual( diff --git a/app/assets/stylesheets/common/base/edit-category.scss b/app/assets/stylesheets/common/base/edit-category.scss index 2e127489e88..89230ef0f7f 100644 --- a/app/assets/stylesheets/common/base/edit-category.scss +++ b/app/assets/stylesheets/common/base/edit-category.scss @@ -236,7 +236,11 @@ div.edit-category { .remove-permission { margin-left: 0.5em; padding: 0.15em; - color: var(--danger); + + .d-icon { + color: var(--danger); + } + &:hover { color: var(--danger-hover); } diff --git a/spec/system/category_edit_spec.rb b/spec/system/edit_category_form_templates_spec.rb similarity index 97% rename from spec/system/category_edit_spec.rb rename to spec/system/edit_category_form_templates_spec.rb index 83a3e35f91e..d8354f0a6cd 100644 --- a/spec/system/category_edit_spec.rb +++ b/spec/system/edit_category_form_templates_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -describe "Edit Category", type: :system do +describe "Edit Category Form Templates", type: :system do fab!(:color_scheme) fab!(:theme) fab!(:admin) diff --git a/spec/system/edit_category_security_spec.rb b/spec/system/edit_category_security_spec.rb new file mode 100644 index 00000000000..92cfbc96744 --- /dev/null +++ b/spec/system/edit_category_security_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +describe "Edit Category Security", type: :system do + fab!(:current_user) { Fabricate(:admin) } + fab!(:group) + fab!(:category) + fab!(:category_group_everyone) do + Fabricate( + :category_group, + category: category, + permission_type: CategoryGroup.permission_types[:full], + group: Group.find(Group::AUTO_GROUPS[:everyone]), + ) + end + fab!(:category_group_1) do + Fabricate( + :category_group, + category: category, + permission_type: CategoryGroup.permission_types[:full], + group: group, + ) + end + + let(:category_page) { PageObjects::Pages::Category.new } + let(:category_permission_row) { PageObjects::Components::CategoryPermissionRow.new } + + before { sign_in(current_user) } + + it "lists the groups that can access the catgory" do + category_page.visit_security(category) + expect(category_page).to have_public_access_message + expect(category_permission_row).to have_group_permission( + category_group_1.group_name, + %w[create reply], + ) + expect(category_permission_row).to have_group_permission("everyone", %w[create reply]) + end + + it "can navigate to a group" do + category_page.visit_security(category) + category_permission_row.navigate_to_group(category_group_1.group.name) + expect(page).to have_current_path("/g/#{category_group_1.group.name}") + end + + it "can delete a group's permissions" do + category_page.visit_security(category) + category_permission_row.remove_group_permission(category_group_1.group.name) + category_page.save_settings + category_page.visit_security(category) + expect(category_permission_row).to have_no_group_permission(category_group_1.group_name) + end + + it "can modify a group's permissions" do + category_group_everyone.update!(permission_type: CategoryGroup.permission_types[:reply]) + category_page.visit_security(category) + category_permission_row.toggle_group_permission(category_group_1.group.name, "create") + category_page.save_settings + category_page.visit_security(category) + expect(category_permission_row).to have_group_permission(category_group_1.group_name, %w[reply]) + end +end diff --git a/spec/system/page_objects/components/category_permission_row.rb b/spec/system/page_objects/components/category_permission_row.rb new file mode 100644 index 00000000000..f99efff969a --- /dev/null +++ b/spec/system/page_objects/components/category_permission_row.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module PageObjects + module Components + class CategoryPermissionRow < PageObjects::Components::Base + def group_permission_row_selector(group_name) + ".permission-row[data-group-name='#{group_name}']" + end + + def has_group_permission?(group_name, permissions = nil) + if !permissions + page.has_css?(group_permission_row_selector(group_name)) + else + permissions.each do |permission| + page.has_css?("#{group_permission_row_selector(group_name)} .#{permission}-granted") + end + end + end + + def has_no_group_permission?(group_name) + page.has_no_css?(group_permission_row_selector(group_name)) + end + + def navigate_to_group(group_name) + find(group_permission_row_selector(group_name)).find(".group-name-link").click + end + + def remove_group_permission(group_name) + find(group_permission_row_selector(group_name)).find(".remove-permission").click + end + + def toggle_group_permission(group_name, permission) + find(group_permission_row_selector(group_name)).find(".#{permission}-toggle").click + end + end + end +end diff --git a/spec/system/page_objects/pages/category.rb b/spec/system/page_objects/pages/category.rb index 1e66b9748a8..ec528559596 100644 --- a/spec/system/page_objects/pages/category.rb +++ b/spec/system/page_objects/pages/category.rb @@ -30,6 +30,11 @@ module PageObjects self end + def visit_security(category) + page.visit("/c/#{category.slug}/edit/security") + self + end + def back_to_category find(".edit-category-title-bar span", text: "Back to category").click self @@ -92,6 +97,14 @@ module PageObjects def click_new page.find(CATEGORY_NAVIGATION_NEW_NAV_ITEM_SELECTOR).click end + + def has_public_access_message? + page.has_content?(I18n.t("js.category.permissions.everyone_has_access")) + end + + def has_no_public_access_message? + page.has_no_content?(I18n.t("js.category.permissions.everyone_has_access")) + end end end end