Merge pull request #2591 from hypothesis/t105-group_push_notifications

Push notifications of group membership changes

h/static/scripts/app-controller.coffee

 angular = require('angular')
 
+events = require('./events');
+
 module.exports = class AppController
   this.$inject = [
     '$controller', '$document', '$location', '$route', '$scope', '$window',
@@ -38,20 +40,18 @@ module.exports = class AppController
     # Default sort
     $scope.sort = name: 'Location'
 
-    $scope.$on('groupFocused', (event) ->
+    # Reload the view when the focused group changes or the
+    # list of groups that the user is a member of changes
+    reloadEvents = [events.SESSION_CHANGED, events.GROUP_FOCUSED];
+    reloadEvents.forEach((eventName) ->
+      $scope.$on(eventName, (event) ->
         $route.reload()
       )
+    );
 
     identity.watch({
       onlogin: (identity) -> $scope.auth.user = auth.userid(identity)
-      onlogout: ->
-        $scope.auth.user = null
-
-        # Currently all groups are private so when the user logs out they can
-        # no longer see the annotations from any group they may have had
-        # focused. Focus the public group instead, so that they see any public
-        # annotations in the sidebar.
-        groups.focus('__world__')
+      onlogout: -> $scope.auth.user = null
       onready: -> $scope.auth.user ?= null
     })
 
@@ -77,10 +77,6 @@ module.exports = class AppController
       else
         $scope.accountDialog.visible = false
 
-      # Reload the view if this is not the initial load.
-      if oldVal isnt undefined
-        $route.reload()
-
     $scope.login = ->
       $scope.accountDialog.visible = true
       identity.request({

h/static/scripts/events.js

+/**
+ * This module defines the set of global events that are dispatched
+ * on $rootScope
+ */
+
+module.exports = {
+  /** Broadcast when the currently selected group changes */
+  GROUP_FOCUSED: 'groupFocused',
+  /** Broadcast when the session state is updated.
+   * This event is NOT broadcast after the initial session load.
+   */
+  SESSION_CHANGED: 'sessionChanged',
+};

h/static/scripts/groups.js

@@ -2,8 +2,12 @@
  * @ngdoc service
  * @name  groups
  *
- * @description
- * Get and set the UI's currently focused group.
+ * @description Provides access to the list of groups that the user is currently
+ *              a member of and the currently selected group in the UI.
+ *
+ *              The list of groups is initialized from the session state
+ *              and can then later be updated using the add() and remove()
+ *              methods.
  */
 'use strict';
 
@@ -11,20 +15,22 @@ var baseURI = require('document-base-uri');
 
 var STORAGE_KEY = 'hypothesis.groups.focus';
 
+var events = require('./events');
+
 // @ngInject
 function groups(localStorage, session, $rootScope, features, $http) {
   // The currently focused group. This is the group that's shown as selected in
   // the groups dropdown, the annotations displayed are filtered to only ones
   // that belong to this group, and any new annotations that the user creates
   // will be created in this group.
-  var focused;
+  var focusedGroup;
 
-  var all = function all() {
+  function all() {
     return session.state.groups || [];
   };
 
   // Return the full object for the group with the given id.
-  var get = function get(id) {
+  function get(id) {
     var gs = all();
     for (var i = 0, max = gs.length; i < max; i++) {
       if (gs[i].id === id) {
@@ -42,44 +48,59 @@ function groups(localStorage, session, $rootScope, features, $http) {
       url: baseURI + 'groups/' + id + '/leave',
     });
 
-    // TODO - Optimistically call remove() to
-    // remove the group locally when
-    // https://github.com/hypothesis/h/pull/2587 has been merged
+    // the groups list will be updated in response to a session state
+    // change notification from the server. We could improve the UX here
+    // by optimistically updating the session state
 
     return response;
   };
 
-  return {
-    all: all,
-    get: get,
-
-    leave: leave,
 
-    // Return the currently focused group. If no group is explicitly focused we
-    // will check localStorage to see if we have persisted a focused group from
-    // a previous session. Lastly, we fall back to the first group available.
-    focused: function() {
-      if (focused) {
-        return focused;
+  /** Return the currently focused group. If no group is explicitly focused we
+   * will check localStorage to see if we have persisted a focused group from
+   * a previous session. Lastly, we fall back to the first group available.
+   */
+  function focused() {
+    if (focusedGroup) {
+     return focusedGroup;
     } else if (features.flagEnabled('groups')) {
      var fromStorage = get(localStorage.getItem(STORAGE_KEY));
-        if (typeof fromStorage !== 'undefined') {
-          focused = fromStorage;
-          return focused;
+     if (fromStorage) {
+       focusedGroup = fromStorage;
+       return focusedGroup;
      }
     }
     return all()[0];
-    },
+  }
 
-    // Set the group with the passed id as the currently focused group.
-    focus: function(id) {
+  /** Set the group with the passed id as the currently focused group. */
+  function focus(id) {
    var g = get(id);
-      if (typeof g !== 'undefined') {
-        focused = g;
+   if (g) {
+     focusedGroup = g;
      localStorage.setItem(STORAGE_KEY, g.id);
-        $rootScope.$broadcast('groupFocused', g.id);
+     $rootScope.$broadcast(events.GROUP_FOCUSED, g.id);
+   }
+  }
+
+  // reset the focused group if the user leaves it
+  $rootScope.$on(events.SESSION_CHANGED, function () {
+    if (focusedGroup) {
+      focusedGroup = get(focusedGroup.id);
+      if (!focusedGroup) {
+        $rootScope.$broadcast(events.GROUP_FOCUSED, focused());
       }
     }
+  });
+
+  return {
+    all: all,
+    get: get,
+
+    leave: leave,
+
+    focused: focused,
+    focus: focus,
   };
 }
 

h/static/scripts/session.js

@@ -3,6 +3,8 @@
 var Promise = require('core-js/library/es6/promise');
 var angular = require('angular');
 
+var events = require('./events');
+
 var CACHE_TTL = 5 * 60 * 1000; // 5 minutes
 
 var ACCOUNT_ACTIONS = [
@@ -56,7 +58,7 @@ function sessionActions(options) {
  *
  * @ngInject
  */
-function session($document, $http, $resource, flash) {
+function session($document, $http, $resource, $rootScope, flash) {
  // TODO: Move accounts data management (e.g. profile, edit_profile,
  // disable_user, etc) into another module with another route.
   var actions = sessionActions({
@@ -100,6 +102,33 @@ function session($document, $http, $resource, flash) {
     return lastLoad;
   };
 
+  /**
+   * @name session.update()
+   *
+   * @description Update the session state using the provided data.
+   *              This is a counterpart to load(). Whereas load() makes
+   *              a call to the server and then updates itself from
+   *              the response, update() can be used to update the client
+   *              when new state has been pushed to it by the server.
+   */
+  resource.update = function (model) {
+    var isInitialLoad = !resource.state.csrf;
+
+    // Copy the model data (including the CSRF token) into `resource.state`.
+    angular.copy(model, resource.state);
+
+    // Replace lastLoad with the latest data, and update lastLoadTime.
+    lastLoad = {$promise: Promise.resolve(model), $resolved: true};
+    lastLoadTime = Date.now();
+
+    if (!isInitialLoad) {
+      $rootScope.$broadcast(events.SESSION_CHANGED);
+    }
+
+    // Return the model
+    return model;
+  };
+
   function prepare(data, headersGetter) {
     var csrfTok = resource.state.csrf;
     if (typeof csrfTok !== 'undefined') {
@@ -131,15 +160,7 @@ function session($document, $http, $resource, flash) {
       }
     }
 
-    // Copy the model data (including the CSRF token) into `resource.state`.
-    angular.copy(model, resource.state);
-
-    // Replace lastLoad with the latest data, and update lastLoadTime.
-    lastLoad = {$promise: Promise.resolve(model), $resolved: true};
-    lastLoadTime = Date.now();
-
-    // Return the model
-    return model;
+    return resource.update(model);
   }
 
   return resource;

h/static/scripts/streamer.js

@@ -16,11 +16,12 @@ var socket;
  * @param $websocket - angular-websocket constructor
  * @param annotationMapper - The local annotation store
  * @param groups - The local groups store
+ * @param session - Provides access to read and update the session state
  *
  * @return An angular-websocket wrapper around the socket.
  */
 // @ngInject
-function connect($websocket, annotationMapper, groups) {
+function connect($websocket, annotationMapper, groups, session) {
   // Get the socket URL
   var url = new URL('/ws', baseURI);
   url.protocol = url.protocol.replace('http', 'ws');
@@ -39,12 +40,7 @@ function connect($websocket, annotationMapper, groups) {
     value: clientId
   })
 
-  // Listen for updates
-  socket.onMessage(function (event) {
-    message = JSON.parse(event.data)
-    if (!message || message.type !== 'annotation-notification') {
-      return;
-    }
+  function handleAnnotationNotification(message) {
     action = message.options.action
     annotations = message.payload
 
@@ -69,6 +65,26 @@ function connect($websocket, annotationMapper, groups) {
         annotationMapper.unloadAnnotations(annotations);
         break;
     }
+  }
+
+  function handleSessionChangeNotification(message) {
+    session.update(message.model);
+  }
+
+  // Listen for updates
+  socket.onMessage(function (event) {
+    message = JSON.parse(event.data);
+    if (!message) {
+      return;
+    }
+
+    if (message.type === 'annotation-notification') {
+      handleAnnotationNotification(message)
+    } else if (message.type === 'session-change') {
+      handleSessionChangeNotification(message)
+    } else {
+      console.warn('received unsupported notification', message.type)
+    }
   });
 
   return socket

h/static/scripts/test/app-controller-test.coffee

 {module, inject} = angular.mock
+events = require('../events')
 
 describe 'AppController', ->
   $controller = null
@@ -21,7 +22,7 @@ describe 'AppController', ->
     $controller('AppController', locals)
 
   before ->
-    angular.module('h')
+    angular.module('h', [])
     .controller('AppController', require('../app-controller'))
     .controller('AnnotationUIController', angular.noop)
 
@@ -123,12 +124,6 @@ describe 'AppController', ->
     onlogout()
     assert.strictEqual($scope.auth.user, null)
 
-  it 'focuses the default group in logout', ->
-    createController()
-    {onlogout} = fakeIdentity.watch.args[0][0]
-    onlogout()
-    assert.calledWith(fakeGroups.focus, '__world__')
-
   it 'does not show login form for logged in users', ->
     createController()
     assert.isFalse($scope.accountDialog.visible)
@@ -137,7 +132,14 @@ describe 'AppController', ->
     createController()
     assert.isFalse($scope.shareDialog.visible)
 
-  it 'calls $route.reload() when the focused group changes', ->
+  it 'reloads the view when the focused group changes', ->
+    createController()
+    fakeRoute.reload = sinon.spy()
+    $scope.$broadcast(events.GROUP_FOCUSED)
+    assert.calledOnce(fakeRoute.reload)
+
+  it 'reloads the view when the session state changes', ->
     createController()
-    $scope.$broadcast('groupFocused')
+    fakeRoute.reload = sinon.spy()
+    $scope.$broadcast(events.SESSION_CHANGED)
     assert.calledOnce(fakeRoute.reload)

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

@@ -2,6 +2,7 @@
 
 var baseURI = require('document-base-uri');
 
+var events = require('../events');
 var groups = require('../groups');
 
 // Return a mock session service containing three groups.
@@ -17,7 +18,6 @@ var sessionWithThreeGroups = function() {
   };
 };
 
-
 describe('groups', function() {
   var fakeSession;
   var fakeLocalStorage;
@@ -35,7 +35,15 @@ describe('groups', function() {
       setItem: sandbox.stub()
     };
     fakeRootScope = {
-      $broadcast: sandbox.stub()
+      eventCallbacks: [],
+
+      $broadcast: sandbox.stub(),
+
+      $on: function(event, callback) {
+        if (event === events.SESSION_CHANGED) {
+          this.eventCallbacks.push(callback);
+        }
+      }
     };
     fakeFeatures = {
       flagEnabled: function() {return true;}
@@ -107,6 +115,26 @@ describe('groups', function() {
 
       assert.equal(s.focused().id, 'id3');
     });
+
+    it('should update if the user leaves the focused group', function () {
+      var s = service();
+      s.focus('id2');
+
+      var leaveGroup = function(id) {
+        fakeSession.state.groups =
+          fakeSession.state.groups.slice().filter(function (group) {
+            return group.id !== id;
+          });
+          fakeRootScope.eventCallbacks.forEach(function (callback) {
+            callback();
+          });
+      };
+
+      leaveGroup('id3');
+      assert.equal(s.focused().id, 'id2');
+      leaveGroup('id2');
+      assert.notEqual(s.focused().id, 'id2');
+    });
   });
 
   describe('.focus() method', function() {

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

@@ -2,8 +2,12 @@
 
 var mock = angular.mock;
 
+var events = require('../events');
+
 describe('h:session', function () {
   var $httpBackend;
+  var $rootScope;
+
   var fakeFlash;
   var fakeXsrf;
   var sandbox;
@@ -30,9 +34,10 @@ describe('h:session', function () {
   }));
 
 
-  beforeEach(mock.inject(function (_$httpBackend_, _session_) {
+  beforeEach(mock.inject(function (_$httpBackend_, _$rootScope_, _session_) {
     $httpBackend = _$httpBackend_;
     session = _session_;
+    $rootScope = _$rootScope_;
   }));
 
   afterEach(function () {
@@ -151,4 +156,29 @@ describe('h:session', function () {
       $httpBackend.flush();
     });
   });
+
+  describe('#update()', function () {
+    it('broadcasts an event when the session is updated', function () {
+      var sessionChangeCallback = sinon.stub();
+
+      // the initial load should not trigger a SESSION_CHANGED event
+      $rootScope.$on(events.SESSION_CHANGED, sessionChangeCallback);
+      session.update({
+        groups: [{
+          id: 'groupid'
+        }],
+        csrf: 'dummytoken',
+      });
+
+      // subsequent loads should trigger a SESSION_CHANGED event
+      assert.isFalse(sessionChangeCallback.called);
+      session.update({
+        groups: [{
+          id: 'groupid2'
+        }],
+        csrf: 'dummytoken'
+      });
+      assert.calledOnce(sessionChangeCallback);
+    });
+  });
 });

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

@@ -33,6 +33,7 @@ function fakeSocketConstructor(url) {
 describe('streamer', function () {
   var fakeAnnotationMapper;
   var fakeGroups;
+  var fakeSession;
   var socket;
 
   beforeEach(function () {
@@ -47,10 +48,15 @@ describe('streamer', function () {
       },
     };
 
+    fakeSession = {
+      update: sinon.stub(),
+    };
+
     socket = streamer.connect(
       fakeSocketConstructor,
       fakeAnnotationMapper,
-      fakeGroups
+      fakeGroups,
+      fakeSession
     );
   });
 
@@ -97,4 +103,19 @@ describe('streamer', function () {
       assert.ok(fakeAnnotationMapper.unloadAnnotations.calledOnce);
     });
   });
+
+  describe('session change notifications', function () {
+    it('updates the session when a notification is received', function () {
+      var model = {
+        groups: [{
+          id: 'new-group'
+        }]
+      };
+      socket.notify({
+        type: 'session-change',
+        model: model,
+      });
+      assert.ok(fakeSession.update.calledWith(model));
+    });
+  });
 });