Refactor merge request refresh logic on push

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/models/project.rb

@@ -402,43 +402,8 @@ def execute_services(data)
   end
 
   def update_merge_requests(oldrev, newrev, ref, user)
-    return true unless ref =~ /heads/
-    branch_name = ref.gsub("refs/heads/", "")
-    commits = self.repository.commits_between(oldrev, newrev)
-    c_ids = commits.map(&:id)
-
-    # Close merge requests
-    mrs = self.merge_requests.opened.where(target_branch: branch_name).to_a
-    mrs = mrs.select(&:last_commit).select { |mr| c_ids.include?(mr.last_commit.id) }
-
-    mrs.uniq.select(&:source_project).each do |merge_request|
-      MergeRequests::MergeService.new.execute(merge_request, user, nil)
-    end
-
-    # Update code for merge requests into project between project branches
-    mrs = self.merge_requests.opened.by_branch(branch_name).to_a
-    # Update code for merge requests between project and project fork
-    mrs += self.fork_merge_requests.opened.by_branch(branch_name).to_a
-
-    mrs.uniq.select(&:source_project).each do |merge_request|
-      merge_request.reload_code
-      merge_request.mark_as_unchecked
-    end
-
-    # Add comment about pushing new commits to merge requests
-    comment_mr_with_commits(branch_name, commits, user)
-
-    true
-  end
-
-  def comment_mr_with_commits(branch_name, commits, user)
-    mrs = self.origin_merge_requests.opened.where(source_branch: branch_name).to_a
-    mrs += self.fork_merge_requests.opened.where(source_branch: branch_name).to_a
-
-    mrs.uniq.select(&:source_project).each do |merge_request|
-      Note.create_new_commits_note(merge_request, merge_request.project,
-                                   user, commits)
-    end
+    MergeRequests::RefreshService.new(self, user).
+      execute(oldrev, newrev, ref)
   end
 
   def valid_repo?

app/services/merge_requests/refresh_service.rb

@@ -0,0 +1,67 @@
+module MergeRequests
+  class RefreshService < MergeRequests::BaseService
+    def execute(oldrev, newrev, ref)
+      return true unless ref =~ /heads/
+
+      @branch_name = ref.gsub("refs/heads/", "")
+      @fork_merge_requests = @project.fork_merge_requests.opened
+      @commits = @project.repository.commits_between(oldrev, newrev)
+
+      close_merge_requests
+      reload_merge_requests
+      comment_mr_with_commits
+
+      true
+    end
+
+    private
+
+    # Collect open merge requests that target same branch we push into
+    # and close if push to master include last commit from merge request
+    # We need this to close(as merged) merge requests that were merged into
+    # target branch manually
+    def close_merge_requests
+      commit_ids = @commits.map(&:id)
+      merge_requests = @project.merge_requests.opened.where(target_branch: @branch_name).to_a
+      merge_requests = merge_requests.select(&:last_commit)
+
+      merge_requests = merge_requests.select do |merge_request|
+        commit_ids.include?(merge_request.last_commit.id)
+      end
+
+
+      merge_requests.uniq.select(&:source_project).each do |merge_request|
+        MergeRequests::MergeService.new.execute(merge_request, @current_user, nil)
+      end
+    end
+
+    # Refresh merge request diff if we push to source or target branch of merge request
+    # Note: we should update merge requests from forks too
+    def reload_merge_requests
+      merge_requests = @project.merge_requests.opened.by_branch(@branch_name).to_a
+      merge_requests += @fork_merge_requests.by_branch(@branch_name).to_a
+      merge_requests = filter_merge_requests(merge_requests)
+
+      merge_requests.each do |merge_request|
+        merge_request.reload_code
+        merge_request.mark_as_unchecked
+      end
+    end
+
+    # Add comment about pushing new commits to merge requests
+    def comment_mr_with_commits
+      merge_requests = @project.origin_merge_requests.opened.where(source_branch: @branch_name).to_a
+      merge_requests += @fork_merge_requests.where(source_branch: @branch_name).to_a
+      merge_requests = filter_merge_requests(merge_requests)
+
+      merge_requests.each do |merge_request|
+        Note.create_new_commits_note(merge_request, merge_request.project,
+                                     @current_user, @commits)
+      end
+    end
+
+    def filter_merge_requests(merge_requests)
+      merge_requests.uniq.select(&:source_project)
+    end
+  end
+end

spec/models/project_spec.rb

@@ -145,63 +145,6 @@
     end
   end
 
