fix: [P2P Distance] Split - Participants amount displayed 0.00 briefly on confirmation screen. #47370
Conversation
…y on confirmation screen. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@parasharrajat 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@parasharrajat, while testing, I found a bug that is not related to our issue. Should we handle it in this PR, or is it better to open a new issue to avoid regressions? BUG: When we start split a distance request with multiple participants and go back and change the participant to a policy, an error message is shown. STEPS:
Monosnap.screencast.2024-08-18.16-15-06.mp4 |
|
@Krishna2323 Is this issue happening on staging? |
Yes, I can reproduce on latest main without any changes. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
I see. So let's not solve it here. |
|
We can discuss this on the issue and ask to create a new PR for this. |
|
@parasharrajat, do we need to update anything here? |
|
@parasharrajat, friendly bump. |
|
@Krishna2323 I am currently in a priority task. I will take a look as soon as possible. |
|
@Krishna2323 Thanks for waiting. my availability was low last week. I will for sure review this tomorrow. |
|
@Krishna2323 Could you please merge main? |
|
Merged main. |
|
@parasharrajat bump. |
| // Sets `amount` and `split` share data before moving to the next step to avoid briefly showing `0.00` as the split share for participants | ||
| const setDistanceRequestData = useCallback( | ||
| (isPolicyExpenseChat: boolean) => { | ||
| // Get policy report based on transaction participants | ||
| const policyReport = transaction?.participants?.[0] ? ReportUtils.getReport(selectedReportID.current) : report; | ||
|
|
||
| const policyID = IOU.getIOURequestPolicyID(transaction, policyReport); | ||
| const policy = PolicyUtils.getPolicy(report?.policyID ?? policyID); | ||
| const policyCurrency = policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD; | ||
|
|
||
| const customUnitRateID = TransactionUtils.getRateID(transaction) ?? '-1'; | ||
| const mileageRates = DistanceRequestUtils.getMileageRates(policy); | ||
| const defaultMileageRate = DistanceRequestUtils.getDefaultMileageRate(policy); | ||
| const mileageRate: MileageRate = TransactionUtils.isCustomUnitRateIDForP2P(transaction) | ||
| ? DistanceRequestUtils.getRateForP2P(policyCurrency) | ||
| : mileageRates?.[customUnitRateID] ?? defaultMileageRate; | ||
|
|
||
| const {unit, rate} = mileageRate ?? {}; | ||
| const distance = TransactionUtils.getDistanceInMeters(transaction, unit); | ||
| const currency = mileageRate?.currency ?? policyCurrency; | ||
| const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, rate ?? 0); | ||
| IOU.setMoneyRequestAmount(transactionID, amount, currency); | ||
|
|
||
| const participantAccountIDs: number[] | undefined = transaction?.participants?.map((participant) => participant.accountID ?? -1); | ||
| if (isSplitRequest && amount && currency && !isPolicyExpenseChat) { | ||
| IOU.setSplitShares(transaction, amount, currency ?? '', participantAccountIDs ?? []); | ||
| } | ||
| }, | ||
| [report, transaction, transactionID, isSplitRequest], | ||
| ); |
There was a problem hiding this comment.
Don't we have to remove the similar logic from IOURequestStepConfirmation page after we added it here?
There was a problem hiding this comment.
@parasharrajat, yeah you are right. I was looking at the wrong code and thought that it should be there to avoid regressions but we have a similar logic which only runs once when the IOURequestStepConfirmation mounts.
I have removed the code on IOURequestStepConfirmation page, please check and let me know. Thanks
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| const isFirstUpdatedDistanceAmount = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| if (isFirstUpdatedDistanceAmount.current) { | ||
| return; | ||
| } | ||
| if (!isDistanceRequest) { | ||
| return; | ||
| } | ||
| const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, rate ?? 0); | ||
| IOU.setMoneyRequestAmount(transactionID, amount, currency ?? ''); | ||
| isFirstUpdatedDistanceAmount.current = true; | ||
| }, [distance, rate, unit, transactionID, currency, isDistanceRequest]); | ||
|
|
There was a problem hiding this comment.
May be we should keep it in case someone directly accesses the page via URL. I am not too sure on this. What do you think @Krishna2323
There was a problem hiding this comment.
@parasharrajat, I have re-added that, I think we should keep that because incase if the user start a split req from group report, the participant step will be skipped and the amount will be not updated.
There was a problem hiding this comment.
@parasharrajat, friendly bump.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 I need some help testing this PR. Could you point out the a few different flow that you think might be affected from this change. |
|
@parasharrajat, I need some time to check if we need this PR or not because the flow has been changed, now you can't directly split from FAB. I will update very soon. |
|
Sure...man. |
|
@parasharrajat, the flow has been changed but the issue will still occur because initially the amount is 0, I'm thinking to make the same changes in Monosnap.screencast.2024-09-25.05-16-35.mp4 |
|
When you say flow has been changed, what do you mean by that? |
Details
Fixed Issues
$ #47100
PROPOSAL: #47100 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4