-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Remove deprecated report name functions in ReportUtils (Part 1) #77614
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
Conversation
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.
|
|
@eVoloshchak 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] |
waterim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, code-wise looks good.
Thanks for splitting this, reviews are much easier now
grgia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly looks good, a few minor comments
| @@ -1,5 +1,5 @@ | |||
| import type {OnyxEntry} from 'react-native-onyx'; | |||
| import {computeReportName} from '@libs/ReportNameUtils'; | |||
| import {computeReportNameWithoutFormula} from '@libs/ReportNameUtils'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function name isn't intuitive, why rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm fine to rename it, but in reading it here, computeReportNameWithoutFormula is not clear. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grgia bth, both names are ok to me. The current name is a bit more specific and I would prefer to keep the current one. However, I'm happy to hear others' thoughts
|
Checking it now... |
|
Conflicts and failed tests are resolved |
|
No product change. Removing product reviewer |
neil-marcellini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good thanks. Before we merge we need to fix conflicts.
I agree with @grgia that the name isn't very intuitive.
computeReportNameWithoutFormulaimplies the existence ofcomputeReportName, but if we're just renaming it, this will be the onlycomputeReportName, so it's kind of implied it's without formula.
That makes sense to me, let's leave it as computeReportName. We will be calling Formula.compute to generate the optimistic report names upon creation soon with this issue Optimistically compute report names on creation, but I think it's clear what the difference is.
Also, if we don't rename the function the diff will be way smaller which is good.
9e97f63 to
b645eea
Compare
|
|
b645eea to
aaffc34
Compare
|
Sorry for the force push, I have to force push to solve the un-signed commit issue |
|
@lorretheboy is this ready for another review now? |
|
@neil-marcellini Yes, I'm waiting for pipelines to ping for review again. Once all done, it is good to re-review |
neil-marcellini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks!
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-02.at.11.52.13.movAndroid: mWeb ChromeScreen.Recording.2026-01-02.at.11.54.21.moviOS: HybridAppScreen.Recording.2026-01-02.at.11.44.05.moviOS: mWeb SafariScreen.Recording.2026-01-02.at.11.47.37.movMacOS: Chrome / SafariScreen.Recording.2026-01-02.at.11.41.14.mov |
eVoloshchak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
✋ 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/neil-marcellini in version: 9.2.92-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.92-1 🚀
|
Remove deprecated report name functions in ReportUtils (Part 1)

Explanation of Change
Scope of changes:
Renamed
computeReportNametocomputeReportNameWithoutFormulaDeleted functions:
Fixed Issues
$ #76852
PROPOSAL: #76852 (comment)
Tests
Test 01:
Test 02:
Test 03:
Test 04:
Offline 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))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.mov
Android: mWeb Chrome
WEBSITE.ANDROID.mov
iOS: Native
IOS.mov
iOS: mWeb Safari
WEBSITE.IOS.mov
MacOS: Chrome / Safari
WEBSITE-04.mov
WEBSITE-03.mov
WEBSITE-03.mov
WEBSITE-04.mov