Merge branch '249819-ide-improve-errors' into 'master'

Improve WebIDE error messages on committing

See merge request gitlab-org/gitlab!43408

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

@@ -7,7 +7,6 @@ import CommitMessageField from './message_field.vue';
 import Actions from './actions.vue';
 import SuccessMessage from './success_message.vue';
 import { leftSidebarViews, MAX_WINDOW_HEIGHT_COMPACT } from '../../constants';
-import consts from '../../stores/modules/commit/constants';
 import { createUnexpectedCommitError } from '../../lib/errors';
 
 export default {
@@ -45,12 +44,11 @@ export default {
       return this.currentActivityView === leftSidebarViews.commit.name;
     },
     commitErrorPrimaryAction() {
-      if (!this.lastCommitError?.canCreateBranch) {
-        return undefined;
-      }
+      const { primaryAction } = this.lastCommitError || {};
 
       return {
-        text: __('Create new branch'),
+        button: primaryAction ? { text: primaryAction.text } : undefined,
+        callback: primaryAction?.callback?.bind(this, this.$store) || (() => {}),
       };
     },
   },
@@ -78,9 +76,6 @@ export default {
     commit() {
       return this.commitChanges();
     },
-    forceCreateNewBranch() {
-      return this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH).then(() => this.commit());
-    },
     handleCompactState() {
       if (this.lastCommitMsg) {
         this.isCompact = false;
@@ -188,9 +183,9 @@ export default {
           ref="commitErrorModal"
           modal-id="ide-commit-error-modal"
           :title="lastCommitError.title"
-          :action-primary="commitErrorPrimaryAction"
+          :action-primary="commitErrorPrimaryAction.button"
           :action-cancel="{ text: __('Cancel') }"
-          @ok="forceCreateNewBranch"
+          @ok="commitErrorPrimaryAction.callback"
         >
           <div v-safe-html="lastCommitError.messageHTML"></div>
         </gl-modal>

app/assets/javascripts/ide/lib/errors.js

 import { escape } from 'lodash';
 import { __ } from '~/locale';
+import consts from '../stores/modules/commit/constants';
 
 const CODEOWNERS_REGEX = /Push.*protected branches.*CODEOWNERS/;
 const BRANCH_CHANGED_REGEX = /changed.*since.*start.*edit/;
+const BRANCH_ALREADY_EXISTS = /branch.*already.*exists/;
 
-export const createUnexpectedCommitError = () => ({
+const createNewBranchAndCommit = store =>
+  store
+    .dispatch('commit/updateCommitAction', consts.COMMIT_TO_NEW_BRANCH)
+    .then(() => store.dispatch('commit/commitChanges'));
+
+export const createUnexpectedCommitError = message => ({
   title: __('Unexpected error'),
-  messageHTML: __('Could not commit. An unexpected error occurred.'),
-  canCreateBranch: false,
+  messageHTML: escape(message) || __('Could not commit. An unexpected error occurred.'),
 });
 
 export const createCodeownersCommitError = message => ({
   title: __('CODEOWNERS rule violation'),
   messageHTML: escape(message),
-  canCreateBranch: true,
+  primaryAction: {
+    text: __('Create new branch'),
+    callback: createNewBranchAndCommit,
+  },
 });
 
 export const createBranchChangedCommitError = message => ({
   title: __('Branch changed'),
   messageHTML: `${escape(message)}<br/><br/>${__('Would you like to create a new branch?')}`,
-  canCreateBranch: true,
+  primaryAction: {
+    text: __('Create new branch'),
+    callback: createNewBranchAndCommit,
+  },
+});
+
+export const branchAlreadyExistsCommitError = message => ({
+  title: __('Branch already exists'),
+  messageHTML: `${escape(message)}<br/><br/>${__(
+    'Would you like to try auto-generating a branch name?',
+  )}`,
+  primaryAction: {
+    text: __('Create new branch'),
+    callback: store =>
+      store.dispatch('commit/addSuffixToBranchName').then(() => createNewBranchAndCommit(store)),
+  },
 });
 
 export const parseCommitError = e => {
@@ -33,7 +57,9 @@ export const parseCommitError = e => {
     return createCodeownersCommitError(message);
   } else if (BRANCH_CHANGED_REGEX.test(message)) {
     return createBranchChangedCommitError(message);
+  } else if (BRANCH_ALREADY_EXISTS.test(message)) {
+    return branchAlreadyExistsCommitError(message);
   }
 
-  return createUnexpectedCommitError();
+  return createUnexpectedCommitError(message);
 };

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

@@ -6,6 +6,7 @@ import {
   PERMISSION_CREATE_MR,
   PERMISSION_PUSH_CODE,
 } from '../constants';
+import { addNumericSuffix } from '~/ide/utils';
 import Api from '~/api';
 
 export const activeFile = state => state.openFiles.find(file => file.active) || null;
@@ -167,10 +168,7 @@ export const getAvailableFileName = (state, getters) => path => {
   let newPath = path;
 
   while (getters.entryExists(newPath)) {
-    newPath = newPath.replace(
-      /([ _-]?)(\d*)(\..+?$|$)/,
-      (_, before, number, after) => `${before || '_'}${Number(number) + 1}${after}`,
-    );
+    newPath = addNumericSuffix(newPath);
   }
 
   return newPath;

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

@@ -8,6 +8,7 @@ import consts from './constants';
 import { leftSidebarViews } from '../../../constants';
 import eventHub from '../../../eventhub';
 import { parseCommitError } from '../../../lib/errors';
+import { addNumericSuffix } from '~/ide/utils';
 
 export const updateCommitMessage = ({ commit }, message) => {
   commit(types.UPDATE_COMMIT_MESSAGE, message);
@@ -17,11 +18,8 @@ export const discardDraft = ({ commit }) => {
   commit(types.UPDATE_COMMIT_MESSAGE, '');
 };
 
-export const updateCommitAction = ({ commit, getters }, commitAction) => {
-  commit(types.UPDATE_COMMIT_ACTION, {
-    commitAction,
-  });
-  commit(types.TOGGLE_SHOULD_CREATE_MR, !getters.shouldHideNewMrOption);
+export const updateCommitAction = ({ commit }, commitAction) => {
+  commit(types.UPDATE_COMMIT_ACTION, { commitAction });
 };
 
 export const toggleShouldCreateMR = ({ commit }) => {
@@ -32,6 +30,12 @@ export const updateBranchName = ({ commit }, branchName) => {
   commit(types.UPDATE_NEW_BRANCH_NAME, branchName);
 };
 
+export const addSuffixToBranchName = ({ commit, state }) => {
+  const newBranchName = addNumericSuffix(state.newBranchName, true);
+
+  commit(types.UPDATE_NEW_BRANCH_NAME, newBranchName);
+};
+
 export const setLastCommitMessage = ({ commit, rootGetters }, data) => {
   const { currentProject } = rootGetters;
   const commitStats = data.stats
@@ -107,7 +111,7 @@ export const updateFilesAfterCommit = ({ commit, dispatch, rootState, rootGetter
 export const commitChanges = ({ commit, state, getters, dispatch, rootState, rootGetters }) => {
   // Pull commit options out because they could change
   // During some of the pre and post commit processing
-  const { shouldCreateMR, isCreatingNewBranch, branchName } = getters;
+  const { shouldCreateMR, shouldHideNewMrOption, isCreatingNewBranch, branchName } = getters;
   const newBranch = state.commitAction !== consts.COMMIT_TO_CURRENT_BRANCH;
   const stageFilesPromise = rootState.stagedFiles.length
     ? Promise.resolve()
@@ -167,7 +171,7 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo
             commit(rootTypes.SET_LAST_COMMIT_MSG, '', { root: true });
           }, 5000);
 
-          if (shouldCreateMR) {
+          if (shouldCreateMR && !shouldHideNewMrOption) {
             const { currentProject } = rootGetters;
             const targetBranch = isCreatingNewBranch
               ? rootState.currentBranchId

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

@@ -10,9 +10,7 @@ export default {
     Object.assign(state, { commitAction });
   },
   [types.UPDATE_NEW_BRANCH_NAME](state, newBranchName) {
-    Object.assign(state, {
-      newBranchName,
-    });
+    Object.assign(state, { newBranchName });
   },
   [types.UPDATE_LOADING](state, submitCommitLoading) {
     Object.assign(state, {

app/assets/javascripts/ide/utils.js

@@ -139,6 +139,34 @@ export function getFileEOL(content = '') {
   return content.includes('\r\n') ? 'CRLF' : 'LF';
 }
 
+/**
+ * Adds or increments the numeric suffix to a filename/branch name.
+ * Retains underscore or dash before the numeric suffix if it already exists.
+ *
+ * Examples:
+ *  hello -> hello-1
+ *  hello-2425 -> hello-2425
+ *  hello.md -> hello-1.md
+ *  hello_2.md -> hello_3.md
+ *  hello_ -> hello_1
+ *  master-patch-22432 -> master-patch-22433
+ *  patch_332 -> patch_333
+ *
+ * @param {string} filename File name or branch name
+ * @param {number} [randomize] Should randomize the numeric suffix instead of auto-incrementing?
+ */
+export function addNumericSuffix(filename, randomize = false) {
+  return filename.replace(/([ _-]?)(\d*)(\..+?$|$)/, (_, before, number, after) => {
+    const n = randomize
+      ? Math.random()
+          .toString()
+          .substring(2, 7)
+          .slice(-5)
+      : Number(number) + 1;
+    return `${before || '-'}${n}${after}`;
+  });
+}
+
 export const measurePerformance = (
   mark,
   measureName,

changelogs/unreleased/249819-ide-improve-errors.yml

+---
+title: Improve WebIDE error messages on committing
+merge_request: 43408
+author:
+type: changed

locale/gitlab.pot

@@ -4215,6 +4215,9 @@ msgstr ""
 msgid "Branch %{branch_name} was created. To set up auto deploy, choose a GitLab CI Yaml template and commit your changes. %{link_to_autodeploy_doc}"
 msgstr ""
 
+msgid "Branch already exists"
+msgstr ""
+
 msgid "Branch changed"
 msgstr ""
 
@@ -29476,6 +29479,9 @@ msgstr ""
 msgid "Would you like to create a new branch?"
 msgstr ""
 
+msgid "Would you like to try auto-generating a branch name?"
+msgstr ""
+
 msgid "Write"
 msgstr ""
 

spec/frontend/ide/components/commit_sidebar/form_spec.js

@@ -7,7 +7,12 @@ import { createStore } from '~/ide/stores';
 import consts from '~/ide/stores/modules/commit/constants';
 import CommitForm from '~/ide/components/commit_sidebar/form.vue';
 import { leftSidebarViews } from '~/ide/constants';
-import { createCodeownersCommitError, createUnexpectedCommitError } from '~/ide/lib/errors';
+import {
+  createCodeownersCommitError,
+  createUnexpectedCommitError,
+  createBranchChangedCommitError,
+  branchAlreadyExistsCommitError,
+} from '~/ide/lib/errors';
 
 describe('IDE commit form', () => {
   const Component = Vue.extend(CommitForm);
@@ -290,20 +295,30 @@ describe('IDE commit form', () => {
         jest.spyOn(vm.$store, 'dispatch').mockReturnValue(Promise.resolve());
       });
 
-      it('updates commit action and commits', async () => {
-        store.state.commit.commitError = createCodeownersCommitError('test message');
+      const commitActions = [
+        ['commit/updateCommitAction', consts.COMMIT_TO_NEW_BRANCH],
+        ['commit/commitChanges'],
+      ];
 
-        await vm.$nextTick();
+      it.each`
+        commitError                       | expectedActions
+        ${createCodeownersCommitError}    | ${commitActions}
+        ${createBranchChangedCommitError} | ${commitActions}
+        ${branchAlreadyExistsCommitError} | ${[['commit/addSuffixToBranchName'], ...commitActions]}
+      `(
+        'updates commit action and commits for error: $commitError',
+        async ({ commitError, expectedActions }) => {
+          store.state.commit.commitError = commitError('test message');
 
-        getByText(document.body, 'Create new branch').click();
+          await vm.$nextTick();
 
-        await waitForPromises();
+          getByText(document.body, 'Create new branch').click();
 
-        expect(vm.$store.dispatch.mock.calls).toEqual([
-          ['commit/updateCommitAction', consts.COMMIT_TO_NEW_BRANCH],
-          ['commit/commitChanges', undefined],
-        ]);
-      });
+          await waitForPromises();
+
+          expect(vm.$store.dispatch.mock.calls).toEqual(expectedActions);
+        },
+      );
     });
   });
 

spec/frontend/ide/lib/errors_spec.js

@@ -2,6 +2,7 @@ import {
   createUnexpectedCommitError,
   createCodeownersCommitError,
   createBranchChangedCommitError,
+  branchAlreadyExistsCommitError,
   parseCommitError,
 } from '~/ide/lib/errors';
 
@@ -21,35 +22,22 @@ describe('~/ide/lib/errors', () => {
     },
   });
 
-  describe('createCodeownersCommitError', () => {
-    it('uses given message', () => {
-      expect(createCodeownersCommitError(TEST_MESSAGE)).toEqual({
-        title: 'CODEOWNERS rule violation',
-        messageHTML: TEST_MESSAGE,
-        canCreateBranch: true,
-      });
-    });
+  const NEW_BRANCH_SUFFIX = `<br/><br/>Would you like to create a new branch?`;
+  const AUTOGENERATE_SUFFIX = `<br/><br/>Would you like to try auto-generating a branch name?`;
 
-    it('escapes special chars', () => {
-      expect(createCodeownersCommitError(TEST_SPECIAL)).toEqual({
-        title: 'CODEOWNERS rule violation',
-        messageHTML: TEST_SPECIAL_ESCAPED,
-        canCreateBranch: true,
-      });
-    });
-  });
-
-  describe('createBranchChangedCommitError', () => {
-    it.each`
-      message         | expectedMessage
-      ${TEST_MESSAGE} | ${`${TEST_MESSAGE}<br/><br/>Would you like to create a new branch?`}
-      ${TEST_SPECIAL} | ${`${TEST_SPECIAL_ESCAPED}<br/><br/>Would you like to create a new branch?`}
-    `('uses given message="$message"', ({ message, expectedMessage }) => {
-      expect(createBranchChangedCommitError(message)).toEqual({
-        title: 'Branch changed',
-        messageHTML: expectedMessage,
-        canCreateBranch: true,
-      });
+  it.each`
+    fn                                | title                          | message         | messageHTML
+    ${createCodeownersCommitError}    | ${'CODEOWNERS rule violation'} | ${TEST_MESSAGE} | ${TEST_MESSAGE}
+    ${createCodeownersCommitError}    | ${'CODEOWNERS rule violation'} | ${TEST_SPECIAL} | ${TEST_SPECIAL_ESCAPED}
+    ${branchAlreadyExistsCommitError} | ${'Branch already exists'}     | ${TEST_MESSAGE} | ${`${TEST_MESSAGE}${AUTOGENERATE_SUFFIX}`}
+    ${branchAlreadyExistsCommitError} | ${'Branch already exists'}     | ${TEST_SPECIAL} | ${`${TEST_SPECIAL_ESCAPED}${AUTOGENERATE_SUFFIX}`}
+    ${createBranchChangedCommitError} | ${'Branch changed'}            | ${TEST_MESSAGE} | ${`${TEST_MESSAGE}${NEW_BRANCH_SUFFIX}`}
+    ${createBranchChangedCommitError} | ${'Branch changed'}            | ${TEST_SPECIAL} | ${`${TEST_SPECIAL_ESCAPED}${NEW_BRANCH_SUFFIX}`}
+  `('$fn escapes and uses given message="$message"', ({ fn, title, message, messageHTML }) => {
+    expect(fn(message)).toEqual({
+      title,
+      messageHTML,
+      primaryAction: { text: 'Create new branch', callback: expect.any(Function) },
     });
   });
 
@@ -60,7 +48,7 @@ describe('~/ide/lib/errors', () => {
       ${{}}                                      | ${createUnexpectedCommitError()}
       ${{ response: {} }}                        | ${createUnexpectedCommitError()}
       ${{ response: { data: {} } }}              | ${createUnexpectedCommitError()}
-      ${createResponseError('test')}             | ${createUnexpectedCommitError()}
+      ${createResponseError(TEST_MESSAGE)}       | ${createUnexpectedCommitError(TEST_MESSAGE)}
       ${createResponseError(CODEOWNERS_MESSAGE)} | ${createCodeownersCommitError(CODEOWNERS_MESSAGE)}
       ${createResponseError(CHANGED_MESSAGE)}    | ${createBranchChangedCommitError(CHANGED_MESSAGE)}
     `('parses message into error object with "$message"', ({ message, expectation }) => {

spec/frontend/ide/stores/getters_spec.js

@@ -449,16 +449,16 @@ describe('IDE store getters', () => {
   describe('getAvailableFileName', () => {
     it.each`
       path                                          | newPath
-      ${'foo'}                                      | ${'foo_1'}
+      ${'foo'}                                      | ${'foo-1'}
       ${'foo__93.png'}                              | ${'foo__94.png'}
-      ${'foo/bar.png'}                              | ${'foo/bar_1.png'}
+      ${'foo/bar.png'}                              | ${'foo/bar-1.png'}
       ${'foo/bar--34.png'}                          | ${'foo/bar--35.png'}
       ${'foo/bar 2.png'}                            | ${'foo/bar 3.png'}
       ${'foo/bar-621.png'}                          | ${'foo/bar-622.png'}
-      ${'jquery.min.js'}                            | ${'jquery_1.min.js'}
+      ${'jquery.min.js'}                            | ${'jquery-1.min.js'}
       ${'my_spec_22.js.snap'}                       | ${'my_spec_23.js.snap'}
-      ${'subtitles5.mp4.srt'}                       | ${'subtitles_6.mp4.srt'}
-      ${'sample_file.mp3'}                          | ${'sample_file_1.mp3'}
+      ${'subtitles5.mp4.srt'}                       | ${'subtitles-6.mp4.srt'}
+      ${'sample-file.mp3'}                          | ${'sample-file-1.mp3'}
       ${'Screenshot 2020-05-26 at 10.53.08 PM.png'} | ${'Screenshot 2020-05-26 at 11.53.08 PM.png'}
     `('suffixes the path with a number if the path already exists', ({ path, newPath }) => {
       localState.entries[path] = file();

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

@@ -76,59 +76,38 @@ describe('IDE commit module actions', () => {
         .then(done)
         .catch(done.fail);
     });
+  });
 
-    it('sets shouldCreateMR to true if "Create new MR" option is visible', done => {
-      Object.assign(store.state, {
-        shouldHideNewMrOption: false,
-      });
+  describe('updateBranchName', () => {
+    let originalGon;
 
-      testAction(
-        actions.updateCommitAction,
-        {},
-        store.state,
-        [
-          {
-            type: mutationTypes.UPDATE_COMMIT_ACTION,
-            payload: { commitAction: expect.anything() },
-          },
-          { type: mutationTypes.TOGGLE_SHOULD_CREATE_MR, payload: true },
-        ],
-        [],
-        done,
-      );
+    beforeEach(() => {
+      originalGon = window.gon;
+      window.gon = { current_username: 'johndoe' };
+
+      store.state.currentBranchId = 'master';
     });
 
-    it('sets shouldCreateMR to false if "Create new MR" option is hidden', done => {
-      Object.assign(store.state, {
-        shouldHideNewMrOption: true,
-      });
+    afterEach(() => {
+      window.gon = originalGon;
+    });
 
-      testAction(
-        actions.updateCommitAction,
-        {},
-        store.state,
-        [
-          {
-            type: mutationTypes.UPDATE_COMMIT_ACTION,
-            payload: { commitAction: expect.anything() },
-          },
-          { type: mutationTypes.TOGGLE_SHOULD_CREATE_MR, payload: false },
-        ],
-        [],
-        done,
-      );
+    it('updates store with new branch name', async () => {
+      await store.dispatch('commit/updateBranchName', 'branch-name');
+
+      expect(store.state.commit.newBranchName).toBe('branch-name');
     });
   });
 
-  describe('updateBranchName', () => {
-    it('updates store with new branch name', done => {
-      store
-        .dispatch('commit/updateBranchName', 'branch-name')
-        .then(() => {
-          expect(store.state.commit.newBranchName).toBe('branch-name');
-        })
-        .then(done)
-        .catch(done.fail);
+  describe('addSuffixToBranchName', () => {
+    it('adds suffix to branchName', async () => {
+      jest.spyOn(Math, 'random').mockReturnValue(0.391352525);
+
+      store.state.commit.newBranchName = 'branch-name';
+
+      await store.dispatch('commit/addSuffixToBranchName');
+
+      expect(store.state.commit.newBranchName).toBe('branch-name-39135');
     });
   });
 
@@ -318,13 +297,16 @@ describe('IDE commit module actions', () => {
         currentBranchId: 'master',
         projects: {
           abcproject: {
+            default_branch: 'master',
             web_url: 'webUrl',
             branches: {
               master: {
+                name: 'master',
                 workingReference: '1',
                 commit: {
                   id: TEST_COMMIT_SHA,
                 },
+                can_push: true,
               },
             },
             userPermissions: {
@@ -499,6 +481,16 @@ describe('IDE commit module actions', () => {
             .catch(done.fail);
         });
 
+        it('does not redirect to merge request page if shouldCreateMR is checked, but branch is the default branch', async () => {
+          jest.spyOn(eventHub, '$on').mockImplementation();
+
+          store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH;
+          store.state.commit.shouldCreateMR = true;
+
+          await store.dispatch('commit/commitChanges');
+          expect(visitUrl).not.toHaveBeenCalled();
+        });
+
         it('resets changed files before redirecting', () => {
           jest.spyOn(eventHub, '$on').mockImplementation();
 

spec/frontend/ide/utils_spec.js

@@ -9,6 +9,7 @@ import {
   getPathParents,
   getPathParent,
   readFileAsDataURL,
+  addNumericSuffix,
 } from '~/ide/utils';
 
 describe('WebIDE utils', () => {
@@ -291,4 +292,43 @@ describe('WebIDE utils', () => {
       });
     });
   });
+
+  /*
+   *  hello-2425 -> hello-2425
+   *  hello.md -> hello-1.md
+   *  hello_2.md -> hello_3.md
+   *  hello_ -> hello_1
+   *  master-patch-22432 -> master-patch-22433
+   *  patch_332 -> patch_333
+   */
+
+  describe('addNumericSuffix', () => {
+    it.each`
+      input                   | output
+      ${'hello'}              | ${'hello-1'}
+      ${'hello2'}             | ${'hello-3'}
+      ${'hello.md'}           | ${'hello-1.md'}
+      ${'hello_2.md'}         | ${'hello_3.md'}
+      ${'hello_'}             | ${'hello_1'}
+      ${'master-patch-22432'} | ${'master-patch-22433'}
+      ${'patch_332'}          | ${'patch_333'}
+    `('adds a numeric suffix to a given filename/branch name: $input', ({ input, output }) => {
+      expect(addNumericSuffix(input)).toBe(output);
+    });
+
+    it.each`
+      input                   | output
+      ${'hello'}              | ${'hello-39135'}
+      ${'hello2'}             | ${'hello-39135'}
+      ${'hello.md'}           | ${'hello-39135.md'}
+      ${'hello_2.md'}         | ${'hello_39135.md'}
+      ${'hello_'}             | ${'hello_39135'}
+      ${'master-patch-22432'} | ${'master-patch-39135'}
+      ${'patch_332'}          | ${'patch_39135'}
+    `('adds a random suffix if randomize=true is passed for name: $input', ({ input, output }) => {
+      jest.spyOn(Math, 'random').mockReturnValue(0.391352525);
+
+      expect(addNumericSuffix(input, true)).toBe(output);
+    });
+  });
 });