Merge branch 'winh-notes-error-handling' into 'master'

Handle errors in successful notes reply

Closes #61377

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

6 files changed, 87 insertions, 7 deletions

diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index bac124be34c..63658d49a05 100644
--- a/app/assets/javascripts/notes/stores/actions.js
+++ b/app/assets/javascripts/notes/stores/actions.js
@@ -268,11 +268,20 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
     const { errors } = res;
     const commandsChanges = res.commands_changes;
 
-    if (hasQuickActions && errors && Object.keys(errors).length) {
-      eTagPoll.makeRequest();
+    if (errors && Object.keys(errors).length) {
+      /*
+       The following reply means that quick actions have been successfully applied:
 
-      $('.js-gfm-input').trigger('clear-commands-cache.atwho');
-      Flash(__('Commands applied'), 'notice', noteData.flashContainer);
+       {"commands_changes":{},"valid":false,"errors":{"commands_only":["Commands applied"]}}
+       */
+      if (hasQuickActions) {
+        eTagPoll.makeRequest();
+
+        $('.js-gfm-input').trigger('clear-commands-cache.atwho');
+        Flash(__('Commands applied'), 'notice', noteData.flashContainer);
+      } else {
+        throw new Error(__('Failed to save comment!'));
+      }
     }
 
     if (commandsChanges) {
diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js
index 029fde348fb..ed4cef4a917 100644
--- a/app/assets/javascripts/notes/stores/utils.js
+++ b/app/assets/javascripts/notes/stores/utils.js
@@ -2,7 +2,8 @@ import AjaxCache from '~/lib/utils/ajax_cache';
 import { trimFirstCharOfLineContent } from '~/diffs/store/utils';
 import { sprintf, __ } from '~/locale';
 
-const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm;
+// factory function because global flag makes RegExp stateful
+const createQuickActionsRegex = () => /^\/\w+.*$/gm;
 
 export const findNoteObjectById = (notes, id) => notes.filter(n => n.id === id)[0];
 
@@ -27,9 +28,9 @@ export const getQuickActionText = note => {
   return text;
 };
 
-export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note);
+export const hasQuickActions = note => createQuickActionsRegex().test(note);
 
-export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim();
+export const stripQuickActions = note => note.replace(createQuickActionsRegex(), '').trim();
 
 export const prepareDiffLines = diffLines =>
   diffLines.map(line => ({ ...trimFirstCharOfLineContent(line) }));
diff --git a/changelogs/unreleased/winh-notes-error-handling.yml b/changelogs/unreleased/winh-notes-error-handling.yml
new file mode 100644
index 00000000000..6f23dd459d4
--- /dev/null
+++ b/changelogs/unreleased/winh-notes-error-handling.yml
@@ -0,0 +1,5 @@
+---
+title: Handle errors in successful notes reply
+merge_request: 28082
+author:
+type: fixed
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index b9fc3e00cff..4373d57ac7c 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -4196,6 +4196,9 @@ msgstr ""
 msgid "Failed to remove user key."
 msgstr ""
 
+msgid "Failed to save comment!"
+msgstr ""
+
 msgid "Failed to save merge conflicts resolutions. Please try again!"
 msgstr ""
 
diff --git a/spec/frontend/notes/stores/utils_spec.js b/spec/frontend/notes/stores/utils_spec.js
new file mode 100644
index 00000000000..b31b7491334
--- /dev/null
+++ b/spec/frontend/notes/stores/utils_spec.js
@@ -0,0 +1,17 @@
+import { hasQuickActions } from '~/notes/stores/utils';
+
+describe('hasQuickActions', () => {
+  it.each`
+    input                                | expected
+    ${'some comment'}                    | ${false}
+    ${'/quickaction'}                    | ${true}
+    ${'some comment with\n/quickaction'} | ${true}
+  `('returns $expected for $input', ({ input, expected }) => {
+    expect(hasQuickActions(input)).toBe(expected);
+  });
+
+  it('is stateless', () => {
+    expect(hasQuickActions('some comment')).toBe(hasQuickActions('some comment'));
+    expect(hasQuickActions('/quickaction')).toBe(hasQuickActions('/quickaction'));
+  });
+});
diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js
index 39901276b8c..7a9f32ddcff 100644
--- a/spec/javascripts/notes/stores/actions_spec.js
+++ b/spec/javascripts/notes/stores/actions_spec.js
@@ -794,6 +794,51 @@ describe('Actions Notes Store', () => {
     });
   });
 
+  describe('saveNote', () => {
+    const payload = { endpoint: TEST_HOST, data: { 'note[note]': 'some text' } };
+
+    describe('if response contains errors', () => {
+      const res = { errors: { something: ['went wrong'] } };
+
+      it('throws an error', done => {
+        actions
+          .saveNote(
+            {
+              commit() {},
+              dispatch: () => Promise.resolve(res),
+            },
+            payload,
+          )
+          .then(() => done.fail('Expected error to be thrown!'))
+          .catch(error => {
+            expect(error.message).toBe('Failed to save comment!');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+
+    describe('if response contains no errors', () => {
+      const res = { valid: true };
+
+      it('returns the response', done => {
+        actions
+          .saveNote(
+            {
+              commit() {},
+              dispatch: () => Promise.resolve(res),
+            },
+            payload,
+          )
+          .then(data => {
+            expect(data).toBe(res);
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+  });
+
   describe('submitSuggestion', () => {
     const discussionId = 'discussion-id';
     const noteId = 'note-id';