Merge pull request #756 from hypothesis/fix-group-refetch-triggers

Remove unnecessary extra `/api/groups` call on startup

src/sidebar/events.js

@@ -11,8 +11,6 @@ module.exports = {
 
   // Session state changes
 
-  /** The list of groups changed */
-  GROUPS_CHANGED: 'groupsChanged',
   /** The logged-in user changed */
   USER_CHANGED: 'userChanged',
   /**

src/sidebar/services/groups.js

@@ -20,8 +20,6 @@ var serviceConfig = require('../service-config');
 // @ngInject
 function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, session,
                 settings) {
-  var documentUri;
-
   var svc = serviceConfig(settings);
   var authority = svc ? svc.authority : null;
 
@@ -40,6 +38,12 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
     return awaitStateChange(store, mainUri);
   }
 
+  // The document URI passed to the most recent `GET /api/groups` call in order
+  // to include groups associated with this page. This is retained to determine
+  // whether we need to re-fetch groups if the URLs of frames connected to the
+  // sidebar app changes.
+  var documentUri;
+
   /**
    * Fetch the list of applicable groups from the API.
    *
@@ -61,6 +65,7 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
       if (uri) {
         params.document_uri = uri;
       }
+      documentUri = uri;
       return api.groups.list(params);
     }).then(groups => {
       var isFirstLoad = store.allGroups().length === 0;
@@ -126,12 +131,6 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
     }
   });
 
-  // reset the focused group if the user leaves it
-  $rootScope.$on(events.GROUPS_CHANGED, function () {
-    // return for use in test
-    return load();
-  });
-
   // refetch the list of groups when user changes
   $rootScope.$on(events.USER_CHANGED, () => {
     // FIXME Makes a second api call on page load. better way?
@@ -141,8 +140,6 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
 
   // refetch the list of groups when document url changes
   $rootScope.$on(events.FRAME_CONNECTED, () => {
-    // FIXME Makes a third api call on page load. better way?
-    // return for use in test
     return getDocumentUriForGroupSearch().then(uri => {
       if (documentUri !== uri) {
         documentUri = uri;

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

@@ -3,7 +3,6 @@
 var events = require('../../events');
 var fakeReduxStore = require('../../test/fake-redux-store');
 var groups = require('../groups');
-var unroll = require('../../../shared/test/util').unroll;
 
 // Return a mock session service containing three groups.
 var sessionWithThreeGroups = function() {
@@ -68,7 +67,7 @@ describe('groups', function() {
       },
 
       $on: function(event, callback) {
-        if (event === events.GROUPS_CHANGED || event === events.USER_CHANGED || event === events.FRAME_CONNECTED) {
+        if (event === events.USER_CHANGED || event === events.FRAME_CONNECTED) {
           this.eventCallbacks[event] = callback;
         }
       },
@@ -250,18 +249,41 @@ describe('groups', function() {
   });
 
   describe('calls load on various events', function () {
-    var changeEvents = [
-      {event: events.GROUPS_CHANGED},
-      {event: events.USER_CHANGED},
-      {event: events.FRAME_CONNECTED},
-    ];
-
-    unroll('should fetch the list of groups from the server when #event occurs', function (testCase) {
+    it('refetches groups when the logged-in user changes', () => {
       service();
 
-      return fakeRootScope.eventCallbacks[testCase.event]().then(() => {
+      return fakeRootScope.eventCallbacks[events.USER_CHANGED]().then(() => {
         assert.calledOnce(fakeApi.groups.list);
       });
-    }, changeEvents);
+    });
+
+    context('when a new frame connects', () => {
+      it('should refetch groups if main frame URL has changed', () => {
+        var svc = service();
+
+        fakeStore.setState({ searchUris: ['https://domain.com/page-a'] });
+        return svc.load().then(() => {
+          // Simulate main frame URL change, eg. due to client-side navigation in
+          // a single page application.
+          fakeApi.groups.list.resetHistory();
+          fakeStore.setState({searchUris: ['https://domain.com/page-b']});
+
+          return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
+        }).then(() => {
+          assert.calledOnce(fakeApi.groups.list);
+        });
+      });
+
+      it('should not refetch groups if main frame URL has not changed', () => {
+        var svc = service();
+
+        fakeStore.setState({ searchUris: ['https://domain.com/page-a'] });
+        return svc.load().then(() => {
+          return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
+        }).then(() => {
+          assert.calledOnce(fakeApi.groups.list);
+        });
+      });
+    });
   });
 });