From cdfdeaf31ebddae338d6912e573e0165148c757a Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 19:55:45 +0200 Subject: [PATCH 01/10] Fail CI on translation download errors --- CHANGELOG.md | 2 +- .../helper/android/android_localize_helper.rb | 2 +- .../helper/glotpress_downloader.rb | 59 +++++++++---------- .../helper/ios/ios_l10n_helper.rb | 5 +- .../helper/metadata_download_helper.rb | 8 +-- spec/android_localize_helper_spec.rb | 23 ++++++++ spec/glotpress_downloader_spec.rb | 48 +++++++++++++-- ...nload_strings_files_from_glotpress_spec.rb | 15 +++-- spec/ios_l10n_helper_spec.rb | 16 ++--- spec/metadata_download_helper_spec.rb | 25 ++++++++ 10 files changed, 144 insertions(+), 59 deletions(-) create mode 100644 spec/metadata_download_helper_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 40b860ad1..c9cd55a1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ _None_ ### Bug Fixes -_None_ +- Fail translation download actions when a GlotPress request or downloaded metadata is invalid, preventing CI jobs from silently succeeding without updated translations. [#771] ### Internal Changes diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb index 05fd4f3f7..3c802224d 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb @@ -254,7 +254,7 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, all_xml_documents = glotpress_filters.map do |filters| UI.message "Downloading translations for '#{lang_codes[:android]}' from GlotPress (#{lang_codes[:glotpress]}) [#{filters}]..." download_glotpress_export_file(project_url: glotpress_project_url, locale: lang_codes[:glotpress], filters: filters) - end.compact + end next if all_xml_documents.empty? # Merge all XMLs together diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb index c7b09c118..922aefa8c 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb @@ -7,6 +7,8 @@ module Fastlane module Helper # A helper class to download files from GlotPress with proper error handling and retry mechanism class GlotPressDownloader + class DownloadError < StandardError; end + AUTO_RETRY_SLEEP_TIME = 20 MAX_AUTO_RETRY_ATTEMPTS = 30 @@ -31,7 +33,8 @@ def initialize(url:, locale:, auto_retry: false) # @param [String] locale The locale being downloaded (for logging purposes) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) # @yield [String] The response body if the download was successful - # @return The result of the block if provided, or true/false indicating success if no block provided + # @return The result of the block if provided, or true if no block is provided + # @raise [DownloadError] If the download fails after retry handling # # def self.download(url:, locale:, auto_retry: false, &) @@ -41,7 +44,8 @@ def self.download(url:, locale:, auto_retry: false, &) # Downloads data from GlotPress # # @yield [String] The response body if the download was successful - # @return The result of the block if provided, or true/false indicating success if no block provided + # @return The result of the block if provided, or true if no block is provided + # @raise [DownloadError] If the download fails after retry handling # def download(&) @auto_retry_attempt_counter = 0 # Reset counter only at start of download @@ -53,11 +57,7 @@ def download(&) def download_from_url(url, &) uri = URI(url) response = make_request(uri) - result = nil - success = handle_response(response: response, url: url, original_uri: uri) do |body| - result = yield body if block_given? - end - block_given? ? result : success + handle_response(response: response, url: url, original_uri: uri, &) end def make_request(uri) @@ -68,58 +68,55 @@ def make_request(uri) http.request(request) rescue StandardError => e # Network errors, connection errors, etc. - UI.error("Error downloading locale `#{@locale}` — #{e.message} (#{uri})") + message = "Error downloading locale `#{@locale}` — #{e.message} (#{uri})" + UI.error(message) retry if UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") - nil + raise DownloadError, message end - def handle_response(response:, url:, original_uri:) - return false if response.nil? - + def handle_response(response:, url:, original_uri:, &) case response.code when '200' UI.success("Successfully downloaded `#{@locale}`.") - yield response.body if block_given? - true + block_given? ? yield(response.body) : true when '301', '302', '307', '308' # Follow the redirect UI.message("Received #{response.code} for `#{@locale}`. Following redirect...") redirect_url = response['location'] if redirect_url.nil? - UI.error("Received #{response.code} but no location header found.") - false + message = "Received #{response.code} for `#{@locale}` but no location header was found." + UI.error(message) + raise DownloadError, message else # Follow redirect with the new URL - download_from_url(redirect_url) { |body| yield body if block_given? } + download_from_url(redirect_url, &) end when '429' # Rate limited - handle_rate_limiting(url: url) do |body| - yield body if block_given? - end + handle_rate_limiting(url: url, response: response, &) else # Unexpected status code (including 404, 500, etc.) - status_line = "#{response.code} #{response.message}" - UI.error("Error downloading locale `#{@locale}` — #{status_line} (#{original_uri})") - if UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") - download_from_url(url) { |body| yield body if block_given? } - else - false - end + status_line = [response.code, response.message].compact.join(' ').strip + message = "Error downloading locale `#{@locale}` — #{status_line} (#{original_uri})" + UI.error(message) + raise DownloadError, message unless UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") + + download_from_url(url, &) end end - def handle_rate_limiting(url:) + def handle_rate_limiting(url:, response:, &) if @auto_retry && @auto_retry_attempt_counter < MAX_AUTO_RETRY_ATTEMPTS UI.message("Received 429 for `#{@locale}`. Auto retrying in #{AUTO_RETRY_SLEEP_TIME} seconds... (attempt #{@auto_retry_attempt_counter + 1}/#{MAX_AUTO_RETRY_ATTEMPTS})") sleep(AUTO_RETRY_SLEEP_TIME) @auto_retry_attempt_counter += 1 - download_from_url(url) { |body| yield body if block_given? } + download_from_url(url, &) elsif UI.interactive? && UI.confirm("Retry downloading `#{@locale}` after receiving 429 from the API?") - download_from_url(url) { |body| yield body if block_given? } + download_from_url(url, &) else UI.error("Abandoning `#{@locale}` download.") - false + status_line = [response.code, response.message].compact.join(' ').strip + raise DownloadError, "Error downloading locale `#{@locale}` — #{status_line} (#{url})" end end end 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 5f6a63792..d5f090a52 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -208,9 +208,10 @@ def self.download_glotpress_export_file(project_url:, locale:, filters:, destina destination.write(response_body) end end + rescue Fastlane::Helper::GlotPressDownloader::DownloadError + raise rescue StandardError => e - UI.error "Error downloading locale `#{locale}` — #{e.message} (#{url})" - nil + UI.user_error!("Error writing downloaded locale `#{locale}` — #{e.message} (#{url})") end end end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb index f588cee3f..d034c90d7 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb @@ -109,13 +109,11 @@ def get_target_file_path(locale, file_name) def handle_glotpress_response(response_body:, locale:, is_source:) # Parse the JSON response @alternates.clear - loc_data = begin - JSON.parse(response_body) - rescue StandardError - nil - end + loc_data = JSON.parse(response_body) parse_data(locale, loc_data, is_source) reparse_alternates(locale, loc_data, is_source) unless @alternates.empty? + rescue JSON::ParserError => e + UI.user_error!("Error parsing GlotPress response for locale `#{locale}` — #{e.message}") end end end diff --git a/spec/android_localize_helper_spec.rb b/spec/android_localize_helper_spec.rb index 8add98f9c..a05f9c5d6 100644 --- a/spec/android_localize_helper_spec.rb +++ b/spec/android_localize_helper_spec.rb @@ -340,6 +340,29 @@ def generated_file(code) expect(File.exist?(generated_file_path)).to be(true) expect(File.read(generated_file_path)).to eq(expected_merged_content) end + + it 'raises instead of writing a partial export when one filter download fails' do + FileUtils.mkdir_p(File.dirname(generated_file(nil))) + FileUtils.cp(expected_file(nil), generated_file(nil)) + + stub_path = File.join(fixtures_dir, 'filters', 'current.xml') + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=current&format=android") + .to_return(status: 200, body: File.read(stub_path)) + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=waiting&format=android") + .to_return(status: 500) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + + expect do + described_class.download_from_glotpress( + res_dir: tmpdir, + glotpress_project_url: gp_fake_url, + glotpress_filters: [{ status: 'current' }, { status: 'waiting' }], + locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }] + ) + end.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /500/) + + expect(File).not_to exist(generated_file('fakeanloc')) + end end it 'sets a predefined User Agent so GlotPress will not rate-limit us' do diff --git a/spec/glotpress_downloader_spec.rb b/spec/glotpress_downloader_spec.rb index 97121e1dd..4506758b0 100644 --- a/spec/glotpress_downloader_spec.rb +++ b/spec/glotpress_downloader_spec.rb @@ -18,6 +18,15 @@ expect(result).to eq('test content') end + it 'returns true when downloading without a block' do + stub_request(:get, test_url) + .to_return(status: 200, body: 'test content') + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect(downloader.download).to be(true) + end + it 'resets retry counter at start of download' do # Counter should be reset to 0 at the start of download() call downloader = described_class.new(url: test_url, locale: locale, auto_retry: true) @@ -98,7 +107,9 @@ allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) allow(FastlaneCore::UI).to receive(:error) - downloader.download { |body| body } + expect do + downloader.download { |body| body } + end.to raise_error(described_class::DownloadError, /429/) # Should try: 1 initial + 30 retries = 31 total expect(a_request(:get, test_url)).to have_been_made.times(31) @@ -111,9 +122,11 @@ # Mock UI methods to avoid prompts allow(FastlaneCore::UI).to receive(:error) - allow(FastlaneCore::UI).to receive(:confirm).and_return(false) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) - downloader.download { |body| body } + expect do + downloader.download { |body| body } + end.to raise_error(described_class::DownloadError, /429/) # Should only try once (no auto-retry) expect(a_request(:get, test_url)).to have_been_made.once @@ -136,10 +149,20 @@ expect(a_request(:get, test_url)).to have_been_made.once expect(a_request(:get, redirect_url)).to have_been_made.once end + + it 'raises when a redirect has no location header' do + stub_request(:get, test_url).to_return(status: 302) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect do + downloader.download { |body| body } + end.to raise_error(described_class::DownloadError, 'Received 302 for `test-locale` but no location header was found.') + end end describe 'error handling' do - it 'handles 404 errors gracefully in non-interactive mode' do + it 'raises on 404 errors in non-interactive mode' do stub_request(:get, test_url).to_return(status: 404, body: 'Not Found') downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) @@ -148,9 +171,22 @@ allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) allow(FastlaneCore::UI).to receive(:error) - result = downloader.download { |body| body } + expect do + downloader.download { |body| body } + end.to raise_error(described_class::DownloadError, /404/) + expect(a_request(:get, test_url)).to have_been_made.once + end + + it 'raises on SSL errors in non-interactive mode' do + stub_request(:get, test_url).to_raise(OpenSSL::SSL::SSLError.new('certificate verify failed')) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + allow(FastlaneCore::UI).to receive(:error) - expect(result).to be_falsey + expect do + downloader.download { |body| body } + end.to raise_error(described_class::DownloadError, /certificate verify failed/) expect(a_request(:get, test_url)).to have_been_made.once end end diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index 573beeb6c..0195bbc9a 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -66,7 +66,7 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) end describe 'error handling' do - it 'shows an error if an invalid locale is provided (404)' do + it 'raises 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']) @@ -75,13 +75,16 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) allow(FastlaneCore::UI).to receive(:confirm).and_return(false) # as we will be asked if we want to retry when getting a network error # Act - run_described_fastlane_action( - project_url: gp_fake_url, - locales: { 'unknown-locale': 'Base' }, - download_dir: tmp_dir - ) + act = lambda do + run_described_fastlane_action( + project_url: gp_fake_url, + locales: { 'unknown-locale': 'Base' }, + download_dir: tmp_dir + ) + end # Assert + expect { act.call }.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /404 Not Found/) expect(stub).to have_been_made.once 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 (#{gp_fake_url}/unknown-locale/default/export-translations/?filters%5Bstatus%5D=current&format=strings)"]) diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index e7e492ebb..0ba7c401a 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -390,7 +390,7 @@ def file_encoding(path) end describe 'invalid parameters' do - it 'prints an `UI.error` if passed a non-existing locale (or any other 404)' do + it 'raises if passed a non-existing locale (or any other 404)' do # Arrange stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations/").with(query: { format: 'strings' }).to_return(status: [404, 'Not Found']) error_messages = [] @@ -398,24 +398,26 @@ def file_encoding(path) allow(FastlaneCore::UI).to receive(:confirm).and_return(false) # as we will be asked if we want to retry when getting a network error dest = StringIO.new # Act - described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest) + expect do + described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest) + end.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /404 Not Found/) # Assert expect(stub).to have_been_made.once expect(error_messages).to eq(["Error downloading locale `invalid` — 404 Not Found (#{gp_fake_url}/invalid/default/export-translations/?format=strings)"]) end - it 'prints an `UI.error` if the destination cannot be written to' do + it 'raises if the destination cannot be written to' do # Arrange 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' # Act - described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest) + act = lambda do + described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest) + end # Assert + expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, /Error writing downloaded locale `fr`/) 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} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) end end end diff --git a/spec/metadata_download_helper_spec.rb b/spec/metadata_download_helper_spec.rb new file mode 100644 index 000000000..fa36ba122 --- /dev/null +++ b/spec/metadata_download_helper_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require_relative 'spec_helper' + +describe Fastlane::Helper::MetadataDownloader do + let(:test_url) { 'https://translate.wordpress.org/projects/test/locale/default/export-translations/' } + let(:downloader) { described_class.new('/tmp/metadata', {}, false) } + + it 'propagates download failures' do + stub_request(:get, test_url).to_return(status: 500) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + + expect do + downloader.download('fr', test_url, false) + end.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /500/) + end + + it 'raises when the downloaded metadata is not valid JSON' do + stub_request(:get, test_url).to_return(status: 200, body: '{') + + expect do + downloader.download('fr', test_url, false) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /Error parsing GlotPress response for locale `fr`/) + end +end From 3310f87abcf4bfe1ef7363e634e0bb801579f5e1 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 21:12:08 +0200 Subject: [PATCH 02/10] Address translation download review feedback --- CHANGELOG.md | 2 +- ...s_download_strings_files_from_glotpress.rb | 53 +++++++++----- .../helper/android/android_localize_helper.rb | 14 +++- .../helper/glotpress_downloader.rb | 73 +++++++++++-------- .../helper/ios/ios_l10n_helper.rb | 22 +++--- .../helper/metadata_download_helper.rb | 21 +++++- spec/android_localize_helper_spec.rb | 27 ++++++- spec/glotpress_downloader_spec.rb | 54 ++++++++++++-- ...nload_strings_files_from_glotpress_spec.rb | 65 +++++++---------- spec/ios_l10n_helper_spec.rb | 2 +- spec/metadata_download_helper_spec.rb | 46 ++++++++++-- 11 files changed, 254 insertions(+), 125 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9cd55a1a..a1dc47627 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ _None_ ### Bug Fixes -- Fail translation download actions when a GlotPress request or downloaded metadata is invalid, preventing CI jobs from silently succeeding without updated translations. [#771] +- Fail translation download actions when a GlotPress request or downloaded translation data is invalid, preventing CI jobs from silently succeeding without updated translations. [#771] ### Internal Changes 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 ccbca6cdd..04cb757b2 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,5 +1,7 @@ # frozen_string_literal: true +require 'tempfile' + module Fastlane module Actions class IosDownloadStringsFilesFromGlotpressAction < Action @@ -15,34 +17,45 @@ def self.run(params) UI.message "Downloading translations for '#{lproj_name}' from GlotPress (#{glotpress_locale}) [#{params[:filters]}]..." lproj_dir = File.join(download_dir, "#{lproj_name}.lproj") destination = File.join(lproj_dir, "#{params[:table_basename]}.strings") + destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 FileUtils.mkdir_p(lproj_dir) - 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] + Tempfile.create([params[:table_basename], '.strings'], lproj_dir) do |temporary_file| + Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( + project_url: params[:project_url], + locale: glotpress_locale, + filters: params[:filters], + destination: temporary_file + ) + temporary_file.flush + # Do a quick check of the downloaded `.strings` file to ensure it looks valid + validate_strings_file(temporary_file.path, display_path: destination) unless params[:skip_file_validation] + File.chmod(destination_mode, temporary_file.path) + temporary_file.close + FileUtils.mv(temporary_file.path, destination) + end end end # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations - 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. + def self.validate_strings_file(path, display_path: path) + UI.user_error!("The file exported from GlotPress was not created (`#{display_path}`)") unless File.exist?(path) + UI.user_error!("The file exported from GlotPress is empty (`#{display_path}`)") if File.empty?(path) - translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: destination) - 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.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.' - ) + translations = begin + Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path) + rescue StandardError => e + UI.user_error!("Error while validating the file exported from GlotPress (`#{display_path}`) - #{e.message.chomp}") end - rescue StandardError => e - UI.error("Error while validating the file exported from GlotPress (`#{destination}`) - #{e.message.chomp}") + + empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort + return if empty_keys.empty? + + UI.user_error!( + "Found empty translations in `#{display_path}` 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 ##################################################### diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb index 3c802224d..6016dcb0a 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb @@ -246,6 +246,7 @@ def self.create_available_languages_file(res_dir:, locale_codes:) # def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, glotpress_filters: { status: 'current' }) glotpress_filters = [glotpress_filters] unless glotpress_filters.is_a?(Array) + UI.user_error!('At least one GlotPress filter is required.') if glotpress_filters.empty? orig_file = File.join(res_dir, 'values', 'strings.xml') orig_xml = File.open(orig_file) { |f| Nokogiri::XML(f, nil, Encoding::UTF_8.to_s) } @@ -255,7 +256,6 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, UI.message "Downloading translations for '#{lang_codes[:android]}' from GlotPress (#{lang_codes[:glotpress]}) [#{filters}]..." download_glotpress_export_file(project_url: glotpress_project_url, locale: lang_codes[:glotpress], filters: filters) end - next if all_xml_documents.empty? # Merge all XMLs together merged_xml = merge_xml_documents(all_xml_documents) @@ -294,7 +294,17 @@ def self.download_glotpress_export_file(project_url:, locale:, filters:) auto_retry: true ) do |response_body| # Replace tabs with spaces (GlotPress uses tabs, but we prefer spaces) - Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s) + begin + xml = Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s, &:strict) + rescue Nokogiri::XML::SyntaxError => e + UI.user_error!("Invalid Android translation export for locale `#{locale}` — #{e.message} (#{url})") + end + + unless xml.root&.name == 'resources' + UI.user_error!("Invalid Android translation export for locale `#{locale}` — expected a `resources` root element (#{url})") + end + + xml end end private_class_method :download_glotpress_export_file diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb index 922aefa8c..5c1381b92 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb @@ -7,10 +7,9 @@ module Fastlane module Helper # A helper class to download files from GlotPress with proper error handling and retry mechanism class GlotPressDownloader - class DownloadError < StandardError; end - AUTO_RETRY_SLEEP_TIME = 20 MAX_AUTO_RETRY_ATTEMPTS = 30 + MAX_REDIRECTS = 10 attr_reader :auto_retry, :auto_retry_attempt_counter, :url, :locale @@ -34,8 +33,7 @@ def initialize(url:, locale:, auto_retry: false) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) # @yield [String] The response body if the download was successful # @return The result of the block if provided, or true if no block is provided - # @raise [DownloadError] If the download fails after retry handling - # + # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling # def self.download(url:, locale:, auto_retry: false, &) new(url: url, locale: locale, auto_retry: auto_retry).download(&) @@ -45,19 +43,28 @@ def self.download(url:, locale:, auto_retry: false, &) # # @yield [String] The response body if the download was successful # @return The result of the block if provided, or true if no block is provided - # @raise [DownloadError] If the download fails after retry handling + # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling # def download(&) @auto_retry_attempt_counter = 0 # Reset counter only at start of download - download_from_url(@url, &) + download_from_url(@url, redirect_count: 0, &) end private - def download_from_url(url, &) - uri = URI(url) + def download_from_url(url, redirect_count:, &) + uri = parse_uri(url) response = make_request(uri) - handle_response(response: response, url: url, original_uri: uri, &) + handle_response(response: response, url: url, original_uri: uri, redirect_count: redirect_count, &) + end + + def parse_uri(url) + uri = URI(url) + return uri if uri.is_a?(URI::HTTP) && uri.host + + fail_download!("Invalid URL for locale `#{@locale}` (#{url})") + rescue URI::InvalidURIError, TypeError => e + fail_download!("Invalid URL for locale `#{@locale}` — #{e.message} (#{url})") end def make_request(uri) @@ -69,56 +76,62 @@ def make_request(uri) rescue StandardError => e # Network errors, connection errors, etc. message = "Error downloading locale `#{@locale}` — #{e.message} (#{uri})" - UI.error(message) retry if UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") - raise DownloadError, message + fail_download!(message) end - def handle_response(response:, url:, original_uri:, &) + def handle_response(response:, url:, original_uri:, redirect_count:, &) case response.code when '200' + result = block_given? ? yield(response.body) : true UI.success("Successfully downloaded `#{@locale}`.") - block_given? ? yield(response.body) : true + result when '301', '302', '307', '308' # Follow the redirect UI.message("Received #{response.code} for `#{@locale}`. Following redirect...") redirect_url = response['location'] - if redirect_url.nil? - message = "Received #{response.code} for `#{@locale}` but no location header was found." - UI.error(message) - raise DownloadError, message - else - # Follow redirect with the new URL - download_from_url(redirect_url, &) - end + fail_download!("Received #{response.code} for `#{@locale}` but no location header was found (#{original_uri}).") if redirect_url.nil? || redirect_url.empty? + fail_download!("Too many redirects while downloading locale `#{@locale}` (#{original_uri}).") if redirect_count >= MAX_REDIRECTS + + resolved_url = resolve_redirect_url(original_uri, redirect_url) + download_from_url(resolved_url, redirect_count: redirect_count + 1, &) when '429' # Rate limited - handle_rate_limiting(url: url, response: response, &) + handle_rate_limiting(url: url, response: response, redirect_count: redirect_count, &) else # Unexpected status code (including 404, 500, etc.) status_line = [response.code, response.message].compact.join(' ').strip message = "Error downloading locale `#{@locale}` — #{status_line} (#{original_uri})" - UI.error(message) - raise DownloadError, message unless UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") + fail_download!(message) unless UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") - download_from_url(url, &) + download_from_url(url, redirect_count: redirect_count, &) end end - def handle_rate_limiting(url:, response:, &) + def handle_rate_limiting(url:, response:, redirect_count:, &) if @auto_retry && @auto_retry_attempt_counter < MAX_AUTO_RETRY_ATTEMPTS UI.message("Received 429 for `#{@locale}`. Auto retrying in #{AUTO_RETRY_SLEEP_TIME} seconds... (attempt #{@auto_retry_attempt_counter + 1}/#{MAX_AUTO_RETRY_ATTEMPTS})") sleep(AUTO_RETRY_SLEEP_TIME) @auto_retry_attempt_counter += 1 - download_from_url(url, &) + download_from_url(url, redirect_count: redirect_count, &) elsif UI.interactive? && UI.confirm("Retry downloading `#{@locale}` after receiving 429 from the API?") - download_from_url(url, &) + download_from_url(url, redirect_count: redirect_count, &) else - UI.error("Abandoning `#{@locale}` download.") status_line = [response.code, response.message].compact.join(' ').strip - raise DownloadError, "Error downloading locale `#{@locale}` — #{status_line} (#{url})" + fail_download!("Error downloading locale `#{@locale}` — #{status_line} (#{url})") end end + + def resolve_redirect_url(original_uri, redirect_url) + URI.join(original_uri.to_s, redirect_url).to_s + rescue URI::InvalidURIError => e + fail_download!("Invalid redirect URL for locale `#{@locale}` — #{e.message} (#{redirect_url})") + end + + def fail_download!(message) + UI.error(message) + UI.user_error!(message) + end end end end 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 d5f090a52..5e5a53b15 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -196,20 +196,16 @@ def self.download_glotpress_export_file(project_url:, locale:, filters:, destina query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings') url = "#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}" - begin - Fastlane::Helper::GlotPressDownloader.download( - url: url, - locale: locale, - auto_retry: true - ) do |response_body| - if destination.is_a?(String) - File.write(destination, response_body) - else - destination.write(response_body) - end + Fastlane::Helper::GlotPressDownloader.download( + url: url, + locale: locale, + auto_retry: true + ) do |response_body| + if destination.is_a?(String) + File.write(destination, response_body) + else + destination.write(response_body) end - rescue Fastlane::Helper::GlotPressDownloader::DownloadError - raise rescue StandardError => e UI.user_error!("Error writing downloaded locale `#{locale}` — #{e.message} (#{url})") end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb index d034c90d7..a734e26a4 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb @@ -23,7 +23,7 @@ def download(target_locale, glotpress_url, is_source) locale: target_locale, auto_retry: @auto_retry ) do |response_body| - handle_glotpress_response(response_body: response_body, locale: target_locale, is_source: is_source) + handle_glotpress_response(response_body: response_body, locale: target_locale, is_source: is_source, url: glotpress_url) end end @@ -106,14 +106,27 @@ def get_target_file_path(locale, file_name) private - def handle_glotpress_response(response_body:, locale:, is_source:) + def handle_glotpress_response(response_body:, locale:, is_source:, url:) # Parse the JSON response @alternates.clear loc_data = JSON.parse(response_body) + unless valid_metadata_response?(loc_data) + UI.user_error!("Unexpected GlotPress metadata response for locale `#{locale}` (#{url})") + end + parse_data(locale, loc_data, is_source) reparse_alternates(locale, loc_data, is_source) unless @alternates.empty? - rescue JSON::ParserError => e - UI.user_error!("Error parsing GlotPress response for locale `#{locale}` — #{e.message}") + rescue JSON::ParserError, TypeError => e + UI.user_error!("Error parsing GlotPress response for locale `#{locale}` — #{e.message} (#{url})") + end + + def valid_metadata_response?(loc_data) + return loc_data.empty? if loc_data.is_a?(Array) + return false unless loc_data.is_a?(Hash) && !loc_data.empty? + + loc_data.all? do |source, translations| + source.is_a?(String) && translations.is_a?(Array) && translations.all?(String) + end end end end diff --git a/spec/android_localize_helper_spec.rb b/spec/android_localize_helper_spec.rb index a05f9c5d6..918ea5f21 100644 --- a/spec/android_localize_helper_spec.rb +++ b/spec/android_localize_helper_spec.rb @@ -295,7 +295,7 @@ def generated_file(code) "#{gp_fake_url.chomp('/')}/#{locale[:glotpress]}/default/export-translations/?filters%5Bstatus%5D=custom-status&filters%5Bwarnings%5D=yes&format=android" end custom_gp_urls.each do |url| - stub_request(:get, url) + stub_request(:get, url).to_return(status: 200, body: '') end # Act @@ -359,7 +359,28 @@ def generated_file(code) glotpress_filters: [{ status: 'current' }, { status: 'waiting' }], locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }] ) - end.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /500/) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /500/) + + expect(File).not_to exist(generated_file('fakeanloc')) + end + + it 'raises when no export filters are provided' do + expect do + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, glotpress_filters: [], locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }]) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /At least one GlotPress filter is required/) + end + end + + ['', 'Service unavailable'].each do |response_body| + it "raises instead of writing an invalid successful export: #{response_body.inspect}" do + FileUtils.mkdir_p(File.dirname(generated_file(nil))) + FileUtils.cp(expected_file(nil), generated_file(nil)) + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=current&format=android") + .to_return(status: 200, body: response_body) + + expect do + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }]) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /Invalid Android translation export.*fakegploc/) expect(File).not_to exist(generated_file('fakeanloc')) end @@ -384,7 +405,7 @@ def generated_file(code) # - https://github.com/bblimke/webmock/tree/33d8810c2828fc17010e15cc3f21ad2c726a966f#matching-requests # - https://github.com/bblimke/webmock/issues/276#issuecomment-28625436 headers: { 'User-Agent' => 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/' } - ).to_return(status: 200, body: '') + ).to_return(status: 200, body: '') # Act described_class.download_from_glotpress( diff --git a/spec/glotpress_downloader_spec.rb b/spec/glotpress_downloader_spec.rb index 4506758b0..ab03f94f9 100644 --- a/spec/glotpress_downloader_spec.rb +++ b/spec/glotpress_downloader_spec.rb @@ -27,6 +27,18 @@ expect(downloader.download).to be(true) end + it 'does not report success until the downloaded body has been accepted' do + stub_request(:get, test_url) + .to_return(status: 200, body: 'invalid content') + expect(FastlaneCore::UI).not_to receive(:success) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect do + downloader.download { raise 'invalid downloaded body' } + end.to raise_error(RuntimeError, 'invalid downloaded body') + end + it 'resets retry counter at start of download' do # Counter should be reset to 0 at the start of download() call downloader = described_class.new(url: test_url, locale: locale, auto_retry: true) @@ -105,11 +117,11 @@ # Mock non-interactive environment to avoid prompts allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) - allow(FastlaneCore::UI).to receive(:error) + expect(FastlaneCore::UI).to receive(:error).with(/Error downloading locale `test-locale` — 429.*#{Regexp.escape(test_url)}/) expect do downloader.download { |body| body } - end.to raise_error(described_class::DownloadError, /429/) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /429/) # Should try: 1 initial + 30 retries = 31 total expect(a_request(:get, test_url)).to have_been_made.times(31) @@ -126,7 +138,7 @@ expect do downloader.download { |body| body } - end.to raise_error(described_class::DownloadError, /429/) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /429/) # Should only try once (no auto-retry) expect(a_request(:get, test_url)).to have_been_made.once @@ -150,6 +162,19 @@ expect(a_request(:get, redirect_url)).to have_been_made.once end + it 'resolves relative redirect locations' do + redirect_url = 'https://translate.wordpress.org/redirected' + stub_request(:get, test_url) + .to_return(status: 302, headers: { 'Location' => '/redirected' }) + stub_request(:get, redirect_url) + .to_return(status: 200, body: 'redirected content') + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect(downloader.download { |body| body }).to eq('redirected content') + expect(a_request(:get, redirect_url)).to have_been_made.once + end + it 'raises when a redirect has no location header' do stub_request(:get, test_url).to_return(status: 302) @@ -157,7 +182,17 @@ expect do downloader.download { |body| body } - end.to raise_error(described_class::DownloadError, 'Received 302 for `test-locale` but no location header was found.') + end.to raise_error(FastlaneCore::Interface::FastlaneError, "Received 302 for `test-locale` but no location header was found (#{test_url}).") + end + + it 'raises when the maximum number of redirects is exceeded' do + stub_request(:get, test_url) + .to_return(status: 302, headers: { 'Location' => test_url }) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect { downloader.download { |body| body } }.to raise_error(FastlaneCore::Interface::FastlaneError, /Too many redirects/) + expect(a_request(:get, test_url)).to have_been_made.times(described_class::MAX_REDIRECTS + 1) end end @@ -173,7 +208,7 @@ expect do downloader.download { |body| body } - end.to raise_error(described_class::DownloadError, /404/) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /404/) expect(a_request(:get, test_url)).to have_been_made.once end @@ -186,8 +221,15 @@ expect do downloader.download { |body| body } - end.to raise_error(described_class::DownloadError, /certificate verify failed/) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /certificate verify failed/) expect(a_request(:get, test_url)).to have_been_made.once end + + it 'raises a clean user error for invalid URLs' do + invalid_url = 'https://translate.wordpress.org/an invalid path' + downloader = described_class.new(url: invalid_url, locale: locale, auto_retry: false) + + expect { downloader.download { |body| body } }.to raise_error(FastlaneCore::Interface::FastlaneError, /Invalid URL for locale `test-locale`.*an invalid path/) + 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 index 0195bbc9a..5de0063ab 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -84,7 +84,7 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) end # Assert - expect { act.call }.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /404 Not Found/) + expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, /404 Not Found/) expect(stub).to have_been_made.once 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 (#{gp_fake_url}/unknown-locale/default/export-translations/?filters%5Bstatus%5D=current&format=strings)"]) @@ -109,55 +109,46 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) 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.' - 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. + [ + ['invalid', 'some invalid strings file content', /Error while validating the file exported from GlotPress.*Property List error/m], + ['empty', '', /file exported from GlotPress is empty/], + ].each do |description, response_body, expected_error| + it "raises without replacing the existing file if a download is #{description}" do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: response_body) + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(File.dirname(file)) + File.write(file, 'existing valid content') + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.to raise_error(FastlaneCore::Interface::FastlaneError, expected_error) + + expect(stub).to have_been_made.once + expect(File.read(file)).to eq('existing valid content') + end end end - it 'reports if a downloaded file has empty translations' do + it 'raises 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 - ) + expect do + run_described_fastlane_action( + project_url: gp_fake_url, + locales: { 'fr-FR': 'fr' }, + download_dir: tmp_dir + ) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /Found empty translations.*\["key2", "key3"\]/m) # 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]) + expect(File).not_to exist(file) end end diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index 0ba7c401a..dce78070a 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -400,7 +400,7 @@ def file_encoding(path) # Act expect do described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest) - end.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /404 Not Found/) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /404 Not Found/) # Assert expect(stub).to have_been_made.once expect(error_messages).to eq(["Error downloading locale `invalid` — 404 Not Found (#{gp_fake_url}/invalid/default/export-translations/?format=strings)"]) diff --git a/spec/metadata_download_helper_spec.rb b/spec/metadata_download_helper_spec.rb index fa36ba122..40ee94d0b 100644 --- a/spec/metadata_download_helper_spec.rb +++ b/spec/metadata_download_helper_spec.rb @@ -4,22 +4,52 @@ describe Fastlane::Helper::MetadataDownloader do let(:test_url) { 'https://translate.wordpress.org/projects/test/locale/default/export-translations/' } - let(:downloader) { described_class.new('/tmp/metadata', {}, false) } + let(:target_files) { { release_notes: { desc: 'release_notes.txt', max_size: 0 } } } + + def existing_metadata_file(tmpdir) + path = File.join(tmpdir, 'fr', 'release_notes.txt') + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, 'Existing release notes') + path + end it 'propagates download failures' do stub_request(:get, test_url).to_return(status: 500) allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) - expect do - downloader.download('fr', test_url, false) - end.to raise_error(Fastlane::Helper::GlotPressDownloader::DownloadError, /500/) + in_tmp_dir do |tmpdir| + downloader = described_class.new(tmpdir, target_files, false) + + expect do + downloader.download('fr', test_url, false) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /500/) + end + end + + ['{', 'null', '42', '{}', '["unexpected"]', '{"error":"maintenance"}'].each do |response_body| + it "raises without deleting existing metadata for an invalid response body: #{response_body}" do + stub_request(:get, test_url).to_return(status: 200, body: response_body) + + in_tmp_dir do |tmpdir| + existing_file = existing_metadata_file(tmpdir) + downloader = described_class.new(tmpdir, target_files, false) + + expect { downloader.download('fr', test_url, false) }.to raise_error(FastlaneCore::Interface::FastlaneError, /GlotPress.*locale `fr`.*#{Regexp.escape(test_url)}/) + expect(File.read(existing_file)).to eq('Existing release notes') + end + end end - it 'raises when the downloaded metadata is not valid JSON' do - stub_request(:get, test_url).to_return(status: 200, body: '{') + it 'accepts a legitimate empty export and removes stale metadata' do + stub_request(:get, test_url).to_return(status: 200, body: '[]') + + in_tmp_dir do |tmpdir| + existing_file = existing_metadata_file(tmpdir) + downloader = described_class.new(tmpdir, target_files, false) - expect do downloader.download('fr', test_url, false) - end.to raise_error(FastlaneCore::Interface::FastlaneError, /Error parsing GlotPress response for locale `fr`/) + + expect(File).not_to exist(existing_file) + end end end From c65b77850fd92efef236509c795d5e712aa9b938 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 21:57:21 +0200 Subject: [PATCH 03/10] Add opt-in translation failure mode --- CHANGELOG.md | 4 +- .../android_download_translations_action.rb | 10 ++- .../common/gp_downloadmetadata_action.rb | 9 ++- ...s_download_strings_files_from_glotpress.rb | 59 +++++++++++++---- .../helper/android/android_localize_helper.rb | 38 ++++++----- .../helper/glotpress_downloader.rb | 64 +++++++++++++------ .../helper/ios/ios_l10n_helper.rb | 11 +++- .../helper/metadata_download_helper.rb | 22 +++++-- ...droid_download_translations_action_spec.rb | 27 ++++++++ spec/android_localize_helper_spec.rb | 19 +++++- spec/glotpress_downloader_spec.rb | 28 +++++--- spec/gp_downloadmetadata_action_spec.rb | 27 ++++++++ ...nload_strings_files_from_glotpress_spec.rb | 26 +++++++- spec/ios_l10n_helper_spec.rb | 16 ++--- spec/metadata_download_helper_spec.rb | 18 +++++- 15 files changed, 290 insertions(+), 88 deletions(-) create mode 100644 spec/android_download_translations_action_spec.rb create mode 100644 spec/gp_downloadmetadata_action_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index a1dc47627..78967d54b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,11 @@ _None_ ### New Features -_None_ +- Add an opt-in `fail_on_error` mode to translation download actions so GlotPress request and response errors fail CI jobs. [#771] ### Bug Fixes -- Fail translation download actions when a GlotPress request or downloaded translation data is invalid, preventing CI jobs from silently succeeding without updated translations. [#771] +_None_ ### Internal Changes diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb index 97be798f9..a60f86c73 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb @@ -19,7 +19,8 @@ def self.run(params) res_dir: res_dir, glotpress_project_url: params[:glotpress_url], glotpress_filters: params[:status_filter].map { |s| { status: s } }, - locales_map: params[:locales] + locales_map: params[:locales], + fail_on_error: params[:fail_on_error] ) # Update submodules then lint translations @@ -99,6 +100,13 @@ def self.available_options type: Boolean, default_value: false ), + FastlaneCore::ConfigItem.new( + key: :fail_on_error, + env_name: 'FL_DOWNLOAD_TRANSLATIONS_FAIL_ON_ERROR', + description: 'Whether to fail when a GlotPress request or downloaded response is invalid', + type: Boolean, + default_value: false + ), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb index ebadd8b41..c34700574 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb @@ -13,12 +13,13 @@ def self.run(params) UI.message "Source locale: #{params[:source_locale].nil? ? '-' : params[:source_locale]}" UI.message "Path: #{params[:download_path]}" UI.message "Auto-retry: #{params[:auto_retry]}" + UI.message "Fail on error: #{params[:fail_on_error]}" # Check download path FileUtils.mkdir_p(params[:download_path]) # Download - downloader = Fastlane::Helper::MetadataDownloader.new(params[:download_path], params[:target_files], params[:auto_retry]) + downloader = Fastlane::Helper::MetadataDownloader.new(params[:download_path], params[:target_files], params[:auto_retry], fail_on_error: params[:fail_on_error]) params[:locales].each do |loc| if loc.is_a?(Array) @@ -77,6 +78,12 @@ def self.available_options type: FastlaneCore::Boolean, optional: true, default_value: true), + FastlaneCore::ConfigItem.new(key: :fail_on_error, + env_name: 'FL_DOWNLOAD_METADATA_FAIL_ON_ERROR', + description: 'Whether to fail when a GlotPress request or downloaded response is invalid', + type: FastlaneCore::Boolean, + optional: true, + default_value: false), ] end 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 04cb757b2..b2a6bc9a4 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 @@ -17,19 +17,33 @@ def self.run(params) UI.message "Downloading translations for '#{lproj_name}' from GlotPress (#{glotpress_locale}) [#{params[:filters]}]..." lproj_dir = File.join(download_dir, "#{lproj_name}.lproj") destination = File.join(lproj_dir, "#{params[:table_basename]}.strings") - destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 FileUtils.mkdir_p(lproj_dir) - Tempfile.create([params[:table_basename], '.strings'], lproj_dir) do |temporary_file| + unless params[:fail_on_error] Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( project_url: params[:project_url], locale: glotpress_locale, filters: params[:filters], - destination: temporary_file + destination: destination + ) + validate_strings_file(destination) unless params[:skip_file_validation] + next + end + + destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 + Tempfile.create([params[:table_basename], '.strings'], lproj_dir) do |temporary_file| + downloaded = Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( + project_url: params[:project_url], + locale: glotpress_locale, + filters: params[:filters], + destination: temporary_file, + fail_on_error: true ) + next unless downloaded + temporary_file.flush # Do a quick check of the downloaded `.strings` file to ensure it looks valid - validate_strings_file(temporary_file.path, display_path: destination) unless params[:skip_file_validation] + validate_strings_file(temporary_file.path, display_path: destination, fail_on_error: true) unless params[:skip_file_validation] File.chmod(destination_mode, temporary_file.path) temporary_file.close FileUtils.mv(temporary_file.path, destination) @@ -38,26 +52,41 @@ def self.run(params) end # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations - def self.validate_strings_file(path, display_path: path) - UI.user_error!("The file exported from GlotPress was not created (`#{display_path}`)") unless File.exist?(path) - UI.user_error!("The file exported from GlotPress is empty (`#{display_path}`)") if File.empty?(path) + def self.validate_strings_file(path, display_path: path, fail_on_error: false) + unless File.exist?(path) + report_validation_error("The file exported from GlotPress was not created (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error + return + end - translations = begin - Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path) + if File.empty?(path) + report_validation_error("The file exported from GlotPress is empty (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error + return + end + + translations = nil + begin + translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path) rescue StandardError => e - UI.user_error!("Error while validating the file exported from GlotPress (`#{display_path}`) - #{e.message.chomp}") + report_validation_error("Error while validating the file exported from GlotPress (`#{display_path}`) - #{e.message.chomp}", fail_on_error: fail_on_error) + return end empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort return if empty_keys.empty? - UI.user_error!( + report_validation_error( "Found empty translations in `#{display_path}` 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.' + + 'Please report this to the GlotPress team, and fix the file locally before continuing.', + fail_on_error: fail_on_error ) end + def self.report_validation_error(message, fail_on_error:) + fail_on_error ? UI.user_error!(message) : UI.error(message) + end + private_class_method :report_validation_error + ##################################################### # @!group Documentation ##################################################### @@ -105,6 +134,12 @@ def self.available_options type: Fastlane::Boolean, optional: true, default_value: false), + FastlaneCore::ConfigItem.new(key: :fail_on_error, + env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_FAIL_ON_ERROR', + description: 'Whether to fail when a GlotPress request or downloaded response is invalid', + type: Fastlane::Boolean, + optional: true, + default_value: false), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb index 6016dcb0a..b2c615c35 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb @@ -243,10 +243,11 @@ def self.create_available_languages_file(res_dir:, locale_codes:) # @param [ArrayString}>] locales_map # An array of locales to download. Each item in the array must be a Hash # with keys `:glotpress` and `:android` containing the respective locale codes. + # @param [Boolean] fail_on_error Whether to fail on request errors or invalid downloaded XML. # - def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, glotpress_filters: { status: 'current' }) + def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, glotpress_filters: { status: 'current' }, fail_on_error: false) glotpress_filters = [glotpress_filters] unless glotpress_filters.is_a?(Array) - UI.user_error!('At least one GlotPress filter is required.') if glotpress_filters.empty? + UI.user_error!('At least one GlotPress filter is required.') if fail_on_error && glotpress_filters.empty? orig_file = File.join(res_dir, 'values', 'strings.xml') orig_xml = File.open(orig_file) { |f| Nokogiri::XML(f, nil, Encoding::UTF_8.to_s) } @@ -254,8 +255,9 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, locales_map.each do |lang_codes| all_xml_documents = glotpress_filters.map do |filters| UI.message "Downloading translations for '#{lang_codes[:android]}' from GlotPress (#{lang_codes[:glotpress]}) [#{filters}]..." - download_glotpress_export_file(project_url: glotpress_project_url, locale: lang_codes[:glotpress], filters: filters) - end + download_glotpress_export_file(project_url: glotpress_project_url, locale: lang_codes[:glotpress], filters: filters, fail_on_error: fail_on_error) + end.compact + next if all_xml_documents.empty? # Merge all XMLs together merged_xml = merge_xml_documents(all_xml_documents) @@ -282,29 +284,35 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, # @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' }` or `{ status: 'review' }`. + # @param [Boolean] fail_on_error Whether to fail on request errors or invalid downloaded XML. # @return [Nokogiri::XML::Document] the download XML document, parsed as a Nokogiri::XML object # - def self.download_glotpress_export_file(project_url:, locale:, filters:) + def self.download_glotpress_export_file(project_url:, locale:, filters:, fail_on_error:) query_params = filters.transform_keys { |k| "filters[#{k}]" }.merge(format: 'android') url = "#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}" Fastlane::Helper::GlotPressDownloader.download( url: url, locale: locale, - auto_retry: true + auto_retry: true, + fail_on_error: fail_on_error ) do |response_body| # Replace tabs with spaces (GlotPress uses tabs, but we prefer spaces) - begin - xml = Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s, &:strict) - rescue Nokogiri::XML::SyntaxError => e - UI.user_error!("Invalid Android translation export for locale `#{locale}` — #{e.message} (#{url})") - end + if fail_on_error + begin + xml = Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s, &:strict) + rescue Nokogiri::XML::SyntaxError => e + UI.user_error!("Invalid Android translation export for locale `#{locale}` — #{e.message} (#{url})") + end - unless xml.root&.name == 'resources' - UI.user_error!("Invalid Android translation export for locale `#{locale}` — expected a `resources` root element (#{url})") - end + unless xml.root&.name == 'resources' + UI.user_error!("Invalid Android translation export for locale `#{locale}` — expected a `resources` root element (#{url})") + end - xml + xml + else + Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s) + end end end private_class_method :download_glotpress_export_file diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb index 5c1381b92..f24c7502a 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb @@ -11,18 +11,20 @@ class GlotPressDownloader MAX_AUTO_RETRY_ATTEMPTS = 30 MAX_REDIRECTS = 10 - attr_reader :auto_retry, :auto_retry_attempt_counter, :url, :locale + attr_reader :auto_retry, :auto_retry_attempt_counter, :url, :locale, :fail_on_error # Initialize a new GlotPressDownloader # # @param [String] url The URL to download from # @param [String] locale The locale being downloaded (for logging purposes) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) + # @param [Boolean] fail_on_error Whether to raise a Fastlane error after retry handling instead of returning false # - def initialize(url:, locale:, auto_retry: false) + def initialize(url:, locale:, auto_retry: false, fail_on_error: false) @url = url @locale = locale @auto_retry = auto_retry + @fail_on_error = fail_on_error @auto_retry_attempt_counter = 0 end @@ -31,19 +33,20 @@ def initialize(url:, locale:, auto_retry: false) # @param [String] url The URL to download from # @param [String] locale The locale being downloaded (for logging purposes) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) + # @param [Boolean] fail_on_error Whether to raise a Fastlane error after retry handling instead of returning false # @yield [String] The response body if the download was successful - # @return The result of the block if provided, or true if no block is provided - # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling + # @return The result of the block if provided, or true/false indicating success if no block is provided + # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling and `fail_on_error` is true # - def self.download(url:, locale:, auto_retry: false, &) - new(url: url, locale: locale, auto_retry: auto_retry).download(&) + def self.download(url:, locale:, auto_retry: false, fail_on_error: false, &) + new(url: url, locale: locale, auto_retry: auto_retry, fail_on_error: fail_on_error).download(&) end # Downloads data from GlotPress # # @yield [String] The response body if the download was successful - # @return The result of the block if provided, or true if no block is provided - # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling + # @return The result of the block if provided, or true/false indicating success if no block is provided + # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling and `fail_on_error` is true # def download(&) @auto_retry_attempt_counter = 0 # Reset counter only at start of download @@ -54,17 +57,31 @@ def download(&) def download_from_url(url, redirect_count:, &) uri = parse_uri(url) + return block_given? ? nil : false if uri.nil? + response = make_request(uri) - handle_response(response: response, url: url, original_uri: uri, redirect_count: redirect_count, &) + return block_given? ? nil : false if response.nil? + + unless block_given? + return handle_response(response: response, url: url, original_uri: uri, redirect_count: redirect_count) + end + + result = nil + handle_response(response: response, url: url, original_uri: uri, redirect_count: redirect_count) do |body| + result = yield(body) + end + result end def parse_uri(url) uri = URI(url) return uri if uri.is_a?(URI::HTTP) && uri.host - fail_download!("Invalid URL for locale `#{@locale}` (#{url})") + handle_failure("Invalid URL for locale `#{@locale}` (#{url})") + nil rescue URI::InvalidURIError, TypeError => e - fail_download!("Invalid URL for locale `#{@locale}` — #{e.message} (#{url})") + handle_failure("Invalid URL for locale `#{@locale}` — #{e.message} (#{url})") + nil end def make_request(uri) @@ -77,23 +94,26 @@ def make_request(uri) # Network errors, connection errors, etc. message = "Error downloading locale `#{@locale}` — #{e.message} (#{uri})" retry if UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") - fail_download!(message) + handle_failure(message) + nil end def handle_response(response:, url:, original_uri:, redirect_count:, &) case response.code when '200' - result = block_given? ? yield(response.body) : true + yield(response.body) if block_given? UI.success("Successfully downloaded `#{@locale}`.") - result + true when '301', '302', '307', '308' # Follow the redirect UI.message("Received #{response.code} for `#{@locale}`. Following redirect...") redirect_url = response['location'] - fail_download!("Received #{response.code} for `#{@locale}` but no location header was found (#{original_uri}).") if redirect_url.nil? || redirect_url.empty? - fail_download!("Too many redirects while downloading locale `#{@locale}` (#{original_uri}).") if redirect_count >= MAX_REDIRECTS + return handle_failure("Received #{response.code} for `#{@locale}` but no location header was found (#{original_uri}).") if redirect_url.nil? || redirect_url.empty? + return handle_failure("Too many redirects while downloading locale `#{@locale}` (#{original_uri}).") if redirect_count >= MAX_REDIRECTS resolved_url = resolve_redirect_url(original_uri, redirect_url) + return false if resolved_url.nil? + download_from_url(resolved_url, redirect_count: redirect_count + 1, &) when '429' # Rate limited @@ -102,7 +122,7 @@ def handle_response(response:, url:, original_uri:, redirect_count:, &) # Unexpected status code (including 404, 500, etc.) status_line = [response.code, response.message].compact.join(' ').strip message = "Error downloading locale `#{@locale}` — #{status_line} (#{original_uri})" - fail_download!(message) unless UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") + return handle_failure(message) unless UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") download_from_url(url, redirect_count: redirect_count, &) end @@ -118,19 +138,21 @@ def handle_rate_limiting(url:, response:, redirect_count:, &) download_from_url(url, redirect_count: redirect_count, &) else status_line = [response.code, response.message].compact.join(' ').strip - fail_download!("Error downloading locale `#{@locale}` — #{status_line} (#{url})") + handle_failure("Error downloading locale `#{@locale}` — #{status_line} (#{url})") end end def resolve_redirect_url(original_uri, redirect_url) URI.join(original_uri.to_s, redirect_url).to_s rescue URI::InvalidURIError => e - fail_download!("Invalid redirect URL for locale `#{@locale}` — #{e.message} (#{redirect_url})") + handle_failure("Invalid redirect URL for locale `#{@locale}` — #{e.message} (#{redirect_url})") + nil end - def fail_download!(message) + def handle_failure(message) UI.error(message) - UI.user_error!(message) + UI.user_error!(message) if @fail_on_error + false end end end 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 5e5a53b15..269b4912e 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -191,15 +191,17 @@ def self.generate_strings_file_from_hash(translations:, output_path:) # @param [Hash{Symbol=>String}] filters The hash of filters to apply when exporting from GlotPress. # 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. + # @param [Boolean] fail_on_error Whether to fail on request errors. # - def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:) + def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:, fail_on_error: false) query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings') url = "#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}" Fastlane::Helper::GlotPressDownloader.download( url: url, locale: locale, - auto_retry: true + auto_retry: true, + fail_on_error: fail_on_error ) do |response_body| if destination.is_a?(String) File.write(destination, response_body) @@ -207,7 +209,10 @@ def self.download_glotpress_export_file(project_url:, locale:, filters:, destina destination.write(response_body) end rescue StandardError => e - UI.user_error!("Error writing downloaded locale `#{locale}` — #{e.message} (#{url})") + prefix = fail_on_error ? 'Error writing downloaded locale' : 'Error downloading locale' + message = "#{prefix} `#{locale}` — #{e.message} (#{url})" + fail_on_error ? UI.user_error!(message) : UI.error(message) + nil end end end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb index a734e26a4..8a7b595e8 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb @@ -9,10 +9,11 @@ module Helper class MetadataDownloader attr_reader :target_folder, :target_files - def initialize(target_folder, target_files, auto_retry) + def initialize(target_folder, target_files, auto_retry, fail_on_error: false) @target_folder = target_folder @target_files = target_files @auto_retry = auto_retry + @fail_on_error = fail_on_error @alternates = {} end @@ -21,7 +22,8 @@ def download(target_locale, glotpress_url, is_source) GlotPressDownloader.download( url: glotpress_url, locale: target_locale, - auto_retry: @auto_retry + auto_retry: @auto_retry, + fail_on_error: @fail_on_error ) do |response_body| handle_glotpress_response(response_body: response_body, locale: target_locale, is_source: is_source, url: glotpress_url) end @@ -109,15 +111,21 @@ def get_target_file_path(locale, file_name) def handle_glotpress_response(response_body:, locale:, is_source:, url:) # Parse the JSON response @alternates.clear - loc_data = JSON.parse(response_body) - unless valid_metadata_response?(loc_data) - UI.user_error!("Unexpected GlotPress metadata response for locale `#{locale}` (#{url})") - end + loc_data = parse_metadata_response(response_body: response_body, locale: locale, url: url) parse_data(locale, loc_data, is_source) reparse_alternates(locale, loc_data, is_source) unless @alternates.empty? + end + + def parse_metadata_response(response_body:, locale:, url:) + return JSON.parse(response_body) unless @fail_on_error + + loc_data = JSON.parse(response_body) + UI.user_error!("Unexpected GlotPress metadata response for locale `#{locale}` (#{url})") unless valid_metadata_response?(loc_data) + loc_data rescue JSON::ParserError, TypeError => e - UI.user_error!("Error parsing GlotPress response for locale `#{locale}` — #{e.message} (#{url})") + UI.user_error!("Error parsing GlotPress response for locale `#{locale}` — #{e.message} (#{url})") if @fail_on_error + nil end def valid_metadata_response?(loc_data) diff --git a/spec/android_download_translations_action_spec.rb b/spec/android_download_translations_action_spec.rb new file mode 100644 index 000000000..55a8821be --- /dev/null +++ b/spec/android_download_translations_action_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Fastlane::Actions::AndroidDownloadTranslationsAction do + it 'passes fail_on_error to the download helper' do + allow(Fastlane::Helper::Android::LocalizeHelper).to receive(:create_available_languages_file) + expect(Fastlane::Helper::Android::LocalizeHelper).to receive(:download_from_glotpress).with(hash_including(fail_on_error: true)) + + run_described_fastlane_action( + res_dir: 'res', + glotpress_url: 'https://translate.example/projects/test/', + status_filter: ['current'], + source_locale: 'en_US', + locales: [{ glotpress: 'fr', android: 'fr' }], + lint_task: nil, + skip_commit: true, + fail_on_error: true + ) + end + + it 'keeps fail_on_error disabled by default' do + option = described_class.available_options.find { |item| item.key == :fail_on_error } + + expect(option.default_value).to be(false) + end +end diff --git a/spec/android_localize_helper_spec.rb b/spec/android_localize_helper_spec.rb index 918ea5f21..4e249d4c1 100644 --- a/spec/android_localize_helper_spec.rb +++ b/spec/android_localize_helper_spec.rb @@ -357,7 +357,8 @@ def generated_file(code) res_dir: tmpdir, glotpress_project_url: gp_fake_url, glotpress_filters: [{ status: 'current' }, { status: 'waiting' }], - locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }] + locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }], + fail_on_error: true ) end.to raise_error(FastlaneCore::Interface::FastlaneError, /500/) @@ -366,7 +367,7 @@ def generated_file(code) it 'raises when no export filters are provided' do expect do - described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, glotpress_filters: [], locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }]) + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, glotpress_filters: [], locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }], fail_on_error: true) end.to raise_error(FastlaneCore::Interface::FastlaneError, /At least one GlotPress filter is required/) end end @@ -379,13 +380,25 @@ def generated_file(code) .to_return(status: 200, body: response_body) expect do - described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }]) + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }], fail_on_error: true) end.to raise_error(FastlaneCore::Interface::FastlaneError, /Invalid Android translation export.*fakegploc/) expect(File).not_to exist(generated_file('fakeanloc')) end end + it 'retains permissive response handling by default' do + FileUtils.mkdir_p(File.dirname(generated_file(nil))) + FileUtils.cp(expected_file(nil), generated_file(nil)) + body = 'Service unavailable' + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=current&format=android").to_return(status: 200, body: body) + + expect do + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }]) + end.not_to raise_error + expect(File.read(generated_file('fakeanloc'))).to include(' test_url }) - downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) expect { downloader.download { |body| body } }.to raise_error(FastlaneCore::Interface::FastlaneError, /Too many redirects/) expect(a_request(:get, test_url)).to have_been_made.times(described_class::MAX_REDIRECTS + 1) @@ -197,10 +197,22 @@ end describe 'error handling' do + it 'returns falsey results by default' do + stub_request(:get, test_url).to_return(status: 404, body: 'Not Found') + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + allow(FastlaneCore::UI).to receive(:error) + + downloader = described_class.new(url: test_url, locale: locale) + + expect(downloader.download { |body| body }).to be_nil + expect(downloader.download).to be(false) + expect(a_request(:get, test_url)).to have_been_made.times(2) + end + it 'raises on 404 errors in non-interactive mode' do stub_request(:get, test_url).to_return(status: 404, body: 'Not Found') - downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) # Mock non-interactive environment allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) @@ -215,7 +227,7 @@ it 'raises on SSL errors in non-interactive mode' do stub_request(:get, test_url).to_raise(OpenSSL::SSL::SSLError.new('certificate verify failed')) - downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) allow(FastlaneCore::UI).to receive(:error) @@ -227,7 +239,7 @@ it 'raises a clean user error for invalid URLs' do invalid_url = 'https://translate.wordpress.org/an invalid path' - downloader = described_class.new(url: invalid_url, locale: locale, auto_retry: false) + downloader = described_class.new(url: invalid_url, locale: locale, auto_retry: false, fail_on_error: true) expect { downloader.download { |body| body } }.to raise_error(FastlaneCore::Interface::FastlaneError, /Invalid URL for locale `test-locale`.*an invalid path/) end diff --git a/spec/gp_downloadmetadata_action_spec.rb b/spec/gp_downloadmetadata_action_spec.rb new file mode 100644 index 000000000..56b5f87d5 --- /dev/null +++ b/spec/gp_downloadmetadata_action_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Fastlane::Actions::GpDownloadmetadataAction do + it 'passes fail_on_error to the metadata downloader' do + in_tmp_dir do |tmpdir| + expect(Fastlane::Helper::MetadataDownloader).to receive(:new).with(tmpdir, {}, false, fail_on_error: true) + + run_described_fastlane_action( + project_url: 'https://translate.example/projects/test/', + target_files: {}, + locales: [], + source_locale: nil, + download_path: tmpdir, + auto_retry: false, + fail_on_error: true + ) + end + end + + it 'keeps fail_on_error disabled by default' do + option = described_class.available_options.find { |item| item.key == :fail_on_error } + + expect(option.default_value).to be(false) + end +end diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index 5de0063ab..4e226f45c 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -79,7 +79,8 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) run_described_fastlane_action( project_url: gp_fake_url, locales: { 'unknown-locale': 'Base' }, - download_dir: tmp_dir + download_dir: tmp_dir, + fail_on_error: true ) end @@ -109,6 +110,24 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) 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 invalid files without failing by default' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + body = 'some invalid strings file content' + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: body) + error_messages = [] + allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + expect(stub).to have_been_made.once + expect(File.read(file)).to eq(body) + expect(error_messages.first).to start_with('Error while validating the file exported from GlotPress') + end + end + [ ['invalid', 'some invalid strings file content', /Error while validating the file exported from GlotPress.*Property List error/m], ['empty', '', /file exported from GlotPress is empty/], @@ -121,7 +140,7 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) File.write(file, 'existing valid content') expect do - run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir, fail_on_error: true) end.to raise_error(FastlaneCore::Interface::FastlaneError, expected_error) expect(stub).to have_been_made.once @@ -141,7 +160,8 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) run_described_fastlane_action( project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, - download_dir: tmp_dir + download_dir: tmp_dir, + fail_on_error: true ) end.to raise_error(FastlaneCore::Interface::FastlaneError, /Found empty translations.*\["key2", "key3"\]/m) diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index dce78070a..e7e492ebb 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -390,7 +390,7 @@ def file_encoding(path) end describe 'invalid parameters' do - it 'raises if passed a non-existing locale (or any other 404)' 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/").with(query: { format: 'strings' }).to_return(status: [404, 'Not Found']) error_messages = [] @@ -398,26 +398,24 @@ def file_encoding(path) allow(FastlaneCore::UI).to receive(:confirm).and_return(false) # as we will be asked if we want to retry when getting a network error dest = StringIO.new # Act - expect do - described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest) - end.to raise_error(FastlaneCore::Interface::FastlaneError, /404 Not Found/) + 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 (#{gp_fake_url}/invalid/default/export-translations/?format=strings)"]) end - it 'raises if the destination cannot be written to' do + 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/").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' # Act - act = lambda do - described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest) - end + described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest) # Assert - expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, /Error writing downloaded locale `fr`/) 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} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) end end end diff --git a/spec/metadata_download_helper_spec.rb b/spec/metadata_download_helper_spec.rb index 40ee94d0b..724821caa 100644 --- a/spec/metadata_download_helper_spec.rb +++ b/spec/metadata_download_helper_spec.rb @@ -18,7 +18,7 @@ def existing_metadata_file(tmpdir) allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) in_tmp_dir do |tmpdir| - downloader = described_class.new(tmpdir, target_files, false) + downloader = described_class.new(tmpdir, target_files, false, fail_on_error: true) expect do downloader.download('fr', test_url, false) @@ -32,7 +32,7 @@ def existing_metadata_file(tmpdir) in_tmp_dir do |tmpdir| existing_file = existing_metadata_file(tmpdir) - downloader = described_class.new(tmpdir, target_files, false) + downloader = described_class.new(tmpdir, target_files, false, fail_on_error: true) expect { downloader.download('fr', test_url, false) }.to raise_error(FastlaneCore::Interface::FastlaneError, /GlotPress.*locale `fr`.*#{Regexp.escape(test_url)}/) expect(File.read(existing_file)).to eq('Existing release notes') @@ -45,11 +45,23 @@ def existing_metadata_file(tmpdir) in_tmp_dir do |tmpdir| existing_file = existing_metadata_file(tmpdir) - downloader = described_class.new(tmpdir, target_files, false) + downloader = described_class.new(tmpdir, target_files, false, fail_on_error: true) downloader.download('fr', test_url, false) expect(File).not_to exist(existing_file) end end + + it 'retains legacy malformed-response handling by default' do + stub_request(:get, test_url).to_return(status: 200, body: '{') + + in_tmp_dir do |tmpdir| + existing_file = existing_metadata_file(tmpdir) + downloader = described_class.new(tmpdir, target_files, false) + + expect { downloader.download('fr', test_url, false) }.not_to raise_error + expect(File).not_to exist(existing_file) + end + end end From 9d444ca68dbf7d04129579154af6d4ee6df7218a Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 22:00:36 +0200 Subject: [PATCH 04/10] Keep Buildkite test uploads enabled in specs --- spec/update_apps_cdn_build_metadata_spec.rb | 2 +- spec/upload_build_to_apps_cdn_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/update_apps_cdn_build_metadata_spec.rb b/spec/update_apps_cdn_build_metadata_spec.rb index 797ceaadb..94266fd34 100644 --- a/spec/update_apps_cdn_build_metadata_spec.rb +++ b/spec/update_apps_cdn_build_metadata_spec.rb @@ -22,7 +22,7 @@ end before do - WebMock.disable_net_connect! + WebMock.disable_net_connect!(allow: 'analytics-api.buildkite.com') end describe 'updating visibility' do diff --git a/spec/upload_build_to_apps_cdn_spec.rb b/spec/upload_build_to_apps_cdn_spec.rb index 96788708f..12fafef38 100644 --- a/spec/upload_build_to_apps_cdn_spec.rb +++ b/spec/upload_build_to_apps_cdn_spec.rb @@ -39,12 +39,12 @@ end before do - WebMock.disable_net_connect! + WebMock.disable_net_connect!(allow: 'analytics-api.buildkite.com') allow(SecureRandom).to receive(:hex).with(10).and_return('dabad0001234dabad000') end after do - WebMock.allow_net_connect! + WebMock.disable_net_connect!(allow: 'analytics-api.buildkite.com') end # Helper method to build the expected multipart form data part From 9b02da8b63861b579a3403ffede2b7b219f57538 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 22:03:56 +0200 Subject: [PATCH 05/10] Require explicit strict translation options --- .../actions/android/android_download_translations_action.rb | 1 - .../actions/common/gp_downloadmetadata_action.rb | 1 - .../ios/ios_download_strings_files_from_glotpress.rb | 1 - spec/android_download_translations_action_spec.rb | 1 + spec/gp_downloadmetadata_action_spec.rb | 1 + spec/ios_download_strings_files_from_glotpress_spec.rb | 6 ++++++ 6 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb index a60f86c73..24af78c6e 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb @@ -102,7 +102,6 @@ def self.available_options ), FastlaneCore::ConfigItem.new( key: :fail_on_error, - env_name: 'FL_DOWNLOAD_TRANSLATIONS_FAIL_ON_ERROR', description: 'Whether to fail when a GlotPress request or downloaded response is invalid', type: Boolean, default_value: false diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb index c34700574..25d475a4d 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb @@ -79,7 +79,6 @@ def self.available_options optional: true, default_value: true), FastlaneCore::ConfigItem.new(key: :fail_on_error, - env_name: 'FL_DOWNLOAD_METADATA_FAIL_ON_ERROR', description: 'Whether to fail when a GlotPress request or downloaded response is invalid', type: FastlaneCore::Boolean, optional: true, 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 b2a6bc9a4..3886e732f 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 @@ -135,7 +135,6 @@ def self.available_options optional: true, default_value: false), FastlaneCore::ConfigItem.new(key: :fail_on_error, - env_name: 'FL_IOS_DOWNLOAD_STRINGS_FILES_FROM_GLOTPRESS_FAIL_ON_ERROR', description: 'Whether to fail when a GlotPress request or downloaded response is invalid', type: Fastlane::Boolean, optional: true, diff --git a/spec/android_download_translations_action_spec.rb b/spec/android_download_translations_action_spec.rb index 55a8821be..b0604cd74 100644 --- a/spec/android_download_translations_action_spec.rb +++ b/spec/android_download_translations_action_spec.rb @@ -23,5 +23,6 @@ option = described_class.available_options.find { |item| item.key == :fail_on_error } expect(option.default_value).to be(false) + expect(option.env_name).to be_nil end end diff --git a/spec/gp_downloadmetadata_action_spec.rb b/spec/gp_downloadmetadata_action_spec.rb index 56b5f87d5..febf07685 100644 --- a/spec/gp_downloadmetadata_action_spec.rb +++ b/spec/gp_downloadmetadata_action_spec.rb @@ -23,5 +23,6 @@ option = described_class.available_options.find { |item| item.key == :fail_on_error } expect(option.default_value).to be(false) + expect(option.env_name).to be_nil end end diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index 4e226f45c..0c63852f7 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -12,6 +12,12 @@ def gp_stub(locale:, query:) stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations/").with(query: query) end + it 'does not expose fail_on_error through an environment variable' do + option = described_class.available_options.find { |item| item.key == :fail_on_error } + + expect(option.env_name).to be_nil + 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| From 8555126bf989d6fd35cdf5f1162001e004d25ab3 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 22:09:04 +0200 Subject: [PATCH 06/10] Harden iOS download acceptance --- .../ios_download_strings_files_from_glotpress.rb | 6 +++++- .../helper/glotpress_downloader.rb | 8 +++++--- .../helper/ios/ios_l10n_helper.rb | 2 +- spec/glotpress_downloader_spec.rb | 10 ++++++++++ ..._download_strings_files_from_glotpress_spec.rb | 15 +++++++++++++++ spec/ios_l10n_helper_spec.rb | 4 +++- 6 files changed, 39 insertions(+), 6 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 3886e732f..9c266f912 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,6 +30,10 @@ def self.run(params) next end + if File.exist?(destination) && !File.file?(destination) + UI.user_error!("The destination `#{destination}` exists but is not a regular file") + end + destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 Tempfile.create([params[:table_basename], '.strings'], lproj_dir) do |temporary_file| downloaded = Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( @@ -46,7 +50,7 @@ def self.run(params) validate_strings_file(temporary_file.path, display_path: destination, fail_on_error: true) unless params[:skip_file_validation] File.chmod(destination_mode, temporary_file.path) temporary_file.close - FileUtils.mv(temporary_file.path, destination) + File.rename(temporary_file.path, destination) end end end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb index f24c7502a..27ffbc543 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb @@ -34,7 +34,7 @@ def initialize(url:, locale:, auto_retry: false, fail_on_error: false) # @param [String] locale The locale being downloaded (for logging purposes) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) # @param [Boolean] fail_on_error Whether to raise a Fastlane error after retry handling instead of returning false - # @yield [String] The response body if the download was successful + # @yield [String] The response body if the download was successful. Return `false` to reject it and suppress the success message. # @return The result of the block if provided, or true/false indicating success if no block is provided # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling and `fail_on_error` is true # @@ -44,7 +44,7 @@ def self.download(url:, locale:, auto_retry: false, fail_on_error: false, &) # Downloads data from GlotPress # - # @yield [String] The response body if the download was successful + # @yield [String] The response body if the download was successful. Return `false` to reject it and suppress the success message. # @return The result of the block if provided, or true/false indicating success if no block is provided # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling and `fail_on_error` is true # @@ -101,7 +101,9 @@ def make_request(uri) def handle_response(response:, url:, original_uri:, redirect_count:, &) case response.code when '200' - yield(response.body) if block_given? + accepted = block_given? ? yield(response.body) : true + return false if accepted == false + UI.success("Successfully downloaded `#{@locale}`.") true when '301', '302', '307', '308' 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 269b4912e..dab636a18 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -212,7 +212,7 @@ def self.download_glotpress_export_file(project_url:, locale:, filters:, destina prefix = fail_on_error ? 'Error writing downloaded locale' : 'Error downloading locale' message = "#{prefix} `#{locale}` — #{e.message} (#{url})" fail_on_error ? UI.user_error!(message) : UI.error(message) - nil + false end end end diff --git a/spec/glotpress_downloader_spec.rb b/spec/glotpress_downloader_spec.rb index ef7847a2c..4faeae51e 100644 --- a/spec/glotpress_downloader_spec.rb +++ b/spec/glotpress_downloader_spec.rb @@ -39,6 +39,16 @@ end.to raise_error(RuntimeError, 'invalid downloaded body') end + it 'does not report success when the downloaded body is rejected' do + stub_request(:get, test_url) + .to_return(status: 200, body: 'rejected content') + expect(FastlaneCore::UI).not_to receive(:success) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect(downloader.download { false }).to be(false) + end + it 'resets retry counter at start of download' do # Counter should be reset to 0 at the start of download() call downloader = described_class.new(url: test_url, locale: locale, auto_retry: true) diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index 0c63852f7..0f112a10f 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -134,6 +134,21 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) end end + it 'raises before downloading if the strict destination is not a regular file' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + destination = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(destination) + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: '"key" = "value";') + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir, fail_on_error: true) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /destination.*not a regular file/i) + + expect(stub).not_to have_been_made + expect(Dir).to be_empty(destination) + end + end + [ ['invalid', 'some invalid strings file content', /Error while validating the file exported from GlotPress.*Property List error/m], ['empty', '', /file exported from GlotPress is empty/], diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index e7e492ebb..e658b518d 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -409,12 +409,14 @@ def file_encoding(path) 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) } + expect(FastlaneCore::UI).not_to receive(:success) 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) + result = 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(result).to be(false) expect(error_messages).to eq(["Error downloading locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) end end From a87b3f4811495649a44647cd152fd006d0097355 Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Tue, 18 Aug 2026 22:17:36 +0200 Subject: [PATCH 07/10] Centralize WebMock test configuration --- spec/update_apps_cdn_build_metadata_spec.rb | 4 ---- spec/upload_build_to_apps_cdn_spec.rb | 5 ----- 2 files changed, 9 deletions(-) diff --git a/spec/update_apps_cdn_build_metadata_spec.rb b/spec/update_apps_cdn_build_metadata_spec.rb index 94266fd34..b8531a8d2 100644 --- a/spec/update_apps_cdn_build_metadata_spec.rb +++ b/spec/update_apps_cdn_build_metadata_spec.rb @@ -21,10 +21,6 @@ }.to_json end - before do - WebMock.disable_net_connect!(allow: 'analytics-api.buildkite.com') - end - describe 'updating visibility' do it 'successfully updates the visibility to external' do stub_request(:post, api_url) diff --git a/spec/upload_build_to_apps_cdn_spec.rb b/spec/upload_build_to_apps_cdn_spec.rb index 12fafef38..d7c906bc1 100644 --- a/spec/upload_build_to_apps_cdn_spec.rb +++ b/spec/upload_build_to_apps_cdn_spec.rb @@ -39,14 +39,9 @@ end before do - WebMock.disable_net_connect!(allow: 'analytics-api.buildkite.com') allow(SecureRandom).to receive(:hex).with(10).and_return('dabad0001234dabad000') end - after do - WebMock.disable_net_connect!(allow: 'analytics-api.buildkite.com') - end - # Helper method to build the expected multipart form data part def expected_form_part(name:, value:, filename: nil) lines = ["--#{test_boundary}"] From f5d3883465eecd271a7237fdde0664a733006afc Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Wed, 19 Aug 2026 18:27:10 +0200 Subject: [PATCH 08/10] Validate downloaded iOS string dictionaries --- ...s_download_strings_files_from_glotpress.rb | 5 +++++ ...nload_strings_files_from_glotpress_spec.rb | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+) 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 9c266f912..5c699faf0 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 @@ -75,6 +75,11 @@ def self.validate_strings_file(path, display_path: path, fail_on_error: false) return end + unless translations.is_a?(Hash) && translations.all? { |key, value| key.is_a?(String) && value.is_a?(String) } + report_validation_error("The file exported from GlotPress is not a string-to-string dictionary (`#{display_path}`)", fail_on_error: fail_on_error) + return + end + empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort return if empty_keys.empty? diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index 0f112a10f..fc9215126 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -134,6 +134,24 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) end end + it 'reports non-dictionary files without failing by default' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + body = '("value")' + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: body) + error_messages = [] + allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + expect(stub).to have_been_made.once + expect(File.read(file)).to eq(body) + expect(error_messages).to contain_exactly(a_string_matching(/string-to-string dictionary.*#{Regexp.escape(file)}/)) + end + end + it 'raises before downloading if the strict destination is not a regular file' do Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| destination = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') @@ -152,6 +170,8 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) [ ['invalid', 'some invalid strings file content', /Error while validating the file exported from GlotPress.*Property List error/m], ['empty', '', /file exported from GlotPress is empty/], + ['not a dictionary', '("value")', /string-to-string dictionary/], + ['not string-to-string', '"key" = ("value");', /string-to-string dictionary/], ].each do |description, response_body, expected_error| it "raises without replacing the existing file if a download is #{description}" do Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| From d57f6724b53039300554c81419cc68e6d6b92bbf Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Wed, 19 Aug 2026 18:28:50 +0200 Subject: [PATCH 09/10] Consolidate atomic iOS translation downloads --- ...s_download_strings_files_from_glotpress.rb | 89 ++++++++++--------- .../helper/ios/ios_l10n_helper.rb | 3 +- ...nload_strings_files_from_glotpress_spec.rb | 34 +++++++ spec/ios_l10n_helper_spec.rb | 2 +- 4 files changed, 85 insertions(+), 43 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 5c699faf0..d36a0da35 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 @@ -19,51 +19,60 @@ def self.run(params) destination = File.join(lproj_dir, "#{params[:table_basename]}.strings") FileUtils.mkdir_p(lproj_dir) - unless params[:fail_on_error] - Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( - project_url: params[:project_url], - locale: glotpress_locale, - filters: params[:filters], - destination: destination - ) - validate_strings_file(destination) unless params[:skip_file_validation] - next - end - - if File.exist?(destination) && !File.file?(destination) - UI.user_error!("The destination `#{destination}` exists but is not a regular file") - end - - destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 - Tempfile.create([params[:table_basename], '.strings'], lproj_dir) do |temporary_file| - downloaded = Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( - project_url: params[:project_url], - locale: glotpress_locale, - filters: params[:filters], - destination: temporary_file, - fail_on_error: true - ) - next unless downloaded - - temporary_file.flush - # Do a quick check of the downloaded `.strings` file to ensure it looks valid - validate_strings_file(temporary_file.path, display_path: destination, fail_on_error: true) unless params[:skip_file_validation] - File.chmod(destination_mode, temporary_file.path) - temporary_file.close - File.rename(temporary_file.path, destination) - end + download_and_replace_strings_file( + project_url: params[:project_url], + locale: glotpress_locale, + filters: params[:filters], + destination: destination, + table_basename: params[:table_basename], + skip_file_validation: params[:skip_file_validation], + fail_on_error: params[:fail_on_error] + ) end end + def self.download_and_replace_strings_file(project_url:, locale:, filters:, destination:, table_basename:, skip_file_validation:, fail_on_error:) + if File.exist?(destination) && !File.file?(destination) + report_error("The destination `#{destination}` exists but is not a regular file", fail_on_error: fail_on_error) + return false + end + + destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 + Tempfile.create([table_basename, '.strings'], File.dirname(destination)) do |temporary_file| + downloaded = Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( + project_url: project_url, + locale: locale, + filters: filters, + destination: temporary_file, + fail_on_error: fail_on_error + ) + return false unless downloaded + + temporary_file.flush + # Do a quick check of the downloaded `.strings` file to ensure it looks valid + validate_strings_file(temporary_file.path, display_path: destination, fail_on_error: fail_on_error) unless skip_file_validation + File.chmod(destination_mode, temporary_file.path) + temporary_file.close + File.rename(temporary_file.path, destination) + end + true + rescue FastlaneCore::Interface::FastlaneError + raise + rescue StandardError => e + report_error("Error writing downloaded locale `#{locale}` — #{e.message} (#{destination})", fail_on_error: fail_on_error) + false + end + private_class_method :download_and_replace_strings_file + # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations def self.validate_strings_file(path, display_path: path, fail_on_error: false) unless File.exist?(path) - report_validation_error("The file exported from GlotPress was not created (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error + report_error("The file exported from GlotPress was not created (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error return end if File.empty?(path) - report_validation_error("The file exported from GlotPress is empty (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error + report_error("The file exported from GlotPress is empty (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error return end @@ -71,19 +80,19 @@ def self.validate_strings_file(path, display_path: path, fail_on_error: false) begin translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path) rescue StandardError => e - report_validation_error("Error while validating the file exported from GlotPress (`#{display_path}`) - #{e.message.chomp}", fail_on_error: fail_on_error) + report_error("Error while validating the file exported from GlotPress (`#{display_path}`) - #{e.message.chomp}", fail_on_error: fail_on_error) return end unless translations.is_a?(Hash) && translations.all? { |key, value| key.is_a?(String) && value.is_a?(String) } - report_validation_error("The file exported from GlotPress is not a string-to-string dictionary (`#{display_path}`)", fail_on_error: fail_on_error) + report_error("The file exported from GlotPress is not a string-to-string dictionary (`#{display_path}`)", fail_on_error: fail_on_error) return end empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort return if empty_keys.empty? - report_validation_error( + report_error( "Found empty translations in `#{display_path}` 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.', @@ -91,10 +100,10 @@ def self.validate_strings_file(path, display_path: path, fail_on_error: false) ) end - def self.report_validation_error(message, fail_on_error:) + def self.report_error(message, fail_on_error:) fail_on_error ? UI.user_error!(message) : UI.error(message) end - private_class_method :report_validation_error + private_class_method :report_error ##################################################### # @!group Documentation 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 dab636a18..d6aeaab0d 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -209,8 +209,7 @@ def self.download_glotpress_export_file(project_url:, locale:, filters:, destina destination.write(response_body) end rescue StandardError => e - prefix = fail_on_error ? 'Error writing downloaded locale' : 'Error downloading locale' - message = "#{prefix} `#{locale}` — #{e.message} (#{url})" + message = "Error writing downloaded locale `#{locale}` — #{e.message} (#{url})" fail_on_error ? UI.user_error!(message) : UI.error(message) false end diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index fc9215126..529d24293 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -116,6 +116,23 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) 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 'does not replace an existing file when a permissive download fails' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(status: [500, 'Internal Server Error']) + allow(FastlaneCore::UI).to receive(:confirm).and_return(false) + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(File.dirname(file)) + File.write(file, 'existing valid content') + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + + expect(stub).to have_been_made.once + expect(File.read(file)).to eq('existing valid content') + end + end + it 'reports invalid files without failing by default' do Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| body = 'some invalid strings file content' @@ -167,6 +184,23 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) end end + it 'reports a non-regular destination without failing by default' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + destination = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(destination) + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: '"key" = "value";') + error_messages = [] + allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + + expect(stub).not_to have_been_made + expect(error_messages).to eq(["The destination `#{destination}` exists but is not a regular file"]) + end + end + [ ['invalid', 'some invalid strings file content', /Error while validating the file exported from GlotPress.*Property List error/m], ['empty', '', /file exported from GlotPress is empty/], diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index e658b518d..ab8bdbb96 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -417,7 +417,7 @@ def file_encoding(path) expect(stub).to have_been_made.once expect(File).not_to exist(dest) expect(result).to be(false) - expect(error_messages).to eq(["Error downloading locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) + expect(error_messages).to eq(["Error writing downloaded locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) end end end From 35fa64aa16de185fd304781aac96bb1f5c909b6a Mon Sep 17 00:00:00 2001 From: Ian Maia Date: Wed, 19 Aug 2026 20:14:08 +0200 Subject: [PATCH 10/10] Consolidate opt-in failure configuration --- .../android_download_translations_action.rb | 9 +++------ .../actions/common/get_prs_between_tags.rb | 11 ++--------- .../actions/common/gp_downloadmetadata_action.rb | 7 ++----- .../ios_download_strings_files_from_glotpress.rb | 7 ++----- .../helper/config_item_helper.rb | 15 +++++++++++++++ 5 files changed, 24 insertions(+), 25 deletions(-) create mode 100644 lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb index 24af78c6e..35957fbb9 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative '../../helper/config_item_helper' + # This action is the new version of android_update_metadata (AndroidUpdateMetadataAction) and should now be used instead of that one module Fastlane @@ -100,12 +102,7 @@ def self.available_options type: Boolean, default_value: false ), - FastlaneCore::ConfigItem.new( - key: :fail_on_error, - description: 'Whether to fail when a GlotPress request or downloaded response is invalid', - type: Boolean, - default_value: false - ), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb index e68104c01..c988cf5e9 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'fastlane/action' +require_relative '../../helper/config_item_helper' require_relative '../../helper/github_helper' module Fastlane @@ -104,15 +105,7 @@ def self.available_options 'See https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes#configuration-options', optional: true, type: String), - FastlaneCore::ConfigItem.new(key: :fail_on_error, - description: 'Whether to fail the lane if the changelog cannot be computed. ' \ - 'When `false` (the default), the error message is returned as the changelog itself, ' \ - 'so that it ends up visible in the GitHub Release body. ' \ - 'Set this to `true` if the caller publishes the release only after this action succeeds, ' \ - 'and would rather stop than publish a release whose notes are an error message', - optional: true, - default_value: false, - type: Boolean), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), Fastlane::Helper::GithubHelper.github_token_config_item, ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb index 25d475a4d..a4cc4500f 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb @@ -2,6 +2,7 @@ require 'fastlane/action' require_relative '../../helper/metadata_download_helper' +require_relative '../../helper/config_item_helper' module Fastlane module Actions @@ -78,11 +79,7 @@ def self.available_options type: FastlaneCore::Boolean, optional: true, default_value: true), - FastlaneCore::ConfigItem.new(key: :fail_on_error, - description: 'Whether to fail when a GlotPress request or downloaded response is invalid', - type: FastlaneCore::Boolean, - optional: true, - default_value: false), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), ] end 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 d36a0da35..deef7e604 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,7 @@ # frozen_string_literal: true require 'tempfile' +require_relative '../../helper/config_item_helper' module Fastlane module Actions @@ -152,11 +153,7 @@ def self.available_options type: Fastlane::Boolean, optional: true, default_value: false), - FastlaneCore::ConfigItem.new(key: :fail_on_error, - description: 'Whether to fail when a GlotPress request or downloaded response is invalid', - type: Fastlane::Boolean, - optional: true, - default_value: false), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb new file mode 100644 index 000000000..6fe8392fa --- /dev/null +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Fastlane + module Helper + class ConfigItemHelper + OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS = { + key: :fail_on_error, + description: 'Whether handled errors should fail the lane instead of using the action-specific fallback behavior', + type: FastlaneCore::Boolean, + optional: true, + default_value: false + }.freeze + end + end +end