Fix `android_download_translations` issues

Fixes #569 — i.e. the issues that DayOne Android encountered in their last 2024.14 release (See p1720472497193469-slack-CC7L49W13)

What does it do?

Replicate all XML attributes (e.g. tools:ignore="ImpliedQuantity") from original values/strings.xml on all nodes of translated XMLs exported from GlotPress
Apply text substitutions (...=>…, -=>–, etc) on plurals/item nodes too (before it was only on string and string-array nodes)
Improve our quick_lint check on each copies to (1) be run on all nodes containing a string (including plurals/item and string-array/item not just string) and (2) warn about + auto-fix \@string/… in exports (suggesting to add translatable="false" on the offending key(s) to fix the issue at its root in the future)

Checklist before requesting a review

[x] Run bundle exec rubocop to test for code style violations and recommendations
[x] Add Unit Tests (aka specs/*_spec.rb) if applicable
[x] Run bundle exec rspec to run the whole test suite and ensure all your tests pass
[x] Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
[x] ~If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.~

How to Test

Run bundle exec rspec and validate all unit tests pass
In the DayOne Android repo:
- Create a test branch
- Run bundle exec fastlane download_translations, then commit the changes if any (this will serve as the "before" baseline for the rest of the test)
- Update the Gemfile to point release-toolkit to gem 'fastlane-plugin-wpmreleasetoolkit', git: 'git@github.com:wordpress-mobile/release-toolkit', branch: 'fix/android_download_translations'
- Run bundle install
- Run bundle exec fastlane download_translations again, and check the diff. Especially validate that any \@string/ escaped references previously present in translations were properly fixed, and that attributes present in the values/strings.xml were now all properly replicated on all the translated XMLs
- Run ./gradlew lint and validate it passes
- Edit the app/src/main/res/values/strings.xml:
  - Pick a plural that has been translated in other locales (ideally we'd pick confirm_entry_trash_multiple since that's the one that caused the problem in 2024.14 release, but at the time of writing this PR this key is not present in GlotPress anymore), like daily_prompt_card_menu_view_entries for example
  - Add the tools:ignore="ImpliedQuantity" attribute on one of its item child nodes, e.g. <item quantity="one" tools:ignore="ImpliedQuantity">View one Entry</item>
  - Don't forget to also add the tools XML namespace at the top of the XML: <resources xmlns:tools="http://schemas.android.com/tools">
- Re-run bundle exec fastlane download_translations, and verify that this new tools:ignore="ImpliedQuantity" attribute has been replicated on the daily_prompt_card_menu_view_entries item node of all translated XMLs
- Run ./gradlew lint and validate it passes
Replicate a similar test on WPAndroid and WCAndroid, to validate we wouldn't break those (I'm starting to wonder why we only copied a specific list of attributes before this PR instead of copying them all… maybe there was a reason that I forgot about?)

cc @danilo04 @AmandaRiu

What's Next

Do a new release of release-toolkit
Point DayOne Android to the new version
Inform DOAndroid team that they can revert their workaround and go back to use tools:ignore="ImpliedQuantity" on their plurals/item nodes.
- Any attribute present in values/strings.xml will be replicated in the downloaded-and-post-processed values-{locale}/strings.xml for each corresponding node now.
- Removing their workaround which tests if (entries.size > 1) and go back to ImpliedQuantity will allow to let the OS cover even cases of locales which are using quantity=one not just for 1 but for other values (e.g. Bielorussian uses this plural form also for 11, 21, 31, …)
Inform DOAndroid that, while testing the change on their codebase, it now rightfully informed of a warning about daily_prompt_settings_notifications being a string reference but not having translatable=false. As a side effect, while other translators have left it untouched in GlotPress, the Italian translator did translate @string/notifications to \@stringa/notifiche (and because the prefix is now \@stringa/ not \@string/, this wasn't auto-fixed by the action like it now is for the others).

I ran a few tests and while it worked fine, I noticed a few things specially when running on WPAndroid.

I've pushed my iangmaia/translations-fix-tests branch to show some concrete examples:

On the first run with the updated Gem (f101715ea9f7cd820631503038b428e461421518), it (correctly) added quite a few a8c-src-lib= attributes -- which I don't think should be an issue, but I wonder what's the use of them? 🤔 Anyway, perhaps out of the scope of this PR.
I think this may be a known issue as I remember seeing it before, but mentioning just in case: on my second run (c627bd769c1b052b1eaa07039d9cbb186c467db8), I didn't expect any other change than my removal of translatable="false" from app_theme_entry_values, but it changed the WordPress/src/main/res/values-es-rCL/strings.xml file 😕 and looking closely, it seems just that the order of some strings changed? I wonder if that's from GlotPress or on the Gem side, and if we could force sorting the xml (which would initially create a big diff, but the subsequent ones would be very small and without these strings changing position).
Third run (f402ce4c79000275c794f77764e7a11940c48610) and fourth run (16d3ffb0f10091aaef5b643d3210927bafc29623), same thing as above: no other change but some random strings changing order.
I didn't have this issue on DOAndroid, but WCAndroid showed something similar (see the diff for WooCommerce/src/main/res/values-de/strings.xml).

WCAndroid showed a lot of warnings (as expected). But the warnings for the same string, same language showed multiple times:

[20:53:49]: Downloading translations for 'es' from GlotPress (es) [{:status=>"current"}]...
[20:53:55]: Warning: [es] exported translation for 'analytics_date_range_selectors' contains `\@string/`. This is a sign that this entry was not marked as `translatable=false` in the original `values/strings.xml`, and was thus sent to GlotPress, which added the backslash when exporting it back.
[20:53:55]: Warning: [es] exported translation for 'analytics_date_range_selectors' contains `\@string/`. This is a sign that this entry was not marked as `translatable=false` in the original `values/strings.xml`, and was thus sent to GlotPress, which added the backslash when exporting it back.
[20:53:55]: Warning: [es] exported translation for 'analytics_date_range_selectors' contains `\@string/`. This is a sign that this entry was not marked as `translatable=false` in the original `values/strings.xml`, and was thus sent to GlotPress, which added the backslash when exporting it back.
[...]

I don't think these are critical, specially the sorting issue that seems to be there for some time, but it's perhaps good to tackle them in a near future.

Thanks for the thorough testing @iangmaia !

I think this may be a known issue as I remember seeing it before, but […] looking closely, it seems just that the order of some strings changed?

The non-stable order of the nodes is a known issue (I actually don't remember if this comes from GlotPress not guaranteeing the order during export, or from our post-processing, but I think that's the former?), and one we've long wanted to solve.

I wonder if that's from GlotPress or on the Gem side, and if we could force sorting the xml

Yep, that would be the idea, by adding some sorting of the strings post-export during our post-processing of the XML files. We just never found the time to get to that (also I think because sorting an XML might not be as trivial as it looks, e.g. making sure we keep the child nodes of the same <plurals> and <string-array> together, avoiding blank lines that could be inserted by Nokogiri if we remove a node in one place to insert it in another, …). So this will be for another time 😅

But the warnings for the same string, same language showed multiple times:

This is kind of expected, because that string is a string-array and it was warning you about each element of the array that had that issue.

So if you have:

 <string-array name="weekday_selection">
        <item>None</item>
        <item>\@string/monday</item>
        <item>\@string/tuesday</item>
        <item>\@string/wednesday</item>
        <item>\@string/thursday</item>
        <item>\@string/friday</item>
        <item>\@string/saturday</item>
        <item>\@string/sunday</item>
        <item>All</item>
</string-array>

Then it will warn 7 times: one for each of the <item> that contains such a \@string/, but not for the 2 <item> that contain a plain text like None or All.

I considered making the error message include the index of the child item within the string-array, but that would have made the code quite complicated for little benefit. And that would have made it less DRY-able to also cover the similar case of <plurals>, for which the interesting info in the error message wouldn't be the index of the <item> node, but the value of the corresponding @quantity attribute of the offending <item>:

<string name="empty_selection">No item selected</string>
<string name="delete_one">Delete this item?</string>
<plurals name="delete">
  <item quantity="zero">\@string/empty_selection</item>
  <item quantity="one" tools:ignore="ImpliedQuantity">\@string/delete_one</item>
  <item quantity="other">Delete %d items?</item>
</plurals>

I also considered de-duplicating the error messages to only print one, but given the examples above I figured it still made sense to warn as many times as there were violations (one per violating <item> especially since not all <item> children might violate the rule (e.g. <item>None</item>, <item>All</item> and <item quantity="other"> in examples above).

So in the end I figured it was not worth the effort and figure it'd be ok to have as many warnings as there are invalid \@string/ found even if those are on different <item> of the same <string-array> or <plurals>.

it (correctly) added quite a few a8c-src-lib= attributes -- which I don't think should be an issue, but I wonder what's the use of them?

For the record, this is an attribute our tooling adds to strings that got merged from libraries, so we can easily identify them and understand where they come from. For more context, see paqN3M-xk-p2 🙂

wordpress-mobile / release-toolkit