fix: unable to clear contact manager field

.changeset/thick-ties-hunt.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/models': patch
+'@rocket.chat/meteor': patch
+---
+
+Fixes endpoint `omnichannel/contacts.update` and `omnichannel/contacts.conflicts` where the contact manager field could not be cleared.

apps/meteor/app/livechat/server/lib/contacts/patchContact.ts

@@ -0,0 +1,14 @@
+import type { ILivechatContact } from '@rocket.chat/core-typings';
+import { LivechatContacts } from '@rocket.chat/models';
+
+export const patchContact = async (
+	contactId: ILivechatContact['_id'],
+	data: { set?: Partial<ILivechatContact>; unset?: Array<keyof ILivechatContact> },
+): Promise<ILivechatContact | null> => {
+	const { set = {}, unset = [] } = data;
+
+	if (Object.keys(set).length === 0 && unset.length === 0) {
+		return LivechatContacts.findOneEnabledById(contactId);
+	}
+	return LivechatContacts.patchContact(contactId, data);
+};

apps/meteor/app/livechat/server/lib/contacts/resolveContactConflicts.spec.ts

@@ -6,7 +6,7 @@ import sinon from 'sinon';
 const modelsMock = {
 	LivechatContacts: {
 		findOneEnabledById: sinon.stub(),
-		updateContact: sinon.stub(),
+		patchContact: sinon.stub(),
 	},
 	Settings: {
 		incrementValueById: sinon.stub(),
@@ -15,18 +15,26 @@ const modelsMock = {
 
 const validateContactManagerMock = sinon.stub();
 
+const { patchContact } = proxyquire.noCallThru().load('./patchContact.ts', {
+	'@rocket.chat/models': modelsMock,
+});
+
 const { resolveContactConflicts } = proxyquire.noCallThru().load('./resolveContactConflicts', {
 	'@rocket.chat/models': modelsMock,
 	'./validateContactManager': {
 		validateContactManager: validateContactManagerMock,
 	},
+	'./patchContact': {
+		patchContact,
+	},
 });
 
 describe('resolveContactConflicts', () => {
 	beforeEach(() => {
 		modelsMock.LivechatContacts.findOneEnabledById.reset();
 		modelsMock.Settings.incrementValueById.reset();
-		modelsMock.LivechatContacts.updateContact.reset();
+		modelsMock.LivechatContacts.patchContact.reset();
+		validateContactManagerMock.reset();
 	});
 
 	it('should update the contact with the resolved custom field', async () => {
@@ -36,14 +44,22 @@ describe('resolveContactConflicts', () => {
 			conflictingFields: [{ field: 'customFields.customField', value: 'oldValue' }],
 		});
 		modelsMock.Settings.incrementValueById.resolves(1);
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			customFields: { customField: 'newestValue' },
+			conflictingFields: [],
+		} as Partial<ILivechatContact>);
 
 		await resolveContactConflicts({ contactId: 'contactId', customFields: { customField: 'newestValue' } });
 
 		expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
 
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[0]).to.be.equal('contactId');
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[1]).to.be.deep.contain({
-			customFields: { customField: 'newestValue' },
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.be.deep.contain({
+			set: {
+				customFields: { customField: 'newestValue' },
+				conflictingFields: [],
+			},
 		});
 	});
 
@@ -55,13 +71,24 @@ describe('resolveContactConflicts', () => {
 			conflictingFields: [{ field: 'name', value: 'Old Name' }],
 		});
 		modelsMock.Settings.incrementValueById.resolves(1);
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			name: 'New Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [],
+		} as Partial<ILivechatContact>);
 
 		await resolveContactConflicts({ contactId: 'contactId', name: 'New Name' });
 
 		expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
 
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[0]).to.be.equal('contactId');
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[1]).to.be.deep.contain({ name: 'New Name' });
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.be.deep.contain({
+			set: {
+				name: 'New Name',
+				conflictingFields: [],
+			},
+		});
 	});
 
 	it('should update the contact with the resolved contact manager', async () => {
@@ -72,97 +99,115 @@ describe('resolveContactConflicts', () => {
 			customFields: { customField: 'value' },
 			conflictingFields: [{ field: 'manager', value: 'oldManagerId' }],
 		});
+		validateContactManagerMock.resolves();
+		modelsMock.Settings.incrementValueById.resolves(1);
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			contactManager: 'newContactManagerId',
+			customFields: { customField: 'value' },
+			conflictingFields: [],
+		} as Partial<ILivechatContact>);
 
-		await resolveContactConflicts({ contactId: 'contactId', name: 'New Name', customFields: { manager: 'newContactManagerId' } });
+		await resolveContactConflicts({ contactId: 'contactId', contactManager: 'newContactManagerId' });
 
 		expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
+		expect(validateContactManagerMock.getCall(0).args[0]).to.be.equal('newContactManagerId');
 
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[0]).to.be.equal('contactId');
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[1]).to.be.deep.contain({
-			customFields: { customField: 'value', manager: 'newContactManagerId' },
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.be.deep.contain({
+			set: {
+				contactManager: 'newContactManagerId',
+				conflictingFields: [],
+			},
 		});
 	});
 
-	it('should wipe conflicts if wipeConflicts = true', async () => {
-		it('should update the contact with the resolved name', async () => {
-			modelsMock.LivechatContacts.findOneEnabledById.resolves({
-				_id: 'contactId',
-				name: 'Name',
-				customFields: { customField: 'newValue' },
-				conflictingFields: [
-					{ field: 'name', value: 'NameTest' },
-					{ field: 'customFields.customField', value: 'value' },
-				],
-			});
-			modelsMock.Settings.incrementValueById.resolves(2);
-			modelsMock.LivechatContacts.updateContact.resolves({
-				_id: 'contactId',
-				name: 'New Name',
-				customField: { customField: 'newValue' },
-				conflictingFields: [],
-			} as Partial<ILivechatContact>);
+	it('should wipe all conflicts if wipeConflicts = true', async () => {
+		modelsMock.LivechatContacts.findOneEnabledById.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [
+				{ field: 'name', value: 'NameTest' },
+				{ field: 'customFields.customField', value: 'value' },
+			],
+		});
+		modelsMock.Settings.incrementValueById.resolves({ _id: 'Resolved_Conflicts_Count', value: 2 });
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			name: 'New Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [],
+		} as Partial<ILivechatContact>);
 
-			const result = await resolveContactConflicts({ contactId: 'contactId', name: 'New Name', wipeConflicts: true });
+		const result = await resolveContactConflicts({ contactId: 'contactId', name: 'New Name', wipeConflicts: true });
 
-			expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
 
-			expect(modelsMock.Settings.incrementValueById.getCall(0).args[0]).to.be.equal('Livechat_conflicting_fields_counter');
-			expect(modelsMock.Settings.incrementValueById.getCall(0).args[1]).to.be.equal(2);
+		expect(modelsMock.Settings.incrementValueById.getCall(0).args[0]).to.be.equal('Resolved_Conflicts_Count');
+		expect(modelsMock.Settings.incrementValueById.getCall(0).args[1]).to.be.equal(2);
 
-			expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[0]).to.be.equal('contactId');
-			expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[1]).to.be.deep.contain({ name: 'New Name' });
-			expect(result).to.be.deep.equal({
-				_id: 'contactId',
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.be.deep.contain({
+			set: {
 				name: 'New Name',
-				customField: { customField: 'newValue' },
 				conflictingFields: [],
-			});
+			},
+		});
+		expect(result).to.be.deep.equal({
+			_id: 'contactId',
+			name: 'New Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [],
 		});
 	});
 
