diff options
-rw-r--r-- | app/assets/javascripts/notes/stores/getters.js | 39 | ||||
-rw-r--r-- | spec/javascripts/notes/stores/getters_spec.js | 130 |
2 files changed, 112 insertions, 57 deletions
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 52410f18d4a..3d0ec8cd3a7 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -171,26 +171,33 @@ export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, dif return lastDiscussionId === discussionId; }; -// Gets the ID of the discussion following the one provided, respecting order (diff or date) -// @param {Boolean} discussionId - id of the current discussion -// @param {Boolean} diffOrder - is ordered by diff? -export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const currentIndex = idsOrdered.indexOf(discussionId); - const slicedIds = idsOrdered.slice(currentIndex + 1, currentIndex + 2); +export const findUnresolvedDiscussionIdNeighbor = (state, getters) => ({ + discussionId, + diffOrder, + step, +}) => { + const ids = getters.unresolvedDiscussionsIdsOrdered(diffOrder); + const index = ids.indexOf(discussionId) + step; + + if (index < 0 && step < 0) { + return ids[ids.length - 1]; + } + + if (index === ids.length && step > 0) { + return ids[0]; + } - // Get the first ID if there is none after the currentIndex - return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0]; + return ids[index]; }; -export const previousUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const currentIndex = idsOrdered.indexOf(discussionId); - const slicedIds = idsOrdered.slice(currentIndex - 1, currentIndex); +// Gets the ID of the discussion following the one provided, respecting order (diff or date) +// @param {Boolean} discussionId - id of the current discussion +// @param {Boolean} diffOrder - is ordered by diff? +export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => + getters.findUnresolvedDiscussionIdNeighbor({ discussionId, diffOrder, step: 1 }); - // Get the last ID if there is none after the currentIndex - return slicedIds.length ? slicedIds[0] : idsOrdered[idsOrdered.length - 1]; -}; +export const previousUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => + getters.findUnresolvedDiscussionIdNeighbor({ discussionId, diffOrder, step: -1 }); // @param {Boolean} diffOrder - is ordered by diff? export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => { diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js index 71dcba114a9..d69f469c7c7 100644 --- a/spec/javascripts/notes/stores/getters_spec.js +++ b/spec/javascripts/notes/stores/getters_spec.js @@ -14,6 +14,13 @@ import { const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json'; +// Helper function to ensure that we're using the same schema across tests. +const createDiscussionNeighborParams = (discussionId, diffOrder, step) => ({ + discussionId, + diffOrder, + step, +}); + describe('Getters Notes Store', () => { let state; @@ -25,7 +32,6 @@ describe('Getters Notes Store', () => { targetNoteHash: 'hash', lastFetchedAt: 'timestamp', isNotesFetched: false, - notesData: notesDataMock, userData: userDataMock, noteableData: noteableDataMock, @@ -244,62 +250,104 @@ describe('Getters Notes Store', () => { }); }); - describe('nextUnresolvedDiscussionId', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; + describe('findUnresolvedDiscussionIdNeighbor', () => { + let localGetters; + beforeEach(() => { + localGetters = { + unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], + }; + }); - it('should return the ID of the discussion after the ID provided', () => { - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe('123'); + [ + { step: 1, id: '123', expected: '456' }, + { step: 1, id: '456', expected: '789' }, + { step: 1, id: '789', expected: '123' }, + { step: -1, id: '123', expected: '789' }, + { step: -1, id: '456', expected: '123' }, + { step: -1, id: '789', expected: '456' }, + ].forEach(({ step, id, expected }) => { + it(`with step ${step} and id ${id}, returns next value`, () => { + const params = createDiscussionNeighborParams(id, true, step); + + expect(getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params)).toBe( + expected, + ); + }); }); - }); - describe('previousUnresolvedDiscussionId', () => { - describe('with unresolved discussions', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; + describe('with 1 unresolved discussion', () => { + beforeEach(() => { + localGetters = { + unresolvedDiscussionsIdsOrdered: () => ['123'], + }; + }); + + [{ step: 1, id: '123', expected: '123' }, { step: -1, id: '123', expected: '123' }].forEach( + ({ step, id, expected }) => { + it(`with step ${step} and match, returns only value`, () => { + const params = createDiscussionNeighborParams(id, true, step); - it('with bogus returns falsey', () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('456'); + expect(getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params)).toBe( + expected, + ); + }); + }, + ); + + it('with no match, returns only value', () => { + const params = createDiscussionNeighborParams('bogus', true, 1); + + expect(getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params)).toBe('123'); }); + }); - [ - { id: '123', expected: '789' }, - { id: '456', expected: '123' }, - { id: '789', expected: '456' }, - ].forEach(({ id, expected }) => { - it(`with ${id}, returns previous value`, () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)(id)).toBe(expected); + describe('with 0 unresolved discussions', () => { + beforeEach(() => { + localGetters = { + unresolvedDiscussionsIdsOrdered: () => [], + }; + }); + + [{ step: 1 }, { step: -1 }].forEach(({ step }) => { + it(`with step ${step}, returns undefined`, () => { + const params = createDiscussionNeighborParams('bogus', true, step); + + expect( + getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params), + ).toBeUndefined(); }); }); }); + }); - describe('with 1 unresolved discussion', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123'], - }; + describe('findUnresolvedDiscussionIdNeighbor aliases', () => { + let neighbor; + let findUnresolvedDiscussionIdNeighbor; + let localGetters; - it('with bogus returns id', () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('123'); - }); + beforeEach(() => { + neighbor = {}; + findUnresolvedDiscussionIdNeighbor = jasmine.createSpy().and.returnValue(neighbor); + localGetters = { findUnresolvedDiscussionIdNeighbor }; + }); - it('with match, returns value', () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)('123')).toEqual('123'); + describe('nextUnresolvedDiscussionId', () => { + it('should return result of find neighbor', () => { + const expectedParams = createDiscussionNeighborParams('123', true, 1); + const result = getters.nextUnresolvedDiscussionId(state, localGetters)('123', true); + + expect(findUnresolvedDiscussionIdNeighbor).toHaveBeenCalledWith(expectedParams); + expect(result).toBe(neighbor); }); }); - describe('with 0 unresolved discussions', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => [], - }; + describe('previosuUnresolvedDiscussionId', () => { + it('should return result of find neighbor', () => { + const expectedParams = createDiscussionNeighborParams('123', true, -1); + const result = getters.previousUnresolvedDiscussionId(state, localGetters)('123', true); - it('returns undefined', () => { - expect( - getters.previousUnresolvedDiscussionId(state, localGetters)('bogus'), - ).toBeUndefined(); + expect(findUnresolvedDiscussionIdNeighbor).toHaveBeenCalledWith(expectedParams); + expect(result).toBe(neighbor); }); }); }); |