-  describe 'comment merge requests with commits' do
-    before do
-      @user = create(:user)
-      group = create(:group)
-      group.add_owner(@user)
-
-      @project = create(:project, namespace: group)
-      @fork_project = Projects::ForkService.new(@project, @user).execute
-      @merge_request = create(:merge_request, source_project: @project,
-                              source_branch: 'master',
-                              target_branch: 'feature',
-                              target_project: @project)
-      @fork_merge_request = create(:merge_request, source_project: @fork_project,
-                                   source_branch: 'master',
-                                   target_branch: 'feature',
-                                   target_project: @project)
-
-      @commits = @merge_request.commits
-    end
-
-    context 'push to origin repo source branch' do
-      before do
-        @project.comment_mr_with_commits('master', @commits, @user)
-      end
-
-      it { @merge_request.notes.should_not be_empty }
-      it { @fork_merge_request.notes.should be_empty }
-    end
-
-    context 'push to origin repo target branch' do
-      before do
-        @project.comment_mr_with_commits('feature', @commits, @user)
-      end
-
-      it { @merge_request.notes.should be_empty }
-      it { @fork_merge_request.notes.should be_empty }
-    end
-
-    context 'push to fork repo source branch' do
-      before do
-        @fork_project.comment_mr_with_commits('master', @commits, @user)
-      end
-
-      it { @merge_request.notes.should be_empty }
-      it { @fork_merge_request.notes.should_not be_empty }
-    end
-
-    context 'push to fork repo target branch' do
-      before do
-        @fork_project.comment_mr_with_commits('feature', @commits, @user)
-      end
-
-      it { @merge_request.notes.should be_empty }
-      it { @fork_merge_request.notes.should be_empty }
-    end
-  end
-
   describe :find_with_namespace do
     context 'with namespace' do
       before do

spec/services/merge_requests/refresh_service_spec.rb

@@ -0,0 +1,98 @@
+require 'spec_helper'
+
+describe MergeRequests::RefreshService do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:service) { MergeRequests::RefreshService }
+
+  describe :execute do
+    before do
+      @user = create(:user)
+      group = create(:group)
+      group.add_owner(@user)
+
+      @project = create(:project, namespace: group)
+      @fork_project = Projects::ForkService.new(@project, @user).execute
+      @merge_request = create(:merge_request, source_project: @project,
+                              source_branch: 'master',
+                              target_branch: 'feature',
+                              target_project: @project)
+
+      @fork_merge_request = create(:merge_request, source_project: @fork_project,
+                                   source_branch: 'master',
+                                   target_branch: 'feature',
+                                   target_project: @project)
+
+      @commits = @merge_request.commits
+
+      @oldrev = @commits.last.id
+      @newrev = @commits.first.id
+    end
+
+    context 'push to origin repo source branch' do
+      before do
+        service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master')
+        reload_mrs
+      end
+
+      it { @merge_request.notes.should_not be_empty }
+      it { @merge_request.should be_open }
+      it { @fork_merge_request.should be_open }
+      it { @fork_merge_request.notes.should be_empty }
+    end
+
+    context 'push to origin repo target branch' do
+      before do
+        service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
+        reload_mrs
+      end
+
+      it { @merge_request.notes.should be_empty }
+      it { @merge_request.should be_merged }
+      it { @fork_merge_request.should be_merged }
+      it { @fork_merge_request.notes.should be_empty }
+    end
+
+    context 'push to fork repo source branch' do
+      before do
+        service.new(@fork_project, @user).execute(@oldrev, @newrev, 'refs/heads/master')
+        reload_mrs
+      end
+
+      it { @merge_request.notes.should be_empty }
+      it { @merge_request.should be_open }
+      it { @fork_merge_request.notes.should_not be_empty }
+      it { @fork_merge_request.should be_open }
+    end
+
+    context 'push to fork repo target branch' do
+      before do
+        service.new(@fork_project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
+        reload_mrs
+      end
+
+      it { @merge_request.notes.should be_empty }
+      it { @merge_request.should be_open }
+      it { @fork_merge_request.notes.should be_empty }
+      it { @fork_merge_request.should be_open }
+    end
+
+    context 'push to origin repo target branch after fork project was removed' do
+      before do
+        @fork_project.destroy
+        service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature')
+        reload_mrs
+      end
+
+      it { @merge_request.notes.should be_empty }
+      it { @merge_request.should be_merged }
+      it { @fork_merge_request.should be_open }
+      it { @fork_merge_request.notes.should be_empty }
+    end
+
+    def reload_mrs
+      @merge_request.reload
+      @fork_merge_request.reload
+    end
+  end
+end