diff options
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 4 | ||||
-rw-r--r-- | app/models/commit_status.rb | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 6 | ||||
-rw-r--r-- | app/services/merge_requests/merge_service.rb | 16 | ||||
-rw-r--r-- | app/services/merge_requests/merge_when_build_succeeds_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 4 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_accept.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml | 8 | ||||
-rw-r--r-- | app/workers/merge_worker.rb | 13 | ||||
-rw-r--r-- | config/routes.rb | 3 | ||||
-rw-r--r-- | doc/api/merge_requests.md | 60 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 68 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 14 | ||||
-rw-r--r-- | 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("<i class='fa fa-spinner fa-spin'></i> 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') } |