From b030a198a33f08d566bca5ae0d5835760f0b080a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 30 Jan 2023 17:20:08 +0100 Subject: [PATCH 1/4] Include SentencesParser inside the right class We were including it in the Object class, making its methods available everywhere. --- lib/remote_translations/microsoft/client.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/remote_translations/microsoft/client.rb b/lib/remote_translations/microsoft/client.rb index a4b256fda..e9c4b73b1 100644 --- a/lib/remote_translations/microsoft/client.rb +++ b/lib/remote_translations/microsoft/client.rb @@ -1,7 +1,7 @@ require "translator-text" -include RemoteTranslations::Microsoft::SentencesParser class RemoteTranslations::Microsoft::Client + include SentencesParser CHARACTERS_LIMIT_PER_REQUEST = 5000 PREVENTING_TRANSLATION_KEY = "notranslate".freeze From 63d0e316cf5b25ba9076cb695697caa0ec63e51a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 30 Jan 2023 17:55:53 +0100 Subject: [PATCH 2/4] Replace instance variable usage with a method We usually use this approach because methods are easier to override and stub. --- lib/remote_translations/microsoft/client.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/remote_translations/microsoft/client.rb b/lib/remote_translations/microsoft/client.rb index e9c4b73b1..fd5fb4f8e 100644 --- a/lib/remote_translations/microsoft/client.rb +++ b/lib/remote_translations/microsoft/client.rb @@ -5,11 +5,6 @@ class RemoteTranslations::Microsoft::Client CHARACTERS_LIMIT_PER_REQUEST = 5000 PREVENTING_TRANSLATION_KEY = "notranslate".freeze - def initialize - api_key = Tenant.current_secrets.microsoft_api_key - @client = TranslatorText::Client.new(api_key) - end - def call(fields_values, locale) texts = prepare_texts(fields_values) valid_locale = RemoteTranslations::Microsoft::AvailableLocales.parse_locale(locale) @@ -28,12 +23,16 @@ class RemoteTranslations::Microsoft::Client private + def client + @client ||= TranslatorText::Client.new(Tenant.current_secrets.microsoft_api_key) + end + def request_translation(texts, locale) response = [] split_response = false if characters_count(texts) <= CHARACTERS_LIMIT_PER_REQUEST - response = @client.translate(texts, to: locale) + response = client.translate(texts, to: locale) else texts.each do |text| response << translate_text(text, locale) @@ -46,7 +45,7 @@ class RemoteTranslations::Microsoft::Client def translate_text(text, locale) fragments_for(text).map do |fragment| - @client.translate([fragment], to: locale) + client.translate([fragment], to: locale) end.flatten end From c64b49b128ddca0b0df18fde13f029e855702555 Mon Sep 17 00:00:00 2001 From: taitus Date: Thu, 9 Mar 2023 06:00:41 +0100 Subject: [PATCH 3/4] Change gem from TranslatorText to BingTranslator TranslatorText isn't compatible with Ruby 3, so we need to use a different gem. The 'translator-text' gem response was an array of one or more objects. Now with the 'bing_translator' gem the response is an array with one or several translated texts. We remove the concept of object in the code. And we also remove the "create_response" method from the specs since it is no longer necessary to emulate that object and we can simply use arrays with texts to emulate the new response. --- Gemfile | 2 +- Gemfile.lock | 33 ++---------- lib/remote_translations/microsoft/client.rb | 23 ++++---- .../microsoft/client_spec.rb | 53 +++++++------------ 4 files changed, 34 insertions(+), 77 deletions(-) diff --git a/Gemfile b/Gemfile index fe6ea1101..1546de3b6 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ gem "airbrake", "~> 13.0.2" gem "ancestry", "~> 4.2.0" gem "audited", "~> 5.0.2" gem "autoprefixer-rails", "~> 8.2.0" +gem "bing_translator", "~> 6.2.0" gem "cancancan", "~> 3.4.0" gem "caxlsx", "~> 3.2.0" gem "caxlsx_rails", "~> 0.6.3" @@ -55,7 +56,6 @@ gem "savon", "~> 2.13.0" gem "sitemap_generator", "~> 6.3.0" gem "social-share-button", "~> 1.2.4" gem "sprockets", "~> 4.1.1" -gem "translator-text", "~> 0.1.0" gem "turbolinks", "~> 5.2.1" gem "turnout", "~> 2.5.0" gem "uglifier", "~> 4.2.0" diff --git a/Gemfile.lock b/Gemfile.lock index 15dcee1a8..ce74441b9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -96,6 +96,8 @@ GEM parser (>= 2.4) smart_properties bindex (0.8.1) + bing_translator (6.2.0) + json builder (3.2.4) bullet (7.0.3) activesupport (>= 3.0.0) @@ -178,31 +180,6 @@ GEM devise (>= 4.3.0, < 5.0) diff-lcs (1.5.0) docile (1.4.0) - dry-configurable (0.7.0) - concurrent-ruby (~> 1.0) - dry-container (0.6.0) - concurrent-ruby (~> 1.0) - dry-configurable (~> 0.1, >= 0.1.3) - dry-core (0.4.7) - concurrent-ruby (~> 1.0) - dry-equalizer (0.2.1) - dry-inflector (0.1.2) - dry-logic (0.4.2) - dry-container (~> 0.2, >= 0.2.6) - dry-core (~> 0.2) - dry-equalizer (~> 0.2) - dry-struct (0.5.1) - dry-core (~> 0.4, >= 0.4.3) - dry-equalizer (~> 0.2) - dry-types (~> 0.13) - ice_nine (~> 0.11) - dry-types (0.13.3) - concurrent-ruby (~> 1.0) - dry-container (~> 0.3) - dry-core (~> 0.4, >= 0.4.4) - dry-equalizer (~> 0.2) - dry-inflector (~> 0.1, >= 0.1.2) - dry-logic (~> 0.4, >= 0.4.2) email_spec (2.2.0) htmlentities (~> 4.3.3) launchy (~> 2.1) @@ -309,7 +286,6 @@ GEM rails-i18n rainbow (>= 2.2.2, < 4.0) terminal-table (>= 1.5.1) - ice_nine (0.11.2) image_processing (1.12.2) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) @@ -640,9 +616,6 @@ GEM thread_safe (0.3.6) tilt (2.0.10) tomlrb (1.3.0) - translator-text (0.1.0) - dry-struct (~> 0.5.0) - httparty (~> 0.15) turbolinks (5.2.1) turbolinks-source (~> 5.2) turbolinks-source (5.2.0) @@ -701,6 +674,7 @@ DEPENDENCIES ancestry (~> 4.2.0) audited (~> 5.0.2) autoprefixer-rails (~> 8.2.0) + bing_translator (~> 6.2.0) bullet (~> 7.0.3) byebug (~> 11.1.3) cancancan (~> 3.4.0) @@ -785,7 +759,6 @@ DEPENDENCIES spring (~> 2.1.1) spring-commands-rspec (~> 1.0.4) sprockets (~> 4.1.1) - translator-text (~> 0.1.0) turbolinks (~> 5.2.1) turnout (~> 2.5.0) uglifier (~> 4.2.0) diff --git a/lib/remote_translations/microsoft/client.rb b/lib/remote_translations/microsoft/client.rb index fd5fb4f8e..cbe856de3 100644 --- a/lib/remote_translations/microsoft/client.rb +++ b/lib/remote_translations/microsoft/client.rb @@ -1,5 +1,3 @@ -require "translator-text" - class RemoteTranslations::Microsoft::Client include SentencesParser CHARACTERS_LIMIT_PER_REQUEST = 5000 @@ -24,7 +22,7 @@ class RemoteTranslations::Microsoft::Client private def client - @client ||= TranslatorText::Client.new(Tenant.current_secrets.microsoft_api_key) + @client ||= BingTranslator.new(Tenant.current_secrets.microsoft_api_key) end def request_translation(texts, locale) @@ -32,7 +30,7 @@ class RemoteTranslations::Microsoft::Client split_response = false if characters_count(texts) <= CHARACTERS_LIMIT_PER_REQUEST - response = client.translate(texts, to: locale) + response = client.translate_array(texts, to: locale) else texts.each do |text| response << translate_text(text, locale) @@ -45,27 +43,26 @@ class RemoteTranslations::Microsoft::Client def translate_text(text, locale) fragments_for(text).map do |fragment| - client.translate([fragment], to: locale) + client.translate_array([fragment], to: locale) end.flatten end def parse_response(response, split_response) - response.map do |object| + response.map do |translation| if split_response - build_translation(object) + build_translation(translation) else - get_field_value(object) + get_field_value(translation) end end end - def build_translation(objects) - objects.map { |object| get_field_value(object) }.join + def build_translation(translations) + translations.map { |translation| get_field_value(translation) }.join end - def get_field_value(object) - text = object.translations[0].text - notranslate?(text) ? nil : text + def get_field_value(translation) + notranslate?(translation) ? nil : translation end def prepare_texts(texts) diff --git a/spec/lib/remote_translations/microsoft/client_spec.rb b/spec/lib/remote_translations/microsoft/client_spec.rb index e478e7dfb..903a2ad21 100644 --- a/spec/lib/remote_translations/microsoft/client_spec.rb +++ b/spec/lib/remote_translations/microsoft/client_spec.rb @@ -6,9 +6,9 @@ describe RemoteTranslations::Microsoft::Client do describe "#call" do context "when characters from request are less than the characters limit" do it "response has the expected result" do - response = create_response("Nuevo título", "Nueva descripción") + response = ["Nuevo título", "Nueva descripción"] - expect_any_instance_of(TranslatorText::Client).to receive(:translate).and_return(response) + expect_any_instance_of(BingTranslator).to receive(:translate_array).and_return(response) result = client.call(["New title", "New description"], :es) @@ -16,9 +16,9 @@ describe RemoteTranslations::Microsoft::Client do end it "response nil has the expected result when request has nil value" do - response = create_response("Notranslate", "Nueva descripción") + response = ["Notranslate", "Nueva descripción"] - expect_any_instance_of(TranslatorText::Client).to receive(:translate).and_return(response) + expect_any_instance_of(BingTranslator).to receive(:translate_array).and_return(response) result = client.call([nil, "New description"], :es) @@ -34,15 +34,15 @@ describe RemoteTranslations::Microsoft::Client do translated_text_es = Faker::Lorem.characters(number: 11) another_translated_text_es = Faker::Lorem.characters(number: 11) - response_text = create_response(translated_text_es) - response_another_text = create_response(another_translated_text_es) + response_text = [translated_text_es] + response_another_text = [another_translated_text_es] - expect_any_instance_of(TranslatorText::Client).to receive(:translate).exactly(1) - .times - .and_return(response_text) - expect_any_instance_of(TranslatorText::Client).to receive(:translate).exactly(1) - .times - .and_return(response_another_text) + expect_any_instance_of(BingTranslator).to receive(:translate_array).exactly(1) + .times + .and_return(response_text) + expect_any_instance_of(BingTranslator).to receive(:translate_array).exactly(1) + .times + .and_return(response_another_text) result = client.call([text_en, another_text_en], :es) @@ -58,14 +58,14 @@ describe RemoteTranslations::Microsoft::Client do start_translated_text_es = Faker::Lorem.characters(number: 10) + " " end_translated_text_es = Faker::Lorem.characters(number: 10) translated_text_es = start_translated_text_es + end_translated_text_es - response_start_text = create_response(start_translated_text_es) - response_end_text = create_response(end_translated_text_es) + response_start_text = [start_translated_text_es] + response_end_text = [end_translated_text_es] - expect_any_instance_of(TranslatorText::Client).to receive(:translate).with([start_text_en], to: :es) + expect_any_instance_of(BingTranslator).to receive(:translate_array).with([start_text_en], to: :es) .exactly(1) .times .and_return(response_start_text) - expect_any_instance_of(TranslatorText::Client).to receive(:translate).with([end_text_en], to: :es) + expect_any_instance_of(BingTranslator).to receive(:translate_array).with([end_text_en], to: :es) .exactly(1) .times .and_return(response_end_text) @@ -77,14 +77,14 @@ describe RemoteTranslations::Microsoft::Client do another_start_translated_text_es = Faker::Lorem.characters(number: 12) + "." another_end_translated_text_es = Faker::Lorem.characters(number: 12) another_translated_text_es = another_start_translated_text_es + another_end_translated_text_es - response_another_start_text = create_response(another_start_translated_text_es) - response_another_end_text = create_response(another_end_translated_text_es) + response_another_start_text = [another_start_translated_text_es] + response_another_end_text = [another_end_translated_text_es] - expect_any_instance_of(TranslatorText::Client).to receive(:translate).with([start_another_text_en], to: :es) + expect_any_instance_of(BingTranslator).to receive(:translate_array).with([start_another_text_en], to: :es) .exactly(1) .times .and_return(response_another_start_text) - expect_any_instance_of(TranslatorText::Client).to receive(:translate).with([end_another_text_en], to: :es) + expect_any_instance_of(BingTranslator).to receive(:translate_array).with([end_another_text_en], to: :es) .exactly(1) .times .and_return(response_another_end_text) @@ -146,16 +146,3 @@ describe RemoteTranslations::Microsoft::Client do end end end - -def create_response(*args) - # response = [#] detectedLanguage={"language"=>"en", "score"=>1.0}>, #] detectedLanguage={"language"=>"en", "score"=>1.0}>] - translations = Struct.new(:translations) - text = Struct.new(:text) - response = [] - - args.each do |text_to_response| - response << translations.new([text.new(text_to_response)]) - end - - response -end From 306e7356c3a57153109594aa3b4ee2cb72fe0c3b Mon Sep 17 00:00:00 2001 From: taitus Date: Thu, 9 Mar 2023 09:00:41 +0100 Subject: [PATCH 4/4] Allow translate locales that need to be mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It has been detected that for the :pt-BR, :zh-CN and :zh-TW locales, the translate button was being displayed, but when requesting the translation, the remote translation validation failed due to: ''' validates :locale, inclusion: { in: ->(_) { RemoteTranslations::Microsoft::AvailableLocales.available_locales }} ''' That available_locales method did not contemplate these 3 languages in the format used by the application. To solve this problem the api response is mapped to return all locales in the format expected by the application. Add remote translation model test to ensure that a remote translation is valid when its locale is pt-BR. Co-Authored-By: Javi Martín <35156+javierm@users.noreply.github.com> --- .../microsoft/available_locales.rb | 31 ++++++++++--------- lib/remote_translations/microsoft/client.rb | 2 +- ...mote_translations_button_component_spec.rb | 2 +- .../microsoft/available_locales_spec.rb | 13 ++++++++ spec/models/remote_translation_spec.rb | 10 ++++++ 5 files changed, 42 insertions(+), 16 deletions(-) create mode 100644 spec/lib/remote_translations/microsoft/available_locales_spec.rb diff --git a/lib/remote_translations/microsoft/available_locales.rb b/lib/remote_translations/microsoft/available_locales.rb index 21ebf05cd..2bc39d2e3 100644 --- a/lib/remote_translations/microsoft/available_locales.rb +++ b/lib/remote_translations/microsoft/available_locales.rb @@ -6,29 +6,32 @@ require "json" class RemoteTranslations::Microsoft::AvailableLocales def self.available_locales daily_cache("locales") do - remote_available_locales.map(&:first) + remote_available_locales.map { |locale| remote_locale_to_app_locale(locale) } end end - def self.parse_locale(locale) - case locale - when :"pt-BR" - :pt - when :"zh-CN" - :"zh-Hans" - when :"zh-TW" - :"zh-Hant" - else - locale - end + def self.app_locale_to_remote_locale(locale) + app_locale_to_remote_locale_map[locale] || locale end def self.include_locale?(locale) - available_locales.include?(parse_locale(locale).to_s) + available_locales.include?(locale.to_s) end private + def self.remote_locale_to_app_locale(locale) + app_locale_to_remote_locale_map.invert[locale] || locale + end + + def self.app_locale_to_remote_locale_map + { + "pt-BR" => "pt", + "zh-CN" => "zh-Hans", + "zh-TW" => "zh-Hant" + } + end + def self.remote_available_locales host = "https://api.cognitive.microsofttranslator.com" path = "/languages?api-version=3.0" @@ -44,7 +47,7 @@ class RemoteTranslations::Microsoft::AvailableLocales result = response.body.force_encoding("utf-8") - JSON.parse(result)["translation"] + JSON.parse(result)["translation"].map(&:first) end def self.daily_cache(key, &block) diff --git a/lib/remote_translations/microsoft/client.rb b/lib/remote_translations/microsoft/client.rb index cbe856de3..ad17adccd 100644 --- a/lib/remote_translations/microsoft/client.rb +++ b/lib/remote_translations/microsoft/client.rb @@ -5,7 +5,7 @@ class RemoteTranslations::Microsoft::Client def call(fields_values, locale) texts = prepare_texts(fields_values) - valid_locale = RemoteTranslations::Microsoft::AvailableLocales.parse_locale(locale) + valid_locale = RemoteTranslations::Microsoft::AvailableLocales.app_locale_to_remote_locale(locale) request_translation(texts, valid_locale) end diff --git a/spec/components/layout/remote_translations_button_component_spec.rb b/spec/components/layout/remote_translations_button_component_spec.rb index a427b4a64..541a57835 100644 --- a/spec/components/layout/remote_translations_button_component_spec.rb +++ b/spec/components/layout/remote_translations_button_component_spec.rb @@ -5,7 +5,7 @@ describe Layout::RemoteTranslationsButtonComponent do let(:component) { Layout::RemoteTranslationsButtonComponent.new(translations) } before do - allow(RemoteTranslations::Microsoft::AvailableLocales).to receive(:available_locales) + allow(RemoteTranslations::Microsoft::AvailableLocales).to receive(:remote_available_locales) .and_return(%w[de en es fr pt zh-Hans]) end diff --git a/spec/lib/remote_translations/microsoft/available_locales_spec.rb b/spec/lib/remote_translations/microsoft/available_locales_spec.rb new file mode 100644 index 000000000..421ef606a --- /dev/null +++ b/spec/lib/remote_translations/microsoft/available_locales_spec.rb @@ -0,0 +1,13 @@ +require "rails_helper" + +describe RemoteTranslations::Microsoft::AvailableLocales do + describe ".available_locales" do + it "includes locales with the same format as I18n.available_locales" do + allow(RemoteTranslations::Microsoft::AvailableLocales).to receive(:remote_available_locales) + .and_return(%w[de en es fr pt zh-Hans]) + + available_locales = RemoteTranslations::Microsoft::AvailableLocales.available_locales + expect(available_locales).to eq %w[de en es fr pt-BR zh-CN] + end + end +end diff --git a/spec/models/remote_translation_spec.rb b/spec/models/remote_translation_spec.rb index ce8508898..8ce808ce5 100644 --- a/spec/models/remote_translation_spec.rb +++ b/spec/models/remote_translation_spec.rb @@ -49,6 +49,16 @@ describe RemoteTranslation, :remote_translations do expect(remote_translation).to be_valid end + it "is valid with a locale that uses a different name in the remote service" do + allow(RemoteTranslations::Microsoft::AvailableLocales).to receive(:available_locales).and_call_original + allow(RemoteTranslations::Microsoft::AvailableLocales).to receive(:remote_available_locales) + .and_return(["pt"]) + + remote_translation.locale = :"pt-BR" + + expect(remote_translation).to be_valid + end + describe "#enqueue_remote_translation", :delay_jobs do it "after create enqueue Delayed Job" do expect { remote_translation.save }.to change { Delayed::Job.count }.by(1)