FIX: Publish membership update events when refreshing automatic groups. (#17668)

Adding or removing users from automatic groups is now consistent with `Group#add` and `Group#remove`.

app/jobs/regular/publish_group_membership_updates.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Jobs
+  class PublishGroupMembershipUpdates < ::Jobs::Base
+    def execute(args)
+      raise Discourse::InvalidParameters.new(:type) if !%w[add remove].include?(args[:type])
+
+      group = Group.find_by(id: args[:group_id])
+      return if !group
+
+      added_members = args[:type] == 'add'
+
+      User.human_users.where(id: args[:user_ids]).each do |user|
+        if added_members
+          group.trigger_user_added_event(user, group.automatic?)
+        else
+          group.trigger_user_removed_event(user)
+        end
+      end
+    end
+  end
+end

app/models/concerns/roleable.rb

@@ -35,7 +35,6 @@ module Roleable
     auto_approve_user
     enqueue_staff_welcome_message(:moderator)
     set_default_notification_levels(:moderators)
-    DiscourseEvent.trigger(:staff_granted, self, :moderator)
   end
 
   def revoke_moderation!
@@ -48,7 +47,6 @@ module Roleable
     auto_approve_user
     enqueue_staff_welcome_message(:admin)
     set_default_notification_levels(:admins)
-    DiscourseEvent.trigger(:staff_granted, self, :admin)
   end
 
   def revoke_admin!

app/models/group.rb

@@ -482,13 +482,21 @@ class Group < ActiveRecord::Base
         "SELECT id FROM users WHERE id <= 0 OR trust_level < #{id - 10} OR staged"
       end
 
-    DB.exec <<-SQL
+    removed_user_ids = DB.query_single <<-SQL
       DELETE FROM group_users
             USING (#{remove_subquery}) X
             WHERE group_id = #{group.id}
               AND user_id = X.id
+      RETURNING group_users.user_id
     SQL
 
+    if removed_user_ids.present?
+      Jobs.enqueue(
+        :publish_group_membership_updates,
+        user_ids: removed_user_ids, group_id: group.id, type: :remove
+      )
+    end
+
     # Add people to groups
     insert_subquery =
       case name
@@ -504,16 +512,24 @@ class Group < ActiveRecord::Base
         "SELECT id FROM users WHERE id > 0 AND NOT staged"
       end
 
-    DB.exec <<-SQL
+    added_user_ids = DB.query_single <<-SQL
       INSERT INTO group_users (group_id, user_id, created_at, updated_at)
            SELECT #{group.id}, X.id, now(), now()
              FROM group_users
        RIGHT JOIN (#{insert_subquery}) X ON X.id = user_id AND group_id = #{group.id}
             WHERE user_id IS NULL
+       RETURNING group_users.user_id
     SQL
 
     group.save!
 
+    if added_user_ids.present?
+      Jobs.enqueue(
+        :publish_group_membership_updates,
+        user_ids: added_user_ids, group_id: group.id, type: :add
+      )
+    end
+
     # we want to ensure consistency
     Group.reset_counters(group.id, :group_users)
 
@@ -631,7 +647,7 @@ class Group < ActiveRecord::Base
       if group = find_by(id: id)
         unless GroupUser.where(group_id: id, user_id: user_id).exists?
           group_user = group.group_users.create!(user_id: user_id)
-          DiscourseEvent.trigger(:user_added_to_group, group_user.user, group, automatic: true)
+          group.trigger_user_added_event(group_user.user, true)
         end
       else
         name = AUTO_GROUP_IDS[trust_level]
@@ -704,7 +720,7 @@ class Group < ActiveRecord::Base
       Discourse.request_refresh!(user_ids: [user.id])
     end
 
-    DiscourseEvent.trigger(:user_added_to_group, user, self, automatic: automatic)
+    trigger_user_added_event(user, automatic)
 
     self
   end
@@ -716,7 +732,7 @@ class Group < ActiveRecord::Base
     has_webhooks = WebHook.active_web_hooks(:group_user)
     payload = WebHook.generate_payload(:group_user, group_user, WebHookGroupUserSerializer) if has_webhooks
     group_user.destroy
-    DiscourseEvent.trigger(:user_removed_from_group, user, self)
+    trigger_user_removed_event(user)
     WebHook.enqueue_hooks(:group_user, :user_removed_from_group,
       id: group_user.id,
       payload: payload
@@ -724,6 +740,14 @@ class Group < ActiveRecord::Base
     true
   end
 
+  def trigger_user_added_event(user, automatic)
+    DiscourseEvent.trigger(:user_added_to_group, user, self, automatic: automatic)
+  end
+
+  def trigger_user_removed_event(user)
+    DiscourseEvent.trigger(:user_removed_from_group, user, self)
+  end
+
   def add_owner(user)
     if group_user = self.group_users.find_by(user: user)
       group_user.update!(owner: true) if !group_user.owner

spec/jobs/regular/publish_group_membership_updates_spec.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+describe Jobs::PublishGroupMembershipUpdates do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:group) { Fabricate(:group) }
+
+  it 'publishes events for added users' do
+    events = DiscourseEvent.track_events do
+      subject.execute(
+        user_ids: [user.id], group_id: group.id, type: 'add'
+      )
+    end
+
+    expect(events).to include(
+      event_name: :user_added_to_group,
+      params: [user, group, { automatic: group.automatic }]
+    )
+  end
+
+  it 'publishes events for removed users' do
+    events = DiscourseEvent.track_events do
+      subject.execute(
+        user_ids: [user.id], group_id: group.id, type: 'remove'
+      )
+    end
+
+    expect(events).to include(
+      event_name: :user_removed_from_group,
+      params: [user, group]
+    )
+  end
+
+  it "does nothing if the group doesn't exist" do
+    events = DiscourseEvent.track_events do
+      subject.execute(
+        user_ids: [user.id], group_id: nil, type: 'add'
+      )
+    end
+
+    expect(events).not_to include(
+      event_name: :user_added_to_group,
+      params: [user, group, { automatic: group.automatic }]
+    )
+  end
+
+  it 'fails when the update type is invalid' do
+    expect {
+      subject.execute(
+        user_ids: [user.id], group_id: nil, type: nil
+      )
+    }.to raise_error(Discourse::InvalidParameters)
+  end
+
+  it 'does nothing when the user is not human' do
+    events = DiscourseEvent.track_events do
+      subject.execute(
+        user_ids: [Discourse.system_user.id], group_id: nil, type: 'add'
+      )
+    end
+
+    expect(events).not_to include(
+      event_name: :user_added_to_group,
+      params: [user, group, { automatic: group.automatic }]
+    )
+  end
+end

spec/models/group_spec.rb

@@ -242,6 +242,41 @@ describe Group do
       expect(GroupUser.where(user_id: staged.id).count).to eq(2)
     end
 
+    describe 'after updating automatic group members' do
+      fab!(:user) { Fabricate(:user) }
+
+      it 'triggers an event when a user is removed from an automatic group' do
+        tl3_users = Group.find(Group::AUTO_GROUPS[:trust_level_3])
+        tl3_users.add(user)
+
+        events = DiscourseEvent.track_events do
+          Group.refresh_automatic_group!(:trust_level_3)
+        end
+
+        expect(GroupUser.exists?(group: tl3_users, user: user)).to eq(false)
+        publish_event_job_args = Jobs::PublishGroupMembershipUpdates.jobs.last['args'].first
+        expect(publish_event_job_args["user_ids"]).to include(user.id)
+        expect(publish_event_job_args["group_id"]).to eq(tl3_users.id)
+        expect(publish_event_job_args["type"]).to include('remove')
+      end
+
+      it 'triggers an event when a user is added to an automatic group' do
+        tl0_users = Group.find(Group::AUTO_GROUPS[:trust_level_0])
+
+        expect(GroupUser.exists?(group: tl0_users, user: user)).to eq(false)
+
+        events = DiscourseEvent.track_events do
+          Group.refresh_automatic_group!(:trust_level_0)
+        end
+
+        expect(GroupUser.exists?(group: tl0_users, user: user)).to eq(true)
+        publish_event_job_args = Jobs::PublishGroupMembershipUpdates.jobs.last['args'].first
+        expect(publish_event_job_args["user_ids"]).to include(user.id)
+        expect(publish_event_job_args["group_id"]).to eq(tl0_users.id)
+        expect(publish_event_job_args["type"]).to eq('add')
+      end
+    end
+
     it "makes sure the everyone group is not visible except to staff" do
       g = Group.refresh_automatic_group!(:everyone)
       expect(g.visibility_level).to eq(Group.visibility_levels[:staff])