Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
628c372
Remove the `en-gb => en-US` hack for WP metadata
AliSoftware Oct 14, 2022
43783c2
DRY the `update_*_appstore_strings` lanes together
AliSoftware Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions fastlane/Fastfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ UI.user_error!('Please run fastlane via `bundle exec`') unless FastlaneCore::Hel

APP_SPECIFIC_VALUES = {
wordpress: {
display_name: 'WordPress',
metadata_dir: 'metadata',
glotpress_appstrings_project: 'https://translate.wordpress.org/projects/apps/android/dev/',
glotpress_metadata_project: 'https://translate.wordpress.org/projects/apps/android/release-notes/',
package_name: 'org.wordpress.android',
bundle_name_prefix: 'wpandroid'
},
jetpack: {
display_name: 'Jetpack',
metadata_dir: 'jetpack_metadata',
glotpress_appstrings_project: 'https://translate.wordpress.com/projects/jetpack/apps/android/',
glotpress_metadata_project: 'https://translate.wordpress.com/projects/jetpack/apps/android/release-notes/',
Expand Down
204 changes: 69 additions & 135 deletions fastlane/lanes/localization.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
# First are the locales which are used for *both* downloading the `strings.xml` files from GlotPress *and* for generating the release notes XML files.
{ glotpress: 'ar', android: 'ar', google_play: 'ar', promo_config: {} },
{ glotpress: 'de', android: 'de', google_play: 'de-DE', promo_config: {} },
{ glotpress: 'en-gb', android: 'en-rGB', google_play: 'en-US', promo_config: {} },
{ glotpress: 'es', android: 'es', google_play: 'es-ES', promo_config: {} },
{ glotpress: 'fr', android: 'fr-rCA', google_play: 'fr-CA', promo_config: false },
{ glotpress: 'fr', android: 'fr', google_play: 'fr-FR', promo_config: {} },
Expand Down Expand Up @@ -95,78 +94,36 @@
#####################################################################################
desc 'Updates the PlayStoreStrings.po files for WP + JP'
lane :update_appstore_strings do |options|
update_wordpress_appstore_strings(options)
update_jetpack_appstore_strings(options)
end

#####################################################################################
# update_wordpress_appstore_strings
# -----------------------------------------------------------------------------------
# This lane gets the data from the txt files in the `WordPress/metadata/` folder
# and updates the `.po` file that is then picked by GlotPress for translations.
# -----------------------------------------------------------------------------------
# Usage:
# fastlane update_wordpress_appstore_strings [version:<version>]
#
# Example:
# fastlane update_wordpress_appstore_strings version:10.3
#####################################################################################
desc 'Updates the PlayStoreStrings.po file for WordPress'
lane :update_wordpress_appstore_strings do |options|
metadata_folder = File.join(Dir.pwd, '..', 'WordPress', 'metadata')
version = options.fetch(:version, android_get_app_version)

# <key in po file> => <path to txt file to read the content from>
files = {
release_note: File.join(metadata_folder, 'release_notes.txt'),
release_note_short: File.join(metadata_folder, 'release_notes_short.txt'),
play_store_app_title: File.join(metadata_folder, 'title.txt'),
play_store_promo: File.join(metadata_folder, 'short_description.txt'),
play_store_desc: File.join(metadata_folder, 'full_description.txt')
}
files.merge!((1..9).map do |n|
[:"play_store_screenshot_#{n}", File.join(metadata_folder, "screenshot_#{n}.txt")]
end.to_h)

update_po_file_for_metadata_localization(
po_path: File.join(metadata_folder, 'PlayStoreStrings.po'),
sources: files,
release_version: version,
commit_message: "Update WordPress `PlayStoreStrings.po` for version #{version}"
)
end

#####################################################################################
# update_jetpack_appstore_strings
# -----------------------------------------------------------------------------------
# This lane gets the data from the txt files in the `WordPress/jetpack_metadata/` folder
# and updates the `.po` file that is then picked by GlotPress for translations.
# -----------------------------------------------------------------------------------
# Usage:
# fastlane update_jetpack_appstore_strings [version:<version>]
#
# Example:
# fastlane update_jetpack_appstore_strings version:10.3
#####################################################################################
desc 'Updates the PlayStoreStrings.po file for Jetpack'
lane :update_jetpack_appstore_strings do |options|
metadata_folder = File.join(Dir.pwd, '..', 'WordPress', 'jetpack_metadata')
version = options.fetch(:version, android_get_app_version)
# If no `app:` is specified, call this for both WordPress and Jetpack
apps = options[:app].nil? ? %i[wordpress jetpack] : Array(options[:app]&.downcase&.to_sym)

apps.each do |app|
app_values = APP_SPECIFIC_VALUES[app]

metadata_folder = File.join(PROJECT_ROOT_FOLDER, 'WordPress', app_values[:metadata_dir])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🥇

version = options.fetch(:version, android_get_app_version)

# <key in po file> => <path to txt file to read the content from>
files = {
release_note: File.join(metadata_folder, 'release_notes.txt'),
release_note_short: File.join(metadata_folder, 'release_notes_short.txt'),
play_store_app_title: File.join(metadata_folder, 'title.txt'),
play_store_promo: File.join(metadata_folder, 'short_description.txt'),
play_store_desc: File.join(metadata_folder, 'full_description.txt')
}
# Add entries for `screenshot_*.txt` files as well
Dir.glob('screenshot_*.txt', base: metadata_folder).sort.each do |screenshot_file|
key = "play_store_#{File.basename(screenshot_file, '.txt')}".to_sym
files[key] = File.join(metadata_folder, screenshot_file)
end

files = {
release_note: File.join(metadata_folder, 'release_notes.txt'),
release_note_short: File.join(metadata_folder, 'release_notes_short.txt'),
play_store_app_title: File.join(metadata_folder, 'title.txt'),
play_store_promo: File.join(metadata_folder, 'short_description.txt'),
play_store_desc: File.join(metadata_folder, 'full_description.txt')
}

update_po_file_for_metadata_localization(
po_path: File.join(metadata_folder, 'PlayStoreStrings.po'),
sources: files,
release_version: version,
commit_message: "Update Jetpack `PlayStoreStrings.po` for version #{version}"
)
update_po_file_for_metadata_localization(
po_path: File.join(metadata_folder, 'PlayStoreStrings.po'),
sources: files,
release_version: version,
commit_message: "Update #{app_values[:display_name]} `PlayStoreStrings.po` for version #{version}"
)
end
end

# Updates the metadata in the Play Store (Main store listing) from the content of `fastlane/{metadata|jetpack_metadata}/android/*/*.txt` files
Expand Down Expand Up @@ -209,70 +166,47 @@
#####################################################################################
desc 'Downloads translated metadata from GlotPress'
lane :download_metadata_strings do |options|
download_wordpress_metadata_strings(options)
download_jetpack_metadata_strings(options)
end

desc "Downloads WordPress's translated metadata from GlotPress"
lane :download_wordpress_metadata_strings do |options|
app_values = APP_SPECIFIC_VALUES[:wordpress]
values = options[:version].split('.')
files = {
"release_note_#{values[0]}#{values[1]}" => { desc: "changelogs/#{options[:build_number]}.txt", max_size: 500, alternate_key: "release_note_short_#{values[0]}#{values[1]}" },
play_store_app_title: { desc: 'title.txt', max_size: 30 },
play_store_promo: { desc: 'short_description.txt', max_size: 80 },
play_store_desc: { desc: 'full_description.txt', max_size: 4000 }
}

delete_old_changelogs(app: 'wordpress', build: options[:build_number])
download_path = File.join(Dir.pwd, app_values[:metadata_dir], 'android')
# The case for the source locale (en-US) is pulled in a hacky way, by having an {en-gb => en-US} mapping as part of the WP_RELEASE_NOTES_LOCALES,
# which is then treated in a special way by gp_downloadmetadata by specifying a `source_locale: 'en-US'` to process it differently from the rest.
gp_downloadmetadata(
project_url: app_values[:glotpress_metadata_project],
target_files: files,
locales: WP_RELEASE_NOTES_LOCALES,
source_locale: 'en-US',
download_path: download_path
)

git_add(path: download_path)
git_commit(path: download_path, message: "Update WordPress metadata translations for #{options[:version]}", allow_nothing_to_commit: true)
push_to_git_remote
end

desc "Downloads Jetpack's translated metadata from GlotPress"
lane :download_jetpack_metadata_strings do |options|
UI.message('Hey')
app_values = APP_SPECIFIC_VALUES[:jetpack]
values = options[:version].split('.')
files = {
"release_note_#{values[0]}#{values[1]}" => { desc: "changelogs/#{options[:build_number]}.txt", max_size: 500, alternate_key: "release_note_short_#{values[0]}#{values[1]}" },
play_store_app_title: { desc: 'title.txt', max_size: 30 },
play_store_promo: { desc: 'short_description.txt', max_size: 80 },
play_store_desc: { desc: 'full_description.txt', max_size: 4000 }
}

delete_old_changelogs(app: 'jetpack', build: options[:build_number])
download_path = File.join(Dir.pwd, app_values[:metadata_dir], 'android')
gp_downloadmetadata(
project_url: app_values[:glotpress_metadata_project],
target_files: files,
locales: JP_RELEASE_NOTES_LOCALES,
download_path: download_path
)

# For WordPress, the en-US release notes come from using the source keys (instead of translations) downloaded from GlotPress' en-gb locale (which is unused otherwise).
# But for Jetpack, we don't have an unused locale like en-gb in the GP release notes project, so copy from source instead as a fallback
metadata_source_dir = File.join(Dir.pwd, '..', 'WordPress', 'jetpack_metadata')
FileUtils.cp(File.join(metadata_source_dir, 'release_notes.txt'), File.join(download_path, 'en-US', 'changelogs', "#{options[:build_number]}.txt"))
FileUtils.cp(
['title.txt', 'short_description.txt', 'full_description.txt'].map { |f| File.join(metadata_source_dir, f) },
File.join(download_path, 'en-US')
)
version = options.fetch(:version, android_get_app_version)
build_number = options.fetch(:build_number, android_get_release_version['code'])

# If no `app:` is specified, call this for both WordPress and Jetpack
apps = options[:app].nil? ? %i[wordpress jetpack] : Array(options[:app]&.downcase&.to_sym)

apps.each do |app|
app_values = APP_SPECIFIC_VALUES[app]

version_suffix = version.split('.').join
files = {
"release_note_#{version_suffix}" => { desc: "changelogs/#{build_number}.txt", max_size: 500, alternate_key: "release_note_short_#{version_suffix}" },
play_store_app_title: { desc: 'title.txt', max_size: 30 },
play_store_promo: { desc: 'short_description.txt', max_size: 80 },
play_store_desc: { desc: 'full_description.txt', max_size: 4000 }
}

delete_old_changelogs(app: app, build: build_number)

download_path = File.join(Dir.pwd, app_values[:metadata_dir], 'android')
locales = { wordpress: WP_RELEASE_NOTES_LOCALES, jetpack: JP_RELEASE_NOTES_LOCALES }[app]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

🤞 ❤️ 🙏

UI.header("Downloading metadata translations for #{app_values[:display_name]}")
gp_downloadmetadata(
project_url: app_values[:glotpress_metadata_project],
target_files: files,
locales: locales,
download_path: download_path
)

git_add(path: download_path)
git_commit(path: download_path, message: "Update Jetpack metadata translations for #{options[:version]}", allow_nothing_to_commit: true)
# Copy the source `.txt` files (used as source of truth when we generated the `.po`) to the `fastlane/*metadata/android/en-US` dir,
# as `en-US` is the source language, and isn't exported from GlotPress during `gp_downloadmetadata`
metadata_source_dir = File.join(PROJECT_ROOT_FOLDER, 'WordPress', app_values[:metadata_dir])
FileUtils.cp(File.join(metadata_source_dir, 'release_notes.txt'), File.join(download_path, 'en-US', 'changelogs', "#{build_number}.txt"))
FileUtils.cp(
['title.txt', 'short_description.txt', 'full_description.txt'].map { |f| File.join(metadata_source_dir, f) },
File.join(download_path, 'en-US')
)

git_add(path: download_path)
git_commit(path: download_path, message: "Update #{app_values[:display_name]} metadata translations for #{version}", allow_nothing_to_commit: true)
end
push_to_git_remote
end

Expand Down
2 changes: 1 addition & 1 deletion fastlane/lanes/release.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@
def get_app_name_option!(options)
app = options[:app]&.downcase
UI.user_error!("Missing 'app' parameter. Expected 'app:wordpress' or 'app:jetpack'") if app.nil?
unless ['wordpress', 'jetpack'].include?(app)
unless %i[wordpress jetpack].include?(app.to_sym)
UI.user_error!("Invalid 'app' parameter #{app.inspect}. Expected 'wordpress' or 'jetpack'")
end
return app
Expand Down