Merge branch 'jramsay/configure-default-branch-deletion' into 'master'

Add project option for deleting source branch

Closes #18283

See merge request gitlab-org/gitlab!18408

app/controllers/projects_controller.rb

@@ -371,6 +371,7 @@ class ProjectsController < Projects::ApplicationController
       :path,
       :printing_merge_request_link_enabled,
       :public_builds,
+      :remove_source_branch_after_merge,
       :request_access_enabled,
       :runners_token,
       :tag_list,

app/graphql/types/project_type.rb

@@ -66,6 +66,7 @@ module Types
     field :request_access_enabled, GraphQL::BOOLEAN_TYPE, null: true # rubocop:disable Graphql/Descriptions
     field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true # rubocop:disable Graphql/Descriptions
     field :printing_merge_request_link_enabled, GraphQL::BOOLEAN_TYPE, null: true # rubocop:disable Graphql/Descriptions
+    field :remove_source_branch_after_merge, GraphQL::BOOLEAN_TYPE, null: true, description: 'Remove the source branch by default after merge'
 
     field :namespace, Types::NamespaceType, null: true # rubocop:disable Graphql/Descriptions
     field :group, Types::GroupType, null: true # rubocop:disable Graphql/Descriptions

app/models/project.rb

@@ -91,6 +91,7 @@ class Project < ApplicationRecord
   default_value_for :wiki_enabled, gitlab_config_features.wiki
   default_value_for :snippets_enabled, gitlab_config_features.snippets
   default_value_for :only_allow_merge_if_all_discussions_are_resolved, false
+  default_value_for :remove_source_branch_after_merge, true
 
   add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:projects_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
 

app/services/merge_requests/build_service.rb

@@ -10,13 +10,20 @@ module MergeRequests
       # TODO: this should handle all quick actions that don't have side effects
       # https://gitlab.com/gitlab-org/gitlab-foss/issues/53658
       merge_quick_actions_into_params!(merge_request, only: [:target_branch])
-      merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch)
 
       # Assign the projects first so we can use policies for `filter_params`
       merge_request.author = current_user
       merge_request.source_project = find_source_project
       merge_request.target_project = find_target_project
 
+      # Source project sets the default source branch removal setting
+      merge_request.merge_params['force_remove_source_branch'] =
+        if params.key?(:force_remove_source_branch)
+          params.delete(:force_remove_source_branch)
+        else
+          merge_request.source_project.remove_source_branch_after_merge?
+        end
+
       filter_params(merge_request)
 
       # merge_request.assign_attributes(...) below is a Rails

app/views/projects/_merge_request_merge_options_settings.html.haml

@@ -12,3 +12,9 @@
     = form.check_box :printing_merge_request_link_enabled, class: 'form-check-input'
     = form.label :printing_merge_request_link_enabled, class: 'form-check-label' do
       = s_('ProjectSettings|Show link to create/view merge request when pushing from the command line')
+  .form-check.mb-2
+    = form.check_box :remove_source_branch_after_merge, class: 'form-check-input'
+    = form.label :remove_source_branch_after_merge, class: 'form-check-label' do
+      = s_("ProjectSettings|Enable 'Delete source branch' option by default")
+      .descr.text-secondary
+        = s_('ProjectSettings|Existing merge requests and protected branches are not affected')

changelogs/unreleased/jramsay-configure-default-branch-deletion.yml

+---
+title: Add project option for deleting source branch
+merge_request: 18408
+author: Zsolt Kovari
+type: added

db/migrate/20191017094449_add_remove_source_branch_after_merge_to_projects.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRemoveSourceBranchAfterMergeToProjects < ActiveRecord::Migration[5.1]
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    add_column :projects, :remove_source_branch_after_merge, :boolean
+  end
+
+  def down
+    remove_column :projects, :remove_source_branch_after_merge
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_10_17_045817) do
+ActiveRecord::Schema.define(version: 2019_10_17_094449) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -3058,6 +3058,7 @@ ActiveRecord::Schema.define(version: 2019_10_17_045817) do
     t.integer "max_pages_size"
     t.integer "max_artifacts_size"
     t.string "pull_mirror_branch_prefix", limit: 50
+    t.boolean "remove_source_branch_after_merge"
     t.index "lower((name)::text)", name: "index_projects_on_lower_name"
     t.index ["archived", "pending_delete", "merge_requests_require_code_owner_approval"], name: "projects_requiring_code_owner_approval", where: "((pending_delete = false) AND (archived = false) AND (merge_requests_require_code_owner_approval = true))"
     t.index ["created_at"], name: "index_projects_on_created_at"

doc/api/graphql/reference/index.md

