From a33b2097535ec945e33e8f23b901ff4d2bab9987 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com> Date: Mon, 2 Dec 2024 14:07:47 -0300 Subject: [PATCH] regression: old omnichannel rooms only being migrated after their data is loaded by the client (#34090) --- .../server/lib/maybeMigrateLivechatRoom.ts | 12 ++-- apps/meteor/app/api/server/v1/rooms.ts | 5 +- .../app/livechat/server/api/v1/contact.ts | 7 +- .../lib/contacts/getContactByChannel.ts | 30 -------- apps/meteor/app/livechat/server/startup.ts | 3 +- .../tests/end-to-end/api/livechat/contacts.ts | 71 ++----------------- packages/rest-typings/src/v1/omnichannel.ts | 30 +++----- 7 files changed, 28 insertions(+), 130 deletions(-) delete mode 100644 apps/meteor/app/livechat/server/lib/contacts/getContactByChannel.ts diff --git a/apps/meteor/app/api/server/lib/maybeMigrateLivechatRoom.ts b/apps/meteor/app/api/server/lib/maybeMigrateLivechatRoom.ts index ac5b2cd866ca..3a66cef2e416 100644 --- a/apps/meteor/app/api/server/lib/maybeMigrateLivechatRoom.ts +++ b/apps/meteor/app/api/server/lib/maybeMigrateLivechatRoom.ts @@ -1,5 +1,6 @@ -import { isOmnichannelRoom, type IRoom } from '@rocket.chat/core-typings'; -import { Rooms } from '@rocket.chat/models'; +import { isOmnichannelRoom } from '@rocket.chat/core-typings'; +import type { IOmnichannelRoom, IRoom } from '@rocket.chat/core-typings'; +import { LivechatRooms } from '@rocket.chat/models'; import type { FindOptions } from 'mongodb'; import { projectionAllowsAttribute } from './projectionAllowsAttribute'; @@ -9,7 +10,10 @@ import { migrateVisitorIfMissingContact } from '../../../livechat/server/lib/con * If the room is a livechat room and it doesn't yet have a contact, trigger the migration for its visitor and source * The migration will create/use a contact and assign it to every room that matches this visitorId and source. **/ -export async function maybeMigrateLivechatRoom(room: IRoom | null, options: FindOptions = {}): Promise { +export async function maybeMigrateLivechatRoom( + room: IOmnichannelRoom | null, + options: FindOptions = {}, +): Promise { if (!room || !isOmnichannelRoom(room)) { return room; } @@ -32,5 +36,5 @@ export async function maybeMigrateLivechatRoom(room: IRoom | null, options: Find } // Load the room again with the same options so it can be reloaded with the contactId in place - return Rooms.findOneById(room._id, options); + return LivechatRooms.findOneById(room._id, options); } diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index e8a152db914a..50bc12cab618 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -32,7 +32,6 @@ import { composeRoomWithLastMessage } from '../helpers/composeRoomWithLastMessag import { getPaginationItems } from '../helpers/getPaginationItems'; import { getUserFromParams } from '../helpers/getUserFromParams'; import { getUploadFormData } from '../lib/getUploadFormData'; -import { maybeMigrateLivechatRoom } from '../lib/maybeMigrateLivechatRoom'; import { findAdminRoom, findAdminRooms, @@ -442,10 +441,8 @@ API.v1.addRoute( const { team, parentRoom } = await Team.getRoomInfo(room); const parent = discussionParent || parentRoom; - const options = { projection: fields }; - return API.v1.success({ - room: (await maybeMigrateLivechatRoom(await Rooms.findOneByIdOrName(room._id, options), options)) ?? undefined, + room: await Rooms.findOneByIdOrName(room._id, { projection: fields }), ...(team && { team }), ...(parent && { parent }), }); diff --git a/apps/meteor/app/livechat/server/api/v1/contact.ts b/apps/meteor/app/livechat/server/api/v1/contact.ts index 0baa5584a243..019ea07252dc 100644 --- a/apps/meteor/app/livechat/server/api/v1/contact.ts +++ b/apps/meteor/app/livechat/server/api/v1/contact.ts @@ -14,7 +14,6 @@ import { Meteor } from 'meteor/meteor'; import { API } from '../../../../api/server'; import { getPaginationItems } from '../../../../api/server/helpers/getPaginationItems'; import { createContact } from '../../lib/contacts/createContact'; -import { getContactByChannel } from '../../lib/contacts/getContactByChannel'; import { getContactChannelsGrouped } from '../../lib/contacts/getContactChannelsGrouped'; import { getContactHistory } from '../../lib/contacts/getContactHistory'; import { getContacts } from '../../lib/contacts/getContacts'; @@ -133,13 +132,13 @@ API.v1.addRoute( { authRequired: true, permissionsRequired: ['view-livechat-contact'], validateParams: isGETOmnichannelContactsProps }, { async get() { - const { contactId, visitor } = this.queryParams; + const { contactId } = this.queryParams; - if (!contactId && !visitor) { + if (!contactId) { return API.v1.notFound(); } - const contact = await (contactId ? LivechatContacts.findOneById(contactId) : getContactByChannel(visitor)); + const contact = await LivechatContacts.findOneById(contactId); if (!contact) { return API.v1.notFound(); diff --git a/apps/meteor/app/livechat/server/lib/contacts/getContactByChannel.ts b/apps/meteor/app/livechat/server/lib/contacts/getContactByChannel.ts deleted file mode 100644 index 052c28206047..000000000000 --- a/apps/meteor/app/livechat/server/lib/contacts/getContactByChannel.ts +++ /dev/null @@ -1,30 +0,0 @@ -import type { ILivechatContact, ILivechatContactVisitorAssociation } from '@rocket.chat/core-typings'; -import { LivechatContacts, LivechatVisitors } from '@rocket.chat/models'; - -import { migrateVisitorToContactId } from './migrateVisitorToContactId'; - -export async function getContactByChannel(association: ILivechatContactVisitorAssociation): Promise { - // If a contact already exists for that visitor, return it - const linkedContact = await LivechatContacts.findOneByVisitor(association); - if (linkedContact) { - return linkedContact; - } - - // If the contact was not found, Load the visitor data so we can migrate it - const visitor = await LivechatVisitors.findOneById(association.visitorId); - - // If there is no visitor data, there's nothing we can do - if (!visitor) { - return null; - } - - const newContactId = await migrateVisitorToContactId({ visitor, source: association.source }); - - // If no contact was created by the migration, this visitor doesn't need a contact yet, so let's return null - if (!newContactId) { - return null; - } - - // Finally, let's return the data of the migrated contact - return LivechatContacts.findOneById(newContactId); -} diff --git a/apps/meteor/app/livechat/server/startup.ts b/apps/meteor/app/livechat/server/startup.ts index 5c02587f17e6..b41fc425bf06 100644 --- a/apps/meteor/app/livechat/server/startup.ts +++ b/apps/meteor/app/livechat/server/startup.ts @@ -13,6 +13,7 @@ import { callbacks } from '../../../lib/callbacks'; import { beforeLeaveRoomCallback } from '../../../lib/callbacks/beforeLeaveRoomCallback'; import { i18n } from '../../../server/lib/i18n'; import { roomCoordinator } from '../../../server/lib/rooms/roomCoordinator'; +import { maybeMigrateLivechatRoom } from '../../api/server/lib/maybeMigrateLivechatRoom'; import { hasPermissionAsync } from '../../authorization/server/functions/hasPermission'; import { notifyOnUserChange } from '../../lib/server/lib/notifyListener'; import { settings } from '../../settings/server'; @@ -21,7 +22,7 @@ import './roomAccessValidator.internalService'; const logger = new Logger('LivechatStartup'); Meteor.startup(async () => { - roomCoordinator.setRoomFind('l', (_id) => LivechatRooms.findOneById(_id)); + roomCoordinator.setRoomFind('l', async (id) => maybeMigrateLivechatRoom(await LivechatRooms.findOneById(id))); beforeLeaveRoomCallback.add( (user, room) => { diff --git a/apps/meteor/tests/end-to-end/api/livechat/contacts.ts b/apps/meteor/tests/end-to-end/api/livechat/contacts.ts index daaffb1f9eb3..4095e7df8b23 100644 --- a/apps/meteor/tests/end-to-end/api/livechat/contacts.ts +++ b/apps/meteor/tests/end-to-end/api/livechat/contacts.ts @@ -710,7 +710,6 @@ describe('LIVECHAT - contacts', () => { describe('[GET] omnichannel/contacts.get', () => { let contactId: string; let contactId2: string; - let association: ILivechatContactVisitorAssociation; const email = faker.internet.email().toLowerCase(); const phone = faker.phone.number(); @@ -743,14 +742,7 @@ describe('LIVECHAT - contacts', () => { const visitor = await createVisitor(undefined, contact.name, email, phone); - const room = await createLivechatRoom(visitor.token); - association = { - visitorId: visitor._id, - source: { - type: room.source.type, - id: room.source.id, - }, - }; + await createLivechatRoom(visitor.token); }); after(async () => { @@ -774,23 +766,6 @@ describe('LIVECHAT - contacts', () => { expect(res.body.contact.contactManager).to.be.equal(contact.contactManager); }); - it('should be able get a contact by visitor association', async () => { - const res = await request.get(api(`omnichannel/contacts.get`)).set(credentials).query({ visitor: association }); - - expect(res.status).to.be.equal(200); - expect(res.body).to.have.property('success', true); - expect(res.body.contact).to.have.property('createdAt'); - expect(res.body.contact._id).to.be.equal(contactId); - expect(res.body.contact.name).to.be.equal(contact.name); - expect(res.body.contact.emails).to.be.deep.equal([ - { - address: contact.emails[0], - }, - ]); - expect(res.body.contact.phones).to.be.deep.equal([{ phoneNumber: contact.phones[0] }]); - expect(res.body.contact.contactManager).to.be.equal(contact.contactManager); - }); - it('should return 404 if contact does not exist using contactId', async () => { const res = await request.get(api(`omnichannel/contacts.get`)).set(credentials).query({ contactId: 'invalid' }); @@ -799,28 +774,6 @@ describe('LIVECHAT - contacts', () => { expect(res.body).to.have.property('error', 'Resource not found'); }); - it('should return 404 if contact does not exist using visitor association', async () => { - const res = await request - .get(api(`omnichannel/contacts.get`)) - .set(credentials) - .query({ visitor: { ...association, visitorId: 'invalidId' } }); - - expect(res.status).to.be.equal(404); - expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', 'Resource not found'); - }); - - it('should return 404 if contact does not exist using visitor source', async () => { - const res = await request - .get(api(`omnichannel/contacts.get`)) - .set(credentials) - .query({ visitor: { ...association, source: { type: 'email' } } }); - - expect(res.status).to.be.equal(404); - expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', 'Resource not found'); - }); - it("should return an error if user doesn't have 'view-livechat-contact' permission", async () => { await removePermissionFromAllRoles('view-livechat-contact'); @@ -835,21 +788,7 @@ describe('LIVECHAT - contacts', () => { it('should return an error if contactId and visitor association is missing', async () => { const res = await request.get(api(`omnichannel/contacts.get`)).set(credentials); - expectInvalidParams(res, [ - "must have required property 'contactId'", - "must have required property 'visitor'", - 'must match exactly one schema in oneOf [invalid-params]', - ]); - }); - - it('should return an error if more than one field is provided', async () => { - const res = await request.get(api(`omnichannel/contacts.get`)).set(credentials).query({ contactId, visitor: association }); - - expectInvalidParams(res, [ - 'must NOT have additional properties', - 'must NOT have additional properties', - 'must match exactly one schema in oneOf [invalid-params]', - ]); + expectInvalidParams(res, ["must have required property 'contactId' [invalid-params]"]); }); describe('Contact Channels', () => { @@ -1258,7 +1197,7 @@ describe('LIVECHAT - contacts', () => { expect(res.status).to.be.equal(200); expect(res.body).to.have.property('success', true); - const { body } = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ visitor: association }); + const { body } = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ contactId: room.contactId }); expect(body.contact.channels).to.be.an('array'); expect(body.contact.channels.length).to.be.equal(1); @@ -1388,7 +1327,7 @@ describe('LIVECHAT - contacts', () => { it('should be able to unblock a contact channel', async () => { await request.post(api('omnichannel/contacts.block')).set(credentials).send({ visitor: association }); - const { body } = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ visitor: association }); + const { body } = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ contactId: room.contactId }); expect(body.contact.channels).to.be.an('array'); expect(body.contact.channels.length).to.be.equal(1); @@ -1399,7 +1338,7 @@ describe('LIVECHAT - contacts', () => { expect(res.status).to.be.equal(200); expect(res.body).to.have.property('success', true); - const { body: body2 } = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ visitor: association }); + const { body: body2 } = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ contactId: room.contactId }); expect(body2.contact.channels).to.be.an('array'); expect(body2.contact.channels.length).to.be.equal(1); diff --git a/packages/rest-typings/src/v1/omnichannel.ts b/packages/rest-typings/src/v1/omnichannel.ts index 37a259187b66..2cd33e6cf8fb 100644 --- a/packages/rest-typings/src/v1/omnichannel.ts +++ b/packages/rest-typings/src/v1/omnichannel.ts @@ -1362,28 +1362,16 @@ export const ContactVisitorAssociationSchema = { }; const GETOmnichannelContactsSchema = { - oneOf: [ - { - type: 'object', - properties: { - contactId: { - type: 'string', - nullable: false, - isNotEmpty: true, - }, - }, - required: ['contactId'], - additionalProperties: false, - }, - { - type: 'object', - properties: { - visitor: ContactVisitorAssociationSchema, - }, - required: ['visitor'], - additionalProperties: false, + type: 'object', + properties: { + contactId: { + type: 'string', + nullable: false, + isNotEmpty: true, }, - ], + }, + required: ['contactId'], + additionalProperties: false, }; export const isGETOmnichannelContactsProps = ajv.compile(GETOmnichannelContactsSchema);