[No QA] chore/refactor user avatar utils#73868
Conversation
grgia
left a comment
There was a problem hiding this comment.
I know this is a draft, just a few minor thoughts
| function isLetterAvatar(originalFileName?: string): boolean { | ||
| return !!(originalFileName && LETTER_AVATAR_NAME_REGEX.test(originalFileName)); | ||
| } |
There was a problem hiding this comment.
note:
For defaults/web we can also check if the path is in web static
There was a problem hiding this comment.
you mean to check the avatarUrl if it contains LETTER_AVATAR_NAME_REGEX ?
The issue is that it does not retain the OG file name :( always comes as sth like
https://d1wpcgnaa73g0y.cloudfront.net/2445d8267399f0ae3149d9f3237cccd62444449f.png
Would be soo much easier if it did 😢
| @@ -0,0 +1,275 @@ | |||
| import * as defaultAvatars from '@components/Icon/DefaultAvatars'; | |||
There was a problem hiding this comment.
given this is not fixing anything, is it better to push the passing tests in one PR, and then push this PR to prove it doesn't change the logic?
There was a problem hiding this comment.
oh I see these were just moved
There was a problem hiding this comment.
Yup, moved + added some test cases
src/libs/UserAvatarUtils.ts
Outdated
| * getCustomAvatarURL(CONST.ACCOUNT_ID.CONCIERGE) | ||
| * // => CONST.CONCIERGE_ICON_URL | ||
| */ | ||
| function getCustomAvatarURL(accountID: number = CONST.DEFAULT_NUMBER_ID, accountEmail?: string, avatarURL?: string): string { |
There was a problem hiding this comment.
Should we be changing this from getDefault? I wonder if instead we should separate the generator from the getter. The old getDefaultAvatarURL seems to mostly have been used for optimistic generation.
WDYT?
|
LGTM 👍 Thank you for your hard work! |
| } | ||
|
|
||
| const isUsingDefaultAvatar = (!imageData.uri && isDefaultOrCustomDefaultAvatar(currentUserPersonalDetails?.avatar, currentUserPersonalDetails?.originalFileName)) || !!selected; | ||
| const isUsingDefaultAvatar = (!imageData.uri && (isCustomAvatar(currentUserPersonalDetails?.avatar) || isLetterAvatar(currentUserPersonalDetails?.originalFileName))) || !!selected; |
There was a problem hiding this comment.
I think we should rename isUsingDefaultAvatar.
Yes it should be in this grouping, but since this is still causing confusing we should rename @jmusial
|
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…sify-app-fork into chore/refactor-user-avatar-utils
|
@grgia changed |
|
that's a great name choice, happy with that |
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Code looks good. Start testing now |
|
@jmusial can you please resolve the conflict? |
grgia
left a comment
There was a problem hiding this comment.
Triggering a final build, that rename choice made everything 10x clearer. Great work here 🤞
|
@jmusial actually ill trigger it once you merge main, ping me! |
|
@grgia @dukenv0307 done |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-11-07.at.21.59.54.movAndroid: mWeb ChromeScreen.Recording.2025-11-07.at.16.50.48.moviOS: HybridAppScreen.Recording.2025-11-07.at.21.49.27.moviOS: mWeb SafariScreen.Recording.2025-11-07.at.15.03.02.movMacOS: Chrome / SafariScreen.Recording.2025-11-07.at.14.54.42.movMacOS: DesktopScreen.Recording.2025-11-07.at.16.55.05.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.2.47-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.2.47-1 🚀
|
Explanation of Change
Cleanup refactor.
UserAvatarUtilsfromUserUtilsFixed Issues
$ #73221
PROPOSAL:
Depends on
#73412
Tests
Offline tests
n/a
QA Steps
n/a
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
0072.android.native.mov
Android: mWeb Chrome
0072.android.chrome.mov
iOS: Native
0072.ios.native.mov
iOS: mWeb Safari
0072.ios.safari.mov
MacOS: Chrome / Safari
0072.desktop.chrome.mov
MacOS: Desktop
0072.desktop.native.mov