Merge branch '212595-ide-commit-errors' into 'master'

Handle different commit errors in IDE

See merge request gitlab-org/gitlab!42383

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

 <script>
 import { mapState, mapActions, mapGetters } from 'vuex';
-import { GlModal } from '@gitlab/ui';
+import { GlModal, GlSafeHtmlDirective } from '@gitlab/ui';
 import { n__, __ } from '~/locale';
 import LoadingButton from '~/vue_shared/components/loading_button.vue';
 import CommitMessageField from './message_field.vue';
@@ -8,6 +8,7 @@ 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 {
   components: {
@@ -17,15 +18,20 @@ export default {
     SuccessMessage,
     GlModal,
   },
+  directives: {
+    SafeHtml: GlSafeHtmlDirective,
+  },
   data() {
     return {
       isCompact: true,
       componentHeight: null,
+      // Keep track of "lastCommitError" so we hold onto the value even when "commitError" is cleared.
+      lastCommitError: createUnexpectedCommitError(),
     };
   },
   computed: {
     ...mapState(['changedFiles', 'stagedFiles', 'currentActivityView', 'lastCommitMsg']),
-    ...mapState('commit', ['commitMessage', 'submitCommitLoading']),
+    ...mapState('commit', ['commitMessage', 'submitCommitLoading', 'commitError']),
     ...mapGetters(['someUncommittedChanges']),
     ...mapGetters('commit', ['discardDraftButtonDisabled', 'preBuiltCommitMessage']),
     overviewText() {
@@ -38,11 +44,28 @@ export default {
     currentViewIsCommitView() {
       return this.currentActivityView === leftSidebarViews.commit.name;
     },
+    commitErrorPrimaryAction() {
+      if (!this.lastCommitError?.canCreateBranch) {
+        return undefined;
+      }
+
+      return {
+        text: __('Create new branch'),
+      };
+    },
   },
   watch: {
     currentActivityView: 'handleCompactState',
     someUncommittedChanges: 'handleCompactState',
     lastCommitMsg: 'handleCompactState',
+    commitError(val) {
+      if (!val) {
+        return;
+      }
+
+      this.lastCommitError = val;
+      this.$refs.commitErrorModal.show();
+    },
   },
   methods: {
     ...mapActions(['updateActivityBarView']),
@@ -53,9 +76,7 @@ export default {
       'updateCommitAction',
     ]),
     commit() {
-      return this.commitChanges().catch(() => {
-        this.$refs.createBranchModal.show();
-      });
+      return this.commitChanges();
     },
     forceCreateNewBranch() {
       return this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH).then(() => this.commit());
@@ -164,17 +185,14 @@ export default {
           </button>
         </div>
         <gl-modal
-          ref="createBranchModal"
-          modal-id="ide-create-branch-modal"
-          :ok-title="__('Create new branch')"
-          :title="__('Branch has changed')"
-          ok-variant="success"
+          ref="commitErrorModal"
+          modal-id="ide-commit-error-modal"
+          :title="lastCommitError.title"
+          :action-primary="commitErrorPrimaryAction"
+          :action-cancel="{ text: __('Cancel') }"
           @ok="forceCreateNewBranch"
         >
-          {{
-            __(`This branch has changed since you started editing.
-                Would you like to create a new branch?`)
-          }}
+          <div v-safe-html="lastCommitError.messageHTML"></div>
         </gl-modal>
       </form>
     </transition>

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

+import { escape } from 'lodash';
+import { __ } from '~/locale';
+
+const CODEOWNERS_REGEX = /Push.*protected branches.*CODEOWNERS/;
+const BRANCH_CHANGED_REGEX = /changed.*since.*start.*edit/;
+
+export const createUnexpectedCommitError = () => ({
+  title: __('Unexpected error'),
+  messageHTML: __('Could not commit. An unexpected error occurred.'),
+  canCreateBranch: false,
+});
+
+export const createCodeownersCommitError = message => ({
+  title: __('CODEOWNERS rule violation'),
+  messageHTML: escape(message),
+  canCreateBranch: true,
+});
+
+export const createBranchChangedCommitError = message => ({
+  title: __('Branch changed'),
+  messageHTML: `${escape(message)}<br/><br/>${__('Would you like to create a new branch?')}`,
+  canCreateBranch: true,
+});
+
+export const parseCommitError = e => {
+  const { message } = e?.response?.data || {};
+
+  if (!message) {
+    return createUnexpectedCommitError();
+  }
+
+  if (CODEOWNERS_REGEX.test(message)) {
+    return createCodeownersCommitError(message);
+  } else if (BRANCH_CHANGED_REGEX.test(message)) {
+    return createBranchChangedCommitError(message);
+  }
+
+  return createUnexpectedCommitError();
+};

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

 import { sprintf, __ } from '~/locale';
 import { deprecatedCreateFlash as flash } from '~/flash';
-import httpStatusCodes from '~/lib/utils/http_status';
 import * as rootTypes from '../../mutation_types';
 import { createCommitPayload, createNewMergeRequestUrl } from '../../utils';
 import service from '../../../services';
@@ -8,6 +7,7 @@ import * as types from './mutation_types';
 import consts from './constants';
 import { leftSidebarViews } from '../../../constants';
 import eventHub from '../../../eventhub';
+import { parseCommitError } from '../../../lib/errors';
 
 export const updateCommitMessage = ({ commit }, message) => {
   commit(types.UPDATE_COMMIT_MESSAGE, message);
@@ -113,6 +113,7 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo
     ? Promise.resolve()
     : dispatch('stageAllChanges', null, { root: true });
 
+  commit(types.CLEAR_ERROR);
   commit(types.UPDATE_LOADING, true);
 
   return stageFilesPromise
@@ -128,6 +129,12 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo
 
       return service.commit(rootState.currentProjectId, payload);
     })
+    .catch(e => {
+      commit(types.UPDATE_LOADING, false);
+      commit(types.SET_ERROR, parseCommitError(e));
+
+      throw e;
+    })
     .then(({ data }) => {
       commit(types.UPDATE_LOADING, false);
 
@@ -214,24 +221,5 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo
             { root: true },
           ),
         );
-    })
-    .catch(err => {
-      commit(types.UPDATE_LOADING, false);
-
-      // don't catch bad request errors, let the view handle them
-      if (err.response.status === httpStatusCodes.BAD_REQUEST) throw err;
-
-      dispatch(
-        'setErrorMessage',
-        {
-          text: __('An error occurred while committing your changes.'),
-          action: () =>
-            dispatch('commitChanges').then(() => dispatch('setErrorMessage', null, { root: true })),
-          actionText: __('Please try again'),
-        },
-        { root: true },
-      );
-
-      window.dispatchEvent(new Event('resize'));
     });
 };

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

@@ -3,3 +3,6 @@ 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 CLEAR_ERROR = 'CLEAR_ERROR';
+export const SET_ERROR = 'SET_ERROR';

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

@@ -24,4 +24,10 @@ export default {
       shouldCreateMR: shouldCreateMR === undefined ? !state.shouldCreateMR : shouldCreateMR,
     });
   },
