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

Revert "Remove feature flag from group service"

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

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

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

@@ -31,49 +31,26 @@ 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,
-        scopesEnforced,
-        expectedIsSelectable,
-      }) => {
+      ({ description, fakeDisableOosGroupSelection, expectedIsSelectable }) => {
         it(description, () => {
           const fakeSectionGroups = [
-            {
-              isScopedToUri: true,
-              scopes: { enforced: scopesEnforced },
-              id: 0,
-            },
-            {
-              isScopedToUri: false,
-              scopes: { enforced: scopesEnforced },
-              id: 1,
-            },
+            { isScopedToUri: true, id: 0 },
+            { isScopedToUri: false, id: 1 },
           ];
 
           const element = createGroupListSection(

src/sidebar/services/groups.js

@@ -16,7 +16,6 @@ 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
@@ -78,9 +77,7 @@ 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__' && g.isScopedToUri
-    );
+    const pageHasAssociatedGroups = groups.some(g => g.id !== '__world__');
     if (!pageHasAssociatedGroups) {
       return Promise.resolve(groups);
     }
@@ -147,7 +144,7 @@ function groups(
     return uri
       .then(uri => {
         const params = {
-          expand: ['organization', 'scopes'],
+          expand: 'organization',
         };
         if (authority) {
           params.authority = authority;
@@ -157,11 +154,13 @@ function groups(
         }
         documentUri = uri;
 
-        const profileGroupsApi = api.profile.groups.read({
-          expand: params.expand,
-        });
+        if (features.flagEnabled('community_groups')) {
+          params.expand = ['organization', 'scopes'];
+          const profileParams = {
+            expand: ['organization', 'scopes'],
+          };
+          const profileGroupsApi = api.profile.groups.read(profileParams);
           const listGroupsApi = api.groups.list(params);
-
           return Promise.all([
             profileGroupsApi,
             listGroupsApi,
@@ -170,6 +169,12 @@ 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;
@@ -191,26 +196,9 @@ 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,9 +70,6 @@ describe('groups', function() {
         allGroups() {
           return this.getState().groups;
         },
-        getInScopeGroups() {
-          return this.getState().groups;
-        },
         focusedGroup() {
           return this.getState().focusedGroup;
         },
@@ -117,7 +114,12 @@ describe('groups', function() {
         },
       },
       groups: {
-        list: sandbox.stub().returns(dummyGroups),
+        list: sandbox.stub().returns(
+          Promise.resolve({
+            data: dummyGroups,
+            token: '1234',
+          })
+        ),
       },
       profile: {
         groups: {
@@ -149,40 +151,15 @@ describe('groups', function() {
   }
 
   describe('#all', function() {
-    it('returns all groups from store.allGroups when community-groups feature flag is enabled', () => {
+    it('returns all groups', function() {
       const svc = service();
-      fakeStore.allGroups = sinon.stub().returns(dummyGroups);
-      fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
+      fakeStore.setState({ groups: dummyGroups });
       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', function() {
+    it('combines groups from both endpoints if community-groups feature flag is set', function() {
       const svc = service();
 
       const groups = [
@@ -192,6 +169,7 @@ 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);
@@ -206,11 +184,21 @@ describe('groups', function() {
       });
     });
 
-    it('sends `expand` parameter', 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() {
       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(
@@ -253,7 +241,7 @@ describe('groups', function() {
         return loaded.then(() => {
           assert.calledWith(fakeApi.groups.list, {
             document_uri: 'https://asite.com',
-            expand: ['organization', 'scopes'],
+            expand: 'organization',
           });
         });
       });
@@ -269,7 +257,7 @@ describe('groups', function() {
         const svc = service();
         return svc.load().then(() => {
           assert.calledWith(fakeApi.groups.list, {
-            expand: ['organization', 'scopes'],
+            expand: 'organization',
           });
         });
       });
@@ -289,7 +277,12 @@ 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(groups));
+      fakeApi.groups.list.returns(
+        Promise.resolve({
+          token: '1234',
+          data: groups,
+        })
+      );
       return svc.load().then(groups => {
         assert.isObject(groups[0].organization);
         assert.hasAllKeys(groups[0].organization, ['id', 'logo']);
@@ -324,9 +317,12 @@ describe('groups', function() {
             groups.push({ name: 'BioPub', id: 'biopub' });
           }
 
-          fakeAuth.tokenGetter.returns(loggedIn ? '1234' : null);
-          fakeApi.groups.list.returns(Promise.resolve(groups));
-          fakeApi.profile.groups.read.returns(Promise.resolve([]));
+          fakeApi.groups.list.returns(
+            Promise.resolve({
+              token: loggedIn ? '1234' : null,
+              data: groups,
+            })
+          );
 
           return svc.load().then(groups => {
             const publicGroupShown = groups.some(g => g.id === '__world__');
@@ -379,8 +375,12 @@ describe('groups', function() {
           { name: 'DEF', id: 'def456', groupid: null },
         ];
 
-        fakeApi.groups.list.returns(Promise.resolve(groups));
-        fakeApi.profile.groups.read.returns(Promise.resolve([]));
+        fakeApi.groups.list.returns(
+          Promise.resolve({
+            token: '1234',
+            data: groups,
+          })
+        );
 
         return svc.load().then(groups => {
           let displayedGroups = groups.map(g => g.id);

src/sidebar/store/modules/groups.js

@@ -158,17 +158,6 @@ 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,
@@ -182,7 +171,6 @@ module.exports = {
     getCurrentlyViewingGroups,
     getFeaturedGroups,
     getMyGroups,
-    getInScopeGroups,
     focusedGroup,
     focusedGroupId,
   },

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

@@ -134,13 +134,6 @@ 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,9 +35,10 @@ function combineGroups(userGroups, featuredGroups, uri) {
 }
 
 function isScopedToUri(group, uri) {
-  // Groups with no scopes or groups with no uri_patterns defined
-  // are considered in scope.
-  if (group.scopes && group.scopes.uri_patterns.length > 0) {
+  // 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) {
     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,
-        isScopedToUri: true,
+        shouldBeSelectable: true,
         uri: 'https://foo.com/bar',
       },
       {
         description:
-          'sets `isScopedToUri` to true if `scopes.uri_patterns` is empty',
-        scopes: { uri_patterns: [] },
-        isScopedToUri: true,
+          'sets `isScopedToUri` to true if `scopes.enforced` is false',
+        scopes: { enforced: false },
+        shouldBeSelectable: true,
         uri: 'https://foo.com/bar',
       },
       {
@@ -119,20 +119,20 @@ describe('sidebar.util.groups', () => {
           enforced: true,
           uri_patterns: ['http://foo.com*', 'https://foo.com*'],
         },
-        isScopedToUri: true,
+        shouldBeSelectable: 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*'] },
-        isScopedToUri: false,
+        shouldBeSelectable: 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*'] },
-        isScopedToUri: true,
+        shouldBeSelectable: 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=*'],
         },
-        isScopedToUri: true,
+        shouldBeSelectable: true,
         uri: 'https://foo.com?bar=foo$[^]($){mu}+&boo=foo',
       },
-    ].forEach(({ description, scopes, isScopedToUri, uri }) => {
+    ].forEach(({ description, scopes, shouldBeSelectable, 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, isScopedToUri));
+        groups.forEach(g => assert.equal(g.isScopedToUri, shouldBeSelectable));
       });
     });