Avoid reloading whole view when groups list changes

When the client receives a notification that the
list of groups changes, update just the groups list
and, if necessary, focused group rather than reloading
the whole view.

This is done by splitting the SESSION_CHANGED event
into finer-grained GROUPS_CHANGED and USER_CHANGED
events. The <groups-list> directive now listens for
GROUPS_CHANGED and updates itself in response.

This fixes an issue where joining or leaving a group
would always result in unsaved changes to annotation text
being lost.

If the user leaves a group which is currently focused and
which has an unsaved changes to an annotation, that will still
result in changes to the annotation being lost.

 * Add finer-grained GROUPS_CHANGED and USER_CHANGED
   events which components can react to.

 * Avoid directly exposing the groups service to the
   <group-list> template and instead only expose
   the required methods. This makes it easier to
   track what is going on in the template.

Fixes #2641

h/static/scripts/app-controller.coffee

@@ -48,7 +48,7 @@ module.exports = class AppController
 
     # 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 = [events.USER_CHANGED, events.GROUP_FOCUSED];
     reloadEvents.forEach((eventName) ->
       $scope.$on(eventName, (event) ->
         $route.reload()

h/static/scripts/directive/group-list.js

 'use strict';
 
+var events = require('../events');
+
 // @ngInject
-function GroupListController($scope, $window) {
+function GroupListController($scope, $window, groups) {
   $scope.expandedGroupId = undefined;
 
   // show the share link for the specified group or clear it if
@@ -19,13 +21,27 @@ function GroupListController($scope, $window) {
   };
 
   $scope.leaveGroup = function (groupId) {
-    var groupName = $scope.groups.get(groupId).name;
+    var groupName = groups.get(groupId).name;
     var message = 'Are you sure you want to leave the group "' +
       groupName + '"?';
     if ($window.confirm(message)) {
-      $scope.groups.leave(groupId);
+      groups.leave(groupId);
     }
   }
+
+  $scope.focusGroup = function (groupId) {
+    groups.focus(groupId);
+  }
+
+  $scope.allGroups = groups.all();
+  $scope.$on(events.GROUPS_CHANGED, function () {
+    $scope.allGroups = groups.all();
+  });
+
+  $scope.focusedGroup = groups.focused();
+  $scope.$on(events.GROUP_FOCUSED, function () {
+    $scope.focusedGroup = groups.focused();
+  });
 }
 
 /**
@@ -39,8 +55,6 @@ function groupList(groups, $window) {
   return {
     controller: GroupListController,
     link: function ($scope, elem, attrs) {
-      $scope.groups = groups;
-
       $scope.createNewGroup = function() {
         $window.open('/groups/new', '_blank');
       }

h/static/scripts/directive/test/group-list-test.js

 'use strict';
 
+var events = require('../../events');
 var groupList = require('../group-list');
 var util = require('./util');
 
@@ -8,8 +9,15 @@ describe('GroupListController', function () {
   var $scope;
 
   beforeEach(function () {
-    $scope = {};
-    controller = new groupList.Controller($scope);
+    $scope = {
+      $on: sinon.stub(),
+    };
+    var fakeWindow = {};
+    var fakeGroups = {
+      all: sinon.stub(),
+      focused: sinon.stub(),
+    };
+    controller = new groupList.Controller($scope, fakeWindow, fakeGroups);
   });
 
   it('toggles share links', function () {
@@ -44,19 +52,12 @@ function isElementHidden(element) {
 }
 
 describe('groupList', function () {
+  var $rootScope;
   var $window;
 
   var GROUP_LINK = 'https://hypothes.is/groups/hdevs';
 
-  var groups = [{
-    id: 'public',
-    public: true
-  },{
-    id: 'h-devs',
-    name: 'Hypothesis Developers',
-    url: GROUP_LINK
-  }];
-
+  var groups;
   var fakeGroups;
 
   before(function() {
@@ -72,9 +73,19 @@ describe('groupList', function () {
     angular.mock.module('h.templates');
   });
 
-  beforeEach(angular.mock.inject(function (_$window_) {
+  beforeEach(angular.mock.inject(function (_$rootScope_, _$window_) {
+    $rootScope = _$rootScope_;
     $window = _$window_;
 
+    groups = [{
+       id: 'public',
+       public: true
+    },{
+       id: 'h-devs',
+       name: 'Hypothesis Developers',
+       url: GROUP_LINK
+    }];
+
     fakeGroups = {
       all: function () {
         return groups;
@@ -87,6 +98,7 @@ describe('groupList', function () {
       },
       leave: sinon.stub(),
       focus: sinon.stub(),
+      focused: sinon.stub(),
     };
   }));
 
@@ -152,4 +164,20 @@ describe('groupList', function () {
     clickLeaveIcon(element, true);
     assert.notCalled(fakeGroups.focus);
   });
+
+  it('should update when a GROUPS_CHANGED event is received', function () {
+    var element = createGroupList();
+    var groupItems = element.find('.group-item');
+    assert.equal(groupItems.length, groups.length + 1);
+
+    groups.push({
+      id: 'new-group',
+      name: 'New Group',
+      url: GROUP_LINK
+    });
+    $rootScope.$broadcast(events.GROUPS_CHANGED);
+    element.scope.$digest();
+    groupItems = element.find('.group-item');
+    assert.equal(groupItems.length, groups.length + 1);
+  });
 });

h/static/scripts/events.js

@@ -6,6 +6,10 @@
 module.exports = {
   /** Broadcast when the currently selected group changes */
   GROUP_FOCUSED: 'groupFocused',
+  /** Broadcast when the list of groups changes */
+  GROUPS_CHANGED: 'groupsChanged',
+  /** Broadcast when the signed-in user changes */
+  USER_CHANGED: 'userChanged',
   /** Broadcast when the session state is updated.
    * This event is NOT broadcast after the initial session load.
    */

h/static/scripts/groups.js

@@ -84,7 +84,7 @@ function groups(localStorage, session, $rootScope, features, $http) {
   }
 
   // reset the focused group if the user leaves it
-  $rootScope.$on(events.SESSION_CHANGED, function () {
+  $rootScope.$on(events.GROUPS_CHANGED, function () {
     if (focusedGroup) {
       focusedGroup = get(focusedGroup.id);
       if (!focusedGroup) {

h/static/scripts/session.js

@@ -116,6 +116,9 @@ function session($document, $http, $resource, $rootScope, flash) {
   resource.update = function (model) {
     var isInitialLoad = !resource.state.csrf;
 
+    var userChanged = model.userid !== resource.state.userid;
+    var groupsChanged = !angular.equals(model.groups, resource.state.groups);
+
     // Copy the model data (including the CSRF token) into `resource.state`.
     angular.copy(model, resource.state);
 
@@ -130,6 +133,12 @@ function session($document, $http, $resource, $rootScope, flash) {
 
     if (!isInitialLoad) {
       $rootScope.$broadcast(events.SESSION_CHANGED);
+      if (userChanged) {
+        $rootScope.$broadcast(events.USER_CHANGED);
+      }
+      if (groupsChanged) {
+        $rootScope.$broadcast(events.GROUPS_CHANGED);
+      }
     }
 
     // Return the model

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

@@ -151,8 +151,8 @@ describe 'AppController', ->
     $scope.$broadcast(events.GROUP_FOCUSED)
     assert.calledOnce(fakeRoute.reload)
 
-  it 'reloads the view when the session state changes', ->
+  it 'reloads the view when the logged-in user changes', ->
     createController()
     fakeRoute.reload = sinon.spy()
-    $scope.$broadcast(events.SESSION_CHANGED)
+    $scope.$broadcast(events.USER_CHANGED)
     assert.calledOnce(fakeRoute.reload)

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

@@ -40,7 +40,7 @@ describe('groups', function() {
       $broadcast: sandbox.stub(),
 
       $on: function(event, callback) {
-        if (event === events.SESSION_CHANGED) {
+        if (event === events.GROUPS_CHANGED) {
           this.eventCallbacks.push(callback);
         }
       }

h/templates/client/group_list.html

 <span ng-if="auth.status === 'signed-out'"
-      ng-switch on="groups.focused().public">
+      ng-switch on="focusedGroup.public">
   <i class="group-list-label__icon h-icon-public" ng-switch-when="true"></i><!-- nospace
   !--><i class="group-list-label__icon h-icon-group" ng-switch-default></i>
-  <span class="group-list-label__label">{{groups.focused().name}}</span>
+  <span class="group-list-label__label">{{focusedGroup.name}}</span>
 </span>
 
 <div class="pull-right"
@@ -13,20 +13,20 @@
         dropdown-toggle
         data-toggle="dropdown"
         role="button"
-        ng-switch on="groups.focused().public"
+        ng-switch on="focusedGroup.public"
         ng-click="toggleShareLink(undefined)"
         title="Change the selected group">
     <i class="group-list-label__icon h-icon-public" ng-switch-when="true"></i><!-- nospace
     !--><i class="group-list-label__icon h-icon-group" ng-switch-default></i>
-    <span class="group-list-label__label">{{groups.focused().name}}</span><!-- nospace
+    <span class="group-list-label__label">{{focusedGroup.name}}</span><!-- nospace
     !--><i class="h-icon-arrow-drop-down"></i>
   </span>
   <div class="dropdown-menu__top-arrow"></div>
   <ul class="dropdown-menu pull-none" role="menu">
     <li class="dropdown-menu__row dropdown-menu__row--unpadded "
-        ng-repeat="group in groups.all()">
-      <div ng-class="{'group-item': true, selected: group.id == groups.focused().id}"
-           ng-click="groups.focus(group.id)">
+        ng-repeat="group in allGroups">
+      <div ng-class="{'group-item': true, selected: group.id == focusedGroup.id}"
+           ng-click="focusGroup(group.id)">
         <!-- the group icon !-->
         <div class="group-icon-container" ng-switch on="group.public">
           <i class="h-icon-public" ng-switch-when="true"></i>