From 18fe0be45923c55072eb5592a407293431f2b820 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Mon, 31 Jan 2022 18:46:14 +0100
Subject: [PATCH 01/20] Introduce download_glotpress_export_file in helper

---
 .../helper/ios/ios_l10n_helper.rb             | 31 +++++++++-
 spec/ios_l10n_helper_spec.rb                  | 60 +++++++++++++++++++
 2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
index 3c227300a..0df3b7ea9 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
@@ -1,8 +1,9 @@
-require 'open3'
-require 'tempfile'
 require 'fileutils'
-require 'nokogiri'
 require 'json'
+require 'nokogiri'
+require 'open3'
+require 'open-uri'
+require 'tempfile'
 
 module Fastlane
   module Helper
@@ -116,6 +117,30 @@ def self.generate_strings_file_from_hash(translations:, output_path:)
           end
           File.write(output_path, builder.to_xml)
         end
+
+        # Downloads the export from GlotPress for a given locale and given filters.
+        #
+        # Also post-process the downloaded content to comment out any potential line with an empty (`""`) translation,
+        # to ensure those would fall back to the English/Base locale at runtime instead. These case should theoretically
+        # never happen as GlotPress should not include empty translations in its exports, but we've seen issues in the
+        # past, so better safe than sorry.
+        #
+        # @param [String] project_url The URL to the GlotPress project to export from, e.g. `"https://translate.wordpress.org/projects/apps/ios/dev"`
+        # @param [String] locale The GlotPress locale code to download strings for.
+        # @param [Hash{Symbol=>String}] filters The hash of filters to apply when exporting from GlotPress.
+        #                               Typical examples include `{ status: 'current' }` (the default) or `{ status: 'review' }`.
+        # @param [String, IO] destination The path or `IO`-like instance, where to write the downloaded file on disk.
+        #
+        def self.download_glotpress_export_file(project_url:, locale:, destination:, filters: { status: 'current' })
+          query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings')
+          uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations?#{URI.encode_www_form(query_params)}")
+          begin
+            IO.copy_stream(uri.open, destination)
+          rescue StandardError => e
+            UI.error "Error downloading locale `#{locale}` — #{e.message}"
+            return nil
+          end
+        end
       end
     end
   end
diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb
index 05ca42b71..704de1efa 100644
--- a/spec/ios_l10n_helper_spec.rb
+++ b/spec/ios_l10n_helper_spec.rb
@@ -160,4 +160,64 @@ def file_encoding(path)
       end
     end
   end
+
+  describe '#download_glotpress_export_file' do
+    let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }
+
+    describe 'the request query parameters' do
+      it 'passes the expected params when filters are forced to nil' do
+        # Arrange
+        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: 'content')
+        dest = StringIO.new
+        # Act
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', destination: dest, filters: nil)
+        # Assert
+        expect(stub).to have_been_made.once
+        expect(dest.string).to eq('content')
+      end
+
+      it 'passes the expected params when a list of filters is provided' do
+        # Arrange
+        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings&filters%5Bstatus%5D=current&filters%5Bterm%5D=foobar").to_return(body: 'content')
+        dest = StringIO.new
+        # Act
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', destination: dest, filters: { status: 'current', term: 'foobar' })
+        # Assert
+        expect(stub).to have_been_made.once
+        expect(dest.string).to eq('content')
+      end
+
+      it 'prints UI.error if passed a non-existing locale (or any other 404)' do
+        # Arrange
+        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations?format=strings&filters%5Bstatus%5D=current").to_return(status: [404, 'Not Found'])
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+        dest = StringIO.new
+        # Act
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', destination: dest)
+        # Assert
+        expect(stub).to have_been_made.once
+        expect(error_messages).to eq(['Error downloading locale `invalid` — 404 Not Found'])
+      end
+
+      # it 'accepts an IO (File) as destination' # Not really needed, as all previous tests already cover this by using a `StringIO`
+
+      it 'accepts a String (path) as destination' do
+        Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
+          # Arrange
+          # Note: in practice it seems that GlotPress's `.strings` exports are using UTF8 (but served as `application/octet-stream`)
+          #       but it does not hurt to ensure the download to a file can work with UTF16 ()and copy the binary stream verbatim)
+          body = File.read(fixture('Localizable-utf16.strings'))
+          stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: body)
+          dest = File.join(tmp_dir, 'export.strings')
+          # Act
+          described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', destination: dest, filters: nil)
+          # Assert
+          expect(stub).to have_been_made.once
+          expect(File).to exist(dest)
+          expect(File.read(dest)).to eq(body)
+        end
+      end
+    end
+  end
 end

From 1e462e0cc12aa8bb4c81eaa97b02ab52f7f1b70f Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Mon, 31 Jan 2022 19:01:23 +0100
Subject: [PATCH 02/20] Remove default value from filters param

Makes more sense to have that default value hosted on the fastlane action rather than the helper.
---
 .../wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb  |  2 +-
 spec/ios_l10n_helper_spec.rb                         | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
index 0df3b7ea9..7cc2f47c5 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
@@ -131,7 +131,7 @@ def self.generate_strings_file_from_hash(translations:, output_path:)
         #                               Typical examples include `{ status: 'current' }` (the default) or `{ status: 'review' }`.
         # @param [String, IO] destination The path or `IO`-like instance, where to write the downloaded file on disk.
         #
