Add localization download and upload automation for Jetpack by mokagio · Pull Request #16733 · wordpress-mobile/WordPress-iOS

mokagio · 2021-06-24T04:54:05Z

Adds the tooling required to download the localized metadata for Jetpack and to upload it to App Store Connect.

How to test

Metadata download

Create a new branch from this one
Push it to origin so that when the release toolkit logic tries to push it won't fail
Run git revert f93a7cfb7b to ensure you'll have localization changes to fetch
Run bundle exec fastlane download_localized_strings_and_metadata
The lane should succeed and you should see three commits in your branch updating the different localized files

* fd94ac7d55 - (HEAD -> test/mokagio-pr-16733) Update Jetpack metadata translations (70 seconds ago)  Gio Lodi
* 99a4f67586 - (origin/test/mokagio-pr-16733) Update metadata translations (85 seconds ago)  Gio Lodi
* 0ee83d8851 - Update translations (2 minutes ago)  Gio Lodi
* 5c21a48c39 - Revert "Update metadata translations" (4 minutes ago)  Gio Lodi

Here's the commit history for my test branch if you want to compare.

Metadata upload

Unfortunately, the new lane that uploads metadata to ASC for both WordPress and Jetpack can't be tested end-to-end unless we actually upload the data. If we cancel the deliver submission for WordPress, the lane interrupts without giving us a chance to test the Jetpack call. As such, Jetpack needs to be tested in isolation.

Checkout this branch
Run bundle exec fastlane update_metadata_on_app_store_connect
Verify the Preview.html contains valid data. In particular, that it's not attempting to upload screenshots

Cancel the upload 🙏 Does the Preview on path './fastlane/Preview.html' look okay for you? (y/n) n
Repeat for Jetpack bundle exec fastlane update_jetpack_metadata_on_app_store_connect

_Notice how the description says Jetpack, not WordPress 😉

Cancel the upload 🙏 Does the Preview on path './fastlane/Preview.html' look okay for you? (y/n) n

Optionally, call one or both lanes with with_screenshots:true to see that it will attempt to load the screenshots

Obviously, one should only call this lane if they have the screenshots locally

Regression Notes

Potential unintended areas of impact
N.A.
What I did to test those areas of impact (or what existing automated tests I relied on)
N.A.
What automated tests I added (or what prevented me from doing so)
N.A.

I have completed the Regression Notes.
I have considered adding unit tests for my changes. N.A.
I have considered adding accessibility improvements for my changes. N.A.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

peril-wordpress-mobile · 2021-06-24T04:54:08Z

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

peril-wordpress-mobile · 2021-06-24T04:54:08Z

You can trigger an installable build for these changes by visiting CircleCI here.

mokagio · 2021-06-24T05:01:15Z

  lane :new_beta_release do |options|
    ios_betabuild_prechecks(options)
-    ios_update_metadata(options)
+    download_localized_strings_and_metadata(options)


This is a new lane that does the .strings download for WordPress (which Jetpack shares) and the metadata download for WordPress and Jetpack.

I decide to name it like that because I found the release toolkit action name misleading. This name seems more accurate to me. What do you think?

In particular, "download" clearly tells me where the data is coming from, where as "update" could be going both way (e.g. it could be the app updating/uploading metadata to ASC)

💯 + 1000000 on misleading toolkit action names, that has been bothering me for a while and has been a big barrier to entry that made me almost lose my mind the first times I had to work with the toolkit 😅

In the long term I'm hoping we could rename them all (probably deprecating the existing ones and creating new ones in their place, taking the occasion to modernize and refactor their code), but that's for another day.

mokagio · 2021-06-24T05:05:09Z

+  # bundle exec fastlane update_metadata_on_app_store_connect with_screenshots:true
+  #####################################################################################
+  desc 'Updates the App Store Connect localized metadata'
+  lane :update_metadata_on_app_store_connect do |options|


We hinted at having a lane replacing the call to deliver in this PR.

