diff options
author | Stan Hu <stanhu@gmail.com> | 2015-10-15 11:41:46 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2015-10-16 09:54:13 +0300 |
commit | 2611d9f63cb6c22a00eccdac75535f93890b176c (patch) | |
tree | 2fd094664dc39103a7da10a2583572e22d7bb6c0 | |
parent | bd3689e9e0aebe43f7c5f787e03a3bbaa8b2ef68 (diff) |
Add a system note and update relevant merge requests when a branch is deleted or re-added
If a branch is deleted with an open merge request, amended offline, and then pushed again,
GitLab doesn't bother to update the merge request even though the last commit ID and/or
code may have changed. This MR ensures that each push will update any relevant merge
requests and adds a system note if this happens as well.
Closes #2926
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 3 | ||||
-rw-r--r-- | app/services/git_push_service.rb | 5 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 55 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 19 | ||||
-rw-r--r-- | spec/services/git_push_service_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 19 | ||||
-rw-r--r-- | spec/services/system_note_service_spec.rb | 12 |
8 files changed, 98 insertions, 24 deletions
diff --git a/CHANGELOG b/CHANGELOG index 39926692147..814a6772cfd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.1.0 (unreleased) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Speed up load times of issue detail pages by roughly 1.5x + - Add a system note and update relevant merge requests when a branch is deleted or re-added (Stan Hu) - Make diff file view easier to use on mobile screens (Stan Hu) - Improved performance of finding users by username or Email address - Fix bug where merge request comments created by API would not trigger notifications (Stan Hu) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index c9ef8023aea..bc2d691ece0 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -163,7 +163,8 @@ class MergeRequestDiff < ActiveRecord::Base merge_request.fetch_ref # Get latest sha of branch from source project - source_sha = merge_request.source_project.commit(source_branch).sha + source_commit = merge_request.source_project.commit(source_branch) + source_sha = source_commit.try(:sha) Gitlab::CompareResult.new( Gitlab::Git::Compare.new( diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 81d47602f13..e54044365b9 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -49,10 +49,13 @@ class GitPushService elsif push_to_existing_branch?(ref, oldrev) # Collect data for this git push @push_commits = project.repository.commits_between(oldrev, newrev) - project.update_merge_requests(oldrev, newrev, ref, @user) process_commit_messages(ref) end + # Update merge requests that may be affected by this push. A new branch + # could cause the last commit of a merge request to change. + project.update_merge_requests(oldrev, newrev, ref, @user) + @push_data = build_push_data(oldrev, newrev, ref) # If CI was disabled but .gitlab-ci.yml file was pushed diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index e903e48e3cd..802d02b0790 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -6,12 +6,22 @@ module MergeRequests @oldrev, @newrev = oldrev, newrev @branch_name = Gitlab::Git.ref_name(ref) @fork_merge_requests = @project.fork_merge_requests.opened - @commits = @project.repository.commits_between(oldrev, newrev) + @commits = [] + @merge_requests = merge_requests_for_branch + + # Leave a system note if a branch were deleted/added + if Gitlab::Git.blank_ref?(oldrev) or Gitlab::Git.blank_ref?(newrev) + presence = Gitlab::Git.blank_ref?(oldrev) ? 'added' : 'deleted' + comment_mr_branch_presence_changed(presence) + else + @commits = @project.repository.commits_between(oldrev, newrev) + + close_merge_requests + comment_mr_with_commits + end - close_merge_requests reload_merge_requests execute_mr_web_hooks - comment_mr_with_commits true end @@ -31,7 +41,6 @@ module MergeRequests commit_ids.include?(merge_request.last_commit.id) end - merge_requests.uniq.select(&:source_project).each do |merge_request| MergeRequests::PostMergeService. new(merge_request.target_project, @current_user). @@ -46,11 +55,7 @@ module MergeRequests # 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_requests.each do |merge_request| if merge_request.source_branch == @branch_name || force_push? merge_request.reload_code @@ -70,13 +75,20 @@ module MergeRequests 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) + # Add comment about branches being deleted or added to merge requests + def comment_mr_branch_presence_changed(presence) + merge_requests = merge_requests_for_branch merge_requests.each do |merge_request| + SystemNoteService.change_branch_presence( + merge_request, merge_request.project, @current_user, + 'source', @branch_name, presence) + end + end + + # Add comment about pushing new commits to merge requests + def comment_mr_with_commits + @merge_requests.each do |merge_request| mr_commit_ids = Set.new(merge_request.commits.map(&:id)) new_commits, existing_commits = @commits.partition do |commit| @@ -91,14 +103,7 @@ module MergeRequests # Call merge request webhook with update branches def execute_mr_web_hooks - 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| + @merge_requests.each do |merge_request| execute_hooks(merge_request, 'update') end end @@ -106,5 +111,11 @@ module MergeRequests def filter_merge_requests(merge_requests) merge_requests.uniq.select(&:source_project) end + + def merge_requests_for_branch + 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 + filter_merge_requests(merge_requests) + end end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 8253c1f780d..24e06504078 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -168,6 +168,25 @@ class SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end + # Called when a branch in Noteable is added or deleted + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # branch_type - 'source' or 'target' + # branch - branch name + # presence - 'deleted' or 'created' + # + # Example Note text: + # + # "Target branch `feature` deleted" + # + # Returns the created Note object + def self.change_branch_presence(noteable, project, author, branch_type, branch, presence) + body = "#{branch_type} branch `#{branch}` #{presence}".capitalize + create_note(noteable: noteable, project: project, author: author, note: body) + end + # Called when a Mentionable references a Noteable # # noteable - Noteable object being referenced diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index fd72905c331..17015d29e51 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -112,6 +112,14 @@ describe GitPushService do it { expect(@event.project).to eq(project) } it { expect(@event.action).to eq(Event::PUSHED) } it { expect(@event.data).to eq(service.push_data) } + + 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) + service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master') + end + end end describe "Web Hooks" do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 9516e7936d8..edb9fd0b43c 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -106,6 +106,25 @@ describe MergeRequests::RefreshService do it { expect(@fork_merge_request.notes).to be_empty } end + context 'push new branch that exists in a merge request' do + let(:refresh_service) { service.new(@fork_project, @user) } + before do + allow(refresh_service).to receive(:execute_hooks) + refresh_service.execute(Gitlab::Git::BLANK_SHA, @newrev, 'refs/heads/master') + reload_mrs + end + + it 'should execute hooks with update action' do + expect(refresh_service).to have_received(:execute_hooks). + with(@fork_merge_request, 'update') + end + + it { expect(@merge_request.notes).to be_empty } + it { expect(@merge_request).to be_open } + it { expect(@fork_merge_request.notes.last.note).to include('Source branch `master` added') } + it { expect(@fork_merge_request).to be_open } + end + def reload_mrs @merge_request.reload @fork_merge_request.reload diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 2658576640c..108bc5995df 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -242,6 +242,18 @@ describe SystemNoteService do end end + describe '.change_branch_presence' do + subject { described_class.change_branch_presence(noteable, project, author, 'source', 'feature', 'deleted') } + + it_behaves_like 'a system note' + + context 'when source branch deleted' do + it 'sets the note text' do + expect(subject.note).to eq "Source branch `feature` deleted" + end + end + end + describe '.cross_reference' do subject { described_class.cross_reference(noteable, mentioner, author) } |