diff --git a/webapp/channels/src/components/admin_console/system_properties/section_utils.ts b/webapp/channels/src/components/admin_console/system_properties/section_utils.ts index e33aced9f2..8b098f73ec 100644 --- a/webapp/channels/src/components/admin_console/system_properties/section_utils.ts +++ b/webapp/channels/src/components/admin_console/system_properties/section_utils.ts @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import type {ReactNode} from 'react'; +import type {ReactElement} from 'react'; import {useState, useCallback, useEffect} from 'react'; import {useSelector} from 'react-redux'; @@ -12,7 +12,7 @@ export class BatchProcessingError extends Error { } export type SectionHook = SectionIO & { - content: ReactNode; + content: ReactElement; } export type SectionIO = { @@ -114,7 +114,13 @@ export function useThing(ops: ReadOperations, initial: T) { * @param opts.commit Action to save pending thing. * @remarks After successfully committing, sync the resulting thing back to the current thing to reconcile or complete or the cycle and clear any diffs. */ -export function usePendingThing, TErr extends Error>(data: T, opts: {commit: (pending: T, current: T) => T | Promise}) { +export function usePendingThing, TErr extends Error>( + data: T, + opts: { + commit: (pending: T, current: T) => T | Promise; + beforeUpdate?: (pending: T, current: T) => T; + }, +) { const [pending, setPending] = useState(data); const hasChanges = pending !== data; @@ -122,11 +128,19 @@ export function usePendingThing, TErr extends useEffect(() => { setPending(data); - }, [data]); + }, [setPending, data]); const apply = useCallback((update: T | ((current: T) => T)) => { - setPending((current) => (typeof update === 'function' ? update(current) : ({...current, ...update}))); - }, [setPending]); + setPending((currentPending) => { + const next = typeof update === 'function' ? update(currentPending) : ({...currentPending, ...update}); + + if (opts.beforeUpdate) { + return opts?.beforeUpdate(next, data); + } + + return next; + }); + }, [setPending, data, opts.beforeUpdate]); const reset = useCallback(() => { setPending(data); diff --git a/webapp/channels/src/components/admin_console/system_properties/system_properties.test.tsx b/webapp/channels/src/components/admin_console/system_properties/system_properties.test.tsx new file mode 100644 index 0000000000..3d5105dffb --- /dev/null +++ b/webapp/channels/src/components/admin_console/system_properties/system_properties.test.tsx @@ -0,0 +1,70 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {screen, waitFor} from '@testing-library/react'; +import React from 'react'; + +import type {UserPropertyField} from '@mattermost/types/properties'; +import type {DeepPartial} from '@mattermost/types/utilities'; + +import {Client4} from 'mattermost-redux/client'; + +import {renderWithContext} from 'tests/react_testing_utils'; +import {TestHelper} from 'utils/test_helper'; + +import type {GlobalState} from 'types/store'; + +import SystemProperties from './system_properties'; + +function getBaseState(): DeepPartial { + const currentUser = TestHelper.getUserMock(); + const otherUser = TestHelper.getUserMock(); + + return { + entities: { + users: { + currentUserId: currentUser.id, + profiles: { + [currentUser.id]: currentUser, + [otherUser.id]: otherUser, + }, + }, + general: { + + }, + }, + }; +} + +describe('SystemProperties', () => { + const getFields = jest.spyOn(Client4, 'getCustomProfileAttributeFields'); + + const baseField = {type: 'text' as const, group_id: 'custom_profile_attributes' as const, create_at: 1736541716295, delete_at: 0, update_at: 0}; + const field0: UserPropertyField = {id: 'f0', name: 'test attribute 0', ...baseField}; + const field1: UserPropertyField = {id: 'f1', name: 'test attribute 1', ...baseField}; + const field2: UserPropertyField = {id: 'f2', name: 'test attribute 2', ...baseField}; + const field3: UserPropertyField = {id: 'f3', name: 'test attribute 3', ...baseField}; + + getFields.mockResolvedValue([field0, field1, field2, field3]); + + describe('UserProperties', () => { + it('loads custom user properties', async () => { + renderWithContext(, getBaseState()); + + await waitFor(() => { + expect(screen.queryByText('Loading')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(screen.queryByText('Loading')).not.toBeInTheDocument(); + }); + + expect(screen.getByRole('heading', {name: 'User Properties'})).toBeInTheDocument(); + + expect(screen.queryByDisplayValue('test attribute 0')).toBeInTheDocument(); + expect(screen.queryByDisplayValue('test attribute 1')).toBeInTheDocument(); + expect(screen.queryByDisplayValue('test attribute 2')).toBeInTheDocument(); + expect(screen.queryByDisplayValue('test attribute 3')).toBeInTheDocument(); + }); + }); +}); diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx index d94aea4bcf..7581eec3d0 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx @@ -19,7 +19,7 @@ import {DangerText, FieldDeleteButton, FieldInput, LinkButton} from './controls' import type {SectionHook} from './section_utils'; import {useUserPropertyFieldDelete} from './user_properties_delete_modal'; import type {UserPropertyFields} from './user_properties_utils'; -import {isCreatePending, useUserPropertyFields, ValidationWarningNameRequired, ValidationWarningNameUnique} from './user_properties_utils'; +import {isCreatePending, useUserPropertyFields, ValidationWarningNameRequired, ValidationWarningNameTaken, ValidationWarningNameUnique} from './user_properties_utils'; import {AdminConsoleListTable} from '../list_table'; @@ -34,6 +34,7 @@ type FieldActions = { export const useUserPropertiesTable = (): SectionHook => { const [userPropertyFields, readIO, pendingIO, itemOps] = useUserPropertyFields(); + const nonDeletedCount = Object.values(userPropertyFields.data).filter((f) => f.delete_at === 0).length; const save = async () => { const newData = await pendingIO.commit(); @@ -53,7 +54,7 @@ export const useUserPropertiesTable = (): SectionHook => { updateField={itemOps.update} deleteField={itemOps.delete} /> - {userPropertyFields.order.length < Constants.MAX_CUSTOM_ATTRIBUTES && ( + {nonDeletedCount < Constants.MAX_CUSTOM_ATTRIBUTES && ( { }; export function UserPropertiesTable({data: collection, updateField, deleteField}: Props & FieldActions) { + const {formatMessage} = useIntl(); const data = collectionToArray(collection); const col = createColumnHelper(); const columns = useMemo>>(() => { @@ -115,6 +117,14 @@ export function UserPropertiesTable({data: collection, updateField, deleteField} defaultMessage='Property names must be unique.' /> ); + } else if (warningId === ValidationWarningNameTaken) { + warning = ( + + ); } return ( @@ -122,8 +132,10 @@ export function UserPropertiesTable({data: collection, updateField, deleteField} { updateField({...row.original, name: value.trim()}); @@ -331,6 +343,8 @@ const ActionsRoot = styled.div` type EditableValueProps = { value: string; + label?: string; + testid?: string; setValue: (value: string) => void; autoFocus?: boolean; disabled?: boolean; @@ -351,7 +365,8 @@ const EditableValue = (props: EditableValueProps) => { <> { const currentUser = TestHelper.getUserMock(); @@ -38,16 +43,20 @@ function getBaseState(): DeepPartial { describe('useUserPropertyFields', () => { jest.useFakeTimers(); - const getCustomProfileAttributeFields = jest.spyOn(Client4, 'getCustomProfileAttributeFields'); + const getFields = jest.spyOn(Client4, 'getCustomProfileAttributeFields'); + const patchField = jest.spyOn(Client4, 'patchCustomProfileAttributeField'); + const deleteField = jest.spyOn(Client4, 'deleteCustomProfileAttributeField'); + const createField = jest.spyOn(Client4, 'createCustomProfileAttributeField'); + + const baseField = {type: 'text' as const, group_id: 'custom_profile_attributes' as const, create_at: 1736541716295, delete_at: 0, update_at: 0}; + const field0: UserPropertyField = {id: 'f0', name: 'test attribute 0', ...baseField}; + const field1: UserPropertyField = {id: 'f1', name: 'test attribute 1', ...baseField}; + const field2: UserPropertyField = {id: 'f2', name: 'test attribute 2', ...baseField}; + const field3: UserPropertyField = {id: 'f3', name: 'test attribute 3', ...baseField}; + + getFields.mockResolvedValue([field0, field1, field2, field3]); it('should return a collection', async () => { - const field0: UserPropertyField = {id: 'f0', name: 'test attribute 0', type: 'text', create_at: 1736541716295, delete_at: 0, update_at: 0}; - const field1: UserPropertyField = {id: 'f1', name: 'test attribute 1', type: 'text', create_at: 1736541716295, delete_at: 0, update_at: 0}; - const field2: UserPropertyField = {id: 'f2', name: 'test attribute 2', type: 'text', create_at: 1736541716295, delete_at: 0, update_at: 0}; - const field3: UserPropertyField = {id: 'f3', name: 'test attribute 3', type: 'text', create_at: 1736541716295, delete_at: 0, update_at: 0}; - - getCustomProfileAttributeFields.mockResolvedValue([field0, field1, field2, field3]); - const {result, rerender, waitFor} = renderHookWithContext(() => { return useUserPropertyFields(); }, getBaseState()); @@ -55,7 +64,7 @@ describe('useUserPropertyFields', () => { const [fields1, read1] = result.current; expect(read1.loading).toBe(true); expect(read1.error).toBe(undefined); - expect(getCustomProfileAttributeFields).toBeCalledTimes(1); + expect(getFields).toBeCalledTimes(1); expect(fields1.data).toEqual({}); expect(fields1.order).toEqual([]); @@ -75,4 +84,254 @@ describe('useUserPropertyFields', () => { expect(fields2.data).toEqual({[field0.id]: field0, [field1.id]: field1, [field2.id]: field2, [field3.id]: field3}); expect(fields2.order).toEqual(['f0', 'f1', 'f2', 'f3']); }); + + it('should successfully handle edits', async () => { + const {result, rerender, waitFor} = renderHookWithContext(() => { + return useUserPropertyFields(); + }, getBaseState()); + + act(() => { + jest.runAllTimers(); + }); + rerender(); + + await waitFor(() => { + const [, read] = result.current; + expect(read.loading).toBe(false); + }); + const [fields2,,, ops2] = result.current; + + act(() => { + ops2.update({...fields2.data[field1.id], name: 'changed attribute value'}); + }); + rerender(); + + const [fields3, readIO3, pendingIO3] = result.current; + expect(fields3.data[field1.id].name).toBe('changed attribute value'); + expect(pendingIO3.hasChanges).toBe(true); + + patchField.mockResolvedValue({...fields3.data[field1.id]}); + + await act(async () => { + const data = await pendingIO3.commit(); + if (data) { + readIO3.setData(data); + } + jest.runAllTimers(); + rerender(); + }); + + await waitFor(() => { + const [,, pending] = result.current; + expect(pending.saving).toBe(false); + }); + + expect(patchField).toHaveBeenCalledWith(field1.id, {type: 'text', name: 'changed attribute value'}); + + const [fields4,, pendingIO4] = result.current; + expect(pendingIO4.hasChanges).toBe(false); + expect(pendingIO4.error).toBe(undefined); + expect(fields4.data[field1.id].name).toBe('changed attribute value'); + }); + + it('should successfully handle deletes', async () => { + const {result, rerender, waitFor} = renderHookWithContext(() => { + return useUserPropertyFields(); + }, getBaseState()); + + act(() => { + jest.runAllTimers(); + }); + rerender(); + + await waitFor(() => { + const [, read] = result.current; + expect(read.loading).toBe(false); + }); + const [,,, ops2] = result.current; + + act(() => { + ops2.delete(field1.id); + }); + rerender(); + + const [fields3, readIO3, pendingIO3] = result.current; + expect(fields3.data[field1.id].delete_at).not.toBe(0); + + deleteField.mockResolvedValue({status: 'OK'}); + + await act(async () => { + const data = await pendingIO3.commit(); + if (data) { + readIO3.setData(data); + } + jest.runAllTimers(); + rerender(); + }); + + await waitFor(() => { + const [,, pendingIO4] = result.current; + expect(pendingIO4.saving).toBe(false); + }); + + expect(deleteField).toHaveBeenCalledWith(field1.id); + + const [fields4,,,] = result.current; + expect(fields4.data).not.toHaveProperty(field1.id); + expect(fields4.order).not.toEqual(expect.arrayContaining([field1.id])); + }); + + it('should successfully handle creates', async () => { + createField.mockImplementation((patch) => Promise.resolve({...baseField, ...patch, id: generateId()} as UserPropertyField)); + + const {result, rerender, waitFor} = renderHookWithContext(() => { + return useUserPropertyFields(); + }, getBaseState()); + + act(() => { + jest.runAllTimers(); + }); + rerender(); + + await waitFor(() => { + const [, read] = result.current; + expect(read.loading).toBe(false); + }); + const [,,, ops2] = result.current; + + act(() => { + ops2.create(); + ops2.create(); + }); + rerender(); + + const [fields3, readIO3, pendingIO3] = result.current; + const [createdId0, createdId1] = [...fields3.order].splice(-2, 2); + expect(fields3.data[createdId0].create_at).toBe(0); + expect(fields3.data[createdId1].create_at).toBe(0); + + await act(async () => { + const data = await pendingIO3.commit(); + if (data) { + readIO3.setData(data); + } + jest.runAllTimers(); + rerender(); + }); + + await waitFor(() => { + const [,, pendingIO4] = result.current; + expect(pendingIO4.saving).toBe(false); + }); + + expect(createField).toHaveBeenCalledWith({type: 'text', name: 'Text'}); + expect(createField).toHaveBeenCalledWith({type: 'text', name: 'Text 2'}); + + const [fields4,,,] = result.current; + expect(Object.values(fields4.data)).toEqual(expect.arrayContaining([ + expect.objectContaining({name: 'Text'}), + expect.objectContaining({name: 'Text 2'}), + ])); + + expect(fields4.order).toEqual(expect.arrayContaining(Object.keys(fields4.data))); + }); + + it('should validate name uniqueness', async () => { + const {result, rerender, waitFor} = renderHookWithContext(() => { + return useUserPropertyFields(); + }, getBaseState()); + + act(() => { + jest.runAllTimers(); + }); + rerender(); + + await waitFor(() => { + const [, read] = result.current; + expect(read.loading).toBe(false); + }); + + act(() => { + const [fields,,, ops] = result.current; + ops.update({...fields.data[field0.id], name: 'test attribute 1'}); + }); + rerender(); + + const [fields,, pendingIO3] = result.current; + expect(fields.data[field0.id].name).toBe('test attribute 1'); + expect(pendingIO3.hasChanges).toBe(true); + expect(fields.warnings).toEqual(expect.objectContaining({ + [field0.id]: {name: ValidationWarningNameUnique}, + [field1.id]: {name: ValidationWarningNameUnique}, + })); + }); + + it('should validate names already taken', async () => { + const {result, rerender, waitFor} = renderHookWithContext(() => { + return useUserPropertyFields(); + }, getBaseState()); + + act(() => { + jest.runAllTimers(); + }); + rerender(); + + await waitFor(() => { + const [, read] = result.current; + expect(read.loading).toBe(false); + }); + + act(() => { + const [fields,,, ops] = result.current; + + ops.update({...fields.data[field0.id], name: 'test attribute 1'}); + ops.update({...fields.data[field1.id], name: 'Test something else'}); + }); + rerender(); + + const [fields,, pendingIO] = result.current; + expect(pendingIO.hasChanges).toBe(true); + expect(fields.warnings).toEqual(expect.objectContaining({ + [field0.id]: {name: ValidationWarningNameTaken}, + })); + + // no warning when conflict field is to be deleted + act(() => { + const [,,, ops] = result.current; + ops.delete(field1.id); + }); + rerender(); + + const [fields2] = result.current; + expect(fields2.warnings).toBeUndefined(); + }); + + it('should validate name required', async () => { + const {result, rerender, waitFor} = renderHookWithContext(() => { + return useUserPropertyFields(); + }, getBaseState()); + + act(() => { + jest.runAllTimers(); + }); + rerender(); + + await waitFor(() => { + const [, read] = result.current; + expect(read.loading).toBe(false); + }); + + act(() => { + const [fields,,, ops] = result.current; + + ops.update({...fields.data[field0.id], name: ''}); + }); + rerender(); + + const [fields,, pendingIO] = result.current; + expect(pendingIO.hasChanges).toBe(true); + expect(fields.warnings).toEqual(expect.objectContaining({ + [field0.id]: {name: ValidationWarningNameRequired}, + })); + }); }); diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts b/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts index f8c88b3be4..1dfea816db 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts @@ -3,13 +3,12 @@ import groupBy from 'lodash/groupBy'; import isEmpty from 'lodash/isEmpty'; -import {useCallback, useMemo} from 'react'; +import {useMemo} from 'react'; import type {ClientError} from '@mattermost/client'; -import {isStatusOK} from '@mattermost/types/client4'; -import type {UserPropertyField, UserPropertyFieldPatch} from '@mattermost/types/properties'; +import type {UserPropertyField} from '@mattermost/types/properties'; import {collectionAddItem, collectionFromArray, collectionRemoveItem, collectionReplaceItem, collectionToArray} from '@mattermost/types/utilities'; -import type {PartialExcept, IDMappedCollection} from '@mattermost/types/utilities'; +import type {PartialExcept, IDMappedCollection, IDMappedObjects} from '@mattermost/types/utilities'; import {Client4} from 'mattermost-redux/client'; @@ -20,6 +19,8 @@ import {useThing, usePendingThing, BatchProcessingError} from './section_utils'; export type UserPropertyFields = IDMappedCollection; +type PendingOps = {[op: string]: T[]}; + export const useUserPropertyFields = () => { // current fields const [fieldCollection, readIO] = useThing(useMemo(() => ({ @@ -34,82 +35,142 @@ export const useUserPropertyFields = () => { opts: {forceInitialGet: true}, }), []), collectionFromArray([])); - // save-sync operations - const commit = useCallback(async (collection: UserPropertyFields, prevCollection: UserPropertyFields) => { - const process = collectionToArray(collection).filter((field) => { - // process changed fields - return field !== prevCollection.data[field.id]; - }); + // pending fields to be saved + const [pendingCollection, pendingIO] = usePendingThing>(fieldCollection, useMemo(() => ({ + commit: async (collection: UserPropertyFields, prevCollection: UserPropertyFields) => { + // prepare ops + const process = collectionToArray(collection).reduce>((ops, item) => { + // don't process unchanged items + if (item === prevCollection.data[item.id]) { + return ops; + } - // prepare operations - create, delete, update - const fieldResults = await Promise.allSettled(process.map((item) => { - const {id, name, type} = item; - const patch: UserPropertyFieldPatch = {name, type}; + switch (true) { + case isCreatePending(item): + ops.create.push(item); + break; + case isDeletePending(item): + ops.delete.push(item); + break; + case item !== prevCollection.data[item.id]: + ops.edit.push(item); + } - if (isCreatePending(item)) { - return Client4.createCustomProfileAttributeField(patch); - } else if (isDeletePending(item)) { - return Client4.deleteCustomProfileAttributeField(id); + return ops; + }, {delete: [], edit: [], create: []}); + + const next: UserPropertyFields = { + data: {...collection.data}, + order: [...collection.order], + errors: {}, // start with errors cleared; don't keep stale errors + }; + + // delete - all + await Promise.all(process.delete.map(async ({id}) => { + return Client4.deleteCustomProfileAttributeField(id). + then(() => { + // data:deleted + Reflect.deleteProperty(next.data, id); + + // order:deleted + next.order = next.order.filter((orderId) => orderId !== id); + }). + catch((reason: ClientError) => { + next.errors = {...next.errors, [id]: reason}; + }); + })); + + // update - all + await Promise.all(process.edit.map(async (pendingItem) => { + const {id, name, type} = pendingItem; + + return Client4.patchCustomProfileAttributeField(id, {name, type}). + then((nextItem) => { + // data:updated + next.data[id] = nextItem; + }). + catch((reason: ClientError) => { + next.errors = {...next.errors, [id]: reason}; + }); + })); + + // create - each, to preserve created/sort ordering + for (const pendingItem of process.create) { + const {id, name, type} = pendingItem; + + // eslint-disable-next-line no-await-in-loop + await Client4.createCustomProfileAttributeField({name, type}). + then((newItem) => { + // data:created (delete pending data) + Reflect.deleteProperty(next.data, id); + next.data[newItem?.id] = newItem; + + // order:created (replace pending id with created id) + next.order = next.order.map((orderId) => (orderId === pendingItem?.id ? newItem.id : orderId)); + }). + catch((reason: ClientError) => { + next.errors = {...next.errors, [id]: reason}; + }); } - return Client4.patchCustomProfileAttributeField(id, patch); - })); + if (isEmpty(next.errors)) { + Reflect.deleteProperty(next, 'errors'); + } else { + // set pendingIO master error + throw new BatchProcessingError('error processing operations', {cause: next.errors}); + } - // process operation results - const processedCollection = fieldResults.reduce((results, op, i) => { - const preparedItem = process[i]; + return next; + }, + beforeUpdate: (pending, current) => { + const byNamesLower = (data: IDMappedObjects) => { + return groupBy(data, ({name}) => name.toLowerCase()); + }; - if (op.status === 'fulfilled') { - if (isStatusOK(op.value)) { - // process:data:deleted - Reflect.deleteProperty(results.data, preparedItem.id); + // Name + const pendingByName = byNamesLower(pending.data); + const currentByName = byNamesLower(current.data); - // process:order:deleted - results.order = results.order.filter((id) => id !== preparedItem.id); - } else { - const item = op.value; + const warnings = Object.values(pending.data).reduce>((acc, field) => { + if (!field.name) { + // name not provided + acc[field.id] = {name: ValidationWarningNameRequired}; + } else if (pendingByName[field.name.toLowerCase()]?.filter((x) => x.delete_at === 0)?.length > 1) { + // duplicate pending name + acc[field.id] = {name: ValidationWarningNameUnique}; + } else if ( + currentByName?.[field.name.toLowerCase()]?.length >= 1 && + field.id !== currentByName?.[field.name.toLowerCase()]?.[0]?.id + ) { + // name already in use + const correspondingPending = pending.data[currentByName?.[field.name.toLowerCase()]?.[0]?.id]; - // process:data:created, process:data:updated (set new data) - results.data[item?.id] = item; - - if (item.id !== preparedItem.id) { - // process:order:deleted (delete old data) - Reflect.deleteProperty(results.data, preparedItem.id); - - // process:order:created (replace pending id with created id) - results.order = results.order.map((id) => (id === preparedItem?.id ? item.id : id)); + // except when corresponding field is going to be deleted, then it is no longer in conflict + if (correspondingPending.delete_at === 0) { + // not going to be deleted, so in conflict + acc[field.id] = {name: ValidationWarningNameTaken}; } } - } else if (op.status === 'rejected') { - // failed, log error - results.errors = {...results.errors, [preparedItem.id]: op.reason}; + + return acc; + }, {}); + + const next = {...pending, warnings}; + + if (isEmpty(warnings)) { + Reflect.deleteProperty(next, 'warnings'); } - return results; - }, { - data: {...collection.data}, - order: [...collection.order], - errors: {}, // start with errors cleared; don't keep stale errors - }); + return next; + }, - if (isEmpty(processedCollection.errors)) { - Reflect.deleteProperty(processedCollection, 'errors'); - } else { - // set pendingIO master error - throw new BatchProcessingError('error processing operations', {cause: processedCollection.errors}); - } - - return processedCollection; - }, []); - - // pending fields to be saved - const [pendingCollection, pendingIO] = usePendingThing>(fieldCollection, {commit}); + }), [])); // edit pending fields before saving const itemOps = useMemo(() => ({ update: (field) => { pendingIO.apply((pending) => { - return validate(collectionReplaceItem(pending, field)); + return collectionReplaceItem(pending, field); }); }, create: () => { @@ -125,10 +186,10 @@ export const useUserPropertyFields = () => { if (isCreatePending(field)) { // immediately remove if deleting a field that is pending creation - return validate(collectionRemoveItem(pending, field)); + return collectionRemoveItem(pending, field); } - return validate(collectionReplaceItem(pending, {...field, delete_at: Date.now()})); + return collectionReplaceItem(pending, {...field, delete_at: Date.now()}); }); }, } satisfies CollectionIO), [pendingIO.apply]); @@ -136,31 +197,9 @@ export const useUserPropertyFields = () => { return [pendingCollection, readIO, pendingIO, itemOps] as const; }; -const validate = (pending: UserPropertyFields) => { - // Name - const byName = groupBy(pending.data, 'name'); - - const warnings = Object.values(pending.data).reduce>((acc, field) => { - if (!field.name) { - acc[field.id] = {name: ValidationWarningNameRequired}; - } else if (byName[field.name].length > 1) { - acc[field.id] = {name: ValidationWarningNameUnique}; - } - - return acc; - }, {}); - - const next = {...pending, warnings}; - - if (isEmpty(warnings)) { - Reflect.deleteProperty(next, 'warnings'); - } - - return next; -}; - export const ValidationWarningNameRequired = 'user_properties.validation.name_required'; export const ValidationWarningNameUnique = 'user_properties.validation.name_unique'; +export const ValidationWarningNameTaken = 'user_properties.validation.name_taken'; const getIncrementedName = (desiredName: string, collection: UserPropertyFields) => { const names = new Set(Object.values(collection.data).map(({name}) => name)); @@ -190,6 +229,7 @@ export const newPendingField = (patch: PartialExcept) return { ...patch, type: 'text', + group_id: 'custom_profile_attributes', id: newPendingId(), create_at: 0, delete_at: 0, diff --git a/webapp/channels/src/components/user_settings/general/user_settings_general.test.tsx b/webapp/channels/src/components/user_settings/general/user_settings_general.test.tsx index 3848d382bb..90d5ccc396 100644 --- a/webapp/channels/src/components/user_settings/general/user_settings_general.test.tsx +++ b/webapp/channels/src/components/user_settings/general/user_settings_general.test.tsx @@ -67,6 +67,7 @@ describe('components/user_settings/general/UserSettingsGeneral', () => { const customProfileAttribute: UserPropertyField = { id: '1', + group_id: 'custom_profile_attributes', name: 'Test Attribute', type: 'text', create_at: 0, diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 1bab9075d0..4cf2cb70f1 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -2560,9 +2560,11 @@ "admin.system_properties.user_properties.table.actions": "Actions", "admin.system_properties.user_properties.table.actions.delete": "Delete", "admin.system_properties.user_properties.table.property": "Property", + "admin.system_properties.user_properties.table.property_name.input.name": "Property Name", "admin.system_properties.user_properties.table.type": "Type", "admin.system_properties.user_properties.table.type.text": "Text", "admin.system_properties.user_properties.table.validation.name_required": "Please enter a property name.", + "admin.system_properties.user_properties.table.validation.name_taken": "Property name already taken.", "admin.system_properties.user_properties.table.validation.name_unique": "Property names must be unique.", "admin.system_properties.user_properties.title": "User Properties", "admin.system_roles_feature_discovery.copy": "Assign customizable admin roles to give designated users read and/or write access to select sections of System Console.", diff --git a/webapp/platform/types/src/properties.ts b/webapp/platform/types/src/properties.ts index c1b6202fb6..5e70b8a53e 100644 --- a/webapp/platform/types/src/properties.ts +++ b/webapp/platform/types/src/properties.ts @@ -3,7 +3,7 @@ export type PropertyField = { id: string; - group_id?: string; + group_id: string; name: string; type: string; attrs?: {[key: string]: unknown}; @@ -26,9 +26,10 @@ export type PropertyValue = { } export type UserPropertyFieldType = 'text'; -export type UserPropertyFieldGroupID = 'user_properties'; +export type UserPropertyFieldGroupID = 'custom_profile_attributes'; export type UserPropertyField = PropertyField & { type: UserPropertyFieldType; + group_id: UserPropertyFieldGroupID; } export type UserPropertyFieldPatch = Partial>;