woocommerce-android icon indicating copy to clipboard operation
woocommerce-android copied to clipboard

Update automation that freezes strings for translation

Open mokagio opened this issue 3 years ago β€’ 10 comments

Description

After the prep work from #6741 and #6703, this PR updates the automation to freeze the strings for translation.

Notably it:

  • Uses the new source_id parameter to differentiate the generated strings
  • Uses the Fastlane APIs to interact with Git
  • Uses a structure consistent with WordPress Android

Testing instructions

To test this, I created a new branch from this one, hacked the complete_code_freeze lane to work outside of a release branch, and used it to verify the process succeeds. You can see it, including the commit with the new frozen strings, in #6800, but for reference here's the lane output

➜ bf complete_code_freeze

[βœ”] πŸš€
[14:46:14]: ------------------------------
[14:46:14]: --- Step: default_platform ---
[14:46:14]: ------------------------------
[14:46:14]: Driving the lane 'android complete_code_freeze' πŸš€
[14:46:14]: -------------------
[14:46:14]: --- Step: is_ci ---
[14:46:14]: -------------------
[14:46:14]: ---------------------------------------
[14:46:14]: --- Step: check_for_toolkit_updates ---
[14:46:14]: ---------------------------------------
[14:46:14]: Currently using release toolkit version 4.2.0.
[14:46:14]: Checking for updates now...
[14:46:16]: Your release toolkit is up-to-date! βœ…
[14:46:16]: -------------------------------------------------------
[14:46:16]: --- Step: Switch to android localize_libraries lane ---
[14:46:16]: -------------------------------------------------------
[14:46:16]: Cruising over to lane 'android localize_libraries' πŸš–
[14:46:16]: ----------------------------------------------
[14:46:16]: --- Step: android_download_file_by_version ---
[14:46:16]: ----------------------------------------------
[14:46:16]: Downloading WordPressLoginFlow/src/main/res/values/strings.xml from wordpress-mobile/WordPress-Login-Flow-Android at version 0.13.0 to /var/folders/29/wn6913pn73gf2l7j5n4n56mw0000gn/T
[14:46:17]: Logged in as: Gio Lodi
[14:46:17]: Strings.xml file for Login Library downloaded to /var/folders/29/wn6913pn73gf2l7j5n4n56mw0000gn/T/strings.xml.
[14:46:17]: ------------------------------
[14:46:17]: --- Step: an_localize_libs ---
[14:46:17]: ------------------------------
[14:46:17]: Merging Login Library strings into WooCommerce/src/main/res/values/strings.xml
[14:46:17]:  - Skipping enter_email_wordpress_com string
[14:46:17]:  - Skipping enter_email_for_site string
[14:46:17]:  - Skipping enter_site_address string
[14:46:17]:  - Skipping default_web_client_id string
[14:46:17]:  - Skipping login_notification_channel_id string
[14:46:17]: Done (0 added, 0 updated, 117 untouched, 5 skipped).
[14:46:17]: ----------------------------------------------
[14:46:17]: --- Step: android_download_file_by_version ---
[14:46:17]: ----------------------------------------------
[14:46:17]: Downloading library/src/main/res/values/strings.xml from Automattic/about-automattic-android at version 0.0.4 to /var/folders/29/wn6913pn73gf2l7j5n4n56mw0000gn/T
[14:46:18]: Strings.xml file for About Library downloaded to /var/folders/29/wn6913pn73gf2l7j5n4n56mw0000gn/T/strings.xml.
[14:46:18]: ------------------------------
[14:46:18]: --- Step: an_localize_libs ---
[14:46:18]: ------------------------------
[14:46:18]: Merging About Library strings into WooCommerce/src/main/res/values/strings.xml
[14:46:19]: Done (0 added, 0 updated, 27 untouched, 0 skipped).
[14:46:19]: ------------------------
[14:46:19]: --- Step: git_commit ---
[14:46:19]: ------------------------
[14:46:19]: $ git status WooCommerce/src/main/res/values/strings.xml --porcelain
[14:46:19]: Nothing to commit, working tree clean βœ….
[14:46:19]: Cruising back to lane 'android complete_code_freeze' 🚘
[14:46:19]: --------------------------------------------------------------------------
[14:46:19]: --- Step: Switch to android update_frozen_strings_for_translation lane ---
[14:46:19]: --------------------------------------------------------------------------
[14:46:19]: Cruising over to lane 'android update_frozen_strings_for_translation' πŸš–
[14:46:19]: ------------------------
[14:46:19]: --- Step: git_commit ---
[14:46:19]: ------------------------
[14:46:19]: $ git status ./fastlane/resources/values/strings.xml --porcelain
[14:46:19]: β–Έ M fastlane/resources/values/strings.xml
[14:46:19]: $ git commit -m Freeze\ strings\ for\ translation ./fastlane/resources/values/strings.xml
[14:46:19]: β–Έ [test/6742 b0a0fede9b] Freeze strings for translation
[14:46:19]: β–Έ 1 file changed, 11 insertions(+), 1 deletion(-)
[14:46:19]: Successfully committed "["./fastlane/resources/values/strings.xml"]" πŸ’Ύ.
[14:46:19]: Cruising back to lane 'android complete_code_freeze' 🚘
[14:46:19]: -------------------------------------
[14:46:19]: --- Step: ensure_git_status_clean ---
[14:46:19]: -------------------------------------
[14:46:19]: $ git status --porcelain
[14:46:19]: Git status is clean, all good! πŸ’ͺ
[14:46:19]: --------------------------------
[14:46:19]: --- Step: push_to_git_remote ---
[14:46:19]: --------------------------------
[14:46:19]: $ git push origin test/6742:test/6742 --tags
[14:46:28]: β–Έ remote:
[14:46:28]: β–Έ remote: Create a pull request for 'test/6742' on GitHub by visiting:
[14:46:28]: β–Έ remote:      https://github.com/woocommerce/woocommerce-android/pull/new/test/6742
[14:46:28]: β–Έ remote:
[14:46:28]: β–Έ remote: GitHub found 1 vulnerability on woocommerce/woocommerce-android's default branch (1 low). To find out more, visit:
[14:46:28]: β–Έ remote:      https://github.com/woocommerce/woocommerce-android/security/dependabot/8
[14:46:28]: β–Έ remote:
[14:46:28]: β–Έ To github.com:woocommerce/woocommerce-android
[14:46:28]: β–Έ * [new branch]            test/6742 -> test/6742
[14:46:28]: Successfully pushed to remote.

