[CP Staging] Use selected currency when utilising tax rate #42064

ShridharGoel · 2024-05-13T13:31:49Z

Details

Use selected currency when utilising tax rate.

Fixed Issues

$ #42047
PROPOSAL: #42047 (comment)

Tests

Set default tax as 0 and foreign tax as some value like 5%.
Go to submit expense flow.
Try to change the tax amount.
It should change to the new amount if it is below the 5% limit.

QA Steps

Set default tax as 0 and foreign tax as some value like 5%.
Go to submit expense flow.
Try to change the tax amount.
It should change to the new amount if it is below the 5% limit.

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

Screenrecording_20240514_002141.mp4

iOS: Native

Screen.Recording.2024-05-14.at.12.16.13.AM.mov

iOS: mWeb Safari

Screen.Recording.2024-05-14.at.12.18.08.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-05-13.at.11.18.14.PM.mov

MacOS: Desktop

Screen.Recording.2024-05-14.at.12.28.01.AM.mov

melvin-bot · 2024-05-13T13:31:55Z

@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

allroundexperts · 2024-05-13T13:35:03Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android build not working locally

Android: mWeb Chrome

Screen.Recording.2024-05-14.at.12.29.53.AM.mov

iOS: Native

Screen.Recording.2024-05-14.at.12.21.52.AM.mov

iOS: mWeb Safari

Screen.Recording.2024-05-14.at.12.19.05.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-05-13.at.11.47.37.PM.mov

MacOS: Desktop

Screen.Recording.2024-05-14.at.12.13.12.AM.mov

allroundexperts · 2024-05-13T13:45:51Z

@ShridharGoel TS errors.

MonilBhavsar · 2024-05-13T13:58:27Z

src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx

@@ -34,13 +34,13 @@ type IOURequestStepTaxAmountPageProps = IOURequestStepTaxAmountPageOnyxProps &
        transaction: OnyxEntry<Transaction>;
    };

