Skip to content
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

Add support for Japan to the tax details modal #8974

Merged
merged 27 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
93cad6f
generalise title of VAT/GST modal
Jun 18, 2024
7ede193
WIP/experiment: refactor VAT modal with country specific hint & tax name
Jun 18, 2024
232a92b
tweak title to align more closely with existing user docs
Jun 19, 2024
66f1501
factor out "you gotta register a VAT number" hint into a country-spec…
Jun 19, 2024
51880a5
comments / ideas for requirement hint
Jun 19, 2024
c12c5d8
parameterise most mentions of "VAT number", remove enforced prefix fo…
Jun 19, 2024
addbd1d
remove getVatTaxName, not used
Jun 19, 2024
1175bfe
comment to explain that JP has no prefix
Jun 19, 2024
9919e3a
Merge branch 'develop' into fix/8926-localise-vat-details-modal
haszari Jul 12, 2024
e871333
Merge branch 'develop' into fix/8926-localise-vat-details-modal
haszari Jul 21, 2024
7a313c4
document VatNumberTask component, remove TODO comment
Jul 21, 2024
7e15b02
add comment suggestions for alternative long-term approach: component…
Jul 22, 2024
276137b
changelog
Jul 22, 2024
60797d6
Merge branch 'develop' into fix/8926-localise-vat-details-modal
shendy-a8c Jul 24, 2024
3d11f18
add relevant japan tax keywords to changelog
Jul 24, 2024
a1ee604
Merge branch 'develop' into fix/8926-localise-vat-details-modal
haszari Jul 24, 2024
63f1ff9
update tax modal e2e tests:
Jul 24, 2024
355a0b9
update various tests for renamed "Tax details" form
Jul 24, 2024
98cc50e
reduce specificity of test, `VAT number` is not hard-coded in UI anymore
Jul 24, 2024
f9b72ee
fix references to (changed) tax modal title in tests
Jul 25, 2024
44822c6
remove todo comments, clarify, link to follow up issue for legal/regu…
Jul 25, 2024
4632375
Merge branch 'develop' into fix/8926-localise-vat-details-modal
haszari Jul 25, 2024
acea9d9
fix typo – VAT Number is capitalised in UI
Jul 25, 2024
0ae6d2e
parameterise e2e test expectation for tax id name
Jul 25, 2024
fa3efa6
update snapshot for tax details modal to match new code
Jul 25, 2024
f7f94a1
remove obsolete e2e snapshot
Jul 25, 2024
bfb3bd1
fix test for no prefix for JP corporate number 😅
Jul 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-8926-localise-vat-details-modal
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: add

Support adding tax details (corporate number) for Japan merchants (so can generate tax documents for consumption tax, aka VAT).
16 changes: 11 additions & 5 deletions client/documents/list/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -238,20 +238,24 @@ describe( 'Document download button', () => {
expect( window.open ).not.toHaveBeenCalled();
} );

