-
Notifications
You must be signed in to change notification settings - Fork 361
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
feat: [M3-7919] Add Fonoa tax id event notification on invalidation #10512
Conversation
@@ -96,6 +96,7 @@ export const EVENT_ACTIONS: Event['action'][] = [ | |||
'subnet_create', | |||
'subnet_delete', | |||
'subnet_update', | |||
'tax_id_invalid', |
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.
This is the only new action, all the others needed alphabetization
@@ -861,6 +861,9 @@ export const eventMessageCreators: { [index: string]: CreatorsForStatus } = { | |||
tag_delete: { | |||
notification: (e) => `Tag ${e.entity!.label} has been deleted.`, | |||
}, | |||
tax_id_invalid: { | |||
notification: (e) => `Tax Identification Number format is invalid.`, | |||
}, |
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.
This is the only new notification, all the others were just formatting done by eslint
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.
You will need to rebase your PR cause there's been a whole lot of changes to this and this new event will need to be added to event messages V2. pls reach out if needs clarification
...BillingPanels/ContactInfoPanel/UpdateContactInformationForm/UpdateContactInformationForm.tsx
Outdated
Show resolved
Hide resolved
@bnussman-akamai thanks for review, see: 0d23300 |
Coverage Report: ✅ |
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 tested everything but the events/notifications. Generally, things looked good but I noticed a couple of edge cases:
- If a user lives in a country with a Tax ID and sets + saves that value, and then reopens the drawer to update their country to the
United States of America
, they will still see the Tax ID text field and, when submitting the form, also see the Tax ID show up in the saved Billing Contact section of the Account landing page. They will not see a toast upon form submission, though, as we'd expect.
And one UX question: did UX opt to place the helper text outside of a tooltip for visibility, even though it is long?
General question: I see we have an event for an invalid tax ID but nothing else. I'm curious as to why we didn't go for an account notification or a new endpoint/field. There is no way for us to know on the frontend if the event is still relevant/true |
This is a good question! I proposed it to the team for an answer.
I believe this is expected if I'm understanding what you're saying correctly. If the country is the US, they will not see a toast indicating their tax id is being validated even if it's included in the POST.
Yea due to the fact that invalid or no tax ids (when required) will be caught and rejected at the time of initiating any B2C sales |
Spoke with API and we're going to improve this (transition to using |
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.
Thanks for clarifying and checking on the US islands.
If a user lives in a country with a Tax ID and sets + saves that value, and then reopens the drawer to update their country to the United States of America, they will still see the Tax ID text field and, when submitting the form, also see the
Tax ID show up in the saved Billing Contact section of the Account landing page. They will not see a toast upon form submission, though, as we'd expect.I believe this is expected if I'm understanding what you're saying correctly. If the country is the US, they will not see a toast indicating their tax id is being validated even if it's included in the POST.
If this is the case, then the UI can show another country's Tax ID beside a US address, which seems strange. I think a user would expect that, if they were to select a new country (or at least a country without Tax ID - i.e. US and maybe US islands) then the Tax ID text field would get cleared.
@@ -432,6 +432,7 @@ export const EventActionKeys = [ | |||
'subnet_update', | |||
'tag_create', | |||
'tag_delete', | |||
'tax_id_invalid', |
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.
This is the only real change here
@mjac0bs I should have fixed the issue you pointed out |
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.
factory.test.tsx
is failing because we no longer have a defined event message for the new event key tax_id_invalid
. This looks like it will need a new factory for tax, as described here with "Tax Identification Number format is invalid." added as a message for this action.
Approving pending that is fixed and tests pass, as well as we get confirmation of whether the US Virgin and Minor Outlying Islands are not being validated for tax ids.
The clearing of the Tax ID field upon selection of another tax id is working as expected now.
I haven't received any information about US territories, so we'll proceed without that for now |
Thanks, I like the new way events are typed now 🎉 |
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.
Approving pending a couple nits
Co-authored-by: Alban Bailly <[email protected]>
Description 📝
To enhance our tax ID validation process, we've updated Cloud Manager to offer clearer guidance on these UX changes.
Changes 🔄
tax_id_invalid
is returned and emitted if the tax id is invalid.Target release date 🗓️
6/10
Preview 📷
How to test 🧪
Prerequisites
Reproduction steps
TaxId
Verification steps
As an Author I have considered 🤔
Check all that apply