Default MR checkbox to true in most cases

- Pull the new MR option out into it's own component
- Default MR checkbox to true when creating a new MR and committing to a
  branch that does not have an MR
- Still change the MR checkbox to false when a user is on a branch that
  already has an MR
- Hide MR option when on a branch that already has an MR and committing
  to current branch
- Don't default to true when committing directly to master
This commit is contained in:
Sam Bigelow 2019-06-03 17:00:31 -04:00
parent 55920e074c
commit bf8ab12433
17 changed files with 377 additions and 112 deletions

View File

@ -1,23 +1,24 @@
<script>
import _ from 'underscore';
import { mapActions, mapState, mapGetters, createNamespacedHelpers } from 'vuex';
import { mapState, mapGetters, createNamespacedHelpers } from 'vuex';
import { sprintf, __ } from '~/locale';
import consts from '../../stores/modules/commit/constants';
import RadioGroup from './radio_group.vue';
import NewMergeRequestOption from './new_merge_request_option.vue';
const { mapState: mapCommitState, mapGetters: mapCommitGetters } = createNamespacedHelpers(
const { mapState: mapCommitState, mapActions: mapCommitActions } = createNamespacedHelpers(
'commit',
);
export default {
components: {
RadioGroup,
NewMergeRequestOption,
},
computed: {
...mapState(['currentBranchId', 'changedFiles', 'stagedFiles']),
...mapCommitState(['commitAction', 'shouldCreateMR', 'shouldDisableNewMrOption']),
...mapGetters(['currentProject', 'currentBranch', 'currentMergeRequest']),
...mapCommitGetters(['shouldDisableNewMrOption']),
...mapCommitState(['commitAction']),
...mapGetters(['currentBranch']),
commitToCurrentBranchText() {
return sprintf(
__('Commit to %{branchName} branch'),
@ -25,12 +26,12 @@ export default {
false,
);
},
disableMergeRequestRadio() {
containsStagedChanges() {
return this.changedFiles.length > 0 && this.stagedFiles.length > 0;
},
},
watch: {
disableMergeRequestRadio() {
containsStagedChanges() {
this.updateSelectedCommitAction();
},
},
@ -38,11 +39,11 @@ export default {
this.updateSelectedCommitAction();
},
methods: {
...mapActions('commit', ['updateCommitAction', 'toggleShouldCreateMR']),
...mapCommitActions(['updateCommitAction']),
updateSelectedCommitAction() {
if (this.currentBranch && !this.currentBranch.can_push) {
this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH);
} else if (this.disableMergeRequestRadio) {
} else if (this.containsStagedChanges) {
this.updateCommitAction(consts.COMMIT_TO_CURRENT_BRANCH);
}
},
@ -56,7 +57,7 @@ export default {
</script>
<template>
<div class="append-bottom-15 ide-commit-radios">
<div class="append-bottom-15 ide-commit-options">
<radio-group
:value="$options.commitToCurrentBranch"
:disabled="currentBranch && !currentBranch.can_push"
@ -69,17 +70,6 @@ export default {
:label="__('Create a new branch')"
:show-input="true"
/>
<hr class="my-2" />
<label class="mb-0">
<input
:checked="shouldCreateMR"
:disabled="shouldDisableNewMrOption"
type="checkbox"
@change="toggleShouldCreateMR"
/>
<span class="prepend-left-10" :class="{ 'text-secondary': shouldDisableNewMrOption }">
{{ __('Start a new merge request') }}
</span>
</label>
<new-merge-request-option />
</div>
</template>

View File

@ -0,0 +1,43 @@
<script>
import { mapGetters, createNamespacedHelpers } from 'vuex';
const {
mapState: mapCommitState,
mapGetters: mapCommitGetters,
mapActions: mapCommitActions,
} = createNamespacedHelpers('commit');
export default {
computed: {
...mapCommitState(['shouldCreateMR']),
...mapCommitGetters(['isCommittingToCurrentBranch', 'isCommittingToDefaultBranch']),
...mapGetters(['hasMergeRequest', 'isOnDefaultBranch']),
currentBranchHasMr() {
return this.hasMergeRequest && this.isCommittingToCurrentBranch;
},
showNewMrOption() {
return (
this.isCommittingToDefaultBranch || !this.currentBranchHasMr || this.isCommittingToNewBranch
);
},
},
mounted() {
this.setShouldCreateMR();
},
methods: {
...mapCommitActions(['toggleShouldCreateMR', 'setShouldCreateMR']),
},
};
</script>
<template>
<div v-if="showNewMrOption">
<hr class="my-2" />
<label class="mb-0">
<input :checked="shouldCreateMR" type="checkbox" @change="toggleShouldCreateMR" />
<span class="prepend-left-10">
{{ __('Start a new merge request') }}
</span>
</label>
</div>
</template>

View File

@ -97,7 +97,12 @@ export const lastCommit = (state, getters) => {
export const currentBranch = (state, getters) =>
getters.currentProject && getters.currentProject.branches[state.currentBranchId];
export const branchName = (_state, getters) => getters.currentBranch && getters.currentBranch.name;
export const packageJson = state => state.entries[packageJsonPath];
export const isOnDefaultBranch = (_state, getters) =>
getters.currentProject && getters.currentProject.default_branch === getters.branchName;
// prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {};

View File

@ -18,15 +18,34 @@ export const discardDraft = ({ commit }) => {
commit(types.UPDATE_COMMIT_MESSAGE, '');
};
export const updateCommitAction = ({ commit, rootGetters }, commitAction) => {
export const updateCommitAction = ({ commit, dispatch }, commitAction) => {
commit(types.UPDATE_COMMIT_ACTION, {
commitAction,
currentMergeRequest: rootGetters.currentMergeRequest,
});
dispatch('setShouldCreateMR');
};
export const toggleShouldCreateMR = ({ commit }) => {
commit(types.TOGGLE_SHOULD_CREATE_MR);
commit(types.INTERACT_WITH_NEW_MR);
};
export const setShouldCreateMR = ({
commit,
getters,
rootGetters,
state: { interactedWithNewMR },
}) => {
const committingToExistingMR =
getters.isCommittingToCurrentBranch &&
rootGetters.hasMergeRequest &&
!rootGetters.isOnDefaultBranch;
if ((getters.isCommittingToDefaultBranch && !interactedWithNewMR) || committingToExistingMR) {
commit(types.TOGGLE_SHOULD_CREATE_MR, false);
} else if (!interactedWithNewMR) {
commit(types.TOGGLE_SHOULD_CREATE_MR, true);
}
};
export const updateBranchName = ({ commit }, branchName) => {

View File

@ -48,8 +48,11 @@ export const preBuiltCommitMessage = (state, _, rootState) => {
export const isCreatingNewBranch = state => state.commitAction === consts.COMMIT_TO_NEW_BRANCH;
export const shouldDisableNewMrOption = (state, _getters, _rootState, rootGetters) =>
rootGetters.currentMergeRequest && state.commitAction === consts.COMMIT_TO_CURRENT_BRANCH;
export const isCommittingToCurrentBranch = state =>
state.commitAction === consts.COMMIT_TO_CURRENT_BRANCH;
export const isCommittingToDefaultBranch = (_state, getters, _rootState, rootGetters) =>
getters.isCommittingToCurrentBranch && rootGetters.isOnDefaultBranch;
// prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {};

View File

@ -3,3 +3,4 @@ export const UPDATE_COMMIT_ACTION = 'UPDATE_COMMIT_ACTION';
export const UPDATE_NEW_BRANCH_NAME = 'UPDATE_NEW_BRANCH_NAME';
export const UPDATE_LOADING = 'UPDATE_LOADING';
export const TOGGLE_SHOULD_CREATE_MR = 'TOGGLE_SHOULD_CREATE_MR';
export const INTERACT_WITH_NEW_MR = 'INTERACT_WITH_NEW_MR';

View File

@ -1,5 +1,4 @@
import * as types from './mutation_types';
import consts from './constants';
export default {
[types.UPDATE_COMMIT_MESSAGE](state, commitMessage) {
@ -7,14 +6,8 @@ export default {
commitMessage,
});
},
[types.UPDATE_COMMIT_ACTION](state, { commitAction, currentMergeRequest }) {
Object.assign(state, {
commitAction,
shouldCreateMR:
commitAction === consts.COMMIT_TO_CURRENT_BRANCH && currentMergeRequest
? false
: state.shouldCreateMR,
});
[types.UPDATE_COMMIT_ACTION](state, { commitAction }) {
Object.assign(state, { commitAction });
},
[types.UPDATE_NEW_BRANCH_NAME](state, newBranchName) {
Object.assign(state, {
@ -26,9 +19,12 @@ export default {
submitCommitLoading,
});
},
[types.TOGGLE_SHOULD_CREATE_MR](state) {
[types.TOGGLE_SHOULD_CREATE_MR](state, shouldCreateMR) {
Object.assign(state, {
shouldCreateMR: !state.shouldCreateMR,
shouldCreateMR: shouldCreateMR === undefined ? !state.shouldCreateMR : shouldCreateMR,
});
},
[types.INTERACT_WITH_NEW_MR](state) {
Object.assign(state, { interactedWithNewMR: true });
},
};

View File

@ -4,4 +4,5 @@ export default () => ({
newBranchName: '',
submitCommitLoading: false,
shouldCreateMR: false,
interactedWithNewMR: false,
});

View File

@ -719,7 +719,7 @@ $ide-commit-header-height: 48px;
border: 1px solid $white-dark;
}
.ide-commit-radios {
.ide-commit-options {
label {
font-weight: normal;

View File

@ -0,0 +1,5 @@
---
title: Default MR checkbox to true in most cases
merge_request: !28665
author:
type: changed

View File

@ -54,5 +54,20 @@ describe('IDE commit module mutations', () => {
expect(state.shouldCreateMR).toBe(false);
});
it('sets shouldCreateMR to given value when passed in', () => {
state.shouldCreateMR = false;
mutations.TOGGLE_SHOULD_CREATE_MR(state, false);
expect(state.shouldCreateMR).toBe(false);
});
});
describe('INTERACT_WITH_NEW_MR', () => {
it('sets interactedWithNewMR to true', () => {
mutations.INTERACT_WITH_NEW_MR(state);
expect(state.interactedWithNewMR).toBe(true);
});
});
});

View File

@ -73,47 +73,4 @@ describe('IDE commit sidebar actions', () => {
expect(vm.commitToCurrentBranchText).not.toContain(injectedSrc);
});
});
describe('create new MR checkbox', () => {
it('disables `createMR` button when an MR already exists and committing to current branch', () => {
createComponent({ hasMR: true, commitAction: consts.COMMIT_TO_CURRENT_BRANCH });
expect(vm.$el.querySelector('input[type="checkbox"]').disabled).toBe(true);
});
it('does not disable checkbox if MR does not exist', () => {
createComponent({ hasMR: false });
expect(vm.$el.querySelector('input[type="checkbox"]').disabled).toBe(false);
});
it('does not disable checkbox when creating a new branch', () => {
createComponent({ commitAction: consts.COMMIT_TO_NEW_BRANCH });
expect(vm.$el.querySelector('input[type="checkbox"]').disabled).toBe(false);
});
it('toggles off new MR when switching back to commit to current branch and MR exists', () => {
createComponent({
commitAction: consts.COMMIT_TO_NEW_BRANCH,
shouldCreateMR: true,
});
const currentBranchRadio = vm.$el.querySelector(
`input[value="${consts.COMMIT_TO_CURRENT_BRANCH}"`,
);
currentBranchRadio.click();
vm.$nextTick(() => {
expect(vm.$store.state.commit.shouldCreateMR).toBe(false);
});
});
it('toggles `shouldCreateMR` when clicking checkbox', () => {
createComponent();
const el = vm.$el.querySelector('input[type="checkbox"]');
el.dispatchEvent(new Event('change'));
expect(vm.$store.state.commit.shouldCreateMR).toBe(true);
});
});
});

View File

@ -0,0 +1,73 @@
import Vue from 'vue';
import store from '~/ide/stores';
import consts from '~/ide/stores/modules/commit/constants';
import NewMergeRequestOption from '~/ide/components/commit_sidebar/new_merge_request_option.vue';
import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
import { projectData } from 'spec/ide/mock_data';
import { resetStore } from 'spec/ide/helpers';
describe('create new MR checkbox', () => {
let vm;
const createComponent = ({
hasMR = false,
commitAction = consts.COMMIT_TO_NEW_BRANCH,
currentBranchId = 'master',
} = {}) => {
const Component = Vue.extend(NewMergeRequestOption);
vm = createComponentWithStore(Component, store);
vm.$store.state.currentBranchId = currentBranchId;
vm.$store.state.currentProjectId = 'abcproject';
vm.$store.state.commit.commitAction = commitAction;
Vue.set(vm.$store.state.projects, 'abcproject', { ...projectData });
if (hasMR) {
vm.$store.state.currentMergeRequestId = '1';
vm.$store.state.projects[store.state.currentProjectId].mergeRequests[
store.state.currentMergeRequestId
] = { foo: 'bar' };
}
return vm.$mount();
};
afterEach(() => {
vm.$destroy();
resetStore(vm.$store);
});
it('is hidden when an MR already exists and committing to current branch', () => {
createComponent({
hasMR: true,
commitAction: consts.COMMIT_TO_CURRENT_BRANCH,
currentBranchId: 'feature',
});
expect(vm.$el.textContent).toBe('');
});
it('does not hide checkbox if MR does not exist', () => {
createComponent({ hasMR: false });
expect(vm.$el.querySelector('input[type="checkbox"]').hidden).toBe(false);
});
it('does not hide checkbox when creating a new branch', () => {
createComponent({ commitAction: consts.COMMIT_TO_NEW_BRANCH });
expect(vm.$el.querySelector('input[type="checkbox"]').hidden).toBe(false);
});
it('dispatches toggleShouldCreateMR when clicking checkbox', () => {
createComponent();
const el = vm.$el.querySelector('input[type="checkbox"]');
spyOn(vm.$store, 'dispatch');
el.dispatchEvent(new Event('change'));
expect(vm.$store.dispatch.calls.allArgs()).toEqual(
jasmine.arrayContaining([['commit/toggleShouldCreateMR', jasmine.any(Object)]]),
);
});
});

View File

@ -16,6 +16,7 @@ export const projectData = {
},
mergeRequests: {},
merge_requests_enabled: true,
default_branch: 'master',
};
export const pipelines = [

View File

@ -180,6 +180,38 @@ describe('IDE store getters', () => {
});
});
describe('isOnDefaultBranch', () => {
it('returns false when no project exists', () => {
const localGetters = {
currentProject: undefined,
};
expect(getters.isOnDefaultBranch({}, localGetters)).toBeFalsy();
});
it("returns true when project's default branch matches current branch", () => {
const localGetters = {
currentProject: {
default_branch: 'master',
},
branchName: 'master',
};
expect(getters.isOnDefaultBranch({}, localGetters)).toBeTruthy();
});
it("returns false when project's default branch doesn't match current branch", () => {
const localGetters = {
currentProject: {
default_branch: 'master',
},
branchName: 'feature',
};
expect(getters.isOnDefaultBranch({}, localGetters)).toBeFalsy();
});
});
describe('packageJson', () => {
it('returns package.json entry', () => {
localState.entries['package.json'] = { name: 'package.json' };

View File

@ -1,9 +1,12 @@
import actions from '~/ide/stores/actions';
import rootActions from '~/ide/stores/actions';
import store from '~/ide/stores';
import service from '~/ide/services';
import router from '~/ide/ide_router';
import eventHub from '~/ide/eventhub';
import consts from '~/ide/stores/modules/commit/constants';
import * as mutationTypes from '~/ide/stores/modules/commit/mutation_types';
import * as actions from '~/ide/stores/modules/commit/actions';
import testAction from '../../../../helpers/vuex_action_helper';
import { commitActionTypes } from '~/ide/constants';
import { resetStore, file } from 'spec/ide/helpers';
@ -225,7 +228,7 @@ describe('IDE commit module actions', () => {
let visitUrl;
beforeEach(() => {
visitUrl = spyOnDependency(actions, 'visitUrl');
visitUrl = spyOnDependency(rootActions, 'visitUrl');
document.body.innerHTML += '<div class="flash-container"></div>';
@ -523,4 +526,154 @@ describe('IDE commit module actions', () => {
});
});
});
describe('toggleShouldCreateMR', () => {
it('commits both toggle and interacting with MR checkbox actions', done => {
testAction(
actions.toggleShouldCreateMR,
{},
store.state,
[
{ type: mutationTypes.TOGGLE_SHOULD_CREATE_MR },
{ type: mutationTypes.INTERACT_WITH_NEW_MR },
],
[],
done,
);
});
});
describe('setShouldCreateMR', () => {
beforeEach(() => {
store.state.projects = {
project: {
default_branch: 'master',
branches: {
master: {
name: 'master',
},
feature: {
name: 'feature',
},
},
},
};
store.state.currentProjectId = 'project';
});
it('sets to false when the current branch already has an MR', done => {
store.state.commit.currentMergeRequestId = 1;
store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
store.state.currentMergeRequestId = '1';
store.state.currentBranchId = 'feature';
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit.calls.allArgs()[0]).toEqual(
jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, false]),
);
done();
})
.catch(done.fail);
});
it('changes to false when current branch is the default branch and user has not interacted', done => {
store.state.commit.interactedWithNewMR = false;
store.state.currentBranchId = 'master';
store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit.calls.allArgs()[0]).toEqual(
jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, false]),
);
done();
})
.catch(done.fail);
});
it('changes to true when "create new branch" is selected and user has not interacted', done => {
store.state.commit.commitAction = consts.COMMIT_TO_NEW_BRANCH;
store.state.commit.interactedWithNewMR = false;
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit.calls.allArgs()[0]).toEqual(
jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, true]),
);
done();
})
.catch(done.fail);
});
it('does not change anything if user has interacted and comitting to new branch', done => {
store.state.commit.commitAction = consts.COMMIT_TO_NEW_BRANCH;
store.state.commit.interactedWithNewMR = true;
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit).not.toHaveBeenCalled();
done();
})
.catch(done.fail);
});
it('does not change anything if user has interacted and comitting to branch without MR', done => {
store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
store.state.commit.currentMergeRequestId = null;
store.state.commit.interactedWithNewMR = true;
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit).not.toHaveBeenCalled();
done();
})
.catch(done.fail);
});
it('still changes to false if hiding the checkbox', done => {
store.state.currentBranchId = 'feature';
store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
store.state.currentMergeRequestId = '1';
store.state.commit.interactedWithNewMR = true;
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit.calls.allArgs()[0]).toEqual(
jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, false]),
);
done();
})
.catch(done.fail);
});
it('does not change to false when on master and user has interacted even if MR exists', done => {
store.state.currentBranchId = 'master';
store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
store.state.currentMergeRequestId = '1';
store.state.commit.interactedWithNewMR = true;
spyOn(store, 'commit').and.callThrough();
store
.dispatch('commit/setShouldCreateMR')
.then(() => {
expect(store.commit).not.toHaveBeenCalled();
done();
})
.catch(done.fail);
});
});
});

View File

@ -144,33 +144,4 @@ describe('IDE commit module getters', () => {
});
});
});
describe('shouldDisableNewMrOption', () => {
it('returns false if commitAction `COMMIT_TO_NEW_BRANCH`', () => {
state.commitAction = consts.COMMIT_TO_NEW_BRANCH;
const rootState = {
currentMergeRequest: { foo: 'bar' },
};
expect(getters.shouldDisableNewMrOption(state, null, null, rootState)).toBeFalsy();
});
it('returns false if there is no current merge request', () => {
state.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
const rootState = {
currentMergeRequest: null,
};
expect(getters.shouldDisableNewMrOption(state, null, null, rootState)).toBeFalsy();
});
it('returns true an MR exists and commit action is `COMMIT_TO_CURRENT_BRANCH`', () => {
state.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
const rootState = {
currentMergeRequest: { foo: 'bar' },
};
expect(getters.shouldDisableNewMrOption(state, null, null, rootState)).toBeTruthy();
});
});
});