Merge branch '354749-refactor-members-update-service-multiple-members' into 'master'

Refactor Members::UpdateService to update multiple members

See merge request !96745



Merged-by: Alex Pooley <apooley@gitlab.com>
Approved-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Approved-by: Alex Pooley <apooley@gitlab.com>
Co-authored-by: Abdul Wadood <awadood@gitlab.com>

.rubocop_todo/layout/line_length.yml

@@ -597,7 +597,6 @@ Layout/LineLength:
     - 'app/services/loose_foreign_keys/cleaner_service.rb'
     - 'app/services/members/destroy_service.rb'
     - 'app/services/members/invitation_reminder_email_service.rb'
-    - 'app/services/members/update_service.rb'
     - 'app/services/merge_requests/add_context_service.rb'
     - 'app/services/merge_requests/assign_issues_service.rb'
     - 'app/services/merge_requests/base_service.rb'
@@ -5539,7 +5538,6 @@ Layout/LineLength:
     - 'spec/services/members/destroy_service_spec.rb'
     - 'spec/services/members/invitation_reminder_email_service_spec.rb'
     - 'spec/services/members/unassign_issuables_service_spec.rb'
-    - 'spec/services/members/update_service_spec.rb'
     - 'spec/services/merge_requests/add_context_service_spec.rb'
     - 'spec/services/merge_requests/after_create_service_spec.rb'
     - 'spec/services/merge_requests/assign_issues_service_spec.rb'

.rubocop_todo/rspec/context_wording.yml

@@ -2972,7 +2972,6 @@ RSpec/ContextWording:
     - 'spec/services/members/destroy_service_spec.rb'
     - 'spec/services/members/groups/creator_service_spec.rb'
     - 'spec/services/members/projects/creator_service_spec.rb'
-    - 'spec/services/members/update_service_spec.rb'
     - 'spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb'
     - 'spec/services/merge_requests/after_create_service_spec.rb'
     - 'spec/services/merge_requests/approval_service_spec.rb'

.rubocop_todo/style/if_unless_modifier.yml

@@ -309,7 +309,6 @@ Style/IfUnlessModifier:
     - 'app/services/issues/update_service.rb'
     - 'app/services/lfs/lock_file_service.rb'
     - 'app/services/members/destroy_service.rb'
-    - 'app/services/members/update_service.rb'
     - 'app/services/merge_requests/add_context_service.rb'
     - 'app/services/merge_requests/base_service.rb'
     - 'app/services/merge_requests/build_service.rb'

app/services/members/update_service.rb

@@ -2,40 +2,84 @@
 
 module Members
   class UpdateService < Members::BaseService
-    # returns the updated member
-    def execute(member, permission: :update)
-      raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), member)
-      raise Gitlab::Access::AccessDeniedError if prevent_upgrade_to_owner?(member) || prevent_downgrade_from_owner?(member)
+    # @param members [Member, Array<Member>]
+    # returns the updated member(s)
+    def execute(members, permission: :update)
+      members = Array.wrap(members)
 
-      return success(member: member) if update_results_in_no_change?(member)
+      old_access_level_expiry_map = members.to_h do |member|
+        [member.id, { human_access: member.human_access, expires_at: member.expires_at }]
+      end
+
+      if Feature.enabled?(:bulk_update_membership_roles, current_user)
+        multiple_members_update(members, permission, old_access_level_expiry_map)
+      else
+        single_member_update(members.first, permission, old_access_level_expiry_map)
+      end
+
+      prepare_response(members)
+    end
+
+    private
 
-      old_access_level = member.human_access
-      old_expiry = member.expires_at
+    def single_member_update(member, permission, old_access_level_expiry_map)
+      raise Gitlab::Access::AccessDeniedError unless has_update_permissions?(member, permission)
 
       member.attributes = params
       return success(member: member) unless member.changed?
 
-      if member.save
-        after_execute(action: permission, old_access_level: old_access_level, old_expiry: old_expiry, member: member)
+      post_update(member, permission, old_access_level_expiry_map) if member.save
+    end
 
-        # Deletes only confidential issues todos for guests
-        enqueue_delete_todos(member) if downgrading_to_guest?
+    def multiple_members_update(members, permission, old_access_level_expiry_map)
+      begin
+        updated_members =
+          Member.transaction do
+            # Using `next` with `filter_map` avoids the `post_update` call for the member that resulted in no change
+            members.filter_map do |member|
+              raise Gitlab::Access::AccessDeniedError unless has_update_permissions?(member, permission)
+
+              member.attributes = params
+              next unless member.changed?
+
+              member.save!
+              member
+            end
+          end
+      rescue ActiveRecord::RecordInvalid
+        return
       end
 
-      if member.errors.any?
-        error(member.errors.full_messages.to_sentence, pass_back: { member: member })
-      else
-        success(member: member)
-      end
+      updated_members.each { |member| post_update(member, permission, old_access_level_expiry_map) }
     end
 
-    private
+    def post_update(member, permission, old_access_level_expiry_map)
+      old_access_level = old_access_level_expiry_map[member.id][:human_access]
+      old_expiry = old_access_level_expiry_map[member.id][:expires_at]
 
-    def update_results_in_no_change?(member)
-      return false if params[:expires_at]&.to_date != member.expires_at
-      return false if params[:access_level] != member.access_level
+      after_execute(action: permission, old_access_level: old_access_level, old_expiry: old_expiry, member: member)
+      enqueue_delete_todos(member) if downgrading_to_guest? # Deletes only confidential issues todos for guests
+    end
+
+    def prepare_response(members)
+      errored_member = members.detect { |member| member.errors.any? }
+      if errored_member.present?
+        return error(errored_member.errors.full_messages.to_sentence, pass_back: { member: errored_member })
+      end
+
+      # TODO: Remove the :member key when removing the bulk_update_membership_roles FF and update where it's used.
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/373257
+      if members.one?
+        success(member: members.first)
+      else
+        success(members: members)
+      end
+    end
 
-      true
+    def has_update_permissions?(member, permission)
+      can?(current_user, action_member_permission(permission, member), member) &&
+        !prevent_upgrade_to_owner?(member) &&
+        !prevent_downgrade_from_owner?(member)
     end
 
     def downgrading_to_guest?

config/feature_flags/development/bulk_update_membership_roles.yml

+---
+name: bulk_update_membership_roles
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96745
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/373257
+milestone: '15.6'
+type: development
+group: group::workspace
+default_enabled: false

spec/services/members/update_service_spec.rb

@@ -3,23 +3,34 @@
 require 'spec_helper'
 
 RSpec.describe Members::UpdateService do
-  let(:project) { create(:project, :public) }
-  let(:group) { create(:group, :public) }
-  let(:current_user) { create(:user) }
-  let(:member_user) { create(:user) }
-  let(:permission) { :update }
-  let(:member) { source.members_and_requesters.find_by!(user_id: member_user.id) }
-  let(:access_level) { Gitlab::Access::MAINTAINER }
-  let(:params) do
-    { access_level: access_level }
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:group) { create(:group, :public) }
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:member_user1) { create(:user) }
+  let_it_be(:member_user2) { create(:user) }
+  let_it_be(:member_users) { [member_user1, member_user2] }
+  let_it_be(:permission) { :update }
+  let_it_be(:access_level) { Gitlab::Access::MAINTAINER }
+  let(:members) { source.members_and_requesters.where(user_id: member_users).to_a }
+  let(:update_service) { described_class.new(current_user, params) }
+  let(:params) { { access_level: access_level } }
+  let(:updated_members) do
+    result = subject
+    Array.wrap(result[:members] || result[:member])
   end
 
   before do
-    project.add_developer(member_user)
-    group.add_developer(member_user)
-  end
+    member_users.first.tap do |member_user|
+      expires_at = 10.days.from_now
+      project.add_member(member_user, Gitlab::Access::DEVELOPER, expires_at: expires_at)
+      group.add_member(member_user, Gitlab::Access::DEVELOPER, expires_at: expires_at)
+    end
 
-  subject { described_class.new(current_user, params).execute(member, permission: permission) }
+    member_users[1..].each do |member_user|
+      project.add_developer(member_user)
+      group.add_developer(member_user)
+    end
+  end
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
@@ -28,209 +39,326 @@
     end
   end
 
-  shared_examples 'a service updating a member' do
-    it 'updates the member' do
-      expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name)
+  shared_examples 'current user cannot update the given members' do
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let_it_be(:source) { project }
+    end
 
-      updated_member = subject.fetch(:member)
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let_it_be(:source) { group }
+    end
+  end
+
+  shared_examples 'returns error status when params are invalid' do
+    let_it_be(:params) { { expires_at: 2.days.ago } }
 
-      expect(updated_member).to be_valid
-      expect(updated_member.access_level).to eq(access_level)
+    specify do
+      expect(subject[:status]).to eq(:error)
+    end
+  end
+
+  shared_examples 'a service updating members' do
+    it 'updates the members' do
+      new_access_levels = updated_members.map(&:access_level)
+
+      expect(updated_members).not_to be_empty
+      expect(updated_members).to all(be_valid)
+      expect(new_access_levels).to all(be access_level)
     end
 
     it 'returns success status' do
-      result = subject.fetch(:status)
+      expect(subject.fetch(:status)).to eq(:success)
+    end
+
+    it 'invokes after_execute with correct args' do
+      members.each do |member|
+        expect(update_service).to receive(:after_execute).with(
+          action: permission,
+          old_access_level: member.human_access,
+          old_expiry: member.expires_at,
+          member: member
+        )
+      end
 
-      expect(result).to eq(:success)
+      subject
     end
 
-    context 'when member is downgraded to guest' do
-      shared_examples 'schedules to delete confidential todos' do
-        it do
-          expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once
+    it 'authorization update callback is triggered' do
+      expect(members).to all(receive(:refresh_member_authorized_projects).once)
 
-          updated_member = subject.fetch(:member)
+      subject
+    end
+
+    it 'does not enqueues todos for deletion' do
+      members.each do |member|
+        expect(TodosDestroyer::EntityLeaveWorker)
+          .not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name)
+      end
 
-          expect(updated_member).to be_valid
-          expect(updated_member.access_level).to eq(Gitlab::Access::GUEST)
+      subject
+    end
+
+    context 'when members are downgraded to guest' do
+      shared_examples 'schedules to delete confidential todos' do
+        it do
+          members.each do |member|
+            expect(TodosDestroyer::EntityLeaveWorker)
+              .to receive(:perform_in)
+                    .with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once
+          end
+
+          new_access_levels = updated_members.map(&:access_level)
+          expect(updated_members).to all(be_valid)
+          expect(new_access_levels).to all(be Gitlab::Access::GUEST)
         end
       end
 
       context 'with Gitlab::Access::GUEST level as a string' do
-        let(:params) { { access_level: Gitlab::Access::GUEST.to_s } }
+        let_it_be(:params) { { access_level: Gitlab::Access::GUEST.to_s } }
 
         it_behaves_like 'schedules to delete confidential todos'
       end
 
       context 'with Gitlab::Access::GUEST level as an integer' do
-        let(:params) { { access_level: Gitlab::Access::GUEST } }
+        let_it_be(:params) { { access_level: Gitlab::Access::GUEST } }
 
         it_behaves_like 'schedules to delete confidential todos'
       end
     end
 
     context 'when access_level is invalid' do
-      let(:params) { { access_level: 'invalid' } }
+      let_it_be(:params) { { access_level: 'invalid' } }
 
       it 'raises an error' do
-        expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"')
+        expect { described_class.new(current_user, params) }
+          .to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"')
       end
     end
 
-    context 'when member is not valid' do
-      let(:params) { { expires_at: 2.days.ago } }
+    context 'when members update results in no change' do
+      let(:params) { { access_level: members.first.access_level } }
 
-      it 'returns error status' do
-        result = subject
+      it 'does not invoke update! and post_update' do
+        expect(update_service).not_to receive(:save!)
+        expect(update_service).not_to receive(:post_update)
 
-        expect(result[:status]).to eq(:error)
+        expect(subject[:status]).to eq(:success)
       end
-    end
-  end
-
-  context 'when current user cannot update the given member' do
-    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-      let(:source) { project }
-    end
-
-    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-      let(:source) { group }
-    end
-  end
-
-  context 'when current user can update the given member' do
-    before do
-      project.add_maintainer(current_user)
-      group.add_owner(current_user)
-    end
 