-        def self.download_glotpress_export_file(project_url:, locale:, destination:, filters: { status: 'current' })
+        def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:)
           query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings')
           uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations?#{URI.encode_www_form(query_params)}")
           begin
diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb
index 704de1efa..e71e42ab7 100644
--- a/spec/ios_l10n_helper_spec.rb
+++ b/spec/ios_l10n_helper_spec.rb
@@ -165,12 +165,12 @@ def file_encoding(path)
     let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }
 
     describe 'the request query parameters' do
-      it 'passes the expected params when filters are forced to nil' do
+      it 'passes the expected params when no filters are provided' do
         # Arrange
         stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: 'content')
         dest = StringIO.new
         # Act
-        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', destination: dest, filters: nil)
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
         # Assert
         expect(stub).to have_been_made.once
         expect(dest.string).to eq('content')
@@ -181,7 +181,7 @@ def file_encoding(path)
         stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings&filters%5Bstatus%5D=current&filters%5Bterm%5D=foobar").to_return(body: 'content')
         dest = StringIO.new
         # Act
-        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', destination: dest, filters: { status: 'current', term: 'foobar' })
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: { status: 'current', term: 'foobar' }, destination: dest)
         # Assert
         expect(stub).to have_been_made.once
         expect(dest.string).to eq('content')
@@ -189,12 +189,12 @@ def file_encoding(path)
 
       it 'prints UI.error if passed a non-existing locale (or any other 404)' do
         # Arrange
-        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations?format=strings&filters%5Bstatus%5D=current").to_return(status: [404, 'Not Found'])
+        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations?format=strings").to_return(status: [404, 'Not Found'])
         error_messages = []
         allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
         dest = StringIO.new
         # Act
-        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', destination: dest)
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest)
         # Assert
         expect(stub).to have_been_made.once
         expect(error_messages).to eq(['Error downloading locale `invalid` — 404 Not Found'])
@@ -211,7 +211,7 @@ def file_encoding(path)
           stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: body)
           dest = File.join(tmp_dir, 'export.strings')
           # Act
-          described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', destination: dest, filters: nil)
+          described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
           # Assert
           expect(stub).to have_been_made.once
           expect(File).to exist(dest)

From ca0a2f57f0b9ffaed3f7a099e7960e543c0625e1 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Mon, 31 Jan 2022 20:23:47 +0100
Subject: [PATCH 03/20] Rearrange specs to use proper contexts hierarchy

---
 spec/ios_l10n_helper_spec.rb | 50 ++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb
index e71e42ab7..4ae199a33 100644
--- a/spec/ios_l10n_helper_spec.rb
+++ b/spec/ios_l10n_helper_spec.rb
@@ -164,7 +164,7 @@ def file_encoding(path)
   describe '#download_glotpress_export_file' do
     let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }
 
-    describe 'the request query parameters' do
+    describe 'request query parameters' do
       it 'passes the expected params when no filters are provided' do
         # Arrange
         stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: 'content')
@@ -186,23 +186,12 @@ def file_encoding(path)
         expect(stub).to have_been_made.once
         expect(dest.string).to eq('content')
       end
+    end
 
-      it 'prints UI.error if passed a non-existing locale (or any other 404)' do
-        # Arrange
-        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations?format=strings").to_return(status: [404, 'Not Found'])
-        error_messages = []
-        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
-        dest = StringIO.new
-        # Act
-        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest)
-        # Assert
-        expect(stub).to have_been_made.once
-        expect(error_messages).to eq(['Error downloading locale `invalid` — 404 Not Found'])
-      end
-
-      # it 'accepts an IO (File) as destination' # Not really needed, as all previous tests already cover this by using a `StringIO`
+    describe 'destination parameter' do
+      # it 'accepts an IO (File)' # Not really needed, as all previous tests already cover this by using a `StringIO`
 
-      it 'accepts a String (path) as destination' do
+      it 'accepts a String (path)' do
         Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
           # Arrange
           # Note: in practice it seems that GlotPress's `.strings` exports are using UTF8 (but served as `application/octet-stream`)
@@ -219,5 +208,34 @@ def file_encoding(path)
         end
       end
     end
+
+    describe 'invalid parameters' do
+      it 'prints an `UI.error` if passed a non-existing locale (or any other 404)' do
+        # Arrange
+        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations?format=strings").to_return(status: [404, 'Not Found'])
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+        dest = StringIO.new
+        # Act
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest)
+        # Assert
+        expect(stub).to have_been_made.once
+        expect(error_messages).to eq(['Error downloading locale `invalid` — 404 Not Found'])
+      end
+
+      it 'prints an `UI.error` if the destination cannot be written to' do
+        # Arrange
+        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: 'content')
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+        dest = '/these/are/not/the/droids/you/are/looking/for.strings'
+        # Act
+        described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
+        # Assert
+        expect(stub).to have_been_made.once
+        expect(File).not_to exist(dest)
+        expect(error_messages).to eq(["Error downloading locale `fr` — No such file or directory @ rb_sysopen - #{dest}"])
+      end
+    end
   end
 end

From e3a2cb9fd2377f6460e2979812ec17acfa05026b Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Mon, 31 Jan 2022 20:27:10 +0100
Subject: [PATCH 04/20] Introduce ios_download_strings_files_from_glotpress
 action

