-
Notifications
You must be signed in to change notification settings - Fork 16
MWPW-185252 [M@S] Tax label and Include tax toggle broken #498
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
base: main
Are you sure you want to change the base?
Changes from all commits
23e7724
5857960
fadfd0a
dc26918
f593db2
133f948
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,7 +72,7 @@ const DISPLAY_TAX_MAP = { | |
| 'KR_ko', | ||
| ], | ||
| [BUSINESS]: ['MU_en', 'LT_lt', 'LV_lv', 'NG_en', 'CO_es', 'KR_ko'], | ||
| [STUDENT]: ['LT_lt', 'LV_lv', 'SA_en', 'SG_en'], | ||
| [STUDENT]: ['LT_lt', 'LV_lv', 'SA_en', 'SA_ar', 'SG_en'], | ||
| [UNIVERSITY]: ['SG_en', 'KR_ko'], | ||
| }; | ||
|
|
||
|
|
@@ -94,6 +94,28 @@ const TAX_EXCLUDED_MAP_INDEX = [INDIVIDUAL, BUSINESS, STUDENT, UNIVERSITY]; | |
| const defaultTaxExcluded = (segment) => | ||
| [BUSINESS, UNIVERSITY].includes(segment); | ||
|
|
||
| const getFromMap = (map, country, language, isArray) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you use a named function with a more explicit name, it would help in the stacktrace. |
||
| if (map[country]) return map[country]; | ||
| const locale = `${country}_${language}`; | ||
| if (map[locale]) return map[locale]; | ||
|
|
||
| let result; | ||
| if (isArray) { | ||
| map.forEach((item) => { | ||
| if (!result && item.startsWith(`${country}_`)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since forEach cannot be broken, it is not optimal for performance. const result = map.find(item => item.startsWith(`${country}_`)); |
||
| result = item; | ||
| } | ||
| }); | ||
| } else { | ||
| Object.keys(map).forEach((key) => { | ||
| if (!result && key.startsWith(`${country}_`)) { | ||
| result = map[key]; | ||
| } | ||
| }); | ||
| } | ||
| return result; | ||
| }; | ||
|
|
||
| /** | ||
| * Resolves the default value for forceTaxExclusive for the provided geo info and segments. | ||
| * @param {string} country - uppercase country code e.g. US, AT, MX | ||
|
|
@@ -108,9 +130,8 @@ const resolveTaxExclusive = ( | |
| customerSegment, | ||
| marketSegment, | ||
| ) => { | ||
| const locale = `${country}_${language}`; | ||
| const segment = `${customerSegment}_${marketSegment}`; | ||
| const val = TAX_EXCLUDED_MAP[locale]; | ||
| const val = getFromMap(TAX_EXCLUDED_MAP, country, language, false); | ||
| if (val) { | ||
| const index = TAX_EXCLUDED_MAP_INDEX.indexOf(segment); | ||
| return val[index]; | ||
|
|
@@ -133,21 +154,16 @@ const resolveDisplayTaxForGeoAndSegment = ( | |
| customerSegment, | ||
| marketSegment, | ||
| ) => { | ||
| const locale = `${country}_${language}`; | ||
| if ( | ||
| DISPLAY_ALL_TAX_COUNTRIES.includes(country) || | ||
| DISPLAY_ALL_TAX_COUNTRIES.includes(locale) | ||
| ) { | ||
| if (getFromMap(DISPLAY_ALL_TAX_COUNTRIES, country, language, true)) | ||
| return true; | ||
| } | ||
|
|
||
| const segmentConfig = | ||
| DISPLAY_TAX_MAP[`${customerSegment}_${marketSegment}`]; | ||
| if (!segmentConfig) { | ||
| return Defaults.displayTax; | ||
| } | ||
|
|
||
| if (segmentConfig.includes(country) || segmentConfig.includes(locale)) { | ||
| if (getFromMap(segmentConfig, country, language, true)) { | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Cannot find this change listed in the description of the Jira: https://jira.corp.adobe.com/browse/MWPW-185252 , could you please link the Jira where it was requested?
I included this change in my PR: #499 to not cause conflicts/regressions, and updated the documentation I added accordingly.
And in the PR description you need to update the first line:
Resolves https://jira.corp.adobe.com/browse/MWPW-NUMBER > Resolves https://jira.corp.adobe.com/browse/MWPW-185252
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.
Right. Good catch. This is not requested anywhere. This is something that I noticed in production and I think it is a mistake in that excel file with tax included/excluded per country. For students it is included for sa_en but excluded for sa_ar (and tax label not displayed), which is not logical at all. So I changed that table in the code to have it as we have now in production.
But I will double check this with Lucy and Ravneet. Thanks for reminding me. Will CC you.
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.
https://jira.corp.adobe.com/browse/MWPW-185252?focusedId=51912866&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-51912866