-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Multi-Scan] Refactor IOU screens to support handling multiple expense creation #61574
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
base: main
Are you sure you want to change the base?
[Multi-Scan] Refactor IOU screens to support handling multiple expense creation #61574
Conversation
@dukenv0307 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 isTestReceipt = receipt?.isTestReceipt ?? false; | ||
requestMoneyIOUActions({ | ||
report, | ||
participantParams: { |
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.
participantParams and policyParams should be depended on transaction
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 believe the participants and policy parameters in this case will be the same for all transactions. (see the slack)
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.
Ah seems like I can't open the slack link, but it makes sense to me
if (currentUserPersonalDetails.login && !!transaction) { | ||
startSplitBill({ | ||
participants: selectedParticipants, | ||
currentUserLogin: currentUserPersonalDetails.login, | ||
currentUserAccountID: currentUserPersonalDetails.accountID, | ||
comment: trimmedComment, | ||
receipt: receiptFile, | ||
receipt: currentTransactionReceiptFile, |
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 we use the currentTransactionReceiptFile only?
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.
The split functionality won't support multi-scan for now, see the docs:
https://docs.google.com/document/d/1ZjSjfIM7N8jhBwyYA4nz3Fe0cvOEXdu-6FA3pethL1Q/edit?pli=1&tab=t.0#bookmark=id.vmodh43m67v8
@dukenv0307 Could you please take another look? |
on it now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-05-13.at.18.03.02.movAndroid: mWeb ChromeScreen.Recording.2025-05-13.at.16.24.31.moviOS: HybridAppScreen.Recording.2025-05-13.at.18.06.22.moviOS: mWeb SafariScreen.Recording.2025-05-13.at.16.23.40.movMacOS: Chrome / SafariScreen.Recording.2025-05-13.at.16.20.42.movMacOS: DesktopScreen.Recording.2025-05-13.at.18.12.27.mov |
Looks good. But we got the conflicts, can you please resolve it @VickyStash? |
@dukenv0307 Done! |
🚧 @cristipaval has triggered a test 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! 🧪🧪 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Expense - To field is missing on the confirmation page and unable to submit track expenseVersion Number: 9.1.44-6 Action Performed:
Expected Result:There will be To field on the confirmation page. Actual Result:To field is missing on the confirmation page and unable to submit track expense. Workaround:Unknown Platforms:
Screenshots/VideosBug6831962_1747302560206.Screen_Recording_2025-05-14_at_22.35.42.mp4Upwork Automation - Do Not Edit |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Scan - Unable to submit a scan expense via QABVersion Number: 9.1.44-6 Action Performed:Precondition:
Expected Result:The expense is submitted Actual Result:"There was an error uploading your receipt. Please try again or save the receipt to upload later." error appears and the expense is not submitted Workaround:Unknown Platforms:
Screenshots/VideosBug6831977_1747303905165.2025-05-15_13_07_12.mp4Upwork Automation - Do Not Edit |
97062ed
to
bad98e9
Compare
cc @dukenv0307 Great findings @jponikarchuk, thank you! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Expense-Correct category not reflected in confirmation page for distance request created via FABVersion Number: 9.1.44-6 PR:61574 Action Performed:Preconditions:
Member side:
Steps:
Expected Result:On step 5 - Admin set category should be displayed on member side when creating distance request Actual Result:Correct category not reflected in confirmation page for distance request created via FAB Note: Issue NOT reproducible when member directly creating distance request from workspace chat (i.e via + icon next to composer) Workaround:Unknown Platforms:
Screenshots/VideosBug6832216_1747321830137.Recording__163.mp4 |
Expense - Can not enable or disable the billable toggle when creating a new expenseIf you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Version Number: 9.1.44-6 PR:61574 Action Performed:
Expected Result:User can set the expenses to either Non Billable or Billable if the "Re-bill expenses to clients" is set to "Default to Billable" or "Default to Non-Billable" during expense creation Actual Result:The Billable toggle can not be changed when creating an expense. The expense is set to the default value and user needs to edit that later on the expense details Workaround:Unknown Platforms:
Screenshots/VideosBug6832243_1747323196408.ScreenRecording_05-15-2025_18-25-39_1.mp4 |
Reviewing again. BTW, we got the conflicts @VickyStash |
…an-flow # Conflicts: # src/pages/iou/request/step/IOURequestStepScan/index.native.tsx # src/pages/iou/request/step/IOURequestStepScan/index.tsx
Note: The |
Expense-Correct category not reflected in confirmation page for distance request created via FAB and Expense - Can not enable or disable the billable toggle when creating a new expense are fixed in cd2af29. |
…an-flow # Conflicts: # src/pages/iou/request/step/IOURequestStepConfirmation.tsx # src/pages/iou/request/step/IOURequestStepScan/index.native.tsx # src/pages/iou/request/step/IOURequestStepScan/index.tsx
Looks good now. @cristipaval should we trigger the new build? |
I just asked Applause if they finished the regression tests. If they did, then I think it's fine if you test the PR again, @dukenv0307. If you reapprove, I think we're good to merge. |
Alright, the regression is complete. Once we have the reported issues fixed, and the PR retested by the C+, I think we can merge. |
on it now |
Screen.Recording.2025-05-19.at.17.14.53.movScreen.Recording.2025-05-19.at.17.10.39.mp4 |
@cristipaval We're good to merge |
Explanation of Change
Doc section: https://docs.google.com/document/d/1ZjSjfIM7N8jhBwyYA4nz3Fe0cvOEXdu-6FA3pethL1Q/edit?pli=1&tab=t.0#bookmark=id.ez670onfv4jq
Fixed Issues
$ #61176
PROPOSAL: N/A
Tests
Using FAB button:
Inside 1-1 chat:
Expense creation logic should work the same way as before.
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in the Tests steps.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android1.mp4
Android: mWeb Chrome
android_web1.mp4
iOS: Native
ios1.mp4
iOS: mWeb Safari
ios_web1.mp4
MacOS: Chrome / Safari
web1.mp4
MacOS: Desktop
desktop1.mp4