refactor getOptionData and sortReportsToDisplayInLHN to use conciergeReportID from useOnyx#80977
Conversation
…ReportID from useOnyx
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
melvin-bot
bot
requested review from
DylanDylann and
heyjennahay
and removed request for
a team
|
@DylanDylann 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Use ReportNameUtils.computeReportName for full name generation. | ||
| * For reading a stored name only, use ReportNameUtils.getReportName. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/max-params |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
ESLint rule disable lacks justification. While the comment states that max-params is being disabled, it does not explain WHY the function requires more than the maximum allowed parameters or why this exception is acceptable.
Suggested fix: Add a comment explaining the reason:
// eslint-disable-next-line @typescript-eslint/max-params -- getReportName requires many optional parameters for backwards compatibility and full report name generation
function getReportName(Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
getReportName is used in too many places so I don't want to refactor it here
There was a problem hiding this comment.
@dukenv0307 Hmm, I don't think this is the right approach. We shouldn't allow a function to have too many parameters. That said, I noticed this function is deprecated, so I'm wondering whether it's even worth refactoring
There was a problem hiding this comment.
How much effort would it take to add a conciergeReportID param to getReportName? If it's significant, I think we should hold it. On the other hand, I'm fine with the current change
There was a problem hiding this comment.
@DylanDylann A lot of changes for sure, getReportName is used in too many places
There was a problem hiding this comment.
Because this function already been deprecated and should be removed soon, so let's leave this as with disable /max-params rule
![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: 12024bbe3b
ℹ️ 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".
| isReportArchived?: boolean, | ||
| reports?: Report[], | ||
| policies?: Policy[], | ||
| conciergeReportID: string | undefined = onboarding?.chatReportID, | ||
| ): string { |
There was a problem hiding this comment.
The new default conciergeReportID = onboarding?.chatReportID changes behavior for every caller that doesn’t pass this argument: getReportName will now treat the onboarding chat report as the concierge chat. The in-file comment for isChatUsedForOnboarding notes that onboarding.chatReportID historically pointed to a system DM for some older accounts, so this change will mislabel that system DM as “Concierge” (and trigger concierge-only logic) whenever getReportName is used without an explicit concierge ID. That’s a regression in report naming and LHN ordering for those accounts; consider keeping the previous concierge ID source (ONYXKEYS.CONCIERGE_REPORT_ID / conciergeReportIDOnyxConnect) as the default or only using onboarding.chatReportID when you know it’s the concierge chat.
Useful? React with 👍 / 👎.
|
@dukenv0307 Please check codecov comment : #80977 (comment) |
|
@DylanDylann Done! |
Reviewer Checklist
Screenshots/Videos
|
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #66411 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@dukenv0307 Please resolve the conflict when you're avaialable |
|
cc @tgolen |
| const [reportsDrafts, {sourceValue: reportsDraftsUpdates}] = useOnyx(ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT, {canBeMissing: true}); | ||
| const [betas] = useOnyx(ONYXKEYS.BETAS, {canBeMissing: true}); | ||
| const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {selector: reportsSelector, canBeMissing: true}); | ||
| const [conciergeReportID = ''] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID, {canBeMissing: true}); |
There was a problem hiding this comment.
A lot of places in the code use CONST.DEFAULT_NUMBER_ID. Is it really necessary to default this to an empty string? Can the code be updated to handle undefined instead?
There was a problem hiding this comment.
I made conciergeReportID a string before to ensure no one forgets to pass this value. But I think that's fine if it's undefined because we just use it for comparison with reportID
There was a problem hiding this comment.
Just updated the PR
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
That looks great. Thank you!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @tgolen 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, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.3.22-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.22-4 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.22-4 🚀
|
4 participants
Explanation of Change
Fixed Issues
$ #66411
PROPOSAL:
Tests
Offline tests
QA Steps
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
Screen.Recording.2026-01-30.at.08.40.06.mov