Fix different invoice report name in report preview #79819
Conversation
|
@mkhutornyi 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] |
![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: 94eb0975b0
ℹ️ 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".
| style={[styles.headerText]} | ||
| testID="MoneyRequestReportPreview-reportName" | ||
| > | ||
| {reportPreviewName} | ||
| {getReportName(iouReport, reportAttributes)} | ||
| </Text> |
There was a problem hiding this comment.
Using getReportName(iouReport, reportAttributes) drops the previous fallback to action.childReportName. getReportName returns an empty string when iouReport is undefined or has no reportID, which happens when the report hasn’t loaded yet or the user lacks access (shouldShowAccessPlaceHolder), resulting in a blank header where a report name should appear. Consider keeping the action-level fallback so the preview remains identifiable in those states.
Useful? React with 👍 / 👎.
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.
|
| import Performance from '@libs/Performance'; | ||
| import {getConnectedIntegration, hasDynamicExternalWorkflow} from '@libs/PolicyUtils'; | ||
| import {hasPendingDEWSubmit} from '@libs/ReportActionsUtils'; | ||
| import {getInvoicePayerName} from '@libs/ReportNameUtils'; |
There was a problem hiding this comment.
This might cause unexpected regression. I remember a few deploy blockers because of this migration.
Let's keep original logic of using deprecated getReportName from ReportUtils.
The refactor work is not part of our issue. (We can do as follow up if needed)
There was a problem hiding this comment.
Fair point. Updated to use deprecated getReportName with the same signature as this.
Is it possible to add unit tests? |
| testID="MoneyRequestReportPreview-reportName" | ||
| > | ||
| {reportPreviewName} | ||
| {getReportName(iouReport, undefined, undefined, undefined, undefined, reportAttributes)} |
There was a problem hiding this comment.
// This will be fixed as follow up https://github.com/Expensify/App/pull/75357
// eslint-disable-next-line @typescript-eslint/no-deprecated
Add this comment
|
Updated the existing test |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safariweb.mov |
|
@mkhutornyi done |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
@joekaufmanexpensify I see you on the parent issue. I guess this is fine for now while invoices aren't really being prioritised, but I think we honour the workspace setting's default report title formula for invoice type reports as well on Classic. In an ideal world, someone should be able to configure that per type, like report fields and what not.
Yeah, added back the fallback |
4 participants
Explanation of Change
Fixed Issues
$ #78805
PROPOSAL: #78805 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
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