Unverified Commit d705dcf1 authored by Hannah Stepanek's avatar Hannah Stepanek Committed by GitHub

Revert "Remove feature flag from group service"

parent 6e6f3b89
...@@ -4,11 +4,7 @@ ...@@ -4,11 +4,7 @@
function GroupListSectionController() { function GroupListSectionController() {
this.isSelectable = function(groupId) { this.isSelectable = function(groupId) {
const group = this.sectionGroups.find(g => g.id === groupId); const group = this.sectionGroups.find(g => g.id === groupId);
return ( return !this.disableOosGroupSelection || group.isScopedToUri;
!this.disableOosGroupSelection ||
!group.scopes.enforced ||
group.isScopedToUri
);
}; };
} }
......
...@@ -31,49 +31,26 @@ describe('groupListSection', () => { ...@@ -31,49 +31,26 @@ describe('groupListSection', () => {
{ {
description: 'always returns true if disableOosGroupSelection is false', description: 'always returns true if disableOosGroupSelection is false',
fakeDisableOosGroupSelection: false, fakeDisableOosGroupSelection: false,
scopesEnforced: true,
expectedIsSelectable: [true, true], expectedIsSelectable: [true, true],
}, },
{ {
description: description:
'always returns true if disableOosGroupSelection is undefined', 'always returns true if disableOosGroupSelection is undefined',
fakeDisableOosGroupSelection: undefined, fakeDisableOosGroupSelection: undefined,
scopesEnforced: true,
expectedIsSelectable: [true, true], expectedIsSelectable: [true, true],
}, },
{ {
description: description:
'returns false if disableOosGroupSelection is true and group is out of scope', 'returns false if disableOosGroupSelection is true and group is out of scope',
fakeDisableOosGroupSelection: true, fakeDisableOosGroupSelection: true,
scopesEnforced: true,
expectedIsSelectable: [true, false], 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( ].forEach(
({ ({ description, fakeDisableOosGroupSelection, expectedIsSelectable }) => {
description,
fakeDisableOosGroupSelection,
scopesEnforced,
expectedIsSelectable,
}) => {
it(description, () => { it(description, () => {
const fakeSectionGroups = [ const fakeSectionGroups = [
{ { isScopedToUri: true, id: 0 },
isScopedToUri: true, { isScopedToUri: false, id: 1 },
scopes: { enforced: scopesEnforced },
id: 0,
},
{
isScopedToUri: false,
scopes: { enforced: scopesEnforced },
id: 1,
},
]; ];
const element = createGroupListSection( const element = createGroupListSection(
......
...@@ -16,7 +16,6 @@ const DEFAULT_ORGANIZATION = { ...@@ -16,7 +16,6 @@ const DEFAULT_ORGANIZATION = {
const events = require('../events'); const events = require('../events');
const { awaitStateChange } = require('../util/state-util'); const { awaitStateChange } = require('../util/state-util');
const { combineGroups } = require('../util/groups'); const { combineGroups } = require('../util/groups');
const memoize = require('../util/memoize');
const serviceConfig = require('../service-config'); const serviceConfig = require('../service-config');
// @ngInject // @ngInject
...@@ -78,9 +77,7 @@ function groups( ...@@ -78,9 +77,7 @@ function groups(
// If the main document URL has no groups associated with it, always show // If the main document URL has no groups associated with it, always show
// the "Public" group. // the "Public" group.
const pageHasAssociatedGroups = groups.some( const pageHasAssociatedGroups = groups.some(g => g.id !== '__world__');
g => g.id !== '__world__' && g.isScopedToUri
);
if (!pageHasAssociatedGroups) { if (!pageHasAssociatedGroups) {
return Promise.resolve(groups); return Promise.resolve(groups);
} }
...@@ -147,7 +144,7 @@ function groups( ...@@ -147,7 +144,7 @@ function groups(
return uri return uri
.then(uri => { .then(uri => {
const params = { const params = {
expand: ['organization', 'scopes'], expand: 'organization',
}; };
if (authority) { if (authority) {
params.authority = authority; params.authority = authority;
...@@ -157,19 +154,27 @@ function groups( ...@@ -157,19 +154,27 @@ function groups(
} }
documentUri = uri; documentUri = uri;
const profileGroupsApi = api.profile.groups.read({ if (features.flagEnabled('community_groups')) {
expand: params.expand, params.expand = ['organization', 'scopes'];
}); const profileParams = {
const listGroupsApi = api.groups.list(params); expand: ['organization', 'scopes'],
};
return Promise.all([ const profileGroupsApi = api.profile.groups.read(profileParams);
profileGroupsApi, const listGroupsApi = api.groups.list(params);
listGroupsApi, return Promise.all([
auth.tokenGetter(), profileGroupsApi,
]).then(([myGroups, featuredGroups, token]) => [ listGroupsApi,
combineGroups(myGroups, featuredGroups, documentUri), auth.tokenGetter(),
token, ]).then(([myGroups, featuredGroups, token]) => [
]); 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]) => { .then(([groups, token]) => {
const isLoggedIn = token !== null; const isLoggedIn = token !== null;
...@@ -191,25 +196,8 @@ function groups( ...@@ -191,25 +196,8 @@ 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() { function all() {
if (features.flagEnabled('community_groups')) { return store.allGroups();
return store.allGroups();
}
return sortGroups(store.getInScopeGroups());
} }
// Return the full object for the group with the given id. // Return the full object for the group with the given id.
......
...@@ -70,9 +70,6 @@ describe('groups', function() { ...@@ -70,9 +70,6 @@ describe('groups', function() {
allGroups() { allGroups() {
return this.getState().groups; return this.getState().groups;
}, },
getInScopeGroups() {
return this.getState().groups;
},
focusedGroup() { focusedGroup() {
return this.getState().focusedGroup; return this.getState().focusedGroup;
}, },
...@@ -117,7 +114,12 @@ describe('groups', function() { ...@@ -117,7 +114,12 @@ describe('groups', function() {
}, },
}, },
groups: { groups: {
list: sandbox.stub().returns(dummyGroups), list: sandbox.stub().returns(
Promise.resolve({
data: dummyGroups,
token: '1234',
})
),
}, },
profile: { profile: {
groups: { groups: {
...@@ -149,40 +151,15 @@ describe('groups', function() { ...@@ -149,40 +151,15 @@ describe('groups', function() {
} }
describe('#all', 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(); const svc = service();
fakeStore.allGroups = sinon.stub().returns(dummyGroups); fakeStore.setState({ groups: dummyGroups });
fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
assert.deepEqual(svc.all(), 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() { 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 svc = service();
const groups = [ const groups = [
...@@ -192,6 +169,7 @@ describe('groups', function() { ...@@ -192,6 +169,7 @@ describe('groups', function() {
fakeApi.profile.groups.read.returns(Promise.resolve(groups)); fakeApi.profile.groups.read.returns(Promise.resolve(groups));
fakeApi.groups.list.returns(Promise.resolve([groups[0]])); fakeApi.groups.list.returns(Promise.resolve([groups[0]]));
fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
return svc.load().then(() => { return svc.load().then(() => {
assert.calledWith(fakeStore.loadGroups, groups); assert.calledWith(fakeStore.loadGroups, groups);
...@@ -206,11 +184,21 @@ describe('groups', function() { ...@@ -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(); const svc = service();
fakeApi.groups.list.returns( fakeApi.groups.list.returns(
Promise.resolve([{ id: 'groupa', name: 'GroupA' }]) Promise.resolve([{ id: 'groupa', name: 'GroupA' }])
); );
fakeFeatures.flagEnabled.withArgs('community_groups').returns(true);
return svc.load().then(() => { return svc.load().then(() => {
assert.calledWith( assert.calledWith(
...@@ -253,7 +241,7 @@ describe('groups', function() { ...@@ -253,7 +241,7 @@ describe('groups', function() {
return loaded.then(() => { return loaded.then(() => {
assert.calledWith(fakeApi.groups.list, { assert.calledWith(fakeApi.groups.list, {
document_uri: 'https://asite.com', document_uri: 'https://asite.com',
expand: ['organization', 'scopes'], expand: 'organization',
}); });
}); });
}); });
...@@ -269,7 +257,7 @@ describe('groups', function() { ...@@ -269,7 +257,7 @@ describe('groups', function() {
const svc = service(); const svc = service();
return svc.load().then(() => { return svc.load().then(() => {
assert.calledWith(fakeApi.groups.list, { assert.calledWith(fakeApi.groups.list, {
expand: ['organization', 'scopes'], expand: 'organization',
}); });
}); });
}); });
...@@ -289,7 +277,12 @@ describe('groups', function() { ...@@ -289,7 +277,12 @@ describe('groups', function() {
it('injects a defalt organization if group is missing an organization', function() { it('injects a defalt organization if group is missing an organization', function() {
const svc = service(); const svc = service();
const groups = [{ id: '39r39f', name: 'Ding Dong!' }]; 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 => { return svc.load().then(groups => {
assert.isObject(groups[0].organization); assert.isObject(groups[0].organization);
assert.hasAllKeys(groups[0].organization, ['id', 'logo']); assert.hasAllKeys(groups[0].organization, ['id', 'logo']);
...@@ -324,9 +317,12 @@ describe('groups', function() { ...@@ -324,9 +317,12 @@ describe('groups', function() {
groups.push({ name: 'BioPub', id: 'biopub' }); groups.push({ name: 'BioPub', id: 'biopub' });
} }
fakeAuth.tokenGetter.returns(loggedIn ? '1234' : null); fakeApi.groups.list.returns(
fakeApi.groups.list.returns(Promise.resolve(groups)); Promise.resolve({
fakeApi.profile.groups.read.returns(Promise.resolve([])); token: loggedIn ? '1234' : null,
data: groups,
})
);
return svc.load().then(groups => { return svc.load().then(groups => {
const publicGroupShown = groups.some(g => g.id === '__world__'); const publicGroupShown = groups.some(g => g.id === '__world__');
...@@ -379,8 +375,12 @@ describe('groups', function() { ...@@ -379,8 +375,12 @@ describe('groups', function() {
{ name: 'DEF', id: 'def456', groupid: null }, { name: 'DEF', id: 'def456', groupid: null },
]; ];
fakeApi.groups.list.returns(Promise.resolve(groups)); fakeApi.groups.list.returns(
fakeApi.profile.groups.read.returns(Promise.resolve([])); Promise.resolve({
token: '1234',
data: groups,
})
);
return svc.load().then(groups => { return svc.load().then(groups => {
let displayedGroups = groups.map(g => g.id); let displayedGroups = groups.map(g => g.id);
......
...@@ -158,17 +158,6 @@ const getCurrentlyViewingGroups = memoize(state => { ...@@ -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 = { module.exports = {
init, init,
update, update,
...@@ -182,7 +171,6 @@ module.exports = { ...@@ -182,7 +171,6 @@ module.exports = {
getCurrentlyViewingGroups, getCurrentlyViewingGroups,
getFeaturedGroups, getFeaturedGroups,
getMyGroups, getMyGroups,
getInScopeGroups,
focusedGroup, focusedGroup,
focusedGroupId, focusedGroupId,
}, },
......
...@@ -134,13 +134,6 @@ describe('sidebar.store.modules.groups', () => { ...@@ -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', () => { describe('getGroup', () => {
it('returns the group with the given ID', () => { it('returns the group with the given ID', () => {
store.loadGroups([publicGroup, privateGroup]); store.loadGroups([publicGroup, privateGroup]);
......
...@@ -35,9 +35,10 @@ function combineGroups(userGroups, featuredGroups, uri) { ...@@ -35,9 +35,10 @@ function combineGroups(userGroups, featuredGroups, uri) {
} }
function isScopedToUri(group, uri) { function isScopedToUri(group, uri) {
// Groups with no scopes or groups with no uri_patterns defined // If the group has scope info, the scoping is enforced,
// are considered in scope. // and the uri patterns don't include this page's uri
if (group.scopes && group.scopes.uri_patterns.length > 0) { // the group is not selectable, otherwise it is.
if (group.scopes && group.scopes.enforced) {
return uriMatchesScopes(uri, group.scopes.uri_patterns); return uriMatchesScopes(uri, group.scopes.uri_patterns);
} }
return true; return true;
......
...@@ -102,14 +102,14 @@ describe('sidebar.util.groups', () => { ...@@ -102,14 +102,14 @@ describe('sidebar.util.groups', () => {
{ {
description: 'sets `isScopedToUri` to true if `scopes` is missing', description: 'sets `isScopedToUri` to true if `scopes` is missing',
scopes: undefined, scopes: undefined,
isScopedToUri: true, shouldBeSelectable: true,
uri: 'https://foo.com/bar', uri: 'https://foo.com/bar',
}, },
{ {
description: description:
'sets `isScopedToUri` to true if `scopes.uri_patterns` is empty', 'sets `isScopedToUri` to true if `scopes.enforced` is false',
scopes: { uri_patterns: [] }, scopes: { enforced: false },
isScopedToUri: true, shouldBeSelectable: true,
uri: 'https://foo.com/bar', uri: 'https://foo.com/bar',
}, },
{ {
...@@ -119,20 +119,20 @@ describe('sidebar.util.groups', () => { ...@@ -119,20 +119,20 @@ describe('sidebar.util.groups', () => {
enforced: true, enforced: true,
uri_patterns: ['http://foo.com*', 'https://foo.com*'], uri_patterns: ['http://foo.com*', 'https://foo.com*'],
}, },
isScopedToUri: true, shouldBeSelectable: true,
uri: 'https://foo.com/bar', uri: 'https://foo.com/bar',
}, },
{ {
description: description:
'sets `isScopedToUri` to false if `scopes.uri_patterns` do not match the uri', 'sets `isScopedToUri` to false if `scopes.uri_patterns` do not match the uri',
scopes: { enforced: true, uri_patterns: ['http://foo.com*'] }, scopes: { enforced: true, uri_patterns: ['http://foo.com*'] },
isScopedToUri: false, shouldBeSelectable: false,
uri: 'https://foo.com/bar', uri: 'https://foo.com/bar',
}, },
{ {
description: 'it permits multiple *s in the scopes uri pattern', description: 'it permits multiple *s in the scopes uri pattern',
scopes: { enforced: true, uri_patterns: ['https://foo.com*bar*'] }, scopes: { enforced: true, uri_patterns: ['https://foo.com*bar*'] },
isScopedToUri: true, shouldBeSelectable: true,
uri: 'https://foo.com/boo/bar/baz', uri: 'https://foo.com/boo/bar/baz',
}, },
{ {
...@@ -141,17 +141,17 @@ describe('sidebar.util.groups', () => { ...@@ -141,17 +141,17 @@ describe('sidebar.util.groups', () => {
enforced: true, enforced: true,
uri_patterns: ['https://foo.com?bar=foo$[^]($){mu}+&boo=*'], uri_patterns: ['https://foo.com?bar=foo$[^]($){mu}+&boo=*'],
}, },
isScopedToUri: true, shouldBeSelectable: true,
uri: 'https://foo.com?bar=foo$[^]($){mu}+&boo=foo', uri: 'https://foo.com?bar=foo$[^]($){mu}+&boo=foo',
}, },
].forEach(({ description, scopes, isScopedToUri, uri }) => { ].forEach(({ description, scopes, shouldBeSelectable, uri }) => {
it(description, () => { it(description, () => {
const userGroups = [{ id: 'groupa', name: 'GroupA', scopes: scopes }]; const userGroups = [{ id: 'groupa', name: 'GroupA', scopes: scopes }];
const featuredGroups = []; const featuredGroups = [];
const groups = combineGroups(userGroups, featuredGroups, uri); const groups = combineGroups(userGroups, featuredGroups, uri);
groups.forEach(g => assert.equal(g.isScopedToUri, isScopedToUri)); groups.forEach(g => assert.equal(g.isScopedToUri, shouldBeSelectable));
}); });
}); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment