support for offline tax tracking #40443
Conversation
teneeto
changed the title
…6/support-for-offline-tax-tracking
…6/support-for-offline-tax-tracking
…6/support-for-offline-tax-tracking
…6/support-for-offline-tax-tracking
…6/support-for-offline-tax-tracking
|
I noticed a bug -
Looks like we're updating it correctly in draft transaction in Onyx, but not displaying it 
See taxAmount in Onyx is 45 and in UI it is 0 |
This is bug with certain currencies where amount in small decimals is not supported. Not related to this PR |
|
✋ 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/MonilBhavsar in version: 1.4.73-0 🚀
|
|
Hey @teneeto @MonilBhavsar. I believe this deploy blocker was introduced by this large PR. Would you mind taking a look? As this is a huge PR reverting isn't going to be ideal so ideally we can get a quick fix submitted. |
| taxCode, | ||
| taxAmount, |
There was a problem hiding this comment.
This is causing the deploy blocker: #42114 (comment)
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.73-7 🚀
|
|
hey, this PR caused a blocker - #42045 |
| const transactionChanges = { | ||
| taxCode, | ||
| ...(taxAmount && {taxAmount}), |
There was a problem hiding this comment.
It caused this bug, when taxAmount is zero, the value will be ignored
| const getTaxAmount = (transaction: OnyxEntry<OnyxTypes.Transaction>, policy: OnyxEntry<OnyxTypes.Policy>) => { | ||
| const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, transaction) ?? ''; | ||
|
|
||
| const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, transaction?.taxCode ?? defaultTaxCode) ?? ''; |
There was a problem hiding this comment.
When the currency differs from the current transaction currency, the taxCode should have also changed to the default (i.e. local or foreign based on the newly updated currency) tax rate. And, the tax amount should also have been recomputed. Missing these changes here resulted in issue #42049.
| if (transaction?.taxAmount && previousTransactionAmount === transaction?.amount && previousTransactionCurrency === transaction?.currency) { | ||
| return IOU.setMoneyRequestTaxAmount(transaction?.transactionID, transaction?.taxAmount, true); | ||
| } |
There was a problem hiding this comment.
What is this block doing here? is it here in case the currency didn't change?
if transaction?.taxAmount && previousTransactionAmount, what is the purpose of calling IOU.setMoneyRequestTaxAmount? is the draft in a different state?
| const defaultTaxCode = () => { | ||
| if (!transaction) { | ||
| return defaultExternalID; | ||
| } | ||
|
|
||
| return policy && getDefaultTaxCode(policy, transaction); | ||
| }; |
There was a problem hiding this comment.
@teneeto Why did we wrap this code in a function? Why didn't we just do something like:
const defaultTaxCode = !transaction ? defaultExternalID : policy && getDefaultTaxCode(policy, transaction);
Are you expecting that policy or transaction can mutate?
| const moneyRequestTaxPercentage = (transaction?.taxRate ? transaction?.taxRate?.data?.value : defaultTaxValue) ?? ''; | ||
| const editingTaxPercentage = (transactionTaxCode ? taxRates?.taxes[transactionTaxCode]?.value : moneyRequestTaxPercentage) ?? ''; | ||
| const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, transaction) ?? ''; | ||
| const getTaxValue = (taxCode: string) => TransactionUtils.getTaxValue(policy, transaction, taxCode); |
There was a problem hiding this comment.
Why did we create this getTaxValue function, it is only used inside here, right? is it just to type getTaxValue(xxx) instead of TransactionUtils.getTaxValue(policy, transaction, taxCode)?
| if (percentage) { | ||
| return TransactionUtils.calculateTaxAmount(percentage, amount); | ||
| function getTaxAmount(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>, selectedTaxCode: string, amount: number): number | undefined { | ||
| const getTaxValue = (taxCode: string) => TransactionUtils.getTaxValue(policy, transaction, taxCode); |
There was a problem hiding this comment.
Why did we define a function here? this makes the code more confusing
Details
This PR adds Support for tax tracking when offline.
Fixed Issues
$: 39616
PROPOSAL: 39616
Tests
Submit expense(money request) flow.Expectation: when currency is changed from
Policy default currencyto another currency then let's make sure we use tax rate asforeign defaultelse it should remain asworkspace default.Expectation: when changing request amount and tax rate offline, the tax amount will change accordingly and immediately.
Offline tests
Submit expense(money request) flow.Expectation: when currency is changed from
Policy default currencyto another currency then let's make sure we use tax rate asforeign defaultelse it should remain asworkspace default.Expectation: when changing request amount and tax rate offline, the tax amount will change accordingly and immediately.
QA Steps
Submit expense(money request) flow.Expectation: when currency is changed from
Policy default currencyto another currency then let's make sure we use tax rate asforeign defaultelse it should remain asworkspace default.Expectation: when changing request amount and tax rate offline, the tax amount will change accordingly and immediately.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
MacOS: Chrome / Safari
Screen.Recording.2024-04-18.at.15.18.16.mov
iOS: Native
Android: Native
Android: mWeb Chrome
iOS: mWeb Safari
MacOS: Desktop