+ pending test cases
---
 .rubocop_todo.yml                             |   1 +
 ...s_download_strings_files_from_glotpress.rb | 105 ++++++++++++++++++
 ...nload_strings_files_from_glotpress_spec.rb |  21 ++++
 3 files changed, 127 insertions(+)
 create mode 100644 lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
 create mode 100644 spec/ios_download_strings_files_from_glotpress_spec.rb

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 89f5659c1..7f86b6726 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -146,6 +146,7 @@ RSpec/FilePath:
     - 'spec/ios_l10n_helper_spec.rb'
     - 'spec/ios_merge_strings_files_spec.rb'
     - 'spec/gp_update_metadata_source_spec.rb'
+    - 'spec/ios_download_strings_files_from_glotpress_spec.rb'
 
 # Offense count: 8
 # Cop supports --auto-correct.
diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
new file mode 100644
index 000000000..ac03128d4
--- /dev/null
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -0,0 +1,105 @@
+module Fastlane
+  module Actions
+    class IosDownloadStringsFilesFromGlotPressAction < Action
+      def self.run(params)
+        # TODO: Once we introduce the `Locale` POD via #296, check if the param is an array of locales and if so convert it to Hash{glotpress=>lproj}
+        locales = params[:locales]
+
+        locales.each do |glotpress_locale, lproj_name|
+          # Download the export in the proper `.lproj` directory
+          destination = File.join(params[:download_dir], "#{lproj_name}.lproj", "#{params[:table_basename]}.strings")
+          Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file(
+            project_url: params[:project_url],
+            locale: glotpress_locale,
+            filters: params[:filters],
+            destination: destination
+          )
+          # Do a quick check of the downloaded `.strings` file to ensure it looks valid
+          validate_strings_file(destination) unless params[:skip_file_validation]
+        end
+      end
+
+      # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations
+      def self.validate_strings_file(destination)
+        translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(destination)
+        empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys
+        unless empty_keys.empty?
+          UI.error(
+            "Found empty translations in `#{destination}` for the following keys: #{empty_keys}.\n" \
+              + "This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI.\n" \
+              + 'Please report this to the GlotPress team, and fix the file locally this continuing.'
+          )
+        end
+      rescue StandardError => e
+        UI.error("Error while validating the file exported from GlotPress (`#{destination}`) - #{e.message}")
+      end
+
+      #####################################################
+      # @!group Documentation
+      #####################################################
+
+      def self.description
+        'Downloads the `.strings` files from GlotPress for the various locales'
+      end
+
+      def self.details
+        <<~DETAILS
+          Downloads the `.strings` files from GlotPress for the various locales,
+          validates them, and saves them in the relevant `*.lproj` directories for each locale
+        DETAILS
+      end
+
+      def self.available_options
+        [
+          FastlaneCore::ConfigItem.new(key: :project_url,
+                                       env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_PROJECT_URL',
+                                       description: 'URL to the GlotPress project',
+                                       type: String),
+          FastlaneCore::ConfigItem.new(key: :locales,
+                                       env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_LOCALES',
+                                       description: 'The map of locales to download, each entry of the Hash corresponding to a { glotpress-locale-code => lproj-folder-basename } pair',
+                                       type: Hash), # TODO: also support an Array of `Locale` POD/struct type when we introduce it later (see #296)
+          FastlaneCore::ConfigItem.new(key: :download_dir,
+                                       env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_DOWNLOAD_DIR',
+                                       description: 'The parent directory containing all the `*.lproj` subdirectories in which the downloaded files will be saved',
+                                       type: Hash),
+          FastlaneCore::ConfigItem.new(key: :table_basename,
+                                       env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_TABLE_BASENAME',
+                                       description: 'The basename to save the `.strings` files under',
+                                       type: String,
+                                       optional: true,
+                                       default_value: 'Localizable'),
+          FastlaneCore::ConfigItem.new(key: :filters,
+                                       env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_FILTERS',
+                                       description: 'The GlotPress filters to use when requesting the translations export',
+                                       type: Hash,
+                                       optional: true,
+                                       default_value: { status: 'current' }),
+          FastlaneCore::ConfigItem.new(key: :skip_file_validation,
+                                       env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_SKIP_FILE_VALIDATION',
+                                       description: 'If true, skips the validation of `.strings` files after download',
+                                       type: Fastlane::Boolean,
+                                       optional: true,
+                                       default_value: false),
+        ]
+      end
+
+      def self.return_type
+        # Describes what type of data is expected to be returned
+        # see RETURN_TYPES in https://github.com/fastlane/fastlane/blob/master/fastlane/lib/fastlane/action.rb
+      end
+
+      def self.return_value
+        # Textual description of what the return value is
+      end
+
+      def self.authors
+        ['Automattic']
+      end
+
+      def self.is_supported?(platform)
+        [:ios, :mac].include?(platform)
+      end
+    end
+  end
+end
diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
new file mode 100644
index 000000000..45de015f9
--- /dev/null
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+require 'tmpdir'
+
+Locale = Struct.new(:name)
+
+describe Fastlane::Actions::IosDownloadStringsFilesFromGlotPressAction do
+  let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_generate_strings_file_from_code') }
+
+  describe 'downloading export files from GlotPress' do
+    it 'downloads all the locales into the expected directories'
+    it 'uses the proper filters when exporting the files from GlotPress'
+    it 'uses a custom table name for the `.strings` files if provided'
+  end
+
+  describe 'error handling' do
+    it 'shows an error if an invalid locale is provided (404)'
+    it 'shows an error if the file cannot be written in the destination'
+    it 'reports if a downloaded file is invalid by default'
+    it 'does not report invalid downloaded files if skip_file_validation:true'
+  end
+end

