[Part 1] Refactor formatPhoneNumber from NextStepUtils#87288
[Part 1] Refactor formatPhoneNumber from NextStepUtils#87288FitseTLT wants to merge 19 commits intoExpensify:mainfrom
Conversation
melvin-bot
Bot
requested review from
JmillsExpensify and
shubham1206agra
and removed request for
a team
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
FitseTLT
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1872caecdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Put expense on HOLD | ||
| */ | ||
| function putOnHold(transactionID: string, comment: string, initialReportID: string | undefined, isOffline: boolean, ancestors: Ancestor[] = []) { | ||
| function putOnHold(transactionID: string, comment: string, initialReportID: string | undefined, isOffline: boolean, ancestors: Ancestor[] = [], formatPhoneNumber: LocaleContextProps['formatPhoneNumber']) { |
There was a problem hiding this comment.
This change makes formatPhoneNumber mandatory for putOnHold/unholdRequest, but several existing callers still use the old arity (for example src/pages/iou/HoldReasonPage.tsx, src/pages/Search/SearchHoldReasonPage.tsx, src/libs/ReportUtils.ts, src/hooks/useSelectedTransactionsActions.ts, and src/hooks/useSearchBulkActions.ts). In those paths, buildNextStepNew() receives undefined and getDisplayNameForParticipant() will call formatPhoneNumber(...), which can throw when users hold or unhold expenses.
Useful? React with 👍 / 👎.
| hasViolations: boolean, | ||
| isASAPSubmitBetaEnabled: boolean, | ||
| reportCurrentNextStepDeprecated: OnyxEntry<OnyxTypes.ReportNextStepDeprecated>, | ||
| formatPhoneNumber: LocaleContextProps['formatPhoneNumber'], |
There was a problem hiding this comment.
Requiring formatPhoneNumber here (and in addReportApprover) breaks current call sites that were not updated, including src/pages/ReportChangeApproverPage.tsx, src/pages/Search/SearchChangeApproverPage.tsx, src/pages/ReportAddApproverPage.tsx, and src/pages/Search/SearchAddApproverPage.tsx. Those flows can now pass undefined into next-step generation, which will fail when formatting participant logins during change-approver actions.
Useful? React with 👍 / 👎.
| reportNextStep?: OnyxEntry<ReportNextStepDeprecated>; | ||
| policyCategories?: OnyxEntry<PolicyCategories>; | ||
| allTransactions: OnyxCollection<Transaction>; | ||
| formatPhoneNumber: LocaleContextProps['formatPhoneNumber']; |
There was a problem hiding this comment.
Making formatPhoneNumber required in changeTransactionsReport introduces a regression for existing callers that still pass the previous object shape (for example src/components/AddUnreportedExpenseFooter.tsx). That path now provides no formatter, so the deprecated next-step builder can crash while resolving participant display names during “move/add unreported expense” flows.
Useful? React with 👍 / 👎.
| hasViolationsParam, | ||
| recentlyUsedReportFields, | ||
| shouldFixViolations = false, | ||
| formatPhoneNumber, |
There was a problem hiding this comment.
This makes formatPhoneNumber required for updateReportField, but src/pages/EditReportFieldPage.tsx still calls it without that property. As a result, editing a report field can pass undefined into next-step building and fail when display-name formatting is executed.
Useful? React with 👍 / 👎.
|
This is ready for review @shubham1206agra |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@FitseTLT Keep the number of file changes small. It will be super hard to review 91 files |
But it is one function that lead to all these changes I don't how I can still narrow down the scope. U told me to check previous prs and they refactor a function per pr I did the same here. Both buildNextStepMessage, getNextApproverDisplayName have almost 2 file change all the other changes is related with buildNextStepNew I don't know how I could do it better. May be the best idea is for you (as a C+) to work with melvin. |
|
@FitseTLT Make the parameter as an optional parameter with default value being the deprecated function. Then, do the gradual migration. |
|
I think it is enough now. |
2 participants
Explanation of Change
Replaced
formatPhoneNumberin NextStepUtils from buildNextStepMessage, getNextApproverDisplayName, buildNextStepNew utils withformatPhoneNumberfrom useLocalize.Fixed Issues
$ #66327
PROPOSAL:
Tests
This is a refactoring PR so you should test that it doesn't create a new behavior on:
Offline tests
same as tests
QA Steps
same as tests
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
2026-04-08.00-29-22.mp4