Merge branch '214595-add-comment-polling' into 'master'

Add polling for vulnerability comments and state

See merge request gitlab-org/gitlab!30420

ee/app/assets/javascripts/pages/projects/security/vulnerabilities/show/index.js

@@ -34,7 +34,7 @@ function createFooterApp() {
     return false;
   }
 
-  const { vulnerabilityFeedbackHelpPath, hasMr, discussionsUrl } = el.dataset;
+  const { vulnerabilityFeedbackHelpPath, hasMr, discussionsUrl, notesUrl } = el.dataset;
   const vulnerability = JSON.parse(el.dataset.vulnerabilityJson);
   const finding = JSON.parse(el.dataset.findingJson);
   const { issue_feedback: feedback, remediation, solution } = finding;
@@ -44,6 +44,7 @@ function createFooterApp() {
 
   const props = {
     discussionsUrl,
+    notesUrl,
     solutionInfo: {
       solution,
       remediation,

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

 <script>
+import Visibility from 'visibilityjs';
 import axios from '~/lib/utils/axios_utils';
+import Poll from '~/lib/utils/poll';
 import createFlash from '~/flash';
-import { s__ } from '~/locale';
+import { s__, __ } from '~/locale';
 import IssueNote from 'ee/vue_shared/security_reports/components/issue_note.vue';
 import SolutionCard from 'ee/vue_shared/security_reports/components/solution_card.vue';
 import HistoryEntry from './history_entry.vue';
@@ -20,6 +22,10 @@ export default {
       required: false,
       default: null,
     },
+    notesUrl: {
+      type: String,
+      required: true,
+    },
     project: {
       type: Object,
       required: true,
@@ -31,10 +37,22 @@ export default {
   },
 
   data: () => ({
-    discussions: [],
+    discussionsDictionary: {},
+    lastFetchedAt: null,
   }),
 
   computed: {
+    discussions() {
+      return Object.values(this.discussionsDictionary);
+    },
+    noteDictionary() {
+      return this.discussions
+        .flatMap(x => x.notes)
+        .reduce((acc, note) => {
+          acc[note.id] = note;
+          return acc;
+        }, {});
+    },
     hasIssue() {
       return Boolean(this.feedback?.issue_iid);
     },
@@ -49,12 +67,38 @@ export default {
     VulnerabilitiesEventBus.$on('VULNERABILITY_STATE_CHANGE', this.fetchDiscussions);
   },
 
+  beforeDestroy() {
+    if (this.poll) this.poll.stop();
+  },
+
   methods: {
+    dateToSeconds(date) {
+      return Date.parse(date) / 1000;
+    },
     fetchDiscussions() {
       axios
         .get(this.discussionsUrl)
-        .then(({ data }) => {
-          this.discussions = data;
+        .then(({ data, headers: { date } }) => {
+          this.discussionsDictionary = data.reduce((acc, discussion) => {
+            acc[discussion.id] = discussion;
+            return acc;
+          }, {});
+
+          this.lastFetchedAt = this.dateToSeconds(date);
+
+          if (!this.poll) this.createNotesPoll();
+
+          if (!Visibility.hidden()) {
+            this.poll.makeRequest();
+          }
+
+          Visibility.change(() => {
+            if (Visibility.hidden()) {
+              this.poll.stop();
+            } else {
+              this.poll.restart();
+            }
+          });
         })
         .catch(() => {
           createFlash(
@@ -64,6 +108,48 @@ export default {
           );
         });
     },
+    createNotesPoll() {
+      this.poll = new Poll({
+        resource: {
+          fetchNotes: () =>
+            axios.get(this.notesUrl, { headers: { 'X-Last-Fetched-At': this.lastFetchedAt } }),
+        },
+        method: 'fetchNotes',
+        successCallback: ({ data: { notes, last_fetched_at: lastFetchedAt } }) => {
+          this.updateNotes(notes);
+          this.lastFetchedAt = lastFetchedAt;
+        },
+        errorCallback: () =>
+          createFlash(__('Something went wrong while fetching latest comments.')),
+      });
+    },
+    updateNotes(notes) {
+      notes.forEach(note => {
+        // If the note exists, update it.
+        if (this.noteDictionary[note.id]) {
+          const updatedDiscussion = { ...this.discussionsDictionary[note.discussion_id] };
+          updatedDiscussion.notes = updatedDiscussion.notes.map(curr =>
+            curr.id === note.id ? note : curr,
+          );
+          this.discussionsDictionary[note.discussion_id] = updatedDiscussion;
+        }
+        // If the note doesn't exist, but the discussion does, add the note to the discussion.
+        else if (this.discussionsDictionary[note.discussion_id]) {
+          const updatedDiscussion = { ...this.discussionsDictionary[note.discussion_id] };
+          updatedDiscussion.notes.push(note);
+          this.discussionsDictionary[note.discussion_id] = updatedDiscussion;
+        }
+        // If the discussion doesn't exist, create it.
+        else {
+          const newDiscussion = {
+            id: note.discussion_id,
+            reply_id: note.discussion_id,
+            notes: [note],
+          };
+          this.$set(this.discussionsDictionary, newDiscussion.id, newDiscussion);
+        }
+      });
+    },
   },
 };
 </script>
@@ -80,6 +166,7 @@ export default {
         v-for="discussion in discussions"
         :key="discussion.id"
         :discussion="discussion"
+        :notes-url="notesUrl"
       />
     </ul>
   </div>

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

@@ -27,6 +27,10 @@ export default {
       required: false,
       default: undefined,
     },
+    notesUrl: {
+      type: String,
+      required: true,
+    },
   },
 
   data() {
@@ -39,6 +43,9 @@ export default {
   },
 
   computed: {
+    noteIdUrl() {
+      return joinPaths(this.notesUrl, this.comment.id);
+    },
     commentNote() {
       return this.comment?.note;
     },
@@ -65,14 +72,12 @@ export default {
     getSaveConfig(note) {
       const isUpdatingComment = Boolean(this.comment);
       const method = isUpdatingComment ? 'put' : 'post';
-      let url = joinPaths(window.location.pathname, 'notes');
+      const url = isUpdatingComment ? this.noteIdUrl : this.notesUrl;
       const data = { note: { note } };
       const emitName = isUpdatingComment ? 'onCommentUpdated' : 'onCommentAdded';
 
-      // If we're updating the comment, use the comment ID in the URL. Otherwise, use the discussion ID in the request data.
-      if (isUpdatingComment) {
-        url = joinPaths(url, this.comment.id);
-      } else {
+      // If we're saving a new comment, use the discussion ID in the request data.
+      if (!isUpdatingComment) {
         data.in_reply_to_discussion_id = this.discussionId;
       }
 
@@ -100,7 +105,7 @@ export default {
     },
     deleteComment() {
       this.isDeletingComment = true;
-      const deleteUrl = joinPaths(window.location.pathname, 'notes', this.comment.id);
+      const deleteUrl = this.noteIdUrl;
 
       axios
         .delete(deleteUrl)

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

@@ -9,6 +9,10 @@ export default {
       type: Object,
       required: true,
     },
+    notesUrl: {
+      type: String,
+      required: true,
+    },
   },
   data() {
     return {
@@ -23,6 +27,11 @@ export default {
       return this.notes.filter(x => x !== this.systemNote);
     },
   },
+  watch: {
+    discussion(newDiscussion) {
+      this.notes = newDiscussion.notes;
+    },
+  },
   methods: {
     addComment(comment) {
       this.notes.push(comment);
@@ -66,6 +75,7 @@ export default {
         ref="existingComment"
         :comment="comment"
         :discussion-id="discussion.reply_id"
+        :notes-url="notesUrl"
         @onCommentUpdated="updateComment"
         @onCommentDeleted="removeComment"
       />
@@ -75,6 +85,7 @@ export default {
       v-else
       ref="newComment"
       :discussion-id="discussion.reply_id"
+      :notes-url="notesUrl"
       @onCommentAdded="addComment"
     />
   </li>

ee/spec/frontend/vulnerabilities/footer_spec.js

@@ -31,6 +31,7 @@ describe('Vulnerability Footer', () => {
       url: '/root/security-reports',
       value: 'Administrator / Security Reports',
     },
+    notesUrl: '/notes',
   };
 
   const solutionInfoProp = {
@@ -123,7 +124,7 @@ describe('Vulnerability Footer', () => {
       // The shape of this object doesn't matter for this test, we just need to verify that it's passed to the history
       // entry.
       const historyItems = [{ id: 1, note: 'some note' }, { id: 2, note: 'another note' }];
-      mockAxios.onGet(discussionUrl).replyOnce(200, historyItems);
+      mockAxios.onGet(discussionUrl).replyOnce(200, historyItems, { date: Date.now() });
       createWrapper();
 
       return axios.waitForAll().then(() => {
@@ -144,5 +145,71 @@ describe('Vulnerability Footer', () => {
         expect(createFlash).toHaveBeenCalledTimes(1);
       });
     });
+
+    describe('new notes polling', () => {
+      const getDiscussion = (entries, index) => entries.at(index).props('discussion');
+      const createNotesRequest = note =>
+        mockAxios
+          .onGet(minimumProps.notesUrl)
+          .replyOnce(200, { notes: [note], last_fetched_at: Date.now() });
+
+      beforeEach(() => {
+        const historyItems = [
+          { id: 1, notes: [{ id: 100, note: 'some note', discussion_id: 1 }] },
+          { id: 2, notes: [{ id: 200, note: 'another note', discussion_id: 2 }] },
+        ];
+        mockAxios.onGet(discussionUrl).replyOnce(200, historyItems, { date: Date.now() });
+        createWrapper();
+      });
+
+      it('updates an existing note if it already exists', () => {
+        const note = { id: 100, note: 'updated note', discussion_id: 1 };
+        createNotesRequest(note);
+
+        return axios.waitForAll().then(() => {
+          const entries = historyEntries();
+          expect(entries).toHaveLength(2);
+          const discussion = getDiscussion(entries, 0);
+          expect(discussion.notes.length).toBe(1);
+          expect(discussion.notes[0].note).toBe('updated note');
+        });
+      });
+
+      it('adds a new note to an existing discussion if the note does not exist', () => {
+        const note = { id: 101, note: 'new note', discussion_id: 1 };
+        createNotesRequest(note);
+
+        return axios.waitForAll().then(() => {
+          const entries = historyEntries();
+          expect(entries).toHaveLength(2);
+          const discussion = getDiscussion(entries, 0);
+          expect(discussion.notes.length).toBe(2);
+          expect(discussion.notes[1].note).toBe('new note');
+        });
+      });
+
+      it('creates a new discussion with a new note if the discussion does not exist', () => {
+        const note = { id: 300, note: 'new note on a new discussion', discussion_id: 3 };
+        createNotesRequest(note);
+
+        return axios.waitForAll().then(() => {
+          const entries = historyEntries();
+          expect(entries).toHaveLength(3);
+          const discussion = getDiscussion(entries, 2);
+          expect(discussion.notes.length).toBe(1);
+          expect(discussion.notes[0].note).toBe('new note on a new discussion');
+        });
+      });
+
+      it('shows an error if the notes poll fails', () => {
+        mockAxios.onGet(minimumProps.notesUrl).replyOnce(500);
+
+        return axios.waitForAll().then(() => {
+          expect(historyEntries()).toHaveLength(2);
+          expect(mockAxios.history.get).toHaveLength(2);
+          expect(createFlash).toHaveBeenCalled();
+        });
+      });
+    });
   });
 });

ee/spec/frontend/vulnerabilities/history_comment_spec.js

@@ -14,7 +14,10 @@ describe('History Comment', () => {
 
   const createWrapper = comment => {
     wrapper = mount(HistoryComment, {
-      propsData: { comment },
+      propsData: {
+        comment,
+        notesUrl: '/notes',
+      },
     });
   };
 

ee/spec/frontend/vulnerabilities/history_entry_spec.js

@@ -31,7 +31,10 @@ describe('History Entry', () => {
     const discussion = { notes };
 
     wrapper = shallowMount(HistoryEntry, {
-      propsData: { discussion },
+      propsData: {
+        discussion,
+        notesUrl: '/notes',
+      },
       stubs: { EventItem },
     });
   };