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

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);
+    }
+  }
+
+  <template>
+    <div class="permission-row row-body" data-group-name={{@groupName}}>
+      <span class="group-name">
+        {{#if this.isEveryoneGroup}}
+          <span class="group-name-label">{{@groupName}}</span>
+        {{else}}
+          <a class="group-name-link" href={{this.groupLink}}>{{@groupName}}</a>
+        {{/if}}
+      </span>
+      <span class="options actionable">
+        <DButton @icon="check-square" @disabled={{true}} class="btn-flat see" />
+
+        <DButton
+          @icon={{this.canReplyIcon}}
+          @action={{this.setPermissionReply}}
+          @translatedTitle={{this.replyTooltip}}
+          @disabled={{this.replyDisabled}}
+          class={{concatClass
+            "btn btn-flat reply-toggle"
+            this.replyGrantedClass
+          }}
+        />
+
+        <DButton
+          @icon={{this.canCreateIcon}}
+          @action={{this.setPermissionFull}}
+          @translatedTitle={{this.createTooltip}}
+          @disabled={{this.createDisabled}}
+          class={{concatClass "btn-flat create-toggle" this.createGrantedClass}}
+        />
+
+        <DButton
+          class="remove-permission btn-flat"
+          @action={{this.removeRow}}
+          @icon="far-trash-alt"
+        />
+      </span>
+    </div>
+  </template>
+}

app/assets/javascripts/discourse/app/components/category-permission-row.hbs

@@ -1,25 +0,0 @@
-<span class="group-name">
-  <span class="group-name-label">{{this.group_name}}</span>
-  <a class="remove-permission" href {{on "click" this.removeRow}}>
-    {{d-icon "far-trash-alt"}}
-  </a>
-</span>
-<span class="options actionable">
-  <DButton @icon="check-square" @disabled={{true}} class="btn-flat see" />
-
-  <DButton
-    @icon={{this.canReplyIcon}}
-    @action={{action "setPermissionReply"}}
-    @translatedTitle={{this.replyTooltip}}
-    @disabled={{this.replyDisabled}}
-    class={{concat-class "btn btn-flat reply-toggle" this.replyGranted}}
-  />
-
-  <DButton
-    @icon={{this.canCreateIcon}}
-    @action={{action "setPermissionFull"}}
-    @translatedTitle={{this.createTooltip}}
-    @disabled={{this.createDisabled}}
-    class={{concat-class "btn-flat create-toggle" this.createGranted}}
-  />
-</span>

app/assets/javascripts/discourse/app/components/category-permission-row.js

@@ -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);
-    }
-  }
-}

app/assets/javascripts/discourse/app/components/edit-category-security.hbs

@@ -15,14 +15,16 @@
           <span class="cell">{{i18n "category.permissions.see"}}</span>
           <span class="cell">{{i18n "category.permissions.reply"}}</span>
           <span class="cell">{{i18n "category.permissions.create"}}</span>
+          <span class="cell"></span>
         </span>
       </div>
       {{#each this.category.permissions as |p|}}
         <CategoryPermissionRow
-          @group_name={{p.group_name}}
+          @groupName={{p.group_name}}
           @type={{p.permission_type}}
           @category={{this.category}}
           @everyonePermission={{this.everyonePermission}}
+          @onChangeEveryonePermission={{action "onChangeEveryonePermission"}}
         />
       {{/each}}
 

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
+          );
+        }
+      });
+    },
   },
 });

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(

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);
     }

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)

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

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

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