@@ -604,6 +604,7 @@ The API can be explored interactively using the [GraphiQL IDE](../index.md#graph
 | `requestAccessEnabled` | Boolean |  |
 | `onlyAllowMergeIfAllDiscussionsAreResolved` | Boolean |  |
 | `printingMergeRequestLinkEnabled` | Boolean |  |
+| `removeSourceBranchAfterMerge` | Boolean | Remove the source branch by default after merge |
 | `namespace` | Namespace |  |
 | `group` | Group |  |
 | `statistics` | ProjectStatistics |  |

doc/api/projects.md

@@ -148,6 +148,7 @@ When the user is authenticated and `simple` is not set this returns something li
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "statistics": {
@@ -232,6 +233,7 @@ When the user is authenticated and `simple` is not set this returns something li
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "statistics": {
@@ -357,6 +359,7 @@ GET /users/:user_id/projects
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "statistics": {
@@ -441,6 +444,7 @@ GET /users/:user_id/projects
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "statistics": {
@@ -550,6 +554,7 @@ Example response:
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "statistics": {
@@ -631,6 +636,7 @@ Example response:
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "statistics": {
@@ -757,6 +763,7 @@ GET /projects/:id
   "repository_storage": "default",
   "only_allow_merge_if_pipeline_succeeds": false,
   "only_allow_merge_if_all_discussions_are_resolved": false,
+  "remove_source_branch_after_merge": false,
   "printing_merge_requests_link_enabled": true,
   "request_access_enabled": false,
   "merge_method": "merge",
@@ -917,6 +924,7 @@ POST /projects
 | `only_allow_merge_if_pipeline_succeeds` | boolean | no | Set whether merge requests can only be merged with successful jobs |
 | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved |
 | `merge_method` | string | no | Set the [merge method](#project-merge-method) used |
+| `remove_source_branch_after_merge` | boolean | no | Enable `Delete source branch` option by default for all new merge requests |
 | `lfs_enabled` | boolean | no | Enable LFS |
 | `request_access_enabled` | boolean | no | Allow users to request member access |
 | `tag_list`    | array   | no       | The list of tags for a project; put array of tags, that should be finally assigned to a project |
@@ -978,6 +986,7 @@ POST /projects/user/:user_id
 | `only_allow_merge_if_pipeline_succeeds` | boolean | no | Set whether merge requests can only be merged with successful jobs |
 | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved |
 | `merge_method` | string | no | Set the [merge method](#project-merge-method) used |
+| `remove_source_branch_after_merge` | boolean | no | Enable `Delete source branch` option by default for all new merge requests |
 | `lfs_enabled` | boolean | no | Enable LFS |
 | `request_access_enabled` | boolean | no | Allow users to request member access |
 | `tag_list`    | array   | no       | The list of tags for a project; put array of tags, that should be finally assigned to a project |
@@ -1039,6 +1048,7 @@ PUT /projects/:id
 | `only_allow_merge_if_pipeline_succeeds` | boolean | no | Set whether merge requests can only be merged with successful jobs |
 | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved |
 | `merge_method` | string | no | Set the [merge method](#project-merge-method) used |
+| `remove_source_branch_after_merge` | boolean | no | Enable `Delete source branch` option by default for all new merge requests |
 | `lfs_enabled` | boolean | no | Enable LFS |
 | `request_access_enabled` | boolean | no | Allow users to request member access |
 | `tag_list`    | array   | no       | The list of tags for a project; put array of tags, that should be finally assigned to a project |
@@ -1165,6 +1175,7 @@ Example responses:
     "shared_with_groups": [],
     "only_allow_merge_if_pipeline_succeeds": false,
     "only_allow_merge_if_all_discussions_are_resolved": false,
+    "remove_source_branch_after_merge": false,
     "request_access_enabled": false,
     "merge_method": "merge",
     "_links": {
@@ -1252,6 +1263,7 @@ Example response:
   "shared_with_groups": [],
   "only_allow_merge_if_pipeline_succeeds": false,
   "only_allow_merge_if_all_discussions_are_resolved": false,
+  "remove_source_branch_after_merge": false,
   "request_access_enabled": false,
   "merge_method": "merge",
   "_links": {
@@ -1338,6 +1350,7 @@ Example response:
   "shared_with_groups": [],
   "only_allow_merge_if_pipeline_succeeds": false,
   "only_allow_merge_if_all_discussions_are_resolved": false,
+  "remove_source_branch_after_merge": false,
   "request_access_enabled": false,
   "merge_method": "merge",
   "_links": {
@@ -1511,6 +1524,7 @@ Example response:
   "shared_with_groups": [],
   "only_allow_merge_if_pipeline_succeeds": false,
   "only_allow_merge_if_all_discussions_are_resolved": false,
+  "remove_source_branch_after_merge": false,
   "request_access_enabled": false,
   "merge_method": "merge",
   "_links": {
@@ -1616,6 +1630,7 @@ Example response:
   "shared_with_groups": [],
   "only_allow_merge_if_pipeline_succeeds": false,
   "only_allow_merge_if_all_discussions_are_resolved": false,
+  "remove_source_branch_after_merge": false,
   "request_access_enabled": false,
   "merge_method": "merge",
   "_links": {

doc/user/project/merge_requests/index.md

@@ -94,7 +94,9 @@ You can [search and filter the results](../../search/index.md#issues-and-merge-r
 
 When creating a merge request, select the "Delete source branch when merge
 request accepted" option and the source branch will be deleted when the merge
-request is merged.
+request is merged. To make this option enabled by default for all new merge
+requests, enable it in the
+[project's settings](../settings/index.md#merge-request-settings).
 
 This option is also visible in an existing merge request next to the merge
 request button and can be selected/deselected before merging. It's only visible

doc/user/project/settings/index.md

@@ -49,10 +49,11 @@ Add an [issue description template](../description_templates.md#description-temp
 Set up your project's merge request settings:
 
 - Set up the merge request method (merge commit, [fast-forward merge](../merge_requests/fast_forward_merge.html)).
-- Merge request [description templates](../description_templates.md#description-templates).
+- Add merge request [description templates](../description_templates.md#description-templates).
 - Enable [merge request approvals](../merge_requests/merge_request_approvals.md). **(STARTER)**
-- Enable [merge only of pipeline succeeds](../merge_requests/merge_when_pipeline_succeeds.md).
-- Enable [merge only when all discussions are resolved](../../discussions/index.md#only-allow-merge-requests-to-be-merged-if-all-threads-are-resolved).
+- Enable [merge only if pipeline succeeds](../merge_requests/merge_when_pipeline_succeeds.md).
+- Enable [merge only when all threads are resolved](../../discussions/index.md#only-allow-merge-requests-to-be-merged-if-all-threads-are-resolved).
+- Enable [`delete source branch after merge` option by default](../merge_requests/index.md#deleting-the-source-branch)
 
 ![project's merge request settings](img/merge_requests_settings.png)
 

lib/api/entities.rb

@@ -307,6 +307,7 @@ module API
       expose :only_allow_merge_if_pipeline_succeeds
       expose :request_access_enabled
       expose :only_allow_merge_if_all_discussions_are_resolved
+      expose :remove_source_branch_after_merge
       expose :printing_merge_request_link_enabled
       expose :merge_method
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: -> (project, options) {

lib/api/helpers/projects_helpers.rb

@@ -30,6 +30,7 @@ module API
 
         optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project'
         optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push'
+        optional :remove_source_branch_after_merge, type: Boolean, desc: 'Remove the source branch by default after merge'
         optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project'
         optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project'
         optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.'
@@ -94,6 +95,7 @@ module API
           :path,
           :printing_merge_request_link_enabled,
           :public_builds,
+          :remove_source_branch_after_merge,
           :repository_access_level,
           :request_access_enabled,
           :resolve_outdated_diff_discussions,
@@ -109,7 +111,6 @@ module API
           :jobs_enabled,
           :merge_requests_enabled,
           :wiki_enabled,
-          :jobs_enabled,
           :snippets_enabled
         ]
       end

locale/gitlab.pot

@@ -12857,9 +12857,15 @@ msgstr ""
 msgid "ProjectSettings|Customize your project badges."
 msgstr ""
 
+msgid "ProjectSettings|Enable 'Delete source branch' option by default"
+msgstr ""
+
 msgid "ProjectSettings|Every merge creates a merge commit"
 msgstr ""
 
+msgid "ProjectSettings|Existing merge requests and protected branches are not affected"
+msgstr ""
+
 msgid "ProjectSettings|Failed to protect the tag"
 msgstr ""
 

spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb

@@ -107,4 +107,27 @@ describe 'Projects > Settings > User manages merge request settings' do
       expect(project.printing_merge_request_link_enabled).to be(false)
     end
   end
+
+  describe 'Checkbox to remove source branch after merge', :js do
+    it 'is initially checked' do
+      checkbox = find_field('project_remove_source_branch_after_merge')
+      expect(checkbox).to be_checked
+    end
+
+    it 'when unchecked sets :remove_source_branch_after_merge to false' do
+      uncheck('project_remove_source_branch_after_merge')
+      within('.merge-request-settings-form') do
+        find('.qa-save-merge-request-changes')
+        click_on('Save changes')
+      end
+
+      find('.flash-notice')
+      checkbox = find_field('project_remove_source_branch_after_merge')
+
+      expect(checkbox).not_to be_checked
+
+      project.reload
+      expect(project.remove_source_branch_after_merge).to be(false)
+    end
+  end
 end

spec/graphql/types/project_type_spec.rb

@@ -23,6 +23,7 @@ describe GitlabSchema.types['Project'] do
       only_allow_merge_if_all_discussions_are_resolved printing_merge_request_link_enabled
       namespace group statistics repository merge_requests merge_request issues
       issue pipelines
+      removeSourceBranchAfterMerge
     ]
 
     is_expected.to have_graphql_fields(*expected_fields)

spec/lib/gitlab/import_export/all_models.yml

@@ -421,6 +421,7 @@ project:
 - pages_metadatum
 - alerts_service
 - grafana_integration
+- remove_source_branch_after_merge
 award_emoji:
 - awardable
 - user

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -512,6 +512,7 @@ Project:
 - request_access_enabled
 - has_external_wiki
 - only_allow_merge_if_all_discussions_are_resolved
+- remove_source_branch_after_merge
 - auto_cancel_pending_pipelines
 - printing_merge_request_link_enabled
 - resolve_outdated_diff_discussions

spec/requests/api/projects_spec.rb

@@ -606,6 +606,7 @@ describe API::Projects do
         merge_requests_enabled: false,
         wiki_enabled: false,
         resolve_outdated_diff_discussions: false,
+        remove_source_branch_after_merge: true,
         only_allow_merge_if_pipeline_succeeds: false,
         request_access_enabled: true,
         only_allow_merge_if_all_discussions_are_resolved: false,
@@ -722,6 +723,22 @@ describe API::Projects do
       expect(json_response['resolve_outdated_diff_discussions']).to be_truthy
     end
 
+    it 'sets a project as not removing source branches' do
+      project = attributes_for(:project, remove_source_branch_after_merge: false)
+
+      post api('/projects', user), params: project
+
+      expect(json_response['remove_source_branch_after_merge']).to be_falsey
+    end
+
+    it 'sets a project as removing source branches' do
+      project = attributes_for(:project, remove_source_branch_after_merge: true)
+
+      post api('/projects', user), params: project
+
+      expect(json_response['remove_source_branch_after_merge']).to be_truthy
+    end
+
     it 'sets a project as allowing merge even if build fails' do
       project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false)
 
@@ -980,6 +997,22 @@ describe API::Projects do
       expect(json_response['resolve_outdated_diff_discussions']).to be_truthy
     end
 
+    it 'sets a project as not removing source branches' do
+      project = attributes_for(:project, remove_source_branch_after_merge: false)
+
+      post api("/projects/user/#{user.id}", admin), params: project
+
+      expect(json_response['remove_source_branch_after_merge']).to be_falsey
+    end
+
+    it 'sets a project as removing source branches' do
+      project = attributes_for(:project, remove_source_branch_after_merge: true)
+
+      post api("/projects/user/#{user.id}", admin), params: project
+
+      expect(json_response['remove_source_branch_after_merge']).to be_truthy
+    end
+
     it 'sets a project as allowing merge even if build fails' do
       project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false)
       post api("/projects/user/#{user.id}", admin), params: project
@@ -1157,6 +1190,7 @@ describe API::Projects do
         expect(json_response['wiki_access_level']).to be_present
         expect(json_response['builds_access_level']).to be_present
         expect(json_response['resolve_outdated_diff_discussions']).to eq(project.resolve_outdated_diff_discussions)
+        expect(json_response['remove_source_branch_after_merge']).to be_truthy
         expect(json_response['container_registry_enabled']).to be_present
         expect(json_response['created_at']).to be_present
         expect(json_response['last_activity_at']).to be_present

spec/services/merge_requests/build_service_spec.rb

@@ -80,7 +80,7 @@ describe MergeRequests::BuildService do
     end
 
     it 'does not assign force_remove_source_branch' do
-      expect(merge_request.force_remove_source_branch?).to be_falsey
+      expect(merge_request.force_remove_source_branch?).to be_truthy
     end
 
     context 'with force_remove_source_branch parameter' do
@@ -90,6 +90,36 @@ describe MergeRequests::BuildService do
       it 'assigns force_remove_source_branch' do
         expect(merge_request.force_remove_source_branch?).to be_truthy
       end
+
+      context 'with project setting remove_source_branch_after_merge false' do
+        before do
+          project.remove_source_branch_after_merge = false
+        end
+
+        it 'assigns force_remove_source_branch' do
+          expect(merge_request.force_remove_source_branch?).to be_truthy
+        end
+      end
+    end
+
+    context 'with project setting remove_source_branch_after_merge true' do
+      before do
+        project.remove_source_branch_after_merge = true
+      end
+
+      it 'assigns force_remove_source_branch' do
+        expect(merge_request.force_remove_source_branch?).to be_truthy
+      end
+
+      context 'with force_remove_source_branch parameter false' do
+        before do
+          params[:force_remove_source_branch] = '0'
+        end
+
+        it 'does not assign force_remove_source_branch' do
+          expect(merge_request.force_remove_source_branch?).to be(false)
+        end
+      end
     end
 
     context 'missing source branch' do