Merge branch 'refactor/notes-getters' into 'master'

Refactor nextUnresolvedDiscussionId and previousUnresolvedDiscussionId getters

Closes #65817

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

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 => {

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);
       });
     });
   });