Merge branch...

Merge branch '32447-remove-source-branch-is-displayed-to-mergers-who-cannot-actually-delete-the-source-branch-from-a-forked-project' into 'master'

Disable "Remove source branch" in MR Widget for users who can't remove, and re-add checkbox to MR form

Closes #32447 and #32907

See merge request !11558

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_ready_to_merge.js

@@ -13,7 +13,7 @@ export default {
   },
   data() {
     return {
-      removeSourceBranch: true,
+      removeSourceBranch: this.mr.shouldRemoveSourceBranch,
       mergeWhenBuildSucceeds: false,
       useCommitMessageWithDescription: false,
       setToMergeWhenPipelineSucceeds: false,
@@ -69,6 +69,9 @@ export default {
         || this.isMakingRequest
         || this.mr.preventMerge);
     },
+    isRemoveSourceBranchButtonDisabled() {
+      return this.isMergeButtonDisabled || !this.mr.canRemoveSourceBranch;
+    },
     shouldShowSquashBeforeMerge() {
       const { commitsCount, enableSquashBeforeMerge } = this.mr;
       return enableSquashBeforeMerge && commitsCount > 1;
@@ -252,8 +255,9 @@ export default {
       <template v-if="isMergeAllowed()">
         <label class="spacing">
           <input
+            id="remove-source-branch-input"
             v-model="removeSourceBranch"
-            :disabled="isMergeButtonDisabled"
+            :disabled="isRemoveSourceBranchButtonDisabled"
             type="checkbox"/> Remove source branch
         </label>
 

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -50,7 +50,7 @@ export default class MergeRequestStore {
     this.cancelAutoMergePath = data.cancel_merge_when_pipeline_succeeds_path;
     this.removeWIPPath = data.remove_wip_path;
     this.sourceBranchRemoved = !data.source_branch_exists;
-    this.shouldRemoveSourceBranch = (data.merge_params || {}).should_remove_source_branch || false;
+    this.shouldRemoveSourceBranch = data.remove_source_branch || false;
     this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false;
     this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false;
     this.mergePath = data.merge_path;

app/serializers/merge_request_entity.rb

@@ -39,6 +39,7 @@ class MergeRequestEntity < IssuableEntity
   expose :commits_count
   expose :cannot_be_merged?, as: :has_conflicts
   expose :can_be_merged?, as: :can_be_merged
+  expose :remove_source_branch?, as: :remove_source_branch
 
   expose :project_archived do |merge_request|
     merge_request.project.archived?

app/views/shared/issuable/form/_merge_params.html.haml

@@ -5,3 +5,13 @@
 
 -# This check is duplicated below, to avoid conflicts with EE.
 - return unless issuable.can_remove_source_branch?(current_user)
+
+.form-group
+  .col-sm-10.col-sm-offset-2
+    - if issuable.can_remove_source_branch?(current_user)
+      .checkbox
+        - initial_checkbox_value = issuable.merge_params.key?('force_remove_source_branch') ? issuable.force_remove_source_branch? : true
+        = label_tag 'merge_request[force_remove_source_branch]' do
+          = hidden_field_tag 'merge_request[force_remove_source_branch]', '0', id: nil
+          = check_box_tag 'merge_request[force_remove_source_branch]', '1', initial_checkbox_value
+          Remove source branch when merge request is accepted.

features/project/merge_requests/accept.feature

@@ -7,6 +7,7 @@ Feature: Project Merge Requests Acceptance
   @javascript
   Scenario: Accepting the Merge Request and removing the source branch
     Given I am on the Merge Request detail page
+    When I check the "Remove source branch" option
     And I click on Accept Merge Request
     Then I should see merge request merged
     And I should not see the Remove Source Branch button
@@ -14,6 +15,7 @@ Feature: Project Merge Requests Acceptance
   @javascript
   Scenario: Accepting the Merge Request when URL has an anchor
     Given I am on the Merge Request detail with note anchor page
+    When I check the "Remove source branch" option
     And I click on Accept Merge Request
     Then I should see merge request merged
     And I should not see the Remove Source Branch button
@@ -21,7 +23,6 @@ Feature: Project Merge Requests Acceptance
   @javascript
   Scenario: Accepting the Merge Request without removing the source branch
     Given I am on the Merge Request detail page
-    When I click on "Remove source branch" option
     When I click on Accept Merge Request
     Then I should see merge request merged
     And I should see the Remove Source Branch button

features/steps/project/merge_requests/acceptance.rb

@@ -11,10 +11,14 @@ class Spinach::Features::ProjectMergeRequestsAcceptance < Spinach::FeatureSteps
     visit merge_request_path(@merge_request, anchor: 'note_123')
   end
 
-  step 'I click on "Remove source branch" option' do
+  step 'I uncheck the "Remove source branch" option' do
     uncheck('Remove source branch')
   end
 
+  step 'I check the "Remove source branch" option' do
+    check('Remove source branch')
+  end
+
   step 'I click on Accept Merge Request' do
     click_button('Merge')
   end

spec/features/merge_requests/edit_mr_spec.rb

@@ -29,6 +29,19 @@ feature 'Edit Merge Request', feature: true do
       expect(page).to have_content 'Someone edited the merge request the same time you did'
     end
 
+    it 'allows to unselect "Remove source branch"', js: true do
+      merge_request.update(merge_params: { 'force_remove_source_branch' => '1' })
+      expect(merge_request.merge_params['force_remove_source_branch']).to be_truthy
+
+      visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
+      uncheck 'Remove source branch when merge request is accepted'
+
+      click_button 'Save changes'
+
+      expect(page).to have_unchecked_field 'remove-source-branch-input'
+      expect(page).to have_content 'Remove source branch'
+    end
+
     it 'should preserve description textarea height', js: true do
       long_description = %q(
         Lorem ipsum dolor sit amet, consectetur adipiscing elit. Etiam ac ornare ligula, ut tempus arcu. Etiam ultricies accumsan dolor vitae faucibus. Donec at elit lacus. Mauris orci ante, aliquam quis lorem eget, convallis faucibus arcu. Aenean at pulvinar lacus. Ut viverra quam massa, molestie ornare tortor dignissim a. Suspendisse tristique pellentesque tellus, id lacinia metus elementum id. Nam tristique, arcu rhoncus faucibus viverra, lacus ipsum sagittis ligula, vitae convallis odio lacus a nibh. Ut tincidunt est purus, ac vestibulum augue maximus in. Suspendisse vel erat et mi ultricies semper. Pellentesque volutpat pellentesque consequat.

spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb

@@ -7,7 +7,8 @@ feature 'Merge When Pipeline Succeeds', :feature, :js do
   let(:merge_request) do
     create(:merge_request_with_diffs, source_project: project,
                                       author: user,
-                                      title: 'Bug NS-04')
+                                      title: 'Bug NS-04',
+                                      merge_params: { force_remove_source_branch: '1' })
   end
 
   let(:pipeline) do
@@ -41,7 +42,7 @@ feature 'Merge When Pipeline Succeeds', :feature, :js do
           click_button "Merge when pipeline succeeds"
 
           expect(page).to have_content "Set by #{user.name} to be merged automatically when the pipeline succeeds."
-          expect(page).to have_content "The source branch will be removed."
+          expect(page).to have_content "The source branch will not be removed."
           expect(page).to have_selector ".js-cancel-auto-merge"
           visit_merge_request(merge_request) # Needed to refresh the page
           expect(page).to have_content /enabled an automatic merge when the pipeline for \h{8} succeeds/i
@@ -82,7 +83,8 @@ feature 'Merge When Pipeline Succeeds', :feature, :js do
                    source_project: project,
                    title: 'Bug NS-04',
                    author: user,
-                   merge_user: user)
+                   merge_user: user,
+                   merge_params: { force_remove_source_branch: '1' })
         end
 
         before do
@@ -99,7 +101,7 @@ feature 'Merge When Pipeline Succeeds', :feature, :js do
         click_link 'Merge when pipeline succeeds'
 
         expect(page).to have_content "Set by #{user.name} to be merged automatically when the pipeline succeeds."
-        expect(page).to have_content "The source branch will be removed."
+        expect(page).to have_content "The source branch will not be removed."
         expect(page).to have_link "Cancel automatic merge"
       end
     end

spec/features/merge_requests/widget_spec.rb

@@ -202,4 +202,25 @@ describe 'Merge request', :feature, :js do
       end
     end
   end
+
+  context 'user can merge into source project but cannot push to fork', js: true do
+    let(:fork_project) { create(:project, :public) }
+    let(:user2) { create(:user) }
+
+    before do
+      project.team << [user2, :master]
+      logout
+      login_as user2
+      merge_request.update(target_project: fork_project)
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    it 'user can merge into the source project' do
+      expect(page).to have_button('Merge', disabled: false)
+    end
+
+    it 'user cannot remove source branch' do
+      expect(page).to have_field('remove-source-branch-input', disabled: true)
+    end
+  end
 end

spec/fixtures/api/schemas/entities/merge_request.json

@@ -92,7 +92,8 @@
     "diverged_commits_count": { "type": "integer" },
     "commit_change_content_path": { "type": "string" },
     "remove_wip_path": { "type": "string" },
-    "commits_count": { "type": "integer" }
+    "commits_count": { "type": "integer" },
+    "remove_source_branch": { "type": ["boolean", "null"] }
   },
   "additionalProperties": false
 }

spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js

@@ -5,7 +5,7 @@ import * as simplePoll from '~/lib/utils/simple_poll';
 
 const commitMessage = 'This is the commit message';
 const commitMessageWithDescription = 'This is the commit message description';
-const createComponent = () => {
+const createComponent = (customConfig = {}) => {
   const Component = Vue.extend(readyToMergeComponent);
   const mr = {
     isPipelineActive: false,
@@ -17,8 +17,12 @@ const createComponent = () => {
     sha: '12345678',
     commitMessage,
     commitMessageWithDescription,
+    shouldRemoveSourceBranch: true,
+    canRemoveSourceBranch: false,
   };
 
+  Object.assign(mr, customConfig.mr);
+
   const service = {
     merge() {},
     poll() {},
@@ -51,7 +55,6 @@ describe('MRWidgetReadyToMerge', () => {
 
   describe('data', () => {
     it('should have default data', () => {
-      expect(vm.removeSourceBranch).toBeTruthy(true);
       expect(vm.mergeWhenBuildSucceeds).toBeFalsy();
       expect(vm.useCommitMessageWithDescription).toBeFalsy();
       expect(vm.setToMergeWhenPipelineSucceeds).toBeFalsy();
@@ -166,6 +169,36 @@ describe('MRWidgetReadyToMerge', () => {
         expect(vm.isMergeButtonDisabled).toBeTruthy();
       });
     });
+
+    describe('Remove source branch checkbox', () => {
+      describe('when user can merge but cannot delete branch', () => {
+        it('isRemoveSourceBranchButtonDisabled should be true', () => {
+          expect(vm.isRemoveSourceBranchButtonDisabled).toBe(true);
+        });
+
+        it('should be disabled in the rendered output', () => {
+          const checkboxElement = vm.$el.querySelector('#remove-source-branch-input');
+          expect(checkboxElement.getAttribute('disabled')).toBe('disabled');
+        });
+      });
+
+      describe('when user can merge and can delete branch', () => {
+        beforeEach(() => {
+          this.customVm = createComponent({
+            mr: { canRemoveSourceBranch: true },
+          });
+        });
+
+        it('isRemoveSourceBranchButtonDisabled should be false', () => {
+          expect(this.customVm.isRemoveSourceBranchButtonDisabled).toBe(false);
+        });
+
+        it('should be enabled in rendered output', () => {
+          const checkboxElement = this.customVm.$el.querySelector('#remove-source-branch-input');
+          expect(checkboxElement.getAttribute('disabled')).toBeNull();
+        });
+      });
+    });
   });
 
   describe('methods', () => {