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.

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
2 changes: 1 addition & 1 deletion src/libs/IOUUtils.ts
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
49 changes: 28 additions & 21 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1053,28 +1053,35 @@ function getUpdateMoneyRequestParams(
// 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)
: {};
}

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},
});
let diff = 0;
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 refactor this not to use let? Maybe we could extract a local or non-local helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cubuspl42 I updated to create a local function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I wasn't clear enough. My suggestion was quite technical. I asked specifically whether we could refactor the code not to use the let, i.e. whether the code could be expressed without introducing a mutable variable.

Copy link
Contributor Author

@dukenv0307 dukenv0307 Feb 7, 2024

Choose a reason for hiding this comment

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

got it . I will re-update soon.

Copy link
Contributor

@cubuspl42 cubuspl42 Feb 7, 2024

Choose a reason for hiding this comment

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

Previously, we defined diff like: const diff = ....

I'm suggesting to keep it like this. The easiest way to keep the const is to introduce a helper function for calculating the difference. It can even be local and take no arguments; it's up to you.

function computeDiff() {
  if (someCondition1) {
    return anything;
  } else if (someCondition2) {
    return something;
  } else {
    return somethingElse;
  }
}

const diff = computeDiff();

I think it's best to introduce as few mutable variables to the code as possible.

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, that is the current ideal I will re-update.

Copy link
Contributor

Choose a reason for hiding this comment

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

By "local" I actually meant local in the most-inner surrounding scope. It could also be understood as module-local, as opposed to exported from a module; I didn't mean that.

Copy link
Contributor Author

@dukenv0307 dukenv0307 Feb 7, 2024

Choose a reason for hiding this comment

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

getUpdateMoneyRequestParams function is very complex so I think we should create an outside function.

const isExpenseReport = ReportUtils.isExpenseReport(iouReport);
const updatedCurrency = TransactionUtils.getCurrency(updatedTransaction);
const currentCurrency = TransactionUtils.getCurrency(transaction);
if (updatedCurrency === iouReport?.currency && currentCurrency !== iouReport?.currency) {
diff = TransactionUtils.getAmount(updatedTransaction, isExpenseReport);
} else if (updatedCurrency !== iouReport?.currency && currentCurrency === iouReport?.currency) {
diff = -TransactionUtils.getAmount(updatedTransaction, isExpenseReport);
} else if (updatedCurrency === iouReport?.currency && updatedTransaction?.modifiedAmount) {
diff = TransactionUtils.getAmount(updatedTransaction, isExpenseReport) - TransactionUtils.getAmount(transaction, isExpenseReport);
}
if (isExpenseReport && typeof updatedMoneyRequestReport.total === 'number') {
updatedMoneyRequestReport.total += diff;
} else {
updatedMoneyRequestReport = iouReport
? IOUUtils.updateIOUOwnerAndTotal(iouReport, updatedReportAction.actorAccountID ?? -1, diff, TransactionUtils.getCurrency(updatedTransaction), 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