-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Remove call to "getReportNameValuePairs()" in method "getPolicyExpenseChatName" from "ReportUtils.ts". #68465
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
…eChatName" from "ReportUtils.ts". Signed-off-by: krishna2323 <belivethatkg@gmail.com>
src/libs/ReportUtils.ts
Outdated
| // This will get removed as part of https://github.com/Expensify/App/issues/59961 | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| if (isArchivedNonExpenseReport(report, !!getReportNameValuePairs(report?.reportID)?.private_isArchived)) { | ||
| if (isArchivedNonExpenseReport(report, 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.
@DylanDylann, can you help me with the testing steps if you're familiar with the scenario mentioned above? I’ve been trying to make the condition (archiveReason === CONST.REPORT.ARCHIVE_REASON.ACCOUNT_MERGED && policyExpenseChatRole !== CONST.POLICY.ROLE.ADMIN) evaluate to true, but haven’t succeeded yet.
Here’s what I’ve tried:
- Created two different accounts (Account A and Account B).
- Added Account A as a member in a workspace created by Account B.
- Merged Account B into Account A.
Despite this, the condition isn’t being met. I’ve already spent a good amount of time on this, so it would be great if you could help. Thanks!
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 don't need to pass isReportArchived here because it's for existing messages.
Could you please detail the reason here? It could be a moved report action in an archived workspace chat
@DylanDylann I need to test this scenario to confirm and provide a proper explanation. What I meant was that we shouldn't modify already existing messages if the workspace was archived afterward.
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.
@Krishna2323 Please check this PR: #14779
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.
@DylanDylann Even after carefully following the test steps from this PR, I couldn’t reach the condition below (line 3806). The reason is that reportOwnerDisplayName already evaluates to true, and even when I tried commenting out the condition, the archiveReason (line 3808) returns default instead of CONST.REPORT.ARCHIVE_REASON.ACCOUNT_MERGED.
Lines 3802 to 3812 in 1ef115b
| // If this user is not admin and this policy expense chat has been archived because of account merging, this must be an old expense chat | |
| // of the account which was merged into the current user's account. Use the name of the policy as the name of the report. | |
| // This will get removed as part of https://github.com/Expensify/App/issues/59961 | |
| // eslint-disable-next-line deprecation/deprecation | |
| if (isArchivedNonExpenseReport(report, !!getReportNameValuePairs(report?.reportID)?.private_isArchived)) { | |
| const lastAction = getLastVisibleActionReportActionsUtils(report?.reportID); | |
| const archiveReason = isClosedAction(lastAction) ? getOriginalMessage(lastAction)?.reason : CONST.REPORT.ARCHIVE_REASON.DEFAULT; | |
| if (archiveReason === CONST.REPORT.ARCHIVE_REASON.ACCOUNT_MERGED && policyExpenseChatRole !== CONST.POLICY.ROLE.ADMIN) { | |
| return getPolicyName({report, policy, policies, reports}); | |
| } | |
| } |
Lines 3788 to 3793 in 1ef115b
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const reportOwnerDisplayName = getDisplayNameForParticipant({accountID: ownerAccountID, shouldRemoveDomain: true}) || login; | |
| if (reportOwnerDisplayName) { | |
| return translateLocal('workspace.common.policyExpenseChatName', {displayName: reportOwnerDisplayName}); | |
| } |
Line 3808 in 1ef115b
| const archiveReason = isClosedAction(lastAction) ? getOriginalMessage(lastAction)?.reason : CONST.REPORT.ARCHIVE_REASON.DEFAULT; |
NOTE: Chat Report (archived) in the video is shown because I commented out the first condition where reportOwnerDisplayName evaluates to true.
Monosnap.screencast.2025-08-20.00-06-10.mp4
|
@DylanDylann App/src/libs/ModifiedExpenseMessage.ts Lines 133 to 153 in 9874e84
|
@Krishna2323 Could you please detail the reason here? It could be a moved report action in an archived workspace chat |
src/libs/ReportUtils.ts
Outdated
| // This will get removed as part of https://github.com/Expensify/App/issues/59961 | ||
| // eslint-disable-next-line deprecation/deprecation |
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 | |
| // eslint-disable-next-line deprecation/deprecation |
|
From this comment, after check it more detail, this is my summary In this PR, we need to remove call to "getReportNameValuePairs()" in the method "getPolicyExpenseChatName". Lines 3805 to 3816 in 4d7d44a
This logic was introduced by this PR (long time ago, so I am not sure the behavior at that time) and in this PR we expect that:
Recently, we had an update on how to display the workspace name to |
|
Hm, it still sounds to me like we would display the policy name for that archived report. I don't understand why changing the workspace name to |
In this update, we changed the condition from Additionally, there’s no reason to use the policy name for non-expense reports. Lines 3805 to 3816 in 4d7d44a
|
|
Aha! Thank you. Makes total sense now. Let's remove it completely then. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@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/ReportUtils.ts
Outdated
| report, | ||
| !!reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`]?.private_isArchived, | ||
| ); | ||
| const isReportArchived = !!reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`]?.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.
Please use isArchivedReport function
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.
We already have a function named isArchivedReport.
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.
@Krishna2323 Use this function please
src/libs/ReportUtils.ts
Outdated
| policies?: SearchPolicy[]; | ||
| reports?: SearchReport[]; | ||
| }): string | undefined { | ||
| function getPolicyExpenseChatName({report}: {report: OnyxEntry<Report>}): 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 some unit tests for this function
| } | ||
|
|
||
| if (isPolicyExpenseChat(report)) { | ||
| formattedName = getPolicyExpenseChatName({report, policy, personalDetailsList: personalDetails, reports}); |
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 remove personalDetailsList param 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.
sorry, it was a mistake. Thanks for catching that 🙇🏻
|
@Krishna2323 Kindly bump |
|
@DylanDylann my dog Daisy passed away last night 💔. I’m not in a state to work today, thank you for understanding. |
|
@Krishna2323 My heartfelt sympathy. I hope you can get through |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
src/libs/ReportUtils.ts
Outdated
| report, | ||
| !!reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`]?.private_isArchived, | ||
| ); | ||
| const isReportArchived = !!reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`]?.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.
@Krishna2323 Use this function please
src/libs/ReportUtils.ts
Outdated
| report, | ||
| !!reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`]?.private_isArchived, | ||
| ); | ||
| const isReportArchived = !!reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`]?.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.
const isReportArchived = isArchivedReport(reportNameValuePairs?.[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? String(CONST.DEFAULT_NUMBER_ID)}`])
|
And Please add unit test for getPolicyExpenseChatName function |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Reviewer Checklist
Screenshots/VideosScreen.Recording.2025-08-26.at.14.39.51.movAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ 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/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
$ #67093
PROPOSAL: #67093
Tests
(archived)Offline tests
(archived)QA Steps
(archived)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))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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4