Merge branch '298827-graphql-getting-mr-diff-discussions-often-returns-500' into 'master'

Resolve "GraphQL: getting MR diff discussions often returns 500"

See merge request gitlab-org/gitlab!56037

app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js

+import { isEmpty } from 'lodash';
 import { deprecatedCreateFlash as flash } from '~/flash';
 import { scrollToElement } from '~/lib/utils/common_utils';
 import { __ } from '~/locale';
@@ -88,18 +89,23 @@ export const updateDiscussionsAfterPublish = async ({ dispatch, getters, rootGet
 export const updateDraft = (
   { commit, getters },
   { note, noteText, resolveDiscussion, position, callback },
-) =>
-  service
-    .update(getters.getNotesData.draftsPath, {
+) => {
+  const params = {
     draftId: note.id,
     note: noteText,
     resolveDiscussion,
-      position: JSON.stringify(position),
-    })
+  };
+  // Stringifying an empty object yields `{}` which breaks graphql queries
+  // https://gitlab.com/gitlab-org/gitlab/-/issues/298827
+  if (!isEmpty(position)) params.position = JSON.stringify(position);
+
+  return service
+    .update(getters.getNotesData.draftsPath, params)
     .then((res) => res.data)
     .then((data) => commit(types.RECEIVE_DRAFT_UPDATE_SUCCESS, data))
     .then(callback)
     .catch(() => flash(__('An error occurred while updating the comment')));
+};
 
 export const scrollToDraft = ({ dispatch, rootGetters }, draft) => {
   const discussion = draft.discussion_id && rootGetters.getDiscussion(draft.discussion_id);

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

 <script>
 import { GlSprintf, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
 import $ from 'jquery';
-import { escape } from 'lodash';
+import { escape, isEmpty } from 'lodash';
 import { mapGetters, mapActions } from 'vuex';
 import { INLINE_DIFF_LINES_KEY } from '~/diffs/constants';
 import httpStatusCodes from '~/lib/utils/http_status';
@@ -282,9 +282,13 @@ export default {
         note: {
           target_type: this.getNoteableData.targetType,
           target_id: this.note.noteable_id,
-          note: { note: noteText, position: JSON.stringify(position) },
+          note: { note: noteText },
         },
       };
+
+      // Stringifying an empty object yields `{}` which breaks graphql queries
+      // https://gitlab.com/gitlab-org/gitlab/-/issues/298827
+      if (!isEmpty(position)) data.note.note.position = JSON.stringify(position);
       this.isRequesting = true;
       this.oldContent = this.note.note_html;
       // eslint-disable-next-line vue/no-mutating-props

changelogs/unreleased/298827-graphql-getting-mr-diff-discussions-often-returns-500.yml

+---
+title: fix stringify empty position object
+merge_request: 56037
+author:
+type: fixed

spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js

 import MockAdapter from 'axios-mock-adapter';
 import { TEST_HOST } from 'helpers/test_constants';
 import testAction from 'helpers/vuex_action_helper';
+import service from '~/batch_comments/services/drafts_service';
 import * as actions from '~/batch_comments/stores/modules/batch_comments/actions';
 import axios from '~/lib/utils/axios_utils';
 
@@ -201,6 +202,12 @@ describe('Batch comments store actions', () => {
 
   describe('updateDraft', () => {
     let getters;
+    service.update = jest.fn();
+    service.update.mockResolvedValue({ data: { id: 1 } });
+
+    const commit = jest.fn();
+    let context;
+    let params;
 
     beforeEach(() => {
       getters = {
@@ -208,43 +215,43 @@ describe('Batch comments store actions', () => {
           draftsPath: TEST_HOST,
         },
       };
-    });
 
-    it('commits RECEIVE_DRAFT_UPDATE_SUCCESS with returned data', (done) => {
-      const commit = jest.fn();
-      const context = {
+      context = {
         getters,
         commit,
       };
       res = { id: 1 };
       mock.onAny().reply(200, res);
+      params = { note: { id: 1 }, noteText: 'test' };
+    });
 
-      actions
-        .updateDraft(context, { note: { id: 1 }, noteText: 'test', callback() {} })
-        .then(() => {
+    afterEach(() => jest.clearAllMocks());
+
+    it('commits RECEIVE_DRAFT_UPDATE_SUCCESS with returned data', () => {
+      return actions.updateDraft(context, { ...params, callback() {} }).then(() => {
         expect(commit).toHaveBeenCalledWith('RECEIVE_DRAFT_UPDATE_SUCCESS', { id: 1 });
-        })
-        .then(done)
-        .catch(done.fail);
+      });
     });
 
-    it('calls passed callback', (done) => {
-      const commit = jest.fn();
-      const context = {
-        getters,
-        commit,
-      };
+    it('calls passed callback', () => {
       const callback = jest.fn();
-      res = { id: 1 };
-      mock.onAny().reply(200, res);
-
-      actions
-        .updateDraft(context, { note: { id: 1 }, noteText: 'test', callback })
-        .then(() => {
+      return actions.updateDraft(context, { ...params, callback }).then(() => {
         expect(callback).toHaveBeenCalled();
-        })
-        .then(done)
-        .catch(done.fail);
+      });
+    });
+
+    it('does not stringify empty position', () => {
+      return actions.updateDraft(context, { ...params, position: {}, callback() {} }).then(() => {
+        expect(service.update.mock.calls[0][1].position).toBeUndefined();
+      });
+    });
+
+    it('stringifies a non-empty position', () => {
+      const position = { test: true };
+      const expectation = JSON.stringify(position);
+      return actions.updateDraft(context, { ...params, position, callback() {} }).then(() => {
+        expect(service.update.mock.calls[0][1].position).toBe(expectation);
+      });
     });
   });
 

spec/frontend/notes/components/noteable_note_spec.js

 import { mount, createLocalVue } from '@vue/test-utils';
 import { escape } from 'lodash';
+import waitForPromises from 'helpers/wait_for_promises';
 import NoteActions from '~/notes/components/note_actions.vue';
 import NoteBody from '~/notes/components/note_body.vue';
 import NoteHeader from '~/notes/components/note_header.vue';
@@ -13,7 +14,7 @@ describe('issue_note', () => {
   let wrapper;
   const findMultilineComment = () => wrapper.find('[data-testid="multiline-comment"]');
 
-  beforeEach(() => {
+  const createWrapper = (props = {}) => {
     store = createStore();
     store.dispatch('setNoteableData', noteableDataMock);
     store.dispatch('setNotesData', notesDataMock);
@@ -23,6 +24,7 @@ describe('issue_note', () => {
       store,
       propsData: {
         note,
+        ...props,
       },
       localVue,
       stubs: [
@@ -33,14 +35,18 @@ describe('issue_note', () => {
         'multiline-comment-form',
       ],
     });
-  });
+  };
 
   afterEach(() => {
     wrapper.destroy();
   });
 
   describe('mutiline comments', () => {
-    it('should render if has multiline comment', () => {
+    beforeEach(() => {
+      createWrapper();
+    });
+
+    it('should render if has multiline comment', async () => {
       const position = {
         line_range: {
           start: {
@@ -69,9 +75,8 @@ describe('issue_note', () => {
         line,
       });
 
-      return wrapper.vm.$nextTick().then(() => {
-        expect(findMultilineComment().text()).toEqual('Comment on lines 1 to 2');
-      });
+      await wrapper.vm.$nextTick();
+      expect(findMultilineComment().text()).toBe('Comment on lines 1 to 2');
     });
 
     it('should only render if it has everything it needs', () => {
@@ -147,9 +152,14 @@ describe('issue_note', () => {
     });
   });
 
+  describe('rendering', () => {
+    beforeEach(() => {
+      createWrapper();
+    });
+
     it('should render user information', () => {
       const { author } = note;
-    const avatar = wrapper.find(UserAvatarLink);
+      const avatar = wrapper.findComponent(UserAvatarLink);
       const avatarProps = avatar.props();
 
       expect(avatarProps.linkHref).toBe(author.path);
@@ -159,17 +169,17 @@ describe('issue_note', () => {
     });
 
     it('should render note header content', () => {
-    const noteHeader = wrapper.find(NoteHeader);
+      const noteHeader = wrapper.findComponent(NoteHeader);
       const noteHeaderProps = noteHeader.props();
 
-    expect(noteHeaderProps.author).toEqual(note.author);
-    expect(noteHeaderProps.createdAt).toEqual(note.created_at);
-    expect(noteHeaderProps.noteId).toEqual(note.id);
+      expect(noteHeaderProps.author).toBe(note.author);
+      expect(noteHeaderProps.createdAt).toBe(note.created_at);
+      expect(noteHeaderProps.noteId).toBe(note.id);
     });
 
     it('should render note actions', () => {
       const { author } = note;
-    const noteActions = wrapper.find(NoteActions);
+      const noteActions = wrapper.findComponent(NoteActions);
       const noteActionsProps = noteActions.props();
 
       expect(noteActionsProps.authorId).toBe(author.id);
@@ -189,66 +199,104 @@ describe('issue_note', () => {
     });
 
     it('should render issue body', () => {
-    const noteBody = wrapper.find(NoteBody);
+      const noteBody = wrapper.findComponent(NoteBody);
       const noteBodyProps = noteBody.props();
 
-    expect(noteBodyProps.note).toEqual(note);
+      expect(noteBodyProps.note).toBe(note);
       expect(noteBodyProps.line).toBe(null);
       expect(noteBodyProps.canEdit).toBe(note.current_user.can_edit);
       expect(noteBodyProps.isEditing).toBe(false);
       expect(noteBodyProps.helpPagePath).toBe('');
     });
 
-  it('prevents note preview xss', (done) => {
-    const imgSrc = 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
-    const noteBody = `<img src="${imgSrc}" onload="alert(1)" />`;
-    const alertSpy = jest.spyOn(window, 'alert');
+    it('prevents note preview xss', async () => {
+      const noteBody =
+        '<img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" onload="alert(1)" />';
+      const alertSpy = jest.spyOn(window, 'alert').mockImplementation(() => {});
+      const noteBodyComponent = wrapper.findComponent(NoteBody);
+
       store.hotUpdate({
         actions: {
           updateNote() {},
           setSelectedCommentPositionHover() {},
         },
       });
-    const noteBodyComponent = wrapper.find(NoteBody);
 
       noteBodyComponent.vm.$emit('handleFormUpdate', noteBody, null, () => {});
 
-    setImmediate(() => {
+      await waitForPromises();
       expect(alertSpy).not.toHaveBeenCalled();
-      expect(wrapper.vm.note.note_html).toEqual(escape(noteBody));
-      done();
+      expect(wrapper.vm.note.note_html).toBe(escape(noteBody));
     });
   });
 
   describe('cancel edit', () => {
-    it('restores content of updated note', (done) => {
+    beforeEach(() => {
+      createWrapper();
+    });
+
+    it('restores content of updated note', async () => {
       const updatedText = 'updated note text';
       store.hotUpdate({
         actions: {
           updateNote() {},
         },
       });
-      const noteBody = wrapper.find(NoteBody);
+      const noteBody = wrapper.findComponent(NoteBody);
       noteBody.vm.resetAutoSave = () => {};
 
       noteBody.vm.$emit('handleFormUpdate', updatedText, null, () => {});
 
-      wrapper.vm
-        .$nextTick()
-        .then(() => {
-          const noteBodyProps = noteBody.props();
+      await wrapper.vm.$nextTick();
+      let noteBodyProps = noteBody.props();
 
       expect(noteBodyProps.note.note_html).toBe(updatedText);
+
       noteBody.vm.$emit('cancelForm');
-        })
-        .then(() => wrapper.vm.$nextTick())
-        .then(() => {
-          const noteBodyProps = noteBody.props();
+      await wrapper.vm.$nextTick();
+
+      noteBodyProps = noteBody.props();
 
       expect(noteBodyProps.note.note_html).toBe(note.note_html);
-        })
-        .then(done)
-        .catch(done.fail);
+    });
+  });
+
+  describe('formUpdateHandler', () => {
+    const updateNote = jest.fn();
+    const params = ['', null, jest.fn(), ''];
+
+    const updateActions = () => {
+      store.hotUpdate({
+        actions: {
+          updateNote,
+          setSelectedCommentPositionHover() {},
+        },
+      });
+    };
+
+    afterEach(() => updateNote.mockReset());
+
+    it('responds to handleFormUpdate', () => {
+      createWrapper();
+      updateActions();
+      wrapper.findComponent(NoteBody).vm.$emit('handleFormUpdate', ...params);
+      expect(wrapper.emitted('handleUpdateNote')).toBeTruthy();
+    });
+
+    it('does not stringify empty position', () => {
+      createWrapper();
+      updateActions();
+      wrapper.findComponent(NoteBody).vm.$emit('handleFormUpdate', ...params);
+      expect(updateNote.mock.calls[0][1].note.note.position).toBeUndefined();
+    });
+
+    it('stringifies populated position', () => {
+      const position = { test: true };
+      const expectation = JSON.stringify(position);
+      createWrapper({ note: { ...note, position } });
+      updateActions();
+      wrapper.findComponent(NoteBody).vm.$emit('handleFormUpdate', ...params);
+      expect(updateNote.mock.calls[0][1].note.note.position).toBe(expectation);
     });
   });
 });