Merge branch '15463-extract-update-merge-requests' into 'master'

Extract project#update_merge_requests to its own worker from GitPushService

See merge request !6767

CHANGELOG

@@ -24,6 +24,7 @@ v 8.13.0 (unreleased)
   - Create a new /templates namespace for the /licenses, /gitignores and /gitlab_ci_ymls API endpoints. !5717 (tbalthazar)
   - Speed-up group milestones show page
   - Fix inconsistent options dropdown caret on mobile viewports (ClemMakesApps)
+  - Extract project#update_merge_requests and SystemHooks to its own worker from GitPushService
   - Don't include archived projects when creating group milestones. !4940 (Jeroen Jacobs)
   - Add tag shortcut from the Commit page. !6543
   - Keep refs for each deployment

app/models/project.rb

@@ -829,11 +829,6 @@ class Project < ActiveRecord::Base
     end
   end
 
-  def update_merge_requests(oldrev, newrev, ref, user)
-    MergeRequests::RefreshService.new(self, user).
-      execute(oldrev, newrev, ref)
-  end
-
   def valid_repo?
     repository.exists?
   rescue

app/services/git_push_service.rb

@@ -63,13 +63,12 @@ class GitPushService < BaseService
   protected
 
   def update_merge_requests
-    @project.update_merge_requests(params[:oldrev], params[:newrev], params[:ref], current_user)
+    UpdateMergeRequestsWorker.perform_async(@project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
 
     EventCreateService.new.push(@project, current_user, build_push_data)
-    SystemHooksService.new.execute_hooks(build_push_data_system_hook.dup, :push_hooks)
     @project.execute_hooks(build_push_data.dup, :push_hooks)
     @project.execute_services(build_push_data.dup, :push_hooks)
-    Ci::CreatePipelineService.new(project, current_user, build_push_data).execute
+    Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute
     ProjectCacheWorker.perform_async(@project.id)
   end
 
@@ -148,16 +147,6 @@ class GitPushService < BaseService
       push_commits)
   end
 
-  def build_push_data_system_hook
-    @push_data_system ||= Gitlab::DataBuilder::Push.build(
-      @project,
-      current_user,
-      params[:oldrev],
-      params[:newrev],
-      params[:ref],
-      [])
-  end
-
   def push_to_existing_branch?
     # Return if this is not a push to a branch (e.g. new commits)
     Gitlab::Git.branch_ref?(params[:ref]) && !Gitlab::Git.blank_ref?(params[:oldrev])

app/workers/update_merge_requests_worker.rb

@@ -0,0 +1,16 @@
+class UpdateMergeRequestsWorker
+  include Sidekiq::Worker
+
+  def perform(project_id, user_id, oldrev, newrev, ref)
+    project = Project.find_by(id: project_id)
+    return unless project
+
+    user = User.find_by(id: user_id)
+    return unless user
+
+    MergeRequests::RefreshService.new(project, user).execute(oldrev, newrev, ref)
+
+    push_data = Gitlab::DataBuilder::Push.build(project, user, oldrev, newrev, ref, [])
+    SystemHooksService.new.execute_hooks(push_data, :push_hooks)
+  end
+end

spec/models/project_spec.rb

@@ -228,7 +228,6 @@ describe Project, models: true do
   describe 'Respond to' do
     it { is_expected.to respond_to(:url_to_repo) }
     it { is_expected.to respond_to(:repo_exists?) }
-    it { is_expected.to respond_to(:update_merge_requests) }
     it { is_expected.to respond_to(:execute_hooks) }
     it { is_expected.to respond_to(:owner) }
     it { is_expected.to respond_to(:path_with_namespace) }
@@ -389,26 +388,6 @@ describe Project, models: true do
     end
   end
 
-  describe '#update_merge_requests' do
-    let(:project) { create(:project) }
-    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-    let(:key) { create(:key, user_id: project.owner.id) }
-    let(:prev_commit_id) { merge_request.commits.last.id }
-    let(:commit_id) { merge_request.commits.first.id }
-
-    it 'closes merge request if last commit from source branch was pushed to target branch' do
-      project.update_merge_requests(prev_commit_id, commit_id, "refs/heads/#{merge_request.target_branch}", key.user)
-      merge_request.reload
-      expect(merge_request.merged?).to be_truthy
-    end
-
-    it 'updates merge request commits with new one if pushed to source branch' do
-      project.update_merge_requests(prev_commit_id, commit_id, "refs/heads/#{merge_request.source_branch}", key.user)
-      merge_request.reload
-      expect(merge_request.diff_head_sha).to eq(commit_id)
-    end
-  end
-
   describe '.find_with_namespace' do
     context 'with namespace' do
       before do

spec/services/git_push_service_spec.rb

@@ -184,8 +184,8 @@ describe GitPushService, services: true do
 
     context "Updates merge requests" do
       it "when pushing a new branch for the first time" do
-        expect(project).to receive(:update_merge_requests).
-                               with(@blankrev, 'newrev', 'refs/heads/master', user)
+        expect(UpdateMergeRequestsWorker).to receive(:perform_async).
+                                                with(project.id, user.id, @blankrev, 'newrev', 'refs/heads/master')
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
       end
     end

spec/services/merge_requests/refresh_service_spec.rb

@@ -62,7 +62,8 @@ describe MergeRequests::RefreshService, services: true do
 
       it { expect(@merge_request.notes).not_to be_empty }
       it { expect(@merge_request).to be_open }
-      it { expect(@merge_request.merge_when_build_succeeds).to be_falsey}
+      it { expect(@merge_request.merge_when_build_succeeds).to be_falsey }
+      it { expect(@merge_request.diff_head_sha).to eq(@newrev) }
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
       it { expect(@build_failed_todo).to be_done }

spec/workers/post_receive_spec.rb

@@ -92,7 +92,13 @@ describe PostReceive do
       allow(Project).to receive(:find_with_namespace).and_return(project)
       expect(project).to receive(:execute_hooks).twice
       expect(project).to receive(:execute_services).twice
-      expect(project).to receive(:update_merge_requests)
+
+      PostReceive.new.perform(pwd(project), key_id, base64_changes)
+    end
+
+    it "enqueues a UpdateMergeRequestsWorker job" do
+      allow(Project).to receive(:find_with_namespace).and_return(project)
+      expect(UpdateMergeRequestsWorker).to receive(:perform_async).with(project.id, project.owner.id, any_args)
 
       PostReceive.new.perform(pwd(project), key_id, base64_changes)
     end

spec/workers/update_merge_requests_worker_spec.rb

@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe UpdateMergeRequestsWorker do
+  include RepoHelpers
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    let(:oldrev) { "123456" }
+    let(:newrev) { "789012" }
+    let(:ref)    { "refs/heads/test" }
+
+    def perform
+      subject.perform(project.id, user.id, oldrev, newrev, ref)
+    end
+
+    it 'executes MergeRequests::RefreshService with expected values' do
+      expect(MergeRequests::RefreshService).to receive(:new).with(project, user).and_call_original
+      expect_any_instance_of(MergeRequests::RefreshService).to receive(:execute).with(oldrev, newrev, ref)
+
+      perform
+    end
+
+    it 'executes SystemHooksService with expected values' do
+      push_data = double('push_data')
+      expect(Gitlab::DataBuilder::Push).to receive(:build).with(project, user, oldrev, newrev, ref, []).and_return(push_data)
+
+      system_hook_service = double('system_hook_service')
+      expect(SystemHooksService).to receive(:new).and_return(system_hook_service)
+      expect(system_hook_service).to receive(:execute_hooks).with(push_data, :push_hooks)
+
+      perform
+    end
+  end
+end