From 77f8a1e392b64f51326df8aebdc77e97af07bfed Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 2 Nov 2015 17:27:38 +0100 Subject: Merge when build succeeds --- CHANGELOG | 1 + app/assets/stylesheets/pages/merge_requests.scss | 4 +- .../projects/merge_requests_controller.rb | 35 +++++++++++++++--- app/models/ci/commit.rb | 8 ++++ app/models/commit_status.rb | 33 +++++++++++++++++ app/models/merge_request.rb | 13 +++++++ .../merge_when_build_succeeds_service.rb | 43 ++++++++++++++++++++++ app/services/merge_requests/refresh_service.rb | 8 +++- app/services/system_note_service.rb | 14 +++++++ .../cancel_merge_when_build_succeeds.js.haml | 2 + app/views/projects/merge_requests/merge.js.haml | 6 ++- .../projects/merge_requests/widget/_open.html.haml | 2 + .../merge_requests/widget/open/_accept.html.haml | 18 ++++++--- .../open/_merge_when_build_succeeds.html.haml | 27 ++++++++++++++ config/routes.rb | 1 + ...d_merge_when_build_succeeds_to_merge_request.rb | 7 ++++ db/schema.rb | 17 ++++++--- 17 files changed, 219 insertions(+), 20 deletions(-) create mode 100644 app/services/merge_requests/merge_when_build_succeeds_service.rb create mode 100644 app/views/projects/merge_requests/cancel_merge_when_build_succeeds.js.haml create mode 100644 app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml create mode 100644 db/migrate/20151028152939_add_merge_when_build_succeeds_to_merge_request.rb diff --git a/CHANGELOG b/CHANGELOG index 0d89fca9fc1..195e53abd8b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -14,6 +14,7 @@ v 8.2.0 (unreleased) - 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 - Fix cloning Wiki repositories via HTTP (Stan Hu) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index f0b3667acca..b5a8c893678 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -19,18 +19,20 @@ .accept-merge-holder { .accept-action { display: inline-block; + float: left; } .accept-control { display: inline-block; + float: left; margin: 0; margin-left: 20px; padding: 5px; + padding-top: 12px; line-height: 20px; &.right { float: right; - padding-top: 12px; a { color: $gl-gray; } diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 16c42386623..2f9b8a25edf 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -2,7 +2,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :module_enabled before_action :merge_request, only: [ :edit, :update, :show, :diffs, :commits, :merge, :merge_check, - :ci_status, :toggle_subscription + :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds ] before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits] before_action :validates_merge_request, only: [:show, :diffs, :commits] @@ -149,15 +149,34 @@ class Projects::MergeRequestsController < Projects::ApplicationController render partial: "projects/merge_requests/widget/show.html.haml", layout: false end + def cancel_merge_when_build_succeeds + return access_denied! unless @merge_request.can_be_merged_by?(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 + end + def merge return access_denied! unless @merge_request.can_be_merged_by?(current_user) - if @merge_request.mergeable? - @merge_request.update(merge_error: nil) - MergeWorker.perform_async(@merge_request.id, current_user.id, params) - @status = true + unless @merge_request.mergeable? + @status = :failed + return + end + + @merge_request.update(merge_error: nil) + + if params[:merge_when_build_succeeds] && @merge_request.ci_commit.active? + MergeRequests::MergeWhenBuildSucceedsService.new(@project, + current_user, + merge_params: merge_params) + .execute(@merge_request) + @status = :merge_when_build_succeeds else - @status = false + MergeWorker.perform_async(@merge_request.id, current_user.id, params) + @status = :success end end @@ -282,6 +301,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController ) end + def merge_params + params.permit(:should_remove_source_branch, :commit_message) + end + # Make sure merge requests created before 8.0 # have head file in refs/merge-requests/ def ensure_ref_fetched diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb index 13437b2483f..ebe4bace3b5 100644 --- a/app/models/ci/commit.rb +++ b/app/models/ci/commit.rb @@ -164,6 +164,14 @@ module Ci status == 'canceled' end + def active? + running? || pending? + end + + def complete? + canceled? || success? || failed? + end + def duration duration_array = latest_statuses.map(&:duration).compact duration_array.reduce(:+).to_i diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 0b73ab6d2eb..b1049fab788 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -1,3 +1,32 @@ +# == Schema Information +# +# project_id integer +# status string +# finished_at datetime +# trace text +# created_at datetime +# updated_at datetime +# started_at datetime +# runner_id integer +# coverage float +# commit_id integer +# commands text +# job_id integer +# name string +# deploy boolean default: false +# options text +# allow_failure boolean default: false, null: false +# stage string +# trigger_request_id integer +# stage_idx integer +# tag boolean +# ref string +# user_id integer +# type string +# target_url string +# description string +# + class CommitStatus < ActiveRecord::Base self.table_name = 'ci_builds' @@ -46,6 +75,10 @@ class CommitStatus < ActiveRecord::Base build.update_attributes finished_at: Time.now end + after_transition running: :success do |build, transition| + MergeRequests::MergeWhenBuildSucceedsService.new(build.commit.gl_project, nil).trigger(build) + end + state :pending, value: 'pending' state :running, value: 'running' state :failed, value: 'failed' diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 85f37e49e62..c5f04dbcf4c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -34,9 +34,12 @@ class MergeRequest < ActiveRecord::Base belongs_to :target_project, foreign_key: :target_project_id, class_name: "Project" belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project" + belongs_to :merge_user, class_name: "User" has_one :merge_request_diff, dependent: :destroy + serialize :merge_params, Hash + after_create :create_merge_request_diff after_update :update_merge_request_diff @@ -385,6 +388,16 @@ class MergeRequest < ActiveRecord::Base message end + def reset_merge_when_build_succeeds + return unless merge_when_build_succeeds? + + self.merge_when_build_succeeds = false + self.merge_user = nil + self.merge_params = nil + + self.save + end + # Return array of possible target branches # depends on target project of MR def target_branches diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb new file mode 100644 index 00000000000..a4418360b8c --- /dev/null +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -0,0 +1,43 @@ +module MergeRequests + class MergeWhenBuildSucceedsService < MergeRequests::BaseService + def execute(merge_request) + merge_request.merge_params.merge!(params[:merge_params]) + + # The service is also called when the merge params are updated. + already_approved = merge_request.merge_when_build_succeeds? + + unless already_approved + merge_request.merge_when_build_succeeds = true + merge_request.merge_user = @current_user + end + + merge_request.save + + unless already_approved + SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user) + end + end + + def trigger(build) + merge_requests = merge_request_from(build) + + merge_requests.each do |merge_request| + next unless merge_request.merge_when_build_succeeds? + + ci_commit = merge_request.ci_commit + if ci_commit && ci_commit.success? && merge_request.mergeable? + MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params) + end + end + end + + private + + def merge_request_from(build) + merge_requests = @project.origin_merge_requests.opened.where(source_branch: build.ref).to_a + merge_requests += @project.fork_merge_requests.opened.where(source_branch: build.ref).to_a + + merge_requests.uniq.select(&:source_project) + end + end +end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index d68bc79ecc0..335ef32abce 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -8,6 +8,7 @@ module MergeRequests find_new_commits reload_merge_requests + reset_merge_when_build_succeeds # Leave a system note if a branch was deleted/added if branch_added? || branch_removed? @@ -57,7 +58,6 @@ module MergeRequests merge_requests = filter_merge_requests(merge_requests) merge_requests.each do |merge_request| - if merge_request.source_branch == @branch_name || force_push? merge_request.reload_code merge_request.mark_as_unchecked @@ -76,6 +76,12 @@ module MergeRequests end end + def reset_merge_when_build_succeeds + merge_requests_for_source_branch.each do |merge_request| + merge_request.reset_merge_when_build_succeeds + end + end + def find_new_commits if branch_added? @commits = [] diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 708c2f00486..c9846e9f26f 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -130,6 +130,20 @@ class SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end + # Called when 'merge when build succeeds' is executed + def self.merge_when_build_succeeds(noteable, project, author) + body = "Approved this request to be merged automatically when the build succeeds" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when 'merge when build succeeds' is canceled + def self.cancel_merge_when_build_succeeds(noteable, project, author) + body = "Canceled the automatic merge" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + # Called when the title of a Noteable is changed # # noteable - Noteable object that responds to `title` diff --git a/app/views/projects/merge_requests/cancel_merge_when_build_succeeds.js.haml b/app/views/projects/merge_requests/cancel_merge_when_build_succeeds.js.haml new file mode 100644 index 00000000000..eab5be488b5 --- /dev/null +++ b/app/views/projects/merge_requests/cancel_merge_when_build_succeeds.js.haml @@ -0,0 +1,2 @@ +:plain + $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/accept'))}"); diff --git a/app/views/projects/merge_requests/merge.js.haml b/app/views/projects/merge_requests/merge.js.haml index 33321651e32..89aae17a606 100644 --- a/app/views/projects/merge_requests/merge.js.haml +++ b/app/views/projects/merge_requests/merge.js.haml @@ -1,6 +1,10 @@ -- if @status +- case @status +- when :success :plain merge_request_widget.mergeInProgress(); +- when :merge_when_build_succeeds + :plain + $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}"); - else :plain $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}"); diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 0aad9bb3e88..e0013fb769a 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -13,6 +13,8 @@ = render 'projects/merge_requests/widget/open/conflicts' - elsif @merge_request.work_in_progress? = render 'projects/merge_requests/widget/open/wip' + - elsif @merge_request.merge_when_build_succeeds? + = render 'projects/merge_requests/widget/open/merge_when_build_succeeds' - elsif !@merge_request.can_be_merged_by?(current_user) = render 'projects/merge_requests/widget/open/not_allowed' - elsif @merge_request.can_be_merged? 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 613525437ab..2afc5f81251 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -2,8 +2,15 @@ = hidden_field_tag :authenticity_token, form_authenticity_token .accept-merge-holder.clearfix.js-toggle-container .accept-action - = f.button class: "btn btn-create accept_merge_request" do - Accept Merge Request + - ci_commit = @merge_request.ci_commit + - if ci_commit && ci_commit.active? + = f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do + Merge when Build Succeeds + = f.button class: "btn btn-warning btn-grouped accept_merge_request" do + Accept Merge Request Now + - else + = f.button class: "btn btn-create btn-grouped accept_merge_request" do + Accept Merge Request - if can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) && !@merge_request.for_fork? .accept-control.checkbox = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do @@ -19,7 +26,8 @@ rows: 14, hint: true :coffeescript - $('.accept-mr-form').on 'ajax:before', -> - btn = $('.accept_merge_request') - btn.disable() + $('.accept_merge_request').on 'click', -> + btn = $(this) btn.html(" Merge in progress") + $('.accept-mr-form').on 'ajax:sen', -> + $(".accept-mr-form :input").disable() 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 new file mode 100644 index 00000000000..7e5385cf8b9 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml @@ -0,0 +1,27 @@ +%h4 + Approved by #{link_to_member(@project, @merge_request.merge_user, avatar: true)} + to be merged automatically when + #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds +%div + - if @merge_request.merge_params["should_remove_source_branch"] + = succeed '.' do + 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 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 + = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_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) + = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request), remote: true, method: :delete, class: "btn btn-grouped btn-warning btn-sm" do + Cancel Automatic Merge diff --git a/config/routes.rb b/config/routes.rb index 0458f538eb6..917c3d3f1ed 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -556,6 +556,7 @@ Gitlab::Application.routes.draw do get :diffs get :commits post :merge + delete :merge, action: :cancel_merge_when_build_succeeds get :merge_check get :ci_status post :toggle_subscription diff --git a/db/migrate/20151028152939_add_merge_when_build_succeeds_to_merge_request.rb b/db/migrate/20151028152939_add_merge_when_build_succeeds_to_merge_request.rb new file mode 100644 index 00000000000..ceb52f0c222 --- /dev/null +++ b/db/migrate/20151028152939_add_merge_when_build_succeeds_to_merge_request.rb @@ -0,0 +1,7 @@ +class AddMergeWhenBuildSucceedsToMergeRequest < ActiveRecord::Migration + def change + add_column :merge_requests, :merge_params, :text + add_column :merge_requests, :merge_when_build_succeeds, :boolean, default: false, null: false + add_column :merge_requests, :merge_user_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 4bde9f0b748..825d95565be 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20151026182941) do +ActiveRecord::Schema.define(version: 20151028152939) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -418,6 +418,7 @@ ActiveRecord::Schema.define(version: 20151026182941) 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 "members", force: true do |t| t.integer "access_level", null: false @@ -453,9 +454,9 @@ ActiveRecord::Schema.define(version: 20151026182941) do add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree create_table "merge_requests", force: true do |t| - t.string "target_branch", null: false - t.string "source_branch", null: false - t.integer "source_project_id", null: false + t.string "target_branch", null: false + t.string "source_branch", null: false + t.integer "source_project_id", null: false t.integer "author_id" t.integer "assignee_id" t.string "title" @@ -464,13 +465,16 @@ ActiveRecord::Schema.define(version: 20151026182941) do t.integer "milestone_id" t.string "state" t.string "merge_status" - t.integer "target_project_id", null: false + t.integer "target_project_id", null: false t.integer "iid" t.text "description" - t.integer "position", default: 0 + t.integer "position", default: 0 t.datetime "locked_at" t.integer "updated_by_id" t.string "merge_error" + t.text "merge_params" + t.boolean "merge_when_build_succeeds", default: false, null: false + t.integer "merge_user_id" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree @@ -499,6 +503,7 @@ ActiveRecord::Schema.define(version: 20151026182941) 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 -- cgit v1.2.1 From 63b234706d46f75c0c0f93bdfdc576e328981eb5 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 2 Nov 2015 20:02:51 +0100 Subject: MRs author can cancel automatic merge --- app/controllers/projects/merge_requests_controller.rb | 4 +++- .../merge_requests/widget/open/_merge_when_build_succeeds.html.haml | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2f9b8a25edf..d58dab2d666 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -150,7 +150,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def cancel_merge_when_build_succeeds - return access_denied! unless @merge_request.can_be_merged_by?(current_user) + unless @merge_request.can_be_merged_by?(current_user) || @merge_request.author == current_user + return access_denied! + end if @merge_request.merge_when_build_succeeds? @merge_request.reset_merge_when_build_succeeds 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 7e5385cf8b9..f3894334968 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 @@ -14,7 +14,7 @@ = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch - The source branch will not be removed. + The source branch won't be removed. - if remove_source_branch_button || @merge_request.can_be_merged_by?(current_user) .clearfix.prepend-top-10 @@ -22,6 +22,6 @@ = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_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) + - if @merge_request.can_be_merged_by?(current_user) || @merge_request.author == current_user = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request), remote: true, method: :delete, class: "btn btn-grouped btn-warning btn-sm" do Cancel Automatic Merge -- cgit v1.2.1 From 2f048df4a4a83ff009d2ef2d14ee04e5a2798618 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 18 Nov 2015 11:17:41 +0100 Subject: API support, incorporated feedback --- .../projects/merge_requests_controller.rb | 4 +- app/models/commit_status.rb | 2 +- app/models/merge_request.rb | 6 +- app/services/merge_requests/merge_service.rb | 16 ++--- .../merge_when_build_succeeds_service.rb | 2 +- app/services/merge_requests/refresh_service.rb | 4 +- app/services/system_note_service.rb | 2 +- .../merge_requests/widget/open/_accept.html.haml | 2 +- .../open/_merge_when_build_succeeds.html.haml | 8 +-- app/workers/merge_worker.rb | 13 +---- config/routes.rb | 3 +- doc/api/merge_requests.md | 60 +++++++++++++++++-- lib/api/merge_requests.rb | 68 +++++++++++++++------- spec/models/merge_request_spec.rb | 14 ++++- spec/services/system_note_service_spec.rb | 16 +++++ 15 files changed, 157 insertions(+), 63 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d58dab2d666..931298df5d8 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -171,9 +171,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_request.update(merge_error: nil) if params[:merge_when_build_succeeds] && @merge_request.ci_commit.active? - MergeRequests::MergeWhenBuildSucceedsService.new(@project, - current_user, - merge_params: merge_params) + MergeRequests::MergeWhenBuildSucceedsService.new(@project, current_user, merge_params) .execute(@merge_request) @status = :merge_when_build_succeeds else diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index b1049fab788..acc86b1a8cd 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -75,7 +75,7 @@ class CommitStatus < ActiveRecord::Base build.update_attributes finished_at: Time.now end - after_transition running: :success do |build, transition| + after_transition [:pending, :running] => :success do |build, transition| MergeRequests::MergeWhenBuildSucceedsService.new(build.commit.gl_project, nil).trigger(build) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c5f04dbcf4c..7b372399a3a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -253,6 +253,10 @@ class MergeRequest < ActiveRecord::Base end end + def mergeable_by_or_author(user) + self.can_be_merged_by?(user) || self.author == user + end + def mr_and_commit_notes # Fetch comments only from last 100 commits commits_for_notes_limit = 100 @@ -390,7 +394,7 @@ class MergeRequest < ActiveRecord::Base def reset_merge_when_build_succeeds return unless merge_when_build_succeeds? - + self.merge_when_build_succeeds = false self.merge_user = nil self.merge_params = nil diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 7963af127e1..db8d18a7d84 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -6,15 +6,12 @@ module MergeRequests # Executed when you do merge via GitLab UI # class MergeService < MergeRequests::BaseService - attr_reader :merge_request, :commit_message + attr_reader :merge_request - def execute(merge_request, commit_message) - @commit_message = commit_message + def execute(merge_request) @merge_request = merge_request - unless @merge_request.mergeable? - return error('Merge request is not mergeable') - end + return error('Merge request is not mergeable') unless @merge_request.mergeable? merge_request.in_locked_state do if commit @@ -32,7 +29,7 @@ module MergeRequests committer = repository.user_to_committer(current_user) options = { - message: commit_message, + message: params[:commit_message] || merge_request.merge_commit_message, author: committer, committer: committer } @@ -46,6 +43,11 @@ module MergeRequests def after_merge MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) + + if params[:should_remove_source_branch] + DeleteBranchService.new(@merge_request.source_project, current_user). + execute(merge_request.source_branch) + end end end end 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 a4418360b8c..d5cae2f98f7 100644 --- a/app/services/merge_requests/merge_when_build_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -1,7 +1,7 @@ module MergeRequests class MergeWhenBuildSucceedsService < MergeRequests::BaseService def execute(merge_request) - merge_request.merge_params.merge!(params[:merge_params]) + merge_request.merge_params.merge!(params) # The service is also called when the merge params are updated. already_approved = merge_request.merge_when_build_succeeds? diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 335ef32abce..e5024857201 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -77,9 +77,7 @@ module MergeRequests end def reset_merge_when_build_succeeds - merge_requests_for_source_branch.each do |merge_request| - merge_request.reset_merge_when_build_succeeds - end + merge_requests_for_source_branch.each(&:reset_merge_when_build_succeeds) end def find_new_commits diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index c9846e9f26f..5e8281a3fd0 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -132,7 +132,7 @@ class SystemNoteService # Called when 'merge when build succeeds' is executed def self.merge_when_build_succeeds(noteable, project, author) - body = "Approved this request to be merged automatically when the build succeeds" + body = "This merge request will be automatically merged when the build for #{noteable.ci_commit.short_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 2afc5f81251..2a51241971f 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -29,5 +29,5 @@ $('.accept_merge_request').on 'click', -> btn = $(this) btn.html(" Merge in progress") - $('.accept-mr-form').on 'ajax:sen', -> + $('.accept-mr-form').on 'ajax:send', -> $(".accept-mr-form :input").disable() 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 f3894334968..ddd1a7bd63d 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 @@ -1,9 +1,9 @@ %h4 Approved by #{link_to_member(@project, @merge_request.merge_user, avatar: true)} to be merged automatically when - #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds + #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds. %div - - if @merge_request.merge_params["should_remove_source_branch"] + - if @merge_request.merge_params["should_remove_source_branch"].present? = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch @@ -19,9 +19,9 @@ - if remove_source_branch_button || @merge_request.can_be_merged_by?(current_user) .clearfix.prepend-top-10 - if remove_source_branch_button - = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_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 + = 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 - = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request), remote: true, method: :delete, class: "btn btn-grouped btn-warning btn-sm" do + = 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/app/workers/merge_worker.rb b/app/workers/merge_worker.rb index 5d1a8555b7d..c87c0a252b1 100644 --- a/app/workers/merge_worker.rb +++ b/app/workers/merge_worker.rb @@ -8,16 +8,7 @@ class MergeWorker current_user = User.find(current_user_id) merge_request = MergeRequest.find(merge_request_id) - result = MergeRequests::MergeService.new(merge_request.target_project, current_user). - execute(merge_request, params[:commit_message]) - - if result[:status] == :success && params[:should_remove_source_branch].present? - DeleteBranchService.new(merge_request.source_project, current_user). - execute(merge_request.source_branch) - - merge_request.source_project.repository.expire_branch_names - end - - result + MergeRequests::MergeService.new(merge_request.target_project, current_user, params). + execute(merge_request) end end diff --git a/config/routes.rb b/config/routes.rb index 917c3d3f1ed..993687665cc 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -556,8 +556,7 @@ Gitlab::Application.routes.draw do get :diffs get :commits post :merge - delete :merge, action: :cancel_merge_when_build_succeeds - get :merge_check + post :cancel_merge_when_build_succeeds get :ci_status post :toggle_subscription end diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index ffa7f2cdf14..1b2ad1caf49 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -2,8 +2,8 @@ ## List merge requests -Get all merge requests for this project. -The `state` parameter can be used to get only merge requests with a given state (`opened`, `closed`, or `merged`) or all of them (`all`). +Get all merge requests for this project. +The `state` parameter can be used to get only merge requests with a given state (`opened`, `closed`, or `merged`) or all of them (`all`). The pagination parameters `page` and `per_page` can be used to restrict the list of merge requests. ``` @@ -292,9 +292,59 @@ PUT /projects/:id/merge_request/:merge_request_id/merge Parameters: -- `id` (required) - The ID of a project -- `merge_request_id` (required) - ID of MR -- `merge_commit_message` (optional) - Custom merge commit message +- `id` (required) - The ID of a project +- `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 + +```json +{ + "id": 1, + "target_branch": "master", + "source_branch": "test1", + "project_id": 3, + "title": "test1", + "state": "merged", + "upvotes": 0, + "downvotes": 0, + "author": { + "id": 1, + "username": "admin", + "email": "admin@example.com", + "name": "Administrator", + "state": "active", + "created_at": "2012-04-29T08:46:00Z" + }, + "assignee": { + "id": 1, + "username": "admin", + "email": "admin@example.com", + "name": "Administrator", + "state": "active", + "created_at": "2012-04-29T08:46:00Z" + } +} +``` + +## Cancel Merge When Build Succeeds + +Cancels the merge when build succeeds and reset the merge parameters + +If successfull 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' +``` +PUT /projects/:id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds +``` +Parameters: + +- `id` (required) - The ID of a project +- `merge_request_id` (required) - ID of MR ```json { diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 6eb84baf9cb..f981432db36 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -179,9 +179,10 @@ module API # Merge MR # # Parameters: - # id (required) - The ID of a project - # merge_request_id (required) - ID of MR - # merge_commit_message (optional) - Custom merge commit message + # 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 # Example: # PUT /projects/:id/merge_request/:merge_request_id/merge # @@ -191,34 +192,57 @@ module API allowed = ::Gitlab::GitAccess.new(current_user, user_project). can_push_to_branch?(merge_request.target_branch) - if allowed - if merge_request.unchecked? - merge_request.check_if_can_be_merged - end + # Merge request can not be merged + # because user dont have permissions to push into target branch + unauthorized! unless allowed + + not_allowed! unless merge_request.open? - if merge_request.open? && !merge_request.work_in_progress? - if merge_request.can_be_merged? - commit_message = params[:merge_commit_message] || merge_request.merge_commit_message + merge_request.check_if_can_be_merged if merge_request.unchecked? - ::MergeRequests::MergeService.new(merge_request.target_project, current_user). - execute(merge_request, commit_message) + merge_params = { + commit_message: params[:merge_commit_message], + should_remove_source_branch: params[:should_remove_source_branch] + } - present merge_request, with: Entities::MergeRequest - else - render_api_error!('Branch cannot be merged', 405) - end + 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 - # Merge request can not be merged - # because it is already closed/merged or marked as WIP - not_allowed! + ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params). + execute(merge_request, merge_params) end else - # Merge request can not be merged - # because user dont have permissions to push into target branch - unauthorized! + render_api_error!('Branch cannot be merged', 405) end + + present merge_request, with: Entities::MergeRequest end + # Cancel Merge if Merge When build succeeds is enabled + # Parameters: + # id (required) - The ID of a project + # merge_request_id (required) - ID of MR + # + post ":id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds" 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 + + 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 + end # Get a merge request's comments # diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index eed2cbc5412..e42534a8491 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -30,7 +30,7 @@ describe MergeRequest do describe 'associations' do it { is_expected.to belong_to(:target_project).with_foreign_key(:target_project_id).class_name('Project') } it { is_expected.to belong_to(:source_project).with_foreign_key(:source_project_id).class_name('Project') } - + it { is_expected.to belong_to(:merge_user).class_name("User") } it { is_expected.to have_one(:merge_request_diff).dependent(:destroy) } end @@ -53,6 +53,8 @@ describe MergeRequest do it { is_expected.to respond_to(:unchecked?) } it { is_expected.to respond_to(:can_be_merged?) } it { is_expected.to respond_to(:cannot_be_merged?) } + it { is_expected.to respond_to(:merge_params) } + it { is_expected.to respond_to(:merge_when_build_succeeds) } end describe '#to_reference' do @@ -171,6 +173,16 @@ describe MergeRequest do end end + describe "#reset_merge_when_build_succeeds" do + let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true } + it "sets the item to false" do + merge_if_green.reset_merge_when_build_succeeds + merge_if_green.reload + + expect(merge_if_green.merge_when_build_succeeds).to be_falsey + end + end + describe "#hook_attrs" do it "has all the required keys" do attrs = subject.hook_attrs diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index a45130bd473..35912ece644 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -207,6 +207,22 @@ describe SystemNoteService do end end + describe '.merge_when_build_succeeds' do + let(:ci_commit) { create :ci_commit, gl_project: project } + let(:merge_request) { create :merge_request, project: project } + + subject { described_class.merge_when_build_succeeds(merge_request, project, author) } + + it_behaves_like 'a system note' + + it "posts the Merge When Build Succeeds system note" do + allow(merge_request).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:short_sha).and_return('12345678') + + expect(subject.note).to eq "This merge request will be automatically merged when the build for 12345678 succeeds" + end + end + describe '.change_title' do subject { described_class.change_title(noteable, project, author, 'Old title') } -- cgit v1.2.1 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 From a7682f8775a4609ac8c70151ffe8f3ccf3b767b6 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 24 Nov 2015 14:59:02 +0100 Subject: Specs for 'Merge When Build Succeeds' --- .gitignore | 1 + app/models/merge_request.rb | 13 ++-- .../merge_when_build_succeeds_service.rb | 6 +- app/services/system_note_service.rb | 2 +- .../merge_requests/widget/_merged.html.haml | 4 +- .../merge_requests/widget/open/_accept.html.haml | 4 +- .../open/_merge_when_build_succeeds.html.haml | 21 +++--- lib/api/entities.rb | 1 + lib/api/merge_requests.rb | 22 +++--- spec/factories/ci/commits.rb | 23 ++++--- spec/factories/commit_statuses.rb | 2 +- .../merge_when_build_succeeds_spec.rb | 80 ++++++++++++++++++++++ spec/models/merge_request_spec.rb | 24 +++++++ spec/requests/api/merge_requests_spec.rb | 17 ++++- spec/services/merge_requests/merge_service_spec.rb | 8 +-- .../merge_when_build_succeeds_service_spec.rb | 78 +++++++++++++++++++++ .../merge_requests/refresh_service_spec.rb | 5 +- spec/services/system_note_service_spec.rb | 22 ++++-- 18 files changed, 268 insertions(+), 65 deletions(-) create mode 100644 spec/features/merge_requests/merge_when_build_succeeds_spec.rb create mode 100644 spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb diff --git a/.gitignore b/.gitignore index f5b6427ca03..881b3fb81ac 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,7 @@ public/assets/ public/uploads.* public/uploads/ shared/artifacts/ +TODO rails_best_practices_output.html /tags tmp/ diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 89f9e8fa6a8..131dfda6b5f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -254,15 +254,14 @@ class MergeRequest < ActiveRecord::Base end end - def can_cancel_merge_when_build_succeeds?(user) - can_be_merged_by?(user) || self.author == user + def can_cancel_merge_when_build_succeeds?(current_user) + can_be_merged_by?(current_user) || self.author == current_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) + def can_remove_source_branch?(current_user) + !source_project.protected_branch?(source_branch) && + !source_project.root_ref?(source_branch) && + Ability.abilities.allowed?(current_user, :push_code, source_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 15dcace5dfb..2f101e53a3f 100644 --- a/app/services/merge_requests/merge_when_build_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -11,13 +11,11 @@ module MergeRequests unless already_approved merge_request.merge_when_build_succeeds = true merge_request.merge_user = @current_user - end - - merge_request.save - unless already_approved SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit) end + + merge_request.save end # Triggers the automatic merge of merge_request once the build succeeds diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 7de4221c4c4..ed557fef814 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -132,7 +132,7 @@ class SystemNoteService # Called when 'merge when build succeeds' is executed def self.merge_when_build_succeeds(noteable, project, author, ci_commit) - body = "Approved an automatic merge when the build for #{ci_commit.sha} succeeds" + body = "Enabled 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/_merged.html.haml b/app/views/projects/merge_requests/widget/_merged.html.haml index a788fcea23f..52e34ee617e 100644 --- a/app/views/projects/merge_requests/widget/_merged.html.haml +++ b/app/views/projects/merge_requests/widget/_merged.html.haml @@ -13,7 +13,7 @@ %span.label-branch= @merge_request.target_branch The source branch has been removed. - - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) + - elsif @merge_request.can_remove_source_branch?(current_user) .remove_source_branch_widget %p = succeed '.' do @@ -48,5 +48,3 @@ $('.remove_source_branch_in_progress').hide(); $('.remove_source_branch_widget.failed').show(); }); - - 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 5ec623b472c..279e2ec91f8 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -7,13 +7,13 @@ - ci_commit = @merge_request.ci_commit - if ci_commit && ci_commit.active? = f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do - Merge when Build Succeeds + Merge When Build Succeeds = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do Accept Merge Request Now - else = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do Accept Merge Request - - if @merge_request.can_remove_source_branch? + - if @merge_request.can_remove_source_branch?(current_user) .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 51e18f84424..43ba49c5a5e 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 @@ -3,22 +3,25 @@ to be merged automatically when #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds. %div - - if @merge_request.merge_params["should_remove_source_branch"].present? + - source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present? + - if source_branch_removed = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch The source branch will be removed. + - else %p = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch The source branch will not be removed. - .clearfix.prepend-top-10 - - 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.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 + - if (@merge_request.can_remove_source_branch?(current_user) && !source_branch_removed) || @merge_request.can_cancel_merge_when_build_succeeds?(current_user) + .clearfix.prepend-top-10 + - if @merge_request.can_remove_source_branch?(current_user) && !source_branch_removed + = 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_cancel_merge_when_build_succeeds?(current_user) + = 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/lib/api/entities.rb b/lib/api/entities.rb index d6aec03d7f5..6f9f71b0945 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -169,6 +169,7 @@ module API expose :description expose :work_in_progress?, as: :work_in_progress expose :milestone, using: Entities::Milestone + expose :merge_when_build_succeeds end class MergeRequestChanges < MergeRequest diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index b72f816932b..32cb1137ef7 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -179,11 +179,11 @@ module API # Merge MR # # Parameters: - # id (required) - The ID of a project - # merge_request_id (required) - ID of MR - # merge_commit_message (optional) - Custom merge commit message - # 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 + # id (required) - The ID of a project + # merge_request_id (required) - ID of MR + # merge_commit_message (optional) - Custom merge commit message + # should_remove_source_branch (optional) - 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 # @@ -193,12 +193,11 @@ module API # Merge request can not be merged # because user dont have permissions to push into target branch unauthorized! unless merge_request.can_be_merged_by?(current_user) - - not_allowed! unless merge_request.open? && !merge_request.work_in_progress? + not_allowed! if !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? + render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? merge_params = { commit_message: params[:merge_commit_message], @@ -206,7 +205,7 @@ module API } 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). + ::MergeRequests::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). @@ -224,11 +223,6 @@ module API post ":id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds" 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 merge_request.can_cancel_merge_when_build_succeeds?(current_user) ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user).cancel(merge_request) diff --git a/spec/factories/ci/commits.rb b/spec/factories/ci/commits.rb index 79e000b7ccb..70e3fa319c6 100644 --- a/spec/factories/ci/commits.rb +++ b/spec/factories/ci/commits.rb @@ -2,17 +2,18 @@ # # Table name: commits # -# id :integer not null, primary key -# project_id :integer -# ref :string(255) -# sha :string(255) -# before_sha :string(255) -# push_data :text -# created_at :datetime -# updated_at :datetime -# tag :boolean default(FALSE) -# yaml_errors :text -# committed_at :datetime +# id :integer not null, primary key +# project_id :integer +# ref :string(255) +# sha :string(255) +# before_sha :string(255) +# push_data :text +# created_at :datetime +# updated_at :datetime +# tag :boolean default(FALSE) +# yaml_errors :text +# committed_at :datetime +# gl_project_id :integer # # Read about factories at https://github.com/thoughtbot/factory_girl diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb index 52de437052d..8898b71e2a3 100644 --- a/spec/factories/commit_statuses.rb +++ b/spec/factories/commit_statuses.rb @@ -5,7 +5,7 @@ FactoryGirl.define do name 'default' status 'success' description 'commit status' - commit factory: :ci_commit + commit factory: :ci_commit_with_one_job factory :generic_commit_status, class: GenericCommitStatus do name 'generic' diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb new file mode 100644 index 00000000000..b25a3f05e29 --- /dev/null +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +feature 'Merge When Build Succeeds', feature: true, js: true do + let(:user) { create(:user) } + + let(:project) { create(:project, :public) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") } + + before do + project.team << [user, :master] + project.enable_ci + end + + context "Active build for Merge Request" do + before do + ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) + ci_build = create(:ci_build, commit: ci_commit) + + login_as user + visit_merge_request(merge_request) + end + + it 'displays the Merge When Build Succeeds button' do + expect(page).to have_button "Merge When Build Succeeds" + end + + context "Merge When Build succeeds enabled" do + before do + click_button "Merge When Build Succeeds" + end + + it 'activates Merge When Build Succeeds feature' do + expect(page).to have_link "Cancel Automatic Merge" + + expect(page).to have_content "Approved by #{user.name} to be merged automatically when the build succeeds." + expect(page).to have_content "The source branch will not be removed." + end + end + end + + context 'When it is enabled' do + # No clue how, but push a new commit to the branch + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, # source_branch: "mepmep", + author: user, title: "Bug NS-04", merge_when_build_succeeds: true) } + + before do + merge_request.source_project.team << [user, :master] + merge_request.source_branch = "feature" + merge_request.target_branch = "master" + merge_request.save! + + ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) + ci_build = create(:ci_build, commit: ci_commit) + + login_as user + visit_merge_request(merge_request) + end + + it 'cancels the automatic merge' do + click_link "Cancel Automatic Merge" + + expect(page).to have_button "Merge When Build Succeeds" + end + + it "allows the user to remove the source branch" do + expect(page).to have_link "Remove Source Branch When Merged" + end + end + + context 'Build is not active' do + it "should not allow for enabling" do + visit_merge_request(merge_request) + expect(page).not_to have_button "Merge When Build Succeeds" + end + end + + def visit_merge_request(merge_request) + visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1bd09a1b0fb..c7a9765825e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -174,6 +174,30 @@ describe MergeRequest do end end + describe '#can_remove_source_branch' do + let(:user) { build(:user)} + + before do + subject.source_project.team << [user, :master] + end + + it "cant be merged when its a a protected branch" do + subject.source_project.protected_branches = []; + + expect(subject.can_remove_source_branch?(user)).to be_falsey + end + + it "cant remove a root ref" do + subject.source_branch = "master"; + + expect(subject.can_remove_source_branch?(user)).to be_falsey + end + + it "is truthy in all other cases" do + expect(subject.can_remove_source_branch?(user)) + end + end + describe "#reset_merge_when_build_succeeds" do let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true } it "sets the item to false" do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index a68c7b1e461..91ae2059e95 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -303,19 +303,21 @@ describe API::API, api: true do end describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do + let (:ci_commit) { create(:ci_commit_without_jobs) } + it "should return merge_request in case of success" do put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) expect(response.status).to eq(200) end - it "should return 405 if branch can't be merged" do + it "should return 406 if branch can't be merged" do allow_any_instance_of(MergeRequest). to receive(:can_be_merged?).and_return(false) put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) - expect(response.status).to eq(405) + expect(response.status).to eq(406) expect(json_response['message']).to eq('Branch cannot be merged') end @@ -340,6 +342,17 @@ describe API::API, api: true do expect(response.status).to eq(401) expect(json_response['message']).to eq('401 Unauthorized') end + + it "enables merge when build succeeds if the ci is active" do + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:active?).and_return(true) + + put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user), merge_when_build_succeeds: true + + expect(response.status).to eq(200) + expect(json_response['title']).to eq('Test') + expect(json_response['merge_when_build_succeeds']).to eq(true) + end end describe "PUT /projects/:id/merge_request/:merge_request_id" do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 7483f51de03..242524286fa 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -13,12 +13,12 @@ describe MergeRequests::MergeService do describe :execute do context 'valid params' do - let(:service) { MergeRequests::MergeService.new(project, user, {}) } + let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') } before do allow(service).to receive(:execute_hooks) - service.execute(merge_request, 'Awesome message') + service.execute(merge_request) end it { expect(merge_request).to be_valid } @@ -37,14 +37,14 @@ describe MergeRequests::MergeService do end context "error handling" do - let(:service) { MergeRequests::MergeService.new(project, user, {}) } + let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') } it 'saves error if there is an exception' do allow(service).to receive(:repository).and_raise("error") allow(service).to receive(:execute_hooks) - service.execute(merge_request, 'Awesome message') + service.execute(merge_request) expect(merge_request.merge_error).to eq("Something went wrong during merge") end diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb new file mode 100644 index 00000000000..8638539173b --- /dev/null +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -0,0 +1,78 @@ +require 'spec_helper' + +describe MergeRequests::MergeWhenBuildSucceedsService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + let(:mr_merge_if_green_enabled) { create(:merge_request, merge_when_build_succeeds: true, + source_branch: "source_branch", target_branch: project.default_branch, + source_project: project, target_project: project, state: "opened") } + let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch) } + let(:project) { create(:project) } + let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') } + + before do + project.ci_commits = [ci_commit] + project.save! + end + describe "#execute" do + context 'first time enabling' do + before do + allow(merge_request).to receive(:ci_commit).and_return(ci_commit) + end + + it 'sets the params, merge_user, and flag' do + service.execute(merge_request) + + expect(merge_request).to be_valid + expect(merge_request.merge_when_build_succeeds).to be_truthy + expect(merge_request.merge_params).to eq commit_message: 'Awesome message' + expect(merge_request.merge_user).to be user + + note = merge_request.notes.last + expect(note.note).to include 'Enabled an automatic merge when the build for' + end + end + + context 'allready approved' do + let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, new_key: true) } + let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } + + before do + allow(mr_merge_if_green_enabled).to receive(:ci_commit).and_return(ci_commit) + allow(mr_merge_if_green_enabled).to receive(:mergeable?).and_return(true) + allow(ci_commit).to receive(:success?).and_return(true) + end + + it 'updates the merge params' do + expect(SystemNoteService).not_to receive(:merge_when_build_succeeds) + + service.execute(mr_merge_if_green_enabled) + expect(mr_merge_if_green_enabled.merge_params).to have_key(:new_key) + end + end + end + + describe "#trigger" do + let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") } + + it "merges all merge requests with merge when build succeeds enabled" do + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:success?).and_return(true) + + expect(MergeWorker).to receive(:perform_async) + service.trigger(build) + end + end + + describe "#cancel" do + before do + service.cancel(mr_merge_if_green_enabled) + end + + it "resets all the merge_when_build_succeeds params" do + expect(mr_merge_if_green_enabled.merge_when_build_succeeds).to be_falsey + expect(mr_merge_if_green_enabled.merge_params).to eq({}) + expect(mr_merge_if_green_enabled.merge_user).to be nil + end + end +end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 7ee4488521d..18b2659c1f6 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -17,7 +17,8 @@ describe MergeRequests::RefreshService do source_project: @project, source_branch: 'master', target_branch: 'feature', - target_project: @project) + target_project: @project, + merge_when_build_succeeds: true) @fork_merge_request = create(:merge_request, source_project: @fork_project, @@ -46,6 +47,7 @@ describe MergeRequests::RefreshService 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(@fork_merge_request).to be_open } it { expect(@fork_merge_request.notes).to be_empty } end @@ -146,6 +148,7 @@ describe MergeRequests::RefreshService do end 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 35912ece644..5d41a5cdc69 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -208,18 +208,28 @@ describe SystemNoteService do end describe '.merge_when_build_succeeds' do - let(:ci_commit) { create :ci_commit, gl_project: project } - let(:merge_request) { create :merge_request, project: project } + let(:ci_commit) { create :ci_commit_without_jobs } + let(:noteable) { create :merge_request } - subject { described_class.merge_when_build_succeeds(merge_request, project, author) } + subject { described_class.merge_when_build_succeeds(noteable, project, author, ci_commit) } it_behaves_like 'a system note' it "posts the Merge When Build Succeeds system note" do - allow(merge_request).to receive(:ci_commit).and_return(ci_commit) - allow(ci_commit).to receive(:short_sha).and_return('12345678') + expect(subject.note).to eq "Enabled an automatic merge when the build for 97de212e80737a608d939f648d959671fb0a0142 succeeds" + end + end + + describe '.cancel_merge_when_build_succeeds' do + let(:ci_commit) { create :ci_commit_without_jobs } + let(:noteable) { create :merge_request } + + subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) } - expect(subject.note).to eq "This merge request will be automatically merged when the build for 12345678 succeeds" + it_behaves_like 'a system note' + + it "posts the Merge When Build Succeeds system note" do + expect(subject.note).to eq "Canceled the automatic merge" end end -- cgit v1.2.1 From 25907ebe476a24bfdd2c451f18227d4fcf314b07 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 2 Dec 2015 13:28:21 +0100 Subject: Doc feature Merge When Build Succeeds --- .../disable_merge_when_build_succeeds.png | Bin 0 -> 20551 bytes .../enable_merge_when_build_succeeds.png | Bin 0 -> 13150 bytes doc/workflow/merge_when_build_succeeds.md | 20 ++++++++++++++++++++ 3 files changed, 20 insertions(+) create mode 100644 doc/workflow/merge_requests/disable_merge_when_build_succeeds.png create mode 100644 doc/workflow/merge_requests/enable_merge_when_build_succeeds.png create mode 100644 doc/workflow/merge_when_build_succeeds.md diff --git a/doc/workflow/merge_requests/disable_merge_when_build_succeeds.png b/doc/workflow/merge_requests/disable_merge_when_build_succeeds.png new file mode 100644 index 00000000000..a45a4890b62 Binary files /dev/null and b/doc/workflow/merge_requests/disable_merge_when_build_succeeds.png differ diff --git a/doc/workflow/merge_requests/enable_merge_when_build_succeeds.png b/doc/workflow/merge_requests/enable_merge_when_build_succeeds.png new file mode 100644 index 00000000000..62a46c9508b Binary files /dev/null and b/doc/workflow/merge_requests/enable_merge_when_build_succeeds.png differ diff --git a/doc/workflow/merge_when_build_succeeds.md b/doc/workflow/merge_when_build_succeeds.md new file mode 100644 index 00000000000..9bf6ddcc569 --- /dev/null +++ b/doc/workflow/merge_when_build_succeeds.md @@ -0,0 +1,20 @@ +# Merge When Build Succeeds + +Select a Merge Request to be merged if the build succeeds so the user does not have to wait for the build to finish and revisit the Merge Request to merge it after the build is done. + +## Enabling for a Merge Request + +Given an active build for a Merge Request, thus pending or running, a `Merge When Build Succeeds` button will appear to any user which can merge it. Once clicked, it ensures this merge request is merged when the build is successful. +When clicking the button, the merge parameters are also saved to allow the merge user to edit the commit message and remove the source branch if he can remove that branch. + +When this feature is enabled, a message will appear to notify other users. Also a note is posted on the thread. + +![Enable Merge When Build Succceeds](merge_requests/enable_merge_when_build_succeeds.png) + +## Canceling + +The automatic merge can be disabled by clicking the `Cancel Automatic Merge` button, or when a new commit is added to the Merge Request. In the former case a note is posted. In the latter case a user able to merge can enable the feature again. + +![Disable the automatic merge](merge_requests/disable_merge_when_build_succeeds.png) + +A failed build does not reset the automatic build so a build can be retried. -- cgit v1.2.1 From 2462a96e459c95f987f39e3c380de7c7cc350cfd Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 3 Dec 2015 10:27:34 +0100 Subject: Incorporate feedback --- app/models/merge_request.rb | 46 +++++++++++----------- .../merge_when_build_succeeds_service.rb | 9 ++--- app/services/system_note_service.rb | 6 +-- .../merge_requests/_merge_request.html.haml | 5 +-- .../merge_requests/widget/_heading.html.haml | 9 ++--- .../merge_requests/widget/open/_accept.html.haml | 3 +- .../open/_merge_when_build_succeeds.html.haml | 24 +++++------ spec/factories/merge_requests.rb | 5 +++ .../merge_when_build_succeeds_spec.rb | 21 ++++++++-- spec/models/merge_request_spec.rb | 44 +++++++++++++++++---- spec/requests/api/merge_requests_spec.rb | 2 +- .../merge_when_build_succeeds_service_spec.rb | 30 ++++++++------ .../merge_requests/refresh_service_spec.rb | 3 +- spec/services/system_note_service_spec.rb | 10 ++--- 14 files changed, 134 insertions(+), 83 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 131dfda6b5f..1f81e23c7a3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -2,25 +2,28 @@ # # Table name: merge_requests # -# id :integer not null, primary key -# target_branch :string(255) not null -# source_branch :string(255) not null -# source_project_id :integer not null -# author_id :integer -# assignee_id :integer -# title :string(255) -# created_at :datetime -# updated_at :datetime -# milestone_id :integer -# state :string(255) -# merge_status :string(255) -# target_project_id :integer not null -# iid :integer -# description :text -# position :integer default(0) -# locked_at :datetime -# updated_by_id :integer -# merge_error :string(255) +# id :integer not null, primary key +# target_branch :string(255) not null +# source_branch :string(255) not null +# source_project_id :integer not null +# author_id :integer +# assignee_id :integer +# title :string(255) +# created_at :datetime +# updated_at :datetime +# milestone_id :integer +# state :string(255) +# merge_status :string(255) +# target_project_id :integer not null +# iid :integer +# description :text +# position :integer default(0) +# locked_at :datetime +# updated_by_id :integer +# merge_error :string(255) +# merge_params :text (serialized to hash) +# merge_when_build_succeeds :boolean default(false), not null +# merge_user_id :integer # require Rails.root.join("app/models/commit") @@ -124,6 +127,7 @@ class MergeRequest < ActiveRecord::Base validates :source_branch, presence: true validates :target_project, presence: true validates :target_branch, presence: true + validates :merge_user, presence: true, if: :merge_when_build_succeeds? validate :validate_branches validate :validate_fork @@ -496,8 +500,6 @@ class MergeRequest < ActiveRecord::Base end def ci_commit - if last_commit - source_project.ci_commit(last_commit.id) - end + @ci_commit ||= source_project.ci_commit(last_commit.id) if last_commit end end 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 2f101e53a3f..5cf7404a493 100644 --- a/app/services/merge_requests/merge_when_build_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -1,6 +1,6 @@ module MergeRequests class MergeWhenBuildSucceedsService < MergeRequests::BaseService - # Marks the passed `merge_request` to be marked when the build succeeds or + # Marks the passed `merge_request` to be merged when the build succeeds or # updates the params for the automatic merge def execute(merge_request) merge_request.merge_params.merge!(params) @@ -12,7 +12,7 @@ module MergeRequests merge_request.merge_when_build_succeeds = true merge_request.merge_user = @current_user - SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit) + SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.last_commit) end merge_request.save @@ -25,8 +25,7 @@ module MergeRequests merge_requests.each do |merge_request| next unless merge_request.merge_when_build_succeeds? - ci_commit = merge_request.ci_commit - if ci_commit && ci_commit.success? && merge_request.mergeable? + if merge_request.ci_commit && merge_request.ci_commit.success? && merge_request.mergeable? MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params) end end @@ -34,7 +33,7 @@ module MergeRequests # Cancels the automatic merge def cancel(merge_request) - if merge_request.merge_when_build_succeeds? && merge_request.open? && !merge_request.merged? + if merge_request.merge_when_build_succeeds? && merge_request.open? merge_request.reset_merge_when_build_succeeds SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index ed557fef814..f84e480ca9c 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -131,15 +131,15 @@ class SystemNoteService end # Called when 'merge when build succeeds' is executed - def self.merge_when_build_succeeds(noteable, project, author, ci_commit) - body = "Enabled an automatic merge when the build for #{ci_commit.sha} succeeds" + def self.merge_when_build_succeeds(noteable, project, author, last_commit) + body = "Enabled an automatic merge when the build for #{last_commit.to_reference} succeeds" create_note(noteable: noteable, project: project, author: author, note: body) end # Called when 'merge when build succeeds' is canceled def self.cancel_merge_when_build_succeeds(noteable, project, author) - body = "Canceled the automatic merge" + body = "Cancelled the automatic merge" create_note(noteable: noteable, project: project, author: author, note: body) end diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index c5234c0618c..d10ccb30571 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -1,4 +1,3 @@ -- ci_commit = merge_request.ci_commit %li{ class: mr_css_classes(merge_request) } .merge-request-title %span.merge-request-title-text @@ -7,8 +6,8 @@ - merge_request.labels.each do |label| = link_to_label(label, project: merge_request.project) .pull-right.light - - if ci_commit - = render_ci_status(ci_commit) + - if merge_request.ci_commit + = render_ci_status(merge_request.ci_commit) - if merge_request.merged? %span %i.fa.fa-check diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index ba5ad22bca7..9513a18f105 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -1,13 +1,12 @@ -- ci_commit = @merge_request.ci_commit -- if ci_commit - - status = ci_commit.status +- if @merge_request.ci_commit + - status = @merge_request.ci_commit.status .mr-widget-heading .ci_widget{class: "ci-#{status}"} - = ci_status_icon(ci_commit) + = ci_status_icon(@merge_request.ci_commit) %span CI build #{status} for #{@merge_request.last_commit_short_sha}. %span.ci-coverage - = link_to "View build details", ci_status_path(ci_commit) + = link_to "View build details", ci_status_path(@merge_request.ci_commit) - elsif @merge_request.has_ci? - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX 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 279e2ec91f8..f7d872aa455 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -4,8 +4,7 @@ = hidden_field_tag :authenticity_token, form_authenticity_token .accept-merge-holder.clearfix.js-toggle-container .accept-action - - ci_commit = @merge_request.ci_commit - - if ci_commit && ci_commit.active? + - if @merge_request.ci_commit && @merge_request.ci_commit.active? = f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do Merge When Build Succeeds = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do 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 43ba49c5a5e..9788d68b54e 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 @@ -1,27 +1,27 @@ %h4 - Approved by #{link_to_member(@project, @merge_request.merge_user, avatar: true)} + Set by #{link_to_member(@project, @merge_request.merge_user, avatar: true)} to be merged automatically when #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds. %div - - source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present? - - if source_branch_removed + - should_remove_source_branch = @merge_request.merge_params["should_remove_source_branch"].present? + %p = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch - The source branch will be removed. - - else - %p - = succeed '.' do - The changes will be merged into - %span.label-branch= @merge_request.target_branch + - if should_remove_source_branch + The source branch will be removed. + - else The source branch will not be removed. - - if (@merge_request.can_remove_source_branch?(current_user) && !source_branch_removed) || @merge_request.can_cancel_merge_when_build_succeeds?(current_user) + - remove_source_branch_button = @merge_request.can_remove_source_branch?(current_user) && !should_remove_source_branch + - user_can_cancel_automatic_merge = @merge_request.can_cancel_merge_when_build_succeeds?(current_user) + - if remove_source_branch_button || user_can_cancel_automatic_merge .clearfix.prepend-top-10 - - if @merge_request.can_remove_source_branch?(current_user) && !source_branch_removed + - if remove_source_branch_button = 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_cancel_merge_when_build_succeeds?(current_user) + + - if user_can_cancel_automatic_merge = 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/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 729a49c9f72..5b4d7f41bc4 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -65,6 +65,11 @@ FactoryGirl.define do target_branch "master" end + trait :merge_when_build_succeeds do + merge_when_build_succeeds true + merge_user author + end + factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index b25a3f05e29..2e64e903d1e 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -1,4 +1,6 @@ require 'spec_helper' +# rubocop:disable Lint/UselessAssignment +# As rubocop doesn't see a need for both `ci_commit` and `ci_build` feature 'Merge When Build Succeeds', feature: true, js: true do let(:user) { create(:user) } @@ -32,16 +34,20 @@ feature 'Merge When Build Succeeds', feature: true, js: true do it 'activates Merge When Build Succeeds feature' do expect(page).to have_link "Cancel Automatic Merge" - expect(page).to have_content "Approved by #{user.name} to be merged automatically when the build succeeds." + expect(page).to have_content "Set by #{user.name} to be merged automatically when the build succeeds." expect(page).to have_content "The source branch will not be removed." + + visit_merge_request(merge_request) # Needed to refresh the page + expect(page).to have_content /Enabled an automatic merge when the build for [0-9a-f]{8} succeeds/i end end end context 'When it is enabled' do - # No clue how, but push a new commit to the branch - let(:merge_request) { create(:merge_request_with_diffs, source_project: project, # source_branch: "mepmep", - author: user, title: "Bug NS-04", merge_when_build_succeeds: true) } + let(:merge_request) do + create(:merge_request_with_diffs, source_project: project, author: user, + merge_user: user, title: "MepMep", merge_when_build_succeeds: true) + end before do merge_request.source_project.team << [user, :master] @@ -60,10 +66,16 @@ feature 'Merge When Build Succeeds', feature: true, js: true do click_link "Cancel Automatic Merge" expect(page).to have_button "Merge When Build Succeeds" + + visit_merge_request(merge_request) # Needed to refresh the page + expect(page).to have_content "Cancelled the automatic merge" end it "allows the user to remove the source branch" do expect(page).to have_link "Remove Source Branch When Merged" + + click_link "Remove Source Branch When Merged" + expect(page).to have_content "The source branch will be removed" end end @@ -78,3 +90,4 @@ feature 'Merge When Build Succeeds', feature: true, js: true do visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) end end +# rubocop:enable Lint/UselessAssignment diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c7a9765825e..63e1cd1fb92 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -48,6 +48,24 @@ describe MergeRequest do describe 'validation' do it { is_expected.to validate_presence_of(:target_branch) } it { is_expected.to validate_presence_of(:source_branch) } + + context "Validation of merge user with Merge When Build succeeds" do + it "allows user to be nil when the feature is disabled" do + expect(subject).to be_valid + end + + it "is invalid without merge user" do + subject.merge_when_build_succeeds = true + expect(subject).not_to be_valid + end + + it "is valid with merge user" do + subject.merge_when_build_succeeds = true + subject.merge_user = build(:user) + + expect(subject).to be_valid + end + end end describe 'respond to' do @@ -175,31 +193,41 @@ describe MergeRequest do end describe '#can_remove_source_branch' do - let(:user) { build(:user)} + let(:user) { create(:user) } + let(:user2) { create(:user) } before do subject.source_project.team << [user, :master] - end - it "cant be merged when its a a protected branch" do - subject.source_project.protected_branches = []; + subject.source_branch = "feature" + subject.target_branch = "master" + subject.save! + end + it "can't be removed when its a protected branch" do + allow(subject.source_project).to receive(:protected_branch?).and_return(true) expect(subject.can_remove_source_branch?(user)).to be_falsey end it "cant remove a root ref" do - subject.source_branch = "master"; + subject.source_branch = "master" + subject.target_branch = "feature" expect(subject.can_remove_source_branch?(user)).to be_falsey end - it "is truthy in all other cases" do - expect(subject.can_remove_source_branch?(user)) + it "is unable to remove the source branch for a project the user cannot push to" do + expect(subject.can_remove_source_branch?(user2)).to be_falsey + end + + it "is can be removed in all other cases" do + expect(subject.can_remove_source_branch?(user)).to be_truthy end end describe "#reset_merge_when_build_succeeds" do - let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true } + let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true, merge_user: create(:user) } + it "sets the item to false" do merge_if_green.reset_merge_when_build_succeeds merge_if_green.reload diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 91ae2059e95..6b7a066ac40 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -303,7 +303,7 @@ describe API::API, api: true do end describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do - let (:ci_commit) { create(:ci_commit_without_jobs) } + let(:ci_commit) { create(:ci_commit_without_jobs) } it "should return merge_request in case of success" do put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb index 8638539173b..a62d88cde86 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -3,37 +3,38 @@ require 'spec_helper' describe MergeRequests::MergeWhenBuildSucceedsService do let(:user) { create(:user) } let(:merge_request) { create(:merge_request) } - let(:mr_merge_if_green_enabled) { create(:merge_request, merge_when_build_succeeds: true, - source_branch: "source_branch", target_branch: project.default_branch, - source_project: project, target_project: project, state: "opened") } - let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch) } + + let(:mr_merge_if_green_enabled) do + create(:merge_request, merge_when_build_succeeds: true, merge_user: user, + source_branch: "source_branch", target_branch: project.default_branch, + source_project: project, target_project: project, state: "opened") + end + let(:project) { create(:project) } + let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch, gl_project: project) } let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') } - before do - project.ci_commits = [ci_commit] - project.save! - end describe "#execute" do context 'first time enabling' do before do allow(merge_request).to receive(:ci_commit).and_return(ci_commit) + service.execute(merge_request) end it 'sets the params, merge_user, and flag' do - service.execute(merge_request) - expect(merge_request).to be_valid expect(merge_request.merge_when_build_succeeds).to be_truthy expect(merge_request.merge_params).to eq commit_message: 'Awesome message' expect(merge_request.merge_user).to be user + end + it 'creates a system note' do note = merge_request.notes.last - expect(note.note).to include 'Enabled an automatic merge when the build for' + expect(note.note).to match /Enabled an automatic merge when the build for (\w+\/\w+@)?[0-9a-z]{8}/ end end - context 'allready approved' do + context 'already approved' do let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, new_key: true) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } @@ -74,5 +75,10 @@ describe MergeRequests::MergeWhenBuildSucceedsService do expect(mr_merge_if_green_enabled.merge_params).to eq({}) expect(mr_merge_if_green_enabled.merge_user).to be nil end + + it 'Posts a system note' do + note = mr_merge_if_green_enabled.notes.last + expect(note.note).to include 'Cancelled the automatic merge' + end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 18b2659c1f6..9a8174f95fd 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -18,7 +18,8 @@ describe MergeRequests::RefreshService do source_branch: 'master', target_branch: 'feature', target_project: @project, - merge_when_build_succeeds: true) + merge_when_build_succeeds: true, + merge_user: @user) @fork_merge_request = create(:merge_request, source_project: @fork_project, diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 5d41a5cdc69..333035f2d2c 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -208,20 +208,20 @@ describe SystemNoteService do end describe '.merge_when_build_succeeds' do - let(:ci_commit) { create :ci_commit_without_jobs } + let(:ci_commit) { build :ci_commit_without_jobs } let(:noteable) { create :merge_request } - subject { described_class.merge_when_build_succeeds(noteable, project, author, ci_commit) } + subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) } it_behaves_like 'a system note' it "posts the Merge When Build Succeeds system note" do - expect(subject.note).to eq "Enabled an automatic merge when the build for 97de212e80737a608d939f648d959671fb0a0142 succeeds" + expect(subject.note).to match /Enabled an automatic merge when the build for (\w+\/\w+@)?[0-9a-f]{40} succeeds/ end end describe '.cancel_merge_when_build_succeeds' do - let(:ci_commit) { create :ci_commit_without_jobs } + let(:ci_commit) { build :ci_commit_without_jobs } let(:noteable) { create :merge_request } subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) } @@ -229,7 +229,7 @@ describe SystemNoteService do it_behaves_like 'a system note' it "posts the Merge When Build Succeeds system note" do - expect(subject.note).to eq "Canceled the automatic merge" + expect(subject.note).to eq "Cancelled the automatic merge" end end -- cgit v1.2.1 From 46278ec7ba7c618acaf7381ad466742ce84e33db Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Dec 2015 09:59:04 +0100 Subject: Button fix --- app/services/system_note_service.rb | 2 +- .../merge_requests/widget/open/_accept.html.haml | 33 +++++++++++++++++++--- .../merge_when_build_succeeds_spec.rb | 2 +- spec/models/merge_request_spec.rb | 2 +- .../merge_when_build_succeeds_service_spec.rb | 2 +- spec/services/system_note_service_spec.rb | 2 +- 6 files changed, 34 insertions(+), 9 deletions(-) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index f84e480ca9c..6d15a49145d 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -139,7 +139,7 @@ class SystemNoteService # Called when 'merge when build succeeds' is canceled def self.cancel_merge_when_build_succeeds(noteable, project, author) - body = "Cancelled the automatic merge" + body = "Canceled the automatic merge" 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 f7d872aa455..c2badf342db 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -5,10 +5,22 @@ .accept-merge-holder.clearfix.js-toggle-container .accept-action - if @merge_request.ci_commit && @merge_request.ci_commit.active? - = f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do - Merge When Build Succeeds - = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do - Accept Merge Request Now + %span.btn-group + = link_to "#", class: "btn btn-create merge_when_build_succeeds" do + Merge When Build Succeeds + %a.btn.btn-success.dropdown-toggle{ 'data-toggle' => 'dropdown' } + %span.caret + %span.sr-only + Select Merge Moment + %ul.dropdown-menu.dropdown-menu-right{ role: 'menu' } + %li + = link_to "#", class: "merge_when_build_succeeds" do + = icon('check fw') + Merge When Build Succeeds + %li + = link_to "#", class: "accept_merge_request" do + = icon('warning fw') + Accept Merge Request Now - else = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do Accept Merge Request @@ -26,6 +38,8 @@ text: @merge_request.merge_commit_message, rows: 14, hint: true + = hidden_field_tag :merge_when_build_succeeds, "" + :javascript $('.accept_merge_request').on('click', function() { $(this).html(" Merge in progress"); @@ -34,3 +48,14 @@ $('.accept-mr-form').on('ajax:send', function() { $(".accept-mr-form :input").disable(); }); + + $('a.accept_merge_request').on('click', function(e) { + e.preventDefault(); + $(this).closest("form").submit(); + }); + + $('a.merge_when_build_succeeds').on('click', function(e) { + e.preventDefault(); + $("#merge_when_build_succeeds").val("1"); + $(this).closest("form").submit(); + }); diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index 2e64e903d1e..b5f319f2040 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -68,7 +68,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do expect(page).to have_button "Merge When Build Succeeds" visit_merge_request(merge_request) # Needed to refresh the page - expect(page).to have_content "Cancelled the automatic merge" + expect(page).to have_content "Canceled the automatic merge" end it "allows the user to remove the source branch" do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 63e1cd1fb92..33acfa37fea 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -192,7 +192,7 @@ describe MergeRequest do end end - describe '#can_remove_source_branch' do + describe '#can_remove_source_branch?' do let(:user) { create(:user) } let(:user2) { create(:user) } diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb index a62d88cde86..188fda6211f 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -78,7 +78,7 @@ describe MergeRequests::MergeWhenBuildSucceedsService do it 'Posts a system note' do note = mr_merge_if_green_enabled.notes.last - expect(note.note).to include 'Cancelled the automatic merge' + expect(note.note).to include 'Canceled the automatic merge' end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 333035f2d2c..15173cee0a2 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -229,7 +229,7 @@ describe SystemNoteService do it_behaves_like 'a system note' it "posts the Merge When Build Succeeds system note" do - expect(subject.note).to eq "Cancelled the automatic merge" + expect(subject.note).to eq "Canceled the automatic merge" end end -- cgit v1.2.1 From cfc95b828724b0d7cb07254091e675d6d8093dba Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Dec 2015 10:14:37 +0100 Subject: Fix schema --- db/schema.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index fb59e187625..94b87040d88 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -476,9 +476,9 @@ ActiveRecord::Schema.define(version: 20151203162133) do add_index "merge_request_diffs", ["merge_request_id"], name: "index_merge_request_diffs_on_merge_request_id", unique: true, using: :btree create_table "merge_requests", force: :cascade do |t| - t.string "target_branch", null: false - t.string "source_branch", null: false - t.integer "source_project_id", null: false + t.string "target_branch", null: false + t.string "source_branch", null: false + t.integer "source_project_id", null: false t.integer "author_id" t.integer "assignee_id" t.string "title" @@ -487,13 +487,16 @@ ActiveRecord::Schema.define(version: 20151203162133) do t.integer "milestone_id" t.string "state" t.string "merge_status" - t.integer "target_project_id", null: false + t.integer "target_project_id", null: false t.integer "iid" t.text "description" - t.integer "position", default: 0 + t.integer "position", default: 0 t.datetime "locked_at" t.integer "updated_by_id" t.string "merge_error" + t.text "merge_params" + t.boolean "merge_when_build_succeeds", default: false, null: false + t.integer "merge_user_id" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree -- cgit v1.2.1 From 6eb75ed6ad2710126b577f8093e78faff64bae1b Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Dec 2015 11:29:23 +0100 Subject: Fix specs --- spec/features/merge_requests/merge_when_build_succeeds_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index b5f319f2040..3a8f0a344e8 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -23,12 +23,12 @@ feature 'Merge When Build Succeeds', feature: true, js: true do end it 'displays the Merge When Build Succeeds button' do - expect(page).to have_button "Merge When Build Succeeds" + expect(page).to have_link "Merge When Build Succeeds" end context "Merge When Build succeeds enabled" do before do - click_button "Merge When Build Succeeds" + click_link "Merge When Build Succeeds" end it 'activates Merge When Build Succeeds feature' do @@ -65,7 +65,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do it 'cancels the automatic merge' do click_link "Cancel Automatic Merge" - expect(page).to have_button "Merge When Build Succeeds" + expect(page).to have_link "Merge When Build Succeeds" visit_merge_request(merge_request) # Needed to refresh the page expect(page).to have_content "Canceled the automatic merge" @@ -82,7 +82,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do context 'Build is not active' do it "should not allow for enabling" do visit_merge_request(merge_request) - expect(page).not_to have_button "Merge When Build Succeeds" + expect(page).not_to have_link "Merge When Build Succeeds" end end -- cgit v1.2.1 From 96520227ae6fc5b02b0725bc515511960bcfefed Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Dec 2015 11:29:48 +0100 Subject: Prevent Firefox from remembering hidden field value --- app/views/projects/merge_requests/widget/open/_accept.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c2badf342db..0fe3af97b70 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -38,7 +38,7 @@ text: @merge_request.merge_commit_message, rows: 14, hint: true - = hidden_field_tag :merge_when_build_succeeds, "" + = hidden_field_tag :merge_when_build_succeeds, "", autocomplete: "off" :javascript $('.accept_merge_request').on('click', function() { -- cgit v1.2.1 From 4c8666e69fc64200bdd2f5069bbc8c9b22fe12ab Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Dec 2015 11:30:09 +0100 Subject: Fix commit message textarea position --- .../merge_requests/widget/open/_accept.html.haml | 63 +++++++++++----------- 1 file changed, 32 insertions(+), 31 deletions(-) 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 0fe3af97b70..6b4395fe4dc 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -3,37 +3,38 @@ = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f| = hidden_field_tag :authenticity_token, form_authenticity_token .accept-merge-holder.clearfix.js-toggle-container - .accept-action - - if @merge_request.ci_commit && @merge_request.ci_commit.active? - %span.btn-group - = link_to "#", class: "btn btn-create merge_when_build_succeeds" do - Merge When Build Succeeds - %a.btn.btn-success.dropdown-toggle{ 'data-toggle' => 'dropdown' } - %span.caret - %span.sr-only - Select Merge Moment - %ul.dropdown-menu.dropdown-menu-right{ role: 'menu' } - %li - = link_to "#", class: "merge_when_build_succeeds" do - = icon('check fw') - Merge When Build Succeeds - %li - = link_to "#", class: "accept_merge_request" do - = icon('warning fw') - Accept Merge Request Now - - else - = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do - Accept Merge Request - - if @merge_request.can_remove_source_branch?(current_user) - .accept-control.checkbox - = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do - = check_box_tag :should_remove_source_branch - Remove source branch - .accept-control.right - = link_to "#", class: "modify-merge-commit-link js-toggle-button" do - = icon('edit') - Modify commit message - .js-toggle-content.hide.prepend-top-20 + .clearfix + .accept-action + - if @merge_request.ci_commit && @merge_request.ci_commit.active? + %span.btn-group + = link_to "#", class: "btn btn-create merge_when_build_succeeds" do + Merge When Build Succeeds + %a.btn.btn-success.dropdown-toggle{ 'data-toggle' => 'dropdown' } + %span.caret + %span.sr-only + Select Merge Moment + %ul.dropdown-menu.dropdown-menu-right{ role: 'menu' } + %li + = link_to "#", class: "merge_when_build_succeeds" do + = icon('check fw') + Merge When Build Succeeds + %li + = link_to "#", class: "accept_merge_request" do + = icon('warning fw') + Merge Immediately + - else + = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do + Accept Merge Request + - if @merge_request.can_remove_source_branch?(current_user) + .accept-control.checkbox + = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do + = check_box_tag :should_remove_source_branch + Remove source branch + .accept-control.right + = link_to "#", class: "modify-merge-commit-link js-toggle-button" do + = icon('edit') + Modify commit message + .js-toggle-content.hide.prepend-top-default = render 'shared/commit_message_container', params: params, text: @merge_request.merge_commit_message, rows: 14, hint: true -- cgit v1.2.1 From e7969d6f6c743ead94b3b430b9184ad21f647337 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 7 Dec 2015 11:54:07 +0100 Subject: Satisfy Douwe Maan --- CHANGELOG | 2 +- app/services/system_note_service.rb | 2 +- .../merge_requests/merge_when_build_succeeds_spec.rb | 14 ++++++-------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 6c2fa75899c..163a0ea54e8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,7 +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) + - Merge when build succeeds (Zeger-Jan van de Weg) - Fix: Assignee selector is empty when 'Unassigned' is selected (Jose Corcuera) - Fix 500 error when update group member permission - Trim leading and trailing whitespace of milestone and issueable titles (Jose Corcuera) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 7cb9417b13e..6975b2ee55b 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -132,7 +132,7 @@ class SystemNoteService # Called when 'merge when build succeeds' is executed def self.merge_when_build_succeeds(noteable, project, author, last_commit) - body = "Enabled an automatic merge when the build for #{last_commit.to_reference} succeeds" + body = "Enabled an automatic merge when the build for #{last_commit.to_reference(project)} succeeds" create_note(noteable: noteable, project: project, author: author, note: body) end diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index 3a8f0a344e8..ee2fb25e9e5 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -1,6 +1,4 @@ require 'spec_helper' -# rubocop:disable Lint/UselessAssignment -# As rubocop doesn't see a need for both `ci_commit` and `ci_build` feature 'Merge When Build Succeeds', feature: true, js: true do let(:user) { create(:user) } @@ -14,10 +12,10 @@ feature 'Merge When Build Succeeds', feature: true, js: true do end context "Active build for Merge Request" do - before do - ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) - ci_build = create(:ci_build, commit: ci_commit) + let!(:ci_commit) { create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) } + let!(:ci_build) { create(:ci_build, commit: ci_commit) } + before do login_as user visit_merge_request(merge_request) end @@ -49,14 +47,15 @@ feature 'Merge When Build Succeeds', feature: true, js: true do merge_user: user, title: "MepMep", merge_when_build_succeeds: true) end + let!(:ci_commit) { create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) } + let!(:ci_build) { create(:ci_build, commit: ci_commit) } + before do merge_request.source_project.team << [user, :master] merge_request.source_branch = "feature" merge_request.target_branch = "master" merge_request.save! - ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) - ci_build = create(:ci_build, commit: ci_commit) login_as user visit_merge_request(merge_request) @@ -90,4 +89,3 @@ feature 'Merge When Build Succeeds', feature: true, js: true do visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) end end -# rubocop:enable Lint/UselessAssignment -- cgit v1.2.1 From b2b548de9d74b01816baca822d39f9dd543bbbf7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 8 Dec 2015 14:02:34 +0100 Subject: Rewrite docs --- doc/workflow/README.md | 1 + .../disable_merge_when_build_succeeds.png | Bin 20551 -> 0 bytes .../enable_merge_when_build_succeeds.png | Bin 13150 -> 0 bytes doc/workflow/merge_when_build_succeeds.md | 19 +++++++------------ doc/workflow/merge_when_build_succeeds/enable.png | Bin 0 -> 151112 bytes doc/workflow/merge_when_build_succeeds/status.png | Bin 0 -> 180318 bytes 6 files changed, 8 insertions(+), 12 deletions(-) delete mode 100644 doc/workflow/merge_requests/disable_merge_when_build_succeeds.png delete mode 100644 doc/workflow/merge_requests/enable_merge_when_build_succeeds.png create mode 100644 doc/workflow/merge_when_build_succeeds/enable.png create mode 100644 doc/workflow/merge_when_build_succeeds/status.png diff --git a/doc/workflow/README.md b/doc/workflow/README.md index a6b4d951188..d2642495c9a 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -17,4 +17,5 @@ - [Milestones](milestones.md) - [Merge Requests](merge_requests.md) - ["Work In Progress" Merge Requests](wip_merge_requests.md) +- [Merge When Build Succeeds](merge_when_build_succeeds.md) - [Manage large binaries with Git LFS](lfs/manage_large_binaries_with_git_lfs.md) diff --git a/doc/workflow/merge_requests/disable_merge_when_build_succeeds.png b/doc/workflow/merge_requests/disable_merge_when_build_succeeds.png deleted file mode 100644 index a45a4890b62..00000000000 Binary files a/doc/workflow/merge_requests/disable_merge_when_build_succeeds.png and /dev/null differ diff --git a/doc/workflow/merge_requests/enable_merge_when_build_succeeds.png b/doc/workflow/merge_requests/enable_merge_when_build_succeeds.png deleted file mode 100644 index 62a46c9508b..00000000000 Binary files a/doc/workflow/merge_requests/enable_merge_when_build_succeeds.png and /dev/null differ diff --git a/doc/workflow/merge_when_build_succeeds.md b/doc/workflow/merge_when_build_succeeds.md index 9bf6ddcc569..3c055650c49 100644 --- a/doc/workflow/merge_when_build_succeeds.md +++ b/doc/workflow/merge_when_build_succeeds.md @@ -1,20 +1,15 @@ # Merge When Build Succeeds -Select a Merge Request to be merged if the build succeeds so the user does not have to wait for the build to finish and revisit the Merge Request to merge it after the build is done. +When reviewing a merge request that looks ready to merge but still has one or more CI builds running, you can set it to be merged automatically when the build succeeds. This way, you don't have to wait for the build to finish and remember to merge the merge request then. -## Enabling for a Merge Request +![Enable](merge_when_build_succeeds/enable.png) -Given an active build for a Merge Request, thus pending or running, a `Merge When Build Succeeds` button will appear to any user which can merge it. Once clicked, it ensures this merge request is merged when the build is successful. -When clicking the button, the merge parameters are also saved to allow the merge user to edit the commit message and remove the source branch if he can remove that branch. +When you hit the "Merge When Build Succeeds" button, the status of the Merge Request will be updated to represent the impending merge. If you cannot wait for the build to succeed and want to build immediately, this option is available in the dropdown menu on the right of the main button. -When this feature is enabled, a message will appear to notify other users. Also a note is posted on the thread. +Both team developers and the author of the merge request have the option to cancel the automatic merge when they find a reason it shouldn't be merged after all. -![Enable Merge When Build Succceeds](merge_requests/enable_merge_when_build_succeeds.png) +![Status](merge_when_build_succeeds/status.png) -## Canceling +When the build succeeds, the merge request will automatically be merged. When the build fails, the author gets a chance to retry any failed builds, or to push new commits to fix the failure. -The automatic merge can be disabled by clicking the `Cancel Automatic Merge` button, or when a new commit is added to the Merge Request. In the former case a note is posted. In the latter case a user able to merge can enable the feature again. - -![Disable the automatic merge](merge_requests/disable_merge_when_build_succeeds.png) - -A failed build does not reset the automatic build so a build can be retried. +When the builds are retried and succeed on the second try, the merge request will automatically be merged after all. When the merge request is updated with new commits, the automatic merge is automatically canceled to allow the new changes to be reviewed. diff --git a/doc/workflow/merge_when_build_succeeds/enable.png b/doc/workflow/merge_when_build_succeeds/enable.png new file mode 100644 index 00000000000..633efa1246f Binary files /dev/null and b/doc/workflow/merge_when_build_succeeds/enable.png differ diff --git a/doc/workflow/merge_when_build_succeeds/status.png b/doc/workflow/merge_when_build_succeeds/status.png new file mode 100644 index 00000000000..c856c7d14dc Binary files /dev/null and b/doc/workflow/merge_when_build_succeeds/status.png differ -- cgit v1.2.1 From 1464a69c76cf492ad8b9674e24260e917dc7d2ef Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 8 Dec 2015 22:37:07 +0100 Subject: Tweak text of documentation --- .gitignore | 1 - doc/workflow/merge_when_build_succeeds.md | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 881b3fb81ac..f5b6427ca03 100644 --- a/.gitignore +++ b/.gitignore @@ -38,7 +38,6 @@ public/assets/ public/uploads.* public/uploads/ shared/artifacts/ -TODO rails_best_practices_output.html /tags tmp/ diff --git a/doc/workflow/merge_when_build_succeeds.md b/doc/workflow/merge_when_build_succeeds.md index 3c055650c49..75e1fdff2b2 100644 --- a/doc/workflow/merge_when_build_succeeds.md +++ b/doc/workflow/merge_when_build_succeeds.md @@ -1,12 +1,12 @@ # Merge When Build Succeeds -When reviewing a merge request that looks ready to merge but still has one or more CI builds running, you can set it to be merged automatically when the build succeeds. This way, you don't have to wait for the build to finish and remember to merge the merge request then. +When reviewing a merge request that looks ready to merge but still has one or more CI builds running, you can set it to be merged automatically when all builds succeed. This way, you don't have to wait for the builds to finish and remember to merge the request manually. ![Enable](merge_when_build_succeeds/enable.png) -When you hit the "Merge When Build Succeeds" button, the status of the Merge Request will be updated to represent the impending merge. If you cannot wait for the build to succeed and want to build immediately, this option is available in the dropdown menu on the right of the main button. +When you hit the "Merge When Build Succeeds" button, the status of the merge request will be updated to represent the impending merge. If you cannot wait for the build to succeed and want to merge immediately, this option is available in the dropdown menu on the right of the main button. -Both team developers and the author of the merge request have the option to cancel the automatic merge when they find a reason it shouldn't be merged after all. +Both team developers and the author of the merge request have the option to cancel the automatic merge if they find a reason why it shouldn't be merged after all. ![Status](merge_when_build_succeeds/status.png) -- cgit v1.2.1 From 28351806aa1f89f584d6905365fd34f5f0e8bbc7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 9 Dec 2015 09:59:19 +0100 Subject: Give merge request widget the vars it desires --- app/controllers/projects/merge_requests_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 3240c77e994..530f3d3dcb8 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -7,6 +7,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits, :builds] before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds] before_action :define_show_vars, only: [:show, :diffs, :commits, :builds] + before_action :define_widget_vars, only: [:merge, :cancel_merge_when_build_succeeds] before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds] # Allow read any merge_request @@ -301,6 +302,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController end end + def define_widget_vars + @ci_commit = @merge_request.ci_commit + end + def invalid_mr # Render special view for MR with removed source or target branch render 'invalid' -- cgit v1.2.1 From 903151141da36ed9b8c2333a9b66f5c611e4efb6 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 9 Dec 2015 09:59:39 +0100 Subject: Tweak specs --- spec/features/merge_requests/merge_when_build_succeeds_spec.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index ee2fb25e9e5..a674124aab7 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -43,20 +43,14 @@ feature 'Merge When Build Succeeds', feature: true, js: true do context 'When it is enabled' do let(:merge_request) do - create(:merge_request_with_diffs, source_project: project, author: user, - merge_user: user, title: "MepMep", merge_when_build_succeeds: true) + create(:merge_request_with_diffs, :simple, source_project: project, author: user, + merge_user: user, title: "MepMep", merge_when_build_succeeds: true) end let!(:ci_commit) { create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) } let!(:ci_build) { create(:ci_build, commit: ci_commit) } before do - merge_request.source_project.team << [user, :master] - merge_request.source_branch = "feature" - merge_request.target_branch = "master" - merge_request.save! - - login_as user visit_merge_request(merge_request) end -- cgit v1.2.1