Merge pull request #1052 from hypothesis/remove-feature-flag-from-group-service

Remove feature flag from group service

src/sidebar/components/group-list-section.js

@@ -4,7 +4,11 @@
 function GroupListSectionController() {
   this.isSelectable = function(groupId) {
     const group = this.sectionGroups.find(g => g.id === groupId);
-    return !this.disableOosGroupSelection || group.isScopedToUri;
+    return (
+      !this.disableOosGroupSelection ||
+      !group.scopes.enforced ||
+      group.isScopedToUri
+    );
   };
 }
 

src/sidebar/components/test/group-list-section-test.js

@@ -31,26 +31,49 @@ describe('groupListSection', () => {
       {
         description: 'always returns true if disableOosGroupSelection is false',
         fakeDisableOosGroupSelection: false,
+        scopesEnforced: true,
         expectedIsSelectable: [true, true],
       },
       {
         description:
           'always returns true if disableOosGroupSelection is undefined',
         fakeDisableOosGroupSelection: undefined,
+        scopesEnforced: true,
         expectedIsSelectable: [true, true],
       },
       {
         description:
           'returns false if disableOosGroupSelection is true and group is out of scope',
         fakeDisableOosGroupSelection: true,
+        scopesEnforced: true,
         expectedIsSelectable: [true, false],
       },
+      {
+        description:
+          'returns true if disableOosGroupSelection is true and group is out of scope but not enforced',
+        fakeDisableOosGroupSelection: true,
+        scopesEnforced: false,
+        expectedIsSelectable: [true, true],
+      },
     ].forEach(
-      ({ description, fakeDisableOosGroupSelection, expectedIsSelectable }) => {
+      ({
+        description,
+        fakeDisableOosGroupSelection,
+        scopesEnforced,
+        expectedIsSelectable,
+      }) => {
         it(description, () => {
           const fakeSectionGroups = [
-            { isScopedToUri: true, id: 0 },
-            { isScopedToUri: false, id: 1 },
+            {
+              isScopedToUri: true,
+              scopes: { enforced: scopesEnforced },
+              id: 0,
+            },
+            {
+              isScopedToUri: false,
+              scopes: { enforced: scopesEnforced },
+              id: 1,
+            },
           ];
 
           const element = createGroupListSection(

src/sidebar/services/groups.js

@@ -16,6 +16,7 @@ const DEFAULT_ORGANIZATION = {
 const events = require('../events');
 const { awaitStateChange } = require('../util/state-util');
 const { combineGroups } = require('../util/groups');
+const memoize = require('../util/memoize');
 const serviceConfig = require('../service-config');
 
 // @ngInject
@@ -77,7 +78,9 @@ function groups(
 
     // If the main document URL has no groups associated with it, always show
     // the "Public" group.
-    const pageHasAssociatedGroups = groups.some(g => g.id !== '__world__');
+    const pageHasAssociatedGroups = groups.some(
+      g => g.id !== '__world__' && g.isScopedToUri
+    );
     if (!pageHasAssociatedGroups) {
       return Promise.resolve(groups);
     }
@@ -144,7 +147,7 @@ function groups(
     return uri
       .then(uri => {
         const params = {
-          expand: 'organization',
+          expand: ['organization', 'scopes'],
         };
         if (authority) {
           params.authority = authority;
@@ -154,13 +157,11 @@ function groups(
         }
         documentUri = uri;
 
-        if (features.flagEnabled('community_groups')) {
-          params.expand = ['organization', 'scopes'];
-          const profileParams = {
-            expand: ['organization', 'scopes'],
-          };
-          const profileGroupsApi = api.profile.groups.read(profileParams);
+        const profileGroupsApi = api.profile.groups.read({
+          expand: params.expand,
+        });
         const listGroupsApi = api.groups.list(params);
+
         return Promise.all([
           profileGroupsApi,
           listGroupsApi,
@@ -169,12 +170,6 @@ function groups(
           combineGroups(myGroups, featuredGroups, documentUri),
           token,
         ]);
-        } else {
-          // Fetch groups from the API.
-          return api.groups
-            .list(params, null, { includeMetadata: true })
-            .then(({ data, token }) => [data, token]);
-        }
       })
       .then(([groups, token]) => {
         const isLoggedIn = token !== null;
@@ -196,9 +191,26 @@ function groups(
       });
   }
 
+  const sortGroups = memoize(groups => {
+    // Sort in the following order: scoped, public, private.
+    // This is for maintaining the order of the old groups menu so when
+    // the old groups menu is removed this can be removed.
+    const worldGroups = groups.filter(g => g.id === '__world__');
+    const nonWorldScopedGroups = groups.filter(
+      g => g.id !== '__world__' && ['open', 'restricted'].includes(g.type)
+    );
+    const remainingGroups = groups.filter(
+      g => !worldGroups.includes(g) && !nonWorldScopedGroups.includes(g)
+    );
+    return nonWorldScopedGroups.concat(worldGroups).concat(remainingGroups);
+  });
+
   function all() {
+    if (features.flagEnabled('community_groups')) {
       return store.allGroups();
     }
+    return sortGroups(store.getInScopeGroups());
+  }
 
   // Return the full object for the group with the given id.
   function get(id) {

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

@@ -70,6 +70,9 @@ describe('groups', function() {
         allGroups() {
           return this.getState().groups;
         },
+        getInScopeGroups() {
+          return this.getState().groups;
+        },
         focusedGroup() {
           return this.getState().focusedGroup;
         },
@@ -114,12 +117,7 @@ describe('groups', function() {
         },
       },
       groups: {
-        list: sandbox.stub().returns(
-          Promise.resolve({
-            data: dummyGroups,
-            token: '1234',
-          })
-        ),
+        list: sandbox.stub().returns(dummyGroups),
       },
       profile: {
         groups: {
@@ -151,15 +149,40 @@ describe('groups', function() {
   }
 
   describe('#all', function() {
-    it('returns all groups', function() {
+    it('returns all groups from store.allGroups when community-groups feature flag is enabled', () => {
       const svc = service();
-      fakeStore.setState({ groups: dummyGroups });
+      fakeStore.allGroups = sinon.stub().returns(dummyGroups);
+      fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
       assert.deepEqual(svc.all(), dummyGroups);
+      assert.called(fakeStore.allGroups);
+    });
+
+    it('returns all groups from store.getInScopeGroups when community-groups feature flag is disabled', () => {
+      const svc = service();
+      fakeStore.getInScopeGroups = sinon.stub().returns(dummyGroups);
+      assert.deepEqual(svc.all(), dummyGroups);
+      assert.called(fakeStore.getInScopeGroups);
+    });
+
+    [[0, 1, 2, 3], [2, 0, 1, 3], [0, 3, 1, 2]].forEach(groupInputOrder => {
+      it('sorts the groups in the following order: scoped, public, private maintaining order within each category.', () => {
+        const groups = [
+          { id: 0, type: 'open' },
+          { id: 1, type: 'restricted' },
+          { id: '__world__', type: 'open' },
+          { id: 3, type: 'private' },
+        ];
+        const svc = service();
+        fakeStore.getInScopeGroups = sinon
+          .stub()
+          .returns(groupInputOrder.map(id => groups[id]));
+        assert.deepEqual(svc.all(), groups);
+      });
     });
   });
 
   describe('#load', function() {
-    it('combines groups from both endpoints if community-groups feature flag is set', function() {
+    it('combines groups from both endpoints', function() {
       const svc = service();
 
       const groups = [
@@ -169,7 +192,6 @@ describe('groups', function() {
 
       fakeApi.profile.groups.read.returns(Promise.resolve(groups));
       fakeApi.groups.list.returns(Promise.resolve([groups[0]]));
-      fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
 
       return svc.load().then(() => {
         assert.calledWith(fakeStore.loadGroups, groups);
@@ -184,21 +206,11 @@ describe('groups', function() {
       });
     });
 
-    it('always sends the `expand` parameter', function() {
-      const svc = service();
-      return svc.load().then(() => {
-        const call = fakeApi.groups.list.getCall(0);
-        assert.isObject(call.args[0]);
-        assert.equal(call.args[0].expand, 'organization');
-      });
-    });
-
-    it('sends `expand` parameter when community-groups feature flag is enabled', function() {
+    it('sends `expand` parameter', function() {
       const svc = service();
       fakeApi.groups.list.returns(
         Promise.resolve([{ id: 'groupa', name: 'GroupA' }])
       );
-      fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
 
       return svc.load().then(() => {
         assert.calledWith(
@@ -241,7 +253,7 @@ describe('groups', function() {
         return loaded.then(() => {
           assert.calledWith(fakeApi.groups.list, {
             document_uri: 'https://asite.com',
-            expand: 'organization',
+            expand: ['organization', 'scopes'],
           });
         });
       });
@@ -257,7 +269,7 @@ describe('groups', function() {
         const svc = service();
         return svc.load().then(() => {
           assert.calledWith(fakeApi.groups.list, {
-            expand: 'organization',
+            expand: ['organization', 'scopes'],
           });
         });
       });
@@ -277,12 +289,7 @@ describe('groups', function() {
     it('injects a defalt organization if group is missing an organization', function() {
       const svc = service();
       const groups = [{ id: '39r39f', name: 'Ding Dong!' }];
-      fakeApi.groups.list.returns(
-        Promise.resolve({
-          token: '1234',
-          data: groups,
-        })
-      );
+      fakeApi.groups.list.returns(Promise.resolve(groups));
       return svc.load().then(groups => {
         assert.isObject(groups[0].organization);
         assert.hasAllKeys(groups[0].organization, ['id', 'logo']);
@@ -317,12 +324,9 @@ describe('groups', function() {
             groups.push({ name: 'BioPub', id: 'biopub' });
           }
 
-          fakeApi.groups.list.returns(
-            Promise.resolve({
-              token: loggedIn ? '1234' : null,
-              data: groups,
-            })
-          );
+          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__');
@@ -375,12 +379,8 @@ describe('groups', function() {
           { name: 'DEF', id: 'def456', groupid: null },
         ];
 
-        fakeApi.groups.list.returns(
-          Promise.resolve({
-            token: '1234',
-            data: groups,
-          })
-        );
+        fakeApi.groups.list.returns(Promise.resolve(groups));
+        fakeApi.profile.groups.read.returns(Promise.resolve([]));
 
         return svc.load().then(groups => {
           let displayedGroups = groups.map(g => g.id);

src/sidebar/store/modules/groups.js

@@ -158,6 +158,17 @@ const getCurrentlyViewingGroups = memoize(state => {
   );
 });
 
+/**
+ * Return groups that are scoped to the uri. This is used to return the groups
+ * that show up in the old groups menu. This should be removed once the new groups
+ * menu is permanent.
+ *
+ * @return {Group[]}
+ */
+const getInScopeGroups = memoize(state => {
+  return state.groups.filter(g => g.isScopedToUri);
+});
+
 module.exports = {
   init,
   update,
@@ -171,6 +182,7 @@ module.exports = {
     getCurrentlyViewingGroups,
     getFeaturedGroups,
     getMyGroups,
+    getInScopeGroups,
     focusedGroup,
     focusedGroupId,
   },

src/sidebar/store/modules/test/groups-test.js

@@ -134,6 +134,13 @@ describe('sidebar.store.modules.groups', () => {
     });
   });
 
+  describe('getInScopeGroups', () => {
+    it('returns all groups that are in scope', () => {
+      store.loadGroups([publicGroup, privateGroup, restrictedOutOfScopeGroup]);
+      assert.deepEqual(store.getInScopeGroups(), [publicGroup, privateGroup]);
+    });
+  });
+
   describe('getGroup', () => {
     it('returns the group with the given ID', () => {
       store.loadGroups([publicGroup, privateGroup]);

src/sidebar/util/groups.js

@@ -35,10 +35,9 @@ function combineGroups(userGroups, featuredGroups, uri) {
 }
 
 function isScopedToUri(group, uri) {
-  // If the group has scope info, the scoping is enforced,
-  // and the uri patterns don't include this page's uri
-  // the group is not selectable, otherwise it is.
-  if (group.scopes && group.scopes.enforced) {
+  // Groups with no scopes or groups with no uri_patterns defined
+  // are considered in scope.
+  if (group.scopes && group.scopes.uri_patterns.length > 0) {
     return uriMatchesScopes(uri, group.scopes.uri_patterns);
   }
   return true;

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

@@ -102,14 +102,14 @@ describe('sidebar.util.groups', () => {
       {
         description: 'sets `isScopedToUri` to true if `scopes` is missing',
         scopes: undefined,
-        shouldBeSelectable: true,
+        isScopedToUri: true,
         uri: 'https://foo.com/bar',
       },
       {
         description:
-          'sets `isScopedToUri` to true if `scopes.enforced` is false',
-        scopes: { enforced: false },
-        shouldBeSelectable: true,
+          'sets `isScopedToUri` to true if `scopes.uri_patterns` is empty',
+        scopes: { uri_patterns: [] },
+        isScopedToUri: true,
         uri: 'https://foo.com/bar',
       },
       {
@@ -119,20 +119,20 @@ describe('sidebar.util.groups', () => {
           enforced: true,
           uri_patterns: ['http://foo.com*', 'https://foo.com*'],
         },
-        shouldBeSelectable: true,
+        isScopedToUri: true,
         uri: 'https://foo.com/bar',
       },
       {
         description:
           'sets `isScopedToUri` to false if `scopes.uri_patterns` do not match the uri',
         scopes: { enforced: true, uri_patterns: ['http://foo.com*'] },
-        shouldBeSelectable: false,
+        isScopedToUri: false,
         uri: 'https://foo.com/bar',
       },
       {
         description: 'it permits multiple *s in the scopes uri pattern',
         scopes: { enforced: true, uri_patterns: ['https://foo.com*bar*'] },
-        shouldBeSelectable: true,
+        isScopedToUri: true,
         uri: 'https://foo.com/boo/bar/baz',
       },
       {
@@ -141,17 +141,17 @@ describe('sidebar.util.groups', () => {
           enforced: true,
           uri_patterns: ['https://foo.com?bar=foo$[^]($){mu}+&boo=*'],
         },
-        shouldBeSelectable: true,
+        isScopedToUri: true,
         uri: 'https://foo.com?bar=foo$[^]($){mu}+&boo=foo',
       },
-    ].forEach(({ description, scopes, shouldBeSelectable, uri }) => {
+    ].forEach(({ description, scopes, isScopedToUri, uri }) => {
       it(description, () => {
         const userGroups = [{ id: 'groupa', name: 'GroupA', scopes: scopes }];
         const featuredGroups = [];
 
         const groups = combineGroups(userGroups, featuredGroups, uri);
 
-        groups.forEach(g => assert.equal(g.isScopedToUri, shouldBeSelectable));
+        groups.forEach(g => assert.equal(g.isScopedToUri, isScopedToUri));
       });
     });