Children Members Followup (#8456)

* refact: remove children memberships when added to parent collective * refact: add includeInheritedMembers flag to members resolver * fix: consider inherited admins in isLastAdmin() * fix: reviewed suggestions Co-authored-by: François Hodierne <francois@hodierne.net> --------- Co-authored-by: François Hodierne <francois@hodierne.net>
opencollective · Jan 30, 2023 · a5fe646 · a5fe646
1 parent b8248de
commit a5fe646
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 4 deletions.
diff --git a/server/graphql/v2/interface/Account.js b/server/graphql/v2/interface/Account.js
@@ -217,6 +217,10 @@ const accountFieldsDefinition = () => ({
         type: new GraphQLList(AccountType),
         description: 'Type of accounts (BOT/COLLECTIVE/EVENT/ORGANIZATION/INDIVIDUAL)',
       },
+      includeInherited: {
+        type: GraphQLBoolean,
+        defaultValue: true,
+      },
     },
   },
   memberInvitations: {

diff --git a/server/graphql/v2/interface/HasMembers.js b/server/graphql/v2/interface/HasMembers.js
@@ -1,4 +1,4 @@
-import { GraphQLInt, GraphQLList, GraphQLNonNull } from 'graphql';
+import { GraphQLBoolean, GraphQLInt, GraphQLList, GraphQLNonNull } from 'graphql';
 import { intersection } from 'lodash';
 
 import { types as CollectiveTypes } from '../../../constants/collectives';
@@ -31,6 +31,10 @@ export const HasMembersFields = {
         defaultValue: { field: 'createdAt', direction: 'ASC' },
         description: 'Order of the results',
       },
+      includeInherited: {
+        type: GraphQLBoolean,
+        defaultValue: true,
+      },
     },
     async resolve(collective, args, req) {
       // TODO: isn't it a better practice to return null?
@@ -52,7 +56,7 @@ export const HasMembersFields = {
       }
 
       // Inherit Accountants and Admin from parent collective for Events and Projects
-      if ([CollectiveTypes.EVENT, CollectiveTypes.PROJECT].includes(collective.type)) {
+      if (args.includeInherited && [CollectiveTypes.EVENT, CollectiveTypes.PROJECT].includes(collective.type)) {
         const inheritedRoles = [MemberRoles.ACCOUNTANT, MemberRoles.ADMIN, MemberRoles.MEMBER];
         where = {
           [Op.or]: [

diff --git a/server/graphql/v2/mutation/MemberMutations.js b/server/graphql/v2/mutation/MemberMutations.js
@@ -17,9 +17,12 @@ import { AccountReferenceInput, fetchAccountWithReference } from '../input/Accou
 import { Member } from '../object/Member';
 
 const isLastAdmin = async (account, memberAccount) => {
+  // When checking if the member is the last admin for Minimum Amount of Admins policy,
+  // make sure we consider inherited admins, otherwise we won't be able to remove the event/project admin.
+  const CollectiveId = account.ParentCollectiveId ? [account.ParentCollectiveId, account.id] : account.id;
   const admins = await models.Member.findAll({
     where: {
-      CollectiveId: account.id,
+      CollectiveId,
       role: MemberRoles.ADMIN,
     },
   });

diff --git a/server/models/Collective.js b/server/models/Collective.js
@@ -1963,11 +1963,25 @@ Collective.prototype.addUserWithRole = async function (user, role, defaultAttrib
 
     case roles.MEMBER:
     case roles.ACCOUNTANT:
-    case roles.ADMIN:
+    case roles.ADMIN: {
       if (![types.FUND, types.PROJECT, types.EVENT].includes(this.type)) {
         await this.sendNewMemberEmail(user, role, member, sequelizeParams);
       }
+
+      // Sanitization: Clean memberships of children collectives
+      const children = await this.getChildren();
+      if (children.length > 0) {
+        await models.Member.destroy({
+          where: {
+            MemberCollectiveId: user.CollectiveId,
+            CollectiveId: children.map(c => c.id),
+            role,
+          },
+        });
+      }
+
       break;
+    }
   }
 
   return member;