From f080aa788c0d00552bcb86485d571ca49d803a84 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Wed, 7 May 2025 14:39:01 -0600 Subject: [PATCH] chore: Improve PDF Transcript request lock mechanism (#35809) --- .../statistics/server/lib/getEEStatistics.ts | 9 +---- .../server/api/transcript.ts | 8 +++- .../server/lib/requestPdfTranscript.ts | 17 ++++---- .../ee/server/models/raw/LivechatRooms.ts | 5 --- .../tests/end-to-end/api/livechat/00-rooms.ts | 2 +- .../livechat/lib/requestPdfTranscript.spec.ts | 40 +++++++++---------- .../src/OmnichannelTranscript.spec.ts | 2 - .../src/OmnichannelTranscript.ts | 20 +++++++--- packages/core-typings/src/IRoom.ts | 3 -- .../src/models/ILivechatRoomsModel.ts | 3 -- packages/models/src/models/LivechatRooms.ts | 31 +------------- 11 files changed, 56 insertions(+), 84 deletions(-) diff --git a/apps/meteor/app/statistics/server/lib/getEEStatistics.ts b/apps/meteor/app/statistics/server/lib/getEEStatistics.ts index 6511feeabb7..0e04a4fab57 100644 --- a/apps/meteor/app/statistics/server/lib/getEEStatistics.ts +++ b/apps/meteor/app/statistics/server/lib/getEEStatistics.ts @@ -93,13 +93,8 @@ async function getEEStatistics(): Promise { }), ); - // Number of PDF transcript requested - statsPms.push( - LivechatRooms.countRoomsWithPdfTranscriptRequested().then((count) => { - statistics.omnichannelPdfTranscriptRequested = count; - }), - ); - + // NOTE: keeping this for compatibility with current stats. Will be removed next major + statistics.omnichannelPdfTranscriptRequested = 0; // Number of PDF transcript that succeeded statsPms.push( LivechatRooms.countRoomsWithTranscriptSent().then((count) => { diff --git a/apps/meteor/ee/app/livechat-enterprise/server/api/transcript.ts b/apps/meteor/ee/app/livechat-enterprise/server/api/transcript.ts index 27b1764bf66..1db808c44ff 100644 --- a/apps/meteor/ee/app/livechat-enterprise/server/api/transcript.ts +++ b/apps/meteor/ee/app/livechat-enterprise/server/api/transcript.ts @@ -1,3 +1,4 @@ +import type { IOmnichannelRoom } from '@rocket.chat/core-typings'; import { LivechatRooms } from '@rocket.chat/models'; import { API } from '../../../../../app/api/server'; @@ -9,7 +10,12 @@ API.v1.addRoute( { authRequired: true, permissionsRequired: ['request-pdf-transcript'], license: ['livechat-enterprise'] }, { async post() { - const room = await LivechatRooms.findOneById(this.urlParams.rid); + const room = await LivechatRooms.findOneById>( + this.urlParams.rid, + { + projection: { _id: 1, open: 1, v: 1, t: 1, pdfTranscriptFileId: 1 }, + }, + ); if (!room) { throw new Error('error-invalid-room'); } diff --git a/apps/meteor/ee/app/livechat-enterprise/server/lib/requestPdfTranscript.ts b/apps/meteor/ee/app/livechat-enterprise/server/lib/requestPdfTranscript.ts index e0bb03e6d85..8bc1b78d397 100644 --- a/apps/meteor/ee/app/livechat-enterprise/server/lib/requestPdfTranscript.ts +++ b/apps/meteor/ee/app/livechat-enterprise/server/lib/requestPdfTranscript.ts @@ -1,12 +1,16 @@ import { OmnichannelTranscript, QueueWorker } from '@rocket.chat/core-services'; import type { AtLeast, IOmnichannelRoom } from '@rocket.chat/core-typings'; -import { LivechatRooms } from '@rocket.chat/models'; +import ExpiryMap from 'expiry-map'; import { logger } from './logger'; +// Allow to request a transcript again after 15 seconds, assuming the first one didn't complete +// This won't prevent multiple transcript generated for the same room in a multi-instance deployment since state is not shared, but we're ok with the drawbacks +const LockMap = new ExpiryMap(15000); + const serviceName = 'omnichannel-transcript' as const; export const requestPdfTranscript = async ( - room: AtLeast, + room: AtLeast, requestedBy: string, ): Promise => { if (room.open) { @@ -17,15 +21,14 @@ export const requestPdfTranscript = async ( throw new Error('improper-room-state'); } - // Don't request a transcript if there's already one requested :) - if (room.pdfTranscriptRequested) { + // Don't request a transcript if there's already one requested + if (LockMap.has(room._id) || room.pdfTranscriptFileId) { // TODO: use logger - logger.info(`Transcript already requested for room ${room._id}`); + logger.info({ msg: `Transcript already requested`, roomId: room._id }); return; } - // TODO: change this with a timestamp, allowing users to request a transcript again after a while if the first one fails - await LivechatRooms.setTranscriptRequestedPdfById(room._id); + LockMap.set(room._id, true); const details = { details: { rid: room._id, userId: requestedBy, from: serviceName } }; // Make the whole process sync when running on test mode diff --git a/apps/meteor/ee/server/models/raw/LivechatRooms.ts b/apps/meteor/ee/server/models/raw/LivechatRooms.ts index 60cb5bf4916..a4f2c639aa4 100644 --- a/apps/meteor/ee/server/models/raw/LivechatRooms.ts +++ b/apps/meteor/ee/server/models/raw/LivechatRooms.ts @@ -43,7 +43,6 @@ declare module '@rocket.chat/model-typings' { unsetPriorityByRoomId(roomId: string): Promise; countPrioritizedRooms(): Promise; countRoomsWithSla(): Promise; - countRoomsWithPdfTranscriptRequested(): Promise; countRoomsWithTranscriptSent(): Promise; getConversationsBySource(start: Date, end: Date, extraQuery: Filter): AggregationCursor; getConversationsByStatus(start: Date, end: Date, extraQuery: Filter): AggregationCursor; @@ -89,10 +88,6 @@ export class LivechatRoomsRawEE extends LivechatRoomsRaw implements ILivechatRoo return this.countDocuments({ slaId: { $exists: true } }); } - countRoomsWithPdfTranscriptRequested(): Promise { - return this.countDocuments({ pdfTranscriptRequested: true }); - } - countRoomsWithTranscriptSent(): Promise { return this.countDocuments({ pdfTranscriptFileId: { $exists: true } }); } 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 bff5aa27b18..c577bd4aa34 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 @@ -2788,7 +2788,7 @@ describe('LIVECHAT - rooms', () => { roomWithTranscriptGenerated = roomId; }); - it('should return immediately if transcript was already requested', async () => { + it('should return when transcript was already requested', async () => { await request .post(api(`omnichannel/${roomWithTranscriptGenerated}/request-transcript`)) .set(credentials) diff --git a/apps/meteor/tests/unit/server/livechat/lib/requestPdfTranscript.spec.ts b/apps/meteor/tests/unit/server/livechat/lib/requestPdfTranscript.spec.ts index 706e06704b5..f9eadc7dae9 100644 --- a/apps/meteor/tests/unit/server/livechat/lib/requestPdfTranscript.spec.ts +++ b/apps/meteor/tests/unit/server/livechat/lib/requestPdfTranscript.spec.ts @@ -3,18 +3,12 @@ import { describe, it, beforeEach, after } from 'mocha'; import proxyquire from 'proxyquire'; import sinon from 'sinon'; -const setStub = sinon.stub(); const workOnPdfStub = sinon.stub(); const queueWorkStub = sinon.stub(); const { requestPdfTranscript } = proxyquire .noCallThru() .load('../../../../../ee/app/livechat-enterprise/server/lib/requestPdfTranscript.ts', { - '@rocket.chat/models': { - LivechatRooms: { - setTranscriptRequestedPdfById: setStub, - }, - }, '@rocket.chat/core-services': { OmnichannelTranscript: { workOnPdf: workOnPdfStub, @@ -29,7 +23,6 @@ describe('requestPdfTranscript', () => { const currentTestModeValue = process.env.TEST_MODE; beforeEach(() => { - setStub.reset(); workOnPdfStub.reset(); queueWorkStub.reset(); }); @@ -44,30 +37,37 @@ describe('requestPdfTranscript', () => { it('should throw an error if room doesnt have a v property', async () => { await expect(requestPdfTranscript({}, 'userId')).to.be.rejectedWith('improper-room-state'); }); - it('should not request a transcript if it was already requested', async () => { - await requestPdfTranscript({ v: 1, pdfTranscriptRequested: true }, 'userId'); - expect(setStub.callCount).to.equal(0); - expect(workOnPdfStub.callCount).to.equal(0); - expect(queueWorkStub.callCount).to.equal(0); + it('should not allow to request a transcript if it already exists', async () => { + const result = await requestPdfTranscript({ _id: 'roomIdxx', v: 1, pdfTranscriptFileId: 'afsdafadsfs' }, 'userId'); + expect(result).to.be.undefined; }); - it('should set pdfTranscriptRequested to true on room', async () => { - await requestPdfTranscript({ _id: 'roomId', v: {}, pdfTranscriptRequested: false }, 'userId'); - expect(setStub.calledWith('roomId')).to.be.true; + it('should not allow to request a transcript if it was already requested during the previous 15 seconds', async () => { + await requestPdfTranscript({ _id: 'roomId', v: 1 }, 'userId'); + expect(workOnPdfStub.callCount).to.equal(0); + expect(queueWorkStub.callCount).to.equal(1); + + const result = await requestPdfTranscript({ _id: 'roomId', v: 1 }, 'userId'); + expect(workOnPdfStub.callCount).to.equal(0); + expect(queueWorkStub.callCount).to.equal(1); + expect(result).to.be.undefined; }); it('should call workOnPdf if TEST_MODE is true', async () => { process.env.TEST_MODE = 'true'; - await requestPdfTranscript({ _id: 'roomId', v: {} }, 'userId'); - expect(workOnPdfStub.getCall(0).calledWithExactly({ details: { rid: 'roomId', userId: 'userId', from: 'omnichannel-transcript' } })).to - .be.true; + await requestPdfTranscript({ _id: 'roomId-fasdafsdas', v: {} }, 'userId'); + expect( + workOnPdfStub + .getCall(0) + .calledWithExactly({ details: { rid: 'roomId-fasdafsdas', userId: 'userId', from: 'omnichannel-transcript' } }), + ).to.be.true; expect(queueWorkStub.calledOnce).to.be.false; }); it('should queue work if TEST_MODE is not set', async () => { delete process.env.TEST_MODE; - await requestPdfTranscript({ _id: 'roomId', v: {} }, 'userId'); + await requestPdfTranscript({ _id: 'roomId-afsdfaefzv', v: {} }, 'userId'); expect(workOnPdfStub.calledOnce).to.be.false; expect( queueWorkStub.getCall(0).calledWithExactly('work', 'omnichannel-transcript.workOnPdf', { - details: { rid: 'roomId', userId: 'userId', from: 'omnichannel-transcript' }, + details: { rid: 'roomId-afsdfaefzv', userId: 'userId', from: 'omnichannel-transcript' }, }), ).to.be.true; }); diff --git a/ee/packages/omnichannel-services/src/OmnichannelTranscript.spec.ts b/ee/packages/omnichannel-services/src/OmnichannelTranscript.spec.ts index 4620fea206d..306be618fb4 100644 --- a/ee/packages/omnichannel-services/src/OmnichannelTranscript.spec.ts +++ b/ee/packages/omnichannel-services/src/OmnichannelTranscript.spec.ts @@ -37,8 +37,6 @@ jest.mock('@rocket.chat/core-services', () => ({ jest.mock('@rocket.chat/models', () => ({ LivechatRooms: { findOneById: jest.fn().mockResolvedValue({}), - unsetTranscriptRequestedPdfById: jest.fn(), - setPdfTranscriptFileIdById: jest.fn(), }, Messages: { findLivechatMessagesWithoutTypes: jest.fn().mockReturnValue({ diff --git a/ee/packages/omnichannel-services/src/OmnichannelTranscript.ts b/ee/packages/omnichannel-services/src/OmnichannelTranscript.ts index 20288a4427c..14ec8b051a6 100644 --- a/ee/packages/omnichannel-services/src/OmnichannelTranscript.ts +++ b/ee/packages/omnichannel-services/src/OmnichannelTranscript.ts @@ -18,6 +18,7 @@ import type { ILivechatAgent, IOmnichannelSystemMessage, AtLeast, + IOmnichannelRoom, } from '@rocket.chat/core-typings'; import { isQuoteAttachment, isFileAttachment, isFileImageAttachment } from '@rocket.chat/core-typings'; import type { Logger } from '@rocket.chat/logger'; @@ -371,10 +372,19 @@ export class OmnichannelTranscript extends ServiceClass implements IOmnichannelT } this.currentJobNumber++; try { - const room = await LivechatRooms.findOneById(details.rid); + const room = await LivechatRooms.findOneById>( + details.rid, + { + projection: { v: 1, servedBy: 1, pdfTranscriptFileId: 1, closedAt: 1 }, + }, + ); if (!room) { throw new Error('room-not-found'); } + if (room.pdfTranscriptFileId) { + this.log.info(`Processing transcript for room ${details.rid} by user ${details.userId} - PDF already exists`); + return; + } const messages = await this.getMessagesFromRoom({ rid: room._id }); const visitor = @@ -436,18 +446,16 @@ export class OmnichannelTranscript extends ServiceClass implements IOmnichannelT private async pdfFailed({ details, e }: { details: WorkDetailsWithSource; e: Error }): Promise { this.log.error(`Transcript for room ${details.rid} by user ${details.userId} - Failed: ${e.message}`); - const room = await LivechatRooms.findOneById(details.rid); + const room = await LivechatRooms.findOneById>(details.rid, { projection: { _id: 1 } }); if (!room) { return; } - const user = await Users.findOneById(details.userId); + // TODO: fix types of translate service (or deprecate, if possible) + const user = await Users.findOneById(details.userId, { projection: { _id: 1, language: 1 } }); if (!user) { return; } - // Remove `transcriptRequestedPdf` from room to allow another request - await LivechatRooms.unsetTranscriptRequestedPdfById(details.rid); - const { rid } = await roomService.createDirectMessage({ to: details.userId, from: 'rocket.cat' }); this.log.info(`Transcript for room ${details.rid} by user ${details.userId} - Sending error message to user`); await messageService.sendMessage({ diff --git a/packages/core-typings/src/IRoom.ts b/packages/core-typings/src/IRoom.ts index ed9c373f200..8c4ed58013a 100644 --- a/packages/core-typings/src/IRoom.ts +++ b/packages/core-typings/src/IRoom.ts @@ -292,9 +292,6 @@ export interface IOmnichannelRoom extends IOmnichannelGenericRoom { slaId?: string; estimatedWaitingTimeQueue: IOmnichannelServiceLevelAgreements['dueTimeInMinutes']; // It should always have a default value for sorting mechanism to work - // Signals if the room already has a pdf transcript requested - // This prevents the user from requesting a transcript multiple times - pdfTranscriptRequested?: boolean; // The ID of the pdf file generated for the transcript // This will help if we want to have this file shown on other places of the UI pdfTranscriptFileId?: string; diff --git a/packages/model-typings/src/models/ILivechatRoomsModel.ts b/packages/model-typings/src/models/ILivechatRoomsModel.ts index d8cf90e346b..2d242e0e1e7 100644 --- a/packages/model-typings/src/models/ILivechatRoomsModel.ts +++ b/packages/model-typings/src/models/ILivechatRoomsModel.ts @@ -128,9 +128,6 @@ export interface ILivechatRoomsModel extends IBaseModel { setAutoTransferredAtById(roomId: string): Promise; findAvailableSources(): AggregationCursor; - - setTranscriptRequestedPdfById(rid: string): Promise; - unsetTranscriptRequestedPdfById(rid: string): Promise; setPdfTranscriptFileIdById(rid: string, fileId: string): Promise; setEmailTranscriptRequestedByRoomId( diff --git a/packages/models/src/models/LivechatRooms.ts b/packages/models/src/models/LivechatRooms.ts index 51a3a133659..0bc848947b1 100644 --- a/packages/models/src/models/LivechatRooms.ts +++ b/packages/models/src/models/LivechatRooms.ts @@ -68,7 +68,8 @@ export class LivechatRoomsRaw extends BaseRaw implements ILive }, }, { key: { 'livechatData.$**': 1 } }, - { key: { pdfTranscriptRequested: 1 }, sparse: true }, + // TODO: Remove index on next major + // { key: { pdfTranscriptRequested: 1 }, sparse: true }, { key: { pdfTranscriptFileId: 1 }, sparse: true }, // used on statistics { key: { callStatus: 1 }, sparse: true }, // used on statistics { key: { priorityId: 1 }, sparse: true }, @@ -1577,30 +1578,6 @@ export class LivechatRoomsRaw extends BaseRaw implements ILive ]); } - // These 3 methods shouldn't be here :( but current EE model has a meteor dependency - // And refactoring it could take time - setTranscriptRequestedPdfById(rid: string) { - return this.updateOne( - { - _id: rid, - }, - { - $set: { pdfTranscriptRequested: true }, - }, - ); - } - - unsetTranscriptRequestedPdfById(rid: string) { - return this.updateOne( - { - _id: rid, - }, - { - $unset: { pdfTranscriptRequested: 1 }, - }, - ); - } - setPdfTranscriptFileIdById(rid: string, fileId: string) { return this.updateOne( { @@ -2708,10 +2685,6 @@ export class LivechatRoomsRaw extends BaseRaw implements ILive throw new Error('Method not implemented.'); } - countRoomsWithPdfTranscriptRequested(): Promise { - throw new Error('Method not implemented.'); - } - countRoomsWithTranscriptSent(): Promise { throw new Error('Method not implemented.'); }