FIX: ensure GroupChooser works with localized group names (#30593)

The "Tag Groups Form" component was using group names to handle
permissions. This works just fine when the default locale is "English"
but breaks as soon as it's changed to a different locale.

The fix is to use the group id's for handling the permissions instead of
the group name.

Reported in https://meta.discourse.org/t/221849

app/assets/javascripts/discourse/app/components/tag-groups-form.hbs

@@ -54,7 +54,6 @@
       @value="public"
       @id="public-permission"
       @selection={{this.buffered.permissionName}}
-      @onChange={{action "setPermissionsType"}}
       class="tag-permissions-choice"
     />
 
@@ -68,7 +67,6 @@
       @value="visible"
       @id="visible-permission"
       @selection={{this.buffered.permissionName}}
-      @onChange={{action "setPermissionsType"}}
       class="tag-permissions-choice"
     />
 
@@ -94,7 +92,6 @@
       @value="private"
       @id="private-permission"
       @selection={{this.buffered.permissionName}}
-      @onChange={{action "setPermissionsType"}}
       class="tag-permissions-choice"
     />
 

app/assets/javascripts/discourse/app/components/tag-groups-form.js

@@ -4,7 +4,6 @@ import { service } from "@ember/service";
 import { isEmpty } from "@ember/utils";
 import { tagName } from "@ember-decorators/component";
 import { bufferedProperty } from "discourse/mixins/buffered-content";
-import Group from "discourse/models/group";
 import PermissionType from "discourse/models/permission-type";
 import discourseComputed from "discourse-common/utils/decorators";
 import { i18n } from "discourse-i18n";
@@ -15,110 +14,40 @@ export default class TagGroupsForm extends Component.extend(
 ) {
   @service router;
   @service dialog;
+  @service site;
 
-  allGroups = null;
+  // All but the "everyone" group
+  allGroups = this.site.groups.filter(({ id }) => id !== 0);
 
-  init() {
-    super.init(...arguments);
-    this.setGroupOptions();
-  }
-
-  setGroupOptions() {
-    Group.findAll().then((groups) => {
-      this.set("allGroups", groups);
-    });
-  }
-
-  @discourseComputed(
-    "buffered.name",
-    "buffered.tag_names",
-    "buffered.permissions"
-  )
-  cannotSave(name, tagNames, permissions) {
-    return (
-      isEmpty(name) ||
-      isEmpty(tagNames) ||
-      (!this.everyoneSelected(permissions) &&
-        isEmpty(this.selectedGroupNames(permissions)))
-    );
-  }
-
-  @discourseComputed("buffered.permissions", "allGroups")
-  selectedGroupIds(permissions, allGroups) {
-    if (!permissions || !allGroups) {
+  @discourseComputed("buffered.permissions")
+  selectedGroupIds(permissions) {
+    if (!permissions) {
       return [];
     }
 
-    const selectedGroupNames = Object.keys(permissions);
     let groupIds = [];
-    allGroups.forEach((group) => {
-      if (selectedGroupNames.includes(group.name)) {
-        groupIds.push(group.id);
+
+    for (const [groupId, permission] of Object.entries(permissions)) {
+      // JS object keys are always strings, so we need to convert them to integers
+      const id = parseInt(groupId, 10);
+
+      if (id !== 0 && permission === PermissionType.FULL) {
+        groupIds.push(id);
       }
-    });
+    }
 
     return groupIds;
   }
 
-  everyoneSelected(permissions) {
-    if (!permissions) {
-      return true;
-    }
-
-    return permissions.everyone === PermissionType.FULL;
-  }
-
-  selectedGroupNames(permissions) {
-    if (!permissions) {
-      return [];
-    }
-
-    return Object.keys(permissions).filter((name) => name !== "everyone");
-  }
-
-  @action
-  setPermissionsType(permissionName) {
-    let updatedPermissions = Object.assign(
-      {},
-      this.buffered.get("permissions")
-    );
-
-    if (permissionName === "private") {
-      delete updatedPermissions.everyone;
-    } else if (permissionName === "visible") {
-      updatedPermissions.everyone = PermissionType.READONLY;
-    } else {
-      updatedPermissions.everyone = PermissionType.FULL;
-    }
-
-    this.buffered.set("permissions", updatedPermissions);
-  }
-
   @action
   setPermissionsGroups(groupIds) {
-    let updatedPermissions = Object.assign(
-      {},
-      this.buffered.get("permissions")
-    );
-
-    this.allGroups.forEach((group) => {
-      if (groupIds.includes(group.id)) {
-        updatedPermissions[group.name] = PermissionType.FULL;
-      } else {
-        delete updatedPermissions[group.name];
-      }
-    });
-
-    this.buffered.set("permissions", updatedPermissions);
+    let permissions = {};
+    groupIds.forEach((id) => (permissions[id] = PermissionType.FULL));
+    this.buffered.set("permissions", permissions);
   }
 
   @action
   save() {
-    if (this.cannotSave) {
-      this.dialog.alert(i18n("tagging.groups.cannot_save"));
-      return false;
-    }
-
     const attrs = this.buffered.getProperties(
       "name",
       "tag_names",
@@ -127,36 +56,40 @@ export default class TagGroupsForm extends Component.extend(
       "permissions"
     );
 
-    // If 'everyone' is set to full, we can remove any groups.
-    if (
-      !attrs.permissions ||
-      attrs.permissions.everyone === PermissionType.FULL
-    ) {
-      attrs.permissions = { everyone: PermissionType.FULL };
+    if (isEmpty(attrs.name)) {
+      this.dialog.alert("tagging.groups.cannot_save.empty_name");
+      return false;
     }
 
-    this.model.save(attrs).then(() => {
-      this.commitBuffer();
+    if (isEmpty(attrs.tag_names)) {
+      this.dialog.alert("tagging.groups.cannot_save.no_tags");
+      return false;
+    }
 
-      if (this.onSave) {
-        this.onSave();
-      } else {
-        this.router.transitionTo("tagGroups.index");
-      }
-    });
+    attrs.permissions ??= {};
+
+    const permissionName = this.buffered.get("permissionName");
+
+    if (permissionName === "public") {
+      attrs.permissions = { 0: PermissionType.FULL };
+    } else if (permissionName === "visible") {
+      attrs.permissions[0] = PermissionType.READONLY;
+    } else if (permissionName === "private") {
+      delete attrs.permissions[0];
+    } else {
+      this.dialog.alert("tagging.groups.cannot_save.no_groups");
+      return false;
+    }
+
+    this.model.save(attrs).then(() => this.onSave?.());
   }
 
   @action
   destroyTagGroup() {
     return this.dialog.yesNoConfirm({
       message: i18n("tagging.groups.confirm_delete"),
-      didConfirm: () => {
-        this.model.destroyRecord().then(() => {
-          if (this.onDestroy) {
-            this.onDestroy();
-          }
-        });
-      },
+      didConfirm: () =>
+        this.model.destroyRecord().then(() => this.onDestroy?.()),
     });
   }
 }

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

@@ -9,9 +9,9 @@ export default class TagGroup extends RestModel {
       return "public";
     }
 
-    if (permissions["everyone"] === PermissionType.FULL) {
+    if (permissions[0] === PermissionType.FULL) {
       return "public";
-    } else if (permissions["everyone"] === PermissionType.READONLY) {
+    } else if (permissions[0] === PermissionType.READONLY) {
       return "visible";
     } else {
       return "private";

app/assets/javascripts/discourse/tests/acceptance/tag-groups-test.js

@@ -15,16 +15,12 @@ acceptance("Tag Groups", function (needs) {
         tag_names: ["monkey"],
         parent_tag_name: [],
         one_per_topic: false,
-        permissions: { everyone: 1 },
+        permissions: { 0: 1 },
       },
     };
-    server.post("/tag_groups", () => {
-      return helper.response(tagGroups);
-    });
-    server.get("/forum/tag_groups", () => {
-      return helper.response(tagGroups);
-    });
 
+    server.post("/tag_groups", () => helper.response(tagGroups));
+    server.get("/forum/tag_groups", () => helper.response(tagGroups));
     server.get("/groups/search.json", () => {
       return helper.response([
         {
@@ -79,8 +75,8 @@ acceptance("Tag Groups", function (needs) {
     await click(".tag-groups-sidebar li:first-child a");
 
     assert
-      .dom("#visible-permission:checked")
-      .exists("selected permission does not change after saving");
+      .dom(".tag-groups-sidebar")
+      .exists("tag group is saved and displayed in the sidebar");
   });
 
   test("going back to tags supports subfolder", async function (assert) {

app/models/group.rb

@@ -762,6 +762,8 @@ class Group < ActiveRecord::Base
   def self.group_id_from_param(group_param)
     return group_param.id if group_param.is_a?(Group)
     return group_param if group_param.is_a?(Integer)
+    return Group[group_param].id if group_param.is_a?(Symbol)
+    return group_param.to_i if group_param.to_i.to_s == group_param
 
     # subtle, using Group[] ensures the group exists in the DB
     Group[group_param.to_sym].id

app/models/tag_group.rb

@@ -56,9 +56,12 @@ class TagGroup < ActiveRecord::Base
   def self.resolve_permissions(permissions)
     permissions.map do |group, permission|
       group_id = Group.group_id_from_param(group)
-      permission = TagGroupPermission.permission_types[permission.to_sym] unless permission.is_a?(
-        Integer,
-      )
+      permission =
+        if permission.is_a?(Integer)
+          permission
+        else
+          TagGroupPermission.permission_types[permission.to_sym]
+        end
       [group_id, permission]
     end
   end

app/serializers/tag_group_serializer.rb

@@ -14,19 +14,8 @@ class TagGroupSerializer < ApplicationSerializer
   def permissions
     @permissions ||=
       begin
-        h = {}
-
-        object
-          .tag_group_permissions
-          .joins(:group)
-          .includes(:group)
-          .find_each do |tgp|
-            name = Group::AUTO_GROUP_IDS.fetch(tgp.group_id, tgp.group.name).to_s
-            h[name] = tgp.permission_type
-          end
-
-        h["everyone"] = TagGroupPermission.permission_types[:full] if h.empty?
-
+        h = object.tag_group_permissions.pluck(:group_id, :permission_type).to_h
+        h[0] = TagGroupPermission.permission_types[:full] if h.empty?
         h
       end
   end

config/locales/client.en.yml

@@ -4752,7 +4752,10 @@ en:
         everyone_can_use: "Tags can be used by everyone"
         usable_only_by_groups: "Tags are visible to everyone, but only the following groups can use them"
         visible_only_to_groups: "Tags are visible only to the following groups"
-        cannot_save: "Cannot save tag group. Make sure that there is at least one tag present, tag group name is not empty and less than 100 characters, and a group is selected for tags permission."
+        cannot_save: 
+          empty_name: "Cannot save tag group. Make sure the tag group name is not empty."
+          no_tags: "Cannot save tag group. Make sure that at least one tag is selected."
+          no_groups: "Cannot save tag group. Make sure that at least one group is selected for permission."
         tags_placeholder: "Search or create tags"
         parent_tag_placeholder: "Optional"
         select_groups_placeholder: "Select groups…"

spec/serializers/tag_group_serializer_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe TagGroupSerializer do
 
     serialized = TagGroupSerializer.new(tag_group, root: false).as_json
 
-    expect(serialized[:permissions].keys).to contain_exactly("staff")
+    expect(serialized[:permissions].keys).to contain_exactly(Group::AUTO_GROUPS[:staff])
   end
 
   it "doesn't return tag synonyms" do
@@ -21,6 +21,6 @@ RSpec.describe TagGroupSerializer do
     synonym = Fabricate(:tag, target_tag: tag)
     tag_group = Fabricate(:tag_group, tags: [tag, synonym])
     serialized = TagGroupSerializer.new(tag_group, root: false).as_json
-    expect(serialized[:tag_names]).to eq([tag.name])
+    expect(serialized[:tag_names]).to contain_exactly(tag.name)
   end
 end