Fixing displaying the hold and unhold option in expense report#60436
Fixing displaying the hold and unhold option in expense report#60436marcaaron merged 16 commits intoExpensify:mainfrom
Conversation
|
@allroundexperts 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] |
|
@allroundexperts, I could not test it on mobile because it does not allow me to select multiple transactions. WDYT? |
|
@Tony-MK Can you please fix the lint issues first? |
|
@allroundexperts, I fixed the lint. You can review it now. |
|
Conflicts again 😢 |
|
Sorry about @allroundexperts. It is ready. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-05-01.at.1.51.24.AM.movAndroid: mWeb ChromeScreen.Recording.2025-05-01.at.1.50.13.AM.moviOS: NativeScreen.Recording.2025-05-01.at.1.47.09.AM.moviOS: mWeb SafariScreen.Recording.2025-05-01.at.1.45.26.AM.movMacOS: Chrome / SafariScreen.Recording.2025-04-28.at.1.49.39.AM.movMacOS: DesktopScreen.Recording.2025-04-28.at.1.54.44.AM.mov |
|
BUG
Screen.Recording.2025-04-28.at.1.58.26.AM.mov |
|
I wanted to mention this bug, but thought it was out of scope because based on the video of the OP shows, we are supposed to be fixing displaying the hold/unhold option inside the expense report. OP Video
1.mp4Nonetheless, this is a App/src/pages/Search/SearchPage.tsx Line 239 in 69b2da4 App/src/pages/Search/SearchPage.tsx Line 258 in 69b2da4 Furthermore, I was trying to reproduce the issue on the latest main, but I got different results from expected. First Result
When adding a new scan expense, the merchant text reads Skip to 1st.movSecond Result
Right after, I added another scan expense to the previous report, the hold option shows and 2nd.mov |
|
@Tony-MK Do you know why the following is not working? (It's not working on staging as well) Screen.Recording.2025-05-01.at.2.00.57.AM.mov |
|
Yeah, I noticed it. It's expected. |
There was a problem hiding this comment.
Looks good to me. Let's fix #60436 (comment) as follow up. @marcaaron let us know if you agree.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Hi! Coming here from a backend server error which looks like it's getting hit when you tested this on mobile - the timing and email address for one attempt line up perfectly with this video. If it helps, the error is that @marcaaron the error is this one - https://github.com/Expensify/Expensify/issues/495613 |
|
@dangrous We will see if this becomes a ND deploy blocker from QA soon, but sounds like it should be. Thoughts @Tony-MK ?
What does it mean exactly? I'm guessing that whatever you are experienced there is what triggered the BE error. Seems like it also happens on production or staging server, but what about production App? |
|
Also, I think if you have |
|
@dangrous @marcaaron As mentioned here, this is an error happening on main as well. Should we fix it as a follow up? |
|
I think I'd be more concerned about the |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.1.39-0 🚀
|
Based on the logs it seems like it's happening in staging too? At least I'm getting the same errors from applause testers. |
| canHoldTransactions = canHoldTransactions && canHoldRequest; | ||
| canUnholdTransactions = canUnholdTransactions && canUnholdRequest; | ||
|
|
||
| return !(canHoldTransactions || canUnholdTransactions); |
There was a problem hiding this comment.
@Tony-MK What is this return value supposed to do? This callback is used in a forEach so the return is ignored... is this logic correct? Maybe what you wanted was to use some?
There was a problem hiding this comment.
It should be correct because the return value is only used to stop iterating forEach when canHoldTransactions || canUnholdTransactions is false, meaning we can not hold or unhold all selected transactions.
So, forEach can stop early instead of iterating over all transactions twice, like when using some.
If there is a problem with this method, we can revert to using some.
There was a problem hiding this comment.
I don't think that is how forEach works. forEach ignores the value returned by the callback:
There is no way to stop or break a forEach() loop other than by throwing an exception. If you need such behavior, the forEach() method is the wrong tool.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach
There was a problem hiding this comment.
Your correct. So, sorry about that.
Should I open a PR to revert forEach to some?
There was a problem hiding this comment.
no problem!
I'm not so sure really, using some just because it will stop when you return true from the callback and ignoring the return of selectedTransactions.some(...) doesn't sound so clean to me either. Maybe you should just keep the forEach but drop the returned values so the code doesn't look weird.
selectedTransactions.forEach((selectedTransaction) => {
if (!canHoldTransactions && !canHoldTransactions) {
return;
}
if (!selectedTransaction?.transactionID) {
canHoldTransactions = false;
canUnholdTransactions = false;
return;
}
const iouReportAction = getIOUActionForTransactionID(reportActions, selectedTransaction.transactionID);
const {canHoldRequest, canUnholdRequest} = canHoldUnholdReportAction(iouReportAction);
canHoldTransactions = canHoldTransactions && canHoldRequest;
canUnholdTransactions = canUnholdTransactions && canUnholdRequest;
});I wouldn't care much for iterating the rest of the elements.. you can add something like
if (!canHoldTransactions && !canHoldTransactions) {
return;
}to make it fast, and it is also already inside a useMemo so this shouldn't happen all the time.
Hey @dangrous and @marcaaron, I did some digging to answer your questions. It is not related to this PR but this PR because it introduced Then, this CP Stag PR introduced Except for the delete option, App/src/pages/Search/SearchPage.tsx Lines 241 to 247 in 74b93b9 App/src/pages/Search/SearchPage.tsx Lines 279 to 286 in 74b93b9 Therefore, in App/src/pages/Search/SearchSelectedNarrow.tsx Lines 34 to 38 in 74b93b9 Thus, sending an empty list for
Since it's also happening on However, I'm unsure due to this comment. |
|
It looks like they've got a PR up that will fix that behavior, I'll ping there and make sure they test against this flow. Thank you for the investigation! Nice work, I think you're off the hook :) |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.1.39-8 🚀
|
|
BTW we were working on a BulkHoldRequest API and that's why we had this limitation in place, to avoid doing N API requests when holding multiple transactions like we did in this PR |
Sorry, I wasn't aware of that at first. I attempted to revert my mistake here. Should I revert the line? But it seems better to use the new API. Thoughts? |
|
@Tony-MK yea, let's just use the new API. We don't have a PR up for that yet |
|
You mentioned here that the new API is in place. Can I raise a PR for the fix? |
|
Yes, please |
|
Great. I started working on the PR, but could you give me some details about the BulkHoldRequest, such as the endpoint and parameters required? Thanks |
|
@Tony-MK sure: The key for holdData is the |

Explanation of Change
Fixed Issues
$ #60111
PROPOSAL: #60111 (comment)
Tests
Offline tests
QA Steps
+button.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
macOS.-.Chrome.mov
MacOS: Desktop
macOS.-.Desktop.mov