From 8608c6325e19f529f7b43ff881c562d3a0114e1c Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 23 Nov 2015 09:42:20 +0100 Subject: 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 +-- .../merge_requests/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 08188324d74..18f5f270ec1 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 9db6ed5022d..f2e9a34dd2e 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 05c3bc074bb..89f9e8fa6a8 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 d5cae2f98f7..15dcace5dfb 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 5e8281a3fd0..7de4221c4c4 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 d2189787d47..5ec623b472c 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 ddd1a7bd63d..51e18f84424 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 49543786686..81f568cfa74 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 fa32617cb99..7d5ed559754 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 1b2ad1caf49..19e6fbb7887 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 f981432db36..b72f816932b 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 -- cgit v1.2.1