-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Enable distance splits #42302
Enable distance splits #42302
Conversation
…istic transaction function
Hi @mananjadhav. Sorry for the delay in getting back to this. I just made one small change for clean up, merged main, and now I think it's all working well. Please test again and let me know if you have any problems. Also please include reproduction steps for any bugs you find. edit-amount-success.movdistance-split-existing.movWhile I wait for your next review I will re-test the bugs I found in this branch and on main and report them. |
thanks @neil-marcellini. I'll review and test this again. I'll put in the reproduction steps for future reports but several of these looked unrelated to the PR. |
Starting on this now. |
@neil-marcellini We can skip for this one, but offline tests won't work. If we create an offline distance split, we don't have the amount and the splits throw an error: Reproduction Steps:
web-offline-distance-split.mov |
Just finished testing different scenarios on web and everything else looks good. Highlighted the offline case in the previous comment. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-distance-split.movAndroid: mWeb Chromemweb-chrome-distance-split.moviOS: Nativeios-distance-split-2.moviOS: mWeb Safarimweb-safari-distance-split.movMacOS: Chrome / SafariDistance Split web-distance-split_y27cNsyv.mp4Distance Split Other web-distance-split-others_GnBaJ7dl.mp4Offline Split -- doesn't work web-offline-distance-split.movMacOS: Desktopdesktop-distance-split_xRgJMAN8.mp4 |
@arosiclair 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] |
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 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/arosiclair in version: 9.0.18-0 🚀
|
Looks like there is a bug where the QAB distance split uses 'SplitBill' endpoint when it should use 'CreateDistanceRequest' #47021. |
This PR also broke TrackExpense on DMs since it started calling the wrong endpoint: #47042 (comment) |
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.18-10 🚀
|
receipt, | ||
filename, | ||
receipt: receipt?.source ? {source: receipt.source, state: receipt.state ?? CONST.IOU.RECEIPT_STATE.SCANREADY} : {}, | ||
filename: receipt?.source ?? receipt?.name ?? filename, |
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 forgot to enforce the filename as a string here which led to crash in iOS. Details #47022
playSound(SOUNDS.DONE); | ||
|
||
if (isDistanceRequest && !isMovingTransactionFromTrackExpense) { |
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.
Moving this createDistanceRequest
call here caused an error in #47042 when tracking a distance expense as we end up calling CreateDistanceRequest
here instead of trackExpense
.
@@ -3849,7 +3751,7 @@ function trackExpense( | |||
|
|||
function getOrCreateOptimisticSplitChatReport(existingSplitChatReportID: string, participants: Participant[], participantAccountIDs: number[], currentUserAccountID: number) { | |||
// The existing chat report could be passed as reportID or exist on the sole "participant" (in this case a report option) | |||
const existingChatReportID = existingSplitChatReportID || participants[0].reportID; | |||
const existingChatReportID = existingSplitChatReportID ?? participants[0].reportID ?? ''; |
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.
Hi team. Coming from this issue #47159. Be careful when change to using "??", because if existingSplitChatReportID
is an empty string, it won't fallback to participants[0].reportID
. For more details: #47159 (comment)
// If it's a split request among individuals, set the split shares | ||
const participantAccountIDs: number[] = selectedParticipantsProp.map((participant) => participant.accountID ?? -1); | ||
if (isTypeSplit && !isPolicyExpenseChat && amount && transaction?.currency) { | ||
IOU.setSplitShares(transaction, amount, currency, participantAccountIDs); |
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 side-effect does not update the values immediately which creates some delay before real values are shown to the user. #47100
Details
Enable splitting distance expenses. The goal is to make sure that the feature works for the mainline cases, that it doesn't break existing split flows, and that it's still hidden by the beta. There will be a number of small bugs that already exist on main and probably a few with this new feature. In order to keep things moving, these bugs will be listed and issues will be created to fix them with help from the community before removing the beta.
Fixed Issues
$ #36967
PROPOSAL: N/A
Tests
Known bugs on main:
While testing please take these into consideration and ignore them since they were not caused by this PR.
New bugs to fix later
Maybe new, maybe happen on main
A. Split distance with new accounts
B. Workspace distance split
C. Manual split with existing accounts
D. Verify splits without the p2pDistanceRequests beta
E. Split with an existing group
F. Manual split new accounts
Offline tests
Skipping these for now since the existing tests are a lot to run through already. I'm pretty sure it will work well offline, but we can fine tune that while the feature is still under beta and before adding the regression tests for the feature.
QA Steps
Same as tests. If you find any issues please tag me in Slack and make sure they are not labeled as a deploy blocker, unless it's an issue that happens outside of the beta.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
I only tested on web because the changes are platform independent, and C+ will help test all platforms.
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
A
splitA.mp4
B
splitB1080.mov
C
splitC.mov
D
splitD.mov
E
splitE.mp4
F
splitF.mov
MacOS: Desktop