From 63bba67eaf4c466770435858fda3884f8f9303fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Vay=C3=A1?= Date: Tue, 25 Feb 2025 10:54:38 +0100 Subject: [PATCH] [MM-62666] Cpa sort order (#8618) * refactor: Use fetchCustomAttributes for user profile custom attributes feat: Add optional filter for empty custom attributes simplify and fix types add again the request time, take into account errors test: Update fetchCustomAttributes tests for filterEmpty parameter feat: Add sort_order to CustomAttribute in fetchCustomAttributes feat: Add optional sort_order to CustomAttribute type fix: Add type definition for sort_order in CustomProfileField attrs feat: Add sorting function for custom profile attributes docs: Add JSDoc comment explaining sortCustomProfileAttributes function refactor: Improve sortCustomProfileAttributes with nullish coalescing and localeCompare add order to custom profile attributes * remove blank line * add ordering to edit_profile screen * keep types together * address code review --- app/actions/remote/user.test.ts | 55 ++++- app/actions/remote/user.ts | 20 +- app/client/rest/custom_profile_attributes.ts | 1 + .../edit_profile/components/form.test.tsx | 46 +++++ app/screens/edit_profile/components/form.tsx | 8 +- .../user_profile/custom_attributes.tsx | 6 +- app/screens/user_profile/user_info.test.tsx | 188 ++++++++++++++++++ app/screens/user_profile/user_info.tsx | 39 ++-- app/utils/user/index.ts | 14 ++ types/api/custom_profile_attributes.d.ts | 26 ++- types/api/users.d.ts | 8 - types/screens/edit_profile.ts | 11 +- 12 files changed, 348 insertions(+), 74 deletions(-) create mode 100644 app/screens/user_profile/user_info.test.tsx diff --git a/app/actions/remote/user.test.ts b/app/actions/remote/user.test.ts index 19965086c..d7209cda6 100644 --- a/app/actions/remote/user.test.ts +++ b/app/actions/remote/user.test.ts @@ -364,29 +364,68 @@ describe('get users', () => { }); describe('Custom Profile Attributes', () => { - it('fetchCustomAttributes - base case', async () => { + it('fetchCustomAttributes - base case without filter', async () => { mockClient.getCustomProfileAttributeFields = jest.fn().mockResolvedValue([ - {id: 'field1', name: 'Field 1'}, - {id: 'field2', name: 'Field 2'}, + {id: 'field1', name: 'Field 1', attrs: {sort_order: 1}}, + {id: 'field2', name: 'Field 2', attrs: {sort_order: 2}}, + {id: 'field3', name: 'Field 3', attrs: {sort_order: 3}}, ]); mockClient.getCustomProfileAttributeValues = jest.fn().mockResolvedValue({ field1: 'value1', - field2: 'value2', + field2: '', + field3: 'value3', }); const result = await fetchCustomAttributes(serverUrl, 'user1'); expect(result.error).toBeUndefined(); expect(result.attributes).toBeDefined(); + expect(Object.keys(result.attributes!)).toHaveLength(3); + expect(result.attributes!.field1).toEqual({ + id: 'field1', + name: 'Field 1', + value: 'value1', + sort_order: 1, + }); + expect(result.attributes!.field2).toEqual({ + id: 'field2', + name: 'Field 2', + value: '', + sort_order: 2, + }); + expect(result.attributes!.field3).toEqual({ + id: 'field3', + name: 'Field 3', + value: 'value3', + sort_order: 3, + }); + }); + + it('fetchCustomAttributes - with filter empty values', async () => { + mockClient.getCustomProfileAttributeFields = jest.fn().mockResolvedValue([ + {id: 'field1', name: 'Field 1'}, + {id: 'field2', name: 'Field 2'}, + {id: 'field3', name: 'Field 3'}, + ]); + mockClient.getCustomProfileAttributeValues = jest.fn().mockResolvedValue({ + field1: 'value1', + field2: '', + field3: 'value3', + }); + + const result = await fetchCustomAttributes(serverUrl, 'user1', true); + expect(result.error).toBeUndefined(); + expect(result.attributes).toBeDefined(); expect(Object.keys(result.attributes!)).toHaveLength(2); expect(result.attributes!.field1).toEqual({ id: 'field1', name: 'Field 1', value: 'value1', }); - expect(result.attributes!.field2).toEqual({ - id: 'field2', - name: 'Field 2', - value: 'value2', + expect(result.attributes!.field2).toBeUndefined(); + expect(result.attributes!.field3).toEqual({ + id: 'field3', + name: 'Field 3', + value: 'value3', }); }); diff --git a/app/actions/remote/user.ts b/app/actions/remote/user.ts index 6cff60ac3..efb0a1d86 100644 --- a/app/actions/remote/user.ts +++ b/app/actions/remote/user.ts @@ -25,8 +25,8 @@ import {fetchGroupsByNames} from './groups'; import {forceLogoutIfNecessary} from './session'; import type {Model} from '@nozbe/watermelondb'; +import type {CustomAttribute, CustomProfileAttributeSimple, CustomProfileField, CustomAttributeSet} from '@typings/api/custom_profile_attributes'; import type UserModel from '@typings/database/models/servers/user'; -import type {CustomAttribute, CustomAttributeSet} from '@typings/screens/edit_profile'; export type MyUserRequest = { user?: UserProfile; @@ -880,7 +880,7 @@ export const getAllSupportedTimezones = async (serverUrl: string) => { } }; -export const fetchCustomAttributes = async (serverUrl: string, userId: string): Promise<{attributes: CustomAttributeSet; error: unknown}> => { +export const fetchCustomAttributes = async (serverUrl: string, userId: string, filterEmpty = false): Promise<{attributes: CustomAttributeSet; error: unknown}> => { try { const client = NetworkManager.getClient(serverUrl); const [fields, attrValues] = await Promise.all([ @@ -890,12 +890,16 @@ export const fetchCustomAttributes = async (serverUrl: string, userId: string): if (fields?.length > 0) { const attributes: Record = {}; - fields.forEach((field) => { - attributes[field.id] = { - id: field.id, - name: field.name, - value: attrValues[field.id] || '', - }; + fields.forEach((field: CustomProfileField) => { + const value = attrValues[field.id] || ''; + if (!filterEmpty || value) { + attributes[field.id] = { + id: field.id, + name: field.name, + value, + sort_order: field.attrs?.sort_order, + }; + } }); return {attributes, error: undefined}; } diff --git a/app/client/rest/custom_profile_attributes.ts b/app/client/rest/custom_profile_attributes.ts index d124ab51f..7bba6adb1 100644 --- a/app/client/rest/custom_profile_attributes.ts +++ b/app/client/rest/custom_profile_attributes.ts @@ -2,6 +2,7 @@ // See LICENSE.txt for license information. import type ClientBase from './base'; +import type {CustomProfileAttributeSimple, CustomProfileField} from '@typings/api/custom_profile_attributes'; export interface ClientCustomAttributesMix { getCustomProfileAttributeFields: () => Promise; diff --git a/app/screens/edit_profile/components/form.test.tsx b/app/screens/edit_profile/components/form.test.tsx index 2470b47ad..9c4ebd5e7 100644 --- a/app/screens/edit_profile/components/form.test.tsx +++ b/app/screens/edit_profile/components/form.test.tsx @@ -8,6 +8,7 @@ import {renderWithIntl} from '@test/intl-test-helper'; import ProfileForm from './form'; +import type {CustomAttributeSet} from '@typings/api/custom_profile_attributes'; import type UserModel from '@typings/database/models/servers/user'; import type {UserInfo} from '@typings/screens/edit_profile'; @@ -126,4 +127,49 @@ describe('ProfileForm', () => { expect(queryByTestId('edit_profile_form.customAttributes.field1')).toBeNull(); }); + + test('should maintain custom attributes sort order', () => { + const customAttributes: CustomAttributeSet = { + attr1: { + id: 'attr1', + name: 'Department', + value: 'Engineering', + sort_order: 1, + }, + attr2: { + id: 'attr2', + name: 'Location', + value: 'Remote', + sort_order: 0, + }, + attr3: { + id: 'attr3', + name: 'Start Date', + value: '2023', + sort_order: 2, + }, + }; + + const props = { + ...baseProps, + enableCustomAttributes: true, + userInfo: { + ...baseProps.userInfo, + customAttributes, + }, + }; + + const {getAllByTestId} = renderWithIntl( + , + ); + + const attributeFields = getAllByTestId(/^edit_profile_form.customAttributes\.attr\d$/); + + // Verify fields are rendered in sort order + expect(attributeFields[0].props.testID).toBe('edit_profile_form.customAttributes.attr2'); // sort_order: 0 + expect(attributeFields[1].props.testID).toBe('edit_profile_form.customAttributes.attr1'); // sort_order: 1 + expect(attributeFields[2].props.testID).toBe('edit_profile_form.customAttributes.attr3'); // sort_order: 2 + }); }); diff --git a/app/screens/edit_profile/components/form.tsx b/app/screens/edit_profile/components/form.tsx index e94fec6b7..648469682 100644 --- a/app/screens/edit_profile/components/form.tsx +++ b/app/screens/edit_profile/components/form.tsx @@ -10,6 +10,7 @@ import useFieldRefs from '@hooks/field_refs'; import {t} from '@i18n'; import {getErrorMessage} from '@utils/errors'; import {logError} from '@utils/log'; +import {sortCustomProfileAttributes} from '@utils/user'; import DisabledFields from './disabled_fields'; import EmailField from './email_field'; @@ -100,7 +101,12 @@ const ProfileForm = ({ ), [enableCustomAttributes, userInfo.customAttributes]); const formKeys = useMemo(() => { - return total_custom_attrs === 0 ? profileKeys : [...profileKeys, ...(Object.keys(userInfo.customAttributes).map((k) => `${CUSTOM_ATTRS_PREFIX}.${k}`))]; + const newKeys = Object.keys(userInfo.customAttributes).sort( + (a: string, b: string): number => { + return sortCustomProfileAttributes(userInfo.customAttributes[a], userInfo.customAttributes[b]); + }).map((k) => `${CUSTOM_ATTRS_PREFIX}.${k}`); + + return total_custom_attrs === 0 ? profileKeys : [...profileKeys, ...newKeys]; }, [userInfo.customAttributes, total_custom_attrs]); const userProfileFields: FieldSequence = useMemo(() => { diff --git a/app/screens/user_profile/custom_attributes.tsx b/app/screens/user_profile/custom_attributes.tsx index 199f80a65..6e5d34306 100644 --- a/app/screens/user_profile/custom_attributes.tsx +++ b/app/screens/user_profile/custom_attributes.tsx @@ -8,14 +8,16 @@ import {FlatList} from 'react-native-gesture-handler'; import UserProfileLabel from './label'; +import type {CustomAttribute} from '@typings/api/custom_profile_attributes'; + type Props = { nickname?: string; position?: string; localTime?: string; - customAttributes?: DisplayCustomAttribute[]; + customAttributes?: CustomAttribute[]; } -const renderAttribute: ListRenderItem = ({item}) => ( +const renderAttribute: ListRenderItem = ({item}) => ( ({ + fetchCustomAttributes: jest.fn().mockResolvedValue({}), +})); + +jest.mock('@context/server', () => ({ + useServerUrl: jest.fn().mockReturnValue(localhost), +})); + +describe('screens/user_profile/UserInfo', () => { + beforeEach(() => { + // Reset mock before each test + (fetchCustomAttributes as jest.Mock).mockReset(); + }); + + const baseProps = { + user: { + id: 'user1', + firstName: 'First', + lastName: 'Last', + username: 'username', + nickname: 'nick', + position: 'Developer', + } as UserModel, + isMyUser: false, + showCustomStatus: false, + showLocalTime: true, + showNickname: true, + showPosition: true, + enableCustomAttributes: true, + }; + + const baseCustomAttributes = { + attr1: { + id: 'attr1', + name: 'Department', + value: 'Engineering', + sort_order: 1, + }, + attr2: { + id: 'attr2', + name: 'Location', + value: 'Remote', + sort_order: 0, + }, + attr3: { + id: 'attr3', + name: 'Start Date', + value: '2023', + sort_order: 2, + }, + }; + + test('should display custom attributes in sort order', async () => { + (fetchCustomAttributes as jest.Mock).mockResolvedValue({attributes: baseCustomAttributes}); + + renderWithIntlAndTheme( + , + ); + + // Wait for the fetch to be called + await waitFor(() => { + expect(fetchCustomAttributes).toHaveBeenCalledWith( + localhost, + 'user1', + true, + ); + }); + + // Wait for all items to be rendered + await waitFor(() => { + // Standard attributes + expect(screen.getByText('Nickname')).toBeTruthy(); + expect(screen.getByText('Position')).toBeTruthy(); + expect(screen.getByText('nick')).toBeTruthy(); + expect(screen.getByText('Developer')).toBeTruthy(); + + // Custom attributes (sorted by sort_order) + expect(screen.getByText('Location')).toBeTruthy(); // sort_order: 0 + expect(screen.getByText('Remote')).toBeTruthy(); + expect(screen.getByText('Department')).toBeTruthy(); // sort_order: 1 + expect(screen.getByText('Engineering')).toBeTruthy(); + expect(screen.getByText('Start Date')).toBeTruthy(); // sort_order: 2 + expect(screen.getByText('2023')).toBeTruthy(); + }); + + // Verify the order of elements + const titles = screen.getAllByTestId(/.*\.title$/); + expect(titles.map((el) => el.props.children)).toEqual([ + 'Nickname', + 'Position', + 'Location', // sort_order: 0 + 'Department', // sort_order: 1 + 'Start Date', // sort_order: 2 + ]); + + const descriptions = screen.getAllByTestId(/.*\.description$/); + expect(descriptions.map((el) => el.props.children)).toEqual([ + 'nick', + 'Developer', + 'Remote', // sort_order: 0 + 'Engineering', // sort_order: 1 + '2023', // sort_order: 2 + ]); + }); + it('should display custom attributes in order when some have no order', async () => { + const withEmptyCustomAttributes = { + attr1: { + id: 'attr1', + name: 'Department', + value: 'Engineering', + }, + attr2: { + id: 'attr2', + name: 'Location', + value: 'Remote', + sort_order: 0, + }, + attr3: { + id: 'attr3', + name: 'Start Date', + value: '2023', + sort_order: 2, + }, + }; + (fetchCustomAttributes as jest.Mock).mockResolvedValue({attributes: withEmptyCustomAttributes}); + + renderWithIntlAndTheme( + , + ); + + await waitFor(() => { + expect(fetchCustomAttributes).toHaveBeenCalledWith( + localhost, + 'user1', + true, + ); + }); + + await waitFor(() => { + // Standard attributes remain + expect(screen.getByText('Nickname')).toBeTruthy(); + expect(screen.getByText('Position')).toBeTruthy(); + expect(screen.getByText('nick')).toBeTruthy(); + expect(screen.getByText('Developer')).toBeTruthy(); + + // Custom attributes (sorted by sort_order) + expect(screen.getByText('Location')).toBeTruthy(); // sort_order: 0 + expect(screen.getByText('Remote')).toBeTruthy(); + expect(screen.getByText('Start Date')).toBeTruthy(); // sort_order: 2 + expect(screen.getByText('2023')).toBeTruthy(); + expect(screen.queryByText('Department')).toBeTruthy(); // sort_order: undefined + expect(screen.queryByText('Engineering')).toBeTruthy(); + }); + + // Verify the order of elements + const titles = screen.getAllByTestId(/.*\.title$/); + expect(titles.map((el) => el.props.children)).toEqual([ + 'Nickname', + 'Position', + 'Location', // sort_order: 0 + 'Start Date', // sort_order: 2 + 'Department', // sort_order: undefined + ]); + + const descriptions = screen.getAllByTestId(/.*\.description$/); + expect(descriptions.map((el) => el.props.children)).toEqual([ + 'nick', + 'Developer', + 'Remote', // sort_order: 0 + '2023', // sort_order: 2 + 'Engineering', // sort_order: undefined + ]); + }); +}); diff --git a/app/screens/user_profile/user_info.tsx b/app/screens/user_profile/user_info.tsx index 97c4b7a2e..d080f8f0e 100644 --- a/app/screens/user_profile/user_info.tsx +++ b/app/screens/user_profile/user_info.tsx @@ -1,16 +1,17 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useEffect, useRef, useState} from 'react'; +import React, {useEffect, useState, useRef} from 'react'; +import {fetchCustomAttributes} from '@actions/remote/user'; import {useServerUrl} from '@context/server'; -import NetworkManager from '@managers/network_manager'; -import {getUserCustomStatus} from '@utils/user'; +import {getUserCustomStatus, sortCustomProfileAttributes} from '@utils/user'; import CustomAttributes from './custom_attributes'; import UserProfileCustomStatus from './custom_status'; import type {UserModel} from '@database/models/server'; +import type {CustomAttribute} from '@typings/api/custom_profile_attributes'; type Props = { localTime?: string; @@ -22,12 +23,13 @@ type Props = { enableCustomAttributes?: boolean; } -const emptyList: DisplayCustomAttribute[] = []; /** avoid re-renders **/ +const emptyList: CustomAttribute[] = []; /** avoid re-renders **/ const UserInfo = ({localTime, showCustomStatus, showLocalTime, showNickname, showPosition, user, enableCustomAttributes}: Props) => { const customStatus = getUserCustomStatus(user); const serverUrl = useServerUrl(); - const [customAttributes, setCustomAttributes] = useState(emptyList); + const [customAttributes, setCustomAttributes] = useState(emptyList); + const lastRequest = useRef(0); useEffect(() => { @@ -35,28 +37,11 @@ const UserInfo = ({localTime, showCustomStatus, showLocalTime, showNickname, sho const fetchData = async () => { const reqTime = Date.now(); lastRequest.current = reqTime; - try { - const client = NetworkManager.getClient(serverUrl); - const [fields, attrValues] = await Promise.all([ - client.getCustomProfileAttributeFields(), - client.getCustomProfileAttributeValues(user.id), - ]); - - // ignore results if there was a newer request - if (fields && fields.length > 0 && lastRequest.current === reqTime) { - const attributes = fields.map((field) => { - if (attrValues[field.id]) { - return ({ - id: field.id, - name: field.name, - value: attrValues[field.id] || '', - } as DisplayCustomAttribute); - } - return {} as DisplayCustomAttribute; // this will be cleaned out in CustomAttributes along with the fixed attributes. - }); - setCustomAttributes(attributes); - } - } catch { + const {attributes, error} = await fetchCustomAttributes(serverUrl, user.id, true); + if (!error && lastRequest.current === reqTime) { + const attributesList = Object.values(attributes).sort(sortCustomProfileAttributes); + setCustomAttributes(attributesList); + } else { setCustomAttributes(emptyList); } }; diff --git a/app/utils/user/index.ts b/app/utils/user/index.ts index 6a9d4c9f4..64cf90bfb 100644 --- a/app/utils/user/index.ts +++ b/app/utils/user/index.ts @@ -10,6 +10,7 @@ import {CustomStatusDurationEnum} from '@constants/custom_status'; import {DEFAULT_LOCALE, getLocalizedMessage, t} from '@i18n'; import {toTitleCase} from '@utils/helpers'; +import type {CustomAttribute} from '@typings/api/custom_profile_attributes'; import type UserModel from '@typings/database/models/servers/user'; import type {IntlShape} from 'react-intl'; @@ -393,3 +394,16 @@ export const getLastPictureUpdate = (user: UserModel | UserProfile) => { return user.is_bot ? user.bot_last_icon_update : user.last_picture_update || 0; }; + +/** + * Sorts custom profile attributes by their sort_order property, falling back to name comparison. + * Attributes with undefined sort_order are placed last. + * @param a First CustomAttribute to compare + * @param b Second CustomAttribute to compare + * @returns Negative if a comes first, positive if b comes first, 0 if equal + */ +export const sortCustomProfileAttributes = (a: CustomAttribute, b: CustomAttribute): number => { + const orderA = a.sort_order ?? Number.MAX_SAFE_INTEGER; + const orderB = b.sort_order ?? Number.MAX_SAFE_INTEGER; + return orderA === orderB ? a.name.localeCompare(b.name) : orderA - orderB; +}; diff --git a/types/api/custom_profile_attributes.d.ts b/types/api/custom_profile_attributes.d.ts index b49eedb65..e553027d4 100644 --- a/types/api/custom_profile_attributes.d.ts +++ b/types/api/custom_profile_attributes.d.ts @@ -20,7 +20,10 @@ type CustomProfileField = { type: string; /** any extra properties of the field **/ - attrs?: unknown; + attrs?: { + sort_order?: number; + [key: string]: unknown; + }; /** id of the target element if empty it is a system property **/ target_id: string; @@ -33,17 +36,20 @@ type CustomProfileField = { }; /** - * DisplayCustomAttribute - * @description a simplified version of a field with its value for display purposes + * CustomProfileAttributeSimple + * @description simpler type to display a field id with its value. **/ -type DisplayCustomAttribute = { +type CustomProfileAttributeSimple = { + [field_id: string]: string; +} - /** field id **/ +export type CustomAttribute = { id: string; - - /** field name **/ name: string; - - /** value assigned to that field **/ value: string; -}; + sort_order?: number; +} + +export interface CustomAttributeSet { + [key: string]: CustomAttribute; +} diff --git a/types/api/users.d.ts b/types/api/users.d.ts index 603e00587..502bd8901 100644 --- a/types/api/users.d.ts +++ b/types/api/users.d.ts @@ -53,14 +53,6 @@ type CustomProfileAttribute = { delete_at: number; } -/** - * CustomProfileAttributeSimple - * @description simpler type to display a field id with its value. - **/ -type CustomProfileAttributeSimple = { - [field_id: string]: string; -} - type UserProfile = { id: string; create_at: number; diff --git a/types/screens/edit_profile.ts b/types/screens/edit_profile.ts index fdbe4f738..e1f0c2213 100644 --- a/types/screens/edit_profile.ts +++ b/types/screens/edit_profile.ts @@ -3,12 +3,9 @@ import type {AvailableScreens} from './navigation'; import type {FieldProps} from '@screens/edit_profile/components/field'; +import type {CustomAttributeSet} from '@typings/api/custom_profile_attributes'; import type UserModel from '@typings/database/models/servers/user'; -export interface CustomAttributeSet { - [key: string]: CustomAttribute; -} - export interface UserInfo { email: string; firstName: string; @@ -19,12 +16,6 @@ export interface UserInfo { customAttributes: CustomAttributeSet; } -export type CustomAttribute = { - id: string; - name: string; - value: string; -} - export type EditProfileProps = { componentId: AvailableScreens; currentUser?: UserModel;