Merge branch '57482-shortcut-to-create-merge-request-from-web-ide' into 'master'

Resolve "Shortcut to create merge request from Web IDE"

Closes #57482

See merge request gitlab-org/gitlab-ce!26792

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

@@ -2537,9 +2537,6 @@ msgstr ""
 msgid "Create a new branch"
 msgstr ""
 
-msgid "Create a new branch and merge request"
-msgstr ""
-
 msgid "Create a new issue"
 msgstr ""
 
@@ -7717,6 +7714,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 ""
 
@@ -8391,9 +8391,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,25 +59,28 @@ 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, () => {
-        beforeEach(() => {
-          Object.assign(state, {
-            commitAction: consts[type],
-          });
+    describe('COMMIT_TO_NEW_BRANCH', () => {
+      beforeEach(() => {
+        Object.assign(state, {
+          commitAction: consts.COMMIT_TO_NEW_BRANCH,
         });
+      });
 
-        it('uses newBranchName when not empty', () => {
-          expect(getters.branchName(state, localGetters, rootState)).toBe('state-newBranchName');
+      it('uses newBranchName when not empty', () => {
+        const newBranchName = 'nonempty-branch-name';
+        Object.assign(state, {
+          newBranchName,
         });
 
-        it('uses placeholderBranchName when state newBranchName is empty', () => {
-          Object.assign(state, {
-            newBranchName: '',
-          });
+        expect(getters.branchName(state, localGetters, rootState)).toBe(newBranchName);
+      });
 
-          expect(getters.branchName(state, localGetters, rootState)).toBe('newBranchName');
+      it('uses placeholderBranchName when state newBranchName is empty', () => {
+        Object.assign(state, {
+          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();
+    });
+  });
 });