Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[MM-62701] [MM-62176]Edit custom profile attributes in user profile #8557

Merged
merged 2 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 112 additions & 1 deletion app/actions/remote/user.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
autoUpdateTimezone,
fetchTeamAndChannelMembership,
getAllSupportedTimezones,
fetchCustomAttributes,
updateCustomAttributes,
} from './user';

import type ServerDataOperator from '@database/operator/server_data_operator';
Expand Down Expand Up @@ -90,6 +92,9 @@ const mockClient = {
getTeamMember: jest.fn((id: string, userId: string) => ({id: userId + '-' + id, user_id: userId, team_id: id, roles: ''})),
getChannelMember: jest.fn((cid: string, userId: string) => ({id: userId + '-' + cid, user_id: userId, channel_id: cid, roles: ''})),
getTimezones: jest.fn(() => ['EST']),
getCustomProfileAttributeFields: jest.fn(),
getCustomProfileAttributeValues: jest.fn(),
updateCustomProfileAttributeValues: jest.fn(),
};

beforeAll(() => {
Expand Down Expand Up @@ -337,7 +342,7 @@ describe('get users', () => {
});

it('buildProfileImageUrlFromUser - base case', async () => {
const result = await buildProfileImageUrlFromUser(serverUrl, user2);
const result = buildProfileImageUrlFromUser(serverUrl, user2);
expect(result).toBeDefined();
});

Expand All @@ -358,6 +363,112 @@ describe('get users', () => {
});
});

describe('Custom Profile Attributes', () => {
it('fetchCustomAttributes - base case', async () => {
mockClient.getCustomProfileAttributeFields = jest.fn().mockResolvedValue([
{id: 'field1', name: 'Field 1'},
{id: 'field2', name: 'Field 2'},
]);
mockClient.getCustomProfileAttributeValues = jest.fn().mockResolvedValue({
field1: 'value1',
field2: 'value2',
});

const result = await fetchCustomAttributes(serverUrl, 'user1');
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',
});
});

it('fetchCustomAttributes - no fields', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: You are missing the network error test.
Also... why not putting all the tests for the same function under one describe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do

describe('Custom Profile Attributes')
  describe('fetchCustomAttributes')
    it('base case')
    it('no fields')
...

mockClient.getCustomProfileAttributeFields = jest.fn().mockResolvedValue([]);
mockClient.getCustomProfileAttributeValues = jest.fn().mockResolvedValue({});

const result = await fetchCustomAttributes(serverUrl, 'user1');
expect(result.error).toBeUndefined();
expect(result.attributes).toEqual({});
});

it('fetchCustomAttributes - error on fields', async () => {
const error = new Error('Sample error');

mockClient.getCustomProfileAttributeFields = jest.fn().mockRejectedValue(error);
mockClient.getCustomProfileAttributeValues = jest.fn().mockResolvedValue({
field1: 'value1',
field2: 'value2',
});

const result = await fetchCustomAttributes(serverUrl, 'user1');
expect(result.error).toBeDefined();
});

it('fetchCustomAttributes - error on values', async () => {
const error = new Error('Sample error');

mockClient.getCustomProfileAttributeFields = jest.fn().mockResolvedValue([
{id: 'field1', name: 'Field 1'},
{id: 'field2', name: 'Field 2'},
]);
mockClient.getCustomProfileAttributeValues = jest.fn().mockRejectedValue(error);

const result = await fetchCustomAttributes(serverUrl, 'user1');
expect(result.error).toBeDefined();
});

it('updateCustomAttributes - base case', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You are missing the error case here too.

mockClient.updateCustomProfileAttributeValues = jest.fn().mockResolvedValue({});

const attributes = {
field1: {
id: 'field1',
name: 'Field 1',
value: 'new value 1',
},
field2: {
id: 'field2',
name: 'Field 2',
value: 'new value 2',
},
};

const result = await updateCustomAttributes(serverUrl, attributes);
expect(result.error).toBeUndefined();
expect(result.success).toBe(true);
expect(mockClient.updateCustomProfileAttributeValues).toHaveBeenCalledWith({
field1: 'new value 1',
field2: 'new value 2',
});
});

