diff options
Diffstat (limited to 'spec/services/members')
-rw-r--r-- | spec/services/members/destroy_service_spec.rb | 102 | ||||
-rw-r--r-- | spec/services/members/update_service_spec.rb | 44 |
2 files changed, 90 insertions, 56 deletions
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index d0f009f1321..d8a8d5881bf 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::DestroyService do +RSpec.describe Members::DestroyService, feature_category: :subgroups do let(:current_user) { create(:user) } let(:member_user) { create(:user) } let(:group) { create(:group, :public) } @@ -100,32 +100,104 @@ RSpec.describe Members::DestroyService do end context 'With ExclusiveLeaseHelpers' do + include ExclusiveLeaseHelpers + + let(:lock_key) do + "delete_members:#{member_to_delete.source.class}:#{member_to_delete.source.id}" + end + + let(:timeout) { 1.minute } let(:service_object) { described_class.new(current_user) } - let!(:member) { group_project.add_developer(member_user) } - subject(:destroy_member) { service_object.execute(member, **opts) } + subject(:destroy_member) { service_object.execute(member_to_delete, **opts) } - before do - group_project.add_maintainer(current_user) + shared_examples_for 'deletes the member without using a lock' do + it 'does not try to perform the delete within a lock' do + # `UpdateHighestRole` concern also uses locks to peform work + # whenever a Member is committed, so that needs to be accounted for. + lock_key_for_update_highest_role = "update_highest_role:#{member_to_delete.user_id}" + expect(Gitlab::ExclusiveLease) + .to receive(:new).with(lock_key_for_update_highest_role, timeout: 10.minutes.to_i).and_call_original + + # We do not use any locks for member deletion process. + expect(Gitlab::ExclusiveLease) + .not_to receive(:new).with(lock_key, timeout: timeout) - allow(service_object).to receive(:in_lock) do |_, &block| - block.call if lock_obtained + destroy_member + end + + it 'destroys the membership' do + expect { destroy_member }.to change { entity.members.count }.by(-1) end end - context 'when lock is obtained' do - let(:lock_obtained) { true } + context 'for group members' do + before do + group.add_owner(current_user) + end + + context 'deleting group owners' do + let!(:member_to_delete) { group.add_owner(member_user) } - it 'destroys the membership' do - expect { destroy_member }.to change { group_project.members.count }.by(-1) + context 'locking to avoid race conditions' do + it 'tries to perform the delete within a lock' do + expect_to_obtain_exclusive_lease(lock_key, timeout: timeout) + + destroy_member + end + + context 'based on status of the lock' do + context 'when lock is obtained' do + it 'destroys the membership' do + expect_to_obtain_exclusive_lease(lock_key, timeout: timeout) + + expect { destroy_member }.to change { group.members.count }.by(-1) + end + end + + context 'when the lock cannot be obtained' do + before do + stub_exclusive_lease_taken(lock_key, timeout: timeout) + end + + it 'raises error' do + expect { destroy_member }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + end + end + end + end + + context 'deleting group members that are not owners' do + let!(:member_to_delete) { group.add_developer(member_user) } + + it_behaves_like 'deletes the member without using a lock' do + let(:entity) { group } + end end end - context 'when the lock can not be obtained' do - let(:lock_obtained) { false } + context 'for project members' do + before do + group_project.add_owner(current_user) + end + + context 'deleting project owners' do + context 'deleting project owners' do + let!(:member_to_delete) { entity.add_owner(member_user) } - it 'does not destroy the membership' do - expect { destroy_member }.not_to change { group_project.members.count } + it_behaves_like 'deletes the member without using a lock' do + let(:entity) { group_project } + end + end + end + + context 'deleting project memebrs that are not owners' do + let!(:member_to_delete) { group_project.add_developer(member_user) } + + it_behaves_like 'deletes the member without using a lock' do + let(:entity) { group_project } + end end end end diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index eb8fae03c39..8a7f9a84c77 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -14,10 +14,7 @@ RSpec.describe Members::UpdateService do 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 + let(:updated_members) { subject[:members] } before do member_users.first.tap do |member_user| @@ -255,40 +252,6 @@ RSpec.describe Members::UpdateService do end end - 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 - - 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 @@ -326,15 +289,14 @@ RSpec.describe Members::UpdateService do it_behaves_like 'updating a group' context 'with a single member' do - let(:member) { create(:group_member, group: group) } - let(:members) { member } + let(:members) { create(:group_member, group: group) } before do group.add_owner(current_user) end it 'returns the correct response' do - expect(subject[:member]).to eq(member) + expect(subject[:members]).to contain_exactly(members) end end |