Merge pull request #181 from hypothesis/centralize-tab-assignment

Consolidate logic for annotation <-> tab assignment

h/static/scripts/reducers/session.js

@@ -49,6 +49,17 @@ function updateSession(session) {
   };
 }
 
+/**
+ * Return true if a given feature flag is enabled.
+ *
+ * @param {object} state - The application state
+ * @param {string} feature - The name of the feature flag. This matches the
+ *        name of the feature flag as declared in the Hypothesis service.
+ */
+function isFeatureEnabled(state, feature) {
+  return !!state.session.features[feature];
+}
+
 module.exports = {
   init: init,
   update: update,
@@ -56,4 +67,7 @@ module.exports = {
   actions: {
     updateSession: updateSession,
   },
+
+  // Selectors
+  isFeatureEnabled: isFeatureEnabled,
 };

h/static/scripts/root-thread.js

@@ -4,6 +4,7 @@ var buildThread = require('./build-thread');
 var events = require('./events');
 var memoize = require('./util/memoize');
 var metadata = require('./annotation-metadata');
+var tabs = require('./tabs');
 var uiConstants = require('./ui-constants');
 
 function truthyKeys(map) {
@@ -39,7 +40,7 @@ var sortFns = {
  * The root thread is then displayed by viewer.html
  */
 // @ngInject
-function RootThread($rootScope, annotationUI, drafts, features, searchFilter, viewFilter) {
+function RootThread($rootScope, annotationUI, drafts, searchFilter, viewFilter) {
 
   /**
    * Build the root conversation thread from the given UI state.
@@ -60,37 +61,15 @@ function RootThread($rootScope, annotationUI, drafts, features, searchFilter, vi
 
     var threadFilterFn;
     if (state.isSidebar && !state.filterQuery) {
-      if (!features.flagEnabled('orphans_tab')) {
-        threadFilterFn = function (thread) {
-          if (!thread.annotation) {
-            return false;
-          }
-          if (state.selectedTab === uiConstants.TAB_NOTES) {
-            return metadata.isPageNote(thread.annotation);
-          } else {
-            return metadata.isAnnotation(thread.annotation) || metadata.isOrphan(thread.annotation);
-          }
-        };
-      } else {
-        threadFilterFn = function (thread) {
-          if (!thread.annotation) {
-            return false;
-          }
-          if (metadata.isWaitingToAnchor(thread.annotation)) {
-            return false;
-          }
-          switch (state.selectedTab) {
-          case uiConstants.TAB_ANNOTATIONS:
-            return metadata.isAnnotation(thread.annotation);
-          case uiConstants.TAB_ORPHANS:
-            return metadata.isOrphan(thread.annotation);
-          case uiConstants.TAB_NOTES:
-            return metadata.isPageNote(thread.annotation);
-          default:
-            throw new Error('Invalid selected tab');
-          }
-        };
-      }
+      threadFilterFn = function (thread) {
+        if (!thread.annotation) {
+          return false;
+        }
+
+        var separateOrphans = tabs.shouldSeparateOrphans(state);
+        return tabs.shouldShowInTab(thread.annotation, state.selectedTab,
+          separateOrphans);
+      };
     }
 
     // Get the currently loaded annotations and the set of inputs which

h/static/scripts/tab-counts.js

-'use strict';
-
-var metadata = require('./annotation-metadata');
-var countIf = require('./util/array-util').countIf;
-
-/**
- * Return a count of the number of Annotations, Page Notes, Orphans and
- * annotations still being anchored in a set of `annotations`
- */
-function tabCounts(annotations, opts) {
-  opts = opts || {separateOrphans: false};
-
-  var counts = {
-    notes: countIf(annotations, metadata.isPageNote),
-    annotations: countIf(annotations, metadata.isAnnotation),
-    orphans: countIf(annotations, metadata.isOrphan),
-    anchoring: countIf(annotations, metadata.isWaitingToAnchor),
-  };
-
-  if (opts.separateOrphans) {
-    return counts;
-  } else {
-    return Object.assign({}, counts, {
-      annotations: counts.annotations + counts.orphans,
-      orphans: 0,
-    });
-  }
-}
-
-module.exports = tabCounts;

h/static/scripts/tabs.js

+'use strict';
+
+// Selectors that calculate the annotation counts displayed in tab headings
+// and determine which tab an annotation should be displayed in.
+
+var countIf = require('./util/array-util').countIf;
+var metadata = require('./annotation-metadata');
+var session = require('./reducers/session');
+var uiConstants = require('./ui-constants');
+
+/**
+ * Return true if Annotations and Orphans should be displayed in separate tabs.
+ *
+ * @param {object} state - The current application state.
+ */
+function shouldSeparateOrphans(state) {
+  return session.isFeatureEnabled(state, 'orphans_tab');
+}
+
+/**
+ * Return the tab in which an annotation should be displayed.
+ *
+ * @param {Annotation} ann
+ * @param {boolean} separateOrphans - True if orphans should be displayed in a
+ *        separate tab.
+ */
+function tabForAnnotation(ann, separateOrphans) {
+  if (metadata.isOrphan(ann) && separateOrphans) {
+    return uiConstants.TAB_ORPHANS;
+  } else if (metadata.isPageNote(ann)) {
+    return uiConstants.TAB_NOTES;
+  } else {
+    return uiConstants.TAB_ANNOTATIONS;
+  }
+}
+
+/**
+ * Return true if an annotation should be displayed in a given tab.
+ *
+ * @param {Annotation} ann
+ * @param {number} tab - The TAB_* value indicating the tab
+ * @param {boolean} separateOrphans - True if orphans should be separated into
+ *        their own tab.
+ */
+function shouldShowInTab(ann, tab, separateOrphans) {
+  if (metadata.isWaitingToAnchor(ann) && separateOrphans) {
+    // Until this annotation anchors or fails to anchor, we do not know which
+    // tab it should be displayed in.
+    return false;
+  }
+  return tabForAnnotation(ann, separateOrphans) === tab;
+}
+
+/**
+ * Return the counts for the headings of different tabs.
+ *
+ * @param {Annotation[]} annotations - List of annotations to display
+ * @param {boolean} separateOrphans - True if orphans should be displayed in a
+ *        separate tab.
+ */
+function counts(annotations, separateOrphans) {
+  var counts = {
+    notes: countIf(annotations, metadata.isPageNote),
+    annotations: countIf(annotations, metadata.isAnnotation),
+    orphans: countIf(annotations, metadata.isOrphan),
+    anchoring: countIf(annotations, metadata.isWaitingToAnchor),
+  };
+
+  if (separateOrphans) {
+    return counts;
+  } else {
+    return Object.assign({}, counts, {
+      annotations: counts.annotations + counts.orphans,
+      orphans: 0,
+    });
+  }
+}
+
+module.exports = {
+  counts: counts,
+  shouldSeparateOrphans: shouldSeparateOrphans,
+  shouldShowInTab: shouldShowInTab,
+  tabForAnnotation: tabForAnnotation,
+};

h/static/scripts/test/root-thread-test.js

@@ -27,7 +27,6 @@ describe('rootThread', function () {
   var fakeAnnotationUI;
   var fakeBuildThread;
   var fakeDrafts;
-  var fakeFeatures;
   var fakeSearchFilter;
   var fakeViewFilter;
 
@@ -46,6 +45,9 @@ describe('rootThread', function () {
         highlighted: [],
         isSidebar: true,
         selectedAnnotationMap: null,
+        session: {
+          features: {'orphans_tab': true},
+        },
         sortKey: 'Location',
         sortKeysAvailable: ['Location'],
         visibleHighlights: false,
@@ -68,10 +70,6 @@ describe('rootThread', function () {
       remove: sinon.stub(),
     };
 
-    fakeFeatures = {
-      flagEnabled: sinon.stub().returns(true),
-    };
-
     fakeSearchFilter = {
       generateFacetedFilter: sinon.stub(),
     };
@@ -83,7 +81,6 @@ describe('rootThread', function () {
     angular.module('app', [])
       .value('annotationUI', fakeAnnotationUI)
       .value('drafts', fakeDrafts)
-      .value('features', fakeFeatures)
       .value('searchFilter', fakeSearchFilter)
       .value('viewFilter', fakeViewFilter)
       .service('rootThread', proxyquire('../root-thread', {
@@ -276,22 +273,6 @@ describe('rootThread', function () {
       // pages, since we show all types of annotations here
       assert.notOk(threadFilterFn);
     });
-
-    it('filter does not match annotation when it is still waiting to anchor', function () {
-      fakeBuildThread.reset();
-
-      fakeAnnotationUI.state = Object.assign({}, fakeAnnotationUI.state,
-        {selectedTab: uiConstants.TAB_ANNOTATIONS});
-
-      rootThread.thread(fakeAnnotationUI.state);
-      var threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn;
-
-      var annotation = {
-        $orphan: undefined,
-        target: [{ selector: {} }],
-      };
-      assert.isFalse(threadFilterFn({annotation: annotation}));
-    });
   });
 
   describe('when the filter query changes', function () {

h/static/scripts/test/tab-counts-test.js

-'use strict';
-
-var fixtures = require('./annotation-fixtures');
-var tabCounts = require('../tab-counts');
-
-describe('tabCounts', function () {
-  var annotation = Object.assign(fixtures.defaultAnnotation(), {$orphan:false});
-  var orphan = Object.assign(fixtures.defaultAnnotation(), {$orphan:true});
-
-  it('counts Annotations and Orphans together when the Orphans tab is not enabled', function () {
-    assert.deepEqual(tabCounts([annotation, orphan]), {
-      anchoring: 0,
-      annotations: 2,
-      notes: 0,
-      orphans: 0,
-    });
-  });
-
-  it('counts Annotations and Orphans separately when the Orphans tab is enabled', function () {
-    assert.deepEqual(tabCounts([annotation, orphan], {separateOrphans: true}), {
-      anchoring: 0,
-      annotations: 1,
-      notes: 0,
-      orphans: 1,
-    });
-  });
-});

h/static/scripts/test/tabs-test.js

+'use strict';
+
+var fixtures = require('./annotation-fixtures');
+var uiConstants = require('../ui-constants');
+var tabs = require('../tabs');
+var unroll = require('./util').unroll;
+
+describe('tabs', function () {
+  describe('shouldSeparateOrphans', function () {
+    it('returns true if the "orphans_tab" flag is enabled', function () {
+      var state = {
+        session: {
+          features: {'orphans_tab': true},
+        },
+      };
+      assert.isTrue(tabs.shouldSeparateOrphans(state));
+    });
+
+    it('returns false if the "orphans_tab" flag is not enabled', function () {
+      var state = {
+        session: {
+          features: {'orphans_tab': false},
+        },
+      };
+      assert.isFalse(tabs.shouldSeparateOrphans(state));
+    });
+  });
+
+  describe('tabForAnnotation', function () {
+    unroll('shows annotation in correct tab', function (testCase) {
+      var ann = testCase.ann;
+      var expectedTab = testCase.expectedTab;
+      assert.equal(tabs.tabForAnnotation(ann, true /* separateOrphans */), expectedTab);
+    }, [{
+      ann: fixtures.defaultAnnotation(),
+      expectedTab: uiConstants.TAB_ANNOTATIONS,
+    },{
+      ann: fixtures.oldPageNote(),
+      expectedTab: uiConstants.TAB_NOTES,
+    },{
+      ann: Object.assign(fixtures.defaultAnnotation(), {$orphan: true}),
+      expectedTab: uiConstants.TAB_ORPHANS,
+    }]);
+  });
+
+  describe('shouldShowInTab', function () {
+    unroll('returns true if the annotation should be shown', function (testCase) {
+      var ann = fixtures.defaultAnnotation();
+      ann.$orphan = testCase.orphan;
+
+      assert.equal(tabs.shouldShowInTab(ann, uiConstants.TAB_ANNOTATIONS,
+        testCase.separateOrphans), testCase.expectedTab === uiConstants.TAB_ANNOTATIONS);
+      assert.equal(tabs.shouldShowInTab(ann, uiConstants.TAB_ORPHANS,
+        testCase.separateOrphans), testCase.expectedTab === uiConstants.TAB_ORPHANS);
+    }, [{
+      orphan: undefined,
+      separateOrphans: false,
+      expectedTab: uiConstants.TAB_ANNOTATIONS,
+    },{
+      orphan: undefined,
+      separateOrphans: true,
+      expectedTab: null,
+    },{
+      orphan: false,
+      separateOrphans: true,
+      expectedTab: uiConstants.TAB_ANNOTATIONS,
+    },{
+      orphan: true,
+      separateOrphans: true,
+      expectedTab: uiConstants.TAB_ORPHANS,
+    }]);
+  });
+
+  describe('counts', function () {
+    var annotation = Object.assign(fixtures.defaultAnnotation(), {$orphan:false});
+    var orphan = Object.assign(fixtures.defaultAnnotation(), {$orphan:true});
+
+    it('counts Annotations and Orphans together when the Orphans tab is not enabled', function () {
+      assert.deepEqual(tabs.counts([annotation, orphan], false), {
+        anchoring: 0,
+        annotations: 2,
+        notes: 0,
+        orphans: 0,
+      });
+    });
+
+    it('counts Annotations and Orphans separately when the Orphans tab is enabled', function () {
+      assert.deepEqual(tabs.counts([annotation, orphan], true), {
+        anchoring: 0,
+        annotations: 1,
+        notes: 0,
+        orphans: 1,
+      });
+    });
+  });
+});

h/static/scripts/widget-controller.js

@@ -3,9 +3,7 @@
 var SearchClient = require('./search-client');
 var events = require('./events');
 var memoize = require('./util/memoize');
-var metadata = require('./annotation-metadata');
-var tabCounts = require('./tab-counts');
-var uiConstants = require('./ui-constants');
+var tabs = require('./tabs');
 
 function firstKey(object) {
   for (var k in object) {
@@ -47,9 +45,8 @@ module.exports = function WidgetController(
     $scope.rootThread = thread();
     $scope.selectedTab = state.selectedTab;
 
-    var counts = tabCounts(state.annotations, {
-      separateOrphans: features.flagEnabled('orphans_tab'),
-    });
+    var separateOrphans = tabs.shouldSeparateOrphans(state);
+    var counts = tabs.counts(state.annotations, separateOrphans);
 
     Object.assign($scope, {
       totalNotes: counts.notes,
@@ -76,23 +73,6 @@ module.exports = function WidgetController(
     frameSync.scrollToAnnotation(annotation.$tag);
   }
 
-  /** Returns the annotation type - note or annotation of the first annotation
-   *  in `results` whose ID is a key in `selectedAnnotationMap`.
-   */
-  function tabContainingAnnotation(annot) {
-    if (metadata.isOrphan(annot)) {
-      if (features.flagEnabled('orphans_tab')) {
-        return uiConstants.TAB_ORPHANS;
-      } else {
-        return uiConstants.TAB_ANNOTATIONS;
-      }
-    } else if (metadata.isPageNote(annot)) {
-      return uiConstants.TAB_NOTES;
-    } else {
-      return uiConstants.TAB_ANNOTATIONS;
-    }
-  }
-
   /**
    * Returns the Annotation object for the first annotation in the
    * selected annotation set. Note that 'first' refers to the order
@@ -249,8 +229,8 @@ module.exports = function WidgetController(
     focusAnnotation(selectedAnnot);
     scrollToAnnotation(selectedAnnot);
 
-    var targetTab = tabContainingAnnotation(selectedAnnot);
-    annotationUI.selectTab(targetTab);
+    var separateOrphans = tabs.shouldSeparateOrphans(annotationUI.getState());
+    annotationUI.selectTab(tabs.tabForAnnotation(selectedAnnot, separateOrphans));
   });
 
   $scope.$on(events.GROUP_FOCUSED, function () {