fix: Updating path responseBy would create a conflict on responseBy

.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.

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),
 	};
 

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,
+		});
+	});
+});