Re-work group auto-reload when logged in user changes

Redo the automatic reload of groups when the logged-in user changes:

 - Ignore the initial change of user on startup if the user is logged
   in from a previous session. This generated an unnecessary additional
   API call on startup if the user had a login from a previous session

 - Replace the use of the deprecated `USER_CHANGED` Angular event in
   favor of reacting to store state changes using
   `watch(store.subscribe, ...)`

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,33 @@ 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({});
+
+      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 +862,7 @@ describe('groups', function () {
           ],
         });
 
-        await new Promise(resolve => setTimeout(resolve, 1));
+        await delay(1);
         assert.notCalled(fakeApi.groups.list);
       });
     });