From 5665d12be118477613f2332070ec9831aafdf052 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 1 Jul 2024 18:39:31 -0300 Subject: [PATCH 01/14] improve: Allow removing dangling subscription when closing livechat rooms --- apps/meteor/app/livechat/server/api/v1/room.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/apps/meteor/app/livechat/server/api/v1/room.ts b/apps/meteor/app/livechat/server/api/v1/room.ts index 2196315ad013..31b658acd0db 100644 --- a/apps/meteor/app/livechat/server/api/v1/room.ts +++ b/apps/meteor/app/livechat/server/api/v1/room.ts @@ -28,6 +28,7 @@ import type { CloseRoomParams } from '../../lib/LivechatTyped'; import { Livechat as LivechatTyped } from '../../lib/LivechatTyped'; import { findGuest, findRoom, getRoom, settings, findAgent, onCheckRoomParams } from '../lib/livechat'; import { findVisitorInfo } from '../lib/visitors'; +import { Subscription } from 'sip.js'; const isAgentWithInfo = (agentObj: ILivechatAgent | { hiddenInfo: boolean }): agentObj is ILivechatAgent => !('hiddenInfo' in agentObj); @@ -182,11 +183,16 @@ API.v1.addRoute( throw new Error('error-invalid-room'); } + const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, this.userId, { projection: { _id: 1 } }); + if (!room.open && subscription) { + await Subscriptions.removeByRoomId(rid); + return API.v1.success(); + } + if (!room.open) { throw new Error('error-room-already-closed'); } - const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, this.userId, { projection: { _id: 1 } }); if (!subscription && !(await hasPermissionAsync(this.userId, 'close-others-livechat-room'))) { throw new Error('error-not-authorized'); } From 211efa84ab7a72d4e043259e5926e76a0ef2724b Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 1 Jul 2024 18:40:18 -0300 Subject: [PATCH 02/14] Remove new import --- apps/meteor/app/livechat/server/api/v1/room.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/app/livechat/server/api/v1/room.ts b/apps/meteor/app/livechat/server/api/v1/room.ts index 31b658acd0db..fc01091a57b5 100644 --- a/apps/meteor/app/livechat/server/api/v1/room.ts +++ b/apps/meteor/app/livechat/server/api/v1/room.ts @@ -28,7 +28,6 @@ import type { CloseRoomParams } from '../../lib/LivechatTyped'; import { Livechat as LivechatTyped } from '../../lib/LivechatTyped'; import { findGuest, findRoom, getRoom, settings, findAgent, onCheckRoomParams } from '../lib/livechat'; import { findVisitorInfo } from '../lib/visitors'; -import { Subscription } from 'sip.js'; const isAgentWithInfo = (agentObj: ILivechatAgent | { hiddenInfo: boolean }): agentObj is ILivechatAgent => !('hiddenInfo' in agentObj); From c120ca46cd0a76fc2154a163bdfee41d56d25546 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 5 Jul 2024 16:15:08 -0300 Subject: [PATCH 03/14] tests: add end-to-end tests --- .../tests/end-to-end/api/livechat/00-rooms.ts | 51 ++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts b/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts index 5c881b530d08..e315a4c42c64 100644 --- a/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts +++ b/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts @@ -10,10 +10,12 @@ import type { ILivechatPriority, ILivechatDepartment, ISubscription, + IUser, } from '@rocket.chat/core-typings'; import { LivechatPriorityWeight } from '@rocket.chat/core-typings'; import { expect } from 'chai'; import { after, before, describe, it } from 'mocha'; +import { MongoClient } from 'mongodb'; import type { Response } from 'supertest'; import type { SuccessResult } from '../../../../app/api/server/definition'; @@ -49,7 +51,7 @@ import { } from '../../../data/permissions.helper'; import { adminUsername, password } from '../../../data/user'; import { createUser, deleteUser, login } from '../../../data/users.helper'; -import { IS_EE } from '../../../e2e/config/constants'; +import { IS_EE, URL_MONGODB } from '../../../e2e/config/constants'; const getSubscriptionForRoom = async (roomId: string, overrideCredential?: Credentials): Promise => { const response = await request @@ -64,6 +66,32 @@ const getSubscriptionForRoom = async (roomId: string, overrideCredential?: Crede return subscription; }; +// For creating dangling subscriptions to rooms (which can't be created in usual conditions) +const addSubscriptionToDb = async (userId: IUser['_id'], room: IOmnichannelRoom) => { + const subscription = { + open: false, + alert: false, + unread: 0, + userMentions: 0, + groupMentions: 0, + ts: room.ts, + rid: room._id, + name: room.name, + fname: room.fname, + ...(room.customFields && { customFields: room.customFields }), + t: room.t, + u: { + _id: userId, + username: 'username', + name: 'name', + }, + }; + const connection = await MongoClient.connect(URL_MONGODB); + + await connection.db().collection('rocketchat_subscription').insertOne(subscription); + await connection.close(); +}; + describe('LIVECHAT - rooms', () => { let visitor: ILivechatVisitor; let room: IOmnichannelRoom; @@ -2136,6 +2164,27 @@ describe('LIVECHAT - rooms', () => { await request.post(api('livechat/room.closeByUser')).set(credentials).send({ rid: _id, comment: 'test' }).expect(400); await deleteVisitor(visitor.token); }); + it('should remove dangling subscriptions if they exist after a room has been closed', async () => { + const visitor = await createVisitor(); + const room = await createLivechatRoom(visitor.token); + + // close room + await request.post(api('livechat/room.closeByUser')).set(credentials).send({ rid: room._id, comment: 'test' }).expect(200); + + await addSubscriptionToDb(credentials['X-User-Id'], room); + const subscription = await getSubscriptionForRoom(room._id); + expect(subscription).to.be.an('object').that.is.not.empty; + expect(subscription).to.have.property('rid', room._id); + expect(subscription).to.have.property('u').that.is.an('object'); + expect(subscription.u).to.have.property('_id', credentials['X-User-Id']); + + // try to close again + await request.post(api('livechat/room.closeByUser')).set(credentials).send({ rid: room._id, comment: 'test' }).expect(200); + const updatedSubscription = await getSubscriptionForRoom(room._id); + expect(updatedSubscription).to.be.null; + + await deleteVisitor(visitor.token); + }); (IS_EE ? it : it.skip)('should close room and generate transcript pdf', async () => { const { From f161122846e39a83cb8a4e09b694c246f6832a48 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 5 Jul 2024 16:16:19 -0300 Subject: [PATCH 04/14] fix: Allow removing dangliong subscriptions in the livechat:closeRoom method --- .../app/livechat/server/methods/closeRoom.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/apps/meteor/app/livechat/server/methods/closeRoom.ts b/apps/meteor/app/livechat/server/methods/closeRoom.ts index 1374d86ab9f7..405c9305ec99 100644 --- a/apps/meteor/app/livechat/server/methods/closeRoom.ts +++ b/apps/meteor/app/livechat/server/methods/closeRoom.ts @@ -60,6 +60,16 @@ Meteor.methods({ }); } + const subscription = await SubscriptionRaw.findOneByRoomIdAndUserId(roomId, user._id, { + projection: { + _id: 1, + }, + }); + if (!room.open && subscription) { + await SubscriptionRaw.removeByRoomId(roomId); + return; + } + if (!room.open) { throw new Meteor.Error('room-closed', 'Room closed', { method: 'livechat:closeRoom' }); } @@ -71,11 +81,6 @@ Meteor.methods({ }); } - const subscription = await SubscriptionRaw.findOneByRoomIdAndUserId(roomId, user._id, { - projection: { - _id: 1, - }, - }); if (!subscription && !(await hasPermissionAsync(userId, 'close-others-livechat-room'))) { throw new Meteor.Error('error-not-authorized', 'Not authorized', { method: 'livechat:closeRoom', From 53218a62246781d2e69f12a970800f7b712ac399 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 5 Jul 2024 17:17:06 -0300 Subject: [PATCH 05/14] fix code check --- apps/meteor/app/livechat/server/methods/closeRoom.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/app/livechat/server/methods/closeRoom.ts b/apps/meteor/app/livechat/server/methods/closeRoom.ts index 405c9305ec99..5fdf9e7d504f 100644 --- a/apps/meteor/app/livechat/server/methods/closeRoom.ts +++ b/apps/meteor/app/livechat/server/methods/closeRoom.ts @@ -60,7 +60,7 @@ Meteor.methods({ }); } - const subscription = await SubscriptionRaw.findOneByRoomIdAndUserId(roomId, user._id, { + const subscription = await SubscriptionRaw.findOneByRoomIdAndUserId(roomId, userId, { projection: { _id: 1, }, From f2758e117b9b8553bd4896b44066c00d8987fe17 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Fri, 5 Jul 2024 17:33:31 -0300 Subject: [PATCH 06/14] Create changeset --- .changeset/happy-peaches-nail.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/happy-peaches-nail.md diff --git a/.changeset/happy-peaches-nail.md b/.changeset/happy-peaches-nail.md new file mode 100644 index 000000000000..52a2c3031d86 --- /dev/null +++ b/.changeset/happy-peaches-nail.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixed issue with livechat agents not being able to leave omnichannel rooms if joining after a room has been closed (due to race conditions) From 8d27824e02de6dd35ec1e435e53e656f0f71d4d1 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Thu, 11 Jul 2024 00:27:14 -0300 Subject: [PATCH 07/14] Update changeset --- .changeset/happy-peaches-nail.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/happy-peaches-nail.md b/.changeset/happy-peaches-nail.md index 52a2c3031d86..2dfb2151ced0 100644 --- a/.changeset/happy-peaches-nail.md +++ b/.changeset/happy-peaches-nail.md @@ -2,4 +2,4 @@ "@rocket.chat/meteor": patch --- -Fixed issue with livechat agents not being able to leave omnichannel rooms if joining after a room has been closed (due to race conditions) +Fixed issue with livechat agents not being able to leave omnichannel rooms if joining after a room has been closed by the visitor (due to race conditions) From 6fa63b0234d386a402e3da24b1a4e0e40615f807 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 11 Jul 2024 17:19:05 -0300 Subject: [PATCH 08/14] tests: Replace end-to-end tests by unit tests --- .../app/lib/server/functions/addUserToRoom.ts | 7 - .../lib/server/functions/closeLivechatRoom.ts | 81 +++++++++++ .../meteor/app/livechat/server/api/v1/room.ts | 54 +------- .../tests/end-to-end/api/livechat/00-rooms.ts | 51 +------ .../functions/closeLivechatRoom.tests.ts | 131 ++++++++++++++++++ 5 files changed, 216 insertions(+), 108 deletions(-) create mode 100644 apps/meteor/app/lib/server/functions/closeLivechatRoom.ts create mode 100644 apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts diff --git a/apps/meteor/app/lib/server/functions/addUserToRoom.ts b/apps/meteor/app/lib/server/functions/addUserToRoom.ts index 57ea20f00cb1..acf075004565 100644 --- a/apps/meteor/app/lib/server/functions/addUserToRoom.ts +++ b/apps/meteor/app/lib/server/functions/addUserToRoom.ts @@ -28,7 +28,6 @@ export const addUserToRoom = async function ( }); } - const userToBeAdded = typeof user === 'string' ? await Users.findOneByUsername(user.replace('@', '')) : await Users.findOneById(user._id); const roomDirectives = roomCoordinator.getRoomDirectives(room.t); if (!userToBeAdded) { @@ -50,12 +49,6 @@ export const addUserToRoom = async function ( await callbacks.run('beforeAddedToRoom', { user: userToBeAdded, inviter: userToBeAdded }); - // Check if user is already in room - const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, userToBeAdded._id); - if (subscription || !userToBeAdded) { - return; - } - try { await Apps.self?.triggerEvent(AppEvents.IPreRoomUserJoined, room, userToBeAdded, inviter); } catch (error: any) { diff --git a/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts b/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts new file mode 100644 index 000000000000..0e60c567d34e --- /dev/null +++ b/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts @@ -0,0 +1,81 @@ +import type { IUser, IRoom, IOmnichannelRoom } from '@rocket.chat/core-typings'; +import { isOmnichannelRoom } from '@rocket.chat/core-typings'; +import { LivechatRooms, Subscriptions } from '@rocket.chat/models'; + +import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission'; +import type { CloseRoomParams } from '../../../livechat/server/lib/LivechatTyped'; +import { Livechat } from '../../../livechat/server/lib/LivechatTyped'; + +export const closeLivechatRoom = async ( + user: IUser, + roomId: IRoom['_id'], + { + comment, + tags, + generateTranscriptPdf, + transcriptEmail, + }: { + comment?: string; + tags?: string[]; + generateTranscriptPdf?: boolean; + transcriptEmail?: + | { + sendToVisitor: false; + } + | { + sendToVisitor: true; + requestData: Pick, 'email' | 'subject'>; + }; + }, +): Promise => { + const room = await LivechatRooms.findOneById(roomId); + if (!room || !isOmnichannelRoom(room)) { + throw new Error('error-invalid-room'); + } + + const subscription = await Subscriptions.findOneByRoomIdAndUserId(roomId, user._id, { projection: { _id: 1 } }); + if (!room.open && subscription) { + await Subscriptions.removeByRoomId(roomId); + return; + } + + if (!room.open) { + throw new Error('error-room-already-closed'); + } + + if (!subscription && !(await hasPermissionAsync(user._id, 'close-others-livechat-room'))) { + throw new Error('error-not-authorized'); + } + + const options: CloseRoomParams['options'] = { + clientAction: true, + tags, + ...(generateTranscriptPdf && { pdfTranscript: { requestedBy: user._id } }), + ...(transcriptEmail && { + ...(transcriptEmail.sendToVisitor + ? { + emailTranscript: { + sendToVisitor: true, + requestData: { + email: transcriptEmail.requestData.email, + subject: transcriptEmail.requestData.subject, + requestedAt: new Date(), + requestedBy: user, + }, + }, + } + : { + emailTranscript: { + sendToVisitor: false, + }, + }), + }), + }; + + await Livechat.closeRoom({ + room, + user, + options, + comment, + }); +}; diff --git a/apps/meteor/app/livechat/server/api/v1/room.ts b/apps/meteor/app/livechat/server/api/v1/room.ts index fc01091a57b5..d2a76e53926f 100644 --- a/apps/meteor/app/livechat/server/api/v1/room.ts +++ b/apps/meteor/app/livechat/server/api/v1/room.ts @@ -1,7 +1,7 @@ import { Omnichannel } from '@rocket.chat/core-services'; import type { ILivechatAgent, IOmnichannelRoom, IUser, SelectedAgent, TransferByData } from '@rocket.chat/core-typings'; import { isOmnichannelRoom, OmnichannelSourceType } from '@rocket.chat/core-typings'; -import { LivechatVisitors, Users, LivechatRooms, Subscriptions, Messages } from '@rocket.chat/models'; +import { LivechatVisitors, Users, LivechatRooms, Messages } from '@rocket.chat/models'; import { Random } from '@rocket.chat/random'; import { isLiveChatRoomForwardProps, @@ -22,6 +22,7 @@ import { isWidget } from '../../../../api/server/helpers/isWidget'; import { canAccessRoomAsync, roomAccessAttributes } from '../../../../authorization/server'; import { hasPermissionAsync } from '../../../../authorization/server/functions/hasPermission'; import { addUserToRoom } from '../../../../lib/server/functions/addUserToRoom'; +import { closeLivechatRoom } from '../../../../lib/server/functions/closeLivechatRoom'; import { settings as rcSettings } from '../../../../settings/server'; import { normalizeTransferredByData } from '../../lib/Helper'; import type { CloseRoomParams } from '../../lib/LivechatTyped'; @@ -177,56 +178,7 @@ API.v1.addRoute( async post() { const { rid, comment, tags, generateTranscriptPdf, transcriptEmail } = this.bodyParams; - const room = await LivechatRooms.findOneById(rid); - if (!room || !isOmnichannelRoom(room)) { - throw new Error('error-invalid-room'); - } - - const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, this.userId, { projection: { _id: 1 } }); - if (!room.open && subscription) { - await Subscriptions.removeByRoomId(rid); - return API.v1.success(); - } - - if (!room.open) { - throw new Error('error-room-already-closed'); - } - - if (!subscription && !(await hasPermissionAsync(this.userId, 'close-others-livechat-room'))) { - throw new Error('error-not-authorized'); - } - - const options: CloseRoomParams['options'] = { - clientAction: true, - tags, - ...(generateTranscriptPdf && { pdfTranscript: { requestedBy: this.userId } }), - ...(transcriptEmail && { - ...(transcriptEmail.sendToVisitor - ? { - emailTranscript: { - sendToVisitor: true, - requestData: { - email: transcriptEmail.requestData.email, - subject: transcriptEmail.requestData.subject, - requestedAt: new Date(), - requestedBy: this.user, - }, - }, - } - : { - emailTranscript: { - sendToVisitor: false, - }, - }), - }), - }; - - await LivechatTyped.closeRoom({ - room, - user: this.user, - options, - comment, - }); + await closeLivechatRoom(this.user, rid, { comment, tags, generateTranscriptPdf, transcriptEmail }); return API.v1.success(); }, diff --git a/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts b/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts index e315a4c42c64..5c881b530d08 100644 --- a/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts +++ b/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts @@ -10,12 +10,10 @@ import type { ILivechatPriority, ILivechatDepartment, ISubscription, - IUser, } from '@rocket.chat/core-typings'; import { LivechatPriorityWeight } from '@rocket.chat/core-typings'; import { expect } from 'chai'; import { after, before, describe, it } from 'mocha'; -import { MongoClient } from 'mongodb'; import type { Response } from 'supertest'; import type { SuccessResult } from '../../../../app/api/server/definition'; @@ -51,7 +49,7 @@ import { } from '../../../data/permissions.helper'; import { adminUsername, password } from '../../../data/user'; import { createUser, deleteUser, login } from '../../../data/users.helper'; -import { IS_EE, URL_MONGODB } from '../../../e2e/config/constants'; +import { IS_EE } from '../../../e2e/config/constants'; const getSubscriptionForRoom = async (roomId: string, overrideCredential?: Credentials): Promise => { const response = await request @@ -66,32 +64,6 @@ const getSubscriptionForRoom = async (roomId: string, overrideCredential?: Crede return subscription; }; -// For creating dangling subscriptions to rooms (which can't be created in usual conditions) -const addSubscriptionToDb = async (userId: IUser['_id'], room: IOmnichannelRoom) => { - const subscription = { - open: false, - alert: false, - unread: 0, - userMentions: 0, - groupMentions: 0, - ts: room.ts, - rid: room._id, - name: room.name, - fname: room.fname, - ...(room.customFields && { customFields: room.customFields }), - t: room.t, - u: { - _id: userId, - username: 'username', - name: 'name', - }, - }; - const connection = await MongoClient.connect(URL_MONGODB); - - await connection.db().collection('rocketchat_subscription').insertOne(subscription); - await connection.close(); -}; - describe('LIVECHAT - rooms', () => { let visitor: ILivechatVisitor; let room: IOmnichannelRoom; @@ -2164,27 +2136,6 @@ describe('LIVECHAT - rooms', () => { await request.post(api('livechat/room.closeByUser')).set(credentials).send({ rid: _id, comment: 'test' }).expect(400); await deleteVisitor(visitor.token); }); - it('should remove dangling subscriptions if they exist after a room has been closed', async () => { - const visitor = await createVisitor(); - const room = await createLivechatRoom(visitor.token); - - // close room - await request.post(api('livechat/room.closeByUser')).set(credentials).send({ rid: room._id, comment: 'test' }).expect(200); - - await addSubscriptionToDb(credentials['X-User-Id'], room); - const subscription = await getSubscriptionForRoom(room._id); - expect(subscription).to.be.an('object').that.is.not.empty; - expect(subscription).to.have.property('rid', room._id); - expect(subscription).to.have.property('u').that.is.an('object'); - expect(subscription.u).to.have.property('_id', credentials['X-User-Id']); - - // try to close again - await request.post(api('livechat/room.closeByUser')).set(credentials).send({ rid: room._id, comment: 'test' }).expect(200); - const updatedSubscription = await getSubscriptionForRoom(room._id); - expect(updatedSubscription).to.be.null; - - await deleteVisitor(visitor.token); - }); (IS_EE ? it : it.skip)('should close room and generate transcript pdf', async () => { const { diff --git a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts new file mode 100644 index 000000000000..eb2044a1db0c --- /dev/null +++ b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts @@ -0,0 +1,131 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import proxyquire from 'proxyquire'; +import sinon from 'sinon'; + +import { createFakeRoom, createFakeSubscription, createFakeUser } from '../../../../../mocks/data'; + +const subscriptionsStub = { + findOneByRoomIdAndUserId: sinon.stub(), + removeByRoomId: sinon.stub(), +}; + +const livechatRoomsStub = { + findOneById: sinon.stub(), +}; + +const livechatStub = { + closeRoom: sinon.stub(), +}; + +const hasPermissionStub = sinon.stub(); + +const { closeLivechatRoom } = proxyquire.noCallThru().load('../../../../../../app/lib/server/functions/closeLivechatRoom.ts', { + '../../../livechat/server/lib/LivechatTyped': { + Livechat: livechatStub, + }, + '../../../authorization/server/functions/hasPermission': { + hasPermissionAsync: hasPermissionStub, + }, + '@rocket.chat/models': { + Subscriptions: subscriptionsStub, + LivechatRooms: livechatRoomsStub, + }, +}); + +describe('closeLivechatRoom', () => { + const user = createFakeUser(); + const room = createFakeRoom({ t: 'l', open: true }); + const subscription = createFakeSubscription({ rid: room._id, u: { username: user.username, _id: user._id } }); + + beforeEach(() => { + subscriptionsStub.findOneByRoomIdAndUserId.reset(); + subscriptionsStub.removeByRoomId.reset(); + livechatRoomsStub.findOneById.reset(); + livechatStub.closeRoom.reset(); + hasPermissionStub.reset(); + }); + + it('should not perform any operation when an invalid room id is provided', async () => { + livechatRoomsStub.findOneById.resolves(null); + hasPermissionStub.resolves(true); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-invalid-room'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should not perform any operation when a non-livechat room is provided', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, t: 'c' }); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + hasPermissionStub.resolves(true); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-invalid-room'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should not perform any operation when a closed room is provided and the caller is not subscribed to it', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + hasPermissionStub.resolves(true); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-room-already-closed'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should remove dangling subscription when a closed room is provided but the user is still subscribed to it', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + hasPermissionStub.resolves(true); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.calledOnceWith(room._id)).to.be.true; + }); + + it('should not perform any operation when the caller is not subscribed to the room and does not have the permission to close others rooms', async () => { + livechatRoomsStub.findOneById.resolves(room); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + hasPermissionStub.resolves(false); + + await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-not-authorized'); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should close the room when the caller is not subscribed to it but has the permission to close others rooms', async () => { + livechatRoomsStub.findOneById.resolves(room); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + hasPermissionStub.resolves(true); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.calledOnceWith(sinon.match({ room, user }))).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should close the room when the caller is subscribed to it and does not have the permission to close others rooms', async () => { + livechatRoomsStub.findOneById.resolves(room); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + hasPermissionStub.resolves(false); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.calledOnceWith(sinon.match({ room, user }))).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); +}); From 6363aeafcbb1038014465f40986f3c2cf478cff3 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 11 Jul 2024 17:20:44 -0300 Subject: [PATCH 09/14] Remove unrelated changes --- apps/meteor/app/lib/server/functions/addUserToRoom.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/apps/meteor/app/lib/server/functions/addUserToRoom.ts b/apps/meteor/app/lib/server/functions/addUserToRoom.ts index acf075004565..57ea20f00cb1 100644 --- a/apps/meteor/app/lib/server/functions/addUserToRoom.ts +++ b/apps/meteor/app/lib/server/functions/addUserToRoom.ts @@ -28,6 +28,7 @@ export const addUserToRoom = async function ( }); } + const userToBeAdded = typeof user === 'string' ? await Users.findOneByUsername(user.replace('@', '')) : await Users.findOneById(user._id); const roomDirectives = roomCoordinator.getRoomDirectives(room.t); if (!userToBeAdded) { @@ -49,6 +50,12 @@ export const addUserToRoom = async function ( await callbacks.run('beforeAddedToRoom', { user: userToBeAdded, inviter: userToBeAdded }); + // Check if user is already in room + const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, userToBeAdded._id); + if (subscription || !userToBeAdded) { + return; + } + try { await Apps.self?.triggerEvent(AppEvents.IPreRoomUserJoined, room, userToBeAdded, inviter); } catch (error: any) { From e95039df8ebfedffff7007af163850938041b464 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 15 Jul 2024 11:06:37 -0300 Subject: [PATCH 10/14] improve: close any dangling subscription --- .../lib/server/functions/closeLivechatRoom.ts | 12 +++++----- .../functions/closeLivechatRoom.tests.ts | 24 ++++++++++++++++++- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts b/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts index 0e60c567d34e..b716be044d57 100644 --- a/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts +++ b/apps/meteor/app/lib/server/functions/closeLivechatRoom.ts @@ -33,16 +33,16 @@ export const closeLivechatRoom = async ( throw new Error('error-invalid-room'); } - const subscription = await Subscriptions.findOneByRoomIdAndUserId(roomId, user._id, { projection: { _id: 1 } }); - if (!room.open && subscription) { - await Subscriptions.removeByRoomId(roomId); - return; - } - if (!room.open) { + const subscriptionsLeft = await Subscriptions.countByRoomId(roomId); + if (subscriptionsLeft) { + await Subscriptions.removeByRoomId(roomId); + return; + } throw new Error('error-room-already-closed'); } + const subscription = await Subscriptions.findOneByRoomIdAndUserId(roomId, user._id, { projection: { _id: 1 } }); if (!subscription && !(await hasPermissionAsync(user._id, 'close-others-livechat-room'))) { throw new Error('error-not-authorized'); } diff --git a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts index eb2044a1db0c..3462688dd11b 100644 --- a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts +++ b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts @@ -8,6 +8,7 @@ import { createFakeRoom, createFakeSubscription, createFakeUser } from '../../.. const subscriptionsStub = { findOneByRoomIdAndUserId: sinon.stub(), removeByRoomId: sinon.stub(), + countByRoomId: sinon.stub(), }; const livechatRoomsStub = { @@ -41,6 +42,7 @@ describe('closeLivechatRoom', () => { beforeEach(() => { subscriptionsStub.findOneByRoomIdAndUserId.reset(); subscriptionsStub.removeByRoomId.reset(); + subscriptionsStub.countByRoomId.reset(); livechatRoomsStub.findOneById.reset(); livechatStub.closeRoom.reset(); hasPermissionStub.reset(); @@ -69,8 +71,9 @@ describe('closeLivechatRoom', () => { expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; }); - it('should not perform any operation when a closed room is provided and the caller is not subscribed to it', async () => { + it('should not perform any operation when a closed room with no subscriptions is provided and the caller is not subscribed to it', async () => { livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.countByRoomId.resolves(0); subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); hasPermissionStub.resolves(true); @@ -78,12 +81,28 @@ describe('closeLivechatRoom', () => { expect(livechatStub.closeRoom.notCalled).to.be.true; expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + }); + + it('should remove dangling subscription when a closed room with subscriptions is provided and the caller is not subscribed to it', async () => { + livechatRoomsStub.findOneById.resolves({ ...room, open: false }); + subscriptionsStub.countByRoomId.resolves(1); + subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + hasPermissionStub.resolves(true); + + await closeLivechatRoom(user, room._id, {}); + expect(livechatStub.closeRoom.notCalled).to.be.true; + expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; }); it('should remove dangling subscription when a closed room is provided but the user is still subscribed to it', async () => { livechatRoomsStub.findOneById.resolves({ ...room, open: false }); subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + subscriptionsStub.countByRoomId.resolves(1); hasPermissionStub.resolves(true); await closeLivechatRoom(user, room._id, {}); @@ -96,6 +115,7 @@ describe('closeLivechatRoom', () => { it('should not perform any operation when the caller is not subscribed to the room and does not have the permission to close others rooms', async () => { livechatRoomsStub.findOneById.resolves(room); subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + subscriptionsStub.countByRoomId.resolves(1); hasPermissionStub.resolves(false); await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-not-authorized'); @@ -108,6 +128,7 @@ describe('closeLivechatRoom', () => { it('should close the room when the caller is not subscribed to it but has the permission to close others rooms', async () => { livechatRoomsStub.findOneById.resolves(room); subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); + subscriptionsStub.countByRoomId.resolves(1); hasPermissionStub.resolves(true); await closeLivechatRoom(user, room._id, {}); @@ -120,6 +141,7 @@ describe('closeLivechatRoom', () => { it('should close the room when the caller is subscribed to it and does not have the permission to close others rooms', async () => { livechatRoomsStub.findOneById.resolves(room); subscriptionsStub.findOneByRoomIdAndUserId.resolves(subscription); + subscriptionsStub.countByRoomId.resolves(1); hasPermissionStub.resolves(false); await closeLivechatRoom(user, room._id, {}); From 9ae42695ec912b63f88fbf9738c616d9dad1c06b Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 15 Jul 2024 11:16:26 -0300 Subject: [PATCH 11/14] tests: fix unit tests --- .../app/lib/server/functions/closeLivechatRoom.tests.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts index 3462688dd11b..a0a032a885de 100644 --- a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts +++ b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts @@ -80,7 +80,7 @@ describe('closeLivechatRoom', () => { await expect(closeLivechatRoom(user, room._id, {})).to.be.rejectedWith('error-room-already-closed'); expect(livechatStub.closeRoom.notCalled).to.be.true; expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; - expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; }); @@ -94,7 +94,7 @@ describe('closeLivechatRoom', () => { await closeLivechatRoom(user, room._id, {}); expect(livechatStub.closeRoom.notCalled).to.be.true; expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; - expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; }); @@ -108,7 +108,8 @@ describe('closeLivechatRoom', () => { await closeLivechatRoom(user, room._id, {}); expect(livechatStub.closeRoom.notCalled).to.be.true; expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; - expect(subscriptionsStub.findOneByRoomIdAndUserId.calledOnceWith(room._id, user._id)).to.be.true; + expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; + expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; expect(subscriptionsStub.removeByRoomId.calledOnceWith(room._id)).to.be.true; }); From 9cfb3eb3ebcab413f84b38bfc699384d2c772d46 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Mon, 15 Jul 2024 12:23:11 -0300 Subject: [PATCH 12/14] fix unit tests once again --- .../unit/app/lib/server/functions/closeLivechatRoom.tests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts index a0a032a885de..07ee437832d2 100644 --- a/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts +++ b/apps/meteor/tests/unit/app/lib/server/functions/closeLivechatRoom.tests.ts @@ -96,7 +96,7 @@ describe('closeLivechatRoom', () => { expect(livechatRoomsStub.findOneById.calledOnceWith(room._id)).to.be.true; expect(subscriptionsStub.findOneByRoomIdAndUserId.notCalled).to.be.true; expect(subscriptionsStub.countByRoomId.calledOnceWith(room._id)).to.be.true; - expect(subscriptionsStub.removeByRoomId.notCalled).to.be.true; + expect(subscriptionsStub.removeByRoomId.calledOnceWith(room._id)).to.be.true; }); it('should remove dangling subscription when a closed room is provided but the user is still subscribed to it', async () => { @@ -113,7 +113,7 @@ describe('closeLivechatRoom', () => { expect(subscriptionsStub.removeByRoomId.calledOnceWith(room._id)).to.be.true; }); - it('should not perform any operation when the caller is not subscribed to the room and does not have the permission to close others rooms', async () => { + it('should not perform any operation when the caller is not subscribed to an open room and does not have the permission to close others rooms', async () => { livechatRoomsStub.findOneById.resolves(room); subscriptionsStub.findOneByRoomIdAndUserId.resolves(null); subscriptionsStub.countByRoomId.resolves(1); From aa2e45da0309920a719361b34ba475f6c17dcefb Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 15 Jul 2024 14:04:42 -0300 Subject: [PATCH 13/14] improve: display button to close livechat room when there is a dangling subscription --- .../Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx b/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx index 7446d0630b09..4b12adba52fb 100644 --- a/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx +++ b/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx @@ -10,6 +10,7 @@ import { useMethod, useTranslation, useRouter, + useUserSubscription, } from '@rocket.chat/ui-contexts'; import React, { useCallback, useState, useEffect } from 'react'; @@ -47,6 +48,7 @@ export const useQuickActions = (): { const visitorRoomId = room.v._id; const rid = room._id; const uid = useUserId(); + const subscription = useUserSubscription(rid); const roomLastMessage = room.lastMessage; const getVisitorInfo = useEndpoint('GET', '/v1/livechat/visitors.info'); @@ -330,7 +332,7 @@ export const useQuickActions = (): { case QuickActionsEnum.TranscriptPDF: return hasLicense && !isRoomOverMacLimit && canSendTranscriptPDF; case QuickActionsEnum.CloseChat: - return !!roomOpen && (canCloseRoom || canCloseOthersRoom); + return (subscription && canCloseRoom) || (!!roomOpen && canCloseOthersRoom); case QuickActionsEnum.OnHoldChat: return !!roomOpen && canPlaceChatOnHold; default: From a40a92ba2c95238a69d6e11d1aa3de400063d22a Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 15 Jul 2024 14:15:56 -0300 Subject: [PATCH 14/14] improve condition to display button to close livechat rooms --- .../Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx b/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx index 4b12adba52fb..edf5ffcbdc8a 100644 --- a/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx +++ b/apps/meteor/client/views/room/Header/Omnichannel/QuickActions/hooks/useQuickActions.tsx @@ -332,7 +332,7 @@ export const useQuickActions = (): { case QuickActionsEnum.TranscriptPDF: return hasLicense && !isRoomOverMacLimit && canSendTranscriptPDF; case QuickActionsEnum.CloseChat: - return (subscription && canCloseRoom) || (!!roomOpen && canCloseOthersRoom); + return (subscription && (canCloseRoom || canCloseOthersRoom)) || (!!roomOpen && canCloseOthersRoom); case QuickActionsEnum.OnHoldChat: return !!roomOpen && canPlaceChatOnHold; default: