Make direct-linked private group show after login

- Clear groups service state on login and logout.
- Handle case where focus group is null when watching the focused group
and userid for changes. This can happen when clearGroups is called since
it sets the focus group to null which triggers a call to the watch
function. There are two watch function that get triggered; one in
sidebar-content, the other in the group service.
- Clear direct-linked id states on clearSelection and when a new
focusedGroup is selected.

src/sidebar/components/group-list-item.js

@@ -10,6 +10,8 @@ const { withServices } = require('../util/service-context');
 function GroupListItem({ analytics, group, store }) {
   const focusGroup = () => {
     analytics.track(analytics.events.GROUP_SWITCH);
+    store.clearDirectLinkedGroupFetchFailed();
+    store.clearDirectLinkedIds();
     store.focusGroup(group.id);
   };
 

src/sidebar/components/hypothesis-app.js

@@ -121,6 +121,7 @@ function HypothesisAppController(
     return auth
       .login()
       .then(() => {
+        store.clearGroups();
         session.reload();
       })
       .catch(err => {
@@ -179,6 +180,7 @@ function HypothesisAppController(
     if (!promptToLogout()) {
       return;
     }
+    store.clearGroups();
     drafts.unsaved().forEach(function(draft) {
       $rootScope.$emit(events.ANNOTATION_DELETED, draft);
     });

src/sidebar/components/sidebar-content.js

@@ -250,24 +250,15 @@ function SidebarContentController(
   // Re-fetch annotations when focused group, logged-in user or connected frames
   // change.
   $scope.$watch(
-    () => [groups.focused().id, store.profile().userid, ...store.searchUris()],
-    ([currentGroupId], [prevGroupId]) => {
-      // FIXME - There is a bug here where the set of displayed annotations can
-      // end up not matching the focused group when the user logs out.
-      //
-      // When a user logs in or out, we re-fetch profile and group information
-      // concurrently. If the profile fetch completes first, it will trigger
-      // an annotation fetch. If the group fetch then completes before the
-      // annotation fetch, and the focused group changes due to the previous
-      // focused group not being in the new set of groups, then the `if` below
-      // will skip refetching annotations a second time. This will result in the
-      // wrong set of displayed annotations.
-      //
-      // This should only affect users logging out because the set of groups for
-      // logged-in users is currently a superset of those for logged-out users on
-      // any given page.
+    () => [groups.focused(), store.profile().userid, ...store.searchUris()],
+    ([currentGroup], [prevGroup]) => {
+      // If the currentGroup is null, clear the selected annotations and return immediatly.
+      if (!currentGroup) {
+        store.clearSelectedAnnotations();
+        return;
+      }
 
-      if (currentGroupId !== prevGroupId) {
+      if (!prevGroup || currentGroup.id !== prevGroup.id) {
         // The focused group may be changed during loading annotations as a result
         // of switching to the group containing a direct-linked annotation.
         //
@@ -338,7 +329,7 @@ function SidebarContentController(
 
     // If user has not landed on a direct linked annotation
     // don't show the CTA.
-    if (!settings.annotations) {
+    if (!store.getState().directLinkedAnnotationsId) {
       return false;
     }
 
@@ -386,6 +377,7 @@ function SidebarContentController(
     store.clearSelectedAnnotations();
     store.selectTab(selectedTab);
     store.clearDirectLinkedGroupFetchFailed();
+    store.clearDirectLinkedIds();
   };
 }
 

src/sidebar/components/test/group-list-item-test.js

@@ -16,6 +16,8 @@ describe('GroupListItem', () => {
     fakeStore = {
       focusGroup: sinon.stub(),
       focusedGroupId: sinon.stub().returns('groupid'),
+      clearDirectLinkedIds: sinon.stub(),
+      clearDirectLinkedGroupFetchFailed: sinon.stub(),
     };
 
     fakeAnalytics = {
@@ -56,6 +58,24 @@ describe('GroupListItem', () => {
     assert.calledWith(fakeAnalytics.track, fakeAnalytics.events.GROUP_SWITCH);
   });
 
+  it('clears the direct linked ids from the store when the group is clicked', () => {
+    const fakeGroup = { id: 'groupid' };
+
+    const wrapper = createGroupListItem(fakeGroup);
+    wrapper.find('.group-list-item__item').simulate('click');
+
+    assert.calledOnce(fakeStore.clearDirectLinkedIds);
+  });
+
+  it('clears the direct-linked group fetch failed from the store when the group is clicked', () => {
+    const fakeGroup = { id: 'groupid' };
+
+    const wrapper = createGroupListItem(fakeGroup);
+    wrapper.find('.group-list-item__item').simulate('click');
+
+    assert.calledOnce(fakeStore.clearDirectLinkedGroupFetchFailed);
+  });
+
   it('sets alt text for organization logo', () => {
     const fakeGroup = {
       id: 'groupid',

src/sidebar/components/test/hypothesis-app-test.js

@@ -63,6 +63,8 @@ describe('sidebar.components.hypothesis-app', function() {
       fakeStore = {
         tool: 'comment',
         clearSelectedAnnotations: sandbox.spy(),
+        getState: sinon.stub(),
+        clearGroups: sinon.stub(),
       };
 
       fakeAnalytics = {
@@ -106,7 +108,9 @@ describe('sidebar.components.hypothesis-app', function() {
         reload: sandbox.stub().returns(Promise.resolve({ userid: null })),
       };
 
-      fakeGroups = { focus: sandbox.spy() };
+      fakeGroups = {
+        focus: sandbox.spy(),
+      };
 
       fakeRoute = { reload: sandbox.spy() };
 
@@ -373,6 +377,15 @@ describe('sidebar.components.hypothesis-app', function() {
   describe('#login()', function() {
     beforeEach(() => {
       fakeAuth.login = sinon.stub().returns(Promise.resolve());
+      fakeStore.getState.returns({ directLinkedGroupFetchFailed: false });
+    });
+
+    it('clears groups', () => {
+      const ctrl = createController();
+
+      ctrl.login().then(() => {
+        assert.called(fakeStore.clearGroups);
+      });
     });
 
     it('initiates the OAuth login flow', () => {
@@ -435,6 +448,14 @@ describe('sidebar.components.hypothesis-app', function() {
         assert.equal(fakeWindow.confirm.callCount, 1);
       });
 
+      it('clears groups', () => {
+        const ctrl = createController();
+
+        ctrl.logout();
+
+        assert.called(fakeStore.clearGroups);
+      });
+
       it('emits "annotationDeleted" for each unsaved draft annotation', function() {
         fakeDrafts.unsaved = sandbox
           .stub()

src/sidebar/components/test/sidebar-content-test.js

@@ -219,6 +219,13 @@ describe('sidebar.components.sidebar-content', function() {
 
       assert.isFalse(store.getState().directLinkedGroupFetchFailed);
     });
+
+    it("clears the direct linked id's from the settings", () => {
+      ctrl.clearSelection();
+
+      assert.equal(store.getState().directLinkedAnnotationsId, null);
+      assert.equal(store.getState().directLinkedGroupId, null);
+    });
   });
 
   describe('showSelectedTabs', () => {
@@ -588,8 +595,7 @@ describe('sidebar.components.sidebar-content', function() {
     }
 
     beforeEach(function() {
-      // There is a direct-linked annotation
-      fakeSettings.annotations = 'test';
+      store.setDirectLinkedAnnotationsId('test');
     });
 
     it('displays a message if the selection is unavailable', function() {
@@ -671,7 +677,7 @@ describe('sidebar.components.sidebar-content', function() {
       ctrl.auth = {
         status: 'logged-out',
       };
-      delete fakeSettings.annotations;
+      store.setDirectLinkedAnnotationsId(null);
       store.addAnnotations([{ id: '123' }]);
       store.selectAnnotations(['123']);
       $scope.$digest();

src/sidebar/services/groups.js

@@ -166,8 +166,8 @@ function groups(
     if (isSidebar) {
       uri = getDocumentUriForGroupSearch();
     }
-    const directLinkedGroupId = settings.group;
-    const directLinkedAnnId = settings.annotations;
+    const directLinkedGroupId = store.getState().directLinkedGroupId;
+    const directLinkedAnnId = store.getState().directLinkedAnnotationsId;
     let directLinkedAnnotationGroupId = null;
     return uri
       .then(uri => {
@@ -374,7 +374,8 @@ function groups(
   let prevFocusedId = store.focusedGroupId();
   store.subscribe(() => {
     const focusedId = store.focusedGroupId();
-    if (focusedId !== prevFocusedId) {
+    // The focused group may be null when the user login state changes.
+    if (focusedId !== null && focusedId !== prevFocusedId) {
       prevFocusedId = focusedId;
 
       localStorage.setItem(STORAGE_KEY, focusedId);

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

@@ -64,6 +64,8 @@ describe('groups', function() {
         searchUris: ['http://example.org'],
         focusedGroup: null,
         groups: [],
+        directLinkedGroupId: null,
+        directLinkedAnnotationsId: null,
       },
       {
         focusGroup: sinon.stub(),
@@ -190,7 +192,9 @@ describe('groups', function() {
         id: 'oos',
         scopes: { enforced: true, uri_patterns: ['http://foo.com'] },
       };
-      fakeSettings.group = outOfScopeEnforcedGroup.id;
+      fakeStore.setState({
+        directLinkedGroupId: outOfScopeEnforcedGroup.id,
+      });
       fakeApi.group.read.returns(Promise.resolve(outOfScopeEnforcedGroup));
       return svc.load().then(groups => {
         // The failure state is captured in the store.
@@ -198,14 +202,14 @@ describe('groups', function() {
         // The focus group is not set to the direct-linked group.
         assert.calledWith(fakeStore.focusGroup, dummyGroups[0].id);
         // The direct-linked group is not in the list of groups.
-        assert.isFalse(groups.some(g => g.id === fakeSettings.group));
+        assert.isFalse(groups.some(g => g.id === outOfScopeEnforcedGroup.id));
       });
     });
 
     it('catches error from api.group.read request', () => {
       const svc = service();
       fakeLocalStorage.getItem.returns(dummyGroups[0].id);
-      fakeSettings.group = 'does-not-exist';
+      fakeStore.setState({ directLinkedGroupId: 'does-not-exist' });
       fakeApi.group.read.returns(
         Promise.reject(
           new Error(
@@ -244,7 +248,7 @@ describe('groups', function() {
       const svc = service();
 
       // Set the direct-linked group to dummyGroups[0].
-      fakeSettings.group = dummyGroups[0].id;
+      fakeStore.setState({ directLinkedGroupId: dummyGroups[0].id });
       fakeApi.group.read.returns(Promise.resolve(dummyGroups[0]));
 
       // Include the dummyGroups[0] in the featured groups.
@@ -253,18 +257,18 @@ describe('groups', function() {
 
       return svc.load().then(groups => {
         const groupIds = groups.map(g => g.id);
-        assert.deepEqual(groupIds, [fakeSettings.group]);
+        assert.deepEqual(groupIds, [dummyGroups[0].id]);
       });
     });
 
     it('combines groups from all 3 endpoints if there is a selectedGroup', () => {
       const svc = service();
 
-      fakeSettings.group = 'selected-id';
+      fakeStore.setState({ directLinkedGroupId: 'selected-id' });
       const groups = [
         { id: 'groupa', name: 'GroupA' },
         { id: 'groupb', name: 'GroupB' },
-        { id: fakeSettings.group, name: 'Selected Group' },
+        { id: 'selected-id', name: 'Selected Group' },
       ];
 
       fakeApi.profile.groups.read.returns(Promise.resolve([groups[0]]));
@@ -276,11 +280,11 @@ describe('groups', function() {
       });
     });
 
-    it('passes the groupid from settings.group to the api.group.read call', () => {
+    it('passes the direct-linked group id from the store to the api.group.read call', () => {
       const svc = service();
 
-      fakeSettings.group = 'selected-id';
-      const group = { id: fakeSettings.group, name: 'Selected Group' };
+      fakeStore.setState({ directLinkedGroupId: 'selected-id' });
+      const group = { id: 'selected-id', name: 'Selected Group' };
 
       fakeApi.profile.groups.read.returns(Promise.resolve([]));
       fakeApi.groups.list.returns(Promise.resolve([]));
@@ -290,7 +294,7 @@ describe('groups', function() {
         assert.calledWith(
           fakeApi.group.read,
           sinon.match({
-            id: fakeSettings.group,
+            id: 'selected-id',
           })
         );
       });
@@ -309,7 +313,7 @@ describe('groups', function() {
       fakeApi.groups.list.returns(
         Promise.resolve([{ id: 'groupa', name: 'GroupA' }])
       );
-      fakeSettings.group = 'group-id';
+      fakeStore.setState({ directLinkedGroupId: 'group-id' });
 
       return svc.load().then(() => {
         assert.calledWith(
@@ -337,8 +341,10 @@ describe('groups', function() {
 
     it("sets the direct-linked annotation's group to take precedence over the group saved in local storage and the direct-linked group", () => {
       const svc = service();
-      fakeSettings.annotations = 'ann-id';
-      fakeSettings.group = dummyGroups[1].id;
+      fakeStore.setState({
+        directLinkedAnnotationsId: 'ann-id',
+        directLinkedGroupId: dummyGroups[1].id,
+      });
       fakeLocalStorage.getItem.returns(dummyGroups[0].id);
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       fakeApi.annotation.get.returns(
@@ -354,7 +360,7 @@ describe('groups', function() {
 
     it("sets the focused group to the direct-linked annotation's group", () => {
       const svc = service();
-      fakeSettings.annotations = 'ann-id';
+      fakeStore.setState({ directLinkedAnnotationsId: 'ann-id' });
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       fakeLocalStorage.getItem.returns(dummyGroups[0].id);
       fakeApi.annotation.get.returns(
@@ -370,7 +376,7 @@ describe('groups', function() {
 
     it('sets the direct-linked group to take precedence over the group saved in local storage', () => {
       const svc = service();
-      fakeSettings.group = dummyGroups[1].id;
+      fakeStore.setState({ directLinkedGroupId: dummyGroups[1].id });
       fakeLocalStorage.getItem.returns(dummyGroups[0].id);
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       return svc.load().then(() => {
@@ -378,18 +384,18 @@ describe('groups', function() {
       });
     });
 
-    it('sets the focused group to the linked group', () => {
+    it('sets the focused group to the direct-linked group', () => {
       const svc = service();
-      fakeSettings.group = dummyGroups[1].id;
+      fakeStore.setState({ directLinkedGroupId: dummyGroups[1].id });
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       return svc.load().then(() => {
-        assert.calledWith(fakeStore.focusGroup, fakeSettings.group);
+        assert.calledWith(fakeStore.focusGroup, dummyGroups[1].id);
       });
     });
 
     it('clears the directLinkedGroupFetchFailed state if loading a direct-linked group', () => {
       const svc = service();
-      fakeSettings.group = dummyGroups[1].id;
+      fakeStore.setState({ directLinkedGroupId: dummyGroups[1].id });
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       return svc.load().then(() => {
         assert.called(fakeStore.clearDirectLinkedGroupFetchFailed);
@@ -464,7 +470,7 @@ describe('groups', function() {
     it('catches error when fetching the direct-linked annotation', () => {
       const svc = service();
 
-      fakeSettings.annotations = 'ann-id';
+      fakeStore.setState({ directLinkedAnnotationsId: 'ann-id' });
 
       fakeApi.profile.groups.read.returns(Promise.resolve([]));
       fakeApi.groups.list.returns(
@@ -488,7 +494,7 @@ describe('groups', function() {
     it("catches error when fetching the direct-linked annotation's group", () => {
       const svc = service();
 
-      fakeSettings.annotations = 'ann-id';
+      fakeStore.setState({ directLinkedAnnotationsId: 'ann-id' });
 
       fakeApi.profile.groups.read.returns(Promise.resolve([]));
       fakeApi.groups.list.returns(
@@ -524,7 +530,7 @@ describe('groups', function() {
     it("includes the direct-linked annotation's group when it is not in the normal list of groups", () => {
       const svc = service();
 
-      fakeSettings.annotations = 'ann-id';
+      fakeStore.setState({ directLinkedAnnotationsId: 'ann-id' });
 
       fakeApi.profile.groups.read.returns(Promise.resolve([]));
       fakeApi.groups.list.returns(
@@ -556,8 +562,10 @@ describe('groups', function() {
       // the frame embedding the client.
       const svc = service();
 
-      fakeSettings.group = 'out-of-scope';
-      fakeSettings.annotations = 'ann-id';
+      fakeStore.setState({
+        directLinkedGroupId: 'out-of-scope',
+        directLinkedAnnotationsId: 'ann-id',
+      });
 
       fakeApi.profile.groups.read.returns(Promise.resolve([]));
       fakeApi.groups.list.returns(
@@ -593,8 +601,10 @@ describe('groups', function() {
       // out and there are associated groups.
       const svc = service();
 
-      fakeSettings.group = '__world__';
-      fakeSettings.annotations = undefined;
+      fakeStore.setState({
+        directLinkedGroupId: '__world__',
+        directLinkedAnnotationsId: null,
+      });
 
       fakeApi.profile.groups.read.returns(Promise.resolve([]));
       fakeApi.groups.list.returns(
@@ -632,9 +642,9 @@ describe('groups', function() {
                 group: '__world__',
               })
             );
-            fakeSettings.annotations = 'direct-linked-ann';
-          } else {
-            fakeSettings.annotations = undefined;
+            fakeStore.setState({
+              directLinkedAnnotationsId: 'direct-linked-ann',
+            });
           }
 
           // Create groups response from server.