Merge branch '228742-use-graphql-to-fetch-notes' into 'master'

Fetch notes along discussions using GraphQL

See merge request gitlab-org/gitlab!69353

ee/app/assets/javascripts/security_dashboard/graphql/queries/vulnerability_discussions.query.graphql

+#import "../fragments/note.fragment.graphql"
+
 query vulnerabilityDiscussions(
   $id: VulnerabilityID!
   $after: String
@@ -11,6 +13,11 @@ query vulnerabilityDiscussions(
       nodes {
         id
         replyId
+        notes {
+          nodes {
+            ...SecurityDashboardNote
+          }
+        }
       }
     }
   }

ee/app/assets/javascripts/vulnerabilities/components/footer.vue

@@ -9,18 +9,18 @@ import { VULNERABILITY_STATE_OBJECTS } from 'ee/vulnerabilities/constants';
 import createFlash from '~/flash';
 import { TYPE_VULNERABILITY } from '~/graphql_shared/constants';
 import { convertToGraphQLId } from '~/graphql_shared/utils';
-import axios from '~/lib/utils/axios_utils';
-import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
-import Poll from '~/lib/utils/poll';
-import { s__, __ } from '~/locale';
+import { s__ } from '~/locale';
 import initUserPopovers from '~/user_popovers';
 import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
+import { normalizeGraphQLNote } from '../helpers';
 import GenericReportSection from './generic_report/report_section.vue';
 import HistoryEntry from './history_entry.vue';
 import RelatedIssues from './related_issues.vue';
 import RelatedJiraIssues from './related_jira_issues.vue';
 import StatusDescription from './status_description.vue';
 