From 4b2fa325a249b998fe9c4f1a4119ae95f2ddf723 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Mon, 31 Jan 2022 20:34:11 +0100
Subject: [PATCH 05/20] Fix action naming as per fastlane's conventions

As we don't want the word `GlotPress` to be split and turned into `glot_press` in the action name when called from the Fastfile.
---
 .../actions/ios/ios_download_strings_files_from_glotpress.rb    | 2 +-
 spec/ios_download_strings_files_from_glotpress_spec.rb          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index ac03128d4..69a1f2d83 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -1,6 +1,6 @@
 module Fastlane
   module Actions
-    class IosDownloadStringsFilesFromGlotPressAction < Action
+    class IosDownloadStringsFilesFromGlotpressAction < Action
       def self.run(params)
         # TODO: Once we introduce the `Locale` POD via #296, check if the param is an array of locales and if so convert it to Hash{glotpress=>lproj}
         locales = params[:locales]
diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
index 45de015f9..f4b5cdcc1 100644
--- a/spec/ios_download_strings_files_from_glotpress_spec.rb
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -3,7 +3,7 @@
 
 Locale = Struct.new(:name)
 
-describe Fastlane::Actions::IosDownloadStringsFilesFromGlotPressAction do
+describe Fastlane::Actions::IosDownloadStringsFilesFromGlotpressAction do
   let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_generate_strings_file_from_code') }
 
   describe 'downloading export files from GlotPress' do

From 8cc44384d0dddf31b37c4505f9ea5ba81a36ebc8 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Tue, 1 Feb 2022 14:26:09 +0100
Subject: [PATCH 06/20] Fix typos

Thanks @mokagio for the sharp eye

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
---
 .../actions/ios/ios_download_strings_files_from_glotpress.rb  | 2 +-
 spec/ios_l10n_helper_spec.rb                                  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index 69a1f2d83..0ae20ee84 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -27,7 +27,7 @@ def self.validate_strings_file(destination)
           UI.error(
             "Found empty translations in `#{destination}` for the following keys: #{empty_keys}.\n" \
               + "This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI.\n" \
-              + 'Please report this to the GlotPress team, and fix the file locally this continuing.'
+              + 'Please report this to the GlotPress team, and fix the file locally before continuing.'
           )
         end
       rescue StandardError => e
diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb
index 4ae199a33..386b5d901 100644
--- a/spec/ios_l10n_helper_spec.rb
+++ b/spec/ios_l10n_helper_spec.rb
@@ -194,8 +194,8 @@ def file_encoding(path)
       it 'accepts a String (path)' do
         Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
           # Arrange
-          # Note: in practice it seems that GlotPress's `.strings` exports are using UTF8 (but served as `application/octet-stream`)
-          #       but it does not hurt to ensure the download to a file can work with UTF16 ()and copy the binary stream verbatim)
+          # Note: in practice it seems that GlotPress's `.strings` exports are using UTF-8 (but served as `application/octet-stream`)
+          #       but it does not hurt to ensure the download to a file can work with UTF-16 ()and copy the binary stream verbatim)
           body = File.read(fixture('Localizable-utf16.strings'))
           stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: body)
           dest = File.join(tmp_dir, 'export.strings')

From e9cd88a9b1dc029648187863c5d237416363113e Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Tue, 1 Feb 2022 14:28:57 +0100
Subject: [PATCH 07/20] Fix typo again

---
 spec/ios_l10n_helper_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb
index 386b5d901..2976f27a8 100644
--- a/spec/ios_l10n_helper_spec.rb
+++ b/spec/ios_l10n_helper_spec.rb
@@ -195,7 +195,7 @@ def file_encoding(path)
         Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
           # Arrange
           # Note: in practice it seems that GlotPress's `.strings` exports are using UTF-8 (but served as `application/octet-stream`)
-          #       but it does not hurt to ensure the download to a file can work with UTF-16 ()and copy the binary stream verbatim)
+          #       but it does not hurt to ensure the download to a file can work with UTF-16 (and copy the binary stream verbatim)
           body = File.read(fixture('Localizable-utf16.strings'))
           stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: body)
           dest = File.join(tmp_dir, 'export.strings')

From 832343b69018b9a0c260740a8581db80a42f4f94 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 15:50:23 +0100
Subject: [PATCH 08/20] Use `.with(query:)` on `stub_request` calls

h/t @mokagio
---
 spec/ios_l10n_helper_spec.rb | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb
index 2976f27a8..8abc3e37f 100644
--- a/spec/ios_l10n_helper_spec.rb
+++ b/spec/ios_l10n_helper_spec.rb
@@ -167,7 +167,7 @@ def file_encoding(path)
     describe 'request query parameters' do
       it 'passes the expected params when no filters are provided' do
         # Arrange
-        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: 'content')
+        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations").with(query: { format: 'strings' }).to_return(body: 'content')
         dest = StringIO.new
         # Act
         described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
@@ -178,7 +178,8 @@ def file_encoding(path)
 
       it 'passes the expected params when a list of filters is provided' do
         # Arrange
