Merge pull request #1036 from hypothesis/add-group-url

Add annotations:group:GROUP_ID fragment url

src/annotator/config/index.js

@@ -23,6 +23,7 @@ function configFrom(window_) {
     enableExperimentalNewNoteButton: settings.hostPageSetting(
       'enableExperimentalNewNoteButton'
     ),
+    group: settings.group,
     theme: settings.hostPageSetting('theme'),
     usernameUrl: settings.hostPageSetting('usernameUrl'),
     onLayoutChange: settings.hostPageSetting('onLayoutChange'),

src/annotator/config/settings.js

@@ -102,6 +102,28 @@ function settingsFrom(window_) {
     return jsonConfigs.annotations || annotationsFromURL();
   }
 
+  /**
+   * Return the `#annotations:group:*` ID from the given URL's fragment.
+   *
+   * If the URL contains a `#annotations:group:<GROUP_ID>` fragment then return
+   * the group ID extracted from the fragment. Otherwise return `null`.
+   *
+   * @return {string|null} - The extracted ID, or null.
+   */
+  function group() {
+    function groupFromURL() {
+      const groupFragmentMatch = window_.location.href.match(
+        /#annotations:group:([A-Za-z0-9_-]+)$/
+      );
+      if (groupFragmentMatch) {
+        return groupFragmentMatch[1];
+      }
+      return null;
+    }
+
+    return jsonConfigs.group || groupFromURL();
+  }
+
   function showHighlights() {
     let showHighlights_ = hostPageSetting('showHighlights');
 
@@ -179,6 +201,9 @@ function settingsFrom(window_) {
     get clientUrl() {
       return clientUrl();
     },
+    get group() {
+      return group();
+    },
     get showHighlights() {
       return showHighlights();
     },

src/annotator/config/test/index-test.js

@@ -27,17 +27,17 @@ describe('annotator.config.index', function() {
     assert.calledWithExactly(fakeSettingsFrom, 'WINDOW');
   });
 
-  ['sidebarAppUrl', 'query', 'annotations', 'showHighlights'].forEach(function(
-    settingName
-  ) {
-    it('returns the ' + settingName + ' setting', function() {
+  ['sidebarAppUrl', 'query', 'annotations', 'group', 'showHighlights'].forEach(
+    settingName => {
+      it('returns the ' + settingName + ' setting', () => {
         fakeSettingsFrom()[settingName] = 'SETTING_VALUE';
 
         const config = configFrom('WINDOW');
 
         assert.equal(config[settingName], 'SETTING_VALUE');
       });
-  });
+    }
+  );
 
   context("when there's no application/annotator+html <link>", function() {
     beforeEach('remove the application/annotator+html <link>', function() {

src/annotator/config/test/settings-test.js

@@ -289,6 +289,29 @@ describe('annotator.config.settingsFrom', function() {
     });
   });
 
+  [
+    {
+      description:
+        "returns an object with the group ID when there's a valid #annotations:group:<ID> fragment",
+      url: 'http://localhost:3000#annotations:group:alphanum3ric_-only',
+      returns: 'alphanum3ric_-only',
+    },
+    {
+      description: "returns null when there's a non-alphanumeric group ID",
+      url: 'http://localhost:3000#annotations:group:not%20alphanumeric',
+      returns: null,
+    },
+    {
+      description: "return null when there's an empty group ID",
+      url: 'http://localhost:3000#annotations:group:',
+      returns: null,
+    },
+  ].forEach(test => {
+    it(test.description, () => {
+      assert.deepEqual(settingsFrom(fakeWindow(test.url)).group, test.returns);
+    });
+  });
+
   describe('#query', function() {
     context(
       'when the host page has a js-hypothesis-config with a query setting',

src/sidebar/host-config.js

@@ -17,6 +17,9 @@ function hostPageConfig(window) {
     // Direct-linked annotation ID
     'annotations',
 
+    // Direct-linked group ID
+    'group',
+
     // Default query passed by url
     'query',
 

src/sidebar/services/api.js

@@ -232,6 +232,7 @@ function api(apiRoutes, auth, store) {
       member: {
         delete: apiCall('group.member.delete'),
       },
+      read: apiCall('group.read'),
     },
     groups: {
       list: apiCall('groups.read'),

src/sidebar/services/groups.js

@@ -59,9 +59,15 @@ function groups(
    * @param {Group[]} groups
    * @param {boolean} isLoggedIn
    * @param {string|null} directLinkedAnnotationId
+   * @param {string|null} directLinkedGroupId
    * @return {Promise<Group[]>}
    */
-  function filterGroups(groups, isLoggedIn, directLinkedAnnotationId) {
+  function filterGroups(
+    groups,
+    isLoggedIn,
+    directLinkedAnnotationId,
+    directLinkedGroupId
+  ) {
     // 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) {
@@ -89,18 +95,26 @@ function groups(
     // link to an annotation in that group.
     const nonWorldGroups = groups.filter(g => g.id !== '__world__');
 
-    if (!directLinkedAnnotationId) {
+    if (!directLinkedAnnotationId && !directLinkedGroupId) {
       return Promise.resolve(nonWorldGroups);
     }
 
-    return api.annotation
-      .get({ id: directLinkedAnnotationId })
-      .then(ann => {
-        if (ann.group === '__world__') {
+    // If directLinkedGroup is the "Public" group, always return groups.
+    if (directLinkedGroupId === '__world__') {
       return groups;
-        } else {
-          return nonWorldGroups;
     }
+
+    // If the directLinkedAnnotationId's group is the "Public" group return groups,
+    // otherwise filter out the "Public" group.
+
+    // Force getAnnotation to enter the catch clause if there is no linked annotation.
+    const getAnnotation = directLinkedAnnotationId
+      ? api.annotation.get({ id: directLinkedAnnotationId })
+      : Promise.reject();
+
+    return getAnnotation
+      .then(ann => {
+        return ann.group === '__world__' ? groups : nonWorldGroups;
       })
       .catch(() => {
         // Annotation does not exist or we do not have permission to read it.
@@ -144,6 +158,7 @@ function groups(
     if (isSidebar) {
       uri = getDocumentUriForGroupSearch();
     }
+    const directLinkedGroup = settings.group;
     return uri
       .then(uri => {
         const params = {
@@ -161,20 +176,44 @@ function groups(
           expand: params.expand,
         });
         const listGroupsApi = api.groups.list(params);
-
-        return Promise.all([
+        let groupApiRequests = [
           profileGroupsApi,
           listGroupsApi,
           auth.tokenGetter(),
-        ]).then(([myGroups, featuredGroups, token]) => [
-          combineGroups(myGroups, featuredGroups, documentUri),
+        ];
+
+        // If there is a directLinkedGroup, add an api request to get that
+        // 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,
+          });
+          groupApiRequests = groupApiRequests.concat(selectedGroupApi);
+        }
+        return Promise.all(groupApiRequests).then(
+          ([myGroups, featuredGroups, token, selectedGroup]) => [
+            combineGroups(
+              myGroups,
+              selectedGroup !== undefined
+                ? featuredGroups.concat([selectedGroup])
+                : featuredGroups,
+              documentUri
+            ),
             token,
-        ]);
+          ]
+        );
       })
       .then(([groups, token]) => {
         const isLoggedIn = token !== null;
         const directLinkedAnnotation = settings.annotations;
-        return filterGroups(groups, isLoggedIn, directLinkedAnnotation);
+        return filterGroups(
+          groups,
+          isLoggedIn,
+          directLinkedAnnotation,
+          directLinkedGroup
+        );
       })
       .then(groups => {
         injectOrganizations(groups);
@@ -183,7 +222,9 @@ function groups(
         const prevFocusedGroup = localStorage.getItem(STORAGE_KEY);
 
         store.loadGroups(groups);
-        if (isFirstLoad && groups.some(g => g.id === prevFocusedGroup)) {
+        if (isFirstLoad && groups.some(g => g.id === directLinkedGroup)) {
+          store.focusGroup(directLinkedGroup);
+        } else if (isFirstLoad && groups.some(g => g.id === prevFocusedGroup)) {
           store.focusGroup(prevFocusedGroup);
         }
 

src/sidebar/services/test/api-index.json

@@ -29,6 +29,11 @@
           "method": "DELETE",
           "desc": "Remove the current user from a group."
         }
+      },
+      "read": {
+        "url": "https://example.com/api/groups/:id",
+        "method": "GET",
+        "desc": "Fetch a group."
       }
     },
     "links": {

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

@@ -124,6 +124,14 @@ describe('sidebar.services.api', function() {
     return api.group.member.delete({ pubid: 'an-id', userid: 'me' });
   });
 
+  it('gets a group by provided group id', () => {
+    const group = { id: 'group-id', name: 'Group' };
+    expectCall('get', 'groups/group-id', 200, group);
+    return api.group.read({ id: 'group-id' }).then(group_ => {
+      assert.deepEqual(group_, group);
+    });
+  });
+
   it('removes internal properties before sending data to the server', () => {
     const annotation = {
       $highlight: true,

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

@@ -50,7 +50,6 @@ describe('groups', function() {
   let fakeLocalStorage;
   let fakeRootScope;
   let fakeServiceUrl;
-  let sandbox;
 
   beforeEach(function() {
     fakeAuth = {
@@ -59,7 +58,6 @@ describe('groups', function() {
     fakeFeatures = {
       flagEnabled: sinon.stub().returns(false),
     };
-    sandbox = sinon.sandbox.create();
 
     fakeStore = fakeReduxStore(
       {
@@ -92,8 +90,8 @@ describe('groups', function() {
     fakeSession = sessionWithThreeGroups();
     fakeIsSidebar = true;
     fakeLocalStorage = {
-      getItem: sandbox.stub(),
-      setItem: sandbox.stub(),
+      getItem: sinon.stub(),
+      setItem: sinon.stub(),
     };
     fakeRootScope = {
       eventCallbacks: {},
@@ -108,7 +106,7 @@ describe('groups', function() {
         }
       },
 
-      $broadcast: sandbox.stub(),
+      $broadcast: sinon.stub(),
     };
     fakeApi = {
       annotation: {
@@ -117,24 +115,21 @@ describe('groups', function() {
 
       group: {
         member: {
-          delete: sandbox.stub().returns(Promise.resolve()),
+          delete: sinon.stub().returns(Promise.resolve()),
         },
+        read: sinon.stub().returns(Promise.resolve()),
       },
       groups: {
-        list: sandbox.stub().returns(dummyGroups),
+        list: sinon.stub().returns(dummyGroups),
       },
       profile: {
         groups: {
-          read: sandbox.stub().returns(Promise.resolve([dummyGroups[0]])),
+          read: sinon.stub().returns(Promise.resolve([dummyGroups[0]])),
         },
       },
     };
-    fakeServiceUrl = sandbox.stub();
-    fakeSettings = {};
-  });
-
-  afterEach(function() {
-    sandbox.restore();
+    fakeServiceUrl = sinon.stub();
+    fakeSettings = { group: null };
   });
 
   function service() {
@@ -202,6 +197,45 @@ describe('groups', function() {
       });
     });
 
+    it('combines groups from all 3 endpoints if there is a selectedGroup', () => {
+      const svc = service();
+
+      fakeSettings.group = 'selected-id';
+      const groups = [
+        { id: 'groupa', name: 'GroupA' },
+        { id: 'groupb', name: 'GroupB' },
+        { id: fakeSettings.group, name: 'Selected Group' },
+      ];
+
+      fakeApi.profile.groups.read.returns(Promise.resolve([groups[0]]));
+      fakeApi.groups.list.returns(Promise.resolve([groups[1]]));
+      fakeApi.group.read.returns(Promise.resolve(groups[2]));
+
+      return svc.load().then(() => {
+        assert.calledWith(fakeStore.loadGroups, groups);
+      });
+    });
+
+    it('passes the groupid from settings.group to the api.group.read call', () => {
+      const svc = service();
+
+      fakeSettings.group = 'selected-id';
+      const group = { id: fakeSettings.group, name: 'Selected Group' };
+
+      fakeApi.profile.groups.read.returns(Promise.resolve([]));
+      fakeApi.groups.list.returns(Promise.resolve([]));
+      fakeApi.group.read.returns(Promise.resolve(group));
+
+      return svc.load().then(() => {
+        assert.calledWith(
+          fakeApi.group.read,
+          sinon.match({
+            id: fakeSettings.group,
+          })
+        );
+      });
+    });
+
     it('loads all available groups', function() {
       const svc = service();
 
@@ -215,6 +249,7 @@ describe('groups', function() {
       fakeApi.groups.list.returns(
         Promise.resolve([{ id: 'groupa', name: 'GroupA' }])
       );
+      fakeSettings.group = 'group-id';
 
       return svc.load().then(() => {
         assert.calledWith(
@@ -225,6 +260,10 @@ describe('groups', function() {
           fakeApi.groups.list,
           sinon.match({ expand: ['organization', 'scopes'] })
         );
+        assert.calledWith(
+          fakeApi.group.read,
+          sinon.match({ expand: ['organization', 'scopes'] })
+        );
       });
     });
 
@@ -236,6 +275,25 @@ describe('groups', function() {
       });
     });
 
+    it('sets the direct-linked group to take precedence over the group saved in local storage', () => {
+      const svc = service();
+      fakeSettings.group = dummyGroups[1].id;
+      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
+      return svc.load().then(() => {
+        assert.calledWith(fakeStore.focusGroup, dummyGroups[1].id);
+      });
+    });
+
+    it('sets the focused group to the linked group', () => {
+      const svc = service();
+      fakeSettings.group = dummyGroups[1].id;
+      fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
+      return svc.load().then(() => {
+        assert.calledWith(fakeStore.focusGroup, fakeSettings.group);
+      });
+    });
+
     [null, 'some-group-id'].forEach(groupId => {
       it('does not set the focused group if not present in the groups list', () => {
         const svc = service();
@@ -300,6 +358,70 @@ describe('groups', function() {
       });
     });
 
+    it('both groups are in the final groups list when an annotation and a group are linked to', () => {
+      // This can happen if the linked to annotation and group are configured by
+      // the frame embedding the client.
+      const svc = service();
+
+      fakeSettings.group = 'out-of-scope';
+      fakeSettings.annotations = 'ann-id';
+
+      fakeApi.profile.groups.read.returns(Promise.resolve([]));
+      fakeApi.groups.list.returns(
+        Promise.resolve([
+          { name: 'BioPub', id: 'biopub' },
+          { name: 'Public', id: '__world__' },
+        ])
+      );
+      fakeApi.group.read.returns(
+        Promise.resolve({ name: 'Restricted', id: 'out-of-scope' })
+      );
+      fakeApi.annotation.get.returns(
+        Promise.resolve({
+          id: 'ann-id',
+          group: '__world__',
+        })
+      );
+
+      // The user is logged out.
+      fakeAuth.tokenGetter.returns(null);
+
+      return svc.load().then(groups => {
+        const linkedToGroupShown = groups.some(g => g.id === 'out-of-scope');
+        assert.isTrue(linkedToGroupShown);
+        const linkedToAnnGroupShown = groups.some(g => g.id === '__world__');
+        assert.isTrue(linkedToAnnGroupShown);
+      });
+    });
+
+    it('includes the "Public" group if the user links to it', () => {
+      // Set up the test under conditions that would otherwise
+      // not return the Public group. Aka: the user is logged
+      // out and there are associated groups.
+      const svc = service();
+
+      fakeSettings.group = '__world__';
+      fakeSettings.annotations = undefined;
+
+      fakeApi.profile.groups.read.returns(Promise.resolve([]));
+      fakeApi.groups.list.returns(
+        Promise.resolve([
+          { name: 'BioPub', id: 'biopub' },
+          { name: 'Public', id: '__world__' },
+        ])
+      );
+      fakeApi.group.read.returns(
+        Promise.resolve({ name: 'Public', id: '__world__' })
+      );
+
+      fakeAuth.tokenGetter.returns(null);
+
+      return svc.load().then(groups => {
+        const publicGroupShown = groups.some(g => g.id === '__world__');
+        assert.isTrue(publicGroupShown);
+      });
+    });
+
     truthTable(3).forEach(
       ([loggedIn, pageHasAssociatedGroups, directLinkToPublicAnnotation]) => {
         it('excludes the "Public" group if user logged out and page has associated groups', () => {
@@ -319,7 +441,7 @@ describe('groups', function() {
             );
             fakeSettings.annotations = 'direct-linked-ann';
           } else {
-            fakeSettings.annotations = null;
+            fakeSettings.annotations = undefined;
           }
 
           // Create groups response from server.
@@ -330,7 +452,6 @@ describe('groups', function() {
 
           fakeAuth.tokenGetter.returns(loggedIn ? '1234' : null);
           fakeApi.groups.list.returns(Promise.resolve(groups));
-          fakeApi.profile.groups.read.returns(Promise.resolve([]));
 
           return svc.load().then(groups => {
             const publicGroupShown = groups.some(g => g.id === '__world__');

src/sidebar/test/host-config-test.js

@@ -14,6 +14,7 @@ describe('hostPageConfig', function() {
   it('parses config from location string and returns whitelisted params', function() {
     const window_ = fakeWindow({
       annotations: '1234',
+      group: 'abc12',
       appType: 'bookmarklet',
       openSidebar: true,
       requestConfigFromFrame: 'https://embedder.com',
@@ -27,6 +28,7 @@ describe('hostPageConfig', function() {
 
     assert.deepEqual(hostPageConfig(window_), {
       annotations: '1234',
+      group: 'abc12',
       appType: 'bookmarklet',
       openSidebar: true,
       requestConfigFromFrame: 'https://embedder.com',