From ad7d5426d8b6f647389873265a97a3e76d54112c Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Fri, 6 Sep 2024 17:22:42 +0200 Subject: [PATCH] FIX: supports groups field in post_created_edited (#28783) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⚠️ This commit is a revert of a revert due to a migration which was causing `{}` metadata to be transformed into `{"value": [null]}`. The new migration shouldn't cause this and will also clean the existing errors, there shouldn't be any data loss given the affected fields where not containing actual data. We might want to stop storing these empty fields in the future. To achieve it, this commit does the following: - create a new `groups field`, ideally we would have reused the existing group field, but many automations now have the expectation that this field will return a group id and not an array of group ids, which makes it a dangerous change - alter the code in `post_created_edited` to use this new groups field and change the logic to use an array - migrate the existing group fields post_created_edited automations to change name from `restricted_group` to `restricted_groups`, the component from `group` to `groups` and the metadata from `{"value": integer}` to `{"value": [integer]}` --- .../admin/components/automation-field.gjs | 2 + .../components/fields/da-groups-field.gjs | 55 ++++++++++++++ .../app/models/discourse_automation/field.rb | 6 ++ .../automation/config/locales/client.en.yml | 3 + ...121_migrate_fields_from_group_to_groups.rb | 36 ++++++++++ .../discourse_automation/event_handlers.rb | 10 +-- .../triggers/post_created_edited.rb | 2 +- .../spec/triggers/post_created_edited_spec.rb | 52 +++++++++----- .../components/da-groups-field-test.js | 71 +++++++++++++++++++ 9 files changed, 215 insertions(+), 22 deletions(-) create mode 100644 plugins/automation/admin/assets/javascripts/admin/components/fields/da-groups-field.gjs create mode 100644 plugins/automation/db/migrate/20240906142121_migrate_fields_from_group_to_groups.rb create mode 100644 plugins/automation/test/javascripts/integration/components/da-groups-field-test.js diff --git a/plugins/automation/admin/assets/javascripts/admin/components/automation-field.gjs b/plugins/automation/admin/assets/javascripts/admin/components/automation-field.gjs index 56e8576ec5c..9aeb2db0237 100644 --- a/plugins/automation/admin/assets/javascripts/admin/components/automation-field.gjs +++ b/plugins/automation/admin/assets/javascripts/admin/components/automation-field.gjs @@ -10,6 +10,7 @@ import DaCustomFields from "./fields/da-custom-fields"; import DaDateTimeField from "./fields/da-date-time-field"; import DaEmailGroupUserField from "./fields/da-email-group-user-field"; import DaGroupField from "./fields/da-group-field"; +import DaGroupsField from "./fields/da-groups-field"; import DaKeyValueField from "./fields/da-key-value-field"; import DaMessageField from "./fields/da-message-field"; import DaPeriodField from "./fields/da-period-field"; @@ -41,6 +42,7 @@ const FIELD_COMPONENTS = { "trust-levels": DaTrustLevelsField, category: DaCategoryField, group: DaGroupField, + groups: DaGroupsField, choices: DaChoicesField, category_notification_level: DaCategoryNotificationlevelField, email_group_user: DaEmailGroupUserField, diff --git a/plugins/automation/admin/assets/javascripts/admin/components/fields/da-groups-field.gjs b/plugins/automation/admin/assets/javascripts/admin/components/fields/da-groups-field.gjs new file mode 100644 index 00000000000..5324e66bd59 --- /dev/null +++ b/plugins/automation/admin/assets/javascripts/admin/components/fields/da-groups-field.gjs @@ -0,0 +1,55 @@ +import { tracked } from "@glimmer/tracking"; +import { hash } from "@ember/helper"; +import { action } from "@ember/object"; +import Group from "discourse/models/group"; +import GroupChooser from "select-kit/components/group-chooser"; +import BaseField from "./da-base-field"; +import DAFieldDescription from "./da-field-description"; +import DAFieldLabel from "./da-field-label"; + +export default class GroupsField extends BaseField { + @tracked allGroups = []; + + constructor() { + super(...arguments); + + Group.findAll({ + ignore_automatic: this.args.field.extra.ignore_automatic ?? false, + }).then((groups) => { + if (this.isDestroying || this.isDestroyed) { + return; + } + + this.allGroups = groups; + }); + } + + get maximum() { + return this.args.field.extra.maximum ?? 10; + } + + @action + setGroupField(groupIds) { + this.mutValue(groupIds); + } + + +} diff --git a/plugins/automation/app/models/discourse_automation/field.rb b/plugins/automation/app/models/discourse_automation/field.rb index 567598fc251..4dea55a2b26 100644 --- a/plugins/automation/app/models/discourse_automation/field.rb +++ b/plugins/automation/app/models/discourse_automation/field.rb @@ -188,6 +188,12 @@ module DiscourseAutomation "type" => "integer", }, }, + "groups" => { + "value" => { + "type" => "array", + "items" => [{ type: "integer" }], + }, + }, "email_group_user" => { "value" => { "type" => "array", diff --git a/plugins/automation/config/locales/client.en.yml b/plugins/automation/config/locales/client.en.yml index 28224d80fc0..9af98e2c8c5 100644 --- a/plugins/automation/config/locales/client.en.yml +++ b/plugins/automation/config/locales/client.en.yml @@ -156,6 +156,9 @@ en: restricted_group: label: Group description: Optional, will trigger only if the post's topic is a private message in this group's inbox + restricted_groups: + label: Groups + description: Optional, will trigger only if the post's topic is a private message in one of the group inboxes ignore_group_members: label: Ignore group members description: Skip the trigger if sender is a member of the Group specified above diff --git a/plugins/automation/db/migrate/20240906142121_migrate_fields_from_group_to_groups.rb b/plugins/automation/db/migrate/20240906142121_migrate_fields_from_group_to_groups.rb new file mode 100644 index 00000000000..10c6d708d5e --- /dev/null +++ b/plugins/automation/db/migrate/20240906142121_migrate_fields_from_group_to_groups.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +class MigrateFieldsFromGroupToGroups < ActiveRecord::Migration[7.1] + def up + # correct a previous erroneous migration which was converting `{}` to `{"value": [null]}` + execute <<-SQL + UPDATE discourse_automation_fields + SET + metadata = '{}'::jsonb + WHERE + metadata = '{"value": [null]}'::jsonb + AND component = 'groups' + AND name = 'restricted_groups'; + SQL + + execute <<-SQL + UPDATE discourse_automation_fields + SET + component = 'groups', + name = 'restricted_groups', + metadata = CASE + WHEN metadata != '{}'::jsonb THEN jsonb_set(metadata, '{value}', to_jsonb(ARRAY[(metadata->>'value')::int])) + ELSE metadata + END + FROM discourse_automation_automations + WHERE discourse_automation_fields.automation_id = discourse_automation_automations.id + AND discourse_automation_automations.trigger = 'post_created_edited' + AND discourse_automation_fields.name = 'restricted_group' + AND discourse_automation_fields.component = 'group'; + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/automation/lib/discourse_automation/event_handlers.rb b/plugins/automation/lib/discourse_automation/event_handlers.rb index cd0b8d75d43..3e3133024fa 100644 --- a/plugins/automation/lib/discourse_automation/event_handlers.rb +++ b/plugins/automation/lib/discourse_automation/event_handlers.rb @@ -54,15 +54,15 @@ module DiscourseAutomation next if (restricted_tags["value"] & topic.tags.map(&:name)).empty? end - restricted_group_id = automation.trigger_field("restricted_group")["value"] - if restricted_group_id.present? + restricted_group_ids = automation.trigger_field("restricted_groups")["value"] + if restricted_group_ids.present? next if !topic.private_message? target_group_ids = topic.allowed_groups.pluck(:id) - next if restricted_group_id != target_group_ids.first + next if (restricted_group_ids & target_group_ids).empty? - ignore_group_members = automation.trigger_field("ignore_group_members") - next if ignore_group_members["value"] && post.user.in_any_groups?([restricted_group_id]) + ignore_group_members = automation.trigger_field("ignore_group_members")["value"] + next if ignore_group_members && post.user.in_any_groups?(restricted_group_ids) end ignore_automated = automation.trigger_field("ignore_automated") diff --git a/plugins/automation/lib/discourse_automation/triggers/post_created_edited.rb b/plugins/automation/lib/discourse_automation/triggers/post_created_edited.rb index 9735a55efcc..9bf62631592 100644 --- a/plugins/automation/lib/discourse_automation/triggers/post_created_edited.rb +++ b/plugins/automation/lib/discourse_automation/triggers/post_created_edited.rb @@ -14,7 +14,7 @@ DiscourseAutomation::Triggerable.add(DiscourseAutomation::Triggers::POST_CREATED } field :restricted_category, component: :category field :restricted_tags, component: :tags - field :restricted_group, component: :group + field :restricted_groups, component: :groups field :ignore_automated, component: :boolean field :ignore_group_members, component: :boolean field :valid_trust_levels, component: :"trust-levels" diff --git a/plugins/automation/spec/triggers/post_created_edited_spec.rb b/plugins/automation/spec/triggers/post_created_edited_spec.rb index 159377400b2..363171eea28 100644 --- a/plugins/automation/spec/triggers/post_created_edited_spec.rb +++ b/plugins/automation/spec/triggers/post_created_edited_spec.rb @@ -164,30 +164,50 @@ describe "PostCreatedEdited" do context "when group is restricted" do fab!(:target_group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:another_group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone]) } before do automation.upsert_field!( - "restricted_group", - "group", - { value: target_group.id }, + "restricted_groups", + "groups", + { value: [target_group.id, another_group.id] }, target: "trigger", ) end - it "fires the trigger" do - list = - capture_contexts do - PostCreator.create( - user, - basic_topic_params.merge( - target_group_names: [target_group.name], - archetype: Archetype.private_message, - ), - ) - end + context "when PM is not sent to the group" do + it "doesnt fire the trigger" do + list = + capture_contexts do + PostCreator.create( + user, + basic_topic_params.merge( + target_group_names: [Fabricate(:group).name], + archetype: Archetype.private_message, + ), + ) + end - expect(list.length).to eq(1) - expect(list[0]["kind"]).to eq("post_created_edited") + expect(list.length).to eq(0) + end + end + + context "when PM is sent to the group" do + it "fires the trigger" do + list = + capture_contexts do + PostCreator.create( + user, + basic_topic_params.merge( + target_group_names: [target_group.name], + archetype: Archetype.private_message, + ), + ) + end + + expect(list.length).to eq(1) + expect(list[0]["kind"]).to eq("post_created_edited") + end end context "when the topic is not a PM" do diff --git a/plugins/automation/test/javascripts/integration/components/da-groups-field-test.js b/plugins/automation/test/javascripts/integration/components/da-groups-field-test.js new file mode 100644 index 00000000000..07fd87fcde3 --- /dev/null +++ b/plugins/automation/test/javascripts/integration/components/da-groups-field-test.js @@ -0,0 +1,71 @@ +import { getOwner } from "@ember/owner"; +import { render } from "@ember/test-helpers"; +import { hbs } from "ember-cli-htmlbars"; +import { module, test } from "qunit"; +import { setupRenderingTest } from "discourse/tests/helpers/component-test"; +import pretender, { response } from "discourse/tests/helpers/create-pretender"; +import selectKit from "discourse/tests/helpers/select-kit-helper"; +import AutomationFabricators from "discourse/plugins/automation/admin/lib/fabricators"; + +module("Integration | Component | da-groups-field", function (hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(function () { + this.automation = new AutomationFabricators(getOwner(this)).automation(); + + pretender.get("/groups/search.json", () => { + return response([ + { + id: 1, + name: "cats", + flair_url: "fa-bars", + flair_bg_color: "CC000A", + flair_color: "FFFFFA", + }, + { + id: 2, + name: "dogs", + flair_url: "fa-bars", + flair_bg_color: "CC000A", + flair_color: "FFFFFA", + }, + ]); + }); + }); + + test("set value", async function (assert) { + this.field = new AutomationFabricators(getOwner(this)).field({ + component: "groups", + }); + + await render( + hbs`` + ); + + await selectKit().expand(); + await selectKit().selectRowByValue(1); + + assert.deepEqual(this.field.metadata.value, [1]); + }); + + test("supports a maxmimum value", async function (assert) { + this.field = new AutomationFabricators(getOwner(this)).field({ + component: "groups", + extra: { maximum: 1 }, + }); + + await render( + hbs`` + ); + + await selectKit().expand(); + await selectKit().selectRowByValue(1); + + assert.deepEqual(this.field.metadata.value, [1]); + + await selectKit().expand(); + await selectKit().selectRowByValue(2); + + assert.deepEqual(this.field.metadata.value, [2]); + }); +});