Shortcut to create MR in web IDE

Before the user had to choose between committing to a new branch,
committing to a new branch AND creating an MR, or committing to the
current branch regardless of whether or not it already has an MR.

This commit separates the creation of an MR from whether or not they
commit to an existing or new branch

app/assets/javascripts/ide/components/commit_sidebar/actions.vue

 <script>
 import _ from 'underscore';
-import { mapActions, mapState, mapGetters } from 'vuex';
+import { mapActions, mapState, mapGetters, createNamespacedHelpers } from 'vuex';
 import { sprintf, __ } from '~/locale';
-import * as consts from '../../stores/modules/commit/constants';
+import consts from '../../stores/modules/commit/constants';
 import RadioGroup from './radio_group.vue';
 
+const { mapState: mapCommitState, mapGetters: mapCommitGetters } = createNamespacedHelpers(
+  'commit',
+);
+
 export default {
   components: {
     RadioGroup,
   },
   computed: {
     ...mapState(['currentBranchId', 'changedFiles', 'stagedFiles']),
-    ...mapGetters(['currentProject', 'currentBranch']),
+    ...mapCommitState(['commitAction', 'shouldCreateMR', 'shouldDisableNewMrOption']),
+    ...mapGetters(['currentProject', 'currentBranch', 'currentMergeRequest']),
+    ...mapCommitGetters(['shouldDisableNewMrOption']),
     commitToCurrentBranchText() {
       return sprintf(
         __('Commit to %{branchName} branch'),
@@ -32,7 +38,7 @@ export default {
     this.updateSelectedCommitAction();
   },
   methods: {
-    ...mapActions('commit', ['updateCommitAction']),
+    ...mapActions('commit', ['updateCommitAction', 'toggleShouldCreateMR']),
     updateSelectedCommitAction() {
       if (this.currentBranch && !this.currentBranch.can_push) {
         this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH);
@@ -43,7 +49,6 @@ export default {
   },
   commitToCurrentBranch: consts.COMMIT_TO_CURRENT_BRANCH,
   commitToNewBranch: consts.COMMIT_TO_NEW_BRANCH,
-  commitToNewBranchMR: consts.COMMIT_TO_NEW_BRANCH_MR,
   currentBranchPermissionsTooltip: __(
     "This option is disabled as you don't have write permissions for the current branch",
   ),
@@ -64,13 +69,17 @@ export default {
       :label="__('Create a new branch')"
       :show-input="true"
     />
-    <radio-group
-      v-if="currentProject.merge_requests_enabled"
-      :value="$options.commitToNewBranchMR"
-      :label="__('Create a new branch and merge request')"
-      :title="__('This option is disabled while you still have unstaged changes')"
-      :show-input="true"
-      :disabled="disableMergeRequestRadio"
+    <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>
   </div>
 </template>

app/assets/javascripts/ide/components/repo_commit_section.vue

@@ -5,7 +5,7 @@ import Icon from '~/vue_shared/components/icon.vue';
 import DeprecatedModal from '~/vue_shared/components/deprecated_modal.vue';
 import CommitFilesList from './commit_sidebar/list.vue';
 import EmptyState from './commit_sidebar/empty_state.vue';
-import * as consts from '../stores/modules/commit/constants';
+import consts from '../stores/modules/commit/constants';
 import { activityBarViews, stageKeys } from '../constants';
 
 export default {

app/assets/javascripts/ide/stores/getters.js

@@ -25,7 +25,10 @@ export const projectsWithTrees = state =>
   });
 
 export const currentMergeRequest = state => {
-  if (state.projects[state.currentProjectId]) {
+  if (
+    state.projects[state.currentProjectId] &&
+    state.projects[state.currentProjectId].mergeRequests
+  ) {
     return state.projects[state.currentProjectId].mergeRequests[state.currentMergeRequestId];
   }
   return null;

app/assets/javascripts/ide/stores/modules/commit/actions.js

@@ -6,7 +6,7 @@ import { createCommitPayload, createNewMergeRequestUrl } from '../../utils';
 import router from '../../../ide_router';
 import service from '../../../services';
 import * as types from './mutation_types';
-import * as consts from './constants';
+import consts from './constants';
 import { activityBarViews } from '../../../constants';
 import eventHub from '../../../eventhub';
 
@@ -18,16 +18,23 @@ export const discardDraft = ({ commit }) => {
   commit(types.UPDATE_COMMIT_MESSAGE, '');
 };
 
-export const updateCommitAction = ({ commit }, commitAction) => {
-  commit(types.UPDATE_COMMIT_ACTION, commitAction);
+export const updateCommitAction = ({ commit, rootGetters }, commitAction) => {
+  commit(types.UPDATE_COMMIT_ACTION, {
+    commitAction,
+    currentMergeRequest: rootGetters.currentMergeRequest,
+  });
+};
+
+export const toggleShouldCreateMR = ({ commit }) => {
+  commit(types.TOGGLE_SHOULD_CREATE_MR);
 };
 
 export const updateBranchName = ({ commit }, branchName) => {
   commit(types.UPDATE_NEW_BRANCH_NAME, branchName);
 };
 
-export const setLastCommitMessage = ({ rootState, commit }, data) => {
-  const currentProject = rootState.projects[rootState.currentProjectId];
+export const setLastCommitMessage = ({ commit, rootGetters }, data) => {
+  const { currentProject } = rootGetters;
   const commitStats = data.stats
     ? sprintf(__('with %{additions} additions, %{deletions} deletions.'), {
         additions: data.stats.additions,
@@ -48,8 +55,8 @@ export const setLastCommitMessage = ({ rootState, commit }, data) => {
   commit(rootTypes.SET_LAST_COMMIT_MSG, commitMsg, { root: true });
 };
 
-export const updateFilesAfterCommit = ({ commit, dispatch, rootState }, { data }) => {
-  const selectedProject = rootState.projects[rootState.currentProjectId];
+export const updateFilesAfterCommit = ({ commit, dispatch, rootState, rootGetters }, { data }) => {
+  const selectedProject = rootGetters.currentProject;
   const lastCommit = {
     commit_path: `${selectedProject.web_url}/commit/${data.id}`,
     commit: {
@@ -135,14 +142,15 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo
         branch: getters.branchName,
       })
         .then(() => {
-          if (state.commitAction === consts.COMMIT_TO_NEW_BRANCH_MR) {
+          if (state.shouldCreateMR) {
+            const { currentProject } = rootGetters;
+            const targetBranch = getters.isCreatingNewBranch
+              ? rootState.currentBranchId
+              : currentProject.default_branch;
+
             dispatch(
               'redirectToUrl',
-              createNewMergeRequestUrl(
-                rootState.projects[rootState.currentProjectId].web_url,
-                getters.branchName,
-                rootState.currentBranchId,
-              ),
+              createNewMergeRequestUrl(currentProject.web_url, getters.branchName, targetBranch),
               { root: true },
             );
           }

app/assets/javascripts/ide/stores/modules/commit/constants.js

-export const COMMIT_TO_CURRENT_BRANCH = '1';
-export const COMMIT_TO_NEW_BRANCH = '2';
-export const COMMIT_TO_NEW_BRANCH_MR = '3';
+const COMMIT_TO_CURRENT_BRANCH = '1';
+const COMMIT_TO_NEW_BRANCH = '2';
+
+export default {
+  COMMIT_TO_CURRENT_BRANCH,
+  COMMIT_TO_NEW_BRANCH,
+};

app/assets/javascripts/ide/stores/modules/commit/getters.js

 import { sprintf, n__, __ } from '../../../../locale';
-import * as consts from './constants';
+import consts from './constants';
 
 const BRANCH_SUFFIX_COUNT = 5;
 const createTranslatedTextForFiles = (files, text) => {
@@ -20,10 +20,7 @@ export const placeholderBranchName = (state, _, rootState) =>
   )}`;
 
 export const branchName = (state, getters, rootState) => {
-  if (
-    state.commitAction === consts.COMMIT_TO_NEW_BRANCH ||
-    state.commitAction === consts.COMMIT_TO_NEW_BRANCH_MR
-  ) {
+  if (state.commitAction === consts.COMMIT_TO_NEW_BRANCH) {
     if (state.newBranchName === '') {
       return getters.placeholderBranchName;
     }
@@ -49,5 +46,10 @@ export const preBuiltCommitMessage = (state, _, rootState) => {
     .join('\n');
 };
 
+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;
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/ide/stores/modules/commit/mutation_types.js

@@ -2,3 +2,4 @@ export const UPDATE_COMMIT_MESSAGE = 'UPDATE_COMMIT_MESSAGE';
 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';

app/assets/javascripts/ide/stores/modules/commit/mutations.js

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

app/assets/javascripts/ide/stores/modules/commit/state.js

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

changelogs/unreleased/57482-shortcut-to-create-merge-request-from-web-ide.yml

+---
+title: Create a shortcut for a new MR in the Web IDE
+merge_request: 26792
+author:
+type: added

locale/gitlab.pot

@@ -2501,9 +2501,6 @@ msgstr ""
 msgid "Create a new branch"
 msgstr ""
 
-msgid "Create a new branch and merge request"
-msgstr ""
-
 msgid "Create a new issue"
 msgstr ""
 
@@ -7600,6 +7597,9 @@ msgstr ""
 msgid "Start a %{new_merge_request} with these changes"
 msgstr ""
 
+msgid "Start a new merge request"
+msgstr ""
+
 msgid "Start and due date"
 msgstr ""
 
@@ -8236,9 +8236,6 @@ msgstr ""
 msgid "This option is disabled as you don't have write permissions for the current branch"
 msgstr ""
 
-msgid "This option is disabled while you still have unstaged changes"
-msgstr ""
-
 msgid "This page is unavailable because you are not allowed to read information across multiple projects."
 msgstr ""
 

spec/frontend/ide/stores/modules/commit/mutations_spec.js

@@ -18,7 +18,7 @@ describe('IDE commit module mutations', () => {
 
   describe('UPDATE_COMMIT_ACTION', () => {
     it('updates commitAction', () => {
-      mutations.UPDATE_COMMIT_ACTION(state, 'testing');
+      mutations.UPDATE_COMMIT_ACTION(state, { commitAction: 'testing' });
 
       expect(state.commitAction).toBe('testing');
     });
@@ -39,4 +39,20 @@ describe('IDE commit module mutations', () => {
       expect(state.submitCommitLoading).toBeTruthy();
     });
   });
+
+  describe('TOGGLE_SHOULD_CREATE_MR', () => {
+    it('changes shouldCreateMR to true when initial state is false', () => {
+      state.shouldCreateMR = false;
+      mutations.TOGGLE_SHOULD_CREATE_MR(state);
+
+      expect(state.shouldCreateMR).toBe(true);
+    });
+
+    it('changes shouldCreateMR to false when initial state is true', () => {
+      state.shouldCreateMR = true;
+      mutations.TOGGLE_SHOULD_CREATE_MR(state);
+
+      expect(state.shouldCreateMR).toBe(false);
+    });
+  });
 });

spec/javascripts/ide/components/commit_sidebar/actions_spec.js

 import Vue from 'vue';
 import store from '~/ide/stores';
+import consts from '~/ide/stores/modules/commit/constants';
 import commitActions from '~/ide/components/commit_sidebar/actions.vue';
 import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
 import { resetStore } from 'spec/ide/helpers';
@@ -7,20 +8,33 @@ import { projectData } from 'spec/ide/mock_data';
 
 describe('IDE commit sidebar actions', () => {
   let vm;
-
-  beforeEach(done => {
+  const createComponent = ({
+    hasMR = false,
+    commitAction = consts.COMMIT_TO_NEW_BRANCH,
+    mergeRequestsEnabled = true,
+    currentBranchId = 'master',
+    shouldCreateMR = false,
+  } = {}) => {
     const Component = Vue.extend(commitActions);
 
     vm = createComponentWithStore(Component, store);
 
-    vm.$store.state.currentBranchId = 'master';
+    vm.$store.state.currentBranchId = currentBranchId;
     vm.$store.state.currentProjectId = 'abcproject';
+    vm.$store.state.commit.commitAction = commitAction;
     Vue.set(vm.$store.state.projects, 'abcproject', { ...projectData });
+    vm.$store.state.projects.abcproject.merge_requests_enabled = mergeRequestsEnabled;
+    vm.$store.state.commit.shouldCreateMR = shouldCreateMR;
 
-    vm.$mount();
+    if (hasMR) {
+      vm.$store.state.currentMergeRequestId = '1';
+      vm.$store.state.projects[store.state.currentProjectId].mergeRequests[
+        store.state.currentMergeRequestId
+      ] = { foo: 'bar' };
+    }
 
-    Vue.nextTick(done);
-  });
+    return vm.$mount();
+  };
 
   afterEach(() => {
     vm.$destroy();
@@ -28,16 +42,20 @@ describe('IDE commit sidebar actions', () => {
     resetStore(vm.$store);
   });
 
-  it('renders 3 groups', () => {
-    expect(vm.$el.querySelectorAll('input[type="radio"]').length).toBe(3);
+  it('renders 2 groups', () => {
+    createComponent();
+
+    expect(vm.$el.querySelectorAll('input[type="radio"]').length).toBe(2);
   });
 
   it('renders current branch text', () => {
+    createComponent();
+
     expect(vm.$el.textContent).toContain('Commit to master branch');
   });
 
   it('hides merge request option when project merge requests are disabled', done => {
-    vm.$store.state.projects.abcproject.merge_requests_enabled = false;
+    createComponent({ mergeRequestsEnabled: false });
 
     vm.$nextTick(() => {
       expect(vm.$el.querySelectorAll('input[type="radio"]').length).toBe(2);
@@ -49,9 +67,53 @@ describe('IDE commit sidebar actions', () => {
 
   describe('commitToCurrentBranchText', () => {
     it('escapes current branch', () => {
-      vm.$store.state.currentBranchId = '<img src="x" />';
+      const injectedSrc = '<img src="x" />';
+      createComponent({ currentBranchId: injectedSrc });
+
+      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.commitToCurrentBranchText).not.toContain('<img src="x" />');
+      expect(vm.$store.state.commit.shouldCreateMR).toBe(true);
     });
   });
 });

spec/javascripts/ide/stores/modules/commit/actions_spec.js

@@ -3,7 +3,7 @@ import store from '~/ide/stores';
 import service from '~/ide/services';
 import router from '~/ide/ide_router';
 import eventHub from '~/ide/eventhub';
-import * as consts from '~/ide/stores/modules/commit/constants';
+import consts from '~/ide/stores/modules/commit/constants';
 import { resetStore, file } from 'spec/ide/helpers';
 
 describe('IDE commit module actions', () => {
@@ -389,7 +389,8 @@ describe('IDE commit module actions', () => {
         it('redirects to new merge request page', done => {
           spyOn(eventHub, '$on');
 
-          store.state.commit.commitAction = '3';
+          store.state.commit.commitAction = consts.COMMIT_TO_NEW_BRANCH;
+          store.state.commit.shouldCreateMR = true;
 
           store
             .dispatch('commit/commitChanges')
@@ -405,6 +406,21 @@ describe('IDE commit module actions', () => {
             .catch(done.fail);
         });
 
+        it('does not redirect to new merge request page when shouldCreateMR is not checked', done => {
+          spyOn(eventHub, '$on');
+
+          store.state.commit.commitAction = consts.COMMIT_TO_NEW_BRANCH;
+          store.state.commit.shouldCreateMR = false;
+
+          store
+            .dispatch('commit/commitChanges')
+            .then(() => {
+              expect(visitUrl).not.toHaveBeenCalled();
+              done();
+            })
+            .catch(done.fail);
+        });
+
         it('resets changed files before redirecting', done => {
           spyOn(eventHub, '$on');
 

spec/javascripts/ide/stores/modules/commit/getters_spec.js

 import commitState from '~/ide/stores/modules/commit/state';
-import * as consts from '~/ide/stores/modules/commit/constants';
+import consts from '~/ide/stores/modules/commit/constants';
 import * as getters from '~/ide/stores/modules/commit/getters';
 
 describe('IDE commit module getters', () => {
@@ -46,7 +46,7 @@ describe('IDE commit module getters', () => {
       currentBranchId: 'master',
     };
     const localGetters = {
-      placeholderBranchName: 'newBranchName',
+      placeholderBranchName: 'placeholder-branch-name',
     };
 
     beforeEach(() => {
@@ -59,16 +59,20 @@ describe('IDE commit module getters', () => {
       expect(getters.branchName(state, null, rootState)).toBe('master');
     });
 
-    ['COMMIT_TO_NEW_BRANCH', 'COMMIT_TO_NEW_BRANCH_MR'].forEach(type => {
-      describe(type, () => {
+    describe('COMMIT_TO_NEW_BRANCH', () => {
       beforeEach(() => {
         Object.assign(state, {
-            commitAction: consts[type],
+          commitAction: consts.COMMIT_TO_NEW_BRANCH,
         });
       });
 
       it('uses newBranchName when not empty', () => {
-          expect(getters.branchName(state, localGetters, rootState)).toBe('state-newBranchName');
+        const newBranchName = 'nonempty-branch-name';
+        Object.assign(state, {
+          newBranchName,
+        });
+
+        expect(getters.branchName(state, localGetters, rootState)).toBe(newBranchName);
       });
 
       it('uses placeholderBranchName when state newBranchName is empty', () => {
@@ -76,8 +80,7 @@ describe('IDE commit module getters', () => {
           newBranchName: '',
         });
 
-          expect(getters.branchName(state, localGetters, rootState)).toBe('newBranchName');
-        });
+        expect(getters.branchName(state, localGetters, rootState)).toBe('placeholder-branch-name');
       });
     });
   });
@@ -141,4 +144,33 @@ 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();
+    });
+  });
 });