-    it_behaves_like 'a service updating a member' do
-      let(:source) { project }
-    end
+      it 'authorization update callback is not triggered' do
+        members.each { |member| expect(member).not_to receive(:refresh_member_authorized_projects) }
 
-    it_behaves_like 'a service updating a member' do
-      let(:source) { group }
+        subject
+      end
     end
   end
 
-  context 'in a project' do
+  shared_examples 'updating a project' do
     let_it_be(:group_project) { create(:project, group: create(:group)) }
+    let_it_be(:source) { group_project }
 
-    let(:source) { group_project }
+    before do
+      member_users.each { |member_user| group_project.add_developer(member_user) }
+    end
 
-    context 'a project maintainer' do
+    context 'as a project maintainer' do
       before do
         group_project.add_maintainer(current_user)
       end
 
-      context 'cannot update a member to OWNER' do
-        before do
-          group_project.add_developer(member_user)
-        end
+      it_behaves_like 'a service updating members'
+
+      context 'when member update results in an error' do
+        it_behaves_like 'a service returning an error'
+      end
 
+      context 'and updating members to OWNER' do
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-          let(:access_level) { Gitlab::Access::OWNER }
+          let_it_be(:access_level) { Gitlab::Access::OWNER }
         end
       end
 
-      context 'cannot update themselves to OWNER' do
-        let(:member) { source.members_and_requesters.find_by!(user_id: current_user.id) }
-
-        before do
-          group_project.add_developer(member_user)
-        end
+      context 'and updating themselves to OWNER' do
+        let(:members) { source.members_and_requesters.find_by!(user_id: current_user.id) }
 
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-          let(:access_level) { Gitlab::Access::OWNER }
+          let_it_be(:access_level) { Gitlab::Access::OWNER }
         end
       end
 
-      context 'cannot downgrade a member from OWNER' do
+      context 'and downgrading members from OWNER' do
         before do
-          group_project.add_owner(member_user)
+          member_users.each { |member_user| group_project.add_owner(member_user) }
         end
 
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-          let(:access_level) { Gitlab::Access::MAINTAINER }
+          let_it_be(:access_level) { Gitlab::Access::MAINTAINER }
         end
       end
     end
 
-    context 'owners' do
+    context 'when current_user is considered an owner in the project via inheritance' do
       before do
-        # so that `current_user` is considered an `OWNER` in the project via inheritance.
         group_project.group.add_owner(current_user)
       end
 
-      context 'can update a member to OWNER' do
+      context 'and can update members to OWNER' do
         before do
-          group_project.add_developer(member_user)
+          member_users.each { |member_user| group_project.add_developer(member_user) }
         end
 
-        it_behaves_like 'a service updating a member' do
-          let(:access_level) { Gitlab::Access::OWNER }
+        it_behaves_like 'a service updating members' do
+          let_it_be(:access_level) { Gitlab::Access::OWNER }
         end
       end
 
-      context 'can downgrade a member from OWNER' do
+      context 'and can downgrade members from OWNER' do
         before do
-          group_project.add_owner(member_user)
+          member_users.each { |member_user| group_project.add_owner(member_user) }
         end
 
-        it_behaves_like 'a service updating a member' do
-          let(:access_level) { Gitlab::Access::MAINTAINER }
+        it_behaves_like 'a service updating members' do
+          let_it_be(:access_level) { Gitlab::Access::MAINTAINER }
         end
       end
     end
   end
 
-  context 'authorization updates' do
-    let_it_be(:user) { create(:user) }
+  shared_examples 'updating a group' do
+    let_it_be(:source) { group }
 
-    shared_examples 'manages authorization updates' do
-      context 'access level changes' do
-        let(:params) do
-          { access_level: Gitlab::Access::MAINTAINER }
-        end
+    before do
+      group.add_owner(current_user)
+    end
 
-        it 'authorization update callback is triggered' do
-          expect(member).to receive(:refresh_member_authorized_projects).once
+    it_behaves_like 'a service updating members'
 
