WIP: Fix member count inaccurate in Guilds and Parties

test/api/v3/integration/groups/POST-groups_id_removeMember.test.js

@@ -1,10 +1,13 @@
+import mongoose from 'mongoose';
 import {
  generateUser,
  createAndPopulateGroup,
  translate as t,
  sleep,
 } from '../../../../helpers/api-integration/v3';
 import * as email from '../../../../../website/server/libs/email';
+import { schema as groupSchema } from '../../../../../website/server/models/group';
+import { schema as userSchema } from '../../../../../website/server/models/user/index';
 
 describe('POST /groups/:groupId/removeMember/:memberId', () => {
  let leader;
@@ -88,6 +91,58 @@ describe('POST /groups/:groupId/removeMember/:memberId', () => {
  await expect(leader.get(`/groups/${guild._id}`)).to.eventually.have.property('memberCount', oldMemberCount - 1);
  });
 
+ it('updates memberCount when removal is successful', async () => {
+ const oldMemberCount = guild.memberCount;
+ await leader.post(`/groups/${guild._id}/removeMember/${member._id}`);
+
+ const updatedGuild = await leader.get(`/groups/${guild._id}`);
+ expect(updatedGuild.memberCount).to.equal(oldMemberCount - 1);
+ });
+
+ it('does not update memberCount when user removal fails', async () => {
+ const oldMemberCount = guild.memberCount;
+ const user = mongoose.model('User', userSchema);
+ sandbox.stub(user.prototype, 'save').throws();
+
+ await expect(leader.post(`/groups/${guild._id}/removeMember/${member._id}`))
+ .to.eventually.be.rejected;
+
+ const updatedGuild = await leader.get(`/groups/${guild._id}`);
+ expect(updatedGuild.memberCount).to.equal(oldMemberCount);
+ });
+
+ it('does not update memberCount when group save fails', async () => {
+ const oldMemberCount = guild.memberCount;
+ const group = mongoose.model('Group', groupSchema);
+
+ sandbox.stub(group.prototype, 'save').throws(new Error('Failed to save group'));
+
+ await expect(leader.post(`/groups/${guild._id}/removeMember/${member._id}`))
+ .to.eventually.be.rejected;
+
+ const updatedGuild = await leader.get(`/groups/${guild._id}`);
+ expect(updatedGuild.memberCount).to.equal(oldMemberCount);
+ });
+
+ it('rolls back memberCount update if transaction fails', async () => {
+ const oldMemberCount = guild.memberCount;
+
+ sandbox.stub(mongoose, 'startSession').returns({
+ startTransaction: () => {},
+ commitTransaction: () => {
+ throw new Error('Transaction failed');
+ },
+ abortTransaction: () => {},
+ endSession: () => {},
+ });
+
+ await expect(leader.post(`/groups/${guild._id}/removeMember/${member._id}`))
+ .to.eventually.be.rejected;
+
+ const updatedGuild = await leader.get(`/groups/${guild._id}`);
+ expect(updatedGuild.memberCount).to.equal(oldMemberCount);
+ });
+
  it('can remove other invites', async () => {
  await leader.post(`/groups/${guild._id}/removeMember/${invitedUser._id}`);
 

website/server/controllers/api-v3/groups.js

@@ -1,6 +1,7 @@
 import _ from 'lodash';
 import nconf from 'nconf';
 import moment from 'moment';
+import mongoose from 'mongoose';
 import { authWithHeaders } from '../../middlewares/auth';
 import {
  model as Group,
@@ -14,6 +15,9 @@ import {
  NotFound,
  BadRequest,
  NotAuthorized,
+ TransactionError,
+ DatabaseError,
+ InternalServerError,
 } from '../../libs/errors';
 import { removeFromArray } from '../../libs/collectionManipulators';
 import { sendTxn as sendTxnEmail } from '../../libs/email';
@@ -957,14 +961,7 @@ api.removeGroupMember = {
  }
 
  if (isInGroup) {
- // For parties we count the number of members from the database to get the correct value.
- // See #12275 on why this is necessary and only done for parties.
- if (group.type === 'party') {
- const currentMembers = await group.getMemberCount();
- group.memberCount = currentMembers - 1;
- } else {
- group.memberCount -= 1;
- }
+ group.memberCount -= 1;
 
  if (group.quest && group.quest.leader === member._id) {
  throw new NotAuthorized(res.t('cannotRemoveQuestOwner'));
@@ -1005,10 +1002,28 @@ api.removeGroupMember = {
  const message = req.query.message || req.body.message;
  _sendMessageToRemoved(group, member, message, isInGroup);
 
- await Promise.all([
- member.save(),
- group.save(),
- ]);
+ const session = await mongoose.startSession();
+
+ try {
+ await session.withTransaction(async () => {
+ await member.save({ session });
+ await group.save({ session });
+ }, {
+ retryWrites: true,
+ });
+ } catch (err) {
+ if (err.name === 'MongoError') {
+ throw err.hasErrorLabel('TransactionTooLargeForCache')
+ ? new TransactionError(`Transaction too large for cache: ${err.message}`)
+ : new DatabaseError(`Database error: ${err.message}`);
+ } else if (err.name === 'ValidationError') {
+ throw validationErrors;
+ } else {
+ throw new InternalServerError(`Unexpected error: ${err.message}`);
+ }
+ } finally {
+ session.endSession();
+ }
 
  if (isInGroup && group.hasNotCancelled()) {
  await group.updateGroupPlan(true);

website/server/libs/errors.js

@@ -117,3 +117,39 @@ export class InternalServerError extends CustomError {
  this.message = customMessage || 'An unexpected error occurred.';
  }
 }
+
+/**
+ * @apiDefine TransactionError
+ * @apiError TransactionError An error occurred during a transaction that could not be resolved.
+ *
+ * @apiErrorExample Error-Response:
+ * HTTP/1.1 500 Internal Server Error
+ * {
+ * "error": "TransactionError",
+ * "message": "The transaction failed to commit after multiple attempts."
+ * }
+ */
+export class TransactionError extends InternalServerError {
+ constructor (customMessage) {
+ super();
+ this.message = customMessage || 'The transaction failed to commit after multiple attempts.';
+ }
+}
+
+/**
+ * @apiDefine DatabaseError
+ * @apiError DatabaseError An error occurred while interacting with the database.
+ *
+ * @apiErrorExample Error-Response:
+ * HTTP/1.1 500 Internal Server Error
+ * {
+ * "error": "DatabaseError",
+ * "message": "A database error occurred."
+ * }
+ */
+export class DatabaseError extends InternalServerError {
+ constructor (customMessage) {
+ super();
+ this.message = customMessage || 'A database error occurred.';
+ }
+}