FIX: Avoid zipping when adding users to groups (#30263)

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

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