-          described_class.new(current_user, params).execute(member, permission: permission)
-        end
+    context 'when member update results in an error' do
+      it_behaves_like 'a service returning an error'
+    end
+
+    context 'when group members expiration date is updated' do
+      let_it_be(:params) { { expires_at: 20.days.from_now } }
+      let(:notification_service) { instance_double(NotificationService) }
+
+      before do
+        allow(NotificationService).to receive(:new).and_return(notification_service)
       end
 
-      context 'no attribute changes' do
-        let(:params) do
-          { access_level: Gitlab::Access::DEVELOPER }
+      it 'emails the users that their group membership expiry has changed' do
+        members.each do |member|
+          expect(notification_service).to receive(:updated_group_member_expiration).with(member)
         end
 
-        it 'authorization update callback is not triggered' do
-          expect(member).not_to receive(:refresh_member_authorized_projects)
+        subject
+      end
+    end
+  end
 
-          described_class.new(current_user, params).execute(member, permission: permission)
+  context 'when :bulk_update_membership_roles feature flag is disabled' do
+    let(:member) { source.members_and_requesters.find_by!(user_id: member_user1.id) }
+    let(:members) { [member] }
+
+    subject { update_service.execute(member, permission: permission) }
+
+    shared_examples 'a service returning an error' do
+      before do
+        allow(member).to receive(:save) do
+          member.errors.add(:user_id)
+          member.errors.add(:access_level)
         end
+          .and_return(false)
+      end
+
+      it_behaves_like 'returns error status when params are invalid'
+
+      it 'returns the error' do
+        response = subject
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('User is invalid and Access level is invalid')
       end
     end
 
-    context 'group member' do
-      let(:source) { group }
+    before do
+      stub_feature_flags(bulk_update_membership_roles: false)
+    end
+
+    it_behaves_like 'current user cannot update the given members'
+    it_behaves_like 'updating a project'
+    it_behaves_like 'updating a group'
+  end
+
+  subject { update_service.execute(members, permission: permission) }
+
+  shared_examples 'a service returning an error' do
+    it_behaves_like 'returns error status when params are invalid'
+
+    context 'when a member update results in invalid record' do
+      let(:member2) { members.second }
 
       before do
-        group.add_owner(current_user)
+        allow(member2).to receive(:save!) do
+          member2.errors.add(:user_id)
+          member2.errors.add(:access_level)
+        end.and_raise(ActiveRecord::RecordInvalid)
+      end
+
+      it 'returns the error' do
+        response = subject
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('User is invalid and Access level is invalid')
+      end
+
+      it 'rollbacks back the entire update' do
+        old_access_levels = members.pluck(:access_level)
+
+        subject
+
+        expect(members.each(&:reset).pluck(:access_level)).to eq(old_access_levels)
       end
+    end
+  end
+
+  it_behaves_like 'current user cannot update the given members'
+  it_behaves_like 'updating a project'
+  it_behaves_like 'updating a group'
 
-      include_examples 'manages authorization updates'
+  context 'with a single member' do
+    let(:member) { create(:group_member, group: group) }
+    let(:members) { member }
+
+    before do
+      group.add_owner(current_user)
     end
 
-    context 'project member' do
-      let(:source) { project }
+    it 'returns the correct response' do
+      expect(subject[:member]).to eq(member)
+    end
+  end
+
+  context 'when current user is an admin', :enable_admin_mode do
+    let_it_be(:current_user) { create(:admin) }
+    let_it_be(:source) { group }
 
+    context 'when all owners are being downgraded' do
       before do
-        project.add_maintainer(current_user)
+        member_users.each { |member_user| group.add_owner(member_user) }
+      end
+
+      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
+    end
+
+    context 'when all blocked owners are being downgraded' do
+      before do
+        member_users.each do |member_user|
+          group.add_owner(member_user)
+          member_user.block
+        end
       end
 
-      include_examples 'manages authorization updates'
+      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
     end
   end
 end

spec/support/rspec_order_todo.yml

@@ -10079,7 +10079,6 @@
 - './spec/services/members/request_access_service_spec.rb'
 - './spec/services/members/standard_member_builder_spec.rb'
 - './spec/services/members/unassign_issuables_service_spec.rb'
-- './spec/services/members/update_service_spec.rb'
 - './spec/services/merge_requests/add_context_service_spec.rb'
 - './spec/services/merge_requests/add_spent_time_service_spec.rb'
 - './spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb'