Merge pull request #2101 from hypothesis/redo-user-changed-in-groups

Revise re-fetching of groups when logged-in user changes

src/sidebar/services/groups.js

-import events from '../events';
 import serviceConfig from '../service-config';
 import { isReply } from '../util/annotation-metadata';
 import { combineGroups } from '../util/groups';
@@ -19,7 +18,6 @@ const DEFAULT_ORGANIZATION = {
 
 // @ngInject
 export default function groups(
-  $rootScope,
   store,
   api,
   isSidebar,
@@ -173,8 +171,6 @@ export default function groups(
     });
   }
 
-  let didSubscribeToUriChanges = false;
-
   /*
    * Fetch an individual group.
    *
@@ -188,6 +184,39 @@ export default function groups(
     });
   }
 
+  let reloadSetUp = false;
+
+  /**
+   * Set up automatic re-fetching of groups in response to various events
+   * in the sidebar.
+   */
+  function setupAutoReload() {
+    if (reloadSetUp) {
+      return;
+    }
+    reloadSetUp = true;
+
+    // Reload groups when main document URI changes.
+    watch(store.subscribe, mainUri, () => {
+      load();
+    });
+
+    // Reload groups when user ID changes. This is a bit inefficient since it
+    // means we are not fetching the groups and profile concurrently after
+    // logging in or logging out.
+    watch(
+      store.subscribe,
+      [() => store.hasFetchedProfile(), () => store.profile().userid],
+      (_, [prevFetchedProfile]) => {
+        if (!prevFetchedProfile) {
+          // Ignore the first time that the profile is loaded.
+          return;
+        }
+        load();
+      }
+    );
+  }
+
   /**
    * Fetch groups from the API, load them into the store and set the focused
    * group.
@@ -209,12 +238,7 @@ export default function groups(
       documentUri = await getDocumentUriForGroupSearch();
     }
 
-    if (!didSubscribeToUriChanges) {
-      didSubscribeToUriChanges = true;
-      watch(store.subscribe, mainUri, () => {
-        load();
-      });
-    }
+    setupAutoReload();
 
     // Step 2: Concurrently fetch the groups the user is a member of,
     // the groups associated with the current document and the annotation
@@ -369,13 +393,6 @@ export default function groups(
     });
   }
 
-  // refetch the list of groups when user changes
-  $rootScope.$on(events.USER_CHANGED, () => {
-    // FIXME Makes a second api call on page load. better way?
-    // return for use in test
-    return load();
-  });
-
   return {
     all: all,
     get: get,

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

-import events from '../../events';
 import fakeReduxStore from '../../test/fake-redux-store';
 import groups, { $imports } from '../groups';
 import { waitFor } from '../../../test-util/wait';
@@ -45,7 +44,6 @@ describe('groups', function () {
   let fakeSession;
   let fakeSettings;
   let fakeApi;
-  let fakeRootScope;
   let fakeServiceUrl;
   let fakeMetadata;
   let fakeToastMessenger;
@@ -83,6 +81,7 @@ describe('groups', function () {
         focusedGroupId: sinon.stub(),
         getDefault: sinon.stub(),
         getGroup: sinon.stub(),
+        hasFetchedProfile: sinon.stub().returns(false),
         loadGroups: sinon.stub(),
         newAnnotations: sinon.stub().returns([]),
         allGroups() {
@@ -97,25 +96,11 @@ describe('groups', function () {
         setDefault: sinon.stub(),
         setDirectLinkedGroupFetchFailed: sinon.stub(),
         clearDirectLinkedGroupFetchFailed: sinon.stub(),
+        profile: sinon.stub().returns({ userid: null }),
       }
     );
     fakeSession = sessionWithThreeGroups();
     fakeIsSidebar = true;
-    fakeRootScope = {
-      eventCallbacks: {},
-
-      $apply: function (callback) {
-        callback();
-      },
-
-      $on: function (event, callback) {
-        if (event === events.USER_CHANGED) {
-          this.eventCallbacks[event] = callback;
-        }
-      },
-
-      $broadcast: sinon.stub(),
-    };
     fakeApi = {
       annotation: {
         get: sinon.stub(),
@@ -150,7 +135,6 @@ describe('groups', function () {
 
   function service() {
     return groups(
-      fakeRootScope,
       fakeStore,
       fakeApi,
       fakeIsSidebar,
@@ -810,13 +794,35 @@ describe('groups', function () {
     });
   });
 
+  const delay = ms => new Promise(resolve => setTimeout(resolve, ms));
+
   describe('automatic re-fetching', function () {
-    it('refetches groups when the logged-in user changes', () => {
-      service();
+    it('refetches groups when the logged-in user changes', async () => {
+      const svc = service();
 
-      return fakeRootScope.eventCallbacks[events.USER_CHANGED]().then(() => {
-        assert.calledOnce(fakeApi.groups.list);
-      });
+      // Load groups before profile fetch has completed.
+      fakeStore.hasFetchedProfile.returns(false);
+      await svc.load();
+
+      // Simulate initial fetch of profile finishing. This should be ignored.
+      fakeApi.groups.list.resetHistory();
+      fakeStore.hasFetchedProfile.returns(true);
+      fakeStore.profile.returns({ userid: 'acct:firstuser@hypothes.is' });
+      fakeStore.setState({}); // Notify store subscribers.
+
+      // Wait briefly, as there are a few async steps before the group fetch
+      // from the API starts, if it is going to happen.
+      await delay(1);
+      assert.notCalled(fakeApi.groups.list);
+
+      // Simulate user logging out (or logging in). This should trigger a re-fetching
+      // of groups.
+      fakeStore.hasFetchedProfile.returns(true);
+      fakeStore.profile.returns({ userid: 'acct:otheruser@hypothes.is' });
+      fakeStore.setState({});
+
+      await waitFor(() => fakeApi.groups.list.callCount > 0);
+      assert.calledOnce(fakeApi.groups.list);
     });
 
     context('when a new frame connects', () => {
@@ -858,7 +864,9 @@ describe('groups', function () {
           ],
         });
 
-        await new Promise(resolve => setTimeout(resolve, 1));
+        // Wait briefly, as there are a few async steps before the group fetch
+        // from the API starts, if it is going to happen.
+        await delay(1);
         assert.notCalled(fakeApi.groups.list);
       });
     });

src/sidebar/store/modules/session.js

 import * as util from '../util';
 
+/**
+ * A dummy profile returned by the `profile` selector before the real profile
+ * is fetched.
+ */
+const initialProfile = {
+  /** A map of features that are enabled for the current user. */
+  features: {},
+  /** A map of preference names and values. */
+  preferences: {},
+  /**
+   * The authenticated user ID or null if the user is not logged in.
+   */
+  userid: null,
+};
+
 function init() {
   return {
     /**
      * Profile object fetched from the `/api/profile` endpoint.
      */
-    profile: {
-      /** A map of features that are enabled for the current user. */
-      features: {},
-      /** A map of preference names and values. */
-      preferences: {},
-      /**
-       * The authenticated user ID or null if the user is not logged in.
-       */
-      userid: null,
-    },
+    profile: initialProfile,
   };
 }
 
@@ -58,10 +64,22 @@ function isFeatureEnabled(state, feature) {
   return !!state.session.profile.features[feature];
 }
 
+/**
+ * Return true if the user's profile has been fetched. This can be used to
+ * distinguish the dummy profile returned by `profile()` on startup from a
+ * logged-out user profile returned by the server.
+ */
+function hasFetchedProfile(state) {
+  return state.session.profile !== initialProfile;
+}
+
 /**
  * Return the user's profile.
  *
  * Returns the current user's profile fetched from the `/api/profile` endpoint.
+ *
+ * If the profile has not yet been fetched yet, a dummy logged-out profile is
+ * returned. This allows code to skip a null check.
  */
 function profile(state) {
   return state.session.profile;
@@ -77,6 +95,7 @@ export default {
   },
 
   selectors: {
+    hasFetchedProfile,
     isFeatureEnabled,
     isLoggedIn,
     profile,

src/sidebar/store/modules/test/session-test.js

@@ -28,6 +28,38 @@ describe('sidebar/store/modules/session', function () {
     });
   });
 
+  describe('#isFeatureEnabled', () => {
+    it('returns false before features have been fetched', () => {
+      assert.isFalse(store.isFeatureEnabled('some_feature'));
+    });
+
+    it('returns false if feature is unknown', () => {
+      store.updateProfile({ userid: null, features: {} });
+      assert.isFalse(store.isFeatureEnabled('some_feature'));
+    });
+
+    [true, false].forEach(enabled => {
+      it('returns feature flag state if profile is fetched and feature exists', () => {
+        store.updateProfile({
+          userid: null,
+          features: { some_feature: enabled },
+        });
+        assert.equal(store.isFeatureEnabled('some_feature'), enabled);
+      });
+    });
+  });
+
+  describe('#hasFetchedProfile', () => {
+    it('returns false before profile is updated', () => {
+      assert.isFalse(store.hasFetchedProfile());
+    });
+
+    it('returns true after profile is updated', () => {
+      store.updateProfile({ userid: 'john' });
+      assert.isTrue(store.hasFetchedProfile());
+    });
+  });
+
   describe('#profile', () => {
     it("returns the user's profile", () => {
       store.updateProfile({ userid: 'john' });