-        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings&filters%5Bstatus%5D=current&filters%5Bterm%5D=foobar").to_return(body: 'content')
+        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations")
+               .with(query: { format: 'strings', 'filters[status]': 'current', 'filters[term]': 'foobar' }).to_return(body: 'content')
         dest = StringIO.new
         # Act
         described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: { status: 'current', term: 'foobar' }, destination: dest)
@@ -197,7 +198,7 @@ def file_encoding(path)
           # Note: in practice it seems that GlotPress's `.strings` exports are using UTF-8 (but served as `application/octet-stream`)
           #       but it does not hurt to ensure the download to a file can work with UTF-16 (and copy the binary stream verbatim)
           body = File.read(fixture('Localizable-utf16.strings'))
-          stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: body)
+          stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations").with(query: { format: 'strings' }).to_return(body: body)
           dest = File.join(tmp_dir, 'export.strings')
           # Act
           described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
@@ -212,7 +213,7 @@ def file_encoding(path)
     describe 'invalid parameters' do
       it 'prints an `UI.error` if passed a non-existing locale (or any other 404)' do
         # Arrange
-        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations?format=strings").to_return(status: [404, 'Not Found'])
+        stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations").with(query: { format: 'strings' }).to_return(status: [404, 'Not Found'])
         error_messages = []
         allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
         dest = StringIO.new
@@ -225,7 +226,7 @@ def file_encoding(path)
 
       it 'prints an `UI.error` if the destination cannot be written to' do
         # Arrange
-        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations?format=strings").to_return(body: 'content')
+        stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations").with(query: { format: 'strings' }).to_return(body: 'content')
         error_messages = []
         allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
         dest = '/these/are/not/the/droids/you/are/looking/for.strings'

From e89fe3afccea46e68207ddb461bb6c162eda4bfe Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 19:04:01 +0100
Subject: [PATCH 09/20] Create `.lproj` subdirectory if it does not exist yet

---
 .../actions/ios/ios_download_strings_files_from_glotpress.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index 0ae20ee84..70012f54e 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -7,7 +7,10 @@ def self.run(params)
 
         locales.each do |glotpress_locale, lproj_name|
           # Download the export in the proper `.lproj` directory
-          destination = File.join(params[:download_dir], "#{lproj_name}.lproj", "#{params[:table_basename]}.strings")
+          lproj_dir = File.join(params[:download_dir], "#{lproj_name}.lproj")
+          destination = File.join(lproj_dir, "#{params[:table_basename]}.strings")
+          FileUtils.mkdir(lproj_dir) unless Dir.exist?(lproj_dir)
+
           Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file(
             project_url: params[:project_url],
             locale: glotpress_locale,

From dfe96a78af7229b9e31c3e10acaa9318a1a5df46 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 19:04:26 +0100
Subject: [PATCH 10/20] Fix invalid ConfigItem type

---
 .../actions/ios/ios_download_strings_files_from_glotpress.rb    | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index 70012f54e..fd0e4ca19 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -65,7 +65,7 @@ def self.available_options
           FastlaneCore::ConfigItem.new(key: :download_dir,
                                        env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_DOWNLOAD_DIR',
                                        description: 'The parent directory containing all the `*.lproj` subdirectories in which the downloaded files will be saved',
-                                       type: Hash),
+                                       type: String),
           FastlaneCore::ConfigItem.new(key: :table_basename,
                                        env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_TABLE_BASENAME',
                                        description: 'The basename to save the `.strings` files under',

From 41225bff34c4c07d609941902dc963a35eeca3a9 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 19:04:44 +0100
Subject: [PATCH 11/20] Implement happy path test cases

---
 ...nload_strings_files_from_glotpress_spec.rb | 59 +++++++++++++++++--
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
index f4b5cdcc1..084683199 100644
--- a/spec/ios_download_strings_files_from_glotpress_spec.rb
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -1,15 +1,64 @@
 require 'spec_helper'
 require 'tmpdir'
 
-Locale = Struct.new(:name)
-
 describe Fastlane::Actions::IosDownloadStringsFilesFromGlotpressAction do
   let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_generate_strings_file_from_code') }
 
   describe 'downloading export files from GlotPress' do
-    it 'downloads all the locales into the expected directories'
-    it 'uses the proper filters when exporting the files from GlotPress'
-    it 'uses a custom table name for the `.strings` files if provided'
+    let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }
+    let(:locales_subset) { {'fr-FR': 'fr', 'zh-cn': 'zh-Hans' } }
+
+    def gp_stub(locale:, query:)
+      stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations").with(query: query)
+    end
+
+    def test_gp_download(filters:, tablename:, expected_gp_params:)
+      Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
+        # Arrange
+        stub_fr = gp_stub(locale: 'fr-FR', query: expected_gp_params).to_return(body: '"key" = "fr-copy";')
+        stub_zh = gp_stub(locale: 'zh-cn', query: expected_gp_params).to_return(body: '"key" = "zh-copy";')
+
+        # Act
+        run_described_fastlane_action({
+          project_url: gp_fake_url,
+          locales: locales_subset,
+          download_dir: tmp_dir,
+          table_basename: tablename,
+          filters: filters
+        }.compact)
+
+        # Assert
+        expect(stub_fr).to have_been_made.once
+        file_fr = File.join(tmp_dir, 'fr.lproj', "#{tablename || 'Localizable'}.strings")
+        expect(File).to exist(file_fr)
+        expect(File.read(file_fr)).to eq('"key" = "fr-copy";')
+
+        expect(stub_zh).to have_been_made.once
+        file_zh = File.join(tmp_dir, 'zh-Hans.lproj', "#{tablename || 'Localizable'}.strings")
+        expect(File).to exist(file_zh)
+        expect(File.read(file_zh)).to eq('"key" = "zh-copy";')
+      end
+    end
+
+    it 'downloads all the locales into the expected directories' do
+      test_gp_download(filters: nil, tablename: nil, expected_gp_params: { 'filters[status]': 'current', format: 'strings' })
+    end
+
+    it 'uses the proper filters when exporting the files from GlotPress' do
+      test_gp_download(
+        filters: { term: 'foo', status: 'review' },
+        tablename: nil,
+        expected_gp_params: { 'filters[term]': 'foo', 'filters[status]': 'review', format: 'strings' }
+      )
+    end
+
+    it 'uses a custom table name for the `.strings` files if provided' do
+      test_gp_download(
+        filters: nil,
+        tablename: "MyApp",
+        expected_gp_params: { 'filters[status]': 'current', format: 'strings' }
+      )
+    end
   end
 
   describe 'error handling' do

From c03b18dd916982d36e5af829fc8c0e068fafe46b Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 19:51:01 +0100
Subject: [PATCH 12/20] Do not report "file not found" during validation if we
 had (and already reported) a download failure (e.g. 404)

---
 .../actions/ios/ios_download_strings_files_from_glotpress.rb  | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index fd0e4ca19..37f372022 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -24,7 +24,9 @@ def self.run(params)
 
       # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations
       def self.validate_strings_file(destination)
-        translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(destination)
+        return unless File.exist?(destination) # If the file failed to download, don't try to validate an non-existing file. We'd already have a separate error for the download failure anyway.
+
+        translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: destination)
         empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys
         unless empty_keys.empty?
           UI.error(

From 3754e1e22c1c82ca2297928c913e721cd64232f3 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 21:16:15 +0100
Subject: [PATCH 13/20] Report error early if :download_dir does not exist

---
 .../actions/ios/ios_download_strings_files_from_glotpress.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index 37f372022..47cdb10ab 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -4,10 +4,13 @@ class IosDownloadStringsFilesFromGlotpressAction < Action
       def self.run(params)
         # TODO: Once we introduce the `Locale` POD via #296, check if the param is an array of locales and if so convert it to Hash{glotpress=>lproj}
         locales = params[:locales]
+        download_dir = params[:download_dir]
+
+        UI.user_error!("The parent directory `#{download_dir}` (which contains all the `*.lproj` subdirectories) must already exist") unless Dir.exist?(download_dir)
 
         locales.each do |glotpress_locale, lproj_name|
           # Download the export in the proper `.lproj` directory
-          lproj_dir = File.join(params[:download_dir], "#{lproj_name}.lproj")
+          lproj_dir = File.join(download_dir, "#{lproj_name}.lproj")
           destination = File.join(lproj_dir, "#{params[:table_basename]}.strings")
           FileUtils.mkdir(lproj_dir) unless Dir.exist?(lproj_dir)
 

From b249b55866a369f13639852f6703c213a9bf50df Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 21:16:30 +0100
Subject: [PATCH 14/20] Sort empty keys in error message

---
 .../ios/ios_download_strings_files_from_glotpress.rb        | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
index 47cdb10ab..4adfcbe34 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb
@@ -30,16 +30,16 @@ def self.validate_strings_file(destination)
         return unless File.exist?(destination) # If the file failed to download, don't try to validate an non-existing file. We'd already have a separate error for the download failure anyway.
 
         translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: destination)
-        empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys
+        empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort
         unless empty_keys.empty?
           UI.error(
-            "Found empty translations in `#{destination}` for the following keys: #{empty_keys}.\n" \
+            "Found empty translations in `#{destination}` for the following keys: #{empty_keys.inspect}.\n" \
               + "This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI.\n" \
               + 'Please report this to the GlotPress team, and fix the file locally before continuing.'
           )
         end
       rescue StandardError => e
-        UI.error("Error while validating the file exported from GlotPress (`#{destination}`) - #{e.message}")
+        UI.error("Error while validating the file exported from GlotPress (`#{destination}`) - #{e.message.chomp}")
       end
 
       #####################################################

From 7e1dbeaa3e5241dac455a8c396a17d04a190ec04 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 21:16:50 +0100
Subject: [PATCH 15/20] Add remaining test cases

---
 ...nload_strings_files_from_glotpress_spec.rb | 134 ++++++++++++++++--
 1 file changed, 122 insertions(+), 12 deletions(-)

diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
index 084683199..510246631 100644
--- a/spec/ios_download_strings_files_from_glotpress_spec.rb
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -3,15 +3,14 @@
 
 describe Fastlane::Actions::IosDownloadStringsFilesFromGlotpressAction do
   let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_generate_strings_file_from_code') }
+  let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }
+  let(:locales_subset) { { 'fr-FR': 'fr', 'zh-cn': 'zh-Hans' } }
 
