change: [M3-7847] - Add Akamai's Japanese QI System ID to Japanese Invoices. #10356
Conversation
cpathipa
requested review from
bnussman-akamai and
jaalah-akamai
and removed request for
a team
| @@ -5,6 +5,7 @@ import type { NoticeVariant } from 'src/components/Notice/Notice'; | |||
| // These flags should correspond with active features flags in LD | |||
|
|
|||
| export interface TaxDetail { | |||
| qi_registration?: string; | |||
There was a problem hiding this comment.
Any thoughts or suggestions regarding the addition or maintenance of IQ registration numbers in LD? Since it's related to the country, I was thinking of adding an optional new field to accommodate this for JP.
{
"country_tax": {
"tax_id": "******",
"tax_name": "Japan JCT",
"qi_registration": "**********"
},
"provincial_tax_ids": {}
}
There was a problem hiding this comment.
Im fine with that unless someone can convince me otherwise :P
There was a problem hiding this comment.
Yup! good adjustment and workaround 👍
|
Coverage Report: ✅ |
| addLine(`${countryTax.tax_name}: ${countryTax.tax_id}`); | ||
| const line = | ||
| country === 'JP' | ||
| ? `QI Registration # ${countryTax.qi_registration}` |
There was a problem hiding this comment.
considering qi_registration could be undefined, how about:
country === 'JP' && countryTax.qi_registration
? ...
: ...
?
or provide a fallback for countryTax.qi_registration? (what we want to display as a fallback may be a product decision)
There was a problem hiding this comment.
We could do that. We could display the default tax ID in case of undefined, which shouldn't be a problem once it is added in LD. f9e5891
There was a problem hiding this comment.
Do we need to have the country === 'JP'? If a qi_registration is in launch darkly, maybe we should render it at all times for any country
There was a problem hiding this comment.
@bnussman-akamai First part - Yes, since its specific to 'JP'.
For the second part: - Are you suggesting that qi_registration shouldn't be part of LD?
There was a problem hiding this comment.
I'm pretty sure Launch Darkly conditionally serves the Tax data based on the user's country automatically. I think just checking for qi_registration would be sufficient, but I guess keeping country === 'JP' would be okay too.
If we remove country === 'JP', we'd be able to push out qi_registration changes for other countries without any code changes required. I'm not sure if that will ever happen, but maybe it would?
There was a problem hiding this comment.
TY @bnussman-akamai Yeah, technically we can rely on qi_registration 34a19d1
| country === 'JP' | ||
| ? `QI Registration # ${countryTax.qi_registration}` | ||
| : `${countryTax.tax_name}: ${countryTax.tax_id}`; | ||
| addLine(line); |
There was a problem hiding this comment.
Am wondering if we could start a map instead of a one of exception. We may get more exception like this one soon?
There was a problem hiding this comment.
Hoping this 34a19d1 could address need of map.
There was a problem hiding this comment.
Looks good!
In the internal ticket, it didn't seem super clear to me if we want both the Tax ID and the QI number to show, or only the QI number if one is present.
I just want to make sure we're okay with not showing the Tax ID when a QI number is present in the Launch Darkly data.
|
@bnussman-akamai Based on the screenshot attached to the ticket my understanding is only QI. But, I will double check on that. |
| @@ -5,6 +5,7 @@ import type { NoticeVariant } from 'src/components/Notice/Notice'; | |||
| // These flags should correspond with active features flags in LD | |||
|
|
|||
| export interface TaxDetail { | |||
| qi_registration?: string; | |||
There was a problem hiding this comment.
Yup! good adjustment and workaround 👍
Description 📝
Add Akamai's Japanese QI System ID to Japanese Invoices.
Changes 🔄
List any change relevant to the reviewer.
Example:
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply