Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: Do not throw an error on custom fields validation when migrating visitors to contacts #34030

Merged
5 changes: 0 additions & 5 deletions apps/meteor/app/livechat/server/lib/contacts/ContactMerger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ export class ContactMerger {
customFieldsPerName.get(customField.type)?.push(customField);
}

const customFieldConflicts: CustomFieldAndValue[] = [];

for (const [key, customFields] of customFieldsPerName) {
const fieldName = key.replace('customFields.', '');

Expand All @@ -274,8 +272,6 @@ export class ContactMerger {
dataToSet[key] = first.value;
}
}

customFieldConflicts.push(...customFields);
}

const allConflicts: ILivechatContactConflictingField[] =
Expand All @@ -284,7 +280,6 @@ export class ContactMerger {
: [
...newNames.map((name): ILivechatContactConflictingField => ({ field: 'name', value: name })),
...newManagers.map((manager): ILivechatContactConflictingField => ({ field: 'manager', value: manager as string })),
...customFieldConflicts.map(({ type, value }): ILivechatContactConflictingField => ({ field: type, value })),
];

// Phones, Emails and Channels are simply added to the contact's existing list
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import sinon from 'sinon';
import type { CreateContactParams } from './createContact';

const getContactManagerIdByUsername = sinon.stub();
const getAllowedCustomFields = sinon.stub();

const { mapVisitorToContact } = proxyquire.noCallThru().load('./mapVisitorToContact', {
'./getContactManagerIdByUsername': {
getContactManagerIdByUsername,
},
'./getAllowedCustomFields': { getAllowedCustomFields },
});

const testDate = new Date();
Expand Down Expand Up @@ -140,6 +142,10 @@ const dataMap: [Partial<ILivechatVisitor>, IOmnichannelSource, CreateContactPara
{
_id: 'visitor1',
username: 'Username',
livechatData: {
customFieldId: 'customFieldValue',
invalidCustomFieldId: 'invalidCustomFieldValue',
},
activity: [],
},
{
Expand All @@ -166,7 +172,9 @@ const dataMap: [Partial<ILivechatVisitor>, IOmnichannelSource, CreateContactPara
},
},
],
customFields: undefined,
customFields: {
customFieldId: 'customFieldValue',
},
contactManager: undefined,
},
],
Expand All @@ -182,6 +190,7 @@ describe('mapVisitorToContact', () => {

return undefined;
});
getAllowedCustomFields.resolves([{ _id: 'customFieldId', label: 'custom-field-label' }]);
});

const index = 0;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import type { ILivechatVisitor, IOmnichannelSource } from '@rocket.chat/core-typings';

import type { CreateContactParams } from './createContact';
import { getAllowedCustomFields } from './getAllowedCustomFields';
import { getContactManagerIdByUsername } from './getContactManagerIdByUsername';
import { validateCustomFields } from './validateCustomFields';

export async function mapVisitorToContact(visitor: ILivechatVisitor, source: IOmnichannelSource): Promise<CreateContactParams> {
return {
Expand All @@ -25,7 +27,12 @@ export async function mapVisitorToContact(visitor: ILivechatVisitor, source: IOm
lastChat: visitor.lastChat,
},
],
customFields: visitor.livechatData,
customFields:
visitor.livechatData &&
validateCustomFields(await getAllowedCustomFields(), visitor.livechatData, {
ignoreAdditionalFields: true,
ignoreValidationErrors: true,
}),
lastChat: visitor.lastChat,
contactManager: visitor.contactManager?.username && (await getContactManagerIdByUsername(visitor.contactManager.username)),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ describe('validateCustomFields', () => {
expect(() => validateCustomFields(mockCustomFields, {})).to.throw();
});

it('should not throw an error if a required custom field is missing, but the ignoreValidationErrors option is provided', () => {
expect(() => validateCustomFields(mockCustomFields, {}, { ignoreValidationErrors: true }))
.not.to.throw()
.and.to.equal({});
});

it('should NOT throw an error when a non-required custom field is missing', () => {
const allowedCustomFields = [{ _id: 'field1', label: 'Field 1', required: false }];
const customFields = {};
Expand All @@ -25,6 +31,12 @@ describe('validateCustomFields', () => {
expect(() => validateCustomFields(mockCustomFields, { cf1: 'invalid' })).to.throw();
});

it('should not throw an error if a custom field value does not match the regexp, but the ignoreValidationErrors option is provided', () => {
expect(() => validateCustomFields(mockCustomFields, { cf1: 'invalid' }, { ignoreValidationErrors: true }))
.not.to.throw()
.and.to.equal({});
});

it('should handle an empty customFields input without throwing an error', () => {
const allowedCustomFields = [{ _id: 'field1', label: 'Field 1', required: false }];
const customFields = {};
Expand All @@ -38,4 +50,13 @@ describe('validateCustomFields', () => {

expect(() => validateCustomFields(allowedCustomFields, customFields)).to.throw();
});

it('should not throw an error if a extra custom field is passed, but the ignoreValidationErrors option is provided', () => {
const allowedCustomFields = [{ _id: 'field1', label: 'Field 1', required: false }];
const customFields = { field2: 'value' };

expect(() => validateCustomFields(allowedCustomFields, customFields, { ignoreValidationErrors: true }))
.not.to.throw()
.and.to.equal({});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,24 @@ import { i18n } from '../../../../utils/lib/i18n';
export function validateCustomFields(
allowedCustomFields: AtLeast<ILivechatCustomField, '_id' | 'label' | 'regexp' | 'required'>[],
customFields: Record<string, string | unknown>,
options?: { ignoreAdditionalFields?: boolean },
{
ignoreAdditionalFields = false,
ignoreValidationErrors = false,
}: { ignoreAdditionalFields?: boolean; ignoreValidationErrors?: boolean } = {},
): Record<string, string> {
const validValues: Record<string, string> = {};

for (const cf of allowedCustomFields) {
if (!customFields.hasOwnProperty(cf._id)) {
if (cf.required) {
if (cf.required && !ignoreValidationErrors) {
throw new Error(i18n.t('error-invalid-custom-field-value', { field: cf.label }));
}
continue;
}
const cfValue: string = trim(customFields[cf._id]);

if (!cfValue || typeof cfValue !== 'string') {
if (cf.required) {
if (cf.required && !ignoreValidationErrors) {
throw new Error(i18n.t('error-invalid-custom-field-value', { field: cf.label }));
}
continue;
Expand All @@ -29,14 +32,18 @@ export function validateCustomFields(
if (cf.regexp) {
const regex = new RegExp(cf.regexp);
if (!regex.test(cfValue)) {
if (ignoreValidationErrors) {
continue;
}

throw new Error(i18n.t('error-invalid-custom-field-value', { field: cf.label }));
}
}

validValues[cf._id] = cfValue;
}

if (!options?.ignoreAdditionalFields) {
if (!ignoreAdditionalFields) {
const allowedCustomFieldIds = new Set(allowedCustomFields.map((cf) => cf._id));
for (const key in customFields) {
if (!allowedCustomFieldIds.has(key)) {
Expand Down
Loading