-  describe 'downloading export files from GlotPress' do
-    let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }
-    let(:locales_subset) { {'fr-FR': 'fr', 'zh-cn': 'zh-Hans' } }
-
-    def gp_stub(locale:, query:)
-      stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations").with(query: query)
-    end
+  def gp_stub(locale:, query:)
+    stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations").with(query: query)
+  end
 
+  describe 'downloading export files from GlotPress' do
     def test_gp_download(filters:, tablename:, expected_gp_params:)
       Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
         # Arrange
@@ -55,16 +54,127 @@ def test_gp_download(filters:, tablename:, expected_gp_params:)
     it 'uses a custom table name for the `.strings` files if provided' do
       test_gp_download(
         filters: nil,
-        tablename: "MyApp",
+        tablename: 'MyApp',
         expected_gp_params: { 'filters[status]': 'current', format: 'strings' }
       )
     end
   end
 
   describe 'error handling' do
-    it 'shows an error if an invalid locale is provided (404)'
-    it 'shows an error if the file cannot be written in the destination'
-    it 'reports if a downloaded file is invalid by default'
-    it 'does not report invalid downloaded files if skip_file_validation:true'
+    it 'shows an error if an invalid locale is provided (404)' do
+      Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
+        # Arrange
+        stub = gp_stub(locale: 'unknown-locale', query: { 'filters[status]': 'current', format: 'strings' }).to_return(status: [404, 'Not Found'])
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+
+        # Act
+        run_described_fastlane_action(
+          project_url: gp_fake_url,
+          locales: { 'unknown-locale': 'Base' },
+          download_dir: tmp_dir
+        )
+
+        # Assert
+        expect(stub).to have_been_made.once
+        file = File.join(tmp_dir, 'Base.lproj', 'Localizable.strings')
+        expect(File).not_to exist(file)
+        expect(error_messages).to eq(['Error downloading locale `unknown-locale` — 404 Not Found'])
+      end
+    end
+
+    it 'shows an error if the file cannot be written in the destination' do
+      # Arrange
+      download_dir = '/these/are/not/the/dirs/you/are/looking/for/'
+
+      # Act
+      act = lambda do
+        run_described_fastlane_action(
+          project_url: gp_fake_url,
+          locales: { 'fr-FR': 'fr' },
+          download_dir: download_dir
+        )
+      end
+
+      # Assert
+      expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, "The parent directory `#{download_dir}` (which contains all the `*.lproj` subdirectories) must already exist")
+    end
+
+    it 'reports if a downloaded file is not a valid `.strings` file' do
+      Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
+        # Arrange
+        stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: 'some invalid strings file content')
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+
+        # Act
+        run_described_fastlane_action(
+          project_url: gp_fake_url,
+          locales: { 'fr-FR': 'fr' },
+          download_dir: tmp_dir
+        )
+
+        # Assert
+        expect(stub).to have_been_made.once
+        file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings')
+        expect(File).to exist(file)
+        expected_error = 'Property List error: Unexpected character s at line 1 / JSON error: JSON text did not start with array or object and option to allow fragments not set. around line 1, column 0.'
+        expect(error_messages).to eq(["Error while validating the file exported from GlotPress (`#{file}`) - #{file}: #{expected_error}"])
+      end
+    end
+
+    it 'reports if a downloaded file has empty translations' do
+      Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
+        # Arrange
+        stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' })
+               .to_return(body: ['"key1" = "value1";', '"key2" = "";', '"key3" = "";', '/* translators: use "" quotes please */', '"key4" = "value4";'].join("\n"))
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+
+        # Act
+        run_described_fastlane_action(
+          project_url: gp_fake_url,
+          locales: { 'fr-FR': 'fr' },
+          download_dir: tmp_dir
+        )
+
+        # Assert
+        expect(stub).to have_been_made.once
+        file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings')
+        expect(File).to exist(file)
+        expected_error = <<~MSG.chomp
+          Found empty translations in `#{file}` for the following keys: ["key2", "key3"].
+          This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI.
+          Please report this to the GlotPress team, and fix the file locally before continuing.
+        MSG
+        expect(error_messages).to eq([expected_error])
+      end
+    end
+
+    it 'does not report invalid downloaded files if `skip_file_validation:true`' do
+      Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir|
+        # Arrange
+        stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: 'some invalid strings file content')
+        error_messages = []
+        allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
+
+        # Act
+        act = lambda do
+          run_described_fastlane_action(
+            project_url: gp_fake_url,
+            locales: { 'fr-FR': 'fr' },
+            download_dir: tmp_dir,
+            skip_file_validation: true
+          )
+        end
+
+        # Assert
+        expect { act.call }.not_to raise_error
+        expect(stub).to have_been_made.once
+        file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings')
+        expect(File).to exist(file)
+        expect(error_messages).to eq([])
+      end
+    end
   end
 end

From 9db104c4fa6cc2aaa94ad7532ec59bd6c44bed44 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 21:39:44 +0100
Subject: [PATCH 16/20] Account for variations of error message

Depending on the version of plutil installed on the machine, apparently
---
 spec/ios_download_strings_files_from_glotpress_spec.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
index 510246631..9720a5308 100644
--- a/spec/ios_download_strings_files_from_glotpress_spec.rb
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -118,8 +118,9 @@ def test_gp_download(filters:, tablename:, expected_gp_params:)
         expect(stub).to have_been_made.once
         file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings')
         expect(File).to exist(file)
-        expected_error = 'Property List error: Unexpected character s at line 1 / JSON error: JSON text did not start with array or object and option to allow fragments not set. around line 1, column 0.'
-        expect(error_messages).to eq(["Error while validating the file exported from GlotPress (`#{file}`) - #{file}: #{expected_error}"])
+        expected_error = 'Property List error: Unexpected character s at line 1 / JSON error: JSON text did not start with array or object and option to allow fragments not set.'
+        expect(error_messages.count).to eq(1)
+        expect(error_messages.first).to start_with("Error while validating the file exported from GlotPress (`#{file}`) - #{file}: #{expected_error}") # Different versions of `plutil` might append the line/column as well, but not all.
       end
     end
 

From 23d43b1040dad6ddf0cec4f95912564fac4428a0 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Wed, 2 Feb 2022 21:47:53 +0100
Subject: [PATCH 17/20] Add CHANGELOG entry

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1130f50ef..f9dd7d07a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
 ### New Features
 
 * Introduce new `ios_merge_strings_files` action. [#329]
+* Introduce new `ios_download_strings_files_from_glotpress` action. [#331]
 
 ### Bug Fixes
 

From fd678842e1ef8a6fe3ffbd33bd403f1354b420e5 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Thu, 3 Feb 2022 15:28:40 +0100
Subject: [PATCH 18/20] Remove unnecessary intermediate variable

H/t @mokagio in https://github.com/wordpress-mobile/release-toolkit/pull/331#discussion_r798413100
---
 spec/ios_download_strings_files_from_glotpress_spec.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
index 9720a5308..54848ac2b 100644
--- a/spec/ios_download_strings_files_from_glotpress_spec.rb
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -77,8 +77,7 @@ def test_gp_download(filters:, tablename:, expected_gp_params:)
 
         # Assert
         expect(stub).to have_been_made.once
-        file = File.join(tmp_dir, 'Base.lproj', 'Localizable.strings')
-        expect(File).not_to exist(file)
+        expect(File).not_to exist(File.join(tmp_dir, 'Base.lproj', 'Localizable.strings'))
         expect(error_messages).to eq(['Error downloading locale `unknown-locale` — 404 Not Found'])
       end
     end

From ba0fdc6d9ee7671b3888f4c838c323644e11caca Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Thu, 3 Feb 2022 15:29:27 +0100
Subject: [PATCH 19/20] Add comments to provide additional explanations

h/t @mokagio in https://github.com/wordpress-mobile/release-toolkit/pull/331#discussion_r798417069
---
 spec/ios_download_strings_files_from_glotpress_spec.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb
index 54848ac2b..45fb23749 100644
--- a/spec/ios_download_strings_files_from_glotpress_spec.rb
+++ b/spec/ios_download_strings_files_from_glotpress_spec.rb
@@ -18,6 +18,9 @@ def test_gp_download(filters:, tablename:, expected_gp_params:)
         stub_zh = gp_stub(locale: 'zh-cn', query: expected_gp_params).to_return(body: '"key" = "zh-copy";')
 
         # Act
+        # Note: The use of `.compact` here allows us to remove the keys (like `table_basename` and `filters`) from the `Hash` whose values are `nil`,
+        # so that we don't include those parameters at all in the action's call site -- making them use the default value from their respective
+        # `ConfigItem` (as opposed to passing a value of `nil` explicitly and overwrite the default value, which is not what we want to test).
         run_described_fastlane_action({
           project_url: gp_fake_url,
           locales: locales_subset,
@@ -96,6 +99,7 @@ def test_gp_download(filters:, tablename:, expected_gp_params:)
       end
 
       # Assert
+      # Note: FastlaneError is the exception raised by UI.user_error!
       expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, "The parent directory `#{download_dir}` (which contains all the `*.lproj` subdirectories) must already exist")
     end
 

From e918ed0c12b3e41b9b868d9c4d41d821e5a32f78 Mon Sep 17 00:00:00 2001
From: Olivier Halligon <olivier.halligon@automattic.com>
Date: Thu, 3 Feb 2022 15:46:04 +0100
Subject: [PATCH 20/20] Fix some leftover legacy notes from the YARD doc

---
 .../plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
index 7cc2f47c5..51b6a1252 100644
--- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
+++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb
@@ -120,15 +120,10 @@ def self.generate_strings_file_from_hash(translations:, output_path:)
 
         # Downloads the export from GlotPress for a given locale and given filters.
         #
-        # Also post-process the downloaded content to comment out any potential line with an empty (`""`) translation,
-        # to ensure those would fall back to the English/Base locale at runtime instead. These case should theoretically
-        # never happen as GlotPress should not include empty translations in its exports, but we've seen issues in the
-        # past, so better safe than sorry.
-        #
         # @param [String] project_url The URL to the GlotPress project to export from, e.g. `"https://translate.wordpress.org/projects/apps/ios/dev"`
         # @param [String] locale The GlotPress locale code to download strings for.
         # @param [Hash{Symbol=>String}] filters The hash of filters to apply when exporting from GlotPress.
-        #                               Typical examples include `{ status: 'current' }` (the default) or `{ status: 'review' }`.
+        #                               Typical examples include `{ status: 'current' }` or `{ status: 'review' }`.
         # @param [String, IO] destination The path or `IO`-like instance, where to write the downloaded file on disk.
         #
         def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:)