From 065a74258a093c6f4dc0e4ebd254d4ea34bd8e02 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 15 Oct 2024 11:16:29 -0600 Subject: [PATCH] fix: Updating path `responseBy` would create a conflict on `responseBy` (#33412) --- .changeset/purple-papayas-collect.md | 5 + .../server/hooks/markRoomResponded.ts | 15 +- .../livechat/hooks/markRoomResponded.spec.ts | 143 ++++++++++++++++++ 3 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 .changeset/purple-papayas-collect.md create mode 100644 apps/meteor/tests/unit/server/livechat/hooks/markRoomResponded.spec.ts diff --git a/.changeset/purple-papayas-collect.md b/.changeset/purple-papayas-collect.md new file mode 100644 index 000000000000..8ebd54fe17ad --- /dev/null +++ b/.changeset/purple-papayas-collect.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixes a logical error when updating the `responseBy` property of a room while `waitingResponse` property was still defined. diff --git a/apps/meteor/app/livechat/server/hooks/markRoomResponded.ts b/apps/meteor/app/livechat/server/hooks/markRoomResponded.ts index 6820bd4664bd..3720780339fb 100644 --- a/apps/meteor/app/livechat/server/hooks/markRoomResponded.ts +++ b/apps/meteor/app/livechat/server/hooks/markRoomResponded.ts @@ -33,10 +33,6 @@ export async function markRoomResponded( } } - if (room.responseBy) { - LivechatRooms.getAgentLastMessageTsUpdateQuery(roomUpdater); - } - if (!room.waitingResponse) { // case where agent sends second message or any subsequent message in a room before visitor responds to the first message // in this case, we just need to update the lastMessageTs of the responseBy object @@ -47,10 +43,13 @@ export async function markRoomResponded( return room.responseBy; } - const responseBy: IOmnichannelRoom['responseBy'] = room.responseBy || { - _id: message.u._id, - username: message.u.username, - firstResponseTs: new Date(message.ts), + // Since we're updating the whole object anyways, we re-use the same values from object (or from message if not present) + // And then we update the lastMessageTs, which is the only thing that should be updating here + const { responseBy: { _id, username, firstResponseTs } = {} } = room; + const responseBy: IOmnichannelRoom['responseBy'] = { + _id: _id || message.u._id, + username: username || message.u.username, + firstResponseTs: firstResponseTs || new Date(message.ts), lastMessageTs: new Date(message.ts), }; diff --git a/apps/meteor/tests/unit/server/livechat/hooks/markRoomResponded.spec.ts b/apps/meteor/tests/unit/server/livechat/hooks/markRoomResponded.spec.ts new file mode 100644 index 000000000000..d8372f6282c3 --- /dev/null +++ b/apps/meteor/tests/unit/server/livechat/hooks/markRoomResponded.spec.ts @@ -0,0 +1,143 @@ +import { expect } from 'chai'; +import { describe, it, beforeEach } from 'mocha'; +import proxyquire from 'proxyquire'; +import Sinon from 'sinon'; + +const models = { + LivechatVisitors: { isVisitorActiveOnPeriod: Sinon.stub(), markVisitorActiveForPeriod: Sinon.stub() }, + LivechatInquiry: { markInquiryActiveForPeriod: Sinon.stub() }, + LivechatRooms: { + getVisitorActiveForPeriodUpdateQuery: Sinon.stub(), + getAgentLastMessageTsUpdateQuery: Sinon.stub(), + getResponseByRoomIdUpdateQuery: Sinon.stub(), + }, +}; + +const { markRoomResponded } = proxyquire.load('../../../../../app/livechat/server/hooks/markRoomResponded.ts', { + '../../../../lib/callbacks': { callbacks: { add: Sinon.stub(), priority: { HIGH: 'high' } } }, + '../../../lib/server/lib/notifyListener': { notifyOnLivechatInquiryChanged: Sinon.stub() }, + '@rocket.chat/models': models, +}); + +describe('markRoomResponded', () => { + beforeEach(() => { + models.LivechatVisitors.isVisitorActiveOnPeriod.reset(); + models.LivechatVisitors.markVisitorActiveForPeriod.reset(); + models.LivechatInquiry.markInquiryActiveForPeriod.reset(); + models.LivechatRooms.getVisitorActiveForPeriodUpdateQuery.reset(); + models.LivechatRooms.getAgentLastMessageTsUpdateQuery.reset(); + models.LivechatRooms.getResponseByRoomIdUpdateQuery.reset(); + }); + + it('should return void if message is system message', async () => { + const message = { + t: 'livechat-started', + }; + + const room = {}; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.undefined; + }); + + it('should return void if message is edited message', async () => { + const message = { + editedAt: new Date(), + editedBy: { _id: '123' }, + }; + + const room = {}; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.undefined; + }); + + it('should return void if message is from visitor', async () => { + const message = { + token: 'token', + }; + + const room = {}; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.undefined; + }); + + it('should try to mark visitor as active for current period', async () => { + const message = {}; + const room = { v: { _id: '1234' } }; + + await markRoomResponded(message, room, {}); + + expect(models.LivechatVisitors.markVisitorActiveForPeriod.calledOnce).to.be.true; + }); + + it('should try to mark inquiry as active for current period when room.v.activity doesnt include current period', async () => { + const message = {}; + const room = { v: { activity: [] } }; + + models.LivechatInquiry.markInquiryActiveForPeriod.resolves({}); + + await markRoomResponded(message, room, {}); + + expect(models.LivechatInquiry.markInquiryActiveForPeriod.calledOnce).to.be.true; + }); + + it('should return room.responseBy when room is not waiting for response', async () => { + const message = {}; + const room = { v: { _id: '1234' }, waitingResponse: false, responseBy: { _id: '1234' } }; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.equal(room.responseBy); + expect(models.LivechatRooms.getAgentLastMessageTsUpdateQuery.calledOnce).to.be.true; + }); + + it('should try to update the lastMessageTs property when a room was already answered by an agent', async () => { + const message = { u: { _id: '1234', username: 'username' }, ts: new Date() }; + const room = { responseBy: { _id: '1234' }, v: { _id: '1234' } }; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.deep.equal(room.responseBy); + expect(models.LivechatRooms.getAgentLastMessageTsUpdateQuery.calledOnce).to.be.true; + }); + + it('should add a new responseBy object when room is waiting for response', async () => { + const message = { u: { _id: '1234', username: 'username' }, ts: new Date() }; + const room = { waitingResponse: true, v: { _id: '1234' } }; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.deep.equal({ _id: '1234', username: 'username', firstResponseTs: message.ts, lastMessageTs: message.ts }); + expect(models.LivechatRooms.getResponseByRoomIdUpdateQuery.calledOnce).to.be.true; + expect(models.LivechatRooms.getResponseByRoomIdUpdateQuery.getCall(0).args[0]).to.be.deep.equal({ + _id: '1234', + lastMessageTs: message.ts, + firstResponseTs: message.ts, + username: 'username', + }); + }); + + // This should never happpen on the wild, checking because of a data inconsistency bug found + it('should update only the lastMessageTs property when a room has both waitingResponse and responseBy properties', async () => { + const message = { u: { _id: '1234', username: 'username' }, ts: new Date() }; + const room = { + waitingResponse: true, + responseBy: { _id: '1234', username: 'username', firstResponseTs: new Date() }, + v: { _id: '1234' }, + }; + + const res = await markRoomResponded(message, room, {}); + + expect(res).to.be.deep.equal({ + _id: '1234', + username: 'username', + firstResponseTs: room.responseBy.firstResponseTs, + lastMessageTs: message.ts, + }); + }); +});