CSV Exports: Simplify localization by setting export language to that in user profile settings #10003
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: -2.52 kB (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
nagpai
changed the title
nagpai
changed the title
nagpai
changed the title
There was a problem hiding this comment.
@nagpai I've conducted some testing of this and found that the server-generated CSV is not being translated to the expected language when using this branch:
- Using Pressable site with production transact-platform-server
- Change site language to Español
- Change user language to Nederlands
develop- Download client-side CSV – exported CSV used user profile language
- Download server-export CSV
- Modal shown
- Exported CSV only showed English or site language – expected to see user profile language as an option
⚠️ - Email also localised to site language when selected via modal or Payments → Settings.
- This branch
- Download client-side CSV – exported CSV used user profile language
- Download server-export CSV
- Modal not shown ✅
- Exported CSV shown in English
⚠️ expected site or user profile language - Email also in English
⚠️ expected site or user profile language - Note that the locale query params in the browser request to
/transactions/downloadarelocale=nl_NL&_locale=user - Language selection item in Payments → Settings is no longer shown ✅
I haven't explored the cause yet but will investigate why the locale is not being respected, suspecting a transact-server cause.
|
@Jinksi Thanks for the test and detailed description of the results 🙌🏼
This is expected, since it uses existing code that considers site language, and not user profile language.
The client seems to send the params correctly via the query. The expected locale is also seen correctly in the API request via the network tab in browser tools. I will also spend some time checking the server today. Top of my mind - I will check if the server takes the full locale - I have also noted some unusual behavior - paJDYF-g5h-p2#comment-27516 that I saw when I tested with a pressable hosted site. The translation worked under certain situations, and in some others it did not. |
Yes, my The changes in this PR should fix this by presenting server-generated CSV exports in the user profile language ✅ (clarifying for my own sake, since there are many language variables at play!) |
|
I've followed the execution stack locally all the way from My server I've also installed |
That is a possibility @Jinksi . The error you have noted may be related to a known issue - paJDYF-fWD-p2 that got introduced in WP 6.7 . Thanks for bringing the attention to the CLI command. I think we will need to use |
This was fixed on my local by upgrading WP Mail Logging to |
|
@nagpai I've tested this against my WPCOM sandbox, I see that Nederlands translations work for emails and CSVs on On this branch, however, Nederlands translations do NOT work for the sent email or CSV export file. I believe this is because the query param is 2 chars on (PS more detailed steps to get these emails working on sandbox are listed in https://github.com/Automattic/transact-platform-server/pull/7096) |
|
Mystery solved ( I hope ) - It looks like the server uses a shorter language code that is different from the WordPress Locale code. I found a past PR where a utility function was added to convert this - #8190 |
Interesting... separate scope from this PR, but I wonder if this is a bug? For example, it would convert the "Australian English" locale to simply "English", resulting in different spelling being applied to certain words (a low-impact example). |
| @@ -121,10 +121,6 @@ export const getDepositScheduleInterval = ( state ) => { | |||
| return getSettings( state ).deposit_schedule_interval || ''; | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Note that it looks like a bunch of code related to the now-redundant reporting_export_language can also be removed from https://github.com/Automattic/woocommerce-payments/blob/2af6f1aed5923631f49a72f1f98cad7a3c2256ad/includes/admin/class-wc-rest-payments-settings-controller.php.
There was a problem hiding this comment.
That is a nice catch! More 🪚 cuts!
I have removed it here edaaef9
There was a problem hiding this comment.
I've taken a look through the code changes and left some suggestions, mainly related to extra type annotations 🎁
👍 Consistent implementation across reporting views 🙌
👍 Lots of code removed, a win for maintenance 💥
Next, I'd like to test this with the transact-platform-server supported locales (and unsupported) to ensure no regressions.
woocommerce-payments/includes/class-wc-payments-utils.php
Lines 1288 to 1305 in 2af6f1a
| @@ -188,6 +185,9 @@ declare global { | |||
| adminUrl: string; | |||
| countries: Record< string, string >; | |||
| homeUrl: string; | |||
| locale: { | |||
| userLocale: string; | |||
| }; | |||
There was a problem hiding this comment.
💭 Since it's present in the wcSettings global browser variable, it can't hurt to let future devs know that it exists.
Another gift to future devs could be explanatory descriptions of what these are. 🎁
| }; | |
| locale: { | |
| /** | |
| * The locale of the current user profile. | |
| * | |
| * @example 'en_AU' // English (Australia) | |
| */ | |
| userLocale: string; | |
| /** | |
| * The locale of the current site, as set in WP → Settings → General. | |
| * | |
| * @example 'nl_NL' // Dutch (Netherlands) | |
| */ | |
| siteLocale: string; | |
| }; |
There was a problem hiding this comment.
That's a great suggestion! I have added it here 319c867
client/globals.d.ts
Outdated
| userLocale: { | ||
| code: string; | ||
| english_name: string; | ||
| native_name: string; |
There was a problem hiding this comment.
Another good opportunity to gift extra information to future devs 🎁
This is particularly helpful for code, because it is specific to the transact-platform-server, not WP-standard locale as one might expect.
| userLocale: { | |
| code: string; | |
| english_name: string; | |
| native_name: string; | |
| userLocale: { | |
| /** | |
| * The locale of the current user profile, represented as a locale code supported by transact-platform-server. | |
| * | |
| * @example 'sv' // Swedish | |
| * | |
| * @see WC_Payments_Utils::convert_to_server_locale | |
| */ | |
| code: string; | |
| /** | |
| * The English name of the locale. | |
| * | |
| * @example 'Swedish' | |
| */ | |
| english_name: string; | |
| /** | |
| * The native name of the locale. | |
| * | |
| * @example 'Svenska' | |
| */ | |
| native_name: string; |
There was a problem hiding this comment.
I have added the comments. This will be certainly helpful for future developers - 7b4f520
| Significance: minor | ||
| Type: update | ||
|
|
||
| Simplify localization of CSV exports to use global site language settings from WP Admin |
There was a problem hiding this comment.
💭 An idea for updating this changelog entry, to mention a bit more of the "why".
| Simplify localization of CSV exports to use global site language settings from WP Admin | |
| Simplify localization of CSV exports to use user language settings from WP Admin, allowing the CSV export to match the localization of the data presented in the UI. |
There was a problem hiding this comment.
That makes sense. I have updated it here ca2a2fa
Clarifying some context about these codes …
It sounds like we might be using country code as a shorthand for language, and we should probably use language codes (e.g. en_UK, nl_NL). |
Fixes #9971
Caution
Please do not merge this PR. It is still under discussion.
Changes proposed in this Pull Request
WP Admin > Users > Profile > LanguagePayments > Settings > Reporting( the reporting section will be removed since it has no other options ) Appears like this if you select Spanish as site language:Testing instructions
Check out develop branch and do these steps:
Make sure the site has more than 25 transactions or override code to trigger server export only
Change site language to non-english - Say Spanish or French ( from
WP Admin > Settings > Site language)Browse to
Payments > Settings. Here you should see a section that translates toReports. It should have an option to select the language of the CSV export, similar to the screenshot above. Do not change anything hereBrowse to
Payments > Transactionssection ( menu items will appear in the language you chose )Try doing a CSV export, it will show a modal similar to the screenshot above.
Now checkout this branch
You should no longer see a
Reportssection withinPayments > SettingsChange the site language back to English.
Change the language in
User > Profile > Languageto a non English language.Clicking export button for CSV, should no longer show a language selection modal
The exported file should show the correct language as per what was selected within
Users > Profile > LanguageIf you test this on a Pressable site, you should see the email sent in the correct language too.
Notes
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