From f1c3670d74c161dd443a7a8baab486a6225bb0bd Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 27 Jul 2022 11:34:08 -0300 Subject: [PATCH] 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`. --- .../publish_group_membership_updates.rb | 22 +++++++ app/models/concerns/roleable.rb | 2 - app/models/group.rb | 34 ++++++++-- .../publish_group_membership_updates_spec.rb | 66 +++++++++++++++++++ spec/models/group_spec.rb | 35 ++++++++++ 5 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 app/jobs/regular/publish_group_membership_updates.rb create mode 100644 spec/jobs/regular/publish_group_membership_updates_spec.rb diff --git a/app/jobs/regular/publish_group_membership_updates.rb b/app/jobs/regular/publish_group_membership_updates.rb new file mode 100644 index 00000000000..a9bc5dc93ea --- /dev/null +++ b/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 diff --git a/app/models/concerns/roleable.rb b/app/models/concerns/roleable.rb index aca850a10f7..4a9a6be3ddb 100644 --- a/app/models/concerns/roleable.rb +++ b/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! diff --git a/app/models/group.rb b/app/models/group.rb index 0f85725caba..1228288ca5e 100644 --- a/app/models/group.rb +++ b/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 diff --git a/spec/jobs/regular/publish_group_membership_updates_spec.rb b/spec/jobs/regular/publish_group_membership_updates_spec.rb new file mode 100644 index 00000000000..822c39e7111 --- /dev/null +++ b/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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index cdc6f6243cf..4c2015d3d24 100644 --- a/spec/models/group_spec.rb +++ b/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])