MM-62543: CPA sequential write ops & better validation (#29933)

* sequential write operations

* group_id required

* warn against names already in use

* add tests

* fix lint

* i18n

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<T = Error> extends Error {
 }
 
 export type SectionHook = SectionIO & {
-    content: ReactNode;
+    content: ReactElement;
 }
 
 export type SectionIO = {
@@ -114,7 +114,13 @@ export function useThing<T>(ops: ReadOperations<T>, 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<T extends Record<string, unknown>, TErr extends Error>(data: T, opts: {commit: (pending: T, current: T) => T | Promise<T>}) {
+export function usePendingThing<T extends Record<string, unknown>, TErr extends Error>(
+    data: T,
+    opts: {
+        commit: (pending: T, current: T) => T | Promise<T>;
+        beforeUpdate?: (pending: T, current: T) => T;
+    },
+) {
     const [pending, setPending] = useState(data);
     const hasChanges = pending !== data;
 
@@ -122,11 +128,19 @@ export function usePendingThing<T extends Record<string, unknown>, 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);

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<GlobalState> {
+    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(<SystemProperties disabled={false}/>, 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();
+        });
+    });
+});

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 && (
                 <LinkButton onClick={itemOps.create}>
                     <PlusIcon size={16}/>
                     <FormattedMessage
@@ -78,6 +79,7 @@ export const useUserPropertiesTable = (): SectionHook => {
 };
 
 export function UserPropertiesTable({data: collection, updateField, deleteField}: Props & FieldActions) {
+    const {formatMessage} = useIntl();
     const data = collectionToArray(collection);
     const col = createColumnHelper<UserPropertyField>();
     const columns = useMemo<Array<ColumnDef<UserPropertyField, any>>>(() => {
@@ -115,6 +117,14 @@ export function UserPropertiesTable({data: collection, updateField, deleteField}
                                 defaultMessage='Property names must be unique.'
                             />
                         );
+                    } else if (warningId === ValidationWarningNameTaken) {
+                        warning = (
+                            <FormattedMessage
+                                tagName={DangerText}
+                                id='admin.system_properties.user_properties.table.validation.name_taken'
+                                defaultMessage='Property name already taken.'
+                            />
+                        );
                     }
 
                     return (
@@ -122,8 +132,10 @@ export function UserPropertiesTable({data: collection, updateField, deleteField}
                             <EditableValue
                                 strong={true}
                                 value={getValue()}
+                                label={formatMessage({id: 'admin.system_properties.user_properties.table.property_name.input.name', defaultMessage: 'Property Name'})}
                                 deleted={toDelete}
                                 borderless={!warning}
+                                testid='property-field-input'
                                 autoFocus={isCreatePending(row.original)}
                                 setValue={(value: string) => {
                                     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) => {
         <>
             <FieldInput
                 type='text'
-                data-testid='property-field-input'
+                aria-label={props.label}
+                data-testid={props.testid}
                 disabled={props.disabled ?? props.deleted}
                 $deleted={props.deleted}
                 $strong={props.strong}

webapp/channels/src/components/admin_console/system_properties/user_properties_utils.test.ts

@@ -7,14 +7,19 @@ import type {UserPropertyField} from '@mattermost/types/properties';
 import type {DeepPartial} from '@mattermost/types/utilities';
 
 import {Client4} from 'mattermost-redux/client';
+import {generateId} from 'mattermost-redux/utils/helpers';
 
 import {renderHookWithContext} from 'tests/react_testing_utils';
 import {TestHelper} from 'utils/test_helper';
 
 import type {GlobalState} from 'types/store';
 
-import {} from './section_utils';
-import {useUserPropertyFields} from './user_properties_utils';
+import {
+    useUserPropertyFields,
+    ValidationWarningNameRequired,
+    ValidationWarningNameTaken,
+    ValidationWarningNameUnique,
+} from './user_properties_utils';
 
 function getBaseState(): DeepPartial<GlobalState> {
     const currentUser = TestHelper.getUserMock();
@@ -38,16 +43,20 @@ function getBaseState(): DeepPartial<GlobalState> {
 
 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},
+        }));
+    });
 });

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<UserPropertyField>;
 
+type PendingOps<T extends {id: string}> = {[op: string]: T[]};
+
 export const useUserPropertyFields = () => {
     // current fields
     const [fieldCollection, readIO] = useThing<UserPropertyFields>(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<UserPropertyFields, BatchProcessingError<ClientError>>(fieldCollection, useMemo(() => ({
+        commit: async (collection: UserPropertyFields, prevCollection: UserPropertyFields) => {
+            // prepare ops
+            const process = collectionToArray(collection).reduce<PendingOps<UserPropertyField>>((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<ClientError>('error processing operations', {cause: next.errors});
+            }
 
-        // process operation results
-        const processedCollection = fieldResults.reduce<UserPropertyFields>((results, op, i) => {
-            const preparedItem = process[i];
+            return next;
+        },
+        beforeUpdate: (pending, current) => {
+            const byNamesLower = (data: IDMappedObjects<UserPropertyField>) => {
+                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<NonNullable<UserPropertyFields['warnings']>>((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<ClientError>('error processing operations', {cause: processedCollection.errors});
-        }
-
-        return processedCollection;
-    }, []);
-
-    // pending fields to be saved
-    const [pendingCollection, pendingIO] = usePendingThing<UserPropertyFields, BatchProcessingError<ClientError>>(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<UserPropertyField>), [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<NonNullable<UserPropertyFields['warnings']>>((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<UserPropertyField, 'name'>)
     return {
         ...patch,
         type: 'text',
+        group_id: 'custom_profile_attributes',
         id: newPendingId(),
         create_at: 0,
         delete_at: 0,

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,

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.",

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<T> = {
 }
 
 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<Pick<UserPropertyField, 'name' | 'attrs' | 'type'>>;