-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Remove call to getReportNameValuePairs() in method getChatRoomSubtitle from ReportUtils.ts #68701
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
|
@DylanDylann 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] |
src/libs/OptionsListUtils.ts
Outdated
|
|
||
| hasMultipleParticipants = personalDetailList.length > 1 || result.isChatRoom || result.isPolicyExpenseChat || reportUtilsIsGroupChat(report); | ||
| subtitle = getChatRoomSubtitle(report, {isCreateExpenseFlow: true}); | ||
| subtitle = getChatRoomSubtitle(report, {isCreateExpenseFlow: true, isReportArchived: !!result.private_isArchived}); |
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.
In this function, could you please update to use isArchivedReport function for all places?
const isArchivedReport = isArchivedReport(reportNameValuePairs)
src/libs/ReportUtils.ts
Outdated
| return `${getReportSubtitlePrefix(report)}${translateLocal('iou.submitsTo', {name: subtitle ?? ''})}`; | ||
| } | ||
|
|
||
| // This will get removed as part of https://github.com/Expensify/App/issues/59961 |
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 will get removed as part of https://github.com/Expensify/App/issues/59961 |
src/libs/SidebarUtils.ts
Outdated
| const isExpense = isExpenseReport(report); | ||
| const hasMultipleParticipants = participantPersonalDetailList.length > 1 || result.isChatRoom || result.isPolicyExpenseChat || isExpense; | ||
| const subtitle = getChatRoomSubtitle(report); | ||
| const subtitle = getChatRoomSubtitle(report, {isReportArchived: !!result.private_isArchived}); |
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.
In this function, could you please update to use isArchivedReport function for all places?
const isArchivedReport = isArchivedReport(reportNameValuePairs)
src/pages/home/HeaderView.tsx
Outdated
| // Use sorted display names for the title for group chats on native small screen widths | ||
| const title = getReportName(reportHeaderData, policy, parentReportAction, personalDetails, invoiceReceiverPolicy); | ||
| const subtitle = getChatRoomSubtitle(reportHeaderData); | ||
| const subtitle = getChatRoomSubtitle(reportHeaderData, {isReportArchived}); |
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.
isReportArchived should be checked for reportHeaderData. The current isReportArchived is the check for report
src/libs/actions/Task.ts
Outdated
| subtitle = LocalePhoneNumber.formatPhoneNumber(login || displayName); | ||
| } else { | ||
| subtitle = ReportUtils.getChatRoomSubtitle(report) ?? ''; | ||
| const reportNameValuePair = reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID}`]; |
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.
In this function, please replace reportNameValuePairs param with isReportArchived param
src/pages/tasks/NewTaskPage.tsx
Outdated
| const [task] = useOnyx(ONYXKEYS.TASK, {canBeMissing: true}); | ||
| const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {canBeMissing: true}); | ||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {canBeMissing: false}); | ||
| const [reportNameValuePairs] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS, {canBeMissing: true}); |
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.
Please use useReportIsArchived hook
src/libs/ReportUtils.ts
Outdated
| * Get either the policyName or domainName the chat is tied to | ||
| */ | ||
| function getChatRoomSubtitle(report: OnyxEntry<Report>, config: GetChatRoomSubtitleConfig = {isCreateExpenseFlow: false}): string | undefined { | ||
| function getChatRoomSubtitle(report: OnyxEntry<Report>, config: GetChatRoomSubtitleConfig = {isCreateExpenseFlow: false, isReportArchived: false}): string | undefined { |
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.
Please add UTs for this function to cover new param
|
Hi @DylanDylann, I have updated the PR to address all your comments. Please check again. Thanks |
src/libs/SidebarUtils.ts
Outdated
| getWelcomeMessage(report, policy, localeCompare, isReportArchived).messageText || translateLocal('report.noActivityYet'), | ||
| ); | ||
| } | ||
| if (shouldShowLastActorDisplayName(report, lastActorDetails, lastAction) && !isArchivedReport(reportNameValuePairs)) { |
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.
using the new variable here
src/pages/home/HeaderView.tsx
Outdated
| const [lastDayFreeTrial] = useOnyx(ONYXKEYS.NVP_LAST_DAY_FREE_TRIAL, {canBeMissing: true}); | ||
| const [account] = useOnyx(ONYXKEYS.ACCOUNT, {canBeMissing: true}); | ||
| const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID}`, {canBeMissing: true}); | ||
| const [reportNameValuePairsForParentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${parentReport?.reportID}`, {canBeMissing: true}); |
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.
In the component, please use useReportIsArchived hook
src/pages/tasks/NewTaskPage.tsx
Outdated
| () => (task?.shareDestination ? getShareDestination(task.shareDestination, reports, personalDetails, localeCompare, isReportArchived) : undefined), | ||
| [task?.shareDestination, reports, personalDetails, localeCompare, isReportArchived], |
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.
task.shareDestination never be archived (isReportArchived will be always be false), so we don't need to update this place
|
Thanks @DylanDylann, I have updated the code |
|
The failed test is due to flaky test which was already mentioned here #68451
|
src/libs/actions/Task.ts
Outdated
| reports: OnyxCollection<OnyxTypes.Report>, | ||
| personalDetails: OnyxEntry<OnyxTypes.PersonalDetailsList>, | ||
| localeCompare: LocaleContextProps['localeCompare'], | ||
| isReportArchived = false, |
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.
Let's revert this
src/pages/ShareCodePage.tsx
Outdated
|
|
||
| return currentUserPersonalDetails.login; | ||
| }, [isReport, currentUserPersonalDetails.login, report, isReportArchived]); | ||
| }, [report, currentUserPersonalDetails, isReport, isReportArchived]); |
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.
Why do you update currentUserPersonalDetails.login to currentUserPersonalDetails
| }, [report, currentUserPersonalDetails, isReport, isReportArchived]); | |
| }, [report, currentUserPersonalDetails.login, isReport, isReportArchived]); |
|
Please update |
tests/unit/SidebarUtilsTest.ts
Outdated
| }); | ||
|
|
||
| expect(optionData?.alternateText).toBe(`test message`); | ||
| expect(optionData?.alternateText).toBe(`One: test message`); |
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.
@mkzie2 Why do you change the expect here?
| ); | ||
|
|
||
| expect(title).toBeCalledWith( | ||
| expect(title).toHaveBeenCalledWith( |
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.
Same question?
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.
Ahh It seems, toBeCalledWith is deprecated. Let's ignore this
| ); | ||
|
|
||
| expect(description).toBeCalledWith( | ||
| expect(description).toHaveBeenCalledWith( |
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.
Same
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.
Ahh It seems, toBeCalledWith is deprecated. Let's ignore this
|
Hey @DylanDylann, could you please check the PR again. Thank you |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2025-08-26.at.16.45.22.movAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
src/libs/OptionsListUtils.ts
Outdated
| result.isOptimisticPersonalDetail = personalDetail?.isOptimisticPersonalDetail; | ||
| if (report) { | ||
| const reportNameValuePairs = allReportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report.reportID}`]; | ||
| const isReportArchived = isArchivedReport(reportNameValuePairs); |
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 is the part I hate about the options utils... We already have result.private_isArchived below, which does exactly the same thing. So, for consistency, I like to keep using result.private_isArchived rather than having two variables for the same thing.
I think we should keep doing this until we really want to rename result.private_isArchived to result.isReportArchived, which is a whole lot more work.
I think the best suggestion I have for this is to modify below to change
result.private_isArchived = reportNameValuePairs?.private_isArchived;
to
result.private_isArchived = isReportArchived;
so that they are at least both referencing the same thing.
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.
@tgolen I also think like this
result.private_isArchived = isReportArchived;
But the type is different. And because result.private_isArchived will be outdated, so I suggest we avoid using it. That’s why we introduced the isReportArchived variable here.
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.
I think we should leave this code better than we found it and having duplicate variables only makes the code messier and harder to maintain (because someone might come and change one variable and not see the other).
But the type is different
How so? Is it because one is a boolean and the other is a string (timestamp)? Isn't it easy to update the types? I think we are never really using the string as an actual string, we are always prepending !! to it in order to treat it as a boolean. So, we're already kind of breaking the purpose of the type.
Ideally, we would completely rename result.private_isArchived to result.reportIsArchived. Without a plan to do that, I really don't think we should add this duplicate variable.
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.
result.private_isArchived is still used in many other places. We should handle this in a separate PR by removing all report.private_isArchived usages and replacing them with reportIsArchived.
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.
@mkzie2 In this PR, please update it to continue using private_isArchived in order to avoid duplication
src/libs/ReportUtils.ts
Outdated
| * Get either the policyName or domainName the chat is tied to | ||
| */ | ||
| function getChatRoomSubtitle(report: OnyxEntry<Report>, config: GetChatRoomSubtitleConfig = {isCreateExpenseFlow: false}): string | undefined { | ||
| function getChatRoomSubtitle(report: OnyxEntry<Report>, config: GetChatRoomSubtitleConfig = {isCreateExpenseFlow: false, isReportArchived: false}): string | undefined { |
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.
I think this config object is an anti-pattern, so please don't add any more properties to it and add isReportArchived as it's own parameter.
Of course, that leaves the config object with only a single property (hence, why it was an anti-pattern to begin with), so it would be nice for you to also move isCreateExpenseFlow to it's own parameter and change the name to something more appropriate like preferPolicyName. isCreateExpenseFlow makes this method too tightly coupled so it's difficult to reuse.
|
@DylanDylann Please help check it one more time. I have updated the PR to address comments |
src/libs/ReportUtils.ts
Outdated
| * Get either the policyName or domainName the chat is tied to | ||
| */ | ||
| function getChatRoomSubtitle(report: OnyxEntry<Report>, config: GetChatRoomSubtitleConfig = {isCreateExpenseFlow: false}): string | undefined { | ||
| function getChatRoomSubtitle(report: OnyxEntry<Report>, preferPolicyName = false, isReportArchived = false): string | undefined { |
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.
| function getChatRoomSubtitle(report: OnyxEntry<Report>, preferPolicyName = false, isReportArchived = false): string | undefined { | |
| function getChatRoomSubtitle(report: OnyxEntry<Report>, isPolicyNamePreferred = false, isReportArchived = false): string | undefined { |
This is Boolean type
tgolen
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.
That looks great now. Thank you!
tests/unit/ReportUtilsTest.ts
Outdated
| expect(result).toBe(policy.name); | ||
| }); | ||
|
|
||
| it('should handle partial config parameter with only isReportArchived', () => { |
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.
Some of these descriptions that mention "config parameter" need cleaned up now.
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.
Thanks for the review. I have updated them. Pls help check again @tgolen
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.0-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.0-5 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.0-5 🚀
|

Explanation of Change
Fixed Issues
$ #67095
PROPOSAL: #67095
Tests
Test Case 1:
Expected: Subtitle should display correctly (workspace name or room description)
Test Case 2:
Expected: Share page shows correct subtitle
Offline tests
QA Steps
Same as Tests part
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-1.mov
Android: mWeb Chrome
web-android.mov
web-android-1.mov
iOS: Native
ios.mov
ios-2.mov
iOS: mWeb Safari
web-ios-2.mov
web-ios.mov
MacOS: Chrome / Safari
web-2.mov
web.mov
MacOS: Desktop
desktop.mov
desktop-1.mov