The reason I think it's valuable to use a lane is that we could remove the Deliverfile and have the Fastfile as the single source of truth. What do you think?

I went down that path for the Jetpack logic, but kept the current setup for WordPress – there's enough going on in here already 😄

I'm happy to revert to use deliver if you don't think this is a good approach, maybe passing a dedicated Jetpack-Deliverfile to the call. But I'd ask to do it as a followup PR against the next release since this one is getting close to the release finalization date.

I agree it can make sense to have a single source of truth. But:

Having that PR ending up doing it one way for Jetpack and another for WordPress actually moves away from that goal, because now we have 3 potential source of truth (Deliver + a bit in Fastfile too for WP, and Jetpack-Fastfile for Jetpack. So personally I would revert the change in that PR to make it only call deliver for now – aligning this with the current setup used for WordPress – and work on this on a separate PR

That separate PR thwt would work on having a unique source of truth might lead to a debate (e.g. given Jetpack-Fastfile is already a separate file, should we put the source of truth in Fastfile, making it not obvious when reading the Jetpack-Fastfile that some constants come from the main Fastfile but are not obivous and visible in the Jetpack-Fastfile? Or given we have 2 fastfiles, should we actually consider the Deliverfile as a good thing to be used as a common source of truth for both?

If we only had one Fastfile I'd agree to get rid of the Deliverfile and use the Fastfile as the single source of truth, but given we already split it the fastfile there… I think we should brainstorm what's best and keep that debate for a separate PR

mokagio · 2021-06-24T05:07:42Z

@@ -1,3 +1,7 @@
+# frozen_string_literal: true


This is just a nice to have, but a no-op for the changes in this PR.

mokagio · 2021-06-24T05:09:47Z

+  # No need to `cd` into `fastlane` because of how Fastlane manages its paths
+  # internally.
+  sh './download_metadata.swift jetpack'
+
+  jetpack_metadata_glob = File.join(PROJECT_ROOT_FOLDER, 'fastlane', 'jetpack_metadata/**/*.txt')
+  sh "git add #{jetpack_metadata_glob}"
+  git_commit(
+    path: jetpack_metadata_glob,
+    message: 'Update Jetpack metadata translations'
+  )


I decided to hand-roll the automation to download metadata using the Swift script in its first iteration instead of extracting directly into the release toolkit.

I'm not actually sure what would be the best way to extract it... Maybe move the script to the release toolkit and pass it a JSON/YML config file where to read all the params such as the GlotPress URL that are currently hardcoded?

mokagio · 2021-06-24T05:11:08Z

+
+  deliver(
+    app_identifier: APP_IDENTIFIER,
+    app_version: read_version_from_config,


I think we should to this (reading the version from the .xcconfig) for all the iOS apps. It would remove the need to modify the Deliverfile during the code freeze.

Good point and good idea! 👍

mokagio · 2021-06-24T05:11:55Z

+  with_screenshots = options.fetch(:with_screenshots, false)
+  skip_screenshots = with_screenshots == false
+
+  deliver(


All the settings below are ports of what's already defined in Deliverfile.

See previous comment, it make sense if we agree to split indeed, but it might be too soon to decide how to split and I'd prefer to migrate to the new way for both WordPress and Jetpack all at once, instead of having a mix of new way for Jetpack, old way for WP.

mokagio · 2021-06-24T05:12:14Z

+    # Reusing the WordPress one here, the values are the same after all
+    app_rating_config_path: File.join(PROJECT_ROOT_FOLDER, 'fastlane', 'metadata', 'ratings_config.json')


Makes sense?

mokagio · 2021-06-24T05:13:01Z

+struct Config {
+    let baseFolder: String
+    let baseURLString: String
+
+    static let wordPress = Config(
+        baseFolder: "./metadata",
+        baseURLString: "https://translate.wordpress.org/projects/apps/ios/release-notes/"
+    )
+
+    static let jetpack = Config(
+        baseFolder: "./jetpack_metadata",
+        baseURLString: "https://translate.wordpress.com/projects/jetpack/apps/ios/release-notes/"
+    )
+
+    static func config(for argument: String?) -> Config {
+        guard let argument = argument else { return .wordPress }
+        return argument == "jetpack" ?  .jetpack : .wordPress
+    }
+}


Had a bit of Swift fun here. But I worry I ended up with something over-engineered...

I like it (but it might be because I miss Swift too and playing with it 😅 )

mokagio · 2021-06-24T05:14:41Z


-func downloadTranslation(languageCode: String, folderName: String) {
+func downloadTranslation(
+    config: Config = .config(for: CommandLine.arguments.second),


There's actually a more idiomatic way of doing this arguments management in the form of ArgumentParser but it seemed out of scope for the context of this PR.

That would require us to use a Package.swift to pull the ArgumentParser package first via SwiftPM to be then able to use it, tho 😓

mokagio · 2021-06-24T05:15:03Z

-            guard let index = key.index(of: Character(UnicodeScalar(0004))) else {
+            guard let index = key.firstIndex(of: Character(UnicodeScalar(0004))) else {


The warning on this had been nagging me for ages!

AliSoftware

Quick initial review after skimming at code from high level, so you can already start replying/addressing initial feedback; but will require a more deeper review later anyway

AliSoftware · 2021-06-24T20:31:07Z

  lane :new_beta_release do |options|
    ios_betabuild_prechecks(options)
-    ios_update_metadata(options)
+    download_localized_strings_and_metadata(options)


💯 + 1000000 on misleading toolkit action names, that has been bothering me for a while and has been a big barrier to entry that made me almost lose my mind the first times I had to work with the toolkit 😅

In the long term I'm hoping we could rename them all (probably deprecating the existing ones and creating new ones in their place, taking the occasion to modernize and refactor their code), but that's for another day.

AliSoftware · 2021-06-24T20:31:48Z

+    ios_update_metadata(options)
+    # Jetpack: use the custom setup. Notice that we don't need extra steps to
+    # download `.strings` because Jetpack uses the same codebase as WordPress.
+    download_jetpack_localized_app_store_metadata


For info: this lane is defined in the dedicated and separate Jetpack-Fastfile.

AliSoftware · 2021-06-24T20:37:27Z

+  # bundle exec fastlane update_metadata_on_app_store_connect with_screenshots:true
+  #####################################################################################
+  desc 'Updates the App Store Connect localized metadata'
+  lane :update_metadata_on_app_store_connect do |options|


I agree it can make sense to have a single source of truth. But:

Having that PR ending up doing it one way for Jetpack and another for WordPress actually moves away from that goal, because now we have 3 potential source of truth (Deliver + a bit in Fastfile too for WP, and Jetpack-Fastfile for Jetpack. So personally I would revert the change in that PR to make it only call deliver for now – aligning this with the current setup used for WordPress – and work on this on a separate PR

That separate PR thwt would work on having a unique source of truth might lead to a debate (e.g. given Jetpack-Fastfile is already a separate file, should we put the source of truth in Fastfile, making it not obvious when reading the Jetpack-Fastfile that some constants come from the main Fastfile but are not obivous and visible in the Jetpack-Fastfile? Or given we have 2 fastfiles, should we actually consider the Deliverfile as a good thing to be used as a common source of truth for both?

If we only had one Fastfile I'd agree to get rid of the Deliverfile and use the Fastfile as the single source of truth, but given we already split it the fastfile there… I think we should brainstorm what's best and keep that debate for a separate PR

AliSoftware · 2021-06-24T20:39:04Z

+  with_screenshots = options.fetch(:with_screenshots, false)
+  skip_screenshots = with_screenshots == false
+
+  deliver(


See previous comment, it make sense if we agree to split indeed, but it might be too soon to decide how to split and I'd prefer to migrate to the new way for both WordPress and Jetpack all at once, instead of having a mix of new way for Jetpack, old way for WP.

AliSoftware · 2021-06-24T20:39:24Z

+
+  deliver(
+    app_identifier: APP_IDENTIFIER,
+    app_version: read_version_from_config,


Good point and good idea! 👍

AliSoftware · 2021-06-24T20:42:56Z

+  version = ''
+  # If the file is not available, the method will raise so we should be fine
+  # not handling that case. We'll never return an empty string.
+  File.open(File.join(PROJECT_ROOT_FOLDER, 'Config', 'Version.public.xcconfig')) do |config|
+    configuration = Xcodeproj::Config.new(config)
+    version = configuration.attributes['VERSION_SHORT']
+  end
+  return version


I think that File.open is able to return whatever the block we pass to returns?

Which means we could get rid of the intermediate variable and use this instead:

Suggested change

version = ''

# If the file is not available, the method will raise so we should be fine

# not handling that case. We'll never return an empty string.

File.open(File.join(PROJECT_ROOT_FOLDER, 'Config', 'Version.public.xcconfig')) do |config|

configuration = Xcodeproj::Config.new(config)

version = configuration.attributes['VERSION_SHORT']

end

return version

# If the file is not available, the method will raise so we should be fine

# not handling that case. We'll never return an empty string.

File.open(File.join(PROJECT_ROOT_FOLDER, 'Config', 'Version.public.xcconfig')) do |config|

configuration = Xcodeproj::Config.new(config)

configuration.attributes['VERSION_SHORT']

end

With the return being implicit in ruby (returning the return value of the last statement, here the File.open, which should itself implicitly return the return value of the last statement of its block, here configuration.attributes['VERSION_SHORT'))

Worth double-checking it works tho, since I haven't tested this and only mention this from memory.

Thank you for the suggestion @AliSoftware 🙇

I just tried that for an upcoming PR and it works 👌

AliSoftware · 2021-06-24T20:44:03Z

+struct Config {
+    let baseFolder: String
+    let baseURLString: String
+
+    static let wordPress = Config(
+        baseFolder: "./metadata",
+        baseURLString: "https://translate.wordpress.org/projects/apps/ios/release-notes/"
+    )
+
+    static let jetpack = Config(
+        baseFolder: "./jetpack_metadata",
+        baseURLString: "https://translate.wordpress.com/projects/jetpack/apps/ios/release-notes/"
+    )
+
+    static func config(for argument: String?) -> Config {
+        guard let argument = argument else { return .wordPress }
+        return argument == "jetpack" ?  .jetpack : .wordPress
+    }
+}


I like it (but it might be because I miss Swift too and playing with it 😅 )

AliSoftware · 2021-06-24T20:45:00Z


-func downloadTranslation(languageCode: String, folderName: String) {
+func downloadTranslation(
+    config: Config = .config(for: CommandLine.arguments.second),


That would require us to use a Package.swift to pull the ArgumentParser package first via SwiftPM to be then able to use it, tho 😓

AliSoftware · 2021-06-24T20:50:10Z

+
+extension Array where Element == String {
+
+    var second: String? {
+        guard count >= 2 else { return .none }
+        return self[1]
+    }
+}


nit (I too want to have some Swift fun! 😄 )

Suggested change

extension Array where Element == String {

var second: String? {

guard count >= 2 else { return .none }

return self[1]

}

}

extension Array {

var second: Element? { dropFirst().first }

}

Will use it later to add the Jetpack configuration.

@AliSoftware

Hat tip @AliSoftware See #16733 (comment)

mokagio · 2021-06-28T06:17:10Z

@AliSoftware raised some good points regarding the difference in approaches etc. I'm going to close this one in favor of #16764 for the download part and I'll followup with a different PR or RFC issue regarding the Deliverfile approach. 👍

mokagio · 2021-06-28T06:17:56Z

Please don't delete the branch, I want to keep it around for the other PR and for reference till the work is done.

@AliSoftware

Hat tip @AliSoftware See #16733 (comment)

mokagio · 2021-07-05T01:02:11Z

Followup: the Deliverfile related part of this PR has finally been extracted into #16805

mokagio commented Jun 24, 2021

View reviewed changes

mokagio changed the base branch from develop to release/17.6 June 24, 2021 05:16

mokagio self-assigned this Jun 24, 2021

mokagio added the Tooling Build, Release, and Validation Tools label Jun 24, 2021

mokagio added this to the 17.6 ❄️ milestone Jun 24, 2021

mokagio requested review from AliSoftware, jkmassel and momo-ozawa and removed request for AliSoftware June 24, 2021 05:35

mokagio marked this pull request as ready for review June 24, 2021 05:45

AliSoftware reviewed Jun 24, 2021

View reviewed changes

AliSoftware self-requested a review June 24, 2021 20:53

mokagio mentioned this pull request Jun 25, 2021

Merge finalized release 17.6 into develop #16749

Merged

4 tasks

mokagio force-pushed the upload-localized-jetpack-strings branch 2 times, most recently from 74aa224 to 0b2e571 Compare June 25, 2021 05:15

mokagio added 12 commits June 28, 2021 15:42

Extract Jetpack app identifier in constant

c55fd0f

Add lane to upload metadata to App Store Connect for Jetpack

611887b

Address Swift warning in download_metadata.swift script

0ce3e2e

Define config value type in download_metadata.swift script

7d07a53

Will use it later to add the Jetpack configuration.

Expand download_metadata.swift script to download Jetpack metadata too

a7d3a17

Add Jetpack lane to download localized metadata from App Store Connect

cb00544

Download strings and metadata for both WP and Jetpack in release lanes

26eefbe

Add lane to update ASC metadata with deliver for both WP and Jetpack

2b3f702

Remove Jetpack subtitles to proceed with the metadata upload

959dbc9

Update Jetpack metadata translations

7e1e402

Add Hebrew (he) to the array of metadata translations to download

8fa7f8f

Track Hebrew metadata for Jetpack

8ebdc4e

mokagio force-pushed the upload-localized-jetpack-strings branch from 0b2e571 to 8ebdc4e Compare June 28, 2021 05:44

mokagio changed the base branch from release/17.6 to release/17.7 June 28, 2021 05:52

mokagio added a commit that referenced this pull request Jun 28, 2021

Refine download_metadata.swift Array "second" element getter

ac237df

Hat tip @AliSoftware See #16733 (comment)

mokagio mentioned this pull request Jun 28, 2021

Add lane to download localized metadata for Jetpack #16764

Merged

4 tasks

mokagio closed this Jun 28, 2021

mokagio added a commit that referenced this pull request Jun 30, 2021

Refine download_metadata.swift Array "second" element getter

11d8b5d

Hat tip @AliSoftware See #16733 (comment)

mokagio deleted the upload-localized-jetpack-strings branch July 5, 2021 01:01

		# Reusing the WordPress one here, the values are the same after all
		app_rating_config_path: File.join(PROJECT_ROOT_FOLDER, 'fastlane', 'metadata', 'ratings_config.json')

		guard let index = key.index(of: Character(UnicodeScalar(0004))) else {
		guard let index = key.firstIndex(of: Character(UnicodeScalar(0004))) else {

Conversation

mokagio commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Metadata download

Metadata upload

Regression Notes

Uh oh!

peril-wordpress-mobile Bot commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile Bot commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Jun 28, 2021

Uh oh!

mokagio commented Jun 28, 2021

Uh oh!

mokagio commented Jul 5, 2021

Uh oh!

Reviewers

mokagio commented Jun 24, 2021 •

edited

Loading

peril-wordpress-mobile Bot commented Jun 24, 2021 •

edited

Loading

peril-wordpress-mobile Bot commented Jun 24, 2021 •

edited

Loading

AliSoftware Jun 24, 2021 •

edited

Loading

AliSoftware Jun 24, 2021 •

edited

Loading