+------+----------------------------------------------------------+-------------+
|                               fastlane summary                                |
+------+----------------------------------------------------------+-------------+
| Step | Action                                                   | Time (in s) |
+------+----------------------------------------------------------+-------------+
| 1    | default_platform                                         | 0           |
| 2    | is_ci                                                    | 0           |
| 3    | check_for_toolkit_updates                                | 2           |
| 4    | Switch to android localize_libraries lane                | 0           |
| 5    | android_download_file_by_version                         | 0           |
| 6    | an_localize_libs                                         | 0           |
| 7    | android_download_file_by_version                         | 1           |
| 8    | an_localize_libs                                         | 0           |
| 9    | git_commit                                               | 0           |
| 10   | Switch to android update_frozen_strings_for_translation  | 0           |
|      | lane                                                     |             |
| 11   | git_commit                                               | 0           |
| 12   | ensure_git_status_clean                                  | 0           |
| 13   | push_to_git_remote                                       | 9           |
+------+----------------------------------------------------------+-------------+

[14:46:28]: fastlane.tools finished successfully πŸŽ‰

  • [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. – N.A.

mokagio avatar Jun 14 '22 05:06 mokagio

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-woocommerce[bot] avatar Jun 14 '22 05:06 peril-woocommerce[bot]

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

wpmobilebot avatar Jun 14 '22 05:06 wpmobilebot

@AliSoftware

the diff of the strings.xml file didn't show any line gaining the a8c-src-lib XML attribute πŸ€”

Finally had a chance to look into this. I basically compared how the action behaves for WordPress Android vs here. I noticed two things:

  • If I add tools:ignore="UnusedResources" to a node that comes from a library, then the automation correctly adds the a8c-src-lib attribute
  • If I remove this check (return :found if lib_string_node =~ main_string_node) in the release toolkit helper, then all the strings from libraries get the tools:ignore and a8c-src-lib attributes

I'm not familiar with how localizing Android strings works so not sure where to go from here. The only thing I can think of is to edit WooCommerce/src/main/res/values/strings.xml adding tools:ignore="UnusedResources" where appropriate (which as far as I can see is for every string coming from a library?) However, the way I read the release toolkit logic below makes me think me that's something the automation should already be able to do, so maybe there's a refinement to do there? πŸ€”

if main_string_node.attr('tools:ignore').nil?
                # No `tools:ignore` attribute; completely replace existing main string node with lib's one
                add_xml_attributes!(lib_string_node, library)

mokagio avatar Jun 29 '22 04:06 mokagio

Haven't had time to dig deeper in this yet, but given the expected behavior described in paaHJt-3fg-p2 I believe there might indeed be a bug in the toolkit. I'd expect the strings coming from the libs to be annotated with the a8c-src-lib attribute regardless if they have been overwritten by the app or have the tools:ignore attribute indeed.

But I'll need to look deeper into this to be sure which path of the code this goes thru and what's really happening. Planned to do that today… but got a surprise hotfix to take care of so that shattered my plans for the day; hopefully I'll be able to check on my tomorrow 🀞

AliSoftware avatar Jul 04 '22 17:07 AliSoftware

@mokagio any update on that?

I have to admit I'm a bit swamped with my projects right now so haven't had the occasion to circle back on this to understand if this lack of a8c-src-lib is a bug in the toolkit or in the config in this PR. Do you plan to take a look at this to debug what might be happening? I just don't want this PR to go stale and be forgotten πŸ˜… (and would love this L10n tooling work to be considered completed on WCA once this lands, I think?)

AliSoftware avatar Sep 05 '22 08:09 AliSoftware

@AliSoftware lol, I was waiting/hoping you could read that code and instantly figure out where the issue was.

But rest assured I have not forgotten this. However, I cannot promise I'll get to it during this sprint.

mokagio avatar Sep 19 '22 10:09 mokagio

πŸ‘‹ @mokagio @AliSoftware !

Pinging you on this as it was accidentally shown in a PR view of mine as I was searching for something, then I got curious. This is just a friendly reminder in case it was buried on top of other priorities. Are we suppose to follow-up on that at some point, wdyt? πŸ€”

ParaskP7 avatar Jan 17 '23 09:01 ParaskP7

We definitively should follow-up on that and finally finish up that workβ€”I think that was the last piece of the puzzle before finally considering the whole Localization Tooling Project Thread to be done, but we never had time to go back to it.

@mokagio I think you said previously that you were planning to take point on finishing the investigation on this and get to the bottom of it to finally finish this PR? Personally my hands are pretty full with all the work needed in Tumblr anyway, so I'll let that to you if you don't mind πŸ˜“

AliSoftware avatar Jan 17 '23 16:01 AliSoftware

Yeah nah. I have my hands full, too. I will get to this at some point, I suppose, but I'm not in a place to have the space to dig into the implementation details.

FWIW and future reference, I did try to look into it in the more quiet period in December, and couldn't wrap my head around why it works in WordPress but not here.

The only guess I have is that the inputs that WordPress was already formatted in such a way that met the expectation of the tool, while the one in Woo doesn't.

I had two things I would have liked to try:

  1. Write new tests using a meaningful subset of the inputs in this and WordPress repos, to see how they behave.
  2. Add new tests, duplicating existing ones, that replicate the XML attributes we see here, and see what they do.

mokagio avatar Jan 18 '23 03:01 mokagio