diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 89a48c598b6721f91c42db2634f636876742aa90..9ba6df5f490c7c71adc26d237a7251135e3397aa 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -468,7 +468,8 @@ def change_additional_attributes(issuable) end def change_state(issuable) - case params.delete(:state_event) + state_event = params.delete(:state_event) + case state_event when 'reopen' service_class = reopen_service when 'close' @@ -476,7 +477,13 @@ def change_state(issuable) end if service_class - service_class.new(**service_class.constructor_container_arg(project), current_user: current_user).execute(issuable) + if issuable.is_a?(MergeRequest) && state_event == 'close' + service_class.new(**service_class.constructor_container_arg(project), current_user: current_user) + .execute(issuable, skip_reset: true) + else + service_class.new(**service_class.constructor_container_arg(project), current_user: current_user) + .execute(issuable) + end end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index b9d2098eba395be6c9ce78d3f17e3f68c276bb75..dda1dc7523f88ddcf27da176c2318951bc650285 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -4,7 +4,7 @@ module MergeRequests class CloseService < MergeRequests::BaseService include RemovesRefs - def execute(merge_request, commit = nil) + def execute(merge_request, commit = nil, skip_reset: false) if Feature.enabled?(:destroy_fork_network_on_archive, project) unless params[:skip_authorization].present? || can?(current_user, :update_merge_request, merge_request) return merge_request @@ -13,28 +13,32 @@ def execute(merge_request, commit = nil) return merge_request unless can?(current_user, :update_merge_request, merge_request) end - # If we close MergeRequest we want to ignore validation - # so we can close broken one (Ex. fork project removed) - merge_request.allow_broken = true - - if merge_request.close - expire_unapproved_key(merge_request) - create_event(merge_request) - merge_request_activity_counter.track_close_mr_action(user: current_user) - create_note(merge_request) - notification_service.async.close_mr(merge_request, current_user) - todo_service.close_merge_request(merge_request, current_user) - execute_hooks(merge_request, 'close') - invalidate_all_users_cache_count(merge_request) - merge_request.invalidate_project_counter_caches - cleanup_environments(merge_request) - deactivate_pages_deployments(merge_request) - abort_auto_merge(merge_request, 'merge request was closed') - cleanup_refs(merge_request) - trigger_merge_request_merge_status_updated(merge_request) - end + ::Gitlab::Database::LoadBalancing::SessionMap.current(merge_request.load_balancer).use_primary do + merge_request.reset unless skip_reset + + # If we close MergeRequest we want to ignore validation + # so we can close broken one (Ex. fork project removed) + merge_request.allow_broken = true - merge_request + if merge_request.close + expire_unapproved_key(merge_request) + create_event(merge_request) + merge_request_activity_counter.track_close_mr_action(user: current_user) + create_note(merge_request) + notification_service.async.close_mr(merge_request, current_user) + todo_service.close_merge_request(merge_request, current_user) + execute_hooks(merge_request, 'close') + invalidate_all_users_cache_count(merge_request) + merge_request.invalidate_project_counter_caches + cleanup_environments(merge_request) + deactivate_pages_deployments(merge_request) + abort_auto_merge(merge_request, 'merge request was closed') + cleanup_refs(merge_request) + trigger_merge_request_merge_status_updated(merge_request) + end + + merge_request + end end private diff --git a/ee/app/services/ee/merge_requests/close_service.rb b/ee/app/services/ee/merge_requests/close_service.rb index 2d47a9d047144f74a517222d488a6928d2264e87..f8edf726d5d41316e348bab3823235935c048d3f 100644 --- a/ee/app/services/ee/merge_requests/close_service.rb +++ b/ee/app/services/ee/merge_requests/close_service.rb @@ -6,7 +6,7 @@ module CloseService extend ::Gitlab::Utils::Override override :execute - def execute(merge_request, commit = nil) + def execute(merge_request, commit = nil, skip_reset: false) super.tap do if current_user.project_bot? log_audit_event(merge_request, 'merge_request_closed_by_project_bot', diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 55c9b3ff1d0b90d24d793de988d08163354f75ed..a524ef79be85c018b22211c04f81190cf5cdaca6 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -66,6 +66,23 @@ def execute end end + context 'when the merge request is merged concurrently in another process' do + it 'does not close the merge request' do + stale_instance = MergeRequest.find(merge_request.id) + fresh_instance = MergeRequest.find(merge_request.id) + + fresh_instance.mark_as_merged! + + result = service.execute(stale_instance) + + expect(result).to eq(stale_instance) + expect(result).to be_merged + expect(result).not_to be_closed + + expect(merge_request.reload).to be_merged + end + end + it 'updates metrics' do metrics = merge_request.metrics metrics_service = double(MergeRequestMetricsService) diff --git a/spec/support/shared_examples/services/issuable_shared_examples.rb b/spec/support/shared_examples/services/issuable_shared_examples.rb index 2357dabf350f8eb83abd3b395304b095577df4c0..97c78a08d917e798d630cca49028e01ca8ea19c4 100644 --- a/spec/support/shared_examples/services/issuable_shared_examples.rb +++ b/spec/support/shared_examples/services/issuable_shared_examples.rb @@ -2,13 +2,19 @@ RSpec.shared_examples 'cache counters invalidator' do it 'invalidates counter cache for assignees' do - expect(merge_request.assignees).to all(receive(:invalidate_merge_request_cache_counts)) + assignees = merge_request.assignees.to_a + allow(merge_request).to receive(:assignees).and_return(assignees) + + expect(assignees).to all(receive(:invalidate_merge_request_cache_counts)) described_class.new(project: project, current_user: user).execute(merge_request) end it 'invalidates counter cache for author' do - expect(merge_request.author).to receive(:invalidate_merge_request_cache_counts) + author = merge_request.author + allow(merge_request).to receive(:author).and_return(author) + + expect(author).to receive(:invalidate_merge_request_cache_counts) described_class.new(project: project, current_user: user).execute(merge_request) end diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb index 76526f1a99fe142dc888985161cffae89b5c4e12..146100d40ff5a335312a81e05ad9af325033bd8a 100644 --- a/spec/support/shared_examples/services/merge_request_shared_examples.rb +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -52,7 +52,10 @@ end it 'invalidates counter cache for reviewers' do - expect(merge_request.reviewers).to all(receive(:invalidate_merge_request_cache_counts)) + reviewers = merge_request.reviewers.to_a + allow(merge_request).to receive(:reviewers).and_return(reviewers) + + expect(reviewers).to all(receive(:invalidate_merge_request_cache_counts)) described_class.new(project: project, current_user: user).execute(merge_request) end