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

Updated total amount when change the different currency #35880

Merged
merged 16 commits into from
Feb 16, 2024
Merged

Updated total amount when change the different currency #35880

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
72dee1d
Add policy avatar
dukenv0307 Feb 5, 2024
1e3e91e
fix lint
dukenv0307 Feb 5, 2024
d52739b
Merge branch 'main' into fix/35209
dukenv0307 Feb 6, 2024
a6ffc90
update total amount when offline and change currency
dukenv0307 Feb 6, 2024
b2ce3f0
fix lint
dukenv0307 Feb 6, 2024
89abd02
Revert "Add policy avatar"
dukenv0307 Feb 6, 2024
312ddbc
Revert "fix lint"
dukenv0307 Feb 6, 2024
c8a3c4c
remove useless code
dukenv0307 Feb 6, 2024
7c5889e
create a new function to update the total of money request report
dukenv0307 Feb 7, 2024
296fb19
create a function to compute diff amount
dukenv0307 Feb 7, 2024
0626907
fix ts error
dukenv0307 Feb 7, 2024
271bd23
fix prettier
dukenv0307 Feb 7, 2024
7d586ef
merge main
dukenv0307 Feb 15, 2024
8796fae
edit comment and refactor new function
dukenv0307 Feb 15, 2024
2a48143
fix incorrect amount for the case create or delete a request with for…
dukenv0307 Feb 15, 2024
9f075c4
revert hard code
dukenv0307 Feb 15, 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/libs/IOUUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function calculateAmount(numberOfParticipants: number, total: number, currency:
* @param isDeleting - whether the user is deleting the request
*/
function updateIOUOwnerAndTotal<TReport extends OnyxEntry<Report>>(iouReport: TReport, actorAccountID: number, amount: number, currency: string, isDeleting = false): TReport {
if (currency !== iouReport?.currency) {
if (!iouReport?.currency) {
return iouReport;
}

Expand Down
69 changes: 47 additions & 22 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,33 @@ function createDistanceRequest(
Report.notifyNewAction(chatReport.reportID, userAccountID);
}

/**
* Compute the diff amount when we update the transaction
*/
function computeDiffAmount(iouReport: OnyxEntry<OnyxTypes.Report>, updatedTransaction: OnyxEntry<OnyxTypes.Transaction>, transaction: OnyxEntry<OnyxTypes.Transaction>): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this change! It clearly separates the computation of the diff from the surrounding logic of getUpdateMoneyRequestParams. That function is already long enough.

Copy link
Contributor

@cubuspl42 cubuspl42 Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, would you have anything against changing this name to calculateDiffAmount? "Calculation" is "computing" but with numbers. I should have suggested this name in the first place.

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 agree.

if (!iouReport) {
return 0;
}
const isExpenseReport = ReportUtils.isExpenseReport(iouReport);
const updatedCurrency = TransactionUtils.getCurrency(updatedTransaction);
const currentCurrency = TransactionUtils.getCurrency(transaction);

if (updatedCurrency === iouReport?.currency && currentCurrency !== iouReport?.currency) {
// add the diff with the total if we change the currency from different currency to the currency of the iou report
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the diff to the total if we change the currency from a different currency to the currency of the IOU report

Copy link
Contributor

Choose a reason for hiding this comment

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

If English is not your native language, it can be very hard to use "a" and "the" correctly. In general, we use "a"/"an" if we talk about an entity/person/object from a group/set/category, without having a specific instance in mind (jn the given context). We use "the" when we talk about a specific instance (in the given context).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your knowledge, noted it and will improve in the future.

return TransactionUtils.getAmount(updatedTransaction, isExpenseReport);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add new helper consts above the if blocks, like...

const oldAmount = TransactionUtils.getAmount(transaction, isExpenseReport);
const updateAmount = TransactionUtils.getAmount(updatedTransaction, isExpenseReport);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with two variables before the if blocks.

}
if (updatedCurrency !== iouReport?.currency && currentCurrency === iouReport?.currency) {
// subtract the diff with the total if we change the currency from the currency of iou report to different currency
Copy link
Contributor

Choose a reason for hiding this comment

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

Subtract the diff from the total if we change the currency from the currency of IOU report to a different currency

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 updated this comment.

return -TransactionUtils.getAmount(updatedTransaction, isExpenseReport);
}
if (updatedCurrency === iouReport?.currency && updatedTransaction?.modifiedAmount) {
// get the diff between the updated amount and the current amount if we change the amount and the currency of the transaction is the currency of the report
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't spot any language mistakes! 😃

I suggested to start the other comment with a capital letter, so we could do this here, too, for consistency. We could also say "calculate", as it's more specific than "get".

Calculate the diff between the updated amount and the current amount if we change the amount and the currency of the transaction is the currency of the report

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 updated this comment.

return TransactionUtils.getAmount(updatedTransaction, isExpenseReport) - TransactionUtils.getAmount(transaction, isExpenseReport);
}

return 0;
}

/**
* @param transactionChanges
* @param [transactionChanges.created] Present when updated the date field
Expand Down Expand Up @@ -1050,31 +1077,29 @@ function getUpdateMoneyRequestParams(
});

// Step 4: Compute the IOU total and update the report preview message (and report header) so LHN amount owed is correct.
// Should only update if the transaction matches the currency of the report, else we wait for the update
// from the server with the currency conversion
let updatedMoneyRequestReport = {...iouReport};
if (updatedTransaction?.currency === iouReport?.currency && updatedTransaction?.modifiedAmount) {
const diff = TransactionUtils.getAmount(transaction, true) - TransactionUtils.getAmount(updatedTransaction, true);
if (ReportUtils.isExpenseReport(iouReport) && typeof updatedMoneyRequestReport.total === 'number') {
updatedMoneyRequestReport.total += diff;
} else {
updatedMoneyRequestReport = iouReport
? IOUUtils.updateIOUOwnerAndTotal(iouReport, updatedReportAction.actorAccountID ?? -1, diff, TransactionUtils.getCurrency(transaction), false)
: {};
}
const diff = computeDiffAmount(iouReport, updatedTransaction, transaction);

updatedMoneyRequestReport.cachedTotal = CurrencyUtils.convertToDisplayString(updatedMoneyRequestReport.total, updatedTransaction.currency);
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport?.reportID}`,
value: updatedMoneyRequestReport,
});
successData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport?.reportID}`,
value: {pendingAction: null},
});
if (ReportUtils.isExpenseReport(iouReport) && typeof updatedMoneyRequestReport.total === 'number') {
// For expense report, the amount is negative so we should subtract total from diff
updatedMoneyRequestReport.total -= diff;
} else {
updatedMoneyRequestReport = iouReport
? IOUUtils.updateIOUOwnerAndTotal(iouReport, updatedReportAction.actorAccountID ?? -1, diff, TransactionUtils.getCurrency(transaction), false)
: {};
}
updatedMoneyRequestReport.cachedTotal = CurrencyUtils.convertToDisplayString(updatedMoneyRequestReport.total, updatedTransaction?.modifiedCurrency);

optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport?.reportID}`,
value: updatedMoneyRequestReport,
});
successData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport?.reportID}`,
value: {pendingAction: null},
});
}

// Optimistically modify the transaction
Expand Down
Loading