Merge branch...

Merge branch '27239-display-a-better-message-when-starting-a-discussion-on-a-deleted-comment' into 'master'

Resolve "Display a better message when starting a discussion on a deleted comment"

See merge request gitlab-org/gitlab!20031

app/assets/javascripts/notes/components/comment_form.vue

@@ -193,23 +193,10 @@ export default {
         this.stopPolling();
 
         this.saveNote(noteData)
-          .then(res => {
+          .then(() => {
             this.enableButton();
             this.restartPolling();
-
-            if (res.errors) {
-              if (res.errors.commands_only) {
-                this.discard();
-              } else {
-                Flash(
-                  __('Something went wrong while adding your comment. Please try again.'),
-                  'alert',
-                  this.$refs.commentForm,
-                );
-              }
-            } else {
-              this.discard();
-            }
+            this.discard();
 
             if (withIssueAction) {
               this.toggleIssueState();

app/assets/javascripts/notes/components/noteable_discussion.vue

@@ -198,23 +198,22 @@ export default {
         data: postData,
       };
 
-      this.isReplying = false;
       this.saveNote(replyData)
-        .then(() => {
-          clearDraft(this.autosaveKey);
+        .then(res => {
+          if (res.hasFlash !== true) {
+            this.isReplying = false;
+            clearDraft(this.autosaveKey);
+          }
           callback();
         })
         .catch(err => {
           this.removePlaceholderNotes();
-          this.isReplying = true;
-          this.$nextTick(() => {
-            const msg = __(
-              'Your comment could not be submitted! Please check your network connection and try again.',
-            );
-            Flash(msg, 'alert', this.$el);
-            this.$refs.noteForm.note = noteText;
-            callback(err);
-          });
+          const msg = __(
+            'Your comment could not be submitted! Please check your network connection and try again.',
+          );
+          Flash(msg, 'alert', this.$el);
+          this.$refs.noteForm.note = noteText;
+          callback(err);
         });
     },
     jumpToNextDiscussion() {

app/assets/javascripts/notes/stores/actions.js

@@ -14,7 +14,7 @@ import sidebarTimeTrackingEventHub from '../../sidebar/event_hub';
 import { isInViewport, scrollToElement, isInMRPage } from '../../lib/utils/common_utils';
 import { mergeUrlParams } from '../../lib/utils/url_utility';
 import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub';
-import { __ } from '~/locale';
+import { __, sprintf } from '~/locale';
 import Api from '~/api';
 
 let eTagPoll;
@@ -252,29 +252,22 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
     }
   }
 
-  const processErrors = res => {
-    const { errors } = res;
-    if (!errors || !Object.keys(errors).length) {
-      return res;
-    }
-
+  const processQuickActions = res => {
+    const { errors: { commands_only: message } = { commands_only: null } } = res;
     /*
      The following reply means that quick actions have been successfully applied:
 
      {"commands_changes":{},"valid":false,"errors":{"commands_only":["Commands applied"]}}
      */
-    if (hasQuickActions) {
+    if (hasQuickActions && message) {
       eTagPoll.makeRequest();
 
       $('.js-gfm-input').trigger('clear-commands-cache.atwho');
 
-      const { commands_only: message } = errors;
       Flash(message || __('Commands applied'), 'notice', noteData.flashContainer);
-
-      return res;
     }
 
-    throw new Error(__('Failed to save comment!'));
+    return res;
   };
 
   const processEmojiAward = res => {
@@ -321,11 +314,33 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
     return res;
   };
 
+  const processErrors = error => {
+    if (error.response) {
+      const {
+        response: { data = {} },
+      } = error;
+      const { errors = {} } = data;
+      const { base = [] } = errors;
+
+      // we handle only errors.base for now
+      if (base.length > 0) {
+        const errorMsg = sprintf(__('Your comment could not be submitted because %{error}'), {
+          error: base[0].toLowerCase(),
+        });
+        Flash(errorMsg, 'alert', noteData.flashContainer);
+        return { ...data, hasFlash: true };
+      }
+    }
+
+    throw error;
+  };
+
   return dispatch(methodToDispatch, postData, { root: true })
-    .then(processErrors)
+    .then(processQuickActions)
     .then(processEmojiAward)
     .then(processTimeTracking)
-    .then(removePlaceholder);
+    .then(removePlaceholder)
+    .catch(processErrors);
 };
 
 const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => {

app/services/notes/build_service.rb

@@ -11,7 +11,7 @@ module Notes
 
         unless discussion && can?(current_user, :create_note, discussion.noteable)
           note = Note.new
-          note.errors.add(:base, 'Discussion to reply to cannot be found')
+          note.errors.add(:base, _('Discussion to reply to cannot be found'))
           return note
         end
 

app/services/notes/create_service.rb

@@ -2,6 +2,7 @@
 
 module Notes
   class CreateService < ::Notes::BaseService
+    # rubocop:disable Metrics/CyclomaticComplexity
     def execute
       merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
 
@@ -9,7 +10,9 @@ module Notes
 
       # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37440
       note_valid = Gitlab::GitalyClient.allow_n_plus_1_calls do
-        note.valid?
+        # We may set errors manually in Notes::BuildService for this reason
+        # we also need to check for already existing errors.
+        note.errors.empty? && note.valid?
       end
 
       return note unless note_valid
@@ -67,6 +70,7 @@ module Notes
 
       note
     end
+    # rubocop:enable Metrics/CyclomaticComplexity
 
     private
 

changelogs/unreleased/27239-display-a-better-message-when-starting-a-discussion-on-a-deleted-co.yml

+---
+title: Display a better message when starting a discussion on a deleted comment
+merge_request: 20031
+author: Jacopo Beschi @jacopo-beschi
+type: changed

locale/gitlab.pot

@@ -6078,6 +6078,9 @@ msgstr ""
 msgid "Discussion"
 msgstr ""
 
+msgid "Discussion to reply to cannot be found"
+msgstr ""
+
 msgid "Disk Usage"
 msgstr ""
 
@@ -7395,9 +7398,6 @@ msgstr ""
 msgid "Failed to reset key. Please try again."
 msgstr ""
 
-msgid "Failed to save comment!"
-msgstr ""
-
 msgid "Failed to save merge conflicts resolutions. Please try again!"
 msgstr ""
 
@@ -16399,9 +16399,6 @@ msgstr ""
 msgid "Something went wrong while adding your award. Please try again."
 msgstr ""
 
-msgid "Something went wrong while adding your comment. Please try again."
-msgstr ""
-
 msgid "Something went wrong while applying the suggestion. Please try again."
 msgstr ""
 
@@ -20647,6 +20644,9 @@ msgstr ""
 msgid "Your changes have been successfully committed."
 msgstr ""
 
+msgid "Your comment could not be submitted because %{error}"
+msgstr ""
+
 msgid "Your comment could not be submitted! Please check your network connection and try again."
 msgstr ""
 

spec/features/merge_request/user_posts_notes_spec.rb

@@ -95,6 +95,24 @@ describe 'Merge request > User posts notes', :js do
     end
   end
 
+  describe 'reply on a deleted conversation' do
+    before do
+      visit project_merge_request_path(project, merge_request)
+    end
+
+    it 'shows an error message' do
+      find('.js-reply-button').click
+      note.delete
+
+      page.within('.discussion-reply-holder') do
+        fill_in 'note[note]', with: 'A reply'
+        click_button 'Comment'
+        wait_for_requests
+        expect(page).to have_content('Your comment could not be submitted because discussion to reply to cannot be found')
+      end
+    end
+  end
+
   describe 'when previewing a note' do
     it 'shows the toolbar buttons when editing a note' do
       page.within('.js-main-target-form') do

spec/javascripts/notes/stores/actions_spec.js

@@ -751,24 +751,54 @@ describe('Actions Notes Store', () => {
   });
 
   describe('saveNote', () => {
-    const payload = { endpoint: TEST_HOST, data: { 'note[note]': 'some text' } };
+    const flashContainer = {};
+    const payload = { endpoint: TEST_HOST, data: { 'note[note]': 'some text' }, flashContainer };
 
     describe('if response contains errors', () => {
       const res = { errors: { something: ['went wrong'] } };
+      const error = { message: 'Unprocessable entity', response: { data: res } };
 
       it('throws an error', done => {
         actions
           .saveNote(
             {
               commit() {},
-              dispatch: () => Promise.resolve(res),
+              dispatch: () => Promise.reject(error),
             },
             payload,
           )
           .then(() => done.fail('Expected error to be thrown!'))
-          .catch(error => {
-            expect(error.message).toBe('Failed to save comment!');
+          .catch(err => {
+            expect(err).toBe(error);
+            expect(flashSpy).not.toHaveBeenCalled();
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+
+    describe('if response contains errors.base', () => {
+      const res = { errors: { base: ['something went wrong'] } };
+      const error = { message: 'Unprocessable entity', response: { data: res } };
+
+      it('sets flash alert using errors.base message', done => {
+        actions
+          .saveNote(
+            {
+              commit() {},
+              dispatch: () => Promise.reject(error),
+            },
+            { ...payload, flashContainer },
+          )
+          .then(resp => {
+            expect(resp.hasFlash).toBe(true);
+            expect(flashSpy).toHaveBeenCalledWith(
+              'Your comment could not be submitted because something went wrong',
+              'alert',
+              flashContainer,
+            );
           })
+          .catch(() => done.fail('Expected success response!'))
           .then(done)
           .catch(done.fail);
       });
@@ -788,6 +818,7 @@ describe('Actions Notes Store', () => {
           )
           .then(data => {
             expect(data).toBe(res);
+            expect(flashSpy).not.toHaveBeenCalled();
           })
           .then(done)
           .catch(done.fail);

spec/services/notes/create_service_spec.rb

@@ -382,6 +382,19 @@ describe Notes::CreateService do
         end.to change { existing_note.type }.from(nil).to('DiscussionNote')
             .and change { existing_note.updated_at }
       end
+
+      context 'discussion to reply cannot be found' do
+        before do
+          existing_note.delete
+        end
+
+        it 'returns an note with errors' do
+          note = subject
+
+          expect(note.errors).not_to be_empty
+          expect(note.errors[:base]).to eq(['Discussion to reply to cannot be found'])
+        end
+      end
     end
 
     describe "usage counter" do

spec/support/shared_examples/graphql/notes_creation_shared_examples.rb

@@ -26,6 +26,7 @@ end
 RSpec.shared_examples 'a Note mutation when there are active record validation errors' do |model: Note|
   before do
     expect_next_instance_of(model) do |note|
+      allow(note).to receive_message_chain(:errors, :empty?).and_return(true)
       expect(note).to receive(:valid?).at_least(:once).and_return(false)
       expect(note).to receive_message_chain(
         :errors,