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

[CP Staging] Fix bug when updating tax rate to zero #42065

Merged
merged 5 commits into from
May 14, 2024
Merged

[CP Staging] Fix bug when updating tax rate to zero #42065

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dc578e9
Remove uncessary checks and improve checks
MonilBhavsar May 13, 2024
45368b3
Add a check for null
MonilBhavsar May 13, 2024
54503e1
Update the check and type definition
MonilBhavsar May 13, 2024
97534f4
Revert a change
MonilBhavsar May 13, 2024
ba16604
Fix typescript check
MonilBhavsar May 13, 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
4 changes: 2 additions & 2 deletions src/components/MoneyRequestConfirmationList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,8 @@ function MoneyRequestConfirmationList({
const taxAmount = getTaxAmount(transaction, policy).toString();
const amountInSmallestCurrencyUnits = CurrencyUtils.convertToBackendAmount(Number.parseFloat(taxAmount));

if (transaction?.taxAmount && previousTransactionAmount === transaction?.amount && previousTransactionCurrency === transaction?.currency) {
return IOU.setMoneyRequestTaxAmount(transaction?.transactionID, transaction?.taxAmount, true);
if (transaction?.taxAmount !== null && previousTransactionAmount === transaction?.amount && previousTransactionCurrency === transaction?.currency) {
return IOU.setMoneyRequestTaxAmount(transactionID, transaction?.taxAmount ?? 0, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to revert the change in this file: src/components/MoneyRequestConfirmationList.tsx

I am not sure why we need to apply this change but It causes this bug:

The Tax amount is 0 when creating a new money request

Screen.Recording.2024-05-14.at.00.32.17.mov

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 think this is because of currency. Could you try with another currencies like GBP, EUR, USD and such

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 am not sure why we need to apply this change but It causes this bug:

transaction?.taxAmount check is wrong because if taxAmount is zero, it doesn't work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw similar bug here #40443 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Screen.Recording.2024-05-14.at.00.39.41.mov

This bug still reproduces with other currencies, I checked the RCA and the reason is that we added the null check in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

App/src/components/MoneyRequestConfirmationList.tsx

Lines 377 to 381 in 5c619c7

if (transaction?.taxAmount && previousTransactionAmount === transaction?.amount && previousTransactionCurrency === transaction?.currency) {
return IOU.setMoneyRequestTaxAmount(transaction?.transactionID, transaction?.taxAmount, true);
}
IOU.setMoneyRequestTaxAmount(transactionID, amountInSmallestCurrencyUnits, true);

In this case, transaction?.taxAmount is 0 so we need to use amountInSmallestCurrencyUnits to update taxAmount

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'll check this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll revert for now, but I'm pretty sure this is responsible for an undiscovered bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, and thank you for thorough test!

}

IOU.setMoneyRequestTaxAmount(transactionID, amountInSmallestCurrencyUnits, true);
Expand Down
6 changes: 3 additions & 3 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2847,7 +2847,7 @@ type UpdateMoneyRequestTaxRateParams = {
transactionID: string;
optimisticReportActionID: string;
taxCode: string;
taxAmount?: number;
taxAmount: number;
policy: OnyxEntry<OnyxTypes.Policy>;
policyTagList: OnyxEntry<OnyxTypes.PolicyTagList>;
policyCategories: OnyxEntry<OnyxTypes.PolicyCategories>;
Expand All @@ -2857,7 +2857,7 @@ type UpdateMoneyRequestTaxRateParams = {
function updateMoneyRequestTaxRate({transactionID, optimisticReportActionID, taxCode, taxAmount, policy, policyTagList, policyCategories}: UpdateMoneyRequestTaxRateParams) {
const transactionChanges = {
taxCode,
...(taxAmount && {taxAmount}),
...(taxAmount !== null && {taxAmount}),
};
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, optimisticReportActionID, transactionChanges, policy, policyTagList, policyCategories, true);
API.write('UpdateMoneyRequestTaxRate', params, onyxData);
Expand Down Expand Up @@ -5062,7 +5062,7 @@ function updateMoneyRequestAmountAndCurrency({
const transactionChanges = {
amount,
currency,
...(taxAmount && {taxAmount}),
...(taxAmount !== null && {taxAmount}),
};
const transactionThreadReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`] ?? null;
const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReport?.parentReportID}`] ?? null;
Expand Down
Loading