Commit a25fb529 authored by Bryce Johnson's avatar Bryce Johnson

Upgrade Remove Source Branch checkbox UX.

parent 18a6d9c5
...@@ -13,7 +13,7 @@ export default { ...@@ -13,7 +13,7 @@ export default {
}, },
data() { data() {
return { return {
removeSourceBranch: true, removeSourceBranch: this.mr.shouldRemoveSourceBranch,
mergeWhenBuildSucceeds: false, mergeWhenBuildSucceeds: false,
useCommitMessageWithDescription: false, useCommitMessageWithDescription: false,
setToMergeWhenPipelineSucceeds: false, setToMergeWhenPipelineSucceeds: false,
...@@ -69,6 +69,9 @@ export default { ...@@ -69,6 +69,9 @@ export default {
|| this.isMakingRequest || this.isMakingRequest
|| this.mr.preventMerge); || this.mr.preventMerge);
}, },
isRemoveSourceBranchButtonDisabled() {
return this.isMergeButtonDisabled || !this.mr.canRemoveSourceBranch;
},
shouldShowSquashBeforeMerge() { shouldShowSquashBeforeMerge() {
const { commitsCount, enableSquashBeforeMerge } = this.mr; const { commitsCount, enableSquashBeforeMerge } = this.mr;
return enableSquashBeforeMerge && commitsCount > 1; return enableSquashBeforeMerge && commitsCount > 1;
...@@ -252,8 +255,9 @@ export default { ...@@ -252,8 +255,9 @@ export default {
<template v-if="isMergeAllowed()"> <template v-if="isMergeAllowed()">
<label class="spacing"> <label class="spacing">
<input <input
id="remove-source-branch-input"
v-model="removeSourceBranch" v-model="removeSourceBranch"
:disabled="isMergeButtonDisabled" :disabled="isRemoveSourceBranchButtonDisabled"
type="checkbox"/> Remove source branch type="checkbox"/> Remove source branch
</label> </label>
......
...@@ -11,6 +11,7 @@ export default class MergeRequestStore { ...@@ -11,6 +11,7 @@ export default class MergeRequestStore {
setData(data) { setData(data) {
const currentUser = data.current_user; const currentUser = data.current_user;
const pipelineStatus = data.pipeline ? data.pipeline.details.status : null; const pipelineStatus = data.pipeline ? data.pipeline.details.status : null;
const mergeParams = data.merge_params || {};
this.title = data.title; this.title = data.title;
this.targetBranch = data.target_branch; this.targetBranch = data.target_branch;
...@@ -51,7 +52,8 @@ export default class MergeRequestStore { ...@@ -51,7 +52,8 @@ export default class MergeRequestStore {
this.cancelAutoMergePath = data.cancel_merge_when_pipeline_succeeds_path; this.cancelAutoMergePath = data.cancel_merge_when_pipeline_succeeds_path;
this.removeWIPPath = data.remove_wip_path; this.removeWIPPath = data.remove_wip_path;
this.sourceBranchRemoved = !data.source_branch_exists; this.sourceBranchRemoved = !data.source_branch_exists;
this.shouldRemoveSourceBranch = (data.merge_params || {}).should_remove_source_branch || false; this.shouldRemoveSourceBranch = mergeParams.should_remove_source_branch ||
mergeParams.force_remove_source_branch === "1" || false;
this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false; this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false;
this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false; this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false;
this.mergePath = data.merge_path; this.mergePath = data.merge_path;
......
...@@ -5,3 +5,13 @@ ...@@ -5,3 +5,13 @@
-# This check is duplicated below, to avoid conflicts with EE. -# This check is duplicated below, to avoid conflicts with EE.
- return unless issuable.can_remove_source_branch?(current_user) - 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.
...@@ -29,6 +29,20 @@ feature 'Edit Merge Request', feature: true do ...@@ -29,6 +29,20 @@ feature 'Edit Merge Request', feature: true do
expect(page).to have_content 'Someone edited the merge request the same time you did' expect(page).to have_content 'Someone edited the merge request the same time you did'
end 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 it 'should preserve description textarea height', js: true do
long_description = %q( 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. 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.
......
...@@ -5,7 +5,7 @@ import * as simplePoll from '~/lib/utils/simple_poll'; ...@@ -5,7 +5,7 @@ import * as simplePoll from '~/lib/utils/simple_poll';
const commitMessage = 'This is the commit message'; const commitMessage = 'This is the commit message';
const commitMessageWithDescription = 'This is the commit message description'; const commitMessageWithDescription = 'This is the commit message description';
const createComponent = () => { const createComponent = (customConfig = {}) => {
const Component = Vue.extend(readyToMergeComponent); const Component = Vue.extend(readyToMergeComponent);
const mr = { const mr = {
isPipelineActive: false, isPipelineActive: false,
...@@ -17,8 +17,12 @@ const createComponent = () => { ...@@ -17,8 +17,12 @@ const createComponent = () => {
sha: '12345678', sha: '12345678',
commitMessage, commitMessage,
commitMessageWithDescription, commitMessageWithDescription,
shouldRemoveSourceBranch: true,
canRemoveSourceBranch: false,
}; };
Object.assign(mr, customConfig.mr);
const service = { const service = {
merge() {}, merge() {},
poll() {}, poll() {},
...@@ -51,7 +55,6 @@ describe('MRWidgetReadyToMerge', () => { ...@@ -51,7 +55,6 @@ describe('MRWidgetReadyToMerge', () => {
describe('data', () => { describe('data', () => {
it('should have default data', () => { it('should have default data', () => {
expect(vm.removeSourceBranch).toBeTruthy(true);
expect(vm.mergeWhenBuildSucceeds).toBeFalsy(); expect(vm.mergeWhenBuildSucceeds).toBeFalsy();
expect(vm.useCommitMessageWithDescription).toBeFalsy(); expect(vm.useCommitMessageWithDescription).toBeFalsy();
expect(vm.setToMergeWhenPipelineSucceeds).toBeFalsy(); expect(vm.setToMergeWhenPipelineSucceeds).toBeFalsy();
...@@ -166,6 +169,36 @@ describe('MRWidgetReadyToMerge', () => { ...@@ -166,6 +169,36 @@ describe('MRWidgetReadyToMerge', () => {
expect(vm.isMergeButtonDisabled).toBeTruthy(); 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.getElementById('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.getElementById('remove-source-branch-input');
expect(checkboxElement.getAttribute('disabled')).toBeNull();
});
});
});
}); });
describe('methods', () => { describe('methods', () => {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment