refactor: Remove deprecated report name functions in ReportUtils (Part 4) - Hubert's fork#85975
Conversation
…SearchTypeMenu, BrowserNotifications, and SavedSearchList components
…and update related calls
melvin-bot
bot
requested review from
eVoloshchak
and removed request for
a team
|
@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] |
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
looking pretty good, just a few tests that should be updated I think
| it('calls initializePusher when SIGN_IN_MODAL is active and conciergeReportID is loaded', async () => { | ||
| mockedIsActiveRoute.mockReturnValue(true); | ||
|
|
||
| const conciergeReportID = '67890'; | ||
|
|
||
| await Onyx.merge(ONYXKEYS.SESSION, {accountID: TEST_ACCOUNT_ID, email: 'test@test.com'}); | ||
| await Onyx.merge(ONYXKEYS.CONCIERGE_REPORT_ID, conciergeReportID); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| renderAuthScreensInitHandler(); | ||
| await waitForBatchedUpdatesWithAct(); | ||
|
|
||
| // subscribeToUserEvents should be called with conciergeReportID from both mount and sign-in modal effects | ||
| expect(subscribeToUserEvents).toHaveBeenCalledWith(TEST_ACCOUNT_ID, expect.any(Function)); | ||
| }); | ||
|
|
||
| it('does not call initializePusher from sign-in modal effect when conciergeReportID is still loading', async () => { | ||
| mockedIsActiveRoute.mockReturnValue(true); | ||
|
|
||
| await Onyx.merge(ONYXKEYS.SESSION, {accountID: TEST_ACCOUNT_ID, email: 'test@test.com'}); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| renderAuthScreensInitHandler(); | ||
| await waitForBatchedUpdatesWithAct(); | ||
|
|
||
| // The mount effect calls subscribeToUserEvents with undefined conciergeReportID | ||
| expect(subscribeToUserEvents).toHaveBeenCalledWith(TEST_ACCOUNT_ID, expect.any(Function)); | ||
| }); |
There was a problem hiding this comment.
Sorry if I was unclear before, but I don't think we should remove these tests. I think they're just using Concierge Report ID as an example report to test certain things, so let's figure out a better way to test this.
Also, in the name of the test or the description, it talks about initialize pusher, and I think that's synonymous with subscribe to user events.
![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: 5f92362835
ℹ️ 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".
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.
|
|
We previously had a C+ review, and pretty sure the changes since then are fine, so we can merge this. |
|
🚧 @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.42-0 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
Migrates non-component call sites from the deprecated ReportUtils.getReportName to ReportNameUtils.getReportName, passing report (and optionally reportAttributes) directly as arguments.
Task.ts
BrowserNotifications.ts
ModifiedExpenseMessage.ts
SearchQueryUtils.ts
MoneyRequestReportPreviewContent.tsx
MoneyRequestReportPreview.test.tsx
Fixed Issues
$ #76852
PROPOSAL:
Tests
getShareDestination uses getReportName to display the name of the report a task is being shared/assigned to.
Test Steps:
Open the app and navigate to a workspace with multiple reports.
Create a new task (e.g., from the + menu → Task).
Tap the Assignee or Share destination field.
Verify report names appear correctly in the destination list (e.g., workspace expense chats, DMs, group rooms).
Assign the task to a workspace expense chat and submit.
Verify the task is created with the correct destination name shown.
2. Browser/Desktop Notification — Room or Group Chat Comment (BrowserNotifications.ts, LocalNotification/index.ts)
pushReportCommentNotification uses getReportName as the notification title for rooms/group chats.
Test Steps:
Use two browser tabs logged in as two different accounts (or use two devices).
Open a Group Chat or a Room (#admins, #announce, etc.) in Account A.
From Account B, send a message into that same group chat or room.
On Account A's desktop/web, verify a browser notification pops up.
Verify the notification title matches the exact room/group chat name as displayed in the LHN.
3. Modified Expense Notification — Expense Moved Between Reports (ModifiedExpenseMessage.ts, BrowserNotifications.ts)
getMovedFromOrToReportMessage uses getReportName for the "moved from report" text in the notification.
Test Steps:
Create an expense and submit it to a workspace report (Report A).
While logged in as an admin on another browser/device, move that expense to a different report (Report B).
On either account, open Report A and scroll to find the system/automated message about the moved expense.
Verify the message reads something like "Moved from [Report A name]" — confirming the report name resolves correctly (not undefined, blank, or a raw report ID).
Open Report B and verify that a corresponding message appears there as well.
4. Search — "In" Filter Display Value (SearchQueryUtils.ts, SearchPageHeaderInput.tsx, buildSubstitutionsMap.ts)
getFilterDisplayValue for filterName === CONST.SEARCH.SYNTAX_FILTER_KEYS.IN uses getReportName to resolve a report ID to a human-readable name. buildUserReadableQueryString and buildSubstitutionsMap now propagate reportAttributes.
Test Steps:
Open the Search page and click/focus the search input.
Type in: and select a specific report/room from the autocomplete suggestions.
Verify the report name appears inline in the search input as styled text (not a raw report ID like in:123456789).
Submit the search.
Verify the query display bar below the input shows the human-readable report name (e.g., in: My Room) instead of the raw ID.
Navigate away (e.g., open a report) and return to Search via the LHN — verify the query bar still shows the correct report name.
Repeat with a #room, a DM, and a group chat to cover different report type name resolutions.
5. Money Request Report Preview Header (MoneyRequestReportPreviewContent.tsx)
getReportName(iouReport, reportAttributes) is used as the heading text of the MoneyRequestReportPreview component (the preview card visible in the 1:1 chat).
Test Steps:
Open a 1:1 chat where an IOU/expense report exists.
Locate the MoneyRequestReportPreview card in the chat thread.
Verify the header shows the correct IOU/expense report name (e.g., "Expense Report" or a custom report name).
Open a workspace-based expense report and find its preview card — verify the name matches the report's actual name.
If the report has a custom reportName, verify that custom name is shown.
6. Pusher-Triggered Notifications (User.ts, Report/index.ts)
triggerNotifications in User.ts now passes reportAttributes?.reports to showReportActionNotification, which then threads it through to both showCommentNotification and showModifiedExpenseNotification.
Test Steps:
Have two accounts open simultaneously.
From Account B, send a message in a Room chatting with Account A.
Verify Account A receives a browser notification with the correct room name as the title.
From Account B, send a message in a 1:1 DM with Account A.
Verify Account A's notification title shows the sender's name (not the room name path).
Verify that no errors appear in the JS console
Offline tests
Same as tests.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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