it('updateCustomAttributes - error', async () => {
const error = new Error('Test Error');

mockClient.updateCustomProfileAttributeValues = jest.fn().mockRejectedValue(error);

const attributes = {
field1: {
id: 'field1',
name: 'Field 1',
value: 'new value 1',
},
};

const result = await updateCustomAttributes(serverUrl, attributes);
expect(result.error).toBeDefined();
expect(result.success).toBe(false);
});
});

describe('update users', () => {
it('updateMe - handle not found database', async () => {
const result = await updateMe('foo', {});
Expand Down
44 changes: 44 additions & 0 deletions app/actions/remote/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {forceLogoutIfNecessary} from './session';

import type {Model} from '@nozbe/watermelondb';
import type UserModel from '@typings/database/models/servers/user';
import type {CustomAttribute, CustomAttributeSet} from '@typings/screens/edit_profile';

export type MyUserRequest = {
user?: UserProfile;
Expand Down Expand Up @@ -878,3 +879,46 @@ export const getAllSupportedTimezones = async (serverUrl: string) => {
return [];
}
};

export const fetchCustomAttributes = async (serverUrl: string, userId: string): Promise<{attributes: CustomAttributeSet; error: unknown}> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These attrs are always ephemeral?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the plan is (at some point) add everything to the database and handle it from there. Due to how the feature was developed that was deprioritized.

try {
const client = NetworkManager.getClient(serverUrl);
const [fields, attrValues] = await Promise.all([
client.getCustomProfileAttributeFields(),
client.getCustomProfileAttributeValues(userId),
]);

if (fields?.length > 0) {
const attributes: Record<string, CustomAttribute> = {};
fields.forEach((field) => {
attributes[field.id] = {
id: field.id,
name: field.name,
value: attrValues[field.id] || '',
};
});
return {attributes, error: undefined};
}
return {attributes: {} as Record<string, CustomAttribute>, error: undefined};
} catch (error) {
logDebug('error on fetchCustomAttributes', getFullErrorMessage(error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to add here the logout if needed function.

forceLogoutIfNecessary(serverUrl, error);
return {attributes: {} as Record<string, CustomAttribute>, error};
}
};

export const updateCustomAttributes = async (serverUrl: string, attributes: CustomAttributeSet): Promise<{success: boolean; error: unknown}> => {
try {
const client = NetworkManager.getClient(serverUrl);
const values: CustomProfileAttributeSimple = {};
Object.keys(attributes).forEach((field) => {
values[field] = attributes[field].value;
});
await client.updateCustomProfileAttributeValues(values);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check: Any error (like forbidden, for example) would go through the catch path, right?

return {success: true, error: undefined};
} catch (error) {
logDebug('error on updateCustomAttributes', getFullErrorMessage(error));
forceLogoutIfNecessary(serverUrl, error);
return {error, success: false};
}
};
4 changes: 4 additions & 0 deletions app/client/rest/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ export default class ClientBase extends ClientTracking {
return `${this.urlVersion}/custom_profile_attributes`;
}

getUserCustomProfileAttributesRoute(userId: string) {
return `${this.getUsersRoute()}/${userId}/custom_profile_attributes`;
}

doFetch = async (url: string, options: ClientOptions, returnDataOnly = true) => {
return this.doFetchWithTracking(url, options, returnDataOnly);
};
Expand Down
16 changes: 16 additions & 0 deletions app/client/rest/custom_profile_attributes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,20 @@ describe('CustomAttributes', () => {
{method: 'get'},
);
});

test('updateCustomProfileAttributeValues', async () => {
const values = {
field_1: 'value1',
field_2: 'value2',
};
await client.updateCustomProfileAttributeValues(values);

expect(client.doFetch).toHaveBeenCalledWith(
`${client.getCustomProfileAttributesRoute()}/values`,
{
method: 'patch',
body: values,
},
);
});
});
12 changes: 11 additions & 1 deletion app/client/rest/custom_profile_attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type ClientBase from './base';
export interface ClientCustomAttributesMix {
getCustomProfileAttributeFields: () => Promise<CustomProfileField[]>;
getCustomProfileAttributeValues: (userID: string) => Promise<CustomProfileAttributeSimple>;
updateCustomProfileAttributeValues: (values: CustomProfileAttributeSimple) => Promise<string>;
}

const ClientCustomAttributes = <TBase extends Constructor<ClientBase>>(superclass: TBase) => class extends superclass {
Expand All @@ -18,10 +19,19 @@ const ClientCustomAttributes = <TBase extends Constructor<ClientBase>>(superclas

getCustomProfileAttributeValues = async (userID: string) => {
return this.doFetch(
`${this.getUserRoute(userID)}/custom_profile_attributes`,
`${this.getUserCustomProfileAttributesRoute(userID)}`,
{method: 'get'},
);
};
updateCustomProfileAttributeValues = async (values: CustomProfileAttributeSimple) => {
return this.doFetch(
`${this.getCustomProfileAttributesRoute()}/values`,
{
method: 'patch',
body: values,
},
);
};
};

export default ClientCustomAttributes;
1 change: 1 addition & 0 deletions app/components/floating_text_input_label/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ const FloatingTextInput = forwardRef<FloatingTextInputRef, FloatingTextInputProp
}

return res;
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to skip this in this memo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the linter is complaining and seems we want to update when anything regarding styling is changed. see #8557 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this memo is to deal with styles. We want anything regarding styles to re-calculate this, right?

}, [styles, theme, shouldShowError, focused, textInputStyle, focusedLabel, multiline, multilineInputHeight, editable]);

