[38614] Use the Groups::UpdateService to remove users from the group

https://community.openproject.org/wp/38614

modules/ldap_groups/app/models/ldap_groups/synchronized_group.rb

@@ -48,18 +48,19 @@ module LdapGroups
     def remove_members!(users_to_remove)
       return if users_to_remove.empty?
 
+      user_ids = users_to_remove.map(&method(:user_id))
+
       self.class.transaction do
         # 1) Delete synchronized group MEMBERSHIPS from collection.
-        # 2) Remove users from users collection of internal group.
+        users.delete users.where(user: user_ids).select(:id)
-        if users_to_remove.first.is_a? User
+
-          users.delete users.where(user: users_to_remove).select(:id)
+        # 2) Remove users from the internal group
-          group.users.delete users_to_remove
+        call = Groups::UpdateService
-        elsif users_to_remove.first.is_a? Integer
+          .new(user: User.system, model: group)
-          users.delete users.where(user_id: users_to_remove).select(:id)
+          .call(user_ids: group.user_ids - user_ids)
-          group.users.delete group.users.where(id: users_to_remove).select(:id)
+
-        else
+        call.on_failure do
-          raise ArgumentError,
+          Rails.logger.error "[LDAP groups] Failed to remove users #{user_ids} from #{group.name}: #{call.message}"
-                "Expected collection of Users or User IDs, got collection of #{users_to_remove.map(&:class).map(&:name).uniq.join(', ')}"
         end
       end
     end

modules/ldap_groups/spec/services/synchronization_spec.rb

@@ -309,20 +309,30 @@ describe LdapGroups::SynchronizeGroupsService, with_ee: %i[ldap_groups] do
   describe 'removing memberships' do
     context 'with a user in a group thats not in ldap' do
       let(:group_foo) { FactoryBot.create :group, lastname: 'foo_internal', members: [user_cc414, user_aa729] }
+      let(:manager)   { FactoryBot.create :role, name: 'Manager' }
+      let(:project) { FactoryBot.create :project, name: 'Project 1', identifier: 'project1', members: { group_foo => [manager] } }
 
       before do
+        project
         synced_foo.users.create(user: user_aa729)
         synced_foo.users.create(user: user_cc414)
-
-        subject
       end
 
       it 'removes the membership' do
+        expect(project.members.count).to eq 2
+        expect(project.users).to contain_exactly user_aa729, user_cc414
+
+        subject
+
         group_foo.reload
         synced_foo.reload
+        project.reload
 
         expect(group_foo.users).to eq([user_aa729])
         expect(synced_foo.users.pluck(:user_id)).to eq([user_aa729.id])
+
+        expect(project.members.count).to eq 1
+        expect(project.users).to contain_exactly user_aa729
       end
     end
   end