Commit 6e049ac5 authored by Thomas Randolph's avatar Thomas Randolph Committed by Simon Knox

Show API errors when a command-only comment fails

parent 07c06377
<script> <script>
import { import {
GlAlert,
GlButton, GlButton,
GlIcon, GlIcon,
GlFormCheckbox, GlFormCheckbox,
...@@ -14,6 +15,7 @@ import { mapActions, mapGetters, mapState } from 'vuex'; ...@@ -14,6 +15,7 @@ import { mapActions, mapGetters, mapState } from 'vuex';
import Autosave from '~/autosave'; import Autosave from '~/autosave';
import { refreshUserMergeRequestCounts } from '~/commons/nav/user_merge_requests'; import { refreshUserMergeRequestCounts } from '~/commons/nav/user_merge_requests';
import { deprecatedCreateFlash as Flash } from '~/flash'; import { deprecatedCreateFlash as Flash } from '~/flash';
import httpStatusCodes from '~/lib/utils/http_status';
import { import {
capitalizeFirstCharacter, capitalizeFirstCharacter,
convertToCamelCase, convertToCamelCase,
...@@ -34,6 +36,8 @@ import CommentFieldLayout from './comment_field_layout.vue'; ...@@ -34,6 +36,8 @@ import CommentFieldLayout from './comment_field_layout.vue';
import discussionLockedWidget from './discussion_locked_widget.vue'; import discussionLockedWidget from './discussion_locked_widget.vue';
import noteSignedOutWidget from './note_signed_out_widget.vue'; import noteSignedOutWidget from './note_signed_out_widget.vue';
const { UNPROCESSABLE_ENTITY } = httpStatusCodes;
export default { export default {
name: 'CommentForm', name: 'CommentForm',
i18n: COMMENT_FORM, i18n: COMMENT_FORM,
...@@ -43,6 +47,7 @@ export default { ...@@ -43,6 +47,7 @@ export default {
noteSignedOutWidget, noteSignedOutWidget,
discussionLockedWidget, discussionLockedWidget,
markdownField, markdownField,
GlAlert,
GlButton, GlButton,
TimelineEntryItem, TimelineEntryItem,
GlIcon, GlIcon,
...@@ -66,6 +71,7 @@ export default { ...@@ -66,6 +71,7 @@ export default {
return { return {
note: '', note: '',
noteType: constants.COMMENT, noteType: constants.COMMENT,
errors: [],
noteIsConfidential: false, noteIsConfidential: false,
isSubmitting: false, isSubmitting: false,
}; };
...@@ -201,11 +207,19 @@ export default { ...@@ -201,11 +207,19 @@ export default {
'reopenIssuable', 'reopenIssuable',
'toggleIssueLocalState', 'toggleIssueLocalState',
]), ]),
handleSaveError({ data, status }) {
if (status === UNPROCESSABLE_ENTITY && data.errors?.commands_only?.length) {
this.errors = data.errors.commands_only;
} else {
this.errors = [this.$options.i18n.GENERIC_UNSUBMITTABLE_NETWORK];
}
},
handleSave(withIssueAction) { handleSave(withIssueAction) {
this.errors = [];
if (this.note.length) { if (this.note.length) {
const noteData = { const noteData = {
endpoint: this.endpoint, endpoint: this.endpoint,
flashContainer: this.$el,
data: { data: {
note: { note: {
noteable_type: this.noteableType, noteable_type: this.noteableType,
...@@ -236,10 +250,10 @@ export default { ...@@ -236,10 +250,10 @@ export default {
this.toggleIssueState(); this.toggleIssueState();
} }
}) })
.catch(() => { .catch(({ response }) => {
this.handleSaveError(response);
this.discard(false); this.discard(false);
const msg = this.$options.i18n.GENERIC_UNSUBMITTABLE_NETWORK;
Flash(msg, 'alert', this.$el);
this.note = noteData.data.note.note; // Restore textarea content. this.note = noteData.data.note.note; // Restore textarea content.
this.removePlaceholderNotes(); this.removePlaceholderNotes();
}) })
...@@ -318,6 +332,9 @@ export default { ...@@ -318,6 +332,9 @@ export default {
hasEmailParticipants() { hasEmailParticipants() {
return this.getNoteableData.issue_email_participants?.length; return this.getNoteableData.issue_email_participants?.length;
}, },
dismissError(index) {
this.errors.splice(index, 1);
},
}, },
}; };
</script> </script>
...@@ -328,7 +345,15 @@ export default { ...@@ -328,7 +345,15 @@ export default {
<discussion-locked-widget v-else-if="!canCreateNote" :issuable-type="issuableTypeTitle" /> <discussion-locked-widget v-else-if="!canCreateNote" :issuable-type="issuableTypeTitle" />
<ul v-else-if="canCreateNote" class="notes notes-form timeline"> <ul v-else-if="canCreateNote" class="notes notes-form timeline">
<timeline-entry-item class="note-form"> <timeline-entry-item class="note-form">
<div class="flash-container error-alert timeline-content"></div> <gl-alert
v-for="(error, index) in errors"
:key="index"
variant="danger"
class="gl-mb-2"
@dismiss="() => dismissError(index)"
>
{{ error }}
</gl-alert>
<div class="timeline-content timeline-content-form"> <div class="timeline-content timeline-content-form">
<form ref="commentForm" class="new-note common-note-form gfm-form js-main-target-form"> <form ref="commentForm" class="new-note common-note-form gfm-form js-main-target-form">
<comment-field-layout <comment-field-layout
......
---
title: Show API errors when a command-only comment fails
merge_request: 55457
author:
type: other
import { GlDropdown } from '@gitlab/ui'; import { GlDropdown, GlAlert } from '@gitlab/ui';
import { mount, shallowMount } from '@vue/test-utils'; import { mount, shallowMount } from '@vue/test-utils';
import Autosize from 'autosize'; import Autosize from 'autosize';
import MockAdapter from 'axios-mock-adapter'; import MockAdapter from 'axios-mock-adapter';
import { nextTick } from 'vue'; import Vue, { nextTick } from 'vue';
import Vuex from 'vuex';
import { extendedWrapper } from 'helpers/vue_test_utils_helper'; import { extendedWrapper } from 'helpers/vue_test_utils_helper';
import { refreshUserMergeRequestCounts } from '~/commons/nav/user_merge_requests'; import { refreshUserMergeRequestCounts } from '~/commons/nav/user_merge_requests';
import { deprecatedCreateFlash as flash } from '~/flash'; import { deprecatedCreateFlash as flash } from '~/flash';
...@@ -10,7 +11,8 @@ import axios from '~/lib/utils/axios_utils'; ...@@ -10,7 +11,8 @@ import axios from '~/lib/utils/axios_utils';
import CommentForm from '~/notes/components/comment_form.vue'; import CommentForm from '~/notes/components/comment_form.vue';
import * as constants from '~/notes/constants'; import * as constants from '~/notes/constants';
import eventHub from '~/notes/event_hub'; import eventHub from '~/notes/event_hub';
import createStore from '~/notes/stores'; import { COMMENT_FORM } from '~/notes/i18n';
import notesModule from '~/notes/stores/modules';
import { loggedOutnoteableData, notesDataMock, userDataMock, noteableDataMock } from '../mock_data'; import { loggedOutnoteableData, notesDataMock, userDataMock, noteableDataMock } from '../mock_data';
jest.mock('autosize'); jest.mock('autosize');
...@@ -18,6 +20,8 @@ jest.mock('~/commons/nav/user_merge_requests'); ...@@ -18,6 +20,8 @@ jest.mock('~/commons/nav/user_merge_requests');
jest.mock('~/flash'); jest.mock('~/flash');
jest.mock('~/gl_form'); jest.mock('~/gl_form');
Vue.use(Vuex);
describe('issue_comment_form component', () => { describe('issue_comment_form component', () => {
let store; let store;
let wrapper; let wrapper;
...@@ -28,6 +32,33 @@ describe('issue_comment_form component', () => { ...@@ -28,6 +32,33 @@ describe('issue_comment_form component', () => {
const findConfidentialNoteCheckbox = () => wrapper.findByTestId('confidential-note-checkbox'); const findConfidentialNoteCheckbox = () => wrapper.findByTestId('confidential-note-checkbox');
const findCommentGlDropdown = () => wrapper.find(GlDropdown); const findCommentGlDropdown = () => wrapper.find(GlDropdown);
const findCommentButton = () => findCommentGlDropdown().find('button'); const findCommentButton = () => findCommentGlDropdown().find('button');
const findErrorAlerts = () => wrapper.findAllComponents(GlAlert).wrappers;
async function clickCommentButton({ waitForComponent = true, waitForNetwork = true } = {}) {
findCommentButton().trigger('click');
if (waitForComponent || waitForNetwork) {
// Wait for the click to bubble out and trigger the handler
await nextTick();
if (waitForNetwork) {
// Wait for the network request promise to resolve
await nextTick();
}
}
}
function createStore({ actions = {} } = {}) {
const baseModule = notesModule();
return new Vuex.Store({
...baseModule,
actions: {
...baseModule.actions,
...actions,
},
});
}
const createNotableDataMock = (data = {}) => { const createNotableDataMock = (data = {}) => {
return { return {
...@@ -103,6 +134,83 @@ describe('issue_comment_form component', () => { ...@@ -103,6 +134,83 @@ describe('issue_comment_form component', () => {
expect(wrapper.vm.resizeTextarea).toHaveBeenCalled(); expect(wrapper.vm.resizeTextarea).toHaveBeenCalled();
}); });
it('does not report errors in the UI when the save succeeds', async () => {
mountComponent({ mountFunction: mount, initialData: { note: '/label ~sdfghj' } });
jest.spyOn(wrapper.vm, 'saveNote').mockResolvedValue();
await clickCommentButton();
// findErrorAlerts().exists returns false if *any* wrapper is empty,
// not necessarily that there aren't any at all.
// We want to check here that there are none found, so we use the
// raw wrapper array length instead.
expect(findErrorAlerts().length).toBe(0);
});
it.each`
httpStatus | errors
${400} | ${[COMMENT_FORM.GENERIC_UNSUBMITTABLE_NETWORK]}
${422} | ${['error 1']}
${422} | ${['error 1', 'error 2']}
${422} | ${['error 1', 'error 2', 'error 3']}
`(
'displays the correct errors ($errors) for a $httpStatus network response',
async ({ errors, httpStatus }) => {
store = createStore({
actions: {
saveNote: jest.fn().mockRejectedValue({
response: { status: httpStatus, data: { errors: { commands_only: errors } } },
}),
},
});
mountComponent({ mountFunction: mount, initialData: { note: '/label ~sdfghj' } });
await clickCommentButton();
const errorAlerts = findErrorAlerts();
expect(errorAlerts.length).toBe(errors.length);
errors.forEach((msg, index) => {
const alert = errorAlerts[index];
expect(alert.text()).toBe(msg);
});
},
);
it('should remove the correct error from the list when it is dismissed', async () => {
const commandErrors = ['1', '2', '3'];
store = createStore({
actions: {
saveNote: jest.fn().mockRejectedValue({
response: { status: 422, data: { errors: { commands_only: [...commandErrors] } } },
}),
},
});
mountComponent({ mountFunction: mount, initialData: { note: '/label ~sdfghj' } });
await clickCommentButton();
let errorAlerts = findErrorAlerts();
expect(errorAlerts.length).toBe(commandErrors.length);
// dismiss the second error
extendedWrapper(errorAlerts[1]).findByTestId('close-icon').trigger('click');
// Wait for the dismissal to bubble out of the Alert component and be handled in this component
await nextTick();
// Refresh the list of alerts
errorAlerts = findErrorAlerts();
expect(errorAlerts.length).toBe(commandErrors.length - 1);
// We want to know that the *correct* error was dismissed, not just that any one is gone
expect(errorAlerts[0].text()).toBe(commandErrors[0]);
expect(errorAlerts[1].text()).toBe(commandErrors[2]);
});
it('should toggle issue state when no note', () => { it('should toggle issue state when no note', () => {
mountComponent({ mountFunction: mount }); mountComponent({ mountFunction: mount });
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment