refactor: Remove deprecated report name functions in ReportUtils (Part 4)#84485
refactor: Remove deprecated report name functions in ReportUtils (Part 4)#84485mohammadjafarinejad wants to merge 15 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Hey @sosek108, quick question on this refactor The new If yes, should we make it required to enforce correct usage, or keep it optional to allow graceful degradation for call sites that don't have easy access to it? // required
function testCall(reportAttributes: ReportAttributesDerivedValue['reports']) {}
// optional
function testCall(reportAttributes?: ReportAttributesDerivedValue['reports']) {}
function testCall(reportAttributes: ReportAttributesDerivedValue['reports'] | undefined) {} |
|
@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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44f626279b
ℹ️ 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".
…SearchTypeMenu, BrowserNotifications, and SavedSearchList components
JmillsExpensify
left a comment
There was a problem hiding this comment.
All looks good for product.
Reviewer Checklist
Screenshots/Videos |
|
@mohammadjafarinejad can you resolve conflicts? And also eslint is failing |
src/libs/actions/User.ts
Outdated
|
|
||
| let reportAttributes: ReportAttributesDerivedValue | undefined; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, |
There was a problem hiding this comment.
Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
We should be able to pass reportAttributes from the function call on AuthScreen (useReportAttributes hook)
There was a problem hiding this comment.
Updated.
|
@eVoloshchak Ready for re-review. |
neil-marcellini
left a comment
There was a problem hiding this comment.
Good to go. Thank you. We do need to fix merge conflicts first. Let me know when that's fixed.
|
@neil-marcellini Ready for re-review. |
|
@mohammadjafarinejad, this branch has conflicts that must be resolved |
|
@mohammadjafarinejad can you please work on the merge conflicts? |
|
Asking @sosek108 if he can fix conflicts since @mohammadjafarinejad hasn't responded in a while |
|
@neil-marcellini Sorry for the delay, I’ll work on it now. |
sosek108
left a comment
There was a problem hiding this comment.
you left behind unused useEffect dep. Please remove this
| ); | ||
| setAutocompleteSubstitutions(substitutionsMap); | ||
| }, [allFeeds, personalAndWorkspaceCards, originalInputQuery, personalDetails, reports, taxRates, policies, currentUserAccountID, translate, conciergeReportID]); | ||
| }, [allFeeds, personalAndWorkspaceCards, originalInputQuery, personalDetails, reports, taxRates, policies, currentUserAccountID, translate, conciergeReportID, reportAttributes]); |
There was a problem hiding this comment.
conciergeReportID is not used anymore
| const feedKeysWithCards = useFeedKeysWithAssignedCards(); | ||
| const {inputQuery: originalInputQuery} = queryJSON; | ||
| const [currentUserAccountID = -1] = useOnyx(ONYXKEYS.SESSION, {selector: accountIDSelector}); | ||
| const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID); |
There was a problem hiding this comment.
conciergeReportID is not used anymore
|
@mohammadjafarinejad There are unfortunately another conflicts :/ |
|
@mohammadjafarinejad @neil-marcellini because there are more and more merge conflicts can we plan a time slot where we resolve them, quickly test & review them, and then merge them into main? |
|
Yes, sounds good. I started a group chat in Slack to coordinate timings. |
neil-marcellini
left a comment
There was a problem hiding this comment.
A bit more clean up is needed from resolving the merge conflict.
| @@ -843,6 +873,154 @@ describe('actions/Report', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should pass conciergeReportID through to showReportActionNotification when provided', () => { | |||
There was a problem hiding this comment.
This test no longer has a purpose. I suggest we remove it.
There was a problem hiding this comment.
Let's also remove all the other "pass conciergeReportID" tests below.
There was a problem hiding this comment.
Same here, a lot of these tests don't make sense anymore.
|
Hubert is available now and so am I, and he cannot push to your fork for this PR, so we have created a new PR and we're moving forward there to try to get this merged today. |

Explanation of Change
Migrates non-component call sites from the deprecated
ReportUtils.getReportNametoReportNameUtils.getReportName, passingreport(and optionallyreportAttributes) directly as arguments.Fixed Issues
$ #76852
PROPOSAL:
Tests
1. Task Share Destination (
Task.ts)getShareDestinationusesgetReportNameto display the name of the report a task is being shared/assigned to.Test Steps:
2. Browser/Desktop Notification — Room or Group Chat Comment (
BrowserNotifications.ts,LocalNotification/index.ts)pushReportCommentNotificationusesgetReportNameas the notification title for rooms/group chats.Test Steps:
3. Modified Expense Notification — Expense Moved Between Reports (
ModifiedExpenseMessage.ts,BrowserNotifications.ts)getMovedFromOrToReportMessageusesgetReportNamefor the "moved from report" text in the notification.Test Steps:
undefined, blank, or a raw report ID).4. Search — "In" Filter Display Value (
SearchQueryUtils.ts,SearchPageHeaderInput.tsx,buildSubstitutionsMap.ts)getFilterDisplayValueforfilterName === CONST.SEARCH.SYNTAX_FILTER_KEYS.INusesgetReportNameto resolve a report ID to a human-readable name.buildUserReadableQueryStringandbuildSubstitutionsMapnow propagatereportAttributes.Test Steps:
in:and select a specific report/room from the autocomplete suggestions.in:123456789).in: My Room) instead of the raw ID.#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 theMoneyRequestReportPreviewcomponent (the preview card visible in the 1:1 chat).Test Steps:
reportName, verify that custom name is shown.6. Pusher-Triggered Notifications (
User.ts,Report/index.ts)triggerNotificationsinUser.tsnow passesreportAttributes?.reportstoshowReportActionNotification, which then threads it through to bothshowCommentNotificationandshowModifiedExpenseNotification.Test Steps:
Offline tests
Same as tests.
QA Steps
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
Tests
Task.ts
BrowserNotifications.ts / LocalNotification/index.ts
ModifiedExpenseMessage.ts
SearchQueryUtils.ts/SearchPageHeaderInput.tsx/buildSubstitutionsMap.ts