From 03119312b57b308681b7f871468beaf2660b876d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 13 Jan 2025 11:29:04 +0100 Subject: [PATCH] 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/components/tag-groups-form.hbs | 3 - .../app/components/tag-groups-form.js | 151 +++++------------- .../discourse/app/models/tag-group.js | 4 +- .../tests/acceptance/tag-groups-test.js | 14 +- app/models/group.rb | 2 + app/models/tag_group.rb | 9 +- app/serializers/tag_group_serializer.rb | 15 +- config/locales/client.en.yml | 5 +- spec/serializers/tag_group_serializer_spec.rb | 4 +- 9 files changed, 65 insertions(+), 142 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/tag-groups-form.hbs b/app/assets/javascripts/discourse/app/components/tag-groups-form.hbs index acd0fd98761..ec2e9278db3 100644 --- a/app/assets/javascripts/discourse/app/components/tag-groups-form.hbs +++ b/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" /> diff --git a/app/assets/javascripts/discourse/app/components/tag-groups-form.js b/app/assets/javascripts/discourse/app/components/tag-groups-form.js index 635bfd3ab46..610371b577e 100644 --- a/app/assets/javascripts/discourse/app/components/tag-groups-form.js +++ b/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?.()), }); } } diff --git a/app/assets/javascripts/discourse/app/models/tag-group.js b/app/assets/javascripts/discourse/app/models/tag-group.js index 64d6412a918..7fc56f977bb 100644 --- a/app/assets/javascripts/discourse/app/models/tag-group.js +++ b/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"; diff --git a/app/assets/javascripts/discourse/tests/acceptance/tag-groups-test.js b/app/assets/javascripts/discourse/tests/acceptance/tag-groups-test.js index d1b23f5ee1b..546e99918bf 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/tag-groups-test.js +++ b/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) { diff --git a/app/models/group.rb b/app/models/group.rb index 4ff0cd13c3e..0106b02cc72 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/models/tag_group.rb b/app/models/tag_group.rb index d8eab81be69..b8c61724ef2 100644 --- a/app/models/tag_group.rb +++ b/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 diff --git a/app/serializers/tag_group_serializer.rb b/app/serializers/tag_group_serializer.rb index 6ccdd3e2d66..d8ff8c74d65 100644 --- a/app/serializers/tag_group_serializer.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index cf6621c69ca..57c658d04d5 100644 --- a/config/locales/client.en.yml +++ b/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…" diff --git a/spec/serializers/tag_group_serializer_spec.rb b/spec/serializers/tag_group_serializer_spec.rb index 56949522250..1165583371f 100644 --- a/spec/serializers/tag_group_serializer_spec.rb +++ b/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