wp-cli make-json and Loco Translate support for admin dashboard. #36739

kenarsuleyman · 2023-02-02T14:38:22Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes #35500 .

This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Set WordPress locale to a language that doesn't have official translations for WooCommerce (Ex. Azerbaijan).
Create translations for that language in Loco Translate. There are 3 possible locations for PO file, you can choose any, It's irrelevant.
*languages/plugins/woocommerce-az.po
*languages/loco/plugins/woocommerce-az.po
*plugins/woocommerce/i18n/languages/woocommerce-az.po
Translate couple random words and save. This will compile MO file and generate JSON files in the chosen directory with following format "woocommerce-{locale}-{MD5}.json". AKA "chunks".
De-Activate and Activate WooCommerce to trigger the associated action.
In the chosen directory, chunk files should be combined into single woocommerce-{locale}-wc-admin-app.json file.

Short summary of bug

Due to file format difference between the official and generated chunk files, WooCommerce was unable to generate combined JSON file.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

Konamiman · 2023-02-03T10:29:32Z

Hi @kenarsuleyman, thanks for your contribution. Could you please make the following adjustments to your pull request:

Fix the formatting issues outlined here: https://github.com/woocommerce/woocommerce/actions/runs/4075563397/jobs/7038439499, you can do so by using phpcbf and phpcs (available in vendor/bin).
Add a changelog file. You can do so by running pnpm --filter=plugins/woocommerce run changelog add.
Add more detail to the testing instructions: how are files generated and compiled with Loco Translate, and whre's the woocommerce-{locale}-wc-admin-app.json file supposed to be generated?

into fix/35500

kenarsuleyman · 2023-02-04T10:47:59Z

@Konamiman Thanks for the clear instructions. For some reason, both phpcbf and phpcs got stuck at %98 but I manually tried to fix formatting based on the error log you showed, also added a changelog and more detailed instructions.

adrianduffell · 2023-02-08T04:02:30Z

plugins/woocommerce/src/Internal/Admin/Translations.php

-			if ( ! isset( $chunk_data['comment']['reference'] ) ) {
+			// Support for wp-cli --make-json and Loco Translate file format.
+
+			if ( ! ( isset( $chunk_data['comment']['reference'] ) || isset( $chunk_data['source'] ) ) ) {


I think it would be better extracting each parser into separate functions rather than lumping them into one. For me, the resulting nested if branch conditions add too much complexity to this function.

An alternative might be to have get_translation_chunk_data, perform heuristics on the file to detect the format, and then call the appropriate parser function for that format.

Hi @kenarsuleyman, Would you be able to make a change as Adrian's suggestion? 🙏

Hi, @chihsuan since this is only a workaround fix and will get reverted sometime in near future, I might refactor the entire function based on Adrian's suggestion. Right now I don't know how long this fix needs to stay in the codebase. If current version is not suitable for standarts to merge it, I can do it this week.

If current version is not suitable for standarts to merge it, I can do it this week.

Hi @kenarsuleyman Yeah, I think that would be perfect, thanks!

Hi @adrianduffell, I wrote separate functions to combine translation chunks after your suggestion. I agree that this made the code more readable only problem is I'm not fully happy how those separate functions ended up with. There are "duplicate looking" lines, they are not exact duplicates, just the same logic for different file formats. Unfortunately, I couldn't think of a more optimized approach.

Hi @kenarsuleyman Just saw you push a new commit, we will review it soon. 👍 Thank you!

Sorry for late notice, @adrianduffell is unavailable.

adrianduffell · 2023-02-08T04:20:13Z

Hi @kenarsuleyman, thanks for this PR and your contribution. I think it's valuable to fix the bug.

I'd like to better understand why the formats are different between the official and Loco Translate generated files. Is it an error / deviation in the way one of them is generating the files?

I think ideally, we should aim to have the tooling converge to provide the data in the same format, rather than adding complexity at WooCommerce's end in supporting multiple formats.

kenarsuleyman · 2023-02-08T13:36:29Z

Hi @kenarsuleyman, thanks for this PR and your contribution. I think it's valuable to fix the bug.

I'd like to better understand why the formats are different between the official and Loco Translate generated files. Is it an error / deviation in the way one of them is generating the files?

I think ideally, we should aim to have the tooling converge to provide the data in the same format, rather than adding complexity at WooCommerce's end in supporting multiple formats.

About the difference, I also agree to focus on why there is a difference between files in the first place. I checked the changelog for both GlotPress(based on official files, It's generated by GlotPress) and Loco, but couldn't see any reference to this format change.

Since wp-cli and Loco has same formats and GlotPress has a different format, the problem is most likely caused by GlotPress. Do you think is it better to write a workaround like this and revert it after the underlying problem gets fixed, or try to find a way to fix the underlying issue( which I don't exactly know )?

adrianduffell · 2023-02-13T02:09:45Z

Since wp-cli and Loco has same formats and GlotPress has a different format, the problem is most likely caused by GlotPress. Do you think is it better to write a workaround like this and revert it after the underlying problem gets fixed, or try to find a way to fix the underlying issue( which I don't exactly know )?

Hi @kenarsuleyman, thanks for the explanation. I think it would be good to merge this as a temporary workaround, while aiming to fix the underlying issue in the meantime. We'll want to merge it by Monday 20 Feb to land it safely in the next release.

I'll take a further look this week.

codecov · 2023-02-21T09:46:19Z

Codecov Report

Merging #36739 (f3f4c90) into trunk (c8074a7) will decrease coverage by 0.0%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             trunk   #36739     +/-   ##
==========================================
- Coverage     46.7%    46.7%   -0.0%     
  Complexity   17178    17178             
==========================================
  Files          429      429             
  Lines        64779    64779             
==========================================
- Hits         30242    30240      -2     
- Misses       34537    34539      +2

Impacted Files	Coverage Δ
plugins/woocommerce/includes/class-wc-tax.php	`78.6% <0.0%> (-0.5%)`	⬇️

Added seperate parsers for chunk files

chihsuan

Thanks for your contribution! @kenarsuleyman LGTM and tested well.

@moon0326 Could you also look at this when you get a chance? 🙏

Konamiman · 2023-03-17T08:37:37Z

LGTM so I approved, but I'll wait for @moon0326 to take a look too as requested by @chihsuan before merging. Please ping me once that happens.

adrianduffell

LGTM and tested well! 🚀

adrianduffell · 2023-03-20T22:41:25Z

Hey @Konamiman, I've reviewed this instead of @moon0326. I'll go ahead and merge now to meet the code freeze deadline.

adrianduffell · 2023-03-20T22:43:15Z

Thanks for the PR @kenarsuleyman !

kenarsuleyman · 2023-03-20T22:54:24Z

Thanks for the PR @kenarsuleyman !

It was pleasure for me 🙌

Update Translations.php

f69fabf

github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution labels Feb 2, 2023

woocommercebot requested review from a team, Konamiman, ilyasfoo and moon0326 and removed request for a team February 2, 2023 14:38

kenarsuleyman added 3 commits February 3, 2023 18:02

fix formatting issues

a8ee8ba

Merge branch 'woocommerce:trunk' into fix/35500

8f7ddfa

Merge branch 'fix/35500' of https://github.com/kenarsuleyman/woocommerce

99a714c

into fix/35500

kenarsuleyman marked this pull request as draft February 3, 2023 15:16

kenarsuleyman added 3 commits February 3, 2023 22:54

Merge branch 'woocommerce:trunk' into fix/35500

dd7321e

added changelog

366b1c4

Merge branch 'fix/35500' of https://github.com/kenarsuleyman/woocommerce

a17248f

into fix/35500

kenarsuleyman marked this pull request as ready for review February 4, 2023 10:48

Merge branch 'woocommerce:trunk' into fix/35500

3e7386b

adrianduffell reviewed Feb 8, 2023

View reviewed changes

Merge branch 'trunk' into fix/35500

01a65fb

kenarsuleyman added 2 commits February 21, 2023 19:50

Update Translations.php

2a7dd82

Added seperate parsers for chunk files

Merge branch 'trunk' into fix/35500

a64a219

kenarsuleyman added 3 commits February 21, 2023 22:36

Merge branch 'trunk' into fix/35500

aa54263

Merge branch 'trunk' into fix/35500

8bea91b

Merge branch 'trunk' into fix/35500

f3f4c90

chihsuan approved these changes Mar 3, 2023

View reviewed changes

Konamiman approved these changes Mar 17, 2023

View reviewed changes

adrianduffell approved these changes Mar 20, 2023

View reviewed changes

adrianduffell merged commit 32fd0a4 into woocommerce:trunk Mar 20, 2023

github-actions bot added this to the 7.6.0 milestone Mar 20, 2023

kenarsuleyman deleted the fix/35500 branch April 14, 2023 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wp-cli make-json and Loco Translate support for admin dashboard. #36739

wp-cli make-json and Loco Translate support for admin dashboard. #36739

kenarsuleyman commented Feb 2, 2023 •

edited

Konamiman commented Feb 3, 2023

kenarsuleyman commented Feb 4, 2023

adrianduffell Feb 8, 2023

chihsuan Feb 20, 2023

kenarsuleyman Feb 21, 2023 •

edited

chihsuan Feb 21, 2023

kenarsuleyman Feb 21, 2023

chihsuan Feb 24, 2023 •

edited

adrianduffell commented Feb 8, 2023 •

edited

kenarsuleyman commented Feb 8, 2023

adrianduffell commented Feb 13, 2023

codecov bot commented Feb 21, 2023 •

edited

chihsuan left a comment

Konamiman commented Mar 17, 2023 •

edited

adrianduffell left a comment

adrianduffell commented Mar 20, 2023

adrianduffell commented Mar 20, 2023

kenarsuleyman commented Mar 20, 2023

wp-cli make-json and Loco Translate support for admin dashboard. #36739

wp-cli make-json and Loco Translate support for admin dashboard. #36739

Conversation

kenarsuleyman commented Feb 2, 2023 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Short summary of bug

Other information:

FOR PR REVIEWER ONLY:

Konamiman commented Feb 3, 2023

kenarsuleyman commented Feb 4, 2023

adrianduffell Feb 8, 2023

Choose a reason for hiding this comment

chihsuan Feb 20, 2023

Choose a reason for hiding this comment

kenarsuleyman Feb 21, 2023 • edited

Choose a reason for hiding this comment

chihsuan Feb 21, 2023

Choose a reason for hiding this comment

kenarsuleyman Feb 21, 2023

Choose a reason for hiding this comment

chihsuan Feb 24, 2023 • edited

Choose a reason for hiding this comment

adrianduffell commented Feb 8, 2023 • edited

kenarsuleyman commented Feb 8, 2023

adrianduffell commented Feb 13, 2023

codecov bot commented Feb 21, 2023 • edited

Codecov Report

chihsuan left a comment

Choose a reason for hiding this comment

Konamiman commented Mar 17, 2023 • edited

adrianduffell left a comment

Choose a reason for hiding this comment

adrianduffell commented Mar 20, 2023

adrianduffell commented Mar 20, 2023

kenarsuleyman commented Mar 20, 2023

kenarsuleyman commented Feb 2, 2023 •

edited

kenarsuleyman Feb 21, 2023 •

edited

chihsuan Feb 24, 2023 •

edited

adrianduffell commented Feb 8, 2023 •

edited

codecov bot commented Feb 21, 2023 •

edited

Konamiman commented Mar 17, 2023 •

edited