Add project option for deleting source branch

Feature branches should be deleted after being merged into a stable
branch like master. This is not universally true, and accidentally
deleting a branch cause frustration and confusion for users.

The change:
- adds a Project Setting to control if 'Delete source branch' option is
enabled or disabled for new merge requests
- the Project Setting is on for new and existing projects

The previous default behaviour can be restored by toggling the new
setting.

Historically, the behaviour was changed to delete the source branch as
part of the merge widget refactor and then reverted 3 months later.

- Widget refactor 8db76243
- Behaviour reverted fd72b4ed

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