Merge pull request #1064 from hypothesis/handle-illegal-group-link-states

Handle illegal group link states

src/sidebar/components/search-status-bar.js

@@ -15,7 +15,8 @@ module.exports = {
     onClearSelection: '&',
     searchQuery: '<',
     selectedTab: '<',
-    selectionCount: '<',
+    // Boolean indicating all annotations are visible (none are hidden).
+    areAllAnnotationsVisible: '<',
     totalAnnotations: '<',
     totalNotes: '<',
   },

src/sidebar/components/sidebar-content.js

@@ -49,6 +49,8 @@ function SidebarContentController(
   streamFilter
 ) {
   const self = this;
+  this.directLinkedGroupFetchFailed =
+    !!settings.group && settings.group !== store.focusedGroup().id;
 
   function thread() {
     return rootThread.thread(store.getState());
@@ -161,7 +163,7 @@ function SidebarContentController(
     searchClient.get({ uri: uris, group: group });
   }
 
-  function isLoading() {
+  this.isLoading = function() {
     if (
       !store.frames().some(function(frame) {
         return frame.uri;
@@ -177,7 +179,7 @@ function SidebarContentController(
     }
 
     return false;
-  }
+  };
 
   /**
    * Load annotations for all URLs associated with `frames`.
@@ -272,7 +274,7 @@ function SidebarContentController(
         // of switching to the group containing a direct-linked annotation.
         //
         // In that case, we don't want to trigger reloading annotations again.
-        if (isLoading()) {
+        if (this.isLoading()) {
           return;
         }
         store.clearSelectedAnnotations();
@@ -283,6 +285,17 @@ function SidebarContentController(
     true
   );
 
+  this.showSelectedTabs = function() {
+    if (
+      this.selectedAnnotationUnavailable() ||
+      this.selectedGroupUnavailable() ||
+      this.search.query()
+    ) {
+      return false;
+    }
+    return true;
+  };
+
   this.setCollapsed = function(id, collapsed) {
     store.setCollapsed(id, collapsed);
   };
@@ -297,17 +310,26 @@ function SidebarContentController(
   this.focus = focusAnnotation;
   this.scrollTo = scrollToAnnotation;
 
-  this.selectedAnnotationCount = function() {
+  this.areAllAnnotationsVisible = function() {
+    if (this.directLinkedGroupFetchFailed) {
+      return true;
+    }
     const selection = store.getState().selectedAnnotationMap;
     if (!selection) {
-      return 0;
+      return false;
     }
-    return Object.keys(selection).length;
+    return Object.keys(selection).length > 0;
+  };
+
+  this.selectedGroupUnavailable = function() {
+    return !this.isLoading() && this.directLinkedGroupFetchFailed;
   };
 
   this.selectedAnnotationUnavailable = function() {
     const selectedID = firstKey(store.getState().selectedAnnotationMap);
-    return !isLoading() && !!selectedID && !store.annotationExists(selectedID);
+    return (
+      !this.isLoading() && !!selectedID && !store.annotationExists(selectedID)
+    );
   };
 
   this.shouldShowLoggedOutMessage = function() {
@@ -332,11 +354,11 @@ function SidebarContentController(
     // annotation. If there is an annotation selection and that
     // selection is available to the user, show the CTA.
     const selectedID = firstKey(store.getState().selectedAnnotationMap);
-    return !isLoading() && !!selectedID && store.annotationExists(selectedID);
+    return (
+      !this.isLoading() && !!selectedID && store.annotationExists(selectedID)
+    );
   };
 
-  this.isLoading = isLoading;
-
   const visibleCount = memoize(function(thread) {
     return thread.children.reduce(
       function(count, child) {
@@ -365,6 +387,8 @@ function SidebarContentController(
 
     store.clearSelectedAnnotations();
     store.selectTab(selectedTab);
+    // Clear direct-linked group fetch failed state.
+    this.directLinkedGroupFetchFailed = false;
   };
 }
 

src/sidebar/components/test/search-status-bar-test.js

@@ -30,7 +30,7 @@ describe('searchStatusBar', function() {
       const msg = 'Show all annotations';
       const msgCount = '(2)';
       const elem = util.createDirective(document, 'searchStatusBar', {
-        selectionCount: 1,
+        areAllAnnotationsVisible: true,
         totalAnnotations: 2,
         selectedTab: 'annotation',
       });
@@ -43,7 +43,7 @@ describe('searchStatusBar', function() {
       const msg = 'Show all notes';
       const msgCount = '(3)';
       const elem = util.createDirective(document, 'searchStatusBar', {
-        selectionCount: 1,
+        areAllAnnotationsVisible: true,
         totalNotes: 3,
         selectedTab: 'note',
       });

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

@@ -6,6 +6,7 @@ const EventEmitter = require('tiny-emitter');
 
 const events = require('../../events');
 const noCallThru = require('../../../shared/test/util').noCallThru;
+const uiConstants = require('../../ui-constants');
 
 let searchClients;
 
@@ -150,7 +151,7 @@ describe('sidebar.components.sidebar-content', function() {
     });
   }
 
-  beforeEach(
+  const makeSidebarContentController = () => {
     angular.mock.inject(function($componentController, _store_, _$rootScope_) {
       $rootScope = _$rootScope_;
       $scope = $rootScope.$new();
@@ -163,13 +164,84 @@ describe('sidebar.components.sidebar-content', function() {
           auth: { status: 'unknown' },
         }
       );
-    })
-  );
+    });
+  };
+
+  beforeEach(() => {
+    makeSidebarContentController();
+  });
 
   afterEach(function() {
     return sandbox.restore();
   });
 
+  describe('clearSelection', () => {
+    it('sets selectedTab to Annotations tab if selectedTab is null', () => {
+      store.selectTab(uiConstants.TAB_ORPHANS);
+      $scope.$digest();
+      ctrl.clearSelection();
+
+      assert.equal(store.getState().selectedTab, uiConstants.TAB_ANNOTATIONS);
+    });
+
+    it('sets selectedTab to Annotations tab if selectedTab is set to orphans', () => {
+      store.selectTab(uiConstants.TAB_ORPHANS);
+      $scope.$digest();
+
+      ctrl.clearSelection();
+
+      assert.equal(store.getState().selectedTab, uiConstants.TAB_ANNOTATIONS);
+    });
+
+    it('clears selected annotations', () => {
+      ctrl.clearSelection();
+
+      assert.equal(store.getState().selectedAnnotationMap, null);
+      assert.equal(store.getState().filterQuery, null);
+    });
+
+    it('clears the directLinkedGroupFetchFailed state', () => {
+      ctrl.directLinkedGroupFetchFailed = true;
+
+      ctrl.clearSelection();
+
+      assert.isFalse(ctrl.directLinkedGroupFetchFailed);
+    });
+  });
+
+  describe('showSelectedTabs', () => {
+    beforeEach(() => {
+      setFrames([{ uri: 'http://www.example.com' }]);
+      ctrl.search = { query: sinon.stub().returns(undefined) };
+    });
+
+    it('returns false if there is a search query', () => {
+      ctrl.search = { query: sinon.stub().returns('tag:foo') };
+      assert.isFalse(ctrl.showSelectedTabs());
+    });
+
+    it('returns false if selected group is unavailable', () => {
+      fakeSettings.group = 'group-id';
+      store.loadGroups([{ id: 'default-id' }]);
+      store.focusGroup('default-id');
+      fakeGroups.focused.returns({ id: 'default-id' });
+      $scope.$digest();
+      // Re-construct the controller after the environment setup.
+      makeSidebarContentController();
+      assert.isFalse(ctrl.showSelectedTabs());
+    });
+
+    it('returns false if selected annotation is unavailable', () => {
+      store.selectAnnotations(['missing']);
+      $scope.$digest();
+      assert.isFalse(ctrl.showSelectedTabs());
+    });
+
+    it('returns true in all other cases', () => {
+      assert.isTrue(ctrl.showSelectedTabs());
+    });
+  });
+
   describe('#loadAnnotations', function() {
     it('unloads any existing annotations', function() {
       // When new clients connect, all existing annotations should be unloaded
@@ -255,7 +327,62 @@ describe('sidebar.components.sidebar-content', function() {
       assert.isTrue(updateSpy.calledWith(frameUris[1], true));
     });
 
-    context('when there is a selection', function() {
+    context('when there is a direct-linked group error', () => {
+      beforeEach(() => {
+        setFrames([{ uri: 'http://www.example.com' }]);
+        fakeSettings.group = 'group-id';
+        store.loadGroups([{ id: 'default-id' }]);
+        store.focusGroup('default-id');
+        fakeGroups.focused.returns({ id: 'default-id' });
+        $scope.$digest();
+        // Re-construct the controller after the environment setup.
+        makeSidebarContentController();
+      });
+
+      it('sets directLinkedGroupFetchFailed to true', () => {
+        assert.isTrue(ctrl.directLinkedGroupFetchFailed);
+      });
+
+      it('areAllAnnotationsVisible returns true since there is an error message', () => {
+        assert.isTrue(ctrl.areAllAnnotationsVisible());
+      });
+
+      it('selectedGroupUnavailable returns true', () => {
+        assert.isTrue(ctrl.selectedGroupUnavailable());
+      });
+    });
+
+    context('when there is a direct-linked group selection', () => {
+      beforeEach(() => {
+        setFrames([{ uri: 'http://www.example.com' }]);
+        fakeSettings.group = 'group-id';
+        store.loadGroups([{ id: fakeSettings.group }]);
+        store.focusGroup(fakeSettings.group);
+        fakeGroups.focused.returns({ id: fakeSettings.group });
+        $scope.$digest();
+      });
+
+      it('sets directLinkedGroupFetchFailed to false', () => {
+        assert.isFalse(ctrl.directLinkedGroupFetchFailed);
+      });
+
+      it('areAllAnnotationsVisible returns false since group has no annotations', () => {
+        assert.isFalse(ctrl.areAllAnnotationsVisible());
+      });
+
+      it('selectedGroupUnavailable returns false', () => {
+        assert.isFalse(ctrl.selectedGroupUnavailable());
+      });
+
+      it('fetches annotations for the direct-linked group', () => {
+        assert.calledWith(searchClients[0].get, {
+          uri: ['http://www.example.com'],
+          group: 'group-id',
+        });
+      });
+    });
+
+    context('when there is a direct-linked annotation selection', function() {
       const uri = 'http://example.com';
       const id = uri + '123';
 
@@ -265,8 +392,8 @@ describe('sidebar.components.sidebar-content', function() {
         $scope.$digest();
       });
 
-      it('selectedAnnotationCount is > 0', function() {
-        assert.equal(ctrl.selectedAnnotationCount(), 1);
+      it('areAllAnnotationsVisible is true', function() {
+        assert.isTrue(ctrl.areAllAnnotationsVisible());
       });
 
       it("switches to the selected annotation's group", function() {
@@ -295,8 +422,8 @@ describe('sidebar.components.sidebar-content', function() {
         $scope.$digest();
       });
 
-      it('selectedAnnotationCount is 0', function() {
-        assert.equal(ctrl.selectedAnnotationCount(), 0);
+      it('areAllAnnotationsVisible is false', function() {
+        assert.isFalse(ctrl.areAllAnnotationsVisible());
       });
 
       it('fetches annotations for the current group', function() {

src/sidebar/components/test/thread-list-test.js

@@ -70,6 +70,13 @@ class FakeVirtualThreadList extends EventEmitter {
     this.yOffsetOf = function() {
       return 42;
     };
+    this.calculateVisibleThreads = () => {
+      return {
+        offscreenLowerHeight: 10,
+        offscreenUpperHeight: 20,
+        visibleThreads: thread.children,
+      };
+    };
   }
 }
 

src/sidebar/components/thread-list.js

@@ -82,30 +82,43 @@ function ThreadListController($element, $scope, settings, VirtualThreadList) {
       this.thread,
       options
     );
-    visibleThreads.on('changed', function(state) {
-      self.virtualThreadList = {
-        visibleThreads: state.visibleThreads,
-        invisibleThreads: state.invisibleThreads,
-        offscreenUpperHeight: state.offscreenUpperHeight + 'px',
-        offscreenLowerHeight: state.offscreenLowerHeight + 'px',
-      };
-
-      scopeTimeout(
-        $scope,
-        function() {
-          state.visibleThreads.forEach(function(thread) {
-            const height = getThreadHeight(thread.id);
-            if (!height) {
-              return;
-            }
-            visibleThreads.setThreadHeight(thread.id, height);
-          });
-        },
-        50
-      );
-    });
+    // Calculate the visible threads.
+    onVisibleThreadsChanged(visibleThreads.calculateVisibleThreads());
+    // Subscribe onVisibleThreadsChanged to the visibleThreads 'changed' event
+    // after calculating visible threads, to avoid an undesired second call to
+    // onVisibleThreadsChanged that occurs from the emission of the 'changed'
+    // event during the visibleThreads calculation.
+    visibleThreads.on('changed', onVisibleThreadsChanged);
   };
 
+  /**
+   * Update which threads are visible in the virtualThreadsList.
+   *
+   * @param {Object} state the new state of the virtualThreadsList
+   */
+  function onVisibleThreadsChanged(state) {
+    self.virtualThreadList = {
+      visibleThreads: state.visibleThreads,
+      invisibleThreads: state.invisibleThreads,
+      offscreenUpperHeight: state.offscreenUpperHeight + 'px',
+      offscreenLowerHeight: state.offscreenLowerHeight + 'px',
+    };
+
+    scopeTimeout(
+      $scope,
+      function() {
+        state.visibleThreads.forEach(function(thread) {
+          const height = getThreadHeight(thread.id);
+          if (!height) {
+            return;
+          }
+          visibleThreads.setThreadHeight(thread.id, height);
+        });
+      },
+      50
+    );
+  }
+
   /**
    * Return the vertical scroll offset for the document in order to position the
    * annotation thread with a given `id` or $tag at the top-left corner

src/sidebar/services/groups.js

@@ -68,6 +68,19 @@ function groups(
     directLinkedAnnotationId,
     directLinkedGroupId
   ) {
+    // Filter the directLinkedGroup out if it is out of scope and scope is enforced.
+    if (directLinkedGroupId) {
+      const directLinkedGroup = groups.find(g => g.id === directLinkedGroupId);
+      if (
+        directLinkedGroup &&
+        !directLinkedGroup.isScopedToUri &&
+        directLinkedGroup.scopes.enforced
+      ) {
+        groups = groups.filter(g => g.id !== directLinkedGroupId);
+        directLinkedGroupId = undefined;
+      }
+    }
+
     // 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) {
@@ -186,10 +199,16 @@ function groups(
         // particular group as well since it may not be in the results returned
         // by group.list or profile.groups.
         if (directLinkedGroup) {
-          const selectedGroupApi = api.group.read({
-            id: directLinkedGroup,
-            expand: params.expand,
-          });
+          const selectedGroupApi = api.group
+            .read({
+              id: directLinkedGroup,
+              expand: params.expand,
+            })
+            .catch(() => {
+              // If the group does not exist or the user doesn't have permission,
+              // return undefined.
+              return undefined;
+            });
           groupApiRequests = groupApiRequests.concat(selectedGroupApi);
         }
         return Promise.all(groupApiRequests).then(

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

@@ -181,6 +181,39 @@ describe('groups', function() {
   });
 
   describe('#load', function() {
+    it('filters out direct-linked groups that are out of scope and scope enforced', () => {
+      const svc = service();
+      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      const outOfScopeEnforcedGroup = {
+        id: 'oos',
+        scopes: { enforced: true, uri_patterns: ['http://foo.com'] },
+      };
+      fakeSettings.group = outOfScopeEnforcedGroup.id;
+      fakeApi.group.read.returns(Promise.resolve(outOfScopeEnforcedGroup));
+      return svc.load().then(groups => {
+        // 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));
+      });
+    });
+
+    it('catches 404 error from api.group.read request', () => {
+      const svc = service();
+      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeSettings.group = 'does-not-exist';
+      fakeApi.group.read.returns(
+        Promise.reject(
+          "404 Not Found: Either the resource you requested doesn't exist, \
+          or you are not currently authorized to see it."
+        )
+      );
+      return svc.load().then(() => {
+        // The focus group is not set to the direct-linked group.
+        assert.calledWith(fakeStore.focusGroup, dummyGroups[0].id);
+      });
+    });
+
     it('combines groups from both endpoints', function() {
       const svc = service();
 

src/sidebar/templates/search-status-bar.html

@@ -11,7 +11,7 @@
                   'one': '1 search result',
                   'other': '{} search results'}"></span>
 </div>
-<div class="search-status-bar" ng-if="!vm.filterActive && vm.selectionCount > 0">
+<div class="search-status-bar" ng-if="!vm.filterActive && vm.areAllAnnotationsVisible">
   <button class="primary-action-btn primary-action-btn--short"
           ng-click="vm.onClearSelection()"
           title="Clear the selection and show all annotations">

src/sidebar/templates/sidebar-content.html

 <selection-tabs
-  ng-if="!vm.search.query() && vm.selectedAnnotationCount() === 0"
+  ng-if="vm.showSelectedTabs()"
   is-waiting-to-anchor-annotations="vm.waitingToAnchorAnnotations"
   is-loading="vm.isLoading"
   selected-tab="vm.selectedTab"
@@ -14,13 +14,14 @@
   filter-match-count="vm.visibleCount()"
   on-clear-selection="vm.clearSelection()"
   search-query="vm.search.query()"
-  selection-count="vm.selectedAnnotationCount()"
+  are-all-annotations-visible="vm.areAllAnnotationsVisible()"
   total-count="vm.topLevelThreadCount()"
   selected-tab="vm.selectedTab"
   total-annotations="vm.totalAnnotations"
   total-notes="vm.totalNotes">
 </search-status-bar>
 
+<!-- Display error message if direct-linked annotation fetch failed. -->
 <sidebar-content-error
   class="sidebar-content-error"
   logged-out-error-message="'This annotation is not available.'"
@@ -31,6 +32,17 @@
 >
 </sidebar-content-error>
 
+<!-- Display error message if direct-linked group fetch failed. -->
+<sidebar-content-error
+  class="sidebar-content-error"
+  logged-out-error-message="'This group is not available.'"
+  logged-in-error-message="'You either do not have permission to view this group, the group does not exist, or the group is not visible at this URL.'"
+  on-login-request="vm.onLogin()"
+  is-logged-in="vm.auth.status === 'logged-in'"
+  ng-if="vm.selectedGroupUnavailable()"
+>
+</sidebar-content-error>
+
 <thread-list
   on-change-collapsed="vm.setCollapsed(id, collapsed)"
   on-clear-selection="vm.clearSelection()"
@@ -38,6 +50,7 @@
   on-force-visible="vm.forceVisible(thread)"
   on-select="vm.scrollTo(annotation)"
   show-document-info="false"
+  ng-if="!vm.selectedGroupUnavailable()"
   thread="vm.rootThread">
 </thread-list>
 

src/sidebar/virtual-thread-list.js

@@ -49,7 +49,7 @@ class VirtualThreadList extends EventEmitter {
     this.scrollRoot = options.scrollRoot || document.body;
 
     const debouncedUpdate = debounce(function() {
-      self._updateVisibleThreads();
+      self.calculateVisibleThreads();
       $scope.$digest();
     }, 20);
     this.scrollRoot.addEventListener('scroll', debouncedUpdate);
@@ -83,7 +83,7 @@ class VirtualThreadList extends EventEmitter {
       return;
     }
     this._rootThread = thread;
-    this._updateVisibleThreads();
+    this.calculateVisibleThreads();
   }
 
   /**
@@ -131,7 +131,7 @@ class VirtualThreadList extends EventEmitter {
    *
    * Emits a `changed` event with the recalculated set of visible threads.
    */
-  _updateVisibleThreads() {
+  calculateVisibleThreads() {
     // Space above the viewport in pixels which should be considered 'on-screen'
     // when calculating the set of visible threads
     const MARGIN_ABOVE = 800;
@@ -205,6 +205,12 @@ class VirtualThreadList extends EventEmitter {
       visibleThreads: visibleThreads,
       invisibleThreads: invisibleThreads,
     });
+    return {
+      offscreenLowerHeight,
+      offscreenUpperHeight,
+      visibleThreads,
+      invisibleThreads,
+    };
   }
 }