Skip to content
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.

Sign up for GitHub

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

Merged
merged 28 commits into from
Dec 2, 2024
Merged

Fix wrong total amount of hold expenses when approve/pay partially #49971

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
09a1942
fix the wrong calculation
bernhardoj Oct 1, 2024
d0037f6
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 1, 2024
2b5a152
use the current iou when creating the new report
bernhardoj Oct 1, 2024
6196f9c
remove unused import
bernhardoj Oct 1, 2024
8c86bc1
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 16, 2024
b831dd1
fix total calculation
bernhardoj Oct 16, 2024
fbdb8ad
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 18, 2024
7c223b9
get the reimbursable amount when the type is pay
bernhardoj Oct 18, 2024
62b6727
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 22, 2024
903d17d
rename
bernhardoj Oct 22, 2024
997de55
update unheldTotal and unheldReimburableTotal optimistically
bernhardoj Oct 22, 2024
3817ddd
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 23, 2024
6e1f6e4
update comment
bernhardoj Oct 23, 2024
077a7d5
calc the amount once
bernhardoj Oct 23, 2024
d7fddd1
store the correct non reimbursable amount
bernhardoj Oct 23, 2024
6932d60
fix lint
bernhardoj Oct 23, 2024
8cbe949
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 24, 2024
8ce39ae
fix wrong non reimbursable amount
bernhardoj Oct 24, 2024
c278a1a
optimistically update unheld total for IOU
bernhardoj Oct 24, 2024
54023a3
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Oct 30, 2024
d0277e2
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Nov 14, 2024
164a73a
fix syntax error
bernhardoj Nov 14, 2024
a3c25a5
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Nov 22, 2024
2c4480d
fix lint
bernhardoj Nov 23, 2024
9ade52c
fix another lint
bernhardoj Nov 23, 2024
fa58c4f
fix type error
bernhardoj Nov 23, 2024
07c33d0
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Nov 27, 2024
ad5a3c5
Merge branch 'main' into fix/48760-wrong-expense-total-if-currency-di…
bernhardoj Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/MoneyReportHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
shouldShowExportIntegrationButton;
const bankAccountRoute = ReportUtils.getBankAccountRoute(chatReport);
const formattedAmount = CurrencyUtils.convertToDisplayString(reimbursableSpend, moneyRequestReport?.currency);
const {nonHeldAmount, fullAmount, hasValidNonHeldAmount} = ReportUtils.getNonHeldAndFullAmount(moneyRequestReport, policy);
const {nonHeldAmount, fullAmount, hasValidNonHeldAmount} = ReportUtils.getNonHeldAndFullAmount(moneyRequestReport, shouldShowPayButton);
const isAnyTransactionOnHold = ReportUtils.hasHeldExpenses(moneyRequestReport?.reportID);
const displayedAmount = isAnyTransactionOnHold && canAllowSettlement && hasValidNonHeldAmount ? nonHeldAmount : formattedAmount;
const isMoreContentShown = shouldShowNextStep || shouldShowStatusBar || (shouldShowAnyButton && shouldUseNarrowLayout);
Expand Down
25 changes: 14 additions & 11 deletions src/components/ReportActionItem/ReportPreview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ function ReportPreview({
const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const [userWallet] = useOnyx(ONYXKEYS.USER_WALLET);
const [invoiceReceiverPolicy] = useOnyx(
`${ONYXKEYS.COLLECTION.POLICY}${chatReport?.invoiceReceiver && 'policyID' in chatReport.invoiceReceiver ? chatReport.invoiceReceiver.policyID : -1}`,
);
const theme = useTheme();
const styles = useThemeStyles();
const {translate} = useLocalize();
Expand All @@ -123,13 +126,20 @@ function ReportPreview({
const [isPaidAnimationRunning, setIsPaidAnimationRunning] = useState(false);
const [isHoldMenuVisible, setIsHoldMenuVisible] = useState(false);
const [requestType, setRequestType] = useState<ActionHandledType>();
const {nonHeldAmount, fullAmount, hasValidNonHeldAmount} = ReportUtils.getNonHeldAndFullAmount(iouReport, policy);
const hasOnlyHeldExpenses = ReportUtils.hasOnlyHeldExpenses(iouReport?.reportID ?? '');

const [paymentType, setPaymentType] = useState<PaymentMethodType>();
const [invoiceReceiverPolicy] = useOnyx(
`${ONYXKEYS.COLLECTION.POLICY}${chatReport?.invoiceReceiver && 'policyID' in chatReport.invoiceReceiver ? chatReport.invoiceReceiver.policyID : -1}`,

const getCanIOUBePaid = useCallback(
(onlyShowPayElsewhere = false) => IOU.canIOUBePaid(iouReport, chatReport, policy, allTransactions, onlyShowPayElsewhere),
[iouReport, chatReport, policy, allTransactions],
);

const canIOUBePaid = useMemo(() => getCanIOUBePaid(), [getCanIOUBePaid]);
const onlyShowPayElsewhere = useMemo(() => !canIOUBePaid && getCanIOUBePaid(true), [canIOUBePaid, getCanIOUBePaid]);
const shouldShowPayButton = isPaidAnimationRunning || canIOUBePaid || onlyShowPayElsewhere;
const {nonHeldAmount, fullAmount, hasValidNonHeldAmount} = ReportUtils.getNonHeldAndFullAmount(iouReport, shouldShowPayButton);
const hasOnlyHeldExpenses = ReportUtils.hasOnlyHeldExpenses(iouReport?.reportID ?? '');

const managerID = iouReport?.managerID ?? action.childManagerAccountID ?? 0;
const {totalDisplaySpend, reimbursableSpend} = ReportUtils.getMoneyRequestSpendBreakdown(iouReport);

Expand Down Expand Up @@ -329,14 +339,7 @@ function ReportPreview({
]);

const bankAccountRoute = ReportUtils.getBankAccountRoute(chatReport);
const getCanIOUBePaid = useCallback(
(onlyShowPayElsewhere = false) => IOU.canIOUBePaid(iouReport, chatReport, policy, allTransactions, onlyShowPayElsewhere),
[iouReport, chatReport, policy, allTransactions],
);

const canIOUBePaid = useMemo(() => getCanIOUBePaid(), [getCanIOUBePaid]);
const onlyShowPayElsewhere = useMemo(() => !canIOUBePaid && getCanIOUBePaid(true), [canIOUBePaid, getCanIOUBePaid]);
const shouldShowPayButton = isPaidAnimationRunning || canIOUBePaid || onlyShowPayElsewhere;
const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(iouReport, policy), [iouReport, policy]);

const shouldDisableApproveButton = shouldShowApproveButton && !ReportUtils.isAllowedToApproveExpenseReport(iouReport);
Expand Down
2 changes: 2 additions & 0 deletions src/libs/DebugUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,7 @@ function validateReportDraftProperty(key: keyof Report, value: string) {
case 'total':
case 'unheldTotal':
case 'nonReimbursableTotal':
case 'unheldNonReimbursableTotal':
return validateNumber(value);
case 'chatType':
return validateConstantEnum(value, CONST.REPORT.CHAT_TYPE);
Expand Down Expand Up @@ -616,6 +617,7 @@ function validateReportDraftProperty(key: keyof Report, value: string) {
participants: CONST.RED_BRICK_ROAD_PENDING_ACTION,
total: CONST.RED_BRICK_ROAD_PENDING_ACTION,
unheldTotal: CONST.RED_BRICK_ROAD_PENDING_ACTION,
unheldNonReimbursableTotal: CONST.RED_BRICK_ROAD_PENDING_ACTION,
isWaitingOnBankAccount: CONST.RED_BRICK_ROAD_PENDING_ACTION,
isCancelledIOU: CONST.RED_BRICK_ROAD_PENDING_ACTION,
iouReportID: CONST.RED_BRICK_ROAD_PENDING_ACTION,
Expand Down
9 changes: 9 additions & 0 deletions src/libs/IOUUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ function updateIOUOwnerAndTotal<TReport extends OnyxInputOrEntry<Report>>(
currency: string,
isDeleting = false,
isUpdating = false,
isOnhold = false,
): TReport {
// For the update case, we have calculated the diff amount in the calculateDiffAmount function so there is no need to compare currencies here
if ((currency !== iouReport?.currency && !isUpdating) || !iouReport) {
Expand All @@ -87,18 +88,26 @@ function updateIOUOwnerAndTotal<TReport extends OnyxInputOrEntry<Report>>(

// Let us ensure a valid value before updating the total amount.
iouReportUpdate.total = iouReportUpdate.total ?? 0;
iouReportUpdate.unheldTotal = iouReportUpdate.unheldTotal ?? 0;

if (actorAccountID === iouReport.ownerAccountID) {
iouReportUpdate.total += isDeleting ? -amount : amount;
if (!isOnhold) {
iouReportUpdate.unheldTotal += isDeleting ? -amount : amount;
}
} else {
iouReportUpdate.total += isDeleting ? amount : -amount;
if (!isOnhold) {
iouReportUpdate.unheldTotal += isDeleting ? amount : -amount;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the optimistic unheldTotal for IOU too. However, there is a problem.

  1. User A submit $2 and $2 to user B (total = 4, unheldTotal = 4, owner = user A)
  2. User A hold $2 (total = 4, unheldTotal = 2, owner user A)
  3. User B submit $10

When user B submits, it will go into the else block, subtracting total from the amount (10). So, the new total will be -6. If it's < 0, we switch the owner with this logic and switch the sign too, so the new optimistic total is 6.

App/src/libs/IOUUtils.ts

Lines 96 to 100 in 866ce4b

if (iouReportUpdate.total < 0) {
// The total sign has changed and hence we need to flip the manager and owner of the report.
iouReportUpdate.ownerAccountID = iouReport.managerID;
iouReportUpdate.managerID = iouReport.ownerAccountID;
iouReportUpdate.total = -iouReportUpdate.total;

The problem I see is, that BE still keeps the sign, so the total from BE is -6. Also, the unheldTotal is -8, which looks like it comes from unheldTotal (2) - amount (10). So, I apply this calculation for the optimistic one too as you can see above. I'm not sure if the unheldTotal should be -8, but at least I match it with the BE. I think it will be another big problem to solve and decide the expected behavior.

Notice the image below shows Pay -8 (unheldTotal).

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this looks tricky, i think we need to agree on the expected behaviour on what the total and the unheldTotal should be in this case and also in the case where we unhold the $2 request. (can we still unhold that request? and how can do it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still unhold it and the unheldTotal will be the same as total (-6). If I hold it again, it will be -8 again.

User A submit $2 and $2 to user B (total = 4, unheldTotal = 4, owner = user A)
User A hold $2 (total = 4, unheldTotal = 2, owner user A)
User B submit $10

I think if we have this case, the new total should be 6 (number is correct already in prod, but it's in negative) and unheldTotal should be:

current held = total (4) - unheldTotal (2) = 2
new unheldTotal = new total (6) - current held (2) = 4

I suspect the BE already did this calculation, but because the total is negative (-6), subtracting with 2 will be -8.

cc: @robertjchen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unheldTotal on the backend is simply the reimbursable total (excluding held transactions) + non reimbursable total (excluding held transactions).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the problem around IOUs and expense reports having different signage? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, we haven't changed that, but it has been a loooong while since I touched that area

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertjchen Would you be able to check again so we can move forward here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertjchen Just merged with the main and fixed some conflicts. I think we just need a confirmation whether it's necessarry need to do this or not.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay- I think switching the sign optimistically as you've proposed may be necessary, as there won't be any further changes to the amounts being returned from the BE 😥

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to change then, I just fixed a conflict, I think it's ready for merge.

}

if (iouReportUpdate.total < 0) {
// The total sign has changed and hence we need to flip the manager and owner of the report.
iouReportUpdate.ownerAccountID = iouReport.managerID;
iouReportUpdate.managerID = iouReport.ownerAccountID;
iouReportUpdate.total = -iouReportUpdate.total;
iouReportUpdate.unheldTotal = -iouReportUpdate.unheldTotal;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the BE stores it as negative, I still switch the sign here optimistically just to make it consistent with total and I think it should be the correct amount, in positive.

}

return iouReportUpdate;
Expand Down
39 changes: 22 additions & 17 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ type OptimisticExpenseReport = Pick<
| 'stateNum'
| 'statusNum'
| 'total'
| 'unheldTotal'
| 'nonReimbursableTotal'
| 'unheldNonReimbursableTotal'
| 'parentReportID'
| 'lastVisibleActionCreated'
| 'parentReportActionID'
Expand Down Expand Up @@ -452,6 +454,9 @@ type OptimisticIOUReport = Pick<
| 'stateNum'
| 'statusNum'
| 'total'
| 'unheldTotal'
| 'nonReimbursableTotal'
| 'unheldNonReimbursableTotal'
| 'reportName'
| 'parentReportID'
| 'lastVisibleActionCreated'
Expand Down Expand Up @@ -4556,6 +4561,9 @@ function buildOptimisticIOUReport(
stateNum: isSendingMoney ? CONST.REPORT.STATE_NUM.APPROVED : CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: isSendingMoney ? CONST.REPORT.STATUS_NUM.REIMBURSED : CONST.REPORT.STATE_NUM.SUBMITTED,
total,
unheldTotal: total,
nonReimbursableTotal: 0,
unheldNonReimbursableTotal: 0,

// We don't translate reportName because the server response is always in English
reportName: `${payerEmail} owes ${formattedTotal}`,
Expand Down Expand Up @@ -4673,11 +4681,12 @@ function buildOptimisticExpenseReport(
payeeAccountID: number,
total: number,
currency: string,
reimbursable = true,
nonReimbursableTotal = 0,
parentReportActionID?: string,
): OptimisticExpenseReport {
// The amount for Expense reports are stored as negative value in the database
const storedTotal = total * -1;
const storedNonReimbursableTotal = nonReimbursableTotal * -1;
const policyName = getPolicyName(ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`]);
const formattedTotal = CurrencyUtils.convertToDisplayString(storedTotal, currency);
const policy = getPolicy(policyID);
Expand All @@ -4696,7 +4705,9 @@ function buildOptimisticExpenseReport(
stateNum,
statusNum,
total: storedTotal,
nonReimbursableTotal: reimbursable ? 0 : storedTotal,
unheldTotal: storedTotal,
nonReimbursableTotal: storedNonReimbursableTotal,
unheldNonReimbursableTotal: storedNonReimbursableTotal,
participants: {
[payeeAccountID]: {
notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
Expand Down Expand Up @@ -7762,27 +7773,21 @@ function hasUpdatedTotal(report: OnyxInputOrEntry<Report>, policy: OnyxInputOrEn
/**
* Return held and full amount formatted with used currency
*/
function getNonHeldAndFullAmount(iouReport: OnyxEntry<Report>, policy: OnyxEntry<Policy>): NonHeldAndFullAmount {
const reportTransactions = reportsTransactions[iouReport?.reportID ?? ''] ?? [];
const hasPendingTransaction = reportTransactions.some((transaction) => !!transaction.pendingAction);

function getNonHeldAndFullAmount(iouReport: OnyxEntry<Report>, shouldExcludeNonReimbursables: boolean): NonHeldAndFullAmount {
// if the report is an expense report, the total amount should be negated
const coefficient = isExpenseReport(iouReport) ? -1 : 1;

if (hasUpdatedTotal(iouReport, policy) && hasPendingTransaction) {
const unheldTotal = reportTransactions.reduce((currentVal, transaction) => currentVal + (!TransactionUtils.isOnHold(transaction) ? transaction.amount : 0), 0);

return {
nonHeldAmount: CurrencyUtils.convertToDisplayString(unheldTotal * coefficient, iouReport?.currency),
fullAmount: CurrencyUtils.convertToDisplayString((iouReport?.total ?? 0) * coefficient, iouReport?.currency),
hasValidNonHeldAmount: unheldTotal * coefficient >= 0,
};
let total = iouReport?.total ?? 0;
let unheldTotal = iouReport?.unheldTotal ?? 0;
if (shouldExcludeNonReimbursables) {
total -= iouReport?.nonReimbursableTotal ?? 0;
unheldTotal -= iouReport?.unheldNonReimbursableTotal ?? 0;
}

return {
nonHeldAmount: CurrencyUtils.convertToDisplayString((iouReport?.unheldTotal ?? 0) * coefficient, iouReport?.currency),
fullAmount: CurrencyUtils.convertToDisplayString((iouReport?.total ?? 0) * coefficient, iouReport?.currency),
hasValidNonHeldAmount: (iouReport?.unheldTotal ?? 0) * coefficient >= 0,
nonHeldAmount: CurrencyUtils.convertToDisplayString(unheldTotal * coefficient, iouReport?.currency),
fullAmount: CurrencyUtils.convertToDisplayString(total * coefficient, iouReport?.currency),
hasValidNonHeldAmount: unheldTotal * coefficient >= 0,
};
}

Expand Down
Loading
Loading