-function getTaxAmount(transaction: OnyxEntry<Transaction>, policy: OnyxEntry<Policy>, isEditing: boolean): number | undefined {
+function getTaxAmount(transaction: OnyxEntry<Transaction>, policy: OnyxEntry<Policy>, currency: string | undefined, isEditing: boolean): number | undefined {


Can you pass currency from all function usages also, please

~~Passing currency from src/pages/iou/request/step/IOURequestStepAmount.tsx might fix #42049~~ NVM

ShridharGoel · 2024-05-13T14:26:51Z

Updating

ShridharGoel · 2024-05-13T14:30:53Z

Should we pass currency to transformedTaxRates as well? It calls getDefaultTaxCode, so we can pass it further to getDefaultTaxCode.

MonilBhavsar · 2024-05-13T14:34:05Z

Yes, I think we should update all usages

ShridharGoel · 2024-05-13T14:35:12Z

We don't have currency in OptionsListUtils though.

ShridharGoel · 2024-05-13T14:36:35Z

Looks like transformedTaxRates needs only the transaction currency. Thoughts?

ShridharGoel · 2024-05-13T14:40:34Z

src/components/MoneyRequestConfirmationList.tsx

@@ -371,7 +371,7 @@ function MoneyRequestConfirmationList({

    // Calculate and set tax amount in transaction draft
    useEffect(() => {
-        const taxAmount = getTaxAmount(transaction, policy).toString();
+        const taxAmount = getTaxAmount(transaction, policy, currency).toString();


@MonilBhavsar Added here as well, does this look fine?

This doesn't look good to me as currency is related to mileage rate or policy default currency. const currency = (mileageRate as MileageRate)?.currency ?? policyCurrency;
We should either use selected currency or transaction currency

src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx

MonilBhavsar · 2024-05-13T14:47:21Z

Looks like transformedTaxRates needs only the transaction currency. Thoughts?

👍 we can leave it. Thanks for checking!

MonilBhavsar · 2024-05-13T16:17:59Z

@allroundexperts over to you

allroundexperts · 2024-05-13T16:19:02Z

Hm... isn't the lint failing?

MonilBhavsar · 2024-05-13T16:20:52Z

@ShridharGoel could you please fix the lint errors

ShridharGoel · 2024-05-13T16:43:59Z

I found another bug which we can fix in this, updating it.

MonilBhavsar · 2024-05-13T16:48:07Z

Just a small feedback, as a reviewer force pushing commits make difficult to know what has been updated since last commit as it deletes commit history.

allroundexperts · 2024-05-13T16:58:45Z

@ShridharGoel Are you done?

ShridharGoel · 2024-05-13T17:11:55Z

The issue which I'm trying to fix: When we change the currency for the first time, on the money request confirmation page it starts showing the max tax amount instead of the amount set by the user. This happens because we have a diff tax calculation logic in MoneyRequestConfirmationList.

ShridharGoel · 2024-05-13T17:47:45Z

Updated.

ShridharGoel · 2024-05-13T17:50:45Z

Screen.Recording.2024-05-13.at.11.18.14.PM.mov

MonilBhavsar · 2024-05-13T18:03:40Z

The video mentioned in issue shows the tax currency being changed. I think then we should just disable the changing option there?

I believe it is already the case. Looking...

MonilBhavsar · 2024-05-13T18:08:02Z

It's a regression from https://github.com/Expensify/App/pull/40159/files
Currency for tax amount is uneditable

ShridharGoel · 2024-05-13T18:14:11Z

@MonilBhavsar No, it's actually from #40443 only.

ShridharGoel · 2024-05-13T18:14:49Z

isEditing definition was changed in #40443.

But actually we should always have isCurrencyPressable as false which I've updated.

MonilBhavsar · 2024-05-13T18:16:49Z

Nice, thanks!
So what is remaining to get final review on this PR. Could you please summarise?

ShridharGoel · 2024-05-13T18:19:23Z

The changes have been completed.

Summary:

We use the selected currency in the flow now.
User can't change the currency via tax editing page.

allroundexperts · 2024-05-13T18:22:00Z

Thanks for letting us know. I'm on the review.

allroundexperts · 2024-05-13T18:33:17Z

@ShridharGoel Can you please update the screen recordings?

ShridharGoel · 2024-05-13T18:35:18Z

On it.

allroundexperts · 2024-05-13T18:48:51Z

Changing the currency resets the tax amount. Is that expected?

ShridharGoel · 2024-05-13T19:01:36Z

I think yes since the tax percent changes.

MonilBhavsar · 2024-05-13T19:01:50Z

Changing the currency resets the tax amount.

Can you please explain "reset". Perhaps with an example

ShridharGoel · 2024-05-13T19:02:05Z

Added all videos except native Android where I have some build issues. Can we start an adhoc build?

ShridharGoel · 2024-05-13T19:04:14Z

Also, I think older tax amount doesn't make sense when the currency is changed so it gets calculated again.

allroundexperts · 2024-05-13T19:16:13Z

@MonilBhavsar Notice how the tax amount changes from 5 to 5.76 after I change the currency at the end.

Screen.Recording.2024-05-14.at.12.13.12.AM.mov

MonilBhavsar · 2024-05-13T19:23:32Z

Yes, Expected

allroundexperts

Tests good!

OSBotify · 2024-05-14T04:43:20Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[CP Staging] Use selected currency when utilising tax rate (cherry picked from commit 2304a1f)

Beamanator · 2024-05-14T06:54:04Z

CPing 👍

…ng-42064-1 🍒 Cherry pick PR #42064 to staging 🍒

OSBotify · 2024-05-14T07:58:56Z

🚀 Deployed to staging by https://github.com/lakchote in version: 1.4.73-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-05-15T09:52:16Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.73-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Use selected currency when utilising tax rate

200aa81

ShridharGoel requested a review from a team as a code owner May 13, 2024 13:31

melvin-bot bot requested review from allroundexperts and removed request for a team May 13, 2024 13:31

ShridharGoel mentioned this pull request May 13, 2024

[HOLD for payment 2024-05-22] Tax - Unable to save valid tax rate amount when changing currency for the first time #42047

Closed

6 tasks

MonilBhavsar reviewed May 13, 2024

View reviewed changes

MonilBhavsar changed the title ~~Use selected currency when utilising tax rate~~ [CP Staging] Use selected currency when utilising tax rate May 13, 2024

ShridharGoel force-pushed the 42407 branch from 825be54 to 4d95e52 Compare May 13, 2024 14:39

ShridharGoel commented May 13, 2024

View reviewed changes

MonilBhavsar reviewed May 13, 2024

View reviewed changes

src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx Show resolved Hide resolved

MonilBhavsar mentioned this pull request May 13, 2024

[PAID] Tax - Tax rate and tax amount does not update when switching currency offline #42049

Closed

6 tasks

ShridharGoel force-pushed the 42407 branch from 4d95e52 to 04cfc54 Compare May 13, 2024 14:49

Update to include selected via multiple places

ca6432e

ShridharGoel force-pushed the 42407 branch from 04cfc54 to ca6432e Compare May 13, 2024 16:36

allroundexperts approved these changes May 13, 2024

View reviewed changes

melvin-bot bot requested a review from lakchote May 13, 2024 19:37

lakchote approved these changes May 14, 2024

View reviewed changes

lakchote merged commit 2304a1f into Expensify:main May 14, 2024
16 of 17 checks passed

OSBotify pushed a commit that referenced this pull request May 14, 2024

Merge pull request #42064 from ShridharGoel/42407

718f9b3

[CP Staging] Use selected currency when utilising tax rate (cherry picked from commit 2304a1f)

os-botify bot mentioned this pull request May 14, 2024

🍒 Cherry pick PR #42064 to staging 🍒 #42129

Merged

Beamanator added a commit that referenced this pull request May 14, 2024

Merge pull request #42129 from Expensify/Beamanator-cherry-pick-stagi…

8e8dc6d

…ng-42064-1 🍒 Cherry pick PR #42064 to staging 🍒

github-actions bot mentioned this pull request May 14, 2024

Deploy Checklist: New Expensify 2024-05-13 #42040

Closed

74 tasks

[CP Staging] Use selected currency when utilising tax rate #42064

[CP Staging] Use selected currency when utilising tax rate #42064

Conversation

ShridharGoel commented May 13, 2024 • edited Loading

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 13, 2024

allroundexperts commented May 13, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

allroundexperts commented May 13, 2024

MonilBhavsar May 13, 2024

Choose a reason for hiding this comment

MonilBhavsar May 13, 2024 • edited Loading

Choose a reason for hiding this comment

ShridharGoel commented May 13, 2024

ShridharGoel commented May 13, 2024

MonilBhavsar commented May 13, 2024

ShridharGoel commented May 13, 2024

ShridharGoel commented May 13, 2024

ShridharGoel May 13, 2024

Choose a reason for hiding this comment

MonilBhavsar May 13, 2024

Choose a reason for hiding this comment

MonilBhavsar commented May 13, 2024

MonilBhavsar commented May 13, 2024

allroundexperts commented May 13, 2024

MonilBhavsar commented May 13, 2024

ShridharGoel commented May 13, 2024

MonilBhavsar commented May 13, 2024

allroundexperts commented May 13, 2024

ShridharGoel commented May 13, 2024

ShridharGoel commented May 13, 2024

ShridharGoel commented May 13, 2024

MonilBhavsar commented May 13, 2024

MonilBhavsar commented May 13, 2024

ShridharGoel commented May 13, 2024

ShridharGoel commented May 13, 2024 • edited Loading

MonilBhavsar commented May 13, 2024

ShridharGoel commented May 13, 2024

allroundexperts commented May 13, 2024

allroundexperts commented May 13, 2024

ShridharGoel commented May 13, 2024

allroundexperts commented May 13, 2024

ShridharGoel commented May 13, 2024

MonilBhavsar commented May 13, 2024

ShridharGoel commented May 13, 2024

ShridharGoel commented May 13, 2024

allroundexperts commented May 13, 2024

MonilBhavsar commented May 13, 2024

allroundexperts left a comment

Choose a reason for hiding this comment

OSBotify commented May 14, 2024

Beamanator commented May 14, 2024

OSBotify commented May 14, 2024

OSBotify commented May 15, 2024

ShridharGoel commented May 13, 2024 •

edited

Loading

allroundexperts commented May 13, 2024 •

edited

Loading

MonilBhavsar May 13, 2024 •

edited

Loading

ShridharGoel commented May 13, 2024 •

edited

Loading