it( 'should open the VAT form modal', () => {
it( 'should open the tax details modal', () => {
// Make sure the modal is not opened before clicking on the button.
expect(
screen.queryByRole( 'dialog', { name: 'VAT details' } )
screen.queryByRole( 'dialog', {
name: 'Set your tax details',
} )
).toBeNull();

user.click( downloadButton );

expect(
screen.getByRole( 'dialog', { name: 'VAT details' } )
screen.getByRole( 'dialog', {
name: 'Set your tax details',
} )
).toBeVisible();
} );

describe( 'after the VAT details are submitted', () => {
describe( 'after the tax details are submitted', () => {
beforeEach( () => {
user.click( downloadButton );

Expand All @@ -260,7 +264,9 @@ describe( 'Document download button', () => {

it( 'should close the modal', () => {
expect(
screen.queryByRole( 'dialog', { name: 'VAT details' } )
screen.queryByRole( 'dialog', {
name: 'Set your tax details',
} )
).toBeNull();
} );

Expand Down
2 changes: 1 addition & 1 deletion client/vat/form-modal/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const VatFormModal = ( {
} ): JSX.Element | null => {
return isModalOpen ? (
<Modal
title={ __( 'VAT details', 'woocommerce-payments' ) }
title={ __( 'Set your tax details', 'woocommerce-payments' ) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note – I've changed the dialog title for two reasons:

  1. Remove VAT, not all regions call it VAT.
  2. 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.

onRequestClose={ () => setModalOpen( false ) }
>
<VatForm onCompleted={ onCompleted } />
Expand Down
94 changes: 77 additions & 17 deletions client/vat/form/tasks/vat-number-task.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
Notice,
TextControl,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import React, { useContext, useState } from 'react';
import CollapsibleBody from 'wcpay/additional-methods-setup/wizard/collapsible-body';
import WizardTaskItem from 'wcpay/additional-methods-setup/wizard/task-item';
Expand All @@ -22,8 +22,14 @@ import apiFetch from '@wordpress/api-fetch';
import { VatError, VatFormOnCompleted, VatValidationResult } from '../../types';
import '../style.scss';

/**
* These country-specific getters may belong on server.
*/
const getVatPrefix = () => {
switch ( wcpaySettings.accountStatus.country ) {
case 'JP':
// Corporate numbers are not prefixed.
return '';
case 'GR':
return 'EL ';
case 'CH':
Expand All @@ -33,6 +39,54 @@ const getVatPrefix = () => {
}
};

const getVatTaxIDName = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

switch ( wcpaySettings.accountStatus.country ) {
case 'JP':
return __( 'Corporate Number', 'woocommerce-payments' );
default:
return __( 'VAT Number', 'woocommerce-payments' );
}
};

const getVatTaxIDRequirementHint = () => {
switch ( wcpaySettings.accountStatus.country ) {
case 'JP':
// TODO do we need a note/legal requirement hint for Japan?
return __( '', 'woocommerce-payments' );
default:
// TODO this message is a little alarming and doesn't provide guidance for confused merchants.
// What are the thresholds?
// How do I register for a VAT number?
// Idea: add a learn more link to our docs page, and link to relevant sources there.
// https://woocommerce.com/document/woopayments/taxes/documents/
// Alternative - maybe this can be removed.
return __(
"If your sales exceed the VAT threshold for your country, you're required to register for a VAT Number.",
'woocommerce-payments'
);
}
};

const getVatTaxIDValidationHint = () => {
switch ( wcpaySettings.accountStatus.country ) {
case 'JP':
return __(
'A 13 digit number, for example 1234567890123.',
'woocommerce-payments'
);
default:
return __(
'8 to 12 digits with your country code prefix, for example DE 123456789.',
'woocommerce-payments'
);
}
};

/**
* A two-step "task" for obtaining merchant's tax details.
*
* @param {VatFormOnCompleted} props.onCompleted - Callback to provide tax details on submit.
*/
export const VatNumberTask = ( {
onCompleted,
}: {
Expand Down Expand Up @@ -95,17 +149,23 @@ export const VatNumberTask = ( {
};

return (
// Note: the VAT ID name is parameterised in strings below.
// Long term, it might be better to implement a dedicated WizardTaskItem component for each tax region.
<WizardTaskItem
index={ 1 }
title={ __(
'Update your VAT information',
'woocommerce-payments'
title={ sprintf(
__(
/* translators: %$1$s: tax ID name, e.g. VAT Number, GST Number, Corporate Number */
'Set your %1$s',
'woocommerce-payments'
),
getVatTaxIDName()
) }
className={ null }
>
<p className="wcpay-wizard-task__description-element">
{ __(
'VAT information saved on this page will be applied to all of your account’s receipts.',
"The information you provide here will be used for all of your account's tax documents.",
'woocommerce-payments'
) }
</p>
Expand All @@ -114,22 +174,22 @@ export const VatNumberTask = ( {
<CheckboxControl
checked={ isVatRegistered }
onChange={ setVatRegistered }
label={ __(
'I’m registered for a VAT number',
'woocommerce-payments'
) }
help={ __(
'If your sales exceed the VAT threshold for your country, you’re required to register for a VAT number.',
'woocommerce-payments'
label={ sprintf(
__(
/* translators: %$1$s: tax ID name, e.g. VAT Number, GST Number, Corporate Number */
"I'm registered for a %1$s",
'woocommerce-payments'
),
getVatTaxIDName()
) }
help={ getVatTaxIDRequirementHint() }
/>
{ isVatRegistered && (
// Note: this TextControl is heavily parameterised to support different regions (VAT vs GST vs Corporate Number).
// Long term, if we implement a dedicated WizardTaskItem component for each tax region, then this component will be simpler.
<TextControl
label={ __( 'VAT Number', 'woocommerce-payments' ) }
help={ __(
'This is 8 to 12 digits with your country code prefix, for example DE 123456789.',
'woocommerce-payments'
) }
label={ getVatTaxIDName() }
help={ getVatTaxIDValidationHint() }
value={ vatNumber }
onChange={ setVatNumber }
/>
Expand Down
7 changes: 3 additions & 4 deletions client/vat/form/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const countries = [
[ 'DE', 'DE' ],
[ 'GR', 'EL' ],
[ 'CH', 'CHE' ],
[ 'JP', '' ],
];

describe( 'VAT form', () => {
Expand All @@ -66,7 +67,7 @@ describe( 'VAT form', () => {
render( <VatForm onCompleted={ mockOnCompleted } /> );

user.click(
screen.getByLabelText( 'I’m registered for a VAT number' )
screen.getByLabelText( "I'm registered for a VAT number" )
);

expect(
Expand Down Expand Up @@ -204,9 +205,7 @@ describe( 'VAT form', () => {

describe( 'when registered for VAT', () => {
beforeEach( () => {
user.click(
screen.getByLabelText( 'I’m registered for a VAT number' )
);
user.click( screen.getByLabelText( "I'm registered for a" ) );
} );

it( 'should disable the Continue button', () => {
Expand Down
Loading