Merge branch 'fix-duplicate-notes' into 'master'

Fixed issue notes being duplicated

Closes #44099

See merge request gitlab-org/gitlab-ce!17671

Author: Filipa Lacerda

app/assets/javascripts/notes/services/notes_service.js

@@ -27,10 +27,11 @@ export default {
     return Vue.http[method](endpoint);
   },
   poll(data = {}) {
-    const { endpoint, lastFetchedAt } = data;
+    const endpoint = data.notesData.notesPath;
+    const lastFetchedAt = data.lastFetchedAt;
     const options = {
       headers: {
-        'X-Last-Fetched-At': lastFetchedAt,
+        'X-Last-Fetched-At': lastFetchedAt ? `${lastFetchedAt}` : undefined,
       },
     };
 

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

@@ -198,18 +198,16 @@ const pollSuccessCallBack = (resp, commit, state, getters) => {
     });
   }
 
-  commit(types.SET_LAST_FETCHED_AT, resp.lastFetchedAt);
+  commit(types.SET_LAST_FETCHED_AT, resp.last_fetched_at);
 
   return resp;
 };
 
 export const poll = ({ commit, state, getters }) => {
-  const requestData = { endpoint: state.notesData.notesPath, lastFetchedAt: state.lastFetchedAt };
-
   eTagPoll = new Poll({
     resource: service,
     method: 'poll',
-    data: requestData,
+    data: state,
     successCallback: resp => resp.json()
       .then(data => pollSuccessCallBack(data, commit, state, getters)),
     errorCallback: () => Flash('Something went wrong while fetching latest comments.'),
@@ -218,7 +216,7 @@ export const poll = ({ commit, state, getters }) => {
   if (!Visibility.hidden()) {
     eTagPoll.makeRequest();
   } else {
-    service.poll(requestData);
+    service.poll(state);
   }
 
   Visibility.change(() => {

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

@@ -90,19 +90,21 @@ export default {
     const notes = [];
 
     notesData.forEach((note) => {
-      const nn = Object.assign({}, note);
-
       // To support legacy notes, should be very rare case.
       if (note.individual_note && note.notes.length > 1) {
         note.notes.forEach((n) => {
-          nn.notes = [n]; // override notes array to only have one item to mimick individual_note
-          notes.push(nn);
+          notes.push({
+            ...note,
+            notes: [n], // override notes array to only have one item to mimick individual_note
+          });
         });
       } else {
         const oldNote = utils.findNoteObjectById(state.notes, note.id);
-        nn.expanded = oldNote ? oldNote.expanded : note.expanded;
 
-        notes.push(nn);
+        notes.push({
+          ...note,
+          expanded: (oldNote ? oldNote.expanded : note.expanded),
+        });
       }
     });
 

app/helpers/notes_helper.rb

@@ -169,7 +169,7 @@ def notes_data(issuable)
       reopenPath: reopen_issuable_path(issuable),
       notesPath: notes_url,
       totalNotes: issuable.discussions.length,
-      lastFetchedAt: Time.now
+      lastFetchedAt: Time.now.to_i
 
     }.to_json
   end

spec/javascripts/notes/mock_data.js

@@ -1,7 +1,7 @@
 /* eslint-disable */
 export const notesDataMock = {
   discussionsPath: '/gitlab-org/gitlab-ce/issues/26/discussions.json',
-  lastFetchedAt: '1501862675',
+  lastFetchedAt: 1501862675,
   markdownDocsPath: '/help/user/markdown',
   newSessionPath: '/users/sign_in?redirect_to_referer=yes',
   notesPath: '/gitlab-org/gitlab-ce/noteable/issue/98/notes',

spec/javascripts/notes/stores/actions_spec.js

@@ -1,5 +1,6 @@
 import Vue from 'vue';
 import _ from 'underscore';
+import { headersInterceptor } from 'spec/helpers/vue_resource_helper';
 import * as actions from '~/notes/stores/actions';
 import store from '~/notes/stores';
 import testAction from '../../helpers/vuex_action_helper';
@@ -129,4 +130,68 @@ describe('Actions Notes Store', () => {
       ], done);
     });
   });
+
+  describe('poll', () => {
+    beforeEach((done) => {
+      jasmine.clock().install();
+
+      spyOn(Vue.http, 'get').and.callThrough();
+
+      store.dispatch('setNotesData', notesDataMock)
+        .then(done)
+        .catch(done.fail);
+    });
+
+    afterEach(() => {
+      jasmine.clock().uninstall();
+    });
+
+    it('calls service with last fetched state', (done) => {
+      const interceptor = (request, next) => {
+        next(request.respondWith(JSON.stringify({
+          notes: [],
+          last_fetched_at: '123456',
+        }), {
+          status: 200,
+          headers: {
+            'poll-interval': '1000',
+          },
+        }));
+      };
+
+      Vue.http.interceptors.push(interceptor);
+      Vue.http.interceptors.push(headersInterceptor);
+
+      store.dispatch('poll')
+        .then(() => new Promise(resolve => requestAnimationFrame(resolve)))
+        .then(() => {
+          expect(Vue.http.get).toHaveBeenCalledWith(jasmine.anything(), {
+            url: jasmine.anything(),
+            method: 'get',
+            headers: {
+              'X-Last-Fetched-At': undefined,
+            },
+          });
+          expect(store.state.lastFetchedAt).toBe('123456');
+
+          jasmine.clock().tick(1500);
+        })
+        .then(() => new Promise((resolve) => {
+          requestAnimationFrame(resolve);
+        }))
+        .then(() => {
+          expect(Vue.http.get.calls.count()).toBe(2);
+          expect(Vue.http.get.calls.mostRecent().args[1].headers).toEqual({
+            'X-Last-Fetched-At': '123456',
+          });
+        })
+        .then(() => store.dispatch('stopPolling'))
+        .then(() => {
+          Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor);
+          Vue.http.interceptors = _.without(Vue.http.interceptors, headersInterceptor);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
 });

spec/javascripts/notes/stores/mutation_spec.js

@@ -101,10 +101,21 @@ describe('Notes Store mutations', () => {
       const state = {
         notes: [],
       };
+      const legacyNote = {
+        id: 2,
+        individual_note: true,
+        notes: [{
+          note: '1',
+        }, {
+          note: '2',
+        }],
+      };
 
-      mutations.SET_INITIAL_NOTES(state, [note]);
+      mutations.SET_INITIAL_NOTES(state, [note, legacyNote]);
       expect(state.notes[0].id).toEqual(note.id);
-      expect(state.notes.length).toEqual(1);
+      expect(state.notes[1].notes[0].note).toBe(legacyNote.notes[0].note);
+      expect(state.notes[2].notes[0].note).toBe(legacyNote.notes[1].note);
+      expect(state.notes.length).toEqual(3);
     });
   });