+  [types.CLEAR_ERROR](state) {
+    state.commitError = null;
+  },
+  [types.SET_ERROR](state, error) {
+    state.commitError = error;
+  },
 };

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

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

changelogs/unreleased/212595-ide-commit-errors.yml

+---
+title: Fix error reporting for Web IDE commits
+merge_request: 42383
+author:
+type: fixed

ee/spec/features/ide/user_commits_changes_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'EE IDE user commits changes', :js do
+  include WebIdeSpecHelpers
+
+  let(:project) { create(:project, :custom_repo, files: { 'docs/CODEOWNERS' => "[Backend]\n*.rb @ruby-owner" }) }
+  let(:ruby_owner) { create(:user, username: 'ruby-owner') }
+  let(:user) { project.owner }
+
+  before do
+    stub_licensed_features(code_owners: true, code_owner_approval_required: true)
+
+    project.add_developer(ruby_owner)
+
+    create(:protected_branch,
+      name: 'master',
+      code_owner_approval_required: true,
+      project: project)
+
+    sign_in(user)
+
+    ide_visit(project)
+  end
+
+  it 'shows error message' do
+    ide_create_new_file('test.rb', content: '# A ruby file')
+
+    ide_commit
+
+    expect(page).to have_content('CODEOWNERS rule violation')
+  end
+end

locale/gitlab.pot

@@ -2678,9 +2678,6 @@ msgstr ""
 msgid "An error occurred while checking group path. Please refresh and try again."
 msgstr ""
 
