Commit d14c255b authored by Robert Knight's avatar Robert Knight

Show "Leave group" option for open and restricted groups that user is a member of

Previously the "Leave group" option was only shown for private groups on the
basis that membership of open and restricted groups was managed by h admins.
However first-party Hypothesis users can now create open and restricted groups
themselves and members of these groups will see an option to leave the group on
activity pages. Align whether the client shows the "Leave group" option with h's
behavior.

During this change it was noticed that the documentation in the code and test
descriptions did not match the actual handling of `allowLeavingGroups`. The
comments said that services had to explicitly set `allowLeavingGroups` to
prevent users leaving. However the code would treat `allowLeavingGroups` as false
if a) service configuration was present and b) the value of `allowLeavingGroups`
was falsey (including undefined). Changing this behavior may cause issues for
existing users of third party authorities, so this commit updates the
documentation and tests to accurately describe the current behavior.

Fixes https://github.com/hypothesis/client/issues/6637
parent 8eea2280
...@@ -5,10 +5,10 @@ import type { SidebarSettings } from '../../types/config'; ...@@ -5,10 +5,10 @@ import type { SidebarSettings } from '../../types/config';
import { serviceConfig } from '../config/service-config'; import { serviceConfig } from '../config/service-config';
/** /**
* Should users be able to leave private groups of which they * Return true if users are allowed to leave groups.
* are a member? Users may leave private groups unless *
* explicitly disallowed in the service configuration of the * Third-party authorities currently have to opt-in to enabling this, since
* `settings` object. * users may not have a way to rejoin a group after leaving.
*/ */
function allowLeavingGroups(settings: SidebarSettings): boolean { function allowLeavingGroups(settings: SidebarSettings): boolean {
const config = serviceConfig(settings); const config = serviceConfig(settings);
...@@ -42,19 +42,16 @@ export function combineGroups( ...@@ -42,19 +42,16 @@ export function combineGroups(
const myGroupIds = userGroups.map(g => g.id); const myGroupIds = userGroups.map(g => g.id);
featuredGroups = featuredGroups.filter(g => !myGroupIds.includes(g.id)); featuredGroups = featuredGroups.filter(g => !myGroupIds.includes(g.id));
// Set the isMember property indicating user membership. // Set flag indicating whether user is a member of the group.
featuredGroups.forEach(group => (group.isMember = false)); featuredGroups.forEach(group => (group.isMember = false));
userGroups.forEach(group => (group.isMember = true)); userGroups.forEach(group => (group.isMember = true));
const groups = userGroups.concat(featuredGroups); const groups = userGroups.concat(featuredGroups);
// Set the `canLeave` property. Groups can be left if they are private unless // Set flag indicating whether user can leave group.
// the global `allowLeavingGroups` value is false in the config settings object. for (const group of groups) {
groups.forEach(group => { group.canLeave = allowLeavingGroups(settings) && group.isMember;
group.canLeave = !allowLeavingGroups(settings) }
? false
: group.type === 'private';
});
// Add isScopedToUri property indicating whether a group is within scope // Add isScopedToUri property indicating whether a group is within scope
// of the given uri. If the scope check cannot be performed, isScopedToUri // of the given uri. If the scope check cannot be performed, isScopedToUri
......
...@@ -2,6 +2,7 @@ import { combineGroups, normalizeGroupIds, $imports } from '../groups'; ...@@ -2,6 +2,7 @@ import { combineGroups, normalizeGroupIds, $imports } from '../groups';
describe('sidebar/helpers/groups', () => { describe('sidebar/helpers/groups', () => {
let fakeServiceConfig; let fakeServiceConfig;
describe('combineGroups', () => { describe('combineGroups', () => {
beforeEach(() => { beforeEach(() => {
fakeServiceConfig = sinon.stub().returns(null); fakeServiceConfig = sinon.stub().returns(null);
...@@ -22,44 +23,67 @@ describe('sidebar/helpers/groups', () => { ...@@ -22,44 +23,67 @@ describe('sidebar/helpers/groups', () => {
assert.equal(groupA.isMember, true); assert.equal(groupA.isMember, true);
}); });
it('sets `canLeave` to true if a group is private and `allowLeavingGroups` is null', () => { // `allowLeavingGroups` defaults to true for first party users and false
const userGroups = [{ id: 'groupa', name: 'GroupA', type: 'private' }]; // for third-party users.
const featuredGroups = [{ id: 'groupb', name: 'GroupB', type: 'open' }]; [null, { allowLeavingGroups: true }, { allowLeavingGroups: 1 }].forEach(
serviceConfig => {
it('sets `canLeave` to true if user is a member and leaving groups is enabled', () => {
fakeServiceConfig.returns(serviceConfig);
const userGroups = [
{ id: 'groupa', name: 'GroupA', type: 'private' },
{ id: 'groupc', name: 'GroupC', type: 'open' },
{ id: 'groupd', name: 'GroupD', type: 'restricted' },
];
const featuredGroups = [
{ id: 'groupb', name: 'GroupB', type: 'open' },
];
const groups = combineGroups( const groups = combineGroups(
userGroups, userGroups,
featuredGroups, featuredGroups,
'https://foo.com/bar', 'https://foo.com/bar',
); );
const groupA = groups.find(g => g.id === 'groupa');
const groupB = groups.find(g => g.id === 'groupb');
assert.equal(groupA.canLeave, true);
assert.equal(groupB.canLeave, false);
});
it('sets `canLeave` to true if a group is private and `allowLeavingGroups` is not a boolean', () => { const expected = [
fakeServiceConfig.returns({ {
allowLeavingGroups: () => {}, id: 'groupa',
}); canLeave: true,
const userGroups = [{ id: 'groupa', name: 'GroupA', type: 'private' }]; },
const featuredGroups = [{ id: 'groupb', name: 'GroupB', type: 'open' }]; {
const groups = combineGroups( id: 'groupb',
userGroups, canLeave: false,
featuredGroups, },
'https://foo.com/bar', {
); id: 'groupc',
const groupA = groups.find(g => g.id === 'groupa'); canLeave: true,
const groupB = groups.find(g => g.id === 'groupb'); },
{
id: 'groupd',
canLeave: true,
},
];
assert.equal(groupA.canLeave, true); for (const { id, canLeave } of expected) {
assert.equal(groupB.canLeave, false); const group = groups.find(g => g.id === id);
assert.strictEqual(
group.canLeave,
canLeave,
`incorrect canLeave for group ${id}`,
);
}
}); });
},
);
it('sets `canLeave` to false for all groups if `allowLeavingGroups` is false', () => { [{}, { allowLeavingGroups: false }, { allowLeavingGroups: null }].forEach(
fakeServiceConfig.returns({ serviceConfig => {
allowLeavingGroups: false, it('sets `canLeave` to false for all groups if leaving groups is disabled', () => {
}); fakeServiceConfig.returns(serviceConfig);
const userGroups = [{ id: 'groupa', name: 'GroupA', type: 'private' }]; const userGroups = [
const featuredGroups = [{ id: 'groupb', name: 'GroupB', type: 'open' }]; { id: 'groupa', name: 'GroupA', type: 'private' },
];
const featuredGroups = [
{ id: 'groupb', name: 'GroupB', type: 'open' },
];
const groups = combineGroups( const groups = combineGroups(
userGroups, userGroups,
featuredGroups, featuredGroups,
...@@ -70,6 +94,8 @@ describe('sidebar/helpers/groups', () => { ...@@ -70,6 +94,8 @@ describe('sidebar/helpers/groups', () => {
assert.equal(groupA.canLeave, false); assert.equal(groupA.canLeave, false);
assert.equal(groupB.canLeave, false); assert.equal(groupB.canLeave, false);
}); });
},
);
it('combines groups from both lists uniquely', () => { it('combines groups from both lists uniquely', () => {
const userGroups = [ const userGroups = [
......
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