[Tooling] Fix issue with en-US release notes not generated if en-GB was not translated

[AliSoftware]

What is this fixing?

This fixes an issue we've been randomly having in the past couple of releases, where if the release notes didn't have any translation for en-GB in GlotPress, then the en-US release notes would not be generated in fastlane/metadata/android/en-US/changelogs/*.txt, leading to missing en-US release notes in Play Store (and Play Store falling back to use the Arabic release notes as default language 😱 !)

The latest occurrences of this happening to us was during the 20.6 submission here, and today with the 20.9 submission to the Play Store — where we almost shipped to end users with those en-US release notes missing and that ar fallback… if it hadn't has been for a careful PR review by Petros catching the missing en-US ones amongst the noise and flood of other changelogs/*.txt in the PR diff.

[See also: internal ref pdnsEh-jJ-p2#comment-1233]

Why did it happen?

The main reason for this issue to have happened was that we relied on a ugly hack when calling gp_downloadmetadata by providing a en-GB => en-US mapping as part of its locales parameter, then setting its source_locale: 'en-US', which had the effect of gp_downloadmetadata to download the GlotPress export for the en-GB translations, and then extract the source copy (en-US) from that translation entry.
This didn't work when the release notes don't have an en-GB translation in GlotPress for the current release notes, as in those cases the export would be empty (and thus not even export the source en-US copy alongside it)

How does this PR fix it?

• Removes the en-gb => en-US hack for WP metadata, to instead use the same solution we've been using for the case of Jetpack (whose lanes never relied on such hack, unlike WordPress' ones), i.e. doing a FileUtils.cp of the source files to fastlane/metadata/android/en-US explicitly for the en-US case
• DRY the download_metadata_strings lanes (into a single one, that can take app: if necessary, defaulting to act on both apps), now that the two implementations were the same after removing the hack
• DRY the update_appstore_strings lanes together (into a single one, that can take app: if necessary, defaulting to act on both apps) while we were at it, inspired by the previous DRY-ing

Note: The diff end up looking a bit long, but in practice it's mostly replacing the duplicated code from download_wordpress_metadata_strings and download_jetpack_metadata_strings with only one instance of that same code, which ends up being indented one extra level due to the apps.each do |app| loop, and have slight tweaks compared to each original code to make the code generic over said app. This means that it might be easier to review this PR with "Ignore Whitespaces" option turned on to make the diff clearer to understand and compare.

To test

I've tested this change by:

• Cutting a test branch from this one ➡️ tooling/test-pr-17341
• Removing all the *.txt files from fastlane/metadata/android and fastlane/jetpack_metadata/android, and doing some dummy modifications to the WordPress/metadata/*.txt and WordPress/jetpack_metadata/*.txt source files ➡️ 2d75cde
• Running bundle exec fastlane update_appstore_strings to re-generate the .po file, and checking that the dummy modifications I made were taken into account in the new .po files ➡️ 7220fe0 & 04cb6f2
• Running bundle exec fastlane download_metadata_strings to re-generate the fastlane/*metadata/android/**/*.txt files from GlotPress, and checking that not only they were all regenerated with the current GlotPress content, but especially that the fastlane/{jetpack_}metadata/android/en-US/changelogs/1281.txt file was properly created with the content of the WordPress/{jetpack_}metadata/release_notes.txt files. ➡️ dcb1326 & a73cbf8

[wpmobilebot]

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17341-43783c2.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	WordPress
	Build Flavor	Jalapeno
	Build Type	Debug
	Commit	43783c2

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

[wpmobilebot]

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17341-43783c2.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	Jetpack
	Build Flavor	Jalapeno
	Build Type	Debug
	Commit	43783c2

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

[AliSoftware]

Ideally this info should go into APP_SPECIFIC_VALUES constant like the others.

But for this to be possible, I'd need to first finish my long-postponed work on adding a Locale Helper to the release-toolkit (initially started in wordpress-mobile/release-toolkit#296, and re-implemented a bit differently and more simply recently as part of https://github.com/wordpress-mobile/release-toolkit/pull/401/files#diff-bdb20f98842eb1bfe87ab2b48e33f15dccb6779e9509cf514759d8bbaa605378, both still being in Draft stage), so that we could just use the list of GlotPress locale codes in new app_locales: and release_notes_locales: keys of the APP_SPECIFIC_LOCALES constant — instead of having to move all the long and complex constants like ALL_LOCALES and all the related *_LOCALES_*constants defined next to it and doing some dance based on it like we do, which makes it quite impractical to move to APP_SPECIFIC_LOCALES in Fastfile.

