Add support for Japan to the tax details modal #8974
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +60 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
…ific getter, since it might not work well for Japan
| @@ -24,7 +24,7 @@ const VatFormModal = ( { | |||
| } ): JSX.Element | null => { | |||
| return isModalOpen ? ( | |||
| <Modal | |||
| title={ __( 'VAT details', 'woocommerce-payments' ) } | |||
| title={ __( 'Set your tax details', 'woocommerce-payments' ) } | |||
There was a problem hiding this comment.
Note – I've changed the dialog title for two reasons:
- Remove
VAT, not all regions call itVAT. - Generalise, higher level title.
Re (2)…
The dialog's purpose is to set various tax details – the tax/VAT number, and address details. It's a multi-step process, so I think this title is a clearer summary of both steps.
haszari
changed the title
FYI this is ready for review, but I still need to fix up the e2e tests. Reviewers please let me know if you have suggestions for scenarios to e2e test! |
- fix expected copy – old code was using unusual apostrophe char - expect no prefix for JP corporate number
|
Note for reviewers: there are a bunch of tests that need updating/fixing, I'm in progress on this :) Feedback or suggestions for improving the tests is welcome! |
There was a problem hiding this comment.
Looks good and tested well.
Thank you for the screenshots. They help to identify what are the changes in the UI.
My notes:
- The
wp wcpay add_document_for_accountcommand from the testing instructions did not work for me. I had to add--dateparameter.
wp wcpay add_document_for_account {account_id} vat_invoice --mode=test --date="2024-07-24" --period_from="2023-01-01 00:00:00" --period_to="2024-07-31 23:59:59"
-
There is a linter issue that needs to be fixed.
-
I tested using US, UK, and JP store.
-
Do you plan to resolve some of the TODOs in this PR?
-
Acknowledging that GST is mentioned in some of the comments but it's not returned in
getVatTaxIDName()because GST, according to ChatGPT, for India, Australia, Canada, New Zealand. -
I tested that the hint returned by
getVatTaxIDValidationHint()for JP works, ie entering1234567890123passes the validation.
I didn't think too much about e2e test but if it's not straight forward, better to do in a separate PR. Not sure if setting certain country is easy in e2e test.
| @@ -33,6 +39,54 @@ const getVatPrefix = () => { | |||
| } | |||
| }; | |||
|
|
|||
| const getVatTaxIDName = () => { | |||
There was a problem hiding this comment.
The return value of getVatTaxIDName() is the return value of __() but I see it will be fed into another __(). I was concerned that the string will be double-translated, but I think it will not because it's considered as a parameter for the 2nd __() call.
There was a problem hiding this comment.
This is part of the reason why I would prefer to have separate dialogs for each region (as needed). Then the translator can translate as whole, and avoid concatenating translated strings (which is a localisation no-no).
There was a problem hiding this comment.
I think the way it's done here is okay. We're not doing {phrase1} {tax number} {phrase2} we're instead doing Set your %s (where %s is tax_number) which is exactly how that link recommends doing it.
While whole strings would be better, this approach is fine.
There was a problem hiding this comment.
Yea it's shippable as is, but the amount of branching and substitutions is the problem, _() on _() is a symptom of that.
While whole strings would be better, this approach is fine.
Exactly, whole strings (or UIs) would be better, my plan is next time we work on this we do that refactor.
|
Thanks @brucealdridge @shendy-a8c ! I'm working on the e2e tests now, I think I can get that sorted, and will also tidy up linter and todos/comments before merge. [Moved back to |
That's by design, a hint for anyone working on this in future to not assume "VAT". We don't yet support tax docs in any "GST" regions (as of right now). Merging 🚢 |
I have the ball, will follow up & add to What's New. |
Co-authored-by: Rua Haszard <rua@automattic.com> Co-authored-by: Shendy <73803630+shendy-a8c@users.noreply.github.com>
Fixes #8926
Changes proposed in this Pull Request
This PR changes the tax details modal so it works well for merchants in Japan, alongside existing VAT merchants in EU and UK.
Prior to this PR, the modal assumes
VATand various other details of the flow are tailored to the currently-supported tax regions:To make the modal work for Japan, I've generalised it a little, by parameterising all the data/copy/content that is region-specific:
VAT numbervsCorporate Number; in futureGST number).VATvsConsumption Tax; in futureGST).I've made some other changes so things are clear and accurate
Set your tax details(covers all tasks - id and address).receiptstotax documents, more consistent with rest of the UI and documentation.here's what's changed, in visual form
In the first screenshot, I've highlighted the non-essential stuff I've changed.
@rogermattic keen for your feedback on this, if you see anything amiss please comment!
† Note: The error message for a bad corporate number / Japan tax ID is currently incorrect. I've made server changes to allow us to customise these error messages, I'm proposing doing that in a follow up PR to keep this PR moving.
this is arguably a temporary fix
IMO this dialog is quite convoluted and complex, since each region is subtly different.
Long term, I think we should implement separate modals/components for each region, with a similar interface, and have one outer conditional for region. I didn't undertake that on this PR because I wanted a relatively quick and low-risk fix.
Why: If each region's UI is self-contained, it's easier to review and test, or customise the flow if needed. Also IMO it's not good practice to translate substrings such as
VAT number, it's better to translate whole sentences (or ideally whole modal/UI).Next time we add a new region, or do any work, let's consider that approach. Reviewers: keen for feedback on this aspect specifically.
Testing instructions
Note you'll need to test this with a store with account country that supports tax docs. Ideally, test with different accounts to see UI for different countries. Tip: if you set
wcpay_accounts[].deletedto a date in the past, you can onboard a new account. Or switch between accounts by setting wcpay_accounts[].deleted=NULL` on the account you want to use.Payments > Documents. Fake one up with this CLI command:wp wcpay add_document_for_account [account_id] vat_invoice --mode=test --period_from="2024-04-01 00:00:00" --period_to="2024-04-30 23:59:59"wp wcpay backfill_merchant_vat_invoicescompany_vat_submittedcompany_vat_numbercompany_namecompany_addressfromwcpay_account_metatable.Payments > Documents.Downloadfor a tax doc row.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge