Fix "Show more" button display when more folder content exists

contentcuration/contentcuration/frontend/shared/data/__tests__/resources.spec.js

@@ -1,7 +1,7 @@
 import { UpdatedDescendantsChange } from '../changes';
-import { ViewerM2M, ChannelUser, Channel, ContentNode } from '../resources';
+import { ViewerM2M, ChannelUser, Channel, ContentNode, TreeResource } from '../resources';
 import db from 'shared/data/db';
-import { CHANGE_TYPES, TABLE_NAMES } from 'shared/data/constants';
+import { CHANGE_TYPES, PAGINATION_TABLE, TABLE_NAMES } from 'shared/data/constants';
 import { ContentKindsNames } from 'shared/leUtils/ContentKinds';
 import { mockChannelScope, resetMockChannelScope } from 'shared/utils/testing';
 import client from 'shared/client';
@@ -172,6 +172,111 @@ describe('Resources', () => {
         expect(change.mods).toEqual(changes);
       });
     });
+    describe('Paginated where', () => {
+      const maxResults = 3;
+
+      // A full page of children for parent, plus the more obj the server would send with it
+      const makePage = parent => {
+        const results = [1, 2, 3].map(lft => ({
+          id: `${parent}-child-${lft}`,
+          parent,
+          lft,
+          title: `test-child-${lft}`,
+          kind: ContentKindsNames.TOPIC,
+        }));
+        return {
+          results,
+          more: { parent, max_results: maxResults, ordering: 'lft', lft__gt: maxResults },
+        };
+      };
+
+      const mockPage = (parent, { hasMore = true } = {}) => {
+        const page = makePage(parent);
+        const more = hasMore ? page.more : null;
+        jest
+          .spyOn(client, 'get')
+          .mockResolvedValue({ data: { results: page.results, more, count: 10 } });
+        return { ...page, more };
+      };
+
+      // Loads a page the way loadChildren does, without an explicit ordering
+      const loadPage = parent =>
+        ContentNode.where({ parent, max_results: maxResults, ordering: null });
+
+      beforeEach(async () => {
+        await db[PAGINATION_TABLE].clear();
+        // Test setup stubs out fetching for every resource, but saving pagination only happens
+        // inside the real fetchCollection
+        jest
+          .spyOn(ContentNode, 'fetchCollection')
+          .mockImplementation(params =>
+            TreeResource.prototype.fetchCollection.call(ContentNode, params),
+          );
+        ContentNode._requests = {};
+      });
+
+      afterEach(() => {
+        // Restore by registry, since a failure in the hook above could leave a spy uninstalled
+        jest.restoreAllMocks();
+        ContentNode._requests = {};
+      });
+
+      it('should return the server "more" object when nothing is cached locally', async () => {
+        const parent = 'test-uncached-parent-id';
+        const { more } = mockPage(parent);
+
+        const response = await ContentNode.where({ parent, max_results: maxResults });
+
+        expect(response.more).toEqual(more);
+      });
+
+      it('should return the saved more obj when the cache holds exactly one full page', async () => {
+        const parent = 'test-cached-parent-id';
+        const { more } = mockPage(parent);
+
+        await loadPage(parent);
+
+        // One cached page is indistinguishable from a complete list locally, so the saved
+        // pagination has to supply the more obj
+        const response = await loadPage(parent);
+
+        expect(response.results.map(node => node.id)).toEqual([
+          `${parent}-child-1`,
+          `${parent}-child-2`,
+          `${parent}-child-3`,
+        ]);
+        expect(response.more).toEqual(more);
+      });
+
+      it('should not invent a more object when the server said there was nothing more', async () => {
+        const parent = 'test-final-page-parent-id';
+        mockPage(parent, { hasMore: false });
+
+        await loadPage(parent);
+        const response = await loadPage(parent);
+
+        expect(response.results).toHaveLength(maxResults);
+        expect(response.more).toBeNull();
+      });
+
+      it('should return the saved more obj even once children have been removed since it was saved', async () => {
+        const parent = 'test-shrunken-parent-id';
+        const { more } = mockPage(parent);
+
+        await loadPage(parent);
+        // Nothing invalidates saved pagination when children leave the parent. A more obj that
+        // fetches nothing beats hiding children that are still there, so it stands until the
+        // next fetch replaces it.
+        await db[TABLE_NAMES.CONTENTNODE].delete(`${parent}-child-2`);
+        await db[TABLE_NAMES.CONTENTNODE].delete(`${parent}-child-3`);
+
+        const response = await loadPage(parent);
+
+        expect(response.results).toHaveLength(1);
+        expect(response.more).toEqual(more);
+      });
+    });
+
     describe('ChannelUser resource', () => {
       const testChannelId = 'test-channel-id';
       const testUserId = 'test-user-id';

contentcuration/contentcuration/frontend/shared/data/resources.js

@@ -400,6 +400,23 @@ class IndexedDBResource {
     });
   }
 
+  isPaginated(params = {}) {
+    return !isNaN(Number(params[PAGINATION_FIELD]));
+  }
+
+  /**
+   * Returns a copy of the params with implicit values made explicit, so that equivalent queries
+   * serialize identically. Saved pagination is keyed off that serialization, which is also
+   * sensitive to key order, so equivalent queries must build their params in the same order.
+   */
+  normalizeParams(params = {}) {
+    const normalized = { ...params };
+    if (this.isPaginated(normalized) && !normalized[ORDER_FIELD] && this.defaultOrdering) {
+      normalized[ORDER_FIELD] = this.defaultOrdering;
+    }
+    return normalized;
+  }
+
   async where(params = {}) {
     const table = db[this.tableName];
     // Indexed parameters
@@ -414,11 +431,17 @@ class IndexedDBResource {
     let sortBy;
     let reverse;
 
+    params = this.normalizeParams(params);
+
     // Check for pagination
     const maxResults = Number(params[PAGINATION_FIELD]);
-    const paginationActive = !isNaN(maxResults);
-    if (paginationActive && !params[ORDER_FIELD]) {
-      params[ORDER_FIELD] = this.defaultOrdering;
+    const paginationActive = this.isPaginated(params);
+    if (paginationActive && !params[ORDER_FIELD] && process.env.NODE_ENV !== 'production') {
+      // `normalizeParams` fills in the default ordering, so reaching here means the resource
+      // has none, and both this page and its cursor will be arbitrary
+      /* eslint-disable no-console */
+      console.warn(`Tried to paginate ${this.tableName} which has no defaultOrdering`);
+      /* eslint-enable */
     }
     for (const key of Object.keys(params)) {
       if (key === PAGINATION_FIELD) {
@@ -906,6 +929,8 @@ class Resource extends mix(APIResource, IndexedDBResource) {
    * @return {Promise<Object[]>}
    */
   where(params = {}, doRefresh = true) {
+    // Normalize before serializing, so this key matches the one `fetchCollection` saves under
+    params = this.normalizeParams(params);
     if (process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') {
       /* eslint-disable no-console */
       console.groupCollapsed(`Getting data for ${this.tableName} table with params: `, params);
@@ -926,6 +951,9 @@ class Resource extends mix(APIResource, IndexedDBResource) {
   }
 
   whereLiveQuery(params = {}, doRefresh = true) {
+    // `super.where` normalizes its own copy, so without this `conditionalFetch` below would
+    // fetch under a different key than the query it is refreshing
+    params = this.normalizeParams(params);
     if (process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') {
       /* eslint-disable no-console */
       console.groupCollapsed(`Getting liveQuery for ${this.tableName} table with params: `, params);