const textAnimatedTextStyle = useAnimatedStyle(() => {
Expand Down
103 changes: 103 additions & 0 deletions app/hooks/field_refs.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

import {renderHook, act} from '@testing-library/react-hooks';

import useFieldRefs from './field_refs';

describe('useFieldRefs', () => {
it('should initialize with empty refs', () => {
const {result} = renderHook(() => useFieldRefs());
const [getRef] = result.current;

expect(getRef('test')).toBeUndefined();
});

it('should set and get refs', () => {
const {result} = renderHook(() => useFieldRefs());
const [getRef, setRef] = result.current;

const mockRef = {
blur: jest.fn(),
focus: jest.fn(),
isFocused: jest.fn(),
};

act(() => {
setRef('testField')(mockRef);
});

expect(getRef('testField')).toBe(mockRef);
});

it('should track number of refs', () => {
const {result} = renderHook(() => useFieldRefs());
const [, setRef] = result.current;

const mockRef1 = {
blur: jest.fn(),
focus: jest.fn(),
isFocused: jest.fn(),
};

const mockRef2 = {
blur: jest.fn(),
focus: jest.fn(),
isFocused: jest.fn(),
};

act(() => {
setRef('field1')(mockRef1);
setRef('field2')(mockRef2);
});
});

it('should remove refs when cleanup function is called', () => {
const {result} = renderHook(() => useFieldRefs());
const [getRef, setRef] = result.current;

const mockRef = {
blur: jest.fn(),
focus: jest.fn(),
isFocused: jest.fn(),
};

let cleanup: () => void;
act(() => {
cleanup = setRef('testField')(mockRef);
});

expect(getRef('testField')).toBe(mockRef);

act(() => {
cleanup!();
});

expect(getRef('testField')).toBeUndefined();
});

it('should handle multiple refs independently', () => {
const {result} = renderHook(() => useFieldRefs());
const [getRef, setRef] = result.current;

const mockRef1 = {
blur: jest.fn(),
focus: jest.fn(),
isFocused: jest.fn(),
};

const mockRef2 = {
blur: jest.fn(),
focus: jest.fn(),
isFocused: jest.fn(),
};

act(() => {
setRef('field1')(mockRef1);
setRef('field2')(mockRef2);
});

expect(getRef('field1')).toBe(mockRef1);
expect(getRef('field2')).toBe(mockRef2);
});
});
Loading