[MM-62701] [MM-62176]Edit custom profile attributes in user profile #8557
Conversation
Willyfrog
changed the title
| @@ -224,7 +224,7 @@ const FloatingTextInput = forwardRef<FloatingTextInputRef, FloatingTextInputProp | |||
| } | |||
|
|
|||
| return res; | |||
| }, [styles, theme, shouldShowError, focused, textInputStyle, focusedLabel, multiline, multilineInputHeight, editable]); | |||
| }, [styles, textInputStyle, multiline, multilineInputHeight]); | |||
There was a problem hiding this comment.
IDE was complaining regarding dependencies. There are others like this throughout the PR.
There was a problem hiding this comment.
Do check the implications of each one of the removed ones
There was a problem hiding this comment.
My understanding is that we want to re-render on those cases, I'll add a comment so there is no warning and it is clear that is intentional.
On the other instances I only added dependencies that are in use within the block, so I believe they are ok. Let me know if you think otherwise :)
There was a problem hiding this comment.
Unless something weird is being done (and therefore, the problem is doing that weird thing) you should be able to trust the linter on any useMemo or useCallback. The dependencies stated there should be the ones you need.
With useEffect is more complicated, since the way useEffect is generally used, you don't always want to execute the effect when something inside changes. So those cases you have to look a bit more careful.
Willyfrog
added
2: Dev Review
| {Object.entries(userInfo.customAttributes || {}).map(([id, customAttr]) => ( | ||
| <CustomProfileField | ||
| key={id} | ||
| fieldKey={`customAttributes.${id}`} | ||
| label={customAttr.name} | ||
| maxLength={128} | ||
| {...fieldConfig} | ||
| returnKeyType='done' | ||
| testID={`edit_profile_form.custom_attribute.${id}`} | ||
| value={customAttr.value} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Sanity check:
Do we ever expect so many custom fields (or so heavy) that we want to virtualize this list?
There was a problem hiding this comment.
right now the max allowed is 20, but there were talks where you could get custom attributes from server + teams + channels. Not sure if we'll ever get that far, but if we do, we can easily go over 50 on complex environments
There was a problem hiding this comment.
50 might be still manageable, but it would be great to do a test, to try to see when it starts to become a problem (also keeping in mind whether you are using a low, med, or high-end device).
A virtualized list comes with its own overhead, and I am not sure how many elements we will be able to hide (if in the screen you can fit around 10, the virtualized list is going to have loaded at least 20 or 30, depending on the configuration), so I don't want to do any early optimization here.
There was a problem hiding this comment.
the posibiliity is there in the future, once we start adding new types that might require front-end calculation.
but at the same time, in my list of tasks is moving everything to watermelondb which might be able to store any intermediate results if needed.
| }; | ||
|
|
||
| fetchCustomAttributes(); | ||
| }, [currentUser, serverUrl]); |
There was a problem hiding this comment.
Nit: This will never be an issue, but we are opening the door for it being an issue :P
This is not going to be a problem because currentUser and serverUrl will never change. But let's imagine it does.
If that was the case, we would make a request every time any of these values change. Since we are overwriting the value of customAttributes, it should be fine, unless... we have two calls done in a short time, and there is a race condition. In that case, we may ovewrite customAttributes with wrong information.
There is also the detail that if one of the requests fail, we may keep stale data, since we don't clean on throw.
How I would solve this for now? Empty the dependency list. This is an "onMount". We don't want to do this again on update ever.
Another solution? Add a timestamp on a ref, so you only store (or clean on) "the latest call".
The reality? This can stay like this, because nothing of what I said is going to cause any real problem.
There was a problem hiding this comment.
To be fair, I added it to make the linter happy, calculations depend on them and didn't think too much of that.
We can remove the serverURL, but I think we should still keep the currentUser in case the session is killed, right?
There was a problem hiding this comment.
If the session is killed, we get directly logged out, and kicked to the login screen. So we should never be in this screen while the user is changing.
But as I said... this is just a philosophical issue, not a really technical one :P
| const client = NetworkManager.getClient(serverUrl); | ||
| await client.updateCustomProfileAttributeValues(values); |
There was a problem hiding this comment.
We may want to add this to a remote action, to avoid having to deal with the network manager at this level. Also, it will handle the possible thrown exceptions.
There was a problem hiding this comment.
Also, if there is any error, I wonder if we want to inform the user somehow.
There was a problem hiding this comment.
I don't see tests for the new behavior on this file.
There was a problem hiding this comment.
I tried to add one to this, but couldn't get it to work. Since there are a ton of mechanisms in place for rendering this piece, I skipped it for the time being. Likely an issue on my end, but since there are already tests for the form section, that should cover most functionality along with the remote action ones.
| }); | ||
| }); | ||
|
|
||
| it('fetchCustomAttributes - no fields', async () => { |
There was a problem hiding this comment.
nitpick: You are missing the network error test.
Also... why not putting all the tests for the same function under one describe?
There was a problem hiding this comment.
You can do
describe('Custom Profile Attributes')
describe('fetchCustomAttributes')
it('base case')
it('no fields')
...
| Object.keys(attributes).forEach((field) => { | ||
| values[field] = attributes[field].value; | ||
| }); | ||
| await client.updateCustomProfileAttributeValues(values); |
There was a problem hiding this comment.
Sanity check: Any error (like forbidden, for example) would go through the catch path, right?
| expect(result.attributes).toEqual({}); | ||
| }); | ||
|
|
||
| it('updateCustomAttributes - base case', async () => { |
There was a problem hiding this comment.
Nit: You are missing the error case here too.
| } | ||
| return {attributes: {}}; | ||
| } catch (error) { | ||
| logDebug('error on fetchCustomAttributes', getFullErrorMessage(error)); |
There was a problem hiding this comment.
We probably want to add here the logout if needed function.
| @@ -224,6 +224,7 @@ const FloatingTextInput = forwardRef<FloatingTextInputRef, FloatingTextInputProp | |||
| } | |||
|
|
|||
| return res; | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
Why do you want to skip this in this memo?
There was a problem hiding this comment.
the linter is complaining and seems we want to update when anything regarding styling is changed. see #8557 (comment)
There was a problem hiding this comment.
But this memo is to deal with styles. We want anything regarding styles to re-calculate this, right?
app/hooks/field_refs.ts
Outdated
| return allRefs.current; | ||
| }; | ||
|
|
||
| const numRefs = () => getAllRefs().size; |
There was a problem hiding this comment.
Why do we want this? Is this just for testing purposes?
There was a problem hiding this comment.
This was an attempt to figure out why it wasn't rendering properly. After deciding for a different approach I thought about removing it, but I found that it was good for debug/testing purposes and left it in. Since it is the last element, it can be easily skipped.
There was a problem hiding this comment.
The wider the interface, the wider the maintenance area. I would say we remove it, just to avoid having something extra to maintain, but I see your point regarding testing. 0/5
app/hooks/field_refs.ts
Outdated
| const getRef = (key: string): FloatingTextInputRef | undefined => { | ||
| return allRefs.current?.get(key); | ||
| }; |
There was a problem hiding this comment.
Being purist, we should try all the returns of the hook to be "stable" (same function if you get the same values). But getRef is more important, because it is more common to have it in other callbacks, and it might cause unneeded re-renders.
Long story short, use useCallback at least for this one (I would prefer for all the returns).
| } else { | ||
| Keyboard.dismiss(); | ||
| } | ||
| }), [canSave, userProfileFields]); | ||
| }), [canSave, userProfileFields, submitUser, getRef]); |
There was a problem hiding this comment.
See here that we are using getRef in a dependency. If we don't make it stable, this callback becomes useless, and may be rendering more callbacks useless down the line.
Willyfrog
added
the
Build Apps for PR
|
@enahum friendly reminder to review :) |
types/screens/edit_profile.ts
Outdated
| email: string; | ||
| firstName: string; | ||
| lastName: string; | ||
| nickname: string; | ||
| position: string; | ||
| username: string; | ||
| customAttributes: Record<string, CustomAttribute>; |
There was a problem hiding this comment.
Might as well make
export interface CustomAttributes {
[key: string]: CustomAttribute;
}
Then you can reuse this in a few instances in the user.ts file.
There was a problem hiding this comment.
added, I also declared the return type which showed a weird return in one of the cases :)
|
/update-branch |
| @@ -878,3 +879,46 @@ export const getAllSupportedTimezones = async (serverUrl: string) => { | |||
| return []; | |||
| } | |||
| }; | |||
|
|
|||
| export const fetchCustomAttributes = async (serverUrl: string, userId: string): Promise<{attributes: CustomAttributeSet; error: unknown}> => { | |||
There was a problem hiding this comment.
These attrs are always ephemeral?
There was a problem hiding this comment.
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.
The idea is to move to a database-backed implementation, but when we started there was a ton of missing stuff missing in the backend. It'll be added in subsequent PRs if everything goes according to plan |
|
Hi @Willyfrog Not sure whether this in within the scope of this PR...
I'm seeing this from Figma on the Jira ticket: And this in the app / build from this PR: The UI is quite different actually
Long.property.description.-.mobile.mp4
Maybe these are questions for @asaadmahmood |
|
|
@asaadmahmood is it ok to add that banner in a separate PR? I already have more changes waiting on this PR being merged. For multiline support, I was waiting for adding the new types and see if we can differentiate text uses (short vs large) to use one or another instead of forcing everything to be multiline (which might not look pretty when used for small texts) |
|
@Willyfrog Sure, if we do not miss that (the banner). |
|
ticket created: https://mattermost.atlassian.net/browse/MM-63216 |
|
There was a problem hiding this comment.
Hey @Willyfrog - based on Asaad's comments and your having created a separate ticket for the banner, I'm happy to go ahead and approve
lindy65
added
QA Review Done
and removed
3: QA Review
Willyfrog
force-pushed
the
edit_profile_cpa
branch
2 times, most recently
from
06043e9 to
c98bf5a
Compare
feat: Add support for custom profile attributes in edit profile refactor: Normalize whitespace in CustomAttribute type definition fix: Resolve type mismatch for customAttributes in UserInfo interface test: Add test for udpateCustomProfileAttributeValues method fix typing, submit changes to server missing files test: Add tests for CustomProfileField component fix naming fix imports fix feat: Add field_refs hook for managing field references feat: Make `setRef` ref parameter optional with default no-op implementation refactor: Replace CustomProfileField with useFieldRefs for profile form refactor: Optimize edit profile screen imports and custom attributes handling refactor: Move custom attributes logic to remote actions in user.ts address PR reviews test: Add tests for custom attributes in edit profile test: Add tests for EditProfile component with custom attributes fix: Add UserModel type assertion to currentUser in edit profile tests test: Add tests for ProfileForm custom attributes functionality test: Add comprehensive tests for useFieldRefs hook test: Add tests for fetchCustomAttributes and updateCustomAttributes add tests remove unneeded files review changes remove counter from hook remove package.resolved create interface for reuse of record
Willyfrog
force-pushed
the
edit_profile_cpa
branch
from
c98bf5a to
51f036b
Compare
|
I had to fiddle around to remove the commits and go back to the original code to make sure the latest changes from main are there |
Summary
Add custom profile attributes to user profile and be able to edit them.
Ticket Link
MM-62701
MM-62176
Checklist
E2E iOS tests for PR.Device Information
This PR was tested on: iOS emulator iPhone and iPad, Pixel 8
Screenshots
Release Note