-	it('should wipe conflicts if wipeConflicts = true', async () => {
-		it('should update the contact with the resolved name', async () => {
-			modelsMock.LivechatContacts.findOneEnabledById.resolves({
-				_id: 'contactId',
-				name: 'Name',
-				customFields: { customField: 'newValue' },
-				conflictingFields: [
-					{ field: 'name', value: 'NameTest' },
-					{ field: 'customFields.customField', value: 'value' },
-				],
-			});
-			modelsMock.Settings.incrementValueById.resolves(2);
-			modelsMock.LivechatContacts.updateContact.resolves({
-				_id: 'contactId',
-				name: 'New Name',
-				customField: { customField: 'newValue' },
-				conflictingFields: [],
-			} as Partial<ILivechatContact>);
+	it('should only resolve specified conflicts when wipeConflicts = false', async () => {
+		modelsMock.LivechatContacts.findOneEnabledById.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [
+				{ field: 'name', value: 'NameTest' },
+				{ field: 'customFields.customField', value: 'value' },
+			],
+		});
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			name: 'New Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [{ field: 'customFields.customField', value: 'value' }],
+		} as Partial<ILivechatContact>);
 
-			const result = await resolveContactConflicts({ contactId: 'contactId', name: 'New Name', wipeConflicts: false });
+		const result = await resolveContactConflicts({ contactId: 'contactId', name: 'New Name', wipeConflicts: false });
 
-			expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.findOneEnabledById.getCall(0).args[0]).to.be.equal('contactId');
 
-			expect(modelsMock.Settings.incrementValueById.getCall(0).args[0]).to.be.equal('Livechat_conflicting_fields_counter');
-			expect(modelsMock.Settings.incrementValueById.getCall(0).args[1]).to.be.equal(1);
+		// When wipeConflicts is false, incrementValueById should NOT be called
+		expect(modelsMock.Settings.incrementValueById.called).to.be.false;
 
-			expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[0]).to.be.equal('contactId');
-			expect(modelsMock.LivechatContacts.updateContact.getCall(0).args[1]).to.be.deep.contain({ name: 'New Name' });
-			expect(result).to.be.deep.equal({
-				_id: 'contactId',
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[0]).to.be.equal('contactId');
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.be.deep.contain({
+			set: {
 				name: 'New Name',
-				customField: { customField: 'newValue' },
 				conflictingFields: [{ field: 'customFields.customField', value: 'value' }],
-			});
+			},
+		});
+		expect(result).to.be.deep.equal({
+			_id: 'contactId',
+			name: 'New Name',
+			customFields: { customField: 'newValue' },
+			conflictingFields: [{ field: 'customFields.customField', value: 'value' }],
 		});
 	});
 
 	it('should throw an error if the contact does not exist', async () => {
 		modelsMock.LivechatContacts.findOneEnabledById.resolves(undefined);
-		await expect(resolveContactConflicts({ contactId: 'id', customField: { customField: 'newValue' } })).to.be.rejectedWith(
+		await expect(resolveContactConflicts({ contactId: 'id', customFields: { customField: 'newValue' } })).to.be.rejectedWith(
 			'error-contact-not-found',
 		);
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0)).to.be.null;
+		expect(modelsMock.LivechatContacts.patchContact.called).to.be.false;
 	});
 
 	it('should throw an error if the contact has no conflicting fields', async () => {
@@ -173,9 +218,61 @@ describe('resolveContactConflicts', () => {
 			customFields: { customField: 'value' },
 			conflictingFields: [],
 		});
-		await expect(resolveContactConflicts({ contactId: 'id', customField: { customField: 'newValue' } })).to.be.rejectedWith(
+		await expect(resolveContactConflicts({ contactId: 'id', customFields: { customField: 'newValue' } })).to.be.rejectedWith(
 			'error-contact-has-no-conflicts',
 		);
-		expect(modelsMock.LivechatContacts.updateContact.getCall(0)).to.be.null;
+		expect(modelsMock.LivechatContacts.patchContact.called).to.be.false;
+	});
+
+	it('should unset contactManager when explicitly set to empty string', async () => {
+		modelsMock.LivechatContacts.findOneEnabledById.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			contactManager: 'oldManagerId',
+			customFields: { customField: 'value' },
+			conflictingFields: [{ field: 'manager', value: 'oldManagerId' }],
+		});
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			customFields: { customField: 'value' },
+			conflictingFields: [],
+		} as Partial<ILivechatContact>);
+
+		await resolveContactConflicts({ contactId: 'contactId', contactManager: '' });
+
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.deep.include({
+			set: {
+				conflictingFields: [],
+			},
+			unset: ['contactManager'],
+		});
+		expect(validateContactManagerMock.called).to.be.false;
+	});
+
+	it('should unset contactManager when explicitly set to undefined', async () => {
+		modelsMock.LivechatContacts.findOneEnabledById.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			contactManager: 'oldManagerId',
+			customFields: { customField: 'value' },
+			conflictingFields: [{ field: 'manager', value: 'oldManagerId' }],
+		});
+		modelsMock.LivechatContacts.patchContact.resolves({
+			_id: 'contactId',
+			name: 'Name',
+			customFields: { customField: 'value' },
+			conflictingFields: [],
+		} as Partial<ILivechatContact>);
+
+		await resolveContactConflicts({ contactId: 'contactId', contactManager: undefined });
+
+		expect(modelsMock.LivechatContacts.patchContact.getCall(0).args[1]).to.deep.include({
+			set: {
+				conflictingFields: [],
+			},
+			unset: ['contactManager'],
+		});
+		expect(validateContactManagerMock.called).to.be.false;
 	});
 });

apps/meteor/app/livechat/server/lib/contacts/resolveContactConflicts.ts

@@ -1,6 +1,7 @@
 import type { ILivechatContact, ILivechatContactConflictingField } from '@rocket.chat/core-typings';
 import { LivechatContacts, Settings } from '@rocket.chat/models';
 
+import { patchContact } from './patchContact';
 import { validateContactManager } from './validateContactManager';
 import { notifyOnSettingChanged } from '../../../../lib/server/lib/notifyListener';
 
@@ -46,7 +47,7 @@ export async function resolveContactConflicts(params: ResolveContactConflictsPar
 		const fieldsToRemove = new Set<string>(
 			[
 				name && 'name',
-				contactManager && 'manager',
+				'contactManager' in params && 'manager',
 				...(customFields ? Object.keys(customFields).map((key) => `customFields.${key}`) : []),
 			].filter((field): field is string => !!field),
 		);
@@ -56,12 +57,20 @@ export async function resolveContactConflicts(params: ResolveContactConflictsPar
 		) as ILivechatContactConflictingField[];
 	}
 
-	const dataToUpdate = {
+	const set = {
 		...(name && { name }),
 		...(contactManager && { contactManager }),
 		...(customFields && { customFields: { ...contact.customFields, ...customFields } }),
 		conflictingFields: updatedConflictingFieldsArr,
 	};
 
-	return LivechatContacts.updateContact(contactId, dataToUpdate);
+	const unset: (keyof ILivechatContact)[] = 'contactManager' in params && !contactManager ? ['contactManager'] : [];
+
+	const updatedContact = await patchContact(contactId, { set, unset });
+
+	if (!updatedContact) {
+		throw new Error('error-contact-not-found');
+	}
+
+	return updatedContact;
 }