Update create report logic while moving expenses#73186
Conversation
|
@sobitneupane 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] |
| const knownOwnerIDs = new Set<number>(); | ||
| let hasUnknownOwner = false; | ||
|
|
||
| Object.values(selectedTransactionsMeta ?? {}).forEach((selectedTransactionInfo) => { |
There was a problem hiding this comment.
This is only to check if the transactions have multiple owners. So, instead of running through all transactions, using a for loop and returning early the moment we found two different owners should be enough, right?
There was a problem hiding this comment.
Updated, thanks for the suggestion.
| } | ||
| }); | ||
|
|
||
| selectedTransactionsList.forEach((selectedTransaction) => { |
|
|
||
| const ownerAccountIDs = new Set<number>(); | ||
| let hasUnknownOwner = false; | ||
| selectedTransactionsKeys.forEach((id) => { |
|
Thanks for the quick work. It tests well. Suggested a couple of minor efficiency improvements. |
|
Is this #73170 from our PR too? Nvm, it is fixed too with this PR. Please add it to the fixed issues. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppmoveAndroid.movAndroid: mWeb ChromemoveAndroidmWeb.moviOS: HybridAppmoveiOS.moviOS: mWeb SafarimoveiOSmWeb.movMacOS: Chrome / SafariallReportsChrome.movmoveExpenseChrome.movUploading createReportChrome.mov… notHereChrome1.movmoveExpenseChrome.movallReportsChrome.movUploading createReportChrome.mov… MacOS: DesktopmoveDesktop.mov |
|
Yes, just checked that #73170 is also fixed. |
|
🚧 @inimaga 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, Desktop, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Okay, I tried to cherry pick this change to Staging but given the number of changes included, the team is more comfortable with reverting the original offending PR and this one, re-merging the changes in both and getting this through staging tomorrow so we don't miss anything and it can be properly tested. (internal discussion link) cc: @c3024 & @ShridharGoel |
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.2.37-1 🚀
|
|
Hi @ShridharGoel Do we need QA today for this? |
|
Not yet, this was reverted and will be added back soon. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.2.37-8 🚀
|





Explanation of Change
Fixed Issues
$ #72485, #73166, #73168, #73167, #73170
PROPOSAL:
Tests
Offline tests
QA Steps
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))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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop