diff --git a/.changeset/wild-kiwis-cover.md b/.changeset/wild-kiwis-cover.md new file mode 100644 index 0000000000000..8bb6afeb72917 --- /dev/null +++ b/.changeset/wild-kiwis-cover.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixes a bug where the `/api/v1/users.update` API call was replacing the entire `customFields` object instead of merging only the specified properties. The fix ensures that when updating custom fields, existing values are preserved while only specified fields are updated or added. \ No newline at end of file diff --git a/apps/meteor/app/lib/server/functions/saveCustomFieldsWithoutValidation.ts b/apps/meteor/app/lib/server/functions/saveCustomFieldsWithoutValidation.ts index e3b19b01cfc16..f99faf9aa795b 100644 --- a/apps/meteor/app/lib/server/functions/saveCustomFieldsWithoutValidation.ts +++ b/apps/meteor/app/lib/server/functions/saveCustomFieldsWithoutValidation.ts @@ -32,22 +32,20 @@ export const saveCustomFieldsWithoutValidation = async function ( // configured custom fields in setting const customFieldsMeta = getCustomFieldsMeta(customFieldsSetting); - const customFields: Record = Object.keys(customFieldsMeta).reduce( - (acc, currentValue) => ({ - ...acc, - [currentValue]: formData[currentValue], - }), - {}, + const customFields = Object.fromEntries( + Object.keys(formData) + .filter((key) => Object.hasOwn(customFieldsMeta, key)) + .map((key) => [key, formData[key]]), ); const { _updater, session } = options || {}; const updater = _updater || Users.getUpdater(); - - updater.set('customFields', customFields); - // add modified records to updater Object.keys(customFields).forEach((fieldName) => { + // @ts-expect-error TODO `Updater.set` does not support `customFields.${fieldName}` syntax + updater.set(`customFields.${fieldName}`, customFields[fieldName]); + if (!customFieldsMeta[fieldName].modifyRecordField) { return; } diff --git a/apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts b/apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts new file mode 100644 index 0000000000000..833acc1b799dd --- /dev/null +++ b/apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts @@ -0,0 +1,128 @@ +import { Users } from './fixtures/userStates'; +import { HomeChannel, Admin } from './page-objects'; +import { test, expect } from './utils/test'; +import { createTestUser, type ITestUser } from './utils/user-helpers'; + +const customFieldInitial1 = 'initial1'; +const adminCustomFieldValue1 = 'admin_value1'; +const adminCustomFieldValue2 = 'admin_value2'; +const adminCustomFieldUpdated1 = 'updated_admin1'; + +test.describe('Admin users custom fields', () => { + let poHomeChannel: HomeChannel; + let poAdmin: Admin; + let addTestUser: ITestUser; + let updateTestUser: ITestUser; + + test.use({ storageState: Users.admin.state }); + + test.beforeAll(async ({ api }) => { + await api.post('/settings/Accounts_CustomFields', { + value: JSON.stringify({ + customFieldText1: { + type: 'text', + required: false, + }, + customFieldText2: { + type: 'text', + required: false, + }, + }), + }); + + [addTestUser, updateTestUser] = await Promise.all([ + createTestUser(api), + createTestUser(api, { + data: { + customFields: { + customFieldText1: customFieldInitial1, + customFieldText2: adminCustomFieldValue2, + }, + }, + }), + ]); + }); + + test.afterAll(async ({ api }) => { + await Promise.all([ + api.post('/settings/Accounts_CustomFields', { + value: '', + }), + addTestUser.delete(), + updateTestUser.delete(), + ]); + }); + + test.beforeEach(async ({ page }) => { + poHomeChannel = new HomeChannel(page); + poAdmin = new Admin(page); + await page.goto('/admin/users'); + }); + + test('should allow admin to add user custom fields', async () => { + await test.step('should find and click on add test user', async () => { + await poAdmin.inputSearchUsers.fill(addTestUser.data.username); + + await expect(poAdmin.getUserRowByUsername(addTestUser.data.username)).toBeVisible(); + await poAdmin.getUserRowByUsername(addTestUser.data.username).click(); + }); + + await test.step('should navigate to edit user form', async () => { + await poAdmin.btnEdit.click(); + }); + + await test.step('should fill custom fields for user', async () => { + await poAdmin.tabs.users.getCustomField('customFieldText1').fill(adminCustomFieldValue1); + await poAdmin.tabs.users.getCustomField('customFieldText2').fill(adminCustomFieldValue2); + }); + + await test.step('should save user custom fields', async () => { + await poAdmin.tabs.users.btnSaveUser.click(); + await poHomeChannel.dismissToast(); + }); + + await test.step('should verify custom fields were saved', async () => { + await poAdmin.tabs.users.btnContextualbarClose.click(); + await poAdmin.getUserRowByUsername(addTestUser.data.username).click(); + await poAdmin.btnEdit.click(); + + await expect(poAdmin.tabs.users.getCustomField('customFieldText1')).toHaveValue(adminCustomFieldValue1); + await expect(poAdmin.tabs.users.getCustomField('customFieldText2')).toHaveValue(adminCustomFieldValue2); + }); + }); + + test('should allow admin to update existing user custom fields', async () => { + await test.step('should find and click on update test user', async () => { + await poAdmin.inputSearchUsers.fill(updateTestUser.data.username); + + await expect(poAdmin.getUserRowByUsername(updateTestUser.data.username)).toBeVisible(); + await poAdmin.getUserRowByUsername(updateTestUser.data.username).click(); + }); + + await test.step('should navigate to edit user form', async () => { + await poAdmin.btnEdit.click(); + }); + + await test.step('should verify existing values and update one custom field', async () => { + await poAdmin.tabs.users.inputName.waitFor(); + + await expect(poAdmin.tabs.users.getCustomField('customFieldText1')).toHaveValue(customFieldInitial1); + await expect(poAdmin.tabs.users.getCustomField('customFieldText2')).toHaveValue(adminCustomFieldValue2); + + await poAdmin.tabs.users.getCustomField('customFieldText1').clear(); + await poAdmin.tabs.users.getCustomField('customFieldText1').fill(adminCustomFieldUpdated1); + }); + + await test.step('should save and verify partial update', async () => { + await poAdmin.tabs.users.btnSaveUser.click(); + await poHomeChannel.dismissToast(); + + await poAdmin.tabs.users.btnContextualbarClose.click(); + await poAdmin.getUserRowByUsername(updateTestUser.data.username).click(); + await poAdmin.btnEdit.click(); + + await expect(poAdmin.tabs.users.getCustomField('customFieldText1')).toHaveValue(adminCustomFieldUpdated1); + await expect(poAdmin.tabs.users.getCustomField('customFieldText2')).toHaveValue(adminCustomFieldValue2); + }); + }); +}); diff --git a/apps/meteor/tests/e2e/containers/saml/Dockerfile b/apps/meteor/tests/e2e/containers/saml/Dockerfile index be87fdb742b65..df79f79af4221 100644 --- a/apps/meteor/tests/e2e/containers/saml/Dockerfile +++ b/apps/meteor/tests/e2e/containers/saml/Dockerfile @@ -1,4 +1,4 @@ -FROM php:7.1-apache +FROM php:7.3-apache # Utilities RUN apt-get update && \ @@ -6,7 +6,7 @@ RUN apt-get update && \ rm -r /var/lib/apt/lists/* # SimpleSAMLphp -ARG SIMPLESAMLPHP_VERSION=1.15.2 +ARG SIMPLESAMLPHP_VERSION=1.15.4 RUN curl -s -L -o /tmp/simplesamlphp.tar.gz https://github.com/simplesamlphp/simplesamlphp/releases/download/v$SIMPLESAMLPHP_VERSION/simplesamlphp-$SIMPLESAMLPHP_VERSION.tar.gz && \ tar xzf /tmp/simplesamlphp.tar.gz -C /tmp && \ rm -f /tmp/simplesamlphp.tar.gz && \ diff --git a/apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-users.ts b/apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-users.ts index 0a9ccd3547c2a..b1f284249f006 100644 --- a/apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-users.ts +++ b/apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-users.ts @@ -15,6 +15,18 @@ export class AdminFlextabUsers { return this.page.locator('role=button[name="Add user"]'); } + get btnSaveUser(): Locator { + return this.page.locator('role=button[name="Save user"]'); + } + + get btnMoreActions(): Locator { + return this.page.locator('role=button[name="More"]'); + } + + get btnDeleteUser(): Locator { + return this.page.locator('role=menuitem[name="Delete"]'); + } + get btnInvite(): Locator { return this.page.locator('role=button[name="Invite"]'); } @@ -67,4 +79,8 @@ export class AdminFlextabUsers { get btnContextualbarClose(): Locator { return this.page.locator('button[data-qa="ContextualbarActionClose"]'); } + + getCustomField(fieldName: string): Locator { + return this.page.getByRole('textbox', { name: fieldName }); + } } diff --git a/apps/meteor/tests/e2e/utils/user-helpers.ts b/apps/meteor/tests/e2e/utils/user-helpers.ts new file mode 100644 index 0000000000000..267fbc6b6b9a9 --- /dev/null +++ b/apps/meteor/tests/e2e/utils/user-helpers.ts @@ -0,0 +1,77 @@ +import { faker } from '@faker-js/faker'; +import type { APIResponse } from '@playwright/test'; +import type { IUser } from '@rocket.chat/core-typings'; + +import type { BaseTest } from './test'; +import { DEFAULT_USER_CREDENTIALS } from '../config/constants'; + +export interface ICreateUserOptions { + username?: string; + email?: string; + name?: string; + password?: string; + roles?: string[]; + data?: Record; +} + +export interface ITestUser { + response: APIResponse; + data: IUser & { username: string }; + deleted: boolean; + delete: () => Promise; + markAsDeleted: () => void; +} + +/** + * Creates a test user with optional customizations + */ +export async function createTestUser(api: BaseTest['api'], options: ICreateUserOptions = {}): Promise { + const userData = { + email: options.email || faker.internet.email(), + name: options.name || faker.person.fullName(), + password: options.password || DEFAULT_USER_CREDENTIALS.password, + username: options.username || `test-user-${faker.string.uuid()}`, + roles: options.roles || ['user'], + ...options.data, + }; + + const response = await api.post('/users.create', userData); + + if (response.status() !== 200) { + throw new Error(`Failed to create user: ${response.status()}, response: ${await response.text()}`); + } + + const { user } = await response.json(); + + return { + response, + data: user, + deleted: false, + markAsDeleted(this: ITestUser) { + this.deleted = true; + }, + async delete(this: ITestUser) { + if (this.deleted) { + return; + } + + const response = await api.post('/users.delete', { userId: user._id }); + this.markAsDeleted(); + return response; + }, + }; +} + +/** + * Creates multiple test users at once + */ +export async function createTestUsers(api: BaseTest['api'], count: number, options: ICreateUserOptions = {}): Promise { + const promises = Array.from({ length: count }, (_, i) => + createTestUser(api, { + ...options, + username: options.username ? `${options.username}-${i + 1}` : undefined, + }), + ); + + return Promise.all(promises); +} diff --git a/apps/meteor/tests/end-to-end/api/users.ts b/apps/meteor/tests/end-to-end/api/users.ts index 84c52e7596fe9..1d4d0e6121d21 100644 --- a/apps/meteor/tests/end-to-end/api/users.ts +++ b/apps/meteor/tests/end-to-end/api/users.ts @@ -2030,6 +2030,168 @@ describe('[Users]', () => { reservedWords.forEach((name) => { failUpdateUser(name); }); + + describe('Custom Fields', () => { + let testUser: TestUser; + + before(async () => { + await setCustomFields({ + customFieldText1: { + type: 'text', + required: false, + }, + customFieldText2: { + type: 'text', + required: false, + }, + }); + }); + + after(async () => { + await clearCustomFields(); + }); + + beforeEach(async () => { + testUser = await createUser(); + }); + + afterEach(async () => { + await deleteUser(testUser); + }); + + it('should merge custom fields instead of replacing them when updating a user', async () => { + await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: 'value1', + customFieldText2: 'value2', + }, + }, + }) + .expect(200); + + const updateResponse = await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: 'updated1', + }, + }, + }) + .expect(200); + + expect(updateResponse.body).to.have.property('success', true); + expect(updateResponse.body).to.have.nested.property('user.customFields.customFieldText1', 'updated1'); + expect(updateResponse.body).to.have.nested.property('user.customFields.customFieldText2', 'value2'); + + const userInfoResponse = await request.get(api('users.info')).set(credentials).query({ userId: testUser._id }).expect(200); + + expect(userInfoResponse.body).to.have.property('success', true); + expect(userInfoResponse.body).to.have.nested.property('user.customFields.customFieldText1', 'updated1'); + expect(userInfoResponse.body).to.have.nested.property('user.customFields.customFieldText2', 'value2'); + }); + + it('should preserve existing custom fields when adding new ones', async () => { + await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: 'initial1', + }, + }, + }) + .expect(200); + + const updateResponse = await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText2: 'additional2', + }, + }, + }) + .expect(200); + + expect(updateResponse.body).to.have.property('success', true); + expect(updateResponse.body).to.have.nested.property('user.customFields.customFieldText1', 'initial1'); + expect(updateResponse.body).to.have.nested.property('user.customFields.customFieldText2', 'additional2'); + }); + + it('should update custom field with empty string', async () => { + await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: 'value1', + }, + }, + }) + .expect(200); + + const updateResponse = await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: '', + }, + }, + }) + .expect(200); + + expect(updateResponse.body).to.have.property('success', true); + expect(updateResponse.body).to.have.nested.property('user.customFields.customFieldText1', ''); + }); + + it('should update custom field with null', async () => { + await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: 'value1', + }, + }, + }) + .expect(200); + + const updateResponse = await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + customFields: { + customFieldText1: null, + }, + }, + }) + .expect(200); + + expect(updateResponse.body).to.have.property('success', true); + expect(updateResponse.body).to.have.nested.property('user.customFields.customFieldText1', null); + }); + }); }); describe('[/users.updateOwnBasicInfo]', () => {