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 #6637

src/sidebar/helpers/groups.ts

@@ -5,10 +5,10 @@ import type { SidebarSettings } from '../../types/config';
 import { serviceConfig } from '../config/service-config';
 
 /**
- * Should users be able to leave private groups of which they
- * are a member? Users may leave private groups unless
- * explicitly disallowed in the service configuration of the
- * `settings` object.
+ * Return true if users are allowed to leave groups.
+ *
+ * Third-party authorities currently have to opt-in to enabling this, since
+ * users may not have a way to rejoin a group after leaving.
  */
 function allowLeavingGroups(settings: SidebarSettings): boolean {
   const config = serviceConfig(settings);
@@ -42,19 +42,16 @@ export function combineGroups(
   const myGroupIds = userGroups.map(g => 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));
   userGroups.forEach(group => (group.isMember = true));
 
   const groups = userGroups.concat(featuredGroups);
 
-  // Set the `canLeave` property. Groups can be left if they are private unless
-  // the global `allowLeavingGroups` value is false in the config settings object.
-  groups.forEach(group => {
-    group.canLeave = !allowLeavingGroups(settings)
-      ? false
-      : group.type === 'private';
-  });
+  // Set flag indicating whether user can leave group.
+  for (const group of groups) {
+    group.canLeave = allowLeavingGroups(settings) && group.isMember;
+  }
 
   // Add isScopedToUri property indicating whether a group is within scope
   // of the given uri. If the scope check cannot be performed, isScopedToUri

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

@@ -2,6 +2,7 @@ import { combineGroups, normalizeGroupIds, $imports } from '../groups';
 
 describe('sidebar/helpers/groups', () => {
   let fakeServiceConfig;
+
   describe('combineGroups', () => {
     beforeEach(() => {
       fakeServiceConfig = sinon.stub().returns(null);
@@ -22,54 +23,79 @@ describe('sidebar/helpers/groups', () => {
       assert.equal(groupA.isMember, true);
     });
 
-    it('sets `canLeave` to true if a group is private and `allowLeavingGroups` is null', () => {
-      const userGroups = [{ id: 'groupa', name: 'GroupA', type: 'private' }];
-      const featuredGroups = [{ id: 'groupb', name: 'GroupB', type: 'open' }];
-      const groups = combineGroups(
-        userGroups,
-        featuredGroups,
-        '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);
-    });
+    // `allowLeavingGroups` defaults to true for first party users and false
+    // for third-party users.
+    [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(
+            userGroups,
+            featuredGroups,
+            'https://foo.com/bar',
+          );
 
-    it('sets `canLeave` to true if a group is private and `allowLeavingGroups` is not a boolean', () => {
-      fakeServiceConfig.returns({
-        allowLeavingGroups: () => {},
-      });
-      const userGroups = [{ id: 'groupa', name: 'GroupA', type: 'private' }];
-      const featuredGroups = [{ id: 'groupb', name: 'GroupB', type: 'open' }];
-      const groups = combineGroups(
-        userGroups,
-        featuredGroups,
-        'https://foo.com/bar',
-      );
-      const groupA = groups.find(g => g.id === 'groupa');
-      const groupB = groups.find(g => g.id === 'groupb');
+          const expected = [
+            {
+              id: 'groupa',
+              canLeave: true,
+            },
+            {
+              id: 'groupb',
+              canLeave: false,
+            },
+            {
+              id: 'groupc',
+              canLeave: true,
+            },
+            {
+              id: 'groupd',
+              canLeave: true,
+            },
+          ];
 
-      assert.equal(groupA.canLeave, true);
-      assert.equal(groupB.canLeave, false);
-    });
+          for (const { id, canLeave } of expected) {
+            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', () => {
-      fakeServiceConfig.returns({
-        allowLeavingGroups: false,
-      });
-      const userGroups = [{ id: 'groupa', name: 'GroupA', type: 'private' }];
-      const featuredGroups = [{ id: 'groupb', name: 'GroupB', type: 'open' }];
-      const groups = combineGroups(
-        userGroups,
-        featuredGroups,
-        'https://foo.com/bar',
-      );
-      const groupA = groups.find(g => g.id === 'groupa');
-      const groupB = groups.find(g => g.id === 'groupb');
-      assert.equal(groupA.canLeave, false);
-      assert.equal(groupB.canLeave, false);
-    });
+    [{}, { allowLeavingGroups: false }, { allowLeavingGroups: null }].forEach(
+      serviceConfig => {
+        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 featuredGroups = [
+            { id: 'groupb', name: 'GroupB', type: 'open' },
+          ];
+          const groups = combineGroups(
+            userGroups,
+            featuredGroups,
+            'https://foo.com/bar',
+          );
+          const groupA = groups.find(g => g.id === 'groupa');
+          const groupB = groups.find(g => g.id === 'groupb');
+          assert.equal(groupA.canLeave, false);
+          assert.equal(groupB.canLeave, false);
+        });
+      },
+    );
 
     it('combines groups from both lists uniquely', () => {
       const userGroups = [