Merge branch 'jdb/fix-js-error-multilinecomment-overview' into 'master'

Fix silent js errors when viewing comments

Closes #229821

See merge request gitlab-org/gitlab!37216

app/assets/javascripts/batch_comments/components/draft_note.vue

@@ -3,6 +3,7 @@ import { mapActions, mapGetters, mapState } from 'vuex';
 import NoteableNote from '~/notes/components/noteable_note.vue';
 import LoadingButton from '~/vue_shared/components/loading_button.vue';
 import PublishButton from './publish_button.vue';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 
 export default {
   components: {
@@ -10,6 +11,7 @@ export default {
     PublishButton,
     LoadingButton,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     draft: {
       type: Object,
@@ -64,14 +66,27 @@ export default {
     handleNotEditing() {
       this.isEditingDraft = false;
     },
+    handleMouseEnter(draft) {
+      if (this.glFeatures.multilineComments && draft.position) {
+        this.setSelectedCommentPositionHover(draft.position.line_range);
+      }
+    },
+    handleMouseLeave(draft) {
+      // Even though position isn't used here we still don't want to unecessarily call a mutation
+      // The lack of position tells us that highlighting is irrelevant in this context
+      if (this.glFeatures.multilineComments && draft.position) {
+        this.setSelectedCommentPositionHover();
+      }
+    },
   },
 };
 </script>
 <template>
   <article
+    role="article"
     class="draft-note-component note-wrapper"
-    @mouseenter="setSelectedCommentPositionHover(draft.position.line_range)"
-    @mouseleave="setSelectedCommentPositionHover()"
+    @mouseenter="handleMouseEnter(draft)"
+    @mouseleave="handleMouseLeave(draft)"
   >
     <ul class="notes draft-notes">
       <noteable-note

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

@@ -9,6 +9,7 @@ import NoteableNote from './noteable_note.vue';
 import ToggleRepliesWidget from './toggle_replies_widget.vue';
 import NoteEditedText from './note_edited_text.vue';
 import DiscussionNotesRepliesWrapper from './discussion_notes_replies_wrapper.vue';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 
 export default {
   name: 'DiscussionNotes',
@@ -17,6 +18,7 @@ export default {
     NoteEditedText,
     DiscussionNotesRepliesWrapper,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     discussion: {
       type: Object,
@@ -93,6 +95,18 @@ export default {
     componentData(note) {
       return note.isPlaceholderNote ? note.notes[0] : note;
     },
+    handleMouseEnter(discussion) {
+      if (this.glFeatures.multilineComments && discussion.position) {
+        this.setSelectedCommentPositionHover(discussion.position.line_range);
+      }
+    },
+    handleMouseLeave(discussion) {
+      // Even though position isn't used here we still don't want to unecessarily call a mutation
+      // The lack of position tells us that highlighting is irrelevant in this context
+      if (this.glFeatures.multilineComments && discussion.position) {
+        this.setSelectedCommentPositionHover();
+      }
+    },
   },
 };
 </script>
@@ -101,8 +115,8 @@ export default {
   <div class="discussion-notes">
     <ul
       class="notes"
-      @mouseenter="setSelectedCommentPositionHover(discussion.position.line_range)"
-      @mouseleave="setSelectedCommentPositionHover()"
+      @mouseenter="handleMouseEnter(discussion)"
+      @mouseleave="handleMouseLeave(discussion)"
     >
       <template v-if="shouldGroupReplies">
         <component

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

@@ -152,9 +152,10 @@ export default {
 
       return this.line && this.startLineNumber !== this.endLineNumber;
     },
+    showMultilineCommentForm() {
+      return Boolean(this.isEditing && this.note.position && this.diffFile && this.line);
+    },
     commentLineOptions() {
-      if (!this.diffFile || !this.line) return [];
-
       const sideA = this.line.type === 'new' ? 'right' : 'left';
       const sideB = sideA === 'left' ? 'right' : 'left';
       const lines = this.diffFile.highlighted_diff_lines.length
@@ -339,7 +340,7 @@ export default {
   >
     <div v-if="showMultiLineComment" data-testid="multiline-comment">
       <multiline-comment-form
-        v-if="isEditing && note.position"
+        v-if="showMultilineCommentForm"
         v-model="commentLineStart"
         :line="line"
         :comment-line-options="commentLineOptions"

changelogs/unreleased/jdb-fix-js-error-multilinecomment-overview.yml

+---
+title: Fix editing note throws js error
+merge_request: 37216
+author:
+type: fixed

spec/frontend/batch_comments/components/draft_note_spec.js

 import { shallowMount, createLocalVue } from '@vue/test-utils';
+import { getByRole } from '@testing-library/dom';
 import DraftNote from '~/batch_comments/components/draft_note.vue';
 import { createStore } from '~/batch_comments/stores';
 import NoteableNote from '~/notes/components/noteable_note.vue';
@@ -8,21 +9,34 @@ import { createDraft } from '../mock_data';
 const localVue = createLocalVue();
 
 describe('Batch comments draft note component', () => {
+  let store;
   let wrapper;
   let draft;
+  const LINE_RANGE = {};
+  const draftWithLineRange = {
+    position: {
+      line_range: LINE_RANGE,
+    },
+  };
 
-  beforeEach(() => {
-    const store = createStore();
-
-    draft = createDraft();
+  const getList = () => getByRole(wrapper.element, 'list');
 
+  const createComponent = (propsData = { draft }, features = {}) => {
     wrapper = shallowMount(localVue.extend(DraftNote), {
       store,
-      propsData: { draft },
+      propsData,
       localVue,
+      provide: {
+        glFeatures: { multilineComments: true, ...features },
+      },
     });
 
     jest.spyOn(wrapper.vm.$store, 'dispatch').mockImplementation();
+  };
+
+  beforeEach(() => {
+    store = createStore();
+    draft = createDraft();
   });
 
   afterEach(() => {
@@ -30,6 +44,7 @@ describe('Batch comments draft note component', () => {
   });
 
   it('renders template', () => {
+    createComponent();
     expect(wrapper.find('.draft-pending-label').exists()).toBe(true);
 
     const note = wrapper.find(NoteableNote);
@@ -40,6 +55,7 @@ describe('Batch comments draft note component', () => {
 
   describe('add comment now', () => {
     it('dispatches publishSingleDraft when clicking', () => {
+      createComponent();
       const publishNowButton = wrapper.find({ ref: 'publishNowButton' });
       publishNowButton.vm.$emit('click');
 
@@ -50,6 +66,7 @@ describe('Batch comments draft note component', () => {
     });
 
     it('sets as loading when draft is publishing', done => {
+      createComponent();
       wrapper.vm.$store.state.batchComments.currentlyPublishingDrafts.push(1);
 
       wrapper.vm.$nextTick(() => {
@@ -64,6 +81,7 @@ describe('Batch comments draft note component', () => {
 
   describe('update', () => {
     it('dispatches updateDraft', done => {
+      createComponent();
       const note = wrapper.find(NoteableNote);
 
       note.vm.$emit('handleEdit');
@@ -91,6 +109,7 @@ describe('Batch comments draft note component', () => {
 
   describe('deleteDraft', () => {
     it('dispatches deleteDraft', () => {
+      createComponent();
       jest.spyOn(window, 'confirm').mockImplementation(() => true);
 
       const note = wrapper.find(NoteableNote);
@@ -103,6 +122,7 @@ describe('Batch comments draft note component', () => {
 
   describe('quick actions', () => {
     it('renders referenced commands', done => {
+      createComponent();
       wrapper.setProps({
         draft: {
           ...draft,
@@ -122,4 +142,26 @@ describe('Batch comments draft note component', () => {
       });
     });
   });
+
+  describe('multiline comments', () => {
+    describe.each`
+      desc                          | props                 | features                        | event           | expectedCalls
+      ${'with `draft.position`'}    | ${draftWithLineRange} | ${{}}                           | ${'mouseenter'} | ${[['setSelectedCommentPositionHover', LINE_RANGE]]}
+      ${'with `draft.position`'}    | ${draftWithLineRange} | ${{}}                           | ${'mouseleave'} | ${[['setSelectedCommentPositionHover']]}
+      ${'with `draft.position`'}    | ${draftWithLineRange} | ${{ multilineComments: false }} | ${'mouseenter'} | ${[]}
+      ${'with `draft.position`'}    | ${draftWithLineRange} | ${{ multilineComments: false }} | ${'mouseleave'} | ${[]}
+      ${'without `draft.position`'} | ${{}}                 | ${{}}                           | ${'mouseenter'} | ${[]}
+      ${'without `draft.position`'} | ${{}}                 | ${{}}                           | ${'mouseleave'} | ${[]}
+    `('$desc and features $features', ({ props, event, features, expectedCalls }) => {
+      beforeEach(() => {
+        createComponent({ draft: { ...draft, ...props } }, features);
+        jest.spyOn(store, 'dispatch');
+      });
+
+      it(`calls store ${expectedCalls.length} times on ${event}`, () => {
+        getList().dispatchEvent(new MouseEvent(event, { bubbles: true }));
+        expect(store.dispatch.mock.calls).toEqual(expectedCalls);
+      });
+    });
+  });
 });

spec/frontend/notes/components/discussion_notes_spec.js

 import { shallowMount } from '@vue/test-utils';
+import { getByRole } from '@testing-library/dom';
 import '~/behaviors/markdown/render_gfm';
 import { SYSTEM_NOTE } from '~/notes/constants';
 import DiscussionNotes from '~/notes/components/discussion_notes.vue';
@@ -9,14 +10,20 @@ import SystemNote from '~/vue_shared/components/notes/system_note.vue';
 import createStore from '~/notes/stores';
 import { noteableDataMock, discussionMock, notesDataMock } from '../mock_data';
 
+const LINE_RANGE = {};
+const DISCUSSION_WITH_LINE_RANGE = {
+  ...discussionMock,
+  position: {
+    line_range: LINE_RANGE,
+  },
+};
+
 describe('DiscussionNotes', () => {
+  let store;
   let wrapper;
 
-  const createComponent = props => {
-    const store = createStore();
-    store.dispatch('setNoteableData', noteableDataMock);
-    store.dispatch('setNotesData', notesDataMock);
-
+  const getList = () => getByRole(wrapper.element, 'list');
+  const createComponent = (props, features = {}) => {
     wrapper = shallowMount(DiscussionNotes, {
       store,
       propsData: {
@@ -31,11 +38,21 @@ describe('DiscussionNotes', () => {
       slots: {
         'avatar-badge': '<span class="avatar-badge-slot-content" />',
       },
+      provide: {
+        glFeatures: { multilineComments: true, ...features },
+      },
     });
   };
 
+  beforeEach(() => {
+    store = createStore();
+    store.dispatch('setNoteableData', noteableDataMock);
+    store.dispatch('setNotesData', notesDataMock);
+  });
+
   afterEach(() => {
     wrapper.destroy();
+    wrapper = null;
   });
 
   describe('rendering', () => {
@@ -160,6 +177,26 @@ describe('DiscussionNotes', () => {
     });
   });
 
+  describe.each`
+    desc                               | props                                         | features                        | event           | expectedCalls
+    ${'with `discussion.position`'}    | ${{ discussion: DISCUSSION_WITH_LINE_RANGE }} | ${{}}                           | ${'mouseenter'} | ${[['setSelectedCommentPositionHover', LINE_RANGE]]}
+    ${'with `discussion.position`'}    | ${{ discussion: DISCUSSION_WITH_LINE_RANGE }} | ${{}}                           | ${'mouseleave'} | ${[['setSelectedCommentPositionHover']]}
+    ${'with `discussion.position`'}    | ${{ discussion: DISCUSSION_WITH_LINE_RANGE }} | ${{ multilineComments: false }} | ${'mouseenter'} | ${[]}
+    ${'with `discussion.position`'}    | ${{ discussion: DISCUSSION_WITH_LINE_RANGE }} | ${{ multilineComments: false }} | ${'mouseleave'} | ${[]}
+    ${'without `discussion.position`'} | ${{}}                                         | ${{}}                           | ${'mouseenter'} | ${[]}
+    ${'without `discussion.position`'} | ${{}}                                         | ${{}}                           | ${'mouseleave'} | ${[]}
+  `('$desc and features $features', ({ props, event, features, expectedCalls }) => {
+    beforeEach(() => {
+      createComponent(props, features);
+      jest.spyOn(store, 'dispatch');
+    });
+
+    it(`calls store ${expectedCalls.length} times on ${event}`, () => {
+      getList().dispatchEvent(new MouseEvent(event));
+      expect(store.dispatch.mock.calls).toEqual(expectedCalls);
+    });
+  });
+
   describe('componentData', () => {
     beforeEach(() => {
       createComponent();

spec/frontend/notes/components/noteable_note_spec.js

@@ -92,8 +92,8 @@ describe('issue_note', () => {
       });
     });
 
-    it('should not render multiline comment form unless it is the discussion root', () => {
-      wrapper.setProps({ discussionRoot: false });
+    it('should only render multiline comment form if it has everything it needs', () => {
+      wrapper.setProps({ line: { line_code: '' } });
       wrapper.vm.isEditing = true;
 
       return wrapper.vm.$nextTick().then(() => {