fix: un-report the transactions on the report (move to selfDM) when delete #82619
fix: un-report the transactions on the report (move to selfDM) when delete #82619luacmartins merged 9 commits intoExpensify:mainfrom
Conversation
| @@ -824,7 +824,15 @@ function bulkDeleteReports( | |||
| const selectedItem = selectedTransactions[key]; | |||
There was a problem hiding this comment.
First pass: Collect all report IDs that are being deleted
Second pass: Collect transaction IDs, but exclude any transactions whose reportID is in the list of reports being deleted
There was a problem hiding this comment.
Can you add comments for the two loops?
|
|
||
| for (const key of Object.keys(selectedTransactions)) { | ||
| const selectedItem = selectedTransactions[key]; | ||
| if (selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The condition selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID is duplicated across both loops (first used to collect report IDs, then used as a continue guard in the second loop). The iteration over Object.keys(selectedTransactions) and the const selectedItem = selectedTransactions[key] lookup are also duplicated.
Since the two-pass approach is needed (the second loop depends on the completed reportIDList), you can still eliminate the duplication by using a Set of report keys from the first loop, then simply checking membership in the second loop instead of re-evaluating the same condition:
const reportIDSet = new Set<string>();
for (const key of Object.keys(selectedTransactions)) {
const selectedItem = selectedTransactions[key];
if (selectedItem.action === CONST.SEARCH.ACTION_TYPES.VIEW && key === selectedItem.reportID) {
reportIDSet.add(selectedItem.reportID);
}
}
const reportIDList = Array.from(reportIDSet);
for (const key of Object.keys(selectedTransactions)) {
if (reportIDSet.has(key)) {
continue;
}
const selectedItem = selectedTransactions[key];
if (\!selectedItem.reportID || \!reportIDSet.has(selectedItem.reportID)) {
transactionIDList.push(key);
}
}This removes the duplicated condition and the duplicated variable lookup, and as a bonus, using a Set makes the .includes() check O(1) instead of O(n).
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
eh2077
left a comment
There was a problem hiding this comment.
@lorretheboy thanks for the PR! Please also add unit tests to cover the change 🙇
| @@ -824,7 +824,15 @@ function bulkDeleteReports( | |||
| const selectedItem = selectedTransactions[key]; | |||
There was a problem hiding this comment.
Can you add comments for the two loops?
|
@eh2077 Hi, do you get this 403 forbidden issue when run locally? Currently, I am facing this issue and couldn't run local
|
|
It works well for me, can you clear cache and try again? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-19.at.11.04.24.PM.movAndroid: mWeb ChromeScreen.Recording.2026-02-19.at.11.02.46.PM.moviOS: HybridAppScreen.Recording.2026-02-19.at.11.13.29.PM.moviOS: mWeb SafariScreen.Recording.2026-02-19.at.11.06.17.PM.movMacOS: Chrome / SafariScreen.Recording.2026-02-19.at.11.01.10.PM.mov |
eh2077
left a comment
There was a problem hiding this comment.
Works well! @lorretheboy Please have a look at https://github.com/Expensify/App/pull/82619/changes#r2816667749, thanks
|
@lorretheboy Can you also add unit tests to cover these cases? thanks! |
|
@eh2077 added tests |
|
@lorretheboy Can you try merge with main again please? thanks |
|
Merged main |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 @luacmartins 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|


Explanation of Change
Fixed Issues
$ #70591
PROPOSAL:
Tests
Offline tests
QA Steps
Same as Tests
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: mWeb Chrome
website.android.mov
iOS: Native
iOS: mWeb Safari
website.ios.mov
MacOS: Chrome / Safari
website.mov