+const TEN_SECONDS = 10000;
+
 export default {
   name: 'VulnerabilityFooter',
   components: {
@@ -48,9 +48,9 @@ export default {
   },
   data() {
     return {
-      notesLoading: true,
+      discussionsLoading: true,
       discussions: [],
-      lastFetchedAt: null,
+      lastFetchedDiscussionIndex: -1,
     };
   },
   apollo: {
@@ -60,49 +60,24 @@ export default {
         return { id: convertToGraphQLId(TYPE_VULNERABILITY, this.vulnerability.id) };
       },
       update: ({ vulnerability }) => {
-        if (!vulnerability) {
-          return [];
-        }
-
-        return vulnerability.discussions.nodes.map((d) => ({ ...d, notes: [] }));
+        return (
+          vulnerability?.discussions?.nodes.map((discussion) => ({
+            ...discussion,
+            notes: discussion.notes.nodes.map(normalizeGraphQLNote),
+          })) || []
+        );
       },
-      result({ error }) {
-        if (!this.poll && !error) {
-          this.createNotesPoll();
-
-          if (!Visibility.hidden()) {
-            this.fetchDiscussions();
-          }
-
-          Visibility.change(() => {
-            if (Visibility.hidden()) {
-              this.poll.stop();
-            } else {
-              this.poll.restart();
-            }
-          });
-        }
+      result() {
+        this.discussionsLoading = false;
+        this.notifyHeaderForStateChangeIfRequired();
+        this.startPolling();
       },
       error() {
-        this.notesLoading = false;
-
-        createFlash({
-          message: s__(
-            'VulnerabilityManagement|Something went wrong while trying to retrieve the vulnerability history. Please try again later.',
-          ),
-        });
+        this.showGraphQLError();
       },
     },
   },
   computed: {
-    noteDictionary() {
-      return this.discussions
-        .flatMap((x) => x.notes)
-        .reduce((acc, note) => {
-          acc[note.id] = note;
-          return acc;
-        }, {});
-    },
     project() {
       return {
         url: this.vulnerability.project.fullPath,
@@ -137,79 +112,74 @@ export default {
       };
     },
   },
+  beforeDestroy() {
+    this.stopPolling();
+  },
   updated() {
     this.$nextTick(() => {
       initUserPopovers(this.$el.querySelectorAll('.js-user-link'));
     });
   },
-  beforeDestroy() {
-    if (this.poll) {
-      this.poll.stop();
-    }
-  },
   methods: {
-    fetchDiscussions() {
-      return this.poll.makeRequest();
+    startPolling() {
+      if (this.pollInterval) {
+        return;
+      }
+
+      if (!Visibility.hidden()) {
+        this.pollInterval = setInterval(this.fetchDiscussions, TEN_SECONDS);
+      }
+
+      this.visibilityListener = Visibility.change(() => {
+        if (Visibility.hidden()) {
+          this.stopPolling();
+        } else {
+          this.startPolling();
+        }
+      });
     },
-    findDiscussion(id) {
-      return this.discussions.find((d) => d.id === id);
+    stopPolling() {
+      if (typeof this.pollInterval !== 'undefined') {
+        clearInterval(this.pollInterval);
+        this.pollInterval = undefined;
+      }
+
+      if (typeof this.visibilityListener !== 'undefined') {
+        Visibility.unbind(this.visibilityListener);
+        this.visibilityListener = undefined;
+      }
     },
-    createNotesPoll() {
-      // note: this polling call will be replaced when migrating the vulnerability details page to GraphQL
-      // related epic: https://gitlab.com/groups/gitlab-org/-/epics/3657
-      this.poll = new Poll({
-        resource: {
-          fetchNotes: () =>
-            axios.get(this.vulnerability.notesUrl, {
-              headers: { 'X-Last-Fetched-At': this.lastFetchedAt },
-            }),
-        },
-        method: 'fetchNotes',
-        successCallback: ({ data: { notes, last_fetched_at: lastFetchedAt } }) => {
-          this.updateNotes(convertObjectPropsToCamelCase(notes, { deep: true }));
-          this.lastFetchedAt = lastFetchedAt;
-          this.notesLoading = false;
-        },
-        errorCallback: () => {
-          this.notesLoading = false;
-          createFlash({
-            message: __('Something went wrong while fetching latest comments.'),
-          });
-        },
+    showGraphQLError() {
+      createFlash({
+        message: s__(
+          'VulnerabilityManagement|Something went wrong while trying to retrieve the vulnerability history. Please try again later.',
+        ),
       });
     },
-    updateNotes(notes) {
-      let shallEmitVulnerabilityChangedEvent;
+    notifyHeaderForStateChangeIfRequired() {
+      const lastItemIndex = this.discussions.length - 1;
 
-      notes.forEach((note) => {
-        const discussion = this.findDiscussion(note.discussionId);
-        // If the note exists, update it.
-        if (this.noteDictionary[note.id]) {
-          discussion.notes = discussion.notes.map((curr) => (curr.id === note.id ? note : curr));
-        }
-        // If the note doesn't exist, but the discussion does, add the note to the discussion.
-        else if (discussion) {
-          discussion.notes.push(note);
-        }
-        // If the discussion doesn't exist, create it.
-        else {
-          this.discussions.push({
-            id: note.discussionId,
-            replyId: note.discussionId,
-            notes: [note],
-          });
-
-          // If the vulnerability status has changed, the note will be a system note.
-          // Emit an event that tells the header to refresh the vulnerability.
-          if (note.system === true) {
-            shallEmitVulnerabilityChangedEvent = true;
-          }
-        }
-      });
+      if (this.lastFetchedDiscussionIndex === lastItemIndex) {
+        return;
+      }
 
-      if (shallEmitVulnerabilityChangedEvent) {
+      // Do not notify on page load, or first mount.
+      if (this.lastFetchedDiscussionIndex !== -1) {
         this.$emit('vulnerability-state-change');
       }
+
+      this.lastFetchedDiscussionIndex = lastItemIndex;
+    },
+    async fetchDiscussions(callback) {
+      try {
+        await this.$apollo.queries.discussions.refetch();
+
+        if (typeof callback === 'function') {
+          callback();
+        }
+      } catch {
+        this.showGraphQLError();
+      }
     },
   },
 };
@@ -250,13 +220,14 @@ export default {
       </div>
     </div>
     <hr />
-    <gl-loading-icon v-if="notesLoading" />
-    <ul v-else-if="discussions.length" class="notes discussion-body">
+    <gl-loading-icon v-if="discussionsLoading" />
+    <div v-else-if="discussions.length" class="notes discussion-body">
       <history-entry
         v-for="discussion in discussions"
         :key="discussion.id"
         :discussion="discussion"
+        @onCommentUpdated="fetchDiscussions"
       />
-    </ul>
+    </div>
   </div>
 </template>

ee/app/assets/javascripts/vulnerabilities/components/history_comment.vue

@@ -8,7 +8,6 @@ import createFlash from '~/flash';
 import { TYPE_NOTE, TYPE_DISCUSSION, TYPE_VULNERABILITY } from '~/graphql_shared/constants';
 import { convertToGraphQLId } from '~/graphql_shared/utils';
 import { __, s__ } from '~/locale';
-import { normalizeGraphQLNote } from '../helpers';
 import HistoryCommentEditor from './history_comment_editor.vue';
 
 export default {
@@ -63,21 +62,13 @@ export default {
       ];
     },
     initialComment() {
-      return this.comment?.note;
+      return this.comment?.body;
     },
     canEditComment() {
-      return this.comment.currentUser?.canEdit;
+      return this.comment.userPermissions?.adminNote;
     },
     noteHtml() {
-      return this.isSavingComment ? undefined : this.comment.noteHtml;
-    },
-  },
-
-  watch: {
-    'comment.updatedAt': {
-      handler() {
-        this.isSavingComment = false;
-      },
+      return this.isSavingComment ? undefined : this.comment.bodyHtml;
     },
   },
 
@@ -95,13 +86,11 @@ export default {
         },
       });
 
-      const { note, errors } = data.createNote;
+      const { errors } = data.createNote;
 
       if (errors?.length > 0) {
         throw errors;
       }
-
-      this.$emit('onCommentAdded', normalizeGraphQLNote(note));
     },
     async updateComment(body) {
       const { data } = await this.$apollo.mutate({
@@ -112,14 +101,11 @@ export default {
         },
       });
 
-      const { note, errors } = data.updateNote;
+      const { errors } = data.updateNote;
 
       if (errors?.length > 0) {
         throw errors;
       }
-
-      this.cancelEditingComment();
-      this.$emit('onCommentUpdated', normalizeGraphQLNote(note));
     },
     async saveComment(body) {
       this.isSavingComment = true;
@@ -131,15 +117,20 @@ export default {
         } else {
           await this.insertComment(body);
         }
+
+        this.$emit('onCommentUpdated', () => {
+          this.isSavingComment = false;
+          this.cancelEditingComment();
+        });
       } catch {
+        this.isSavingComment = false;
+
         createFlash({
           message: s__(
             'VulnerabilityManagement|Something went wrong while trying to save the comment. Please try again later.',
           ),
         });
       }
-
-      this.isSavingComment = false;
     },
     async deleteComment() {
       this.isDeletingComment = true;
@@ -156,16 +147,18 @@ export default {
           throw data.errors;
         }
 
-        this.$emit('onCommentDeleted', this.comment);
+        this.$emit('onCommentUpdated', () => {
+          this.isDeletingComment = false;
+        });
       } catch {
+        this.isDeletingComment = false;
+
         createFlash({
           message: s__(
             'VulnerabilityManagement|Something went wrong while trying to delete the comment. Please try again later.',
           ),
         });
       }
-
-      this.isDeletingComment = false;
     },
     cancelEditingComment() {
       this.isEditingComment = false;

ee/app/assets/javascripts/vulnerabilities/components/history_entry.vue

@@ -3,19 +3,20 @@ import EventItem from 'ee/vue_shared/security_reports/components/event_item.vue'
 import HistoryComment from './history_comment.vue';
 
 export default {
-  components: { EventItem, HistoryComment },
+  components: {
+    EventItem,
+    HistoryComment,
+  },
   props: {
     discussion: {
       type: Object,
       required: true,
     },
   },
-  data() {
-    return {
-      notes: this.discussion.notes,
-    };
-  },
   computed: {
+    notes() {
+      return this.discussion.notes;
+    },
     systemNote() {
       return this.notes.find((x) => x.system === true);
     },
@@ -23,35 +24,11 @@ export default {
       return this.notes.filter((x) => x !== this.systemNote);
     },
   },
-  watch: {
-    discussion(newDiscussion) {
-      this.notes = newDiscussion.notes;
-    },
-  },
-  methods: {
-    addComment(note) {
-      this.notes.push(note);
-    },
-    updateComment(note) {
-      const index = this.notes.findIndex((n) => Number(n.id) === note.id);
-
-      if (index > -1) {
-        this.notes.splice(index, 1, note);
-      }
-    },
-    removeComment(comment) {
-      const index = this.notes.indexOf(comment);
-
-      if (index > -1) {
-        this.notes.splice(index, 1);
-      }
-    },
-  },
 };
 </script>
 
 <template>
-  <li v-if="systemNote" class="card border-bottom system-note p-0">
+  <div v-if="systemNote" class="card border-bottom system-note p-0">
     <event-item
       :id="systemNote.id"
       :author="systemNote.author"
@@ -60,27 +37,22 @@ export default {
       icon-class="timeline-icon m-0"
       class="m-3"
     >
-      <template #header-message>{{ systemNote.note }}</template>
+      <template #header-message>{{ systemNote.body }}</template>
     </event-item>
-
-    <template v-if="comments.length" ref="existingComments">
-      <hr class="m-3" />
-      <history-comment
-        v-for="comment in comments"
-        :key="comment.id"
-        ref="existingComment"
-        :comment="comment"
-        :discussion-id="discussion.replyId"
-        @onCommentUpdated="updateComment"
-        @onCommentDeleted="removeComment"
-      />
-    </template>
-
+    <hr v-if="comments.length" class="gl-m-0" />
+    <history-comment
+      v-for="comment in comments"
+      ref="existingComment"
+      :key="comment.id"
+      :comment="comment"
+      :discussion-id="discussion.replyId"
+      v-on="$listeners"
+    />
     <history-comment
-      v-else
+      v-if="!comments.length"
       ref="newComment"
       :discussion-id="discussion.replyId"
-      @onCommentAdded="addComment"
+      v-on="$listeners"
     />
-  </li>
+  </div>
 </template>

ee/app/assets/javascripts/vulnerabilities/helpers.js

@@ -37,11 +37,6 @@ export const normalizeGraphQLNote = (note) => {
   return {
     ...note,
     id: getIdFromGraphQLId(note.id),
-    note: note.body,
-    noteHtml: note.bodyHtml,
-    currentUser: {
-      canEdit: note.userPermissions?.adminNote,
-    },
     author: {
       ...note.author,
       id: getIdFromGraphQLId(note.author.id),

ee/spec/frontend/vulnerabilities/history_comment_spec.js

@@ -12,6 +12,7 @@ import waitForPromises from 'helpers/wait_for_promises';
 import createFlash from '~/flash';
 import { TYPE_DISCUSSION, TYPE_VULNERABILITY } from '~/graphql_shared/constants';
 import { convertToGraphQLId } from '~/graphql_shared/utils';
+import { generateNote } from './mock_data';
 
 jest.mock('~/flash');
 Vue.use(VueApollo);
@@ -59,28 +60,7 @@ describe('History Comment', () => {
     });
   };
 
-  const note = {
-    id: 'gid://gitlab/DiscussionNote/1295',
-    body: 'Created a note.',
-    bodyHtml: '\u003cp\u003eCreated a note\u003c/p\u003e',
-    updatedAt: '2021-08-25T16:21:18Z',
-    system: false,
-    systemNoteIconName: null,
-    userPermissions: {
-      adminNote: true,
-    },
-    author: {
-      id: 'gid://gitlab/User/1',
-      name: 'Administrator',
-      username: 'root',
-      webPath: '/root',
-    },
-  };
-
-  // Needed for now. Will be removed when fetching notes will be done through GraphQL.
-  note.note = note.body;
-  note.noteHtml = note.bodyHtml;
-  note.currentUser = { canEdit: note.userPermissions.adminNote };
+  const note = generateNote();
 
   beforeEach(() => {
     createNoteMutationSpy = jest
@@ -95,8 +75,8 @@ describe('History Comment', () => {
   });
 
   const addCommentButton = () => wrapper.find({ ref: 'addCommentButton' });
-  const commentEditor = () => wrapper.find(HistoryCommentEditor);
-  const eventItem = () => wrapper.find(EventItem);
+  const commentEditor = () => wrapper.findComponent(HistoryCommentEditor);
+  const eventItem = () => wrapper.findComponent(EventItem);
   const editButton = () => wrapper.find('[title="Edit Comment"]');
   const deleteButton = () => wrapper.find('[title="Delete Comment"]');
   const confirmDeleteButton = () => wrapper.find({ ref: 'confirmDeleteButton' });
@@ -228,10 +208,10 @@ describe('History Comment', () => {
   };
 
   describe.each`
-    desc                           | propsData            | expectedEvent         | expectedVars            | mutationSpyFn                  | queryName
-    ${'inserting a new note'}      | ${{}}                | ${'onCommentAdded'}   | ${EXPECTED_CREATE_VARS} | ${() => createNoteMutationSpy} | ${CREATE_NOTE}
-    ${'updating an existing note'} | ${{ comment: note }} | ${'onCommentUpdated'} | ${EXPECTED_UPDATE_VARS} | ${() => updateNoteMutationSpy} | ${UPDATE_NOTE}
-  `('$desc', ({ propsData, expectedEvent, expectedVars, mutationSpyFn, queryName }) => {
+    desc                           | propsData            | expectedVars            | mutationSpyFn                  | queryName
+    ${'inserting a new note'}      | ${{}}                | ${EXPECTED_CREATE_VARS} | ${() => createNoteMutationSpy} | ${CREATE_NOTE}
+    ${'updating an existing note'} | ${{ comment: note }} | ${EXPECTED_UPDATE_VARS} | ${() => updateNoteMutationSpy} | ${UPDATE_NOTE}
+  `('$desc', ({ propsData, expectedVars, mutationSpyFn, queryName }) => {
     let mutationSpy;
 
     beforeEach(() => {
@@ -258,25 +238,19 @@ describe('History Comment', () => {
       expect(commentEditor().props('isSaving')).toBe(true);
     });
 
-    it('emits event when mutation is successful', async () => {
+    it('emits event when mutation is successful with a callback function that resets the state', async () => {
       createWrapper({ propsData });
 
+      const listener = jest.fn().mockImplementation((callback) => callback());
+      wrapper.vm.$on('onCommentUpdated', listener);
+
       await editAndSaveNewContent('new comment');
+      expect(commentEditor().props('isSaving')).toBe(true);
       await waitForPromises();
 
-      expect(wrapper.emitted(expectedEvent)).toEqual([
-        [
-          {
-            ...note,
-            id: 1295,
-            author: {
-              ...note.author,
-              id: 1,
-              path: note.author.webPath,
-            },
-          },
-        ],
-      ]);
+      expect(wrapper.emitted('onCommentUpdated')).toEqual([[expect.any(Function)]]);
+      expect(listener).toHaveBeenCalled();
+      expect(commentEditor().exists()).toBe(false);
     });
 
     describe('when mutation has data error', () => {
@@ -316,7 +290,7 @@ describe('History Comment', () => {
   });
 
   describe('deleting a note', () => {
-    it('deletes the comment when the confirm delete button is clicked', async () => {
+    it('deletes the comment when the confirm delete button is clicked and submits an event to refect the discussions', async () => {
       createWrapper({
         propsData: { comment: note },
       });
@@ -331,8 +305,8 @@ describe('History Comment', () => {
       expect(cancelDeleteButton().props('disabled')).toBe(true);
 
       await waitForPromises();
-      expect(wrapper.emitted().onCommentDeleted).toBeTruthy();
-      expect(wrapper.emitted().onCommentDeleted[0][0]).toEqual(note);
+      expect(wrapper.emitted().onCommentUpdated).toBeTruthy();
+      expect(wrapper.emitted().onCommentUpdated[0][0]).toEqual(expect.any(Function));
     });
 
     it('sends mutation to delete note', async () => {
@@ -383,7 +357,7 @@ describe('History Comment', () => {
     it('does not show the edit/delete buttons if the current user has no edit permissions', () => {
       createWrapper({
         propsData: {
-          comment: { ...note, userPermissions: undefined, currentUser: { canEdit: false } },
+          comment: { ...note, userPermissions: { adminNote: false } },
         },
       });
 

ee/spec/frontend/vulnerabilities/history_entry_spec.js

@@ -8,7 +8,7 @@ describe('History Entry', () => {
   const systemNote = {
     system: true,
     id: 1,
-    note: 'changed vulnerability status to dismissed',
+    body: 'changed vulnerability status to dismissed',
     systemNoteIconName: 'cancel',
     updatedAt: new Date().toISOString(),
     author: {
@@ -20,11 +20,8 @@ describe('History Entry', () => {
 
   const commentNote = {
     id: 2,
-    note: 'some note',
+    body: 'some note',
     author: {},
-    currentUser: {
-      canEdit: true,
-    },
   };
 
   const createWrapper = (...notes) => {
@@ -33,7 +30,6 @@ describe('History Entry', () => {
     wrapper = shallowMount(HistoryEntry, {
       propsData: {
         discussion,
-        notesUrl: '/notes',
       },
       stubs: { EventItem },
     });
@@ -49,7 +45,7 @@ describe('History Entry', () => {
   it('passes the expected values to the event item component', () => {
     createWrapper(systemNote);
 
-    expect(eventItem().text()).toContain(systemNote.note);
+    expect(eventItem().text()).toContain(systemNote.body);
     expect(eventItem().props()).toMatchObject({
       id: systemNote.id,
       author: systemNote.author,
@@ -80,33 +76,4 @@ describe('History Entry', () => {
     expect(commentAt(0).props('comment')).toEqual(commentNote);
     expect(commentAt(1).props('comment')).toEqual(commentNoteClone);
   });
-
-  it('adds a new comment correctly', async () => {
-    createWrapper(systemNote);
-    newComment().vm.$emit('onCommentAdded', commentNote);
-
-    await wrapper.vm.$nextTick();
-
-    expect(newComment().exists()).toBe(false);
-    expect(existingComments()).toHaveLength(1);
-    expect(commentAt(0).props('comment')).toEqual(commentNote);
-  });
-
-  it('updates an existing comment correctly', async () => {
-    const updatedNote = { ...commentNote, note: 'new note' };
-    createWrapper(systemNote, commentNote);
-    commentAt(0).vm.$emit('onCommentUpdated', updatedNote);
-
-    await wrapper.vm.$nextTick();
-
-    expect(commentAt(0).props('comment')).toBe(updatedNote);
-  });
-
-  it('deletes an existing comment correctly', async () => {
-    createWrapper(systemNote, commentNote);
-    await commentAt(0).vm.$emit('onCommentDeleted', commentNote);
-
-    expect(newComment().exists()).toBe(true);
-    expect(existingComments()).toHaveLength(0);
-  });
 });

ee/spec/frontend/vulnerabilities/mock_data.js

+export const generateNote = ({ id = 1295 } = {}) => ({
+  id: `gid://gitlab/DiscussionNote/${id}`,
+  body: 'Created a note.',
+  bodyHtml: '\u003cp\u003eCreated a note\u003c/p\u003e',
+  updatedAt: '2021-08-25T16:21:18Z',
+  system: false,
+  systemNoteIconName: null,
+  userPermissions: {
+    adminNote: true,
+  },
+  author: {
+    id: 'gid://gitlab/User/1',
+    name: 'Administrator',
+    username: 'root',
+    webPath: '/root',
+  },
+});