From 52b70f0428151dcc6f4154fa584e9478afc34e9e Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 9 Mar 2018 18:22:23 +0000 Subject: Fixed issue notes being duplicated Closes #44099 --- app/assets/javascripts/notes/stores/actions.js | 2 +- app/assets/javascripts/notes/stores/mutations.js | 4 ++-- app/helpers/notes_helper.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 42fc2a131b8..3eaae0bc683 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -197,7 +197,7 @@ 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; }; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index 963b40be3fd..aed0a44e808 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -90,15 +90,15 @@ 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) => { + const nn = Object.assign({}, note); nn.notes = [n]; // override notes array to only have one item to mimick individual_note notes.push(nn); }); } else { + const nn = Object.assign({}, note); const oldNote = utils.findNoteObjectById(state.notes, note.id); nn.expanded = oldNote ? oldNote.expanded : note.expanded; diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index a70e73a6da9..20aed60cb7a 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -169,7 +169,7 @@ module NotesHelper reopenPath: reopen_issuable_path(issuable), notesPath: notes_url, totalNotes: issuable.discussions.length, - lastFetchedAt: Time.now + lastFetchedAt: Time.now.to_i }.to_json end -- cgit v1.2.3 From 87be05bff037bf72e4496b2d3e266d0ba554d2c1 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 9 Mar 2018 18:28:23 +0000 Subject: fix polling not working correctly --- app/assets/javascripts/notes/services/notes_service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/notes/services/notes_service.js b/app/assets/javascripts/notes/services/notes_service.js index 4766351dfc5..bb31be583c7 100644 --- a/app/assets/javascripts/notes/services/notes_service.js +++ b/app/assets/javascripts/notes/services/notes_service.js @@ -30,7 +30,7 @@ export default { const { endpoint, lastFetchedAt } = data; const options = { headers: { - 'X-Last-Fetched-At': lastFetchedAt, + 'X-Last-Fetched-At': `${lastFetchedAt}`, }, }; -- cgit v1.2.3 From 98e31bf437c9934884bd8a7b5c311617757d0785 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Sat, 10 Mar 2018 18:17:01 +0000 Subject: added mutation spec --- app/assets/javascripts/notes/stores/mutations.js | 14 ++++++++------ spec/javascripts/notes/stores/mutation_spec.js | 15 +++++++++++++-- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index aed0a44e808..949628a65c0 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -93,16 +93,18 @@ export default { // To support legacy notes, should be very rare case. if (note.individual_note && note.notes.length > 1) { note.notes.forEach((n) => { - const nn = Object.assign({}, note); - 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 nn = Object.assign({}, note); 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), + }); } }); diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js index e4baefc5bfc..34884f8968f 100644 --- a/spec/javascripts/notes/stores/mutation_spec.js +++ b/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); }); }); -- cgit v1.2.3 From 63d3581e66acc21b79130c5f13bde88a74d136ac Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 12 Mar 2018 09:57:22 +0000 Subject: fixed note polling not sending updated last fetched at date added spec for polling --- .../javascripts/notes/services/notes_service.js | 5 +- app/assets/javascripts/notes/stores/actions.js | 6 +- spec/javascripts/notes/mock_data.js | 2 +- spec/javascripts/notes/stores/actions_spec.js | 65 ++++++++++++++++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/notes/services/notes_service.js b/app/assets/javascripts/notes/services/notes_service.js index bb31be583c7..b4c19a9ec22 100644 --- a/app/assets/javascripts/notes/services/notes_service.js +++ b/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, }, }; diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 3eaae0bc683..fc7faf05c5f 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -203,12 +203,10 @@ const pollSuccessCallBack = (resp, commit, state, getters) => { }; 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.'), @@ -217,7 +215,7 @@ export const poll = ({ commit, state, getters }) => { if (!Visibility.hidden()) { eTagPoll.makeRequest(); } else { - service.poll(requestData); + service.poll(state); } Visibility.change(() => { diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index bf60cb12f52..5be13ed0dfe 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/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', diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index ab80ed7bbfb..b838cc36fb3 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/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); + }); + }); }); -- cgit v1.2.3