Simplify computation of report name for certain types#81513
Simplify computation of report name for certain types#81513neil-marcellini merged 12 commits intoExpensify:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 390694dca4
ℹ️ 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".
neil-marcellini
left a comment
There was a problem hiding this comment.
Good start - a couple things.
src/libs/ReportNameUtils.ts
Outdated
|
|
||
| let allPersonalDetails: OnyxEntry<PersonalDetailsList>; | ||
|
|
||
| const staticReportNameTypes = [ |
There was a problem hiding this comment.
can we please call this CUSTOM_NAME_TYPES to match the backend? Or if you think that's too vague we could call it something that captures the idea that these report types have their report names computed by the backend according to the policy's report title formula
There was a problem hiding this comment.
suggest: BACKEND_GENERATED_NAME_TYPES
CUSTOM_NAME_TYPES is indeed to vague. This name is not informative for people that does not work on backend
There was a problem hiding this comment.
Still needs renaming
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 674ad74aea
ℹ️ 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".
src/libs/ReportNameUtils.ts
Outdated
| if (report?.reportName && shouldReturnStaticReportName(report)) { | ||
| return isArchivedNonExpense ? generateArchivedReportName(report?.reportName) : report?.reportName; | ||
| } |
There was a problem hiding this comment.
Preserve new‑dot invoice naming logic
For invoice reports, this early return now bypasses getInvoiceReportName, which intentionally ignores report.reportName for new‑dot invoices (it uses isNewDotInvoice(...) to compute a money‑request style name). If a new‑dot invoice still has a legacy reportName populated, the UI will now display that stale/old‑dot name instead of the computed payer/amount format. This shows up specifically for invoice reports whose chatReportID points to an invoice room (new‑dot) and reportName is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@neil-marcellini there is complex computation of name for invoice reports done later in computeReportName. I'm starting to doubt if our early check can be done so easily
There was a problem hiding this comment.
Also tests are failing with this check so there is something I'm missing
There was a problem hiding this comment.
As mentioned before, I'll leave only TYPE.EXPENSE on our early return check
|
(Neil's AI agent) @sosek108 Here are manual test steps for creating each custom name report type, to verify 1. Expense (
|
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for the investigation. It looks like we should only skip the computation for expense reports and call it good.
sosek108
left a comment
There was a problem hiding this comment.
Left only TYPE.EXPENSE in early return. Although tests are failing so I'll be investigating this
src/libs/ReportNameUtils.ts
Outdated
| if (report?.reportName && shouldReturnStaticReportName(report)) { | ||
| return isArchivedNonExpense ? generateArchivedReportName(report?.reportName) : report?.reportName; | ||
| } |
There was a problem hiding this comment.
As mentioned before, I'll leave only TYPE.EXPENSE on our early return check
neil-marcellini
left a comment
There was a problem hiding this comment.
I'm not sure if this was totally ready for review, but it looks like it's headed in the right direction.
We still need some naming changes, and then I don't even know if we need a separate function since we're only checking for one report type now. It should be easy to get this ready soon.
src/libs/ReportNameUtils.ts
Outdated
|
|
||
| let allPersonalDetails: OnyxEntry<PersonalDetailsList>; | ||
|
|
||
| const staticReportNameTypes = [ |
There was a problem hiding this comment.
Still needs renaming
@neil-marcellini Done. I'm not sure but somehow I did not pushed my changes and was sure I did. Sorry about that. I agree we don't need this additional function so I removed it and just check the type in place |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks, I think we can go ahead and merge this. I would like to see a follow-up for an additional optimization, but I don't want to block this any longer.
| return parentReportActionBasedName; | ||
| } | ||
|
|
||
| if (report?.reportName && report.type === CONST.REPORT.TYPE.EXPENSE) { |
There was a problem hiding this comment.
NAB: in a follow-up pull request, can we please move this check all the way to the top of the function for better optimization? I don't think we need the "is archived non-expense" check, because this is already within the check that the report type is an expense report.
There was a problem hiding this comment.
I agree, that indeed we don't need to check isArchivedNonExpenseReport so I'll move this after if (report?.reportName && report.type === CONST.REPORT.TYPE.EXPENSE) check.
But if we do our check before this:
if (parentReportActionBasedName) {
return parentReportActionBasedName;
}
unit tests will break as it's assumed that there are some special cases based on parent report's actions
|
simple change, No C+ review needed. |
|
🚧 @neil-marcellini 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! 🧪🧪
|
|
✋ 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.3.18-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.18-8 🚀
|


Explanation of Change
Add early return to computeReportName for report of types:
These types does not require frontend computation, as their value is stored in
reportNameFixed Issues
$ #81339
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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