From 8608c6325e19f529f7b43ff881c562d3a0114e1c Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <mail@zjvandeweg.nl>
Date: Mon, 23 Nov 2015 09:42:20 +0100
Subject: [PATCH] Refactor MergeWhenBuildSucceedsService and incorporate
feedback
---
CHANGELOG | 5 +--
.../projects/merge_requests_controller.rb | 11 ++----
app/models/merge_request.rb | 11 ++++--
.../merge_when_build_succeeds_service.rb | 17 ++++++++-
app/services/system_note_service.rb | 4 +--
.../widget/open/_accept.html.haml | 2 +-
.../open/_merge_when_build_succeeds.html.haml | 9 ++---
config/routes.rb | 1 +
db/schema.rb | 2 --
doc/api/merge_requests.md | 8 ++---
lib/api/merge_requests.rb | 36 +++++++------------
11 files changed, 52 insertions(+), 54 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index 08188324d7..18f5f270ec 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
Please view this file on the master branch, on stable branches it's out of date.
v 8.3.0 (unreleased)
+- Merge when build succeeds (Zeger-Jan van de Weg)
v 8.2.0
- Fix grouping of contributors by email in graph.
@@ -28,10 +29,6 @@ v 8.2.0
- Allow to define cache in `.gitlab-ci.yml`
- Fix: 500 error returned if destroy request without HTTP referer (Kazuki Shimizu)
- Remove deprecated CI events from project settings page
- - Use issue editor as cross reference comment author when issue is edited with a new mention.
- - Merge when build succeeds (Zeger-Jan van de Weg)
-
-v 8.1.1
- [API] Add ability to fetch the commit ID of the last commit that actually touched a file
- Fix omniauth documentation setting for omnibus configuration (Jon Cairns)
- Add "New file" link to dropdown on project page
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 9db6ed5022..f2e9a34dd2 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -151,14 +151,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def cancel_merge_when_build_succeeds
- unless @merge_request.can_be_merged_by?(current_user) || @merge_request.author == current_user
- return access_denied!
- end
+ return access_denied! unless @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
- if @merge_request.merge_when_build_succeeds?
- @merge_request.reset_merge_when_build_succeeds
- SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
- end
+ MergeRequests::MergeWhenBuildSucceedsService.new(@project, current_user).cancel(@merge_request)
end
def merge
@@ -171,7 +166,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@merge_request.update(merge_error: nil)
- if params[:merge_when_build_succeeds] && @merge_request.ci_commit.active?
+ if params[:merge_when_build_succeeds] && @merge_request.ci_commit && @merge_request.ci_commit.active?
MergeRequests::MergeWhenBuildSucceedsService.new(@project, current_user, merge_params)
.execute(@merge_request)
@status = :merge_when_build_succeeds
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 05c3bc074b..89f9e8fa6a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -254,8 +254,15 @@ class MergeRequest < ActiveRecord::Base
end
end
- def mergeable_by_or_author(user)
- self.can_be_merged_by?(user) || self.author == user
+ def can_cancel_merge_when_build_succeeds?(user)
+ can_be_merged_by?(user) || self.author == user
+ end
+
+ def can_remove_source_branch?
+ for_fork? &&
+ !project.protected_branch(source_branch) &&
+ !project.repository.root_ref(source_branch) &&
+ can?(current_user, :push_code, project)
end
def mr_and_commit_notes
diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb
index d5cae2f98f..15dcace5df 100644
--- a/app/services/merge_requests/merge_when_build_succeeds_service.rb
+++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb
@@ -1,5 +1,7 @@
module MergeRequests
class MergeWhenBuildSucceedsService < MergeRequests::BaseService
+ # Marks the passed `merge_request` to be marked when the build succeeds or
+ # updates the params for the automatic merge
def execute(merge_request)
merge_request.merge_params.merge!(params)
@@ -14,10 +16,11 @@ module MergeRequests
merge_request.save
unless already_approved
- SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user)
+ SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit)
end
end
+ # Triggers the automatic merge of merge_request once the build succeeds
def trigger(build)
merge_requests = merge_request_from(build)
@@ -31,6 +34,18 @@ module MergeRequests
end
end
+ # Cancels the automatic merge
+ def cancel(merge_request)
+ if merge_request.merge_when_build_succeeds? && merge_request.open? && !merge_request.merged?
+ merge_request.reset_merge_when_build_succeeds
+ SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
+
+ success
+ else
+ error("Can't cancel the automatic merge", 406)
+ end
+ end
+
private
def merge_request_from(build)
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 5e8281a3fd..7de4221c4c 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -131,8 +131,8 @@ class SystemNoteService
end
# Called when 'merge when build succeeds' is executed
- def self.merge_when_build_succeeds(noteable, project, author)
- body = "This merge request will be automatically merged when the build for #{noteable.ci_commit.short_sha} succeeds"
+ def self.merge_when_build_succeeds(noteable, project, author, ci_commit)
+ body = "Approved an automatic merge when the build for #{ci_commit.sha} succeeds"
create_note(noteable: noteable, project: project, author: author, note: body)
end
diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml
index d2189787d4..5ec623b472 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -13,7 +13,7 @@
- else
= f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do
Accept Merge Request
- - if can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) && !@merge_request.for_fork?
+ - if @merge_request.can_remove_source_branch?
.accept-control.checkbox
= label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
= check_box_tag :should_remove_source_branch
diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
index ddd1a7bd63..51e18f8442 100644
--- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
@@ -8,20 +8,17 @@
The changes will be merged into
%span.label-branch= @merge_request.target_branch
The source branch will be removed.
- - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch)
- - remove_source_branch_button = true
%p
= succeed '.' do
The changes will be merged into
%span.label-branch= @merge_request.target_branch
- The source branch won't be removed.
+ The source branch will not be removed.
-- if remove_source_branch_button || @merge_request.can_be_merged_by?(current_user)
.clearfix.prepend-top-10
- - if remove_source_branch_button
+ - if @merge_request.can_remove_source_branch? && !@merge_request.merge_params["should_remove_source_branch"].present?
= link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
= icon('times')
Remove Source Branch When Merged
- - if @merge_request.can_be_merged_by?(current_user) || @merge_request.author == current_user
+ - if @merge_request.merge_when_build_succeeds
= link_to cancel_merge_when_build_succeeds_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request), remote: true, method: :post, class: "btn btn-grouped btn-warning btn-sm" do
Cancel Automatic Merge
diff --git a/config/routes.rb b/config/routes.rb
index 4954378668..81f568cfa7 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -570,6 +570,7 @@ Gitlab::Application.routes.draw do
member do
get :diffs
get :commits
+ get :merge_check
post :merge
post :cancel_merge_when_build_succeeds
get :ci_status
diff --git a/db/schema.rb b/db/schema.rb
index fa32617cb9..7d5ed55975 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -421,7 +421,6 @@ ActiveRecord::Schema.define(version: 20151116144118) do
end
add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree
- add_index "labels", ["title"], name: "index_labels_on_title", using: :btree
create_table "lfs_objects", force: true do |t|
t.string "oid", null: false
@@ -525,7 +524,6 @@ ActiveRecord::Schema.define(version: 20151116144118) do
add_index "milestones", ["due_date"], name: "index_milestones_on_due_date", using: :btree
add_index "milestones", ["project_id", "iid"], name: "index_milestones_on_project_id_and_iid", unique: true, using: :btree
add_index "milestones", ["project_id"], name: "index_milestones_on_project_id", using: :btree
- add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree
create_table "namespaces", force: true do |t|
t.string "name", null: false
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index 1b2ad1caf4..19e6fbb788 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -296,7 +296,7 @@ Parameters:
- `merge_request_id` (required) - ID of MR
- `merge_commit_message` (optional) - Custom merge commit message
- `should_remove_source_branch` (optional) - if `true` removes the source branch
-- `merge_when_build_succeeds` (optional) - if `true` the MR is merge when the build succeeds
+- `merged_when_build_succeeds` (optional) - if `true` the MR is merge when the build succeeds
```json
{
@@ -329,15 +329,13 @@ Parameters:
## Cancel Merge When Build Succeeds
-Cancels the merge when build succeeds and reset the merge parameters
-
-If successfull you'll get `200 OK`.
+If successful you'll get `200 OK`.
If you don't have permissions to accept this merge request - you'll get a 401
If the merge request is already merged or closed - you get 405 and error message 'Method Not Allowed'
-In case the merge request is not set to be merged when the build succeeds, you'll also get a 405 with the error message 'Method Not Allowed'
+In case the merge request is not set to be merged when the build succeeds, you'll also get a 406 error.
```
PUT /projects/:id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds
```
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index f981432db3..b72f816932 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -182,39 +182,35 @@ module API
# id (required) - The ID of a project
# merge_request_id (required) - ID of MR
# merge_commit_message (optional) - Custom merge commit message
- # merge_when_build_succeeds (optional) - truethy when this MR should be merged when the build is succesfull
+ # should_remove_source_branch - When true, the source branch will be deleted if possible
+ # merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds
# Example:
# PUT /projects/:id/merge_request/:merge_request_id/merge
#
put ":id/merge_request/:merge_request_id/merge" do
merge_request = user_project.merge_requests.find(params[:merge_request_id])
- allowed = ::Gitlab::GitAccess.new(current_user, user_project).
- can_push_to_branch?(merge_request.target_branch)
-
# Merge request can not be merged
# because user dont have permissions to push into target branch
- unauthorized! unless allowed
+ unauthorized! unless merge_request.can_be_merged_by?(current_user)
- not_allowed! unless merge_request.open?
+ not_allowed! unless merge_request.open? && !merge_request.work_in_progress?
merge_request.check_if_can_be_merged if merge_request.unchecked?
+ render_api_error!('Branch cannot be merged', 406) if merge_request.can_be_merged?
+
merge_params = {
commit_message: params[:merge_commit_message],
should_remove_source_branch: params[:should_remove_source_branch]
}
- if !merge_request.work_in_progress?
- if parse_boolean(params[:merge_when_build_succeeds])
- ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params).
- execute(merge_request)
- else
- ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params).
- execute(merge_request, merge_params)
- end
+ if parse_boolean(params[:merge_when_build_succeeds]) && merge_request.ci_commit && merge_request.ci_commit.active?
+ ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params).
+ execute(merge_request)
else
- render_api_error!('Branch cannot be merged', 405)
+ ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params).
+ execute(merge_request)
end
present merge_request, with: Entities::MergeRequest
@@ -233,15 +229,9 @@ module API
# Merge request can not be merged
# because user dont have permissions to push into target branch
- unauthorized! unless allowed
+ unauthorized! unless merge_request.can_cancel_merge_when_build_succeeds?(current_user)
- if merge_request.merged? || !merge_request.open? || !merge_request.merge_when_build_succeeds?
- not_allowed!
- end
-
- merge_request.reset_merge_when_build_succeeds
-
- present merge_request, with: Entities::MergeRequest
+ ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user).cancel(merge_request)
end
# Get a merge request's comments
--
2.30.9