Accomodate membership id (#6221)

* fix: accomodate membership id

* fix: remove only

* fix: event handling

* fix: tests

* fix: use transaction

* Remove useless test

---------

Co-authored-by: Tom Moor <tom.moor@gmail.com>

app/components/WebsocketProvider.tsx

@@ -400,14 +400,20 @@ class WebsocketProvider extends React.Component<Props> {
               err instanceof NotFoundError
             ) {
               collections.remove(event.collectionId);
-              memberships.remove(`${event.userId}-${event.collectionId}`);
+              memberships.revoke({
+                userId: event.userId,
+                collectionId: event.collectionId,
+              });
               return;
             }
           }
 
           documents.removeCollectionDocuments(event.collectionId);
         } else {
-          memberships.remove(`${event.userId}-${event.collectionId}`);
+          memberships.revoke({
+            userId: event.userId,
+            collectionId: event.collectionId,
+          });
         }
       }
     );

app/stores/CollectionGroupMembershipsStore.ts

@@ -71,13 +71,18 @@ export default class CollectionGroupMembershipsStore extends Store<CollectionGro
       id: collectionId,
       groupId,
     });
-    this.remove(`${groupId}-${collectionId}`);
+    const membership = Array.from(this.data.values()).find(
+      (m) => m.groupId === groupId && m.collectionId === collectionId
+    );
+    if (membership) {
+      this.remove(membership.id);
+    }
   }
 
   @action
   removeCollectionMemberships = (collectionId: string) => {
     this.data.forEach((membership, key) => {
-      if (key.includes(collectionId)) {
+      if (membership.collectionId === collectionId) {
         this.remove(key);
       }
     });

app/stores/MembershipsStore.ts

@@ -71,16 +71,31 @@ export default class MembershipsStore extends Store<Membership> {
       id: collectionId,
       userId,
     });
-    this.remove(`${userId}-${collectionId}`);
-    this.rootStore.users.remove(userId);
+    this.revoke({ userId, collectionId });
   }
 
   @action
   removeCollectionMemberships = (collectionId: string) => {
     this.data.forEach((membership, key) => {
-      if (key.includes(collectionId)) {
+      if (membership.collectionId === collectionId) {
         this.remove(key);
       }
     });
   };
+
+  @action
+  revoke = ({
+    userId,
+    collectionId,
+  }: {
+    collectionId: string;
+    userId: string;
+  }) => {
+    const membership = Array.from(this.data.values()).find(
+      (m) => m.userId === userId && m.collectionId === collectionId
+    );
+    if (membership) {
+      this.remove(membership.id);
+    }
+  };
 }

plugins/webhooks/server/tasks/DeliverWebhookTask.ts

@@ -441,7 +441,7 @@ export default class DeliverWebhookTask extends BaseTask<Props> {
       event,
       subscription,
       payload: {
-        id: `${event.userId}-${event.collectionId}`,
+        id: event.data.membershipId,
         model: model && presentMembership(model),
         collection: model && presentCollection(model.collection!),
         user: model && presentUser(model.user),
@@ -468,7 +468,7 @@ export default class DeliverWebhookTask extends BaseTask<Props> {
       event,
       subscription,
       payload: {
-        id: `${event.modelId}-${event.collectionId}`,
+        id: event.data.membershipId,
         model: model && presentCollectionGroupMembership(model),
         collection: model && presentCollection(model.collection!),
         group: model && presentGroup(model.group),

server/presenters/collectionGroupMembership.ts

@@ -12,7 +12,7 @@ export default function presentCollectionGroupMembership(
   membership: GroupPermission
 ): Membership {
   return {
-    id: `${membership.groupId}-${membership.collectionId}`,
+    id: membership.id,
     groupId: membership.groupId,
     collectionId: membership.collectionId,
     permission: membership.permission,

server/presenters/membership.ts

@@ -12,7 +12,7 @@ export default function presentMembership(
   membership: UserPermission
 ): Membership {
   return {
-    id: `${membership.userId}-${membership.collectionId}`,
+    id: membership.id,
     userId: membership.userId,
     collectionId: membership.collectionId,
     permission: membership.permission,

server/routes/api/collections/collections.test.ts

@@ -699,6 +699,28 @@ describe("#collections.remove_user", () => {
     expect(users.length).toEqual(1);
   });
 
+  it("should fail with status 400 bad request if user is not a member", async () => {
+    const admin = await buildAdmin();
+    const collection = await buildCollection({
+      teamId: admin.teamId,
+      userId: admin.id,
+      permission: null,
+    });
+    const nonMember = await buildUser({
+      teamId: admin.teamId,
+    });
+    const res = await server.post("/api/collections.remove_user", {
+      body: {
+        token: admin.getJwtToken(),
+        id: collection.id,
+        userId: nonMember.id,
+      },
+    });
+    const body = await res.json();
+    expect(res.status).toEqual(400);
+    expect(body.message).toEqual("User is not a collection member");
+  });
+
   it("should require user in team", async () => {
     const user = await buildUser();
     const collection = await buildCollection({
@@ -768,11 +790,13 @@ describe("#collections.group_memberships", () => {
         id: collection.id,
       },
     });
+    const [membership] = await collection.$get("collectionGroupMemberships");
     const body = await res.json();
     expect(res.status).toEqual(200);
     expect(body.data.groups.length).toEqual(1);
     expect(body.data.groups[0].id).toEqual(group.id);
     expect(body.data.collectionGroupMemberships.length).toEqual(1);
+    expect(body.data.collectionGroupMemberships[0].id).toEqual(membership.id);
     expect(body.data.collectionGroupMemberships[0].permission).toEqual(
       CollectionPermission.ReadWrite
     );
@@ -906,11 +930,13 @@ describe("#collections.memberships", () => {
         id: collection.id,
       },
     });
+    const [membership] = await collection.$get("memberships");
     const body = await res.json();
     expect(res.status).toEqual(200);
     expect(body.data.users.length).toEqual(1);
     expect(body.data.users[0].id).toEqual(user.id);
     expect(body.data.memberships.length).toEqual(1);
+    expect(body.data.memberships[0].id).toEqual(membership.id);
     expect(body.data.memberships[0].permission).toEqual(
       CollectionPermission.Admin
     );

server/routes/api/collections/collections.ts

@@ -249,6 +249,7 @@ router.post(
       modelId: groupId,
       data: {
         name: group.name,
+        membershipId: membership.id,
       },
       ip: ctx.request.ip,
     });
@@ -267,31 +268,46 @@ router.post(
   "collections.remove_group",
   auth(),
   validate(T.CollectionsRemoveGroupSchema),
+  transaction(),
   async (ctx: APIContext<T.CollectionsRemoveGroupReq>) => {
     const { id, groupId } = ctx.input.body;
     const { user } = ctx.state.auth;
+    const { transaction } = ctx.state;
 
     const collection = await Collection.scope({
       method: ["withMembership", user.id],
-    }).findByPk(id);
+    }).findByPk(id, { transaction });
     authorize(user, "update", collection);
 
-    const group = await Group.findByPk(groupId);
+    const group = await Group.findByPk(groupId, { transaction });
     authorize(user, "read", group);
 
-    await collection.$remove("group", group);
-    await Event.create({
-      name: "collections.remove_group",
-      collectionId: collection.id,
-      teamId: collection.teamId,
-      actorId: user.id,
-      modelId: groupId,
-      data: {
-        name: group.name,
-      },
-      ip: ctx.request.ip,
+    const [membership] = await collection.$get("collectionGroupMemberships", {
+      where: { groupId },
+      transaction,
     });
 
+    if (!membership) {
+      ctx.throw(400, "This Group is not a part of the collection");
+    }
+
+    await collection.$remove("group", group);
+    await Event.create(
+      {
+        name: "collections.remove_group",
+        collectionId: collection.id,
+        teamId: collection.teamId,
+        actorId: user.id,
+        modelId: groupId,
+        data: {
+          name: group.name,
+          membershipId: membership.id,
+        },
+        ip: ctx.request.ip,
+      },
+      { transaction }
+    );
+
     ctx.body = {
       success: true,
     };
@@ -416,6 +432,7 @@ router.post(
         actorId: actor.id,
         data: {
           name: user.name,
+          membershipId: membership.id,
         },
         ip: ctx.request.ip,
       },
@@ -436,8 +453,8 @@ router.post(
 router.post(
   "collections.remove_user",
   auth(),
-  transaction(),
   validate(T.CollectionsRemoveUserSchema),
+  transaction(),
   async (ctx: APIContext<T.CollectionsRemoveUserReq>) => {
     const { auth, transaction } = ctx.state;
     const actor = auth.user;
@@ -451,7 +468,16 @@ router.post(
     const user = await User.findByPk(userId, { transaction });
     authorize(actor, "read", user);
 
+    const [membership] = await collection.$get("memberships", {
+      where: { userId },
+      transaction,
+    });
+    if (!membership) {
+      ctx.throw(400, "User is not a collection member");
+    }
+
     await collection.$remove("user", user, { transaction });
+
     await Event.create(
       {
         name: "collections.remove_user",
@@ -461,6 +487,7 @@ router.post(
         actorId: actor.id,
         data: {
           name: user.name,
+          membershipId: membership.id,
         },
         ip: ctx.request.ip,
       },

server/types.ts

@@ -199,15 +199,14 @@ export type CollectionUserEvent = BaseEvent & {
   name: "collections.add_user" | "collections.remove_user";
   userId: string;
   collectionId: string;
+  data: { name: string; membershipId: string };
 };
 
 export type CollectionGroupEvent = BaseEvent & {
   name: "collections.add_group" | "collections.remove_group";
   collectionId: string;
   modelId: string;
-  data: {
-    name: string;
-  };
+  data: { name: string; membershipId: string };
 };
 
 export type CollectionEvent = BaseEvent &