-msgid "An error occurred while committing your changes."
-msgstr ""
-
 msgid "An error occurred while creating the issue. Please try again."
 msgstr ""
 
@@ -4114,7 +4111,7 @@ 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 has changed"
+msgid "Branch changed"
 msgstr ""
 
 msgid "Branch is already taken"
@@ -4420,6 +4417,9 @@ msgstr ""
 msgid "CLOSED (MOVED)"
 msgstr ""
 
+msgid "CODEOWNERS rule violation"
+msgstr ""
+
 msgid "CONTRIBUTING"
 msgstr ""
 
@@ -7134,6 +7134,9 @@ msgstr ""
 msgid "Could not change HEAD: branch '%{branch}' does not exist"
 msgstr ""
 
+msgid "Could not commit. An unexpected error occurred."
+msgstr ""
+
 msgid "Could not connect to FogBugz, check your URL"
 msgstr ""
 
@@ -25705,9 +25708,6 @@ msgstr ""
 msgid "This board's scope is reduced"
 msgstr ""
 
-msgid "This branch has changed since you started editing. Would you like to create a new branch?"
-msgstr ""
-
 msgid "This chart could not be displayed"
 msgstr ""
 
@@ -27056,6 +27056,9 @@ msgstr ""
 msgid "Undo ignore"
 msgstr ""
 
+msgid "Unexpected error"
+msgstr ""
+
 msgid "Unfortunately, your email message to GitLab could not be processed."
 msgstr ""
 
@@ -28715,6 +28718,9 @@ msgstr ""
 msgid "Workflow Help"
 msgstr ""
 
+msgid "Would you like to create a new branch?"
+msgstr ""
+
 msgid "Write"
 msgstr ""
 

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

 import Vue from 'vue';
+import { getByText } from '@testing-library/dom';
 import { createComponentWithStore } from 'helpers/vue_mount_component_helper';
 import { projectData } from 'jest/ide/mock_data';
 import waitForPromises from 'helpers/wait_for_promises';
 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';
 
 describe('IDE commit form', () => {
   const Component = Vue.extend(CommitForm);
@@ -259,21 +262,47 @@ describe('IDE commit form', () => {
         });
       });
 
-      it('opens new branch modal if commitChanges throws an error', () => {
-        vm.commitChanges.mockRejectedValue({ success: false });
+      it.each`
+        createError                                          | props
+        ${() => createCodeownersCommitError('test message')} | ${{ actionPrimary: { text: 'Create new branch' } }}
+        ${createUnexpectedCommitError}                       | ${{ actionPrimary: null }}
+      `('opens error modal if commitError with $error', async ({ createError, props }) => {
+        jest.spyOn(vm.$refs.commitErrorModal, 'show');
 
-        jest.spyOn(vm.$refs.createBranchModal, 'show').mockImplementation();
+        const error = createError();
+        store.state.commit.commitError = error;
 
-        return vm
-          .$nextTick()
-          .then(() => {
-            vm.$el.querySelector('.btn-success').click();
+        await vm.$nextTick();
 
-            return vm.$nextTick();
-          })
-          .then(() => {
-            expect(vm.$refs.createBranchModal.show).toHaveBeenCalled();
-          });
+        expect(vm.$refs.commitErrorModal.show).toHaveBeenCalled();
+        expect(vm.$refs.commitErrorModal).toMatchObject({
+          actionCancel: { text: 'Cancel' },
+          ...props,
+        });
+        // Because of the legacy 'mountComponent' approach here, the only way to
+        // test the text of the modal is by viewing the content of the modal added to the document.
+        expect(document.body).toHaveText(error.messageHTML);
+      });
+    });
+
+    describe('with error modal with primary', () => {
+      beforeEach(() => {
+        jest.spyOn(vm.$store, 'dispatch').mockReturnValue(Promise.resolve());
+      });
+
+      it('updates commit action and commits', async () => {
+        store.state.commit.commitError = createCodeownersCommitError('test message');
+
+        await vm.$nextTick();
+
+        getByText(document.body, 'Create new branch').click();
+
+        await waitForPromises();
+
+        expect(vm.$store.dispatch.mock.calls).toEqual([
+          ['commit/updateCommitAction', consts.COMMIT_TO_NEW_BRANCH],
+          ['commit/commitChanges', undefined],
+        ]);
       });
     });
   });

spec/frontend/ide/lib/errors_spec.js

