-
Notifications
You must be signed in to change notification settings - Fork 68
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
Provide correct WPCOM language code as locale when requesting CSV exports #10197
base: develop
Are you sure you want to change the base?
Changes from all commits
c3685eb
b2ac3a3
ba727ee
b20b368
fc84c7f
f732adf
069aab4
a8c4a3d
8932cbc
198fec5
3e2c750
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: patch | ||
Type: fix | ||
|
||
Provide correct language code when requesting CSV exports in a region-specific language |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment to above: are these intentionally allow-lists or could we simplify by using generic processing for query args. |
||
|
||
export function getDisputesCSV( query ) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is deceptive!
Using the store settings seems cleaner to me 🚀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(i.e. took me a while to see what was changing) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OMG nice, you have removed |
||
|
||
const { | ||
date_before: dateBefore, | ||
|
@@ -257,7 +257,7 @@ export const DepositsList = (): JSX.Element => { | |
} >( { | ||
path: getDepositsCSV( { | ||
userEmail, | ||
userLocale, | ||
locale, | ||
dateAfter, | ||
dateBefore, | ||
dateBetween, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,28 +123,6 @@ declare global { | |
isNextDepositNoticeDismissed: boolean; | ||
isInstantDepositNoticeDismissed: boolean; | ||
isConnectionSuccessModalDismissed: boolean; | ||
userLocale: { | ||
/** | ||
* The locale of the current user profile, represented as a locale code supported by transact-platform-server. | ||
* | ||
* @example 'es' // Spanish | ||
* | ||
* @see WC_Payments_Utils::convert_locale_to_language_code | ||
*/ | ||
code: string; | ||
/** | ||
* The English name of the locale. | ||
* | ||
* @example 'Spanish' | ||
*/ | ||
english_name: string; | ||
/** | ||
* The native name of the locale. | ||
* | ||
* @example 'Español' | ||
*/ | ||
native_name: string; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🫡 oh captain, my captain remove that redundant code 🧹 |
||
trackingInfo?: { | ||
hosting_provider: string; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,11 +147,13 @@ public function close_dispute( $request ) { | |
* @param WP_REST_Request $request Full data about the request. | ||
*/ | ||
public function get_disputes_export( $request ) { | ||
$user_email = $request->get_param( 'user_email' ); | ||
$locale = $request->get_param( 'locale' ); | ||
$filters = $this->get_disputes_filters( $request ); | ||
$user_email = $request->get_param( 'user_email' ); | ||
$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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 $this->forward_request( 'get_disputes_export', [ $filters, $user_email, $locale ] ); | ||
return $this->forward_request( 'get_disputes_export', [ $filters, $user_email, $wpcom_locale ] ); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1251,74 +1251,31 @@ public static function enqueue_style( $handle, $path = '', $deps = [], $version | |
} | ||
|
||
/** | ||
* Returns language data: english name and native name | ||
* Converts a WP locale to a wpcom-compatible language code. | ||
* | ||
* @param string $language Language code. | ||
* @see Automattic\Jetpack\Jetpack_Mu_Wpcom\Common::get_iso_639_locale() for similar logic. | ||
* @see https://translate.wordpress.com/projects/wpcom/ for the current state of wpcom translations. | ||
* | ||
* @return array | ||
*/ | ||
public static function get_language_data( $language ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. And to confirm, it was only used to generate/render our JS settings data, which was not needed? |
||
require_once ABSPATH . 'wp-admin/includes/translation-install.php'; | ||
|
||
$translations = wp_get_available_translations(); | ||
|
||
if ( isset( $translations[ $language ] ) ) { | ||
return [ | ||
'code' => self::convert_to_server_locale( $language ), | ||
'english_name' => $translations[ $language ]['english_name'] ?? $language, | ||
'native_name' => $translations[ $language ]['native_name'] ?? $language, | ||
]; | ||
} | ||
|
||
return [ | ||
'code' => 'en_US', | ||
'english_name' => 'English (United States)', | ||
'native_name' => 'English (United States)', | ||
]; | ||
} | ||
|
||
/** | ||
* Converts a locale to the server supported languages. | ||
* | ||
* @param string $locale The locale to convert. | ||
* | ||
* @return string Closest locale supported ('en' if NONE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💣 |
||
* @param string $wp_locale a WordPress locale code to be converted e.g. "en_US", "pt_BR". | ||
* @return string language code compatible with wpcom e.g. "en", "pt-br". | ||
*/ | ||
public static function convert_to_server_locale( string $locale ): string { | ||
$supported = [ | ||
'ar', // Arabic. | ||
'de', // German (Germany). | ||
'es', // Spanish (Spain). | ||
'fr', // French (France). | ||
'he', // Hebrew (Israel). | ||
'id', // Indonesian (Indonesia). | ||
'it', // Italian (Italy). | ||
'ja', // Japanese. | ||
'ko', // Korean. | ||
'nl', // Dutch (Netherlands). | ||
'pt-br', // Portuguese (Brazil). | ||
'ru', // Russian (Russia). | ||
'sv', // Swedish (Sweden). | ||
'tr', // Turkish (Turkey). | ||
'zh-cn', // Simplified, Singapore). | ||
'zh-tw', // Chinese Traditional (Taiwan). | ||
]; | ||
|
||
// Replace '-' with '_' (used in WordPress). | ||
$locale = str_replace( '_', '-', $locale ); | ||
|
||
if ( in_array( $locale, $supported, true ) ) { | ||
return $locale; | ||
} | ||
|
||
// Remove the country code and try with that. | ||
$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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nitpick on naming, could be clearer IMO.
Also suggest refreshing the doc comment (it's pretty clear as is), especially if you tweak the name. |
||
$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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reducing the manual mapping of locales only to those that are region-specific. This approach is borrowed from Jetpack's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Recommend adding a comment "maintenance hint" about this, i.e. guide future team when WPCOM adds a new language (or removes one). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe in the docblock. |
||
$is_region_specific_wpcom_language_code = in_array( $wp_locale_lowercase, $region_specific_wpcom_language_codes, true ); | ||
|
||
$language_code = $is_region_specific_wpcom_language_code ? | ||
// If it is a region-specific language code, we replace the underscore with a dash, e.g. 'pt_br' => 'pt-br'. | ||
str_replace( '_', '-', $wp_locale_lowercase ) : | ||
// Otherwise, we remove the country code and return only the language code, e.g. 'nl_NL' => 'nl'. | ||
preg_replace( '/([-_].*)$/i', '', $wp_locale_lowercase ); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
// Return 'en_US' to match the default site language. | ||
return 'en_US'; | ||
return $language_code; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating this to the accurately match what is available in the |
||
weekdaysShort: [ 'Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat' ], | ||
}, | ||
|
@@ -102,10 +103,6 @@ global.wpApiSettings = { | |
}; | ||
|
||
global.wcpaySettings = { | ||
locale: { | ||
code: 'es_ES', | ||
native_name: 'Spanish', | ||
}, | ||
accountLoans: { | ||
loans: [ 'flxln_123456|active' ], | ||
}, | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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:
match
andlocale
, allow them through unchanged.Low priority questions…