Merge pull request #1084 from hypothesis/fix-direct-linked-private-login-flow

Fix direct linked private login flow

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,16 @@ 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 (!currentGroup) {
+        // When switching accounts, groups are cleared and so the focused group
+        // will be null for a brief period of time.
+        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 +330,7 @@ function SidebarContentController(
 
     // If user has not landed on a direct linked annotation
     // don't show the CTA.
-    if (!settings.annotations) {
+    if (!store.getState().directLinkedAnnotationId) {
       return false;
     }
 
@@ -386,6 +378,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 IDs in the store', () => {
+      ctrl.clearSelection();
+
+      assert.equal(store.getState().directLinkedAnnotationId, 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.setDirectLinkedAnnotationId('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.setDirectLinkedAnnotationId(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().directLinkedAnnotationId;
     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,
+        directLinkedAnnotationId: 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({
+        directLinkedAnnotationId: '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({ directLinkedAnnotationId: '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({ directLinkedAnnotationId: '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({ directLinkedAnnotationId: '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({ directLinkedAnnotationId: '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',
+        directLinkedAnnotationId: '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__',
+        directLinkedAnnotationId: 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({
+              directLinkedAnnotationId: 'direct-linked-ann',
+            });
           }
 
           // Create groups response from server.

src/sidebar/store/index.js

@@ -36,7 +36,7 @@ const debugMiddleware = require('./debug-middleware');
 
 const activity = require('./modules/activity');
 const annotations = require('./modules/annotations');
-const directLinkedGroup = require('./modules/direct-linked-group');
+const directLinked = require('./modules/direct-linked');
 const frames = require('./modules/frames');
 const links = require('./modules/links');
 const groups = require('./modules/groups');
@@ -87,7 +87,7 @@ function store($rootScope, settings) {
   const modules = [
     activity,
     annotations,
-    directLinkedGroup,
+    directLinked,
     frames,
     links,
     groups,

src/sidebar/store/modules/direct-linked-group.js → src/sidebar/store/modules/direct-linked.js

@@ -2,8 +2,34 @@
 
 const util = require('../util');
 
-function init() {
+function init(settings) {
   return {
+    /**
+     * The ID of the direct-linked group.
+     *
+     * This ID is initialized from the client's configuration to indicate that
+     * the client should focus on a particular group initially. The user may
+     * need to login for this step to complete. When the user navigates away
+     * from the group or clears the selection, the direct link is "consumed"
+     * and no longer used.
+     *
+     * @type {string}
+     */
+    directLinkedGroupId: settings.group || null,
+
+    /**
+     * The ID of the direct-linked annotation.
+     *
+     * This ID is initialized from the client's configuration to indicate that
+     * the client should focus on a particular annotation. The user may need to
+     * login to see the annotation. When the user clears the selection or
+     * switches to a different group manually, the direct link is "consumed"
+     * and no longer used.
+     *
+     * @type {string}
+     */
+    directLinkedAnnotationId: settings.annotations || null,
+
     /**
      * Indicates that an error occured in retrieving/showing the direct linked group.
      * This could be because:
@@ -22,10 +48,46 @@ const update = {
       directLinkedGroupFetchFailed: action.directLinkedGroupFetchFailed,
     };
   },
+  UPDATE_DIRECT_LINKED_GROUP_ID(state, action) {
+    return {
+      directLinkedGroupId: action.directLinkedGroupId,
+    };
+  },
+  UPDATE_DIRECT_LINKED_ANNOTATION_ID(state, action) {
+    return {
+      directLinkedAnnotationId: action.directLinkedAnnotationId,
+    };
+  },
+  CLEAR_DIRECT_LINKED_IDS() {
+    return {
+      directLinkedAnnotationId: null,
+      directLinkedGroupId: null,
+    };
+  },
 };
 
 const actions = util.actionTypes(update);
 
+/**
+ * Set the direct linked group id.
+ */
+function setDirectLinkedGroupId(groupId) {
+  return {
+    type: actions.UPDATE_DIRECT_LINKED_GROUP_ID,
+    directLinkedGroupId: groupId,
+  };
+}
+
+/**
+ * Set the direct linked annotation's id.
+ */
+function setDirectLinkedAnnotationId(annId) {
+  return {
+    type: actions.UPDATE_DIRECT_LINKED_ANNOTATION_ID,
+    directLinkedAnnotationId: annId,
+  };
+}
+
 /**
  * Set the direct linked group fetch failure to true.
  */
@@ -46,12 +108,27 @@ function clearDirectLinkedGroupFetchFailed() {
   };
 }
 
+/**
+ * Clear the direct linked annotations and group IDs.
+ *
+ * This action indicates that the direct link has been "consumed" and should
+ * not affect future group/annotation etc. fetches.
+ */
+function clearDirectLinkedIds() {
+  return {
+    type: actions.CLEAR_DIRECT_LINKED_IDS,
+  };
+}
+
 module.exports = {
   init,
   update,
   actions: {
     setDirectLinkedGroupFetchFailed,
+    setDirectLinkedGroupId,
+    setDirectLinkedAnnotationId,
     clearDirectLinkedGroupFetchFailed,
+    clearDirectLinkedIds,
   },
   selectors: {},
 };

src/sidebar/store/modules/test/direct-linked-group-test.js

-'use strict';
-
-const createStore = require('../../create-store');
-const directLinkedGroup = require('../direct-linked-group');
-
-describe('sidebar/store/modules/direct-linked-group', () => {
-  let store;
-
-  beforeEach(() => {
-    store = createStore([directLinkedGroup]);
-  });
-
-  it('sets directLinkedGroupFetchFailed to true', () => {
-    store.setDirectLinkedGroupFetchFailed();
-
-    assert.isTrue(store.getState().directLinkedGroupFetchFailed);
-  });
-
-  it('sets directLinkedGroupFetchFailed to false', () => {
-    store.setDirectLinkedGroupFetchFailed();
-
-    store.clearDirectLinkedGroupFetchFailed();
-
-    assert.isFalse(store.getState().directLinkedGroupFetchFailed);
-  });
-});

src/sidebar/store/modules/test/direct-linked-test.js

+'use strict';
+
+const createStore = require('../../create-store');
+const directLinked = require('../direct-linked');
+
+describe('sidebar/store/modules/direct-linked', () => {
+  let store;
+  let fakeSettings = {};
+
+  beforeEach(() => {
+    store = createStore([directLinked], [fakeSettings]);
+  });
+
+  describe('setDirectLinkedGroupFetchFailed', () => {
+    it('sets directLinkedGroupFetchFailed to true', () => {
+      store.setDirectLinkedGroupFetchFailed();
+
+      assert.isTrue(store.getState().directLinkedGroupFetchFailed);
+    });
+  });
+
+  describe('clearDirectLinkedGroupFetchFailed', () => {
+    it('sets directLinkedGroupFetchFailed to false', () => {
+      store.setDirectLinkedGroupFetchFailed();
+
+      store.clearDirectLinkedGroupFetchFailed();
+
+      assert.isFalse(store.getState().directLinkedGroupFetchFailed);
+    });
+  });
+
+  it('sets directLinkedAnnotationId to settings.annotations during store init', () => {
+    fakeSettings.annotations = 'ann-id';
+
+    store = createStore([directLinked], [fakeSettings]);
+
+    assert.equal(store.getState().directLinkedAnnotationId, 'ann-id');
+  });
+
+  describe('setDirectLinkedAnnotationId', () => {
+    it('sets directLinkedAnnotationId to the specified annotation id', () => {
+      store.setDirectLinkedAnnotationId('ann-id');
+
+      assert.equal(store.getState().directLinkedAnnotationId, 'ann-id');
+    });
+  });
+
+  it('sets directLinkedGroupId to settings.group during store init', () => {
+    fakeSettings.group = 'group-id';
+
+    store = createStore([directLinked], [fakeSettings]);
+
+    assert.equal(store.getState().directLinkedGroupId, 'group-id');
+  });
+
+  describe('setDirectLinkedGroupId', () => {
+    it('sets directLinkedGroupId to the specified group id', () => {
+      store.setDirectLinkedGroupId('group-id');
+
+      assert.equal(store.getState().directLinkedGroupId, 'group-id');
+    });
+  });
+
+  describe('clearDirectLinkedIds', () => {
+    it('sets direct-link annotations and group ids to null', () => {
+      store.setDirectLinkedGroupId('ann-id');
+      store.setDirectLinkedGroupId('group-id');
+
+      store.clearDirectLinkedIds();
+
+      assert.equal(store.getState().directLinkedAnnotationId, null);
+      assert.equal(store.getState().directLinkedGroupId, null);
+    });
+  });
+});