fix: Group membership count off after suspending users in groups (#1670)

* fix: Clear group memberships when suspending a user
Refactor to command

* test

* test

server/api/users.js

@@ -1,6 +1,7 @@
 // @flow
 import Router from "koa-router";
 import userInviter from "../commands/userInviter";
+import userSuspender from "../commands/userSuspender";
 import auth from "../middlewares/authentication";
 import { Event, User, Team } from "../models";
 import policy from "../policies";
@@ -135,23 +136,15 @@ router.post("users.demote", auth(), async (ctx) => {
 });
 
 router.post("users.suspend", auth(), async (ctx) => {
-  const admin = ctx.state.user;
   const userId = ctx.body.id;
-  const teamId = ctx.state.user.teamId;
   ctx.assertPresent(userId, "id is required");
 
   const user = await User.findByPk(userId);
   authorize(ctx.state.user, "suspend", user);
 
-  const team = await Team.findByPk(teamId);
-  await team.suspendUser(user, admin);
-
-  await Event.create({
-    name: "users.suspend",
+  await userSuspender({
+    user,
     actorId: ctx.state.user.id,
-    userId,
-    teamId,
-    data: { name: user.name },
     ip: ctx.request.ip,
   });
 

server/commands/userSuspender.js

@@ -0,0 +1,44 @@
+// @flow
+import { ValidationError } from "../errors";
+import { User, Event, GroupUser } from "../models";
+import { sequelize } from "../sequelize";
+
+export default async function userSuspender({
+  user,
+  actorId,
+  ip,
+}: {
+  user: User,
+  actorId: string,
+  ip: string,
+}): Promise<void> {
+  if (user.id === actorId) {
+    throw new ValidationError("Unable to suspend the current user");
+  }
+
+  let transaction;
+  try {
+    transaction = await sequelize.transaction();
+
+    await user.update({
+      suspendedById: actorId,
+      suspendedAt: new Date(),
+    });
+
+    await GroupUser.destroy({ where: { userId: user.id } });
+  } catch (err) {
+    if (transaction) {
+      await transaction.rollback();
+    }
+    throw err;
+  }
+
+  await Event.create({
+    name: "users.suspend",
+    actorId,
+    userId: user.id,
+    teamId: user.teamId,
+    data: { name: user.name },
+    ip,
+  });
+}

server/commands/userSuspender.test.js

@@ -0,0 +1,57 @@
+/* eslint-disable flowtype/require-valid-file-annotation */
+import { GroupUser } from "../models";
+import { buildGroup, buildUser } from "../test/factories";
+import { flushdb } from "../test/support";
+import userSuspender from "./userSuspender";
+
+beforeEach(() => flushdb());
+
+describe("userSuspender", () => {
+  const ip = "127.0.0.1";
+
+  it("should not suspend self", async () => {
+    const user = await buildUser();
+    let error;
+
+    try {
+      await userSuspender({
+        actorId: user.id,
+        user,
+        ip,
+      });
+    } catch (err) {
+      error = err;
+    }
+
+    expect(error.message).toEqual("Unable to suspend the current user");
+  });
+
+  it("should suspend the user", async () => {
+    const admin = await buildUser({ isAdmin: true });
+    const user = await buildUser({ teamId: admin.teamId });
+    await userSuspender({
+      actorId: admin.id,
+      user,
+      ip,
+    });
+    expect(user.suspendedAt).toBeTruthy();
+    expect(user.suspendedById).toEqual(admin.id);
+  });
+
+  it("should remove group memberships", async () => {
+    const admin = await buildUser({ isAdmin: true });
+    const user = await buildUser({ teamId: admin.teamId });
+    const group = await buildGroup({ teamId: user.teamId });
+
+    await group.addUser(user, { through: { createdById: user.id } });
+
+    await userSuspender({
+      actorId: admin.id,
+      user,
+      ip,
+    });
+    expect(user.suspendedAt).toBeTruthy();
+    expect(user.suspendedById).toEqual(admin.id);
+    expect(await GroupUser.count()).toEqual(0);
+  });
+});

server/models/Team.js

@@ -205,15 +205,6 @@ Team.prototype.removeAdmin = async function (user: User) {
   }
 };
 
-Team.prototype.suspendUser = async function (user: User, admin: User) {
-  if (user.id === admin.id)
-    throw new ValidationError("Unable to suspend the current user");
-  return user.update({
-    suspendedById: admin.id,
-    suspendedAt: new Date(),
-  });
-};
-
 Team.prototype.activateUser = async function (user: User, admin: User) {
   return user.update({
     suspendedById: null,