diff --git a/plugins/automation/lib/discourse_automation/scripts/add_user_to_group_through_custom_field.rb b/plugins/automation/lib/discourse_automation/scripts/add_user_to_group_through_custom_field.rb index 9d077665bde..7e214a9a913 100644 --- a/plugins/automation/lib/discourse_automation/scripts/add_user_to_group_through_custom_field.rb +++ b/plugins/automation/lib/discourse_automation/scripts/add_user_to_group_through_custom_field.rb @@ -16,36 +16,28 @@ DiscourseAutomation::Scriptable.add( triggerables %i[recurring user_first_logged_in] script do |trigger, fields| - custom_field_name = fields.dig("custom_field_name", "value") - case trigger["kind"] when DiscourseAutomation::Triggers::API_CALL, DiscourseAutomation::Triggers::RECURRING - query = - DB.query(<<-SQL, prefix: ::User::USER_FIELD_PREFIX, custom_field_name: custom_field_name) - SELECT u.id as user_id, g.id as group_id - FROM users u - JOIN user_custom_fields ucf - ON u.id = ucf.user_id - AND ucf.name = CONCAT(:prefix, :custom_field_name) - JOIN groups g - on g.full_name ilike ucf.value - FULL OUTER JOIN group_users gu - ON gu.user_id = u.id - AND gu.group_id = g.id - WHERE gu.id is null - AND u.active = true - ORDER BY 1, 2 - SQL + custom_field_name = "#{::User::USER_FIELD_PREFIX}#{fields.dig("custom_field_name", "value")}" - groups_by_id = {} + # mapping of group full_names to ids for quick lookup + group_ids_by_name = Group.where.not(full_name: [nil, ""]).pluck(:full_name, :id).to_h + # find users with the custom field who aren't in their designated group User - .where(id: query.map(&:user_id)) - .order(:id) - .zip(query) do |user, query_row| - group_id = query_row.group_id - group = groups_by_id[group_id] ||= Group.find(group_id) + .joins( + "JOIN user_custom_fields ucf ON users.id = ucf.user_id AND ucf.name = '#{custom_field_name}'", + ) + .where(active: true) + .where("ucf.value IS NOT NULL AND ucf.value != ''") + .where( + "NOT EXISTS ( SELECT 1 FROM group_users gu JOIN groups g ON g.id = gu.group_id WHERE gu.user_id = users.id AND g.full_name = ucf.value)", + ) + .select("users.id, ucf.value as group_name") + .find_each do |user| + next unless group_id = group_ids_by_name[user.group_name] + group = Group.find(group_id) group.add(user) GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user) end @@ -58,7 +50,7 @@ DiscourseAutomation::Scriptable.add( WHERE ucf.user_id = :user_id AND ucf.name = CONCAT(:prefix, :custom_field_name) SQL prefix: ::User::USER_FIELD_PREFIX, - custom_field_name: custom_field_name, + custom_field_name: fields.dig("custom_field_name", "value"), user_id: trigger["user"].id, ).first next if !group_name diff --git a/plugins/automation/spec/scripts/add_user_to_group_through_custom_field_spec.rb b/plugins/automation/spec/scripts/add_user_to_group_through_custom_field_spec.rb index 9c240ea81d8..b331338bd1b 100644 --- a/plugins/automation/spec/scripts/add_user_to_group_through_custom_field_spec.rb +++ b/plugins/automation/spec/scripts/add_user_to_group_through_custom_field_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -describe "AddUserTogroupThroughCustomField" do +describe "AddUserToGroupThroughCustomField" do fab!(:user_1) { Fabricate(:user) } fab!(:user_2) { Fabricate(:user) } fab!(:target_group) { Fabricate(:group, full_name: "Groupity Group") } @@ -51,6 +51,22 @@ describe "AddUserTogroupThroughCustomField" do end end + context "with empty field value" do + before do + UserCustomField.create!(user_id: user_1.id, name: "user_field_#{user_field.id}", value: "") + end + + it "does not add user to groups with empty fullnames" do + empty_fullname_group1 = Fabricate(:group, full_name: "") + empty_fullname_group2 = Fabricate(:group, full_name: "") + + automation.trigger!("kind" => DiscourseAutomation::Triggers::RECURRING) + + expect(user_1.reload.in_any_groups?([empty_fullname_group1.id])).to eq(false) + expect(user_1.reload.in_any_groups?([empty_fullname_group2.id])).to eq(false) + end + end + context "when group is already present" do before { target_group.add(user_1) } @@ -117,4 +133,40 @@ describe "AddUserTogroupThroughCustomField" do end end end + + context "with many users and groups" do + fab!(:bangalore) { Fabricate(:group, full_name: "Bangalore, India") } + fab!(:dublin) { Fabricate(:group, full_name: "Dublin, Ireland") } + fab!(:iowa) { Fabricate(:group, full_name: "Iowa") } + fab!(:missouri) { Fabricate(:group, full_name: "Missouri") } + + fab!(:user1) { Fabricate(:user, id: bangalore.id) } + fab!(:user2) { Fabricate(:user) } + fab!(:user3) { Fabricate(:user) } + fab!(:user4) { Fabricate(:user) } + fab!(:user5) { Fabricate(:user) } + fab!(:user6) { Fabricate(:user) } + + before do + [ + [user1, "Dublin, Ireland"], + [user4, "Iowa"], + [user2, "Bangalore, India"], + [user5, ""], + [user6, "Missouri"], + [user3, "Bangalore, India"], + ].each do |user, location| + UserCustomField.create!(user: user, name: "user_field_#{user_field.id}", value: location) + end + end + + it "adds users to their intended groups" do + automation.trigger!("kind" => DiscourseAutomation::Triggers::RECURRING) + + expect(bangalore.users).to match_array([user2, user3]) + expect(dublin.users).to eq([user1]) + expect(iowa.users).to eq([user4]) + expect(missouri.users).to eq([user6]) + end + end end