+import {
+  createUnexpectedCommitError,
+  createCodeownersCommitError,
+  createBranchChangedCommitError,
+  parseCommitError,
+} from '~/ide/lib/errors';
+
+const TEST_SPECIAL = '&special<';
+const TEST_SPECIAL_ESCAPED = '&amp;special&lt;';
+const TEST_MESSAGE = 'Test message.';
+const CODEOWNERS_MESSAGE =
+  'Push to protected branches that contain changes to files matching CODEOWNERS is not allowed';
+const CHANGED_MESSAGE = 'Things changed since you started editing';
+
+describe('~/ide/lib/errors', () => {
+  const createResponseError = message => ({
+    response: {
+      data: {
+        message,
+      },
+    },
+  });
+
+  describe('createCodeownersCommitError', () => {
+    it('uses given message', () => {
+      expect(createCodeownersCommitError(TEST_MESSAGE)).toEqual({
+        title: 'CODEOWNERS rule violation',
+        messageHTML: TEST_MESSAGE,
+        canCreateBranch: true,
+      });
+    });
+
+    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,
+      });
+    });
+  });
+
+  describe('parseCommitError', () => {
+    it.each`
+      message                                    | expectation
+      ${null}                                    | ${createUnexpectedCommitError()}
+      ${{}}                                      | ${createUnexpectedCommitError()}
+      ${{ response: {} }}                        | ${createUnexpectedCommitError()}
+      ${{ response: { data: {} } }}              | ${createUnexpectedCommitError()}
+      ${createResponseError('test')}             | ${createUnexpectedCommitError()}
+      ${createResponseError(CODEOWNERS_MESSAGE)} | ${createCodeownersCommitError(CODEOWNERS_MESSAGE)}
+      ${createResponseError(CHANGED_MESSAGE)}    | ${createBranchChangedCommitError(CHANGED_MESSAGE)}
+    `('parses message into error object with "$message"', ({ message, expectation }) => {
+      expect(parseCommitError(message)).toEqual(expectation);
+    });
+  });
+});

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

@@ -9,6 +9,7 @@ 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 { createUnexpectedCommitError } from '~/ide/lib/errors';
 import { commitActionTypes, PERMISSION_CREATE_MR } from '~/ide/constants';
 import testAction from '../../../../helpers/vuex_action_helper';
 
@@ -510,7 +511,7 @@ describe('IDE commit module actions', () => {
       });
     });
 
-    describe('failed', () => {
+    describe('success response with failed message', () => {
       beforeEach(() => {
         jest.spyOn(service, 'commit').mockResolvedValue({
           data: {
@@ -533,6 +534,25 @@ describe('IDE commit module actions', () => {
       });
     });
 
+    describe('failed response', () => {
+      beforeEach(() => {
+        jest.spyOn(service, 'commit').mockRejectedValue({});
+      });
+
+      it('commits error updates', async () => {
+        jest.spyOn(store, 'commit');
+
+        await store.dispatch('commit/commitChanges').catch(() => {});
+
+        expect(store.commit.mock.calls).toEqual([
+          ['commit/CLEAR_ERROR', undefined, undefined],
+          ['commit/UPDATE_LOADING', true, undefined],
+          ['commit/UPDATE_LOADING', false, undefined],
+          ['commit/SET_ERROR', createUnexpectedCommitError(), undefined],
+        ]);
+      });
+    });
+
     describe('first commit of a branch', () => {
       const COMMIT_RESPONSE = {
         id: '123456',

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

 import commitState from '~/ide/stores/modules/commit/state';
 import mutations from '~/ide/stores/modules/commit/mutations';
+import * as types from '~/ide/stores/modules/commit/mutation_types';
 
 describe('IDE commit module mutations', () => {
   let state;
@@ -62,4 +63,24 @@ describe('IDE commit module mutations', () => {
       expect(state.shouldCreateMR).toBe(false);
     });
   });
+
+  describe(types.CLEAR_ERROR, () => {
+    it('should clear commitError', () => {
+      state.commitError = {};
+
+      mutations[types.CLEAR_ERROR](state);
+
+      expect(state.commitError).toBeNull();
+    });
+  });
+
+  describe(types.SET_ERROR, () => {
+    it('should set commitError', () => {
+      const error = { title: 'foo' };
+
+      mutations[types.SET_ERROR](state, error);
+
+      expect(state.commitError).toBe(error);
+    });
+  });
 });