Provide correct WPCOM language code as locale when requesting CSV exports #10197
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: -5 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
Replace with `wcSettings.locale.userLocale`
| @@ -84,7 +84,7 @@ const formatQueryFilters = ( query ) => ( { | |||
| ], | |||
| status_is: query.statusIs, | |||
| status_is_not: query.statusIsNot, | |||
| locale: query.userLocale, | |||
| locale: query.locale, | |||
There was a problem hiding this comment.
Leaving this as a generic query param, since this could be site, user or manually-selected locale in the future. These endpoints do not mandate this to be the user locale.
There was a problem hiding this comment.
(Side note not relevant to this PR)
Is this formatQueryFilters operating as an "allow list"?
It looks like the outcome of this method is to:
- Convert "pascalCase" keys to underscore snake_case vibe (server uses snake_case convention).
- Format (render to consistent text) some query data e.g. dates.
- And in the case of
matchandlocale, allow them through unchanged.
Low priority questions…
- Is the "allow list" functionality intended, would it be simpler to trust the client (calling code) and only format what needs formatting, allow any keys through?
- Could we use a generic snake_case conversion instead of doing it manually, key by key?
| * | ||
| * @return array | ||
| */ | ||
| public static function get_language_data( $language ) { |
There was a problem hiding this comment.
get_language_data is no longer used.
There was a problem hiding this comment.
Nice. And to confirm, it was only used to generate/render our JS settings data, which was not needed?
| return $base_locale; | ||
| public static function convert_to_server_locale( string $wp_locale ): string { | ||
| $wp_locale_lowercase = strtolower( $wp_locale ); | ||
| $region_specific_wpcom_language_codes = [ 'pt_br', 'pt-br', 'zh_tw', 'zh-tw', 'zh_cn', 'zh-cn' ]; |
There was a problem hiding this comment.
Reducing the manual mapping of locales only to those that are region-specific.
This approach is borrowed from Jetpack's get_iso_639_locale()
There was a problem hiding this comment.
This approach is borrowed from Jetpack's get_iso_639_locale()
Recommend adding a comment "maintenance hint" about this, i.e. guide future team when WPCOM adds a new language (or removes one).
| @@ -75,7 +75,8 @@ global.wcSettings = { | |||
| failed: 'Failed', | |||
| paid: 'Paid', | |||
| }, | |||
| l10n: { | |||
| locale: { | |||
| siteLocale: 'en_US', | |||
| userLocale: 'en_US', | |||
There was a problem hiding this comment.
Updating this to the accurately match what is available in the wcSettings global JS variable.
|
In testing, I've noticed that using "Portugese" other than "Portugese (Brazil)"
Update in testing, I notice that other emails are partially translated when using |
There was a problem hiding this comment.
I have done a code review only. I approve!
- Great to see some redundant code / data (page bloat!) removed.
- The mapping is clearly defined and easier to maintain when supported languages change.
I left a bunch of comments about style and architecture considerations, I'll leave it to y'all to decide if any are worth following up.
Also I suggest tweaking the title to celebrate the win - translated service-generated emails now use correct language? The description has good info on the "how" :)
| @@ -84,7 +84,7 @@ const formatQueryFilters = ( query ) => ( { | |||
| ], | |||
| status_is: query.statusIs, | |||
| status_is_not: query.statusIsNot, | |||
| locale: query.userLocale, | |||
| locale: query.locale, | |||
There was a problem hiding this comment.
(Side note not relevant to this PR)
Is this formatQueryFilters operating as an "allow list"?
It looks like the outcome of this method is to:
- Convert "pascalCase" keys to underscore snake_case vibe (server uses snake_case convention).
- Format (render to consistent text) some query data e.g. dates.
- And in the case of
matchandlocale, allow them through unchanged.
Low priority questions…
- Is the "allow list" functionality intended, would it be simpler to trust the client (calling code) and only format what needs formatting, allow any keys through?
- Could we use a generic snake_case conversion instead of doing it manually, key by key?
| @@ -38,7 +38,7 @@ const formatQueryFilters = ( query ) => ( { | |||
| : query.search, | |||
| status_is: query.statusIs, | |||
| status_is_not: query.statusIsNot, | |||
| locale: query.userLocale, | |||
| locale: query.locale, | |||
| } ); | |||
There was a problem hiding this comment.
Similar comment to above: are these intentionally allow-lists or could we simplify by using generic processing for query args.
| @@ -214,7 +214,7 @@ export const DepositsList = (): JSX.Element => { | |||
|
|
|||
| const endpointExport = async () => { | |||
| const userEmail = wcpaySettings.currentUserEmail; | |||
| const userLocale = wcpaySettings.userLocale.code; | |||
| const locale = wcSettings.locale.userLocale; | |||
There was a problem hiding this comment.
This is deceptive!
- Is
wcSettings.locale.userLocalea ~2-letter code? wcpaySettings.userLocale.codeis a struct with other stuff in it (code: english_name: ; native_name: ;) - do we need it in our settings?
Using the store settings seems cleaner to me 🚀
There was a problem hiding this comment.
This is deceptive!
(i.e. took me a while to see what was changing)
There was a problem hiding this comment.
OMG nice, you have removed wcpaySettings.userLocale 🫡
| * @example 'Español' | ||
| */ | ||
| native_name: string; | ||
| }; |
There was a problem hiding this comment.
🫡 oh captain, my captain
remove that redundant code 🧹
| $wpcom_locale = WC_Payments_Utils::convert_to_server_locale( | ||
| $request->get_param( 'locale' ) | ||
| ); | ||
| $filters = $this->get_disputes_filters( $request ); |
There was a problem hiding this comment.
Nice and simple to do this inline. Long term I wonder if we might want middleware-style generic converters for various standard args.
That is probably overengineering, but at the same time, I worry that our request wrapper handling in client is too manual, since it's almost a 1:1 mapping to the service API, tweaking a few things here and there. One way to document that "magic" is to implement as middleware pattern. For example, WooPayments JavaScript front end calls store PHP/REST API, store PHP generically adds authentication for WooPayments Service (WPCOM) API.
| return $base_locale; | ||
| public static function convert_to_server_locale( string $wp_locale ): string { | ||
| $wp_locale_lowercase = strtolower( $wp_locale ); | ||
| $region_specific_wpcom_language_codes = [ 'pt_br', 'pt-br', 'zh_tw', 'zh-tw', 'zh_cn', 'zh-cn' ]; |
There was a problem hiding this comment.
This approach is borrowed from Jetpack's get_iso_639_locale()
Recommend adding a comment "maintenance hint" about this, i.e. guide future team when WPCOM adds a new language (or removes one).
| $base_locale = substr( $locale, 0, 2 ); | ||
| if ( in_array( $base_locale, $supported, true ) ) { | ||
| return $base_locale; | ||
| public static function convert_to_server_locale( string $wp_locale ): string { |
There was a problem hiding this comment.
Minor nitpick on naming, could be clearer IMO.
convert– ismapclearer? I.e. we are mapping from one system to another.server– I will always rail against the wordserver, it's too generic. In this case we could usewpcom(the platform our service runs on), orwoopayments_service.- Maybe
map_to_wpcom_locale_code? I think code helps too, clarify that the input and output are the ~2 letter code things;localecould be represented a few ways.
Also suggest refreshing the doc comment (it's pretty clear as is), especially if you tweak the name.
| return $base_locale; | ||
| public static function convert_to_server_locale( string $wp_locale ): string { | ||
| $wp_locale_lowercase = strtolower( $wp_locale ); | ||
| $region_specific_wpcom_language_codes = [ 'pt_br', 'pt-br', 'zh_tw', 'zh-tw', 'zh_cn', 'zh-cn' ]; |
|
|
||
| if ( empty( $language_code ) ) { | ||
| // If the language code is empty, we return 'en' as a fallback. | ||
| return 'en'; |
There was a problem hiding this comment.
There's a mix of early-return and "return $language_code" style here. I recommend rewriting as early-return while here, then each case will be a single if, no nesting.
Hmm .. if this is verbatim from Jetpack code … maybe should leave as is? If so make that clear in the comments.
| 'Supported locale (Chinese (Simplified))' => [ 'zh_CN', 'zh-cn' ], | ||
| 'Supported locale (Chinese (Traditional))' => [ 'zh_TW', 'zh-tw' ], | ||
| 'Empty locale fallback to en' => [ '', 'en' ], | ||
| ]; |
There was a problem hiding this comment.
Is it worth testing a range of unsupported locales? The empty case is likely less important than uncommon language.
Also I would like to see a test for inexact match, e.g. en_AU or pt_PT. Maybe useful to group & comment the various test (map) scenarios in the provider too. Another way to do that is to have separate tests for each of the mapping scenarios, then the test name + provider name are "self documenting".
Here's what I see when I change the language to German .
|
|
Thanks for testing @nagpai
In this new implementation, the locale passed from browser -> PHP REST API controller should be the WP locale ('pt_BR'). This will map to 'pt-br' when passing from controller → WPCOM. So the headers are correct for this implementation. The email should translate for 'pt-br', so I'll also conduct further testing to see if I can see the correctly translated emails for various languages. |
Jinksi
added
status: blocked
@nagpai This highlights a problem with our approach to email transactions. I've been able to reproduce this problem where emails are in one language, CSV in another. Config:
Result:
Therefore:
|
I've opened issue https://github.com/Automattic/transact-platform-server/issues/7161 for this. |
Fixes #10149
Warning
Blocked due to issues with email translations not working on transact-platform-server. See https://github.com/Automattic/transact-platform-server/issues/7161 for this.
Changes proposed in this Pull Request
Transact-platform-server (WPCOM) uses different language codes for translations than WordPress. Rather than
language_country(en_US), wpcom language codes are mostly justlanguage(en). Some wpcom language codes are represented aslanguage-region, e.g.pt-brfor Portugese (Brazil) (see translate.wordpress.com).When requesting CSV exports, region-specific language codes were not working correctly, as reported in #10149.
This PR fixes this by:
convert_to_server_locale:wcSettings, I've relied on this to replacewcpaySettings.userLocale.Testing instructions
Important
You must use production or sandbox transact-platform-server for email/CSV translations to work correctly.
Alternatively, if you've figured out how to get these email/CSV translations working on your local transact-platform-server, please let me know how!
WP Admin -> Users -> Profilept_BRPortugese (Brazil)zh_CNChinese (Simplified)zh_TWChinese (Traditional)fron wpcom.fr_FR(translate.wordpress.com link)fr_CA(translate.wordpress.com link)fr_BE(translate.wordpress.com link)pl_PLWPCOM-compatible languages
TODO
localeto match WPCOM langCode in PHP REST API controller insteadnpm 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
Additional information
As discussed in the #10149 comments, I initially considered using the
WP locale→WPCOM language codemapping available in @automattic/languages. This would have allowed us to rely on this package as the source of truth for wpcom "langSlugs". A downside of this approach is that this package is relatively heavy, adding +5kB to various WooPayments JS bundle sizes.Instead, I've decided to reduce the complexity of this solution and move the language code conversion logic to PHP.
I have not found a PHP package/resource with the same list of WP Locale → WPCOM Language. However, in reducing the hardcoded list to only the three that are region-specific, the maintenance burden is reduced. This approach is common across A8C (GH search), of which I found Jetpack's to be the simplest, which I've adapted and used in #10197.