-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: add default rates data #38375
fix: add default rates data #38375
Conversation
src/CONST.ts
Outdated
TAXES: { | ||
DEFAULT: { | ||
EXTERNAL_ID: 'id_TAX_EXEMPT', | ||
FOREIGN_ID: 'id_TAX_EXEMPT', | ||
VALUE: '0%', | ||
EXEMPT_ID: 'id_TAX_EXEMPT', | ||
NAME: 'Tax', | ||
EXEMPT_NAME: 'Tax exempt', | ||
EXEMPT_VALUE: '0%', | ||
TAX_RATE_1_ID: 'id_TAX_RATE_1', | ||
TAX_RATE_1_NAME: 'Tax Rate 1', | ||
TAX_RATE_1_VALUE: '5%', | ||
}, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These data are obtained when I try turning on the tax feature in OD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins @kosmydel @filip-solecki
Please confirm if the following default tax rates as per @nkdengineer proposal would match the BE defaults.
Also, is localization required for name
i.e. "Tax", "Tax exempt", "Tax option 1"?
{
"defaultExternalID": "id_TAX_EXEMPT",
"defaultValue": "0%",
"foreignTaxDefault": "id_TAX_EXEMPT",
"name": "Tax",
"taxes": {
"id_TAX_EXEMPT": {
"name": "Tax exempt",
"value": "0%"
},
"id_TAX_OPTION_1": {
"name": "Tax option 1",
"value": "5%"
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ "defaultExternalID": "id_TAX_EXEMPT", "defaultValue": "0%", "foreignTaxDefault": "id_TAX_EXEMPT", "name": "Tax", "taxes": { "id_TAX_EXEMPT": { "name": "Tax exempt", "value": "0%" }, "id_TAX_RATE_1": { "name": "Tax rate 1", "value": "5%" } } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment above, I think the only difference is id_TAX_OPTION_1
should be id_TAX_RATE_1
and same for the name Tax option 1
-> Tax rate 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment above, I think the only difference is
id_TAX_OPTION_1
should beid_TAX_RATE_1
and same for the nameTax option 1
->Tax rate 1
I think the code has incorporated most of these already. Only one minor change is pending: Tax Rate 1
-> Tax rate 1
@nkdengineer |
@rojiphil I don't see any comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't submit the review comments. Here they are.
src/CONST.ts
Outdated
TAXES: { | ||
DEFAULT: { | ||
EXTERNAL_ID: 'id_TAX_EXEMPT', | ||
FOREIGN_ID: 'id_TAX_EXEMPT', | ||
VALUE: '0%', | ||
EXEMPT_ID: 'id_TAX_EXEMPT', | ||
NAME: 'Tax', | ||
EXEMPT_NAME: 'Tax exempt', | ||
EXEMPT_VALUE: '0%', | ||
TAX_RATE_1_ID: 'id_TAX_RATE_1', | ||
TAX_RATE_1_NAME: 'Tax Rate 1', | ||
TAX_RATE_1_VALUE: '5%', | ||
}, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins @kosmydel @filip-solecki
Please confirm if the following default tax rates as per @nkdengineer proposal would match the BE defaults.
Also, is localization required for name
i.e. "Tax", "Tax exempt", "Tax option 1"?
{
"defaultExternalID": "id_TAX_EXEMPT",
"defaultValue": "0%",
"foreignTaxDefault": "id_TAX_EXEMPT",
"name": "Tax",
"taxes": {
"id_TAX_EXEMPT": {
"name": "Tax exempt",
"value": "0%"
},
"id_TAX_OPTION_1": {
"name": "Tax option 1",
"value": "5%"
}
}
}
@luacmartins |
We're deprecating |
src/CONST.ts
Outdated
TAXES: { | ||
DEFAULT: { | ||
EXTERNAL_ID: 'id_TAX_EXEMPT', | ||
FOREIGN_ID: 'id_TAX_EXEMPT', | ||
VALUE: '0%', | ||
EXEMPT_ID: 'id_TAX_EXEMPT', | ||
NAME: 'Tax', | ||
EXEMPT_NAME: 'Tax exempt', | ||
EXEMPT_VALUE: '0%', | ||
TAX_RATE_1_ID: 'id_TAX_RATE_1', | ||
TAX_RATE_1_NAME: 'Tax Rate 1', | ||
TAX_RATE_1_VALUE: '5%', | ||
}, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment above, I think the only difference is id_TAX_OPTION_1
should be id_TAX_RATE_1
and same for the name Tax option 1
-> Tax rate 1
src/libs/actions/Policy.ts
Outdated
defaultExternalID: CONST.TAXES.DEFAULT.EXTERNAL_ID, | ||
defaultValue: CONST.TAXES.DEFAULT.VALUE, | ||
foreignTaxDefault: CONST.TAXES.DEFAULT.FOREIGN_ID, | ||
name: CONST.TAXES.DEFAULT.NAME, | ||
taxes: { | ||
[CONST.TAXES.DEFAULT.EXEMPT_ID]: { | ||
name: CONST.TAXES.DEFAULT.EXEMPT_NAME, | ||
value: CONST.TAXES.DEFAULT.EXEMPT_VALUE, | ||
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, | ||
}, | ||
[CONST.TAXES.DEFAULT.TAX_RATE_1_ID]: { | ||
name: CONST.TAXES.DEFAULT.TAX_RATE_1_NAME, | ||
value: CONST.TAXES.DEFAULT.TAX_RATE_1_VALUE, | ||
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a const with these defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins Apologies as it looks like I introduced some confusion here. I was unaware of your earlier review comments about using individual values in CONST to generate taxRates
object. But I am ok with that too.
So, I need a clarification here.
Did you mean creating a const
object for taxRates
in the code itself and set it to value
or did you mean creating the entire taxRates
object in CONST?
The reason why I ask is that @nkdengineer has reverted the code in the latest commit to create the entire taxRates
object in CONST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about this one, I'm not sure that it's this simple. We only want to create these defaults if the policy doesn't have any taxRates yet, so we should check that taxRates is empty before we create the defaults.
For example, in OldDot, if you have tax rates created and then turn off/on the feature, we display the previous tax rates you had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think we should send this object to the API so that the frontend and backend don't get out of sync in case one changes
The format for the API is:
{"fieldID":"field_id_TAX","type":"tax","defaultValue":"0%","defaultExternalID":null,"value":null,"name":"Tax","target":"expense","values":["5%","0%"],"keys":["Tax Rate 1","Tax exempt"],"orderWeight":0,"deletable":false,"isTax":true,"externalIDs":[],"disabledOptions":[false,false],"externalID":null,"modifiedTaxAmount":null,"origin":null,"foreignTaxDefault":null}
I'll adjust the API to accept the taxFields
param as described above. Same thing, we only want to send this param if there's no data in taxRates
@luacmartins While investigating OD for the existing enable/disable tax feature, I noticed that, during fresh sign-in, the |
That sounds cool. I think just a small typo to note here while implementing: |
Reviewer Checklist
Screenshots/VideosAndroid: Native38234-android-native-01.mp4Android: mWeb Chrome38234-mweb-chrome-01.mp4iOS: Native38234-ios-native-01.mp4iOS: mWeb Safari38234-mweb-safari-01.mp4MacOS: Chrome / Safari38234-web-safari-01.mp4MacOS: Desktop38234-desktop-01.mp4 |
That's a good point. We should send this data when loading the more features page so we have the data if it exists. I can incorporate that in the backend PR I'm working on. Thanks for bringing this up. |
Update: I just resolved all the above comments except this comment because I need to confirm, and add screenshot (except ios native because of a device issue) |
@nkdengineer i think it makes sense to send the data to the backend just like we do for other optimistic data. I think the advantage is that it'll make sure that the FE and BE are in sync and create/show the same data. Otherwise if we ever change the data in one place, we'd have to update the other to match witch might cause inconsistencies when being deployed. |
@luacmartins Along with the suggested changes here the overall FE code looks super good to go unless you have any further review comments to implement. I have also ticked most of the reviewer checklist. But I think there are a couple of issues also to be addressed by BE:
Do we wait for BE changes too? |
Let's rename it here to be
Yes, I still need to work on the API to save this data. |
Co-authored-by: Carlos Martins <[email protected]>
@luacmartins Fixed all your comments |
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Added #38859 to the list of fixed issues here. |
@nkdengineer @rojiphil @luacmartins looks like we have a regression from this PR #39154 |
@MonilBhavsar It doesn't look like this PR was included the build in the first place. So, I doubt if this is the offending PR. |
The PR being that list indicates that it was included in the build. The unchecked item indicates that this item is yet to be QA'ed by the QA team |
Ok. Thanks for the clarification. This is a strange issue as we have not touched anything related to navigation. |
Thank you! Sorry if this is not the offending PR. This one stood suspicious from the difference and QA also mentioned they found the issue while QA'ing this PR |
Details
Fixed Issues
$ #38234
$ #38859
PROPOSAL: #38234 (comment)
Tests
Test Steps:
p = Policy.getCurrent(); p.policy.isPolicyExpenseChatEnabled = "true"; p.save();
Offline tests
QA Steps
p = Policy.getCurrent(); p.policy.isPolicyExpenseChatEnabled = "true"; p.save();
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-03-15.at.17.35.39.mov
Android: mWeb Chrome
Screen.Recording.2024-03-15.at.16.27.43.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2024-03-16.at.09.58.13.mov
MacOS: Chrome / Safari
output.mp4
MacOS: Desktop
Screen.Recording.2024-03-15.at.16.13.41.mov