-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix wrong total amount of hold expenses when approve/pay partially #49971
Fix wrong total amount of hold expenses when approve/pay partially #49971
Conversation
src/libs/actions/IOU.ts
Outdated
@@ -6448,7 +6448,7 @@ function getReportFromHoldRequestsOnyxData( | |||
chatReport.reportID, | |||
chatReport.policyID ?? iouReport?.policyID ?? '', | |||
recipient.accountID ?? 1, | |||
holdTransactions.reduce((acc, transaction) => acc + transaction.amount, 0) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1), | |||
((iouReport?.total ?? 0) - ((iouReport?.unheldTotal ?? 0) + (iouReport?.nonReimbursableTotal ?? 0))) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1), |
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.
Since the unheldTotal
doesn't include the nonReimbursableTotal
, I add them together. If I have 3 expenses,
A: $1 (track, non-reimbursable)
B: $2 (held)
C: $3
The total is $6, unheldTotal is $3, and nonReimbursableTotal is $1.
held total = $6 - ($3 + $1) = $2
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'm pretty confident that we can/should add them together because of this logic.
Lines 6667 to 6670 in 76c5c81
let total = (iouReport?.total ?? 0) - (iouReport?.nonReimbursableTotal ?? 0); | |
if (ReportUtils.hasHeldExpenses(iouReport?.reportID ?? '') && !full && !!iouReport?.unheldTotal) { | |
total = iouReport?.unheldTotal; | |
} |
The logic above calculates the pay amount to show.
It subtracts the total from the nonReimbursableTotal. But if there are held expenses and it's paid partially, then we just use the unheldTotal which means the unheldTotal doesn't contain the nonReimbursableTotal.
unheldTotal = the total reimbursable amount that is not held
src/libs/actions/IOU.ts
Outdated
@@ -6448,7 +6448,7 @@ function getReportFromHoldRequestsOnyxData( | |||
chatReport.reportID, | |||
chatReport.policyID ?? iouReport?.policyID ?? '', | |||
recipient.accountID ?? 1, | |||
holdTransactions.reduce((acc, transaction) => acc + transaction.amount, 0) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1), | |||
((iouReport?.total ?? 0) - ((iouReport?.unheldTotal ?? 0) + (iouReport?.nonReimbursableTotal ?? 0))) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1), | |||
getCurrency(firstHoldTransaction), |
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.
Can you fix the currency to use the iou's currency
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.
Oh, forgot about it. Fixed.
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.
Yep, I noticed that too, but that's how it's in prod too. I was thinking of adding the Before: Approve only $unheldTotal Do you agree with that? Or should we have someone from the team to confirm the behavior first? |
I think the calculated |
|
Ah, I think this needs to be fixed in BE then. cc @robertjchen Bug: Steps to reproduce:
Expected: the green button to state "Approve only DZD X.XX" Suggested solutions:
|
That won't work because we are not taking into account held non-reimbursable transactions |
Non reimbursable transaction can't be hold. |
Hm, is that true @JmillsExpensify @robertjchen? |
That should not be true, no. You should be able to hold any transaction. |
Oh, then it's a bug then. There is no option to Hold non-reimbursable in ND. |
So, to clarify- given this, the backend needs to be updated such so that: |
How do we expect this to work, @JmillsExpensify? Is it:
|
That's the expected.
In the approve modal we should show the unheldTotal which should include both reimbursable and non-reimbursable transactions. |
That's all fair feedback above, and on second thought, we struggled with this consideration in the design doc – enough that we put this in the out-of-scope section of the doc. In fact, there are kind of two cases to consider here:
This only matters for approval, as you can legitimately approve both reimbursable and non-reimbursable expenses. It is impossible to pay a non-reimbursable in 100% of cases. As a result, I think if we went this route, then we would only show the |
Ah right, then can we go with the following: BE:
FE: In the approve modal display:
In the pay modal display:
Does that make sense? |
If we're adding I think this question is still outstanding:
|
Sorry, we won't, I meant to write
They come off hold same as held reimbursable transactions. |
Gotcha. 👍
I think the UX might be a bit strange on that one as it's not communicated in the modal, but let's see I guess. |
@robertjchen Are we aligned with the changes here #49971 (comment) if so, can you please work on the BE changes? Those are breaking-changes (due to |
Done |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
Can you check the failing lint and ts |
Fixed |
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.
LGTM but we have this bug that we should handle separately
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.
Thanks for the patience here, let's run with what we have now and make adjustments later on if necessary. Thanks!! 🙇
There are some conflicts |
@Gonals Fixed |
✋ 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/Gonals in version: 9.0.70-0 🚀
|
@bernhardoj QA Found this issue #53425 when validation the PR Bug6682495_1733185782355.Recording__1103.mp4 |
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.70-9 🚀
|
Details
When approving/paying the report partially, the total held amount is wrong.
Fixed Issues
$ #48760
PROPOSAL: #48760 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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
Android: Native
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4