diff --git a/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js b/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js index 7d234900315..499b9ea0ce9 100644 --- a/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js +++ b/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js @@ -1,3 +1,4 @@ +import mongoose from 'mongoose'; import { generateUser, createAndPopulateGroup, @@ -5,6 +6,8 @@ import { 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}`); diff --git a/website/server/controllers/api-v3/groups.js b/website/server/controllers/api-v3/groups.js index f8dfbe86d12..24efb6106b4 100644 --- a/website/server/controllers/api-v3/groups.js +++ b/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); diff --git a/website/server/libs/errors.js b/website/server/libs/errors.js index 2b570dbb7e3..9a3573db348 100644 --- a/website/server/libs/errors.js +++ b/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.'; + } +}