So I thought that this solution for this line would do in the meantime; I really hope I can continue and finish my work on the Locale Helper in release-toolkit at some point, which would then simplify a lot of code manipulating locale-related info and constants in many places in our Fastfile code, including this here.

[ParaskP7]

I really hope I can continue and finish my work on the Locale Helper in release-toolkit at some point

🤞 ❤️ 🙏

[ParaskP7]

LGTM! 💯

Thanks for fixing this long-standing bug and for removing all this duplication @AliSoftware, love it! 🙇 ❤️ 🌟

I've just left a suggestion (💡) for you to consider, but other than that it is a 👍 from my side, feel free to merge.

[ParaskP7]

Suggestion (💡): I find lots of references to this 'WordPress' folder here and there, and it took me some seconds to make sure that this code is not doing something specific for the WordPress vs. the Jetpack case, thus maybe it it time we extract this constant to some naming convention that doesn't reference WordPress, but applies to Jetpack as well, something like APP_MODULE_FOLDER = 'WordPress'. Wdyt? 🤔

PS.1: I am also seeing lot of lowercased constants like 'wordpress' and jetpack, for the app and app_flavor parameters. If you like my above suggestion I would also make an additional effort now and extract those into constants as well, just so to make those explicit and possibly in the future more mainstream, like:

• app
  • WORDPRESS_APP_NAME = 'wordpress'
  • JETPACK_APP_NAME = 'jetpack'
• app_flavor
  • WORDPRESS_APP_FLAVOR = 'wordpress`
  • JETPACK_APP_FLAVOR = 'jetpack'

Maybe adding the above to the APP_SPECIFIC_VALUES values is the right way, or reuse them from there, or somewhere else, I am not sure, but I would love to not have it there as is, like a magic number not being explicitly extracted to a constant... 🤔

PS.2: All these suggestions apply to all such fastlane files, not just localization.rb, but we can start with that if extracting for all is too out-of-scope for this PR. 🤔

[AliSoftware]

Suggestion (💡): I find lots of references to this 'WordPress' folder here and there, and it took me some seconds to make sure that this code is not doing something specific for the WordPress vs. the Jetpack case, thus maybe it it time we extract this constant to some naming convention that doesn't reference WordPress, but applies to Jetpack as well, something like APP_MODULE_FOLDER = 'WordPress'. Wdyt?

Note that this WordPress/ folder is referencing the folder at the root of the repo. We don't have a Jetpack folder at the root of the repo, just a WordPress/ one (and then we have subfolders in src for main vs wordpress vs jetpack for the different flavors).

I'm totally ok to extract that WordPress root folder name into some constant like APP_MODULE_FOLDER, though.

I'm also OK in adding an app_flavor entry to the APP_SPECIFIC_VALUES hash, to separate the name of the key used (which was based on the app flavor name to begin with, because we had to choose something to base the app: keys off of 😛 … but we can totally make the name of the app flavor explicit as an app_flavor entry to make the key used for APP_SPECIFIC_VALUES and the app flavor disconnected)

I plan to do that in a separate PR though, in fact I already have a WIP branch that cleans up some of those constants and the APP_SPECIFIC_VALUES that I plan to improve further 🙂

[ParaskP7]

Note that this WordPress/ folder is referencing the folder at the root of the repo. We don't have a Jetpack folder at the root of the repo, just a WordPress/ one (and then we have subfolders in src for main vs wordpress vs jetpack for the different flavors).

Yeap, I know, I understand that to 100%, but this is just unfortunate because Jetpack was born later and reused the WordPress codebase! 👍

I'm totally ok to extract that WordPress root folder name into some constant like APP_MODULE_FOLDER, though.

Exactly, thanks, having to refer to a module instead of WordPress seems a much better choice.

I'm also OK in adding an app_flavor entry to the APP_SPECIFIC_VALUES hash, to separate the name of the key used (which was based on the app flavor name to begin with, because we had to choose something to base the app: keys off of 😛 … but we can totally make the name of the app flavor explicit as an app_flavor entry to make the key used for APP_SPECIFIC_VALUES and the app flavor disconnected)

👍

I plan to do that in a separate PR though, in fact I already have a WIP branch that cleans up some of those constants and the APP_SPECIFIC_VALUES that I plan to improve further 🙂

Coolio, thanks for even considering! 🥇

[AliSoftware]

Updated the target branch to release/21.0 (since code freeze happened since I opened this PR), so that we can benefit from this fix during 21.0 release and avoid having that bug when we'll submit it in 11 days.