Merge pull request #2163 from hypothesis/fix-lms-groups-fetch-race

Fix race condition and error handling when fetching groups in LMS app

docs/publishers/config.rst

@@ -190,12 +190,10 @@ loads.
 
    .. option:: groups
 
-      ``String[]|null``. An array of group IDs. If provided, the list of groups
-      fetched from the API will be filtered against this list so that the user
-      can only select from these groups.
-
-      This can be useful in contexts where it is important that annotations
-      are made in a particular group.
+      ``String[]|null``. An array of group IDs. If provided, only these groups
+      will be fetched and displayed in the client. This is used, for example,
+      in the Hypothesis LMS app to show only the groups appropriate for a
+      user looking at a particular assignment.
 
    .. option:: icon
 

src/sidebar/services/groups.js

@@ -30,6 +30,9 @@ export default function groups(
   const svc = serviceConfig(settings);
   const authority = svc ? svc.authority : null;
 
+  // `expand` parameter for various groups API calls.
+  const expandParam = ['organization', 'scopes'];
+
   /**
    * Return the main document URI that is used to fetch groups associated with
    * the site that the user is on.
@@ -111,22 +114,6 @@ export default function groups(
         directLinkedGroupId = null;
       }
     }
-    // If service groups are specified only return those.
-    // If a service group doesn't exist in the list of groups don't return it.
-    if (svc && svc.groups) {
-      try {
-        // The groups may not be immediately available if they are being fetched via RPC.
-        const focusedGroups = await svc.groups;
-        const filteredGroups = groups.filter(
-          g => focusedGroups.includes(g.id) || focusedGroups.includes(g.groupid)
-        );
-        return filteredGroups;
-      } catch (e) {
-        toastMessenger.error(e.message);
-        // Don't show any groups if we catch an error
-        return [];
-      }
-    }
 
     // Logged-in users always see the "Public" group.
     if (isLoggedIn) {
@@ -172,16 +159,13 @@ export default function groups(
   }
 
   /*
-   * Fetch an individual group.
+   * Fetch a specific group.
    *
-   * @param {Object} requestParams
-   * @return {Promise<Group>|undefined}
+   * @param {string} id
+   * @return {Promise<Group>}
    */
-  function fetchGroup(requestParams) {
-    return api.group.read(requestParams).catch(() => {
-      // If the group does not exist or the user doesn't have permission.
-      return null;
-    });
+  async function fetchGroup(id) {
+    return api.group.read({ id, expand: expandParam });
   }
 
   let reloadSetUp = false;
@@ -218,19 +202,12 @@ export default function groups(
   }
 
   /**
-   * Fetch groups from the API, load them into the store and set the focused
-   * group.
-   *
-   * The groups that are fetched depend on the current user, the URI of
-   * the current document, and the direct-linked group and/or annotation.
-   *
-   * On startup, `load()` must be called to trigger the initial groups fetch.
-   * Subsequently groups are automatically reloaded if the logged-in user or
-   * main document URI changes.
+   * Fetch the groups associated with the current user and document, as well
+   * as any groups that have been direct-linked to.
    *
    * @return {Promise<Group[]>}
    */
-  async function load() {
+  async function loadGroupsForUserAndDocument() {
     // Step 1: Get the URI of the active document, so we can fetch groups
     // associated with that document.
     let documentUri;
@@ -243,15 +220,6 @@ export default function groups(
     // Step 2: Concurrently fetch the groups the user is a member of,
     // the groups associated with the current document and the annotation
     // and/or group that was direct-linked (if any).
-    const params = {
-      expand: ['organization', 'scopes'],
-    };
-    if (authority) {
-      params.authority = authority;
-    }
-    if (documentUri) {
-      params.document_uri = documentUri;
-    }
 
     // If there is a direct-linked annotation, fetch the annotation in case
     // the associated group has not already been fetched and we need to make
@@ -273,18 +241,26 @@ export default function groups(
     const directLinkedGroupId = store.directLinkedGroupId();
     let directLinkedGroupApi = null;
     if (directLinkedGroupId) {
-      directLinkedGroupApi = fetchGroup({
-        id: directLinkedGroupId,
-        expand: params.expand,
-      }).then(group => {
-        // If the group does not exist or the user doesn't have permission.
-        if (group === null) {
-          store.setDirectLinkedGroupFetchFailed();
-        } else {
+      directLinkedGroupApi = fetchGroup(directLinkedGroupId)
+        .then(group => {
           store.clearDirectLinkedGroupFetchFailed();
-        }
-        return group;
-      });
+          return group;
+        })
+        .catch(() => {
+          // If the group does not exist or the user doesn't have permission.
+          store.setDirectLinkedGroupFetchFailed();
+          return null;
+        });
+    }
+
+    const listParams = {
+      expand: expandParam,
+    };
+    if (authority) {
+      listParams.authority = authority;
+    }
+    if (documentUri) {
+      listParams.document_uri = documentUri;
     }
 
     const [
@@ -294,8 +270,8 @@ export default function groups(
       directLinkedAnn,
       directLinkedGroup,
     ] = await Promise.all([
-      api.profile.groups.read({ expand: params.expand }),
-      api.groups.list(params),
+      api.profile.groups.read({ expand: expandParam }),
+      api.groups.list(listParams),
       auth.tokenGetter(),
       directLinkedAnnApi,
       directLinkedGroupApi,
@@ -328,12 +304,11 @@ export default function groups(
         .find(g => g.id === directLinkedAnn.group);
 
       if (!directLinkedAnnGroup) {
-        const directLinkedAnnGroup = await fetchGroup({
-          id: directLinkedAnn.group,
-          expand: params.expand,
-        });
-        if (directLinkedAnnGroup) {
+        try {
+          const directLinkedAnnGroup = await fetchGroup(directLinkedAnn.group);
           featuredGroups.push(directLinkedAnnGroup);
+        } catch (e) {
+          toastMessenger.error('Unable to fetch group for linked annotation');
         }
       }
     }
@@ -348,25 +323,115 @@ export default function groups(
       directLinkedGroupId
     );
 
+    const groupToFocus =
+      directLinkedAnnotationGroupId || directLinkedGroupId || null;
+    addGroupsToStore(groups, groupToFocus);
+
+    return groups;
+  }
+
+  /**
+   * Load the specific groups configured by the annotation service.
+   *
+   * @param {string[]} groupIds - `id` or `groupid`s of groups to fetch
+   */
+  async function loadServiceSpecifiedGroups(groupIds) {
+    // Fetch the groups that the user is a member of in one request and then
+    // fetch any other groups not returned in that request directly.
+    //
+    // This reduces the number of requests to the backend on the assumption
+    // that most or all of the group IDs that the service configures the client
+    // to show are groups that the user is a member of.
+    const userGroups = await api.profile.groups.read({ expand: expandParam });
+
+    let error;
+    const tryFetchGroup = async id => {
+      try {
+        return await fetchGroup(id);
+      } catch (e) {
+        error = e;
+        return null;
+      }
+    };
+
+    const getGroup = id =>
+      userGroups.find(g => g.id === id || g.groupid === id) ||
+      tryFetchGroup(id);
+
+    const groups = (await Promise.all(groupIds.map(getGroup))).filter(
+      g => g !== null
+    );
+
+    addGroupsToStore(groups);
+
+    if (error) {
+      toastMessenger.error(`Unable to fetch groups: ${error.message}`, {
+        autoDismiss: false,
+      });
+    }
+
+    return groups;
+  }
+
+  /**
+   * Add groups to the store and set the initial focused group.
+   *
+   * @param {Group[]} groups
+   * @param {string|null} [groupToFocus]
+   */
+  function addGroupsToStore(groups, groupToFocus) {
+    // Add a default organization to groups that don't have one. The organization
+    // provides the logo to display when the group is selected and is also used
+    // to order groups.
     injectOrganizations(groups);
 
-    // Step 5. Load the groups into the store and focus the appropriate
-    // group.
     const isFirstLoad = store.allGroups().length === 0;
     const prevFocusedGroup = store.getDefault('focusedGroup');
 
     store.loadGroups(groups);
 
     if (isFirstLoad) {
-      if (groups.some(g => g.id === directLinkedAnnotationGroupId)) {
-        focus(directLinkedAnnotationGroupId);
-      } else if (groups.some(g => g.id === directLinkedGroupId)) {
-        focus(directLinkedGroupId);
+      if (groups.some(g => g.id === groupToFocus)) {
+        focus(groupToFocus);
       } else if (groups.some(g => g.id === prevFocusedGroup)) {
         focus(prevFocusedGroup);
       }
     }
+  }
 
+  /**
+   * Fetch groups from the API, load them into the store and set the focused
+   * group.
+   *
+   * There are two main scenarios:
+   *
+   * 1. The groups loaded depend on the current user, current document URI and
+   *    active direct links. This is the default.
+   *
+   *    On startup, `load()` must be called to trigger the initial groups fetch.
+   *    Subsequently groups are automatically reloaded if the logged-in user or
+   *    main document URI changes.
+   *
+   * 2. The annotation service specifies exactly which groups to load via the
+   *    configuration it passes to the client.
+   *
+   * @return {Promise<Group[]>}
+   */
+  async function load() {
+    let groups;
+    if (svc && svc.groups) {
+      let groupIds = [];
+      try {
+        groupIds = await svc.groups;
+      } catch (e) {
+        toastMessenger.error(
+          `Unable to fetch group configuration: ${e.message}`
+        );
+      }
+      groups = await loadServiceSpecifiedGroups(groupIds);
+    } else {
+      groups = await loadGroupsForUserAndDocument();
+    }
     return groups;
   }
 

src/sidebar/services/test/groups-test.js

@@ -706,71 +706,116 @@ describe('groups', function () {
       }
     );
 
-    [
-      {
-        description: 'shows service groups',
-        services: [{ groups: ['abc123'] }],
-        expected: ['abc123'],
-      },
-      {
-        description: 'also supports identifying service groups by groupid',
-        services: [{ groups: ['group:42@example.com'] }],
-        expected: ['abc123'],
-      },
-      {
-        description: 'only shows service groups that exist',
-        services: [{ groups: ['abc123', 'no_exist'] }],
-        expected: ['abc123'],
-      },
-      {
-        description: 'shows no groups if no service groups exist',
-        services: [{ groups: ['no_exist'] }],
-        expected: [],
-      },
-      {
-        description: 'shows all groups if service is null',
-        services: null,
-        expected: ['__world__', 'abc123', 'def456'],
-      },
-      {
-        description: 'shows all groups if service groups does not exist',
-        services: [{}],
-        expected: ['__world__', 'abc123', 'def456'],
-      },
-      {
-        description: 'does not show any groups if the groups promise rejects',
-        services: [
-          { groups: Promise.reject(new Error('something went wrong')) },
-        ],
-        toastMessageError: 'something went wrong',
-        expected: [],
-      },
-    ].forEach(
-      ({ description, services, expected, toastMessageError = null }) => {
-        it(description, () => {
-          fakeSettings.services = services;
-          const svc = service();
+    context('when service config specifies which groups to show', () => {
+      const makeGroup = (id, groupid = null) => ({ id, groupid });
+      const setServiceConfigGroups = groupids => {
+        fakeSettings.services = [{ groups: groupids }];
+      };
 
-          // Create groups response from server.
-          const groups = [
-            { name: 'Public', id: '__world__' },
-            { name: 'ABC', id: 'abc123', groupid: 'group:42@example.com' },
-            { name: 'DEF', id: 'def456', groupid: null },
-          ];
+      const groupA = makeGroup('id-a');
+      const groupB = makeGroup('id-b', 'groupid-b');
+      const groupC = makeGroup('id-c');
 
-          fakeApi.groups.list.returns(Promise.resolve(groups));
-          fakeApi.profile.groups.read.returns(Promise.resolve([]));
+      beforeEach(() => {
+        fakeApi.profile.groups.read.resolves([]);
+        fakeApi.group.read.rejects(new Error('Not Found'));
+      });
 
-          return svc.load().then(groups => {
-            let displayedGroups = groups.map(g => g.id);
-            assert.deepEqual(displayedGroups, expected);
-            if (toastMessageError) {
-              assert.calledWith(fakeToastMessenger.error, toastMessageError);
-            }
-          });
+      it('loads groups specified by id or groupid in service config', async () => {
+        setServiceConfigGroups(['id-a', 'groupid-b']);
+        fakeApi.profile.groups.read.resolves([groupA, groupB, groupC]);
+
+        const svc = service();
+        const groups = await svc.load();
+
+        assert.deepEqual(
+          groups.map(g => g.id),
+          ['id-a', 'id-b']
+        );
+      });
+
+      it('loads groups specified asynchronously in service config', async () => {
+        setServiceConfigGroups(Promise.resolve(['id-a', 'groupid-b']));
+        fakeApi.profile.groups.read.resolves([groupA, groupB, groupC]);
+
+        const svc = service();
+        const groups = await svc.load();
+
+        assert.deepEqual(
+          groups.map(g => g.id),
+          ['id-a', 'id-b']
+        );
+      });
+
+      it(`fetches groups by ID if the group is not in the user's groups`, async () => {
+        setServiceConfigGroups(Promise.resolve(['id-a', 'groupid-b', 'id-c']));
+        const serverGroups = [groupA, groupB, groupC];
+        fakeApi.profile.groups.read.resolves([groupA]);
+        fakeApi.group.read.callsFake(async ({ id }) => {
+          const group = serverGroups.find(g => g.id === id || g.groupid === id);
+          if (!group) {
+            throw new Error(`Group ${id} not found`);
+          }
+          return group;
         });
-      }
-    );
+
+        const svc = service();
+        const groups = await svc.load();
+
+        const expand = ['organization', 'scopes'];
+        assert.calledWith(fakeApi.group.read, { expand, id: 'groupid-b' });
+        assert.calledWith(fakeApi.group.read, { expand, id: 'id-c' });
+        assert.deepEqual(
+          groups.map(g => g.id),
+          ['id-a', 'id-b', 'id-c']
+        );
+      });
+
+      it(`does not fetch group by ID if the group is in the user's groups`, async () => {
+        setServiceConfigGroups(Promise.resolve(['id-a', 'groupid-b']));
+        fakeApi.profile.groups.read.resolves([groupA, groupB, groupC]);
+
+        const svc = service();
+        await svc.load();
+
+        assert.notCalled(fakeApi.group.read);
+      });
+
+      it('reports an error if a group specified in service config fails to load', async () => {
+        setServiceConfigGroups(Promise.resolve(['id-a', 'missing']));
+        fakeApi.profile.groups.read.resolves([groupA]);
+        fakeApi.group.read.rejects(new Error('Not Found'));
+
+        const svc = service();
+        const groups = await svc.load();
+
+        assert.calledWith(
+          fakeToastMessenger.error,
+          'Unable to fetch groups: Not Found'
+        );
+
+        // The groups that were found should still be loaded.
+        assert.deepEqual(
+          groups.map(g => g.id),
+          ['id-a']
+        );
+      });
+
+      it('reports an error if fetching group IDs from service config fails', async () => {
+        setServiceConfigGroups(
+          Promise.reject(new Error('Something went wrong'))
+        );
+
+        const svc = service();
+        const groups = await svc.load();
+
+        assert.calledWith(
+          fakeToastMessenger.error,
+          'Unable to fetch group configuration: Something went wrong'
+        );
+        assert.deepEqual(groups, []);
+      });
+    });
   });
 
   describe('#get', function () {