diff options
23 files changed, 217 insertions, 122 deletions
diff --git a/CHANGELOG b/CHANGELOG index 42d32e53685..a995c7010b9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -25,6 +25,7 @@ v 8.11.0 (unreleased) - Environments have an url to link to - Update `timeago` plugin to use multiple string/locale settings - Remove unused images (ClemMakesApps) + - Check remove source branch by default on a new MR - Limit git rev-list output count to one in forced push check - Clean up unused routes (Josef Strzibny) - Fix issue on empty project to allow developers to only push to protected branches if given permission diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2cf6a2dd1b3..ebb527d2e38 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -239,7 +239,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController TodoService.new.merge_merge_request(merge_request, current_user) - @merge_request.update(merge_error: nil) + @merge_request.update(merge_error: nil, remove_source_branch: !!params[:remove_source_branch]) if params[:merge_when_build_succeeds].present? unless @merge_request.pipeline @@ -434,13 +434,13 @@ class Projects::MergeRequestsController < Projects::ApplicationController params.require(:merge_request).permit( :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, - :state_event, :description, :task_num, :force_remove_source_branch, + :state_event, :description, :task_num, :remove_source_branch, label_ids: [] ) end def merge_params - params.permit(:should_remove_source_branch, :commit_message) + params.permit(:commit_message, :remove_source_branch) end # Make sure merge requests created before 8.0 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e845cfcf66a..90dcfccee5f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -387,19 +387,8 @@ class MergeRequest < ActiveRecord::Base !source_project.protected_branch?(source_branch) && !source_project.root_ref?(source_branch) && Ability.abilities.allowed?(current_user, :push_code, source_project) && - diff_head_commit == source_branch_head - end - - def should_remove_source_branch? - merge_params['should_remove_source_branch'].present? - end - - def force_remove_source_branch? - merge_params['force_remove_source_branch'].present? - end - - def remove_source_branch? - should_remove_source_branch? || force_remove_source_branch? + diff_head_commit == source_branch_head && + !same_source_branch_merge_requests? end def mr_and_commit_notes @@ -532,11 +521,7 @@ class MergeRequest < ActiveRecord::Base self.merge_when_build_succeeds = false self.merge_user = nil - if merge_params - merge_params.delete('should_remove_source_branch') - merge_params.delete('commit_message') - end - + self.merge_params = nil self.save end diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 96a25330af1..22168ae5b2c 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -9,13 +9,11 @@ module MergeRequests filter_params label_params = params.delete(:label_ids) - force_remove_source_branch = params.delete(:force_remove_source_branch) merge_request = MergeRequest.new(params) merge_request.source_project = source_project merge_request.target_project ||= source_project merge_request.author = current_user - merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch if merge_request.save merge_request.update_attributes(label_ids: label_params) diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index a308136f236..4b133f8f4d7 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -56,22 +56,14 @@ module MergeRequests def after_merge MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) - if remove_source_branch? + if merge_request.can_remove_source_branch?(branch_deletion_user) DeleteBranchService.new(@merge_request.source_project, branch_deletion_user). execute(merge_request.source_branch) end end def branch_deletion_user - @merge_request.force_remove_source_branch? ? @merge_request.author : current_user - end - - def remove_source_branch? - return false unless @merge_request.remove_source_branch? - - # If another MR in this project has the same source branch, we should not - # remove this branch - !@merge_request.same_source_branch_merge_requests? + @merge_request.remove_source_branch? ? @merge_request.author : current_user end end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 026a37997d4..477c64e7377 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -11,8 +11,6 @@ module MergeRequests params.except!(:target_project_id) params.except!(:source_branch) - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) - update(merge_request) end diff --git a/app/views/projects/merge_requests/merge.js.haml b/app/views/projects/merge_requests/merge.js.haml index 84b6c9ebc5c..0cf52fef16f 100644 --- a/app/views/projects/merge_requests/merge.js.haml +++ b/app/views/projects/merge_requests/merge.js.haml @@ -1,7 +1,7 @@ - case @status - when :success :plain - merge_request_widget.mergeInProgress(#{params[:should_remove_source_branch] == '1'}); + merge_request_widget.mergeInProgress(#{params[:remove_source_branch] == '1'}); - when :merge_when_build_succeeds :plain $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}"); 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 bf2e76f0083..d9f33059a7d 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -27,13 +27,13 @@ - else = f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request #{status_class}" do Accept Merge Request - - if @merge_request.force_remove_source_branch? + - if @merge_request.remove_source_branch? .accept-control The source branch will be removed. - elsif @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 + = f.label :remove_source_branch, class: "remove_source_checkbox" do + = f.check_box :remove_source_branch Remove source branch .accept-control.right = link_to "#", class: "modify-merge-commit-link js-toggle-button" 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 2b6b5e05e86..aa88a07a33d 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 @@ -16,7 +16,7 @@ - if remove_source_branch_button || user_can_cancel_automatic_merge .clearfix.prepend-top-10 - 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, sha: @merge_request.diff_head_sha), 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, remove_source_branch: true, sha: @merge_request.diff_head_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do = icon('times') Remove Source Branch When Merged diff --git a/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml b/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml index 57ce1959021..c4cca63a605 100644 --- a/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml +++ b/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml @@ -2,5 +2,5 @@ Ready to be merged automatically %p Ask someone with write access to this repository to merge this request. - - if @merge_request.force_remove_source_branch? + - if @merge_request.remove_source_branch? The source branch will be removed. diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index c30bdb0ae91..78f38bbcde5 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -124,8 +124,8 @@ .form-group .col-sm-10.col-sm-offset-2 .checkbox - = label_tag 'merge_request[force_remove_source_branch]' do - = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch? + = f.label :remove_source_branch do + = f.check_box :remove_source_branch Remove source branch when merge request is accepted. - is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?) diff --git a/db/migrate/20160720102606_add_remove_source_branch_to_merge_requests.rb b/db/migrate/20160720102606_add_remove_source_branch_to_merge_requests.rb new file mode 100644 index 00000000000..8abb7a7c0f1 --- /dev/null +++ b/db/migrate/20160720102606_add_remove_source_branch_to_merge_requests.rb @@ -0,0 +1,28 @@ +# online without errors +# This migration makes an actual field now there are more usecases for this field +# Migrating the data between the `merge_params` hash and this field will happen as +# part of this migration, but cleaning up the merge_params field will not. So some +# keys will be there but never used. +class AddRemoveSourceBranchToMergeRequests < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + add_column_with_default(:merge_requests, :remove_source_branch, :boolean, default: true) + + set_to_false = if Gitlab::Database.postgresql? + execute "SELECT id FROM merge_requests WHERE merge_params !~* 'remove_source_branch:.{0,2}1';" + else + execute "SELECT id FROM merge_requests WHERE merge_params NOT REGEXP 'remove_source_branch:.{0,2}1;'" + end + + set_to_false.each_slice(1000) do |ids| + MergeRequest.where(id: ids).update_all(remove_source_branch: false) + end + end + + def down + # noop - We don't want to serialize and deserialize, also, the keys are still there + end +end diff --git a/db/schema.rb b/db/schema.rb index 71980a6d51f..2a6e701b5bc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -632,6 +632,7 @@ ActiveRecord::Schema.define(version: 20160804150737) do t.string "merge_commit_sha" t.datetime "deleted_at" t.string "in_progress_merge_commit_sha" + t.boolean "remove_source_branch", default: true, null: false end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 3e88a758936..e2e9e230817 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -1,5 +1,7 @@ # Merge requests +Merge requests provide a way to contribute your work into the specified branch so your changes can be combined with others. + ## List merge requests Get all merge requests for this project. @@ -15,11 +17,17 @@ GET /projects/:id/merge_requests?iid=42 Parameters: -- `id` (required) - The ID of a project -- `iid` (optional) - Return the request having the given `iid` -- `state` (optional) - Return `all` requests or just those that are `merged`, `opened` or `closed` -- `order_by` (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` -- `sort` (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc` +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `iid` | integer | no | Return the MRs having the given `iid` | +| `state` | string | no | Return `all` MRs or just those that are `merged`, `opened` or `closed` | +| `order_by` | string | no | Return MRs ordered by `created_at` or `updated_at` fields. Default is `created_at` | +| `sort` | string | no | Return MRs sorted in `asc` or `desc` order. Default is `desc` | + +```bash +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests/ +``` ```json [ @@ -70,12 +78,12 @@ Parameters: "subscribed" : false, "user_notes_count": 1, "should_remove_source_branch": true, - "force_remove_source_branch": false + "force_remove_source_branch": true } ] ``` -## Get single MR +## Get a single Merge Request Shows information about a single merge request. @@ -85,8 +93,14 @@ GET /projects/:id/merge_requests/:merge_request_id Parameters: -- `id` (required) - The ID of a project -- `merge_request_id` (required) - The ID of MR +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `merge_request_id` | integer | no | Return the MR having the given `merge_request_id` | + +```bash +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests/1 +``` ```json { @@ -136,7 +150,7 @@ Parameters: "subscribed" : true, "user_notes_count": 1, "should_remove_source_branch": true, - "force_remove_source_branch": false + "force_remove_source_branch": true } ``` @@ -150,9 +164,14 @@ GET /projects/:id/merge_requests/:merge_request_id/commits Parameters: -- `id` (required) - The ID of a project -- `merge_request_id` (required) - The ID of MR +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `merge_request_id` | integer | no | Return the MR having the given `merge_request_id` | +```bash +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests/1/commits +``` ```json [ @@ -187,8 +206,14 @@ GET /projects/:id/merge_requests/:merge_request_id/changes Parameters: -- `id` (required) - The ID of a project -- `merge_request_id` (required) - The ID of MR +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `merge_request_id` | integer | no | Return the MR having the given `merge_request_id` | + +```bash +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/21/merge_requests/1/changes +``` ```json { @@ -238,7 +263,7 @@ Parameters: "subscribed" : true, "user_notes_count": 1, "should_remove_source_branch": true, - "force_remove_source_branch": false, + "force_remove_source_branch": true, "changes": [ { "old_path": "VERSION", @@ -257,21 +282,29 @@ Parameters: ## Create MR Creates a new merge request. + ``` POST /projects/:id/merge_requests ``` Parameters: -- `id` (required) - The ID of a project -- `source_branch` (required) - The source branch -- `target_branch` (required) - The target branch -- `assignee_id` (optional) - Assignee user ID -- `title` (required) - Title of MR -- `description` (optional) - Description of MR -- `target_project_id` (optional) - The target project (numeric id) -- `labels` (optional) - Labels for MR as a comma-separated list -- `milestone_id` (optional) - Milestone ID +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `source_branch` | string | yes | The source branch | +| `target_branch` | string | yes | The target branch | +| `assignee_id` | integer | no | Assignee user ID | +| `title` | string | yes | Title of the new MR | +| `description` | string | no | Description of MR | +| `target_project_id` | integer | no | The target project | +| `labels` | string | no | Labels for MR as a comma-separated list | +| `milestone_id` | string | no | Milestone ID | +| `remove_source_branch` | boolean | no | Remove the source branch when the MR is merged | + +```bash +curl -X POST -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests?source_branch=test1&target_branch=master&title=test1 +``` ```json { @@ -338,15 +371,22 @@ PUT /projects/:id/merge_requests/:merge_request_id Parameters: -- `id` (required) - The ID of a project -- `merge_request_id` (required) - ID of MR -- `target_branch` - The target branch -- `assignee_id` - Assignee user ID -- `title` - Title of MR -- `description` - Description of MR -- `state_event` - New state (close|reopen|merge) -- `labels` (optional) - Labels for MR as a comma-separated list -- `milestone_id` (optional) - Milestone ID +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `merge_request_id` | integer | yes | ID of MR | +| `target_branch` | string | no | The target branch | +| `assignee_id` | integer | no | Assignee user ID | +| `title` | string | no | Title of the new MR | +| `description` | string | no | Description of MR | +| `target_project_id` | integer | no | The target project | +| `labels` | string | no | Labels for MR as a comma-separated list | +| `milestone_id` | string | no | Milestone ID | +| `remove_source_branch` | boolean | no | Remove the source branch when the MR is merged | + +```bash +curl -X PUT -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests/1&title=test11 +``` ```json { @@ -354,7 +394,7 @@ Parameters: "iid": 1, "target_branch": "master", "project_id": 3, - "title": "test1", + "title": "test11", "state": "opened", "upvotes": 0, "downvotes": 0, @@ -395,7 +435,7 @@ Parameters: "subscribed" : true, "user_notes_count": 1, "should_remove_source_branch": true, - "force_remove_source_branch": false + "force_remove_source_branch": true } ``` @@ -441,12 +481,18 @@ PUT /projects/:id/merge_requests/: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 -- `should_remove_source_branch` (optional) - if `true` removes the source branch -- `merge_when_build_succeeds` (optional) - if `true` the MR is merged when the build succeeds -- `sha` (optional) - if present, then this SHA must match the HEAD of the source branch, otherwise the merge will fail +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `merge_request_id` | integer | yes | ID of MR | +| `merge_commit_message` | string | no | Custom merge commit message | +| `remove_source_branch` | boolean | no | if `true` removes the source branch | +| `merge_when_build_succeeds` | boolean | no | if `true` the MR is merged when the build succeeds | +| `sha` | string | no | if present, then this SHA must match the HEAD of the source branch, otherwise the merge will fail | + +```bash +curl -X POST -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests/1/merge +``` ```json { @@ -496,7 +542,7 @@ Parameters: "subscribed" : true, "user_notes_count": 1, "should_remove_source_branch": true, - "force_remove_source_branch": false + "force_remove_source_branch": true } ``` @@ -509,13 +555,22 @@ 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 406 error. + ``` PUT /projects/:id/merge_requests/:merge_request_id/cancel_merge_when_build_succeeds ``` + Parameters: -- `id` (required) - The ID of a project -- `merge_request_id` (required) - ID of MR +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer | yes | The ID of a project | +| `merge_request_id` | integer | yes | ID of MR | + + +```bash +curl -X PUT -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" http://gitlab.example.com/api/v3/projects/1/merge_requests/1/cancel_merge_when_build_succeeds +``` ```json { @@ -565,7 +620,7 @@ Parameters: "subscribed" : true, "user_notes_count": 1, "should_remove_source_branch": true, - "force_remove_source_branch": false + "force_remove_source_branch": true } ``` @@ -886,7 +941,7 @@ Example response: "subscribed": true, "user_notes_count": 7, "should_remove_source_branch": true, - "force_remove_source_branch": false + "force_remove_source_branch": true }, "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/merge_requests/7", "body": "Et voluptas laudantium minus nihil recusandae ut accusamus earum aut non.", diff --git a/features/steps/project/merge_requests/acceptance.rb b/features/steps/project/merge_requests/acceptance.rb index 4fda0731e2f..d3f7411f698 100644 --- a/features/steps/project/merge_requests/acceptance.rb +++ b/features/steps/project/merge_requests/acceptance.rb @@ -30,7 +30,7 @@ class Spinach::Features::ProjectMergeRequestsAcceptance < Spinach::FeatureSteps @user = create(:user) @project = create(:project, :public) @project_member = create(:project_member, :developer, user: @user, project: @project) - @merge_request = create(:merge_request, :with_diffs, :simple, source_project: @project) + @merge_request = create(:merge_request, :with_diffs, :simple, source_project: @project, remove_source_branch: false) end step 'I am signed in as a developer of the project' do diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e5b00dc45a5..f8bb7d215ee 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -218,8 +218,8 @@ module API merge_request.subscribed?(options[:current_user]) end expose :user_notes_count - expose :should_remove_source_branch?, as: :should_remove_source_branch - expose :force_remove_source_branch?, as: :force_remove_source_branch + expose(:should_remove_source_branch?) { |mr| mr.remove_source_branch } + expose(:force_remove_source_branch?) { |mr| mr.remove_source_branch } end class MergeRequestChanges < MergeRequest diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 2b685621da9..02a0bc20354 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -18,6 +18,19 @@ module API render_api_error!(errors, 400) end + + def remove_source_branch(merge_request) + # We only need the update the value if it differs from what we've got + # the to_boolean calls could return `nil`, which can't be inserted into + # to the `remove_source_branch` field as the column is restrained + new_value = to_boolean(params[:should_remove_source_branch]) || + to_boolean(params[:force_remove_source_branch]) || + to_boolean(params[:remove_source_branch]) + + if new_value != merge_request.remove_source_branch + merge_request.remove_source_branch = !merge_request.remove_source_branch + end + end end # List merge requests @@ -63,15 +76,16 @@ module API # # Parameters: # - # id (required) - The ID of a project - this will be the source of the merge request - # source_branch (required) - The source branch - # target_branch (required) - The target branch - # target_project_id - The target project of the merge request defaults to the :id of the project - # assignee_id - Assignee user ID - # title (required) - Title of MR - # description - Description of MR - # labels (optional) - Labels for MR as a comma-separated list - # milestone_id (optional) - Milestone ID + # id (required) - The ID of a project - this will be the source of the merge request + # source_branch (required) - The source branch + # target_branch (required) - The target branch + # target_project_id - The target project of the merge request defaults to the :id of the project + # assignee_id - Assignee user ID + # title (required) - Title of MR + # description(optional) - Description of MR + # labels (optional) - Labels for MR as a comma-separated list + # milestone_id (optional) - Milestone ID + # remove_source_branch (optional) - Should the source branch be deleted if the MR is merged? # # Example: # POST /projects/:id/merge_requests @@ -79,7 +93,7 @@ module API post ":id/merge_requests" do authorize! :create_merge_request, user_project required_attributes! [:source_branch, :target_branch, :title] - attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description, :milestone_id] + attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description, :milestone_id, :remove_source_branch] # Validate label names in advance if (errors = validate_label_params(params)).any? @@ -177,11 +191,13 @@ module API # description - Description of MR # labels (optional) - Labels for a MR as a comma-separated list # milestone_id (optional) - Milestone ID + # remove_source_branch (optional) - Should the source branch be deleted if the MR is merged? + # # Example: # PUT /projects/:id/merge_requests/:merge_request_id # put path do - attrs = attributes_for_keys [:target_branch, :assignee_id, :title, :state_event, :description, :milestone_id] + attrs = attributes_for_keys [:target_branch, :assignee_id, :title, :state_event, :description, :milestone_id, :remove_source_branch] merge_request = user_project.merge_requests.find(params[:merge_request_id]) authorize! :update_merge_request, merge_request @@ -216,7 +232,7 @@ module API # 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 + # 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 # sha (optional) - When present, must have the HEAD SHA of the source branch # Example: @@ -237,10 +253,8 @@ module API render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409) end - merge_params = { - commit_message: params[:merge_commit_message], - should_remove_source_branch: params[:should_remove_source_branch] - } + merge_params = { commit_message: params[:merge_commit_message] } + remove_source_branch(merge_request) if to_boolean(params[:merge_when_build_succeeds]) && merge_request.pipeline && merge_request.pipeline.active? ::MergeRequests::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params). 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 60bc07bd1a0..36c7d79a39d 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -4,7 +4,7 @@ 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") } + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user) } before do project.team << [user, :master] @@ -42,15 +42,16 @@ feature 'Merge When Build Succeeds', feature: true, js: true do end context 'When it is enabled' do - let(:merge_request) do - create(:merge_request_with_diffs, :simple, source_project: project, author: user, - merge_user: user, title: "MepMep", merge_when_build_succeeds: true) - end - let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) } let!(:ci_build) { create(:ci_build, pipeline: pipeline) } before do + # Do not create a new MR here in this scope, this will yield a second MR with the same source branch + merge_request.remove_source_branch = false + merge_request.merge_when_build_succeeds = true + merge_request.merge_user = user + merge_request.save + login_as user visit_merge_request(merge_request) end @@ -68,6 +69,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true 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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3270b877c1a..1f566be7059 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -331,7 +331,7 @@ describe MergeRequest, models: true do describe "#reset_merge_when_build_succeeds" do let(:merge_if_green) do create :merge_request, merge_when_build_succeeds: true, merge_user: create(:user), - merge_params: { "should_remove_source_branch" => "1", "commit_message" => "msg" } + remove_source_branch: true, merge_params: { "commit_message" => "msg" } end it "sets the item to false" do @@ -339,7 +339,6 @@ describe MergeRequest, models: true do merge_if_green.reload expect(merge_if_green.merge_when_build_succeeds).to be_falsey - expect(merge_if_green.merge_params["should_remove_source_branch"]).to be_nil expect(merge_if_green.merge_params["commit_message"]).to be_nil end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 617600d6173..e8ee3febec6 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -216,11 +216,14 @@ describe API::API, api: true do target_branch: 'master', author: user, labels: 'label, label2', - milestone_id: milestone.id + milestone_id: milestone.id, + remove_source_branch: false + expect(response).to have_http_status(201) expect(json_response['title']).to eq('Test merge_request') expect(json_response['labels']).to eq(['label', 'label2']) expect(json_response['milestone']['id']).to eq(milestone.id) + expect(json_response['should_remove_source_branch?']).to be false end it "returns 422 when source_branch equals target_branch" do @@ -470,10 +473,25 @@ describe API::API, api: true do describe "PUT /projects/:id/merge_requests/:merge_request_id" do it "updates title and returns merge_request" do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), title: "New title" + expect(response).to have_http_status(200) expect(json_response['title']).to eq('New title') end + it "updates the state of the merge_request" do + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), state_event: "close" + + expect(response).to have_http_status(200) + expect(json_response['state']).to eq('closed') + end + + it 'updates the remove source branch setting' do + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), remove_source_branch: false + + expect(response).to have_http_status(200) + expect(json_response['should_remove_source_branch?']).to be false + end + it "updates description and returns merge_request" do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), description: "New description" expect(response).to have_http_status(200) @@ -482,18 +500,21 @@ describe API::API, api: true do it "updates milestone_id and returns merge_request" do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), milestone_id: milestone.id + expect(response).to have_http_status(200) expect(json_response['milestone']['id']).to eq(milestone.id) end it "returns 400 when source_branch is specified" do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), + source_branch: "master", target_branch: "master" expect(response).to have_http_status(400) end it "returns merge_request with renamed target_branch" do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), target_branch: "wiki" + expect(response).to have_http_status(200) expect(json_response['target_branch']).to eq('wiki') end @@ -503,6 +524,7 @@ describe API::API, api: true do user), title: 'new issue', labels: 'label, label?, label&foo, ?, &' + expect(response.status).to eq(200) expect(json_response['labels']).to include 'label' expect(json_response['labels']).to include 'label?' diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index b84a580967a..03d580323dd 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -13,7 +13,7 @@ describe MergeRequests::CreateService, services: true do description: 'please fix', source_branch: 'feature', target_branch: 'master', - force_remove_source_branch: '1' + remove_source_branch: true } end @@ -30,7 +30,7 @@ describe MergeRequests::CreateService, services: true do it { expect(@merge_request).to be_valid } it { expect(@merge_request.title).to eq('Awesome merge_request') } it { expect(@merge_request.assignee).to be_nil } - it { expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') } + it { expect(@merge_request.remove_source_branch).to be true } it 'executes hooks with default action' do expect(service).to have_received(:execute_hooks).with(@merge_request) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index df9d96c9493..1f8a8c7cb3d 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -56,7 +56,7 @@ describe MergeRequests::MergeService, services: true do context 'when another merge request has the same source branch' do it 'removes the source branch' do create(:merge_request, source_project: merge_request.source_project, - target_project: merge_request.target_project, target_branch: 'mepmep') + target_project: merge_request.target_project, target_branch: 'mepmep') expect(DeleteBranchService).not_to receive(:new) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 283a336afd9..ed67adbe7a7 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -40,7 +40,7 @@ describe MergeRequests::UpdateService, services: true do state_event: 'close', label_ids: [label.id], target_branch: 'target', - force_remove_source_branch: '1' + remove_source_branch: true } end @@ -62,7 +62,7 @@ describe MergeRequests::UpdateService, services: true do it { expect(@merge_request.labels.count).to eq(1) } it { expect(@merge_request.labels.first.title).to eq(label.name) } it { expect(@merge_request.target_branch).to eq('target') } - it { expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') } + it { expect(@merge_request.remove_source_branch).to be_truthy } it 'executes hooks with update action' do expect(service).to have_received(:execute_hooks). |