FIX: Don't double cook (#297)

This PR fixes the bug where we were double-cooking when saving `PostLocalization` when the AI provider is used.

The reason is due to the fact that we used to translate raw, then cook, and save the cooked. But now, we are required to save both the raw and cooked into `PostLocalization`.

This PR also splits `translate_translatable` into `translate_post` and `translate_topic`. Originally they were taking the same route, but it was proving to be a bit annoying because post uses raw and cooked, whereas topic uses title. It makes understanding any issues with a single model (e.g. Posts) easier to understand if it is a single code path. Lastly, PR also fixes an Amazon provider bug as it was never tested.

Author: nattsw

app/models/concerns/discourse_translator/translatable.rb

@@ -15,6 +15,7 @@ def set_detected_locale(locale)
       # locales should be "en-US" instead of "en_US" per https://www.rfc-editor.org/rfc/rfc5646#section-2.1
       locale = locale.to_s.gsub("_", "-")
       (content_locale || build_content_locale).update!(detected_locale: locale)
+      locale
     end
 
     # This method is used to create a translation for a translatable (Post or Topic) and a specific locale.

app/services/discourse_translator/provider/amazon.rb

@@ -120,38 +120,25 @@ def self.detect!(topic_or_post)
         end
       end
 
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        detected_lang = detect(translatable)
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
+        raw = opts.key?(:raw) ? opts[:raw] : !opts[:cooked]
+        text = text_for_translation(post, raw:)
+        translate_text!(text, target_locale_sym)
+      end
 
-        begin
-          client.translate_text(
-            {
-              text: truncate(text_for_translation(translatable)),
-              source_language_code: "auto",
-              target_language_code: SUPPORTED_LANG_MAPPING[target_locale_sym],
-            },
-          )
-        rescue Aws::Translate::Errors::UnsupportedLanguagePairException
-          raise I18n.t(
-                  "translator.failed.#{translatable.class.name.downcase}",
-                  source_locale: detected_lang,
-                  target_locale: target_locale_sym,
-                )
-        end
+      def self.translate_topic!(topic, target_locale_sym = I18n.locale)
+        text = text_for_translation(topic)
+        translate_text!(text, target_locale_sym)
       end
 
       def self.translate_text!(text, target_locale_sym = I18n.locale)
-        begin
-          client.translate_text(
-            {
-              text: truncate(text),
-              source_language_code: "auto",
-              target_language_code: SUPPORTED_LANG_MAPPING[target_locale_sym],
-            },
-          )
-        rescue Aws::Translate::Errors::UnsupportedLanguagePairException
-          raise I18n.t("translator.not_supported")
-        end
+        client.translate_text(
+          {
+            text: truncate(text),
+            source_language_code: "auto",
+            target_language_code: SUPPORTED_LANG_MAPPING[target_locale_sym],
+          },
+        )&.translated_text
       end
 
       def self.client

app/services/discourse_translator/provider/base_provider.rb

@@ -25,12 +25,11 @@ def self.cache_key
         "#{key_prefix}#{access_token_key}"
       end
 
-      # Returns the stored translation of a post or topic.
-      # If the translation does not exist yet, it will be translated first via the API then stored.
+      # Translates and saves it into a PostTranslation/TopicTranslation
       # If the detected language is the same as the target language, the original text will be returned.
       # @param translatable [Post|Topic]
+      # @return [Array] the detected language and the translated text
       def self.translate(translatable, target_locale_sym = I18n.locale)
-        return if text_for_translation(translatable).blank?
         detected_lang = detect(translatable)
 
         if translatable.locale_matches?(target_locale_sym)
@@ -50,35 +49,46 @@ def self.translate(translatable, target_locale_sym = I18n.locale)
                 )
         end
 
-        translated = translate_translatable!(translatable, target_locale_sym)
-        save_translation(translatable, target_locale_sym) { translated }
-        [detected_lang, translated]
-      end
+        begin
+          begin
+            translated =
+              case translatable.class.name
+              when "Post"
+                translate_post!(translatable, target_locale_sym, { cooked: true })
+              when "Topic"
+                translate_topic!(translatable, target_locale_sym)
+              end
+          end
+        rescue => e
+          raise I18n.t(
+                  "translator.failed.#{translatable.class.name.downcase}",
+                  source_locale: detected_lang,
+                  target_locale: target_locale_sym,
+                )
+        end
 
-      # TODO: Deprecate this in favour of translate_<model>
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        raise "Not Implemented"
+        translatable.set_translation(target_locale_sym, translated)
+        [detected_lang, translated]
       end
 
       def self.translate_text!(text, target_locale_sym = I18n.locale)
         raise "Not Implemented"
       end
 
-      def self.translate_post!(post, target_locale_sym = I18n.locale)
-        translate_translatable!(post, target_locale_sym)
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
+        raise "Not Implemented"
       end
 
       def self.translate_topic!(topic, target_locale_sym = I18n.locale)
-        translate_translatable!(topic, target_locale_sym)
+        raise "Not Implemented"
       end
 
       # Returns the stored detected locale of a post or topic.
       # If the locale does not exist yet, it will be detected first via the API then stored.
       # @param translatable [Post|Topic]
       def self.detect(translatable)
         return if text_for_detection(translatable).blank?
-        get_detected_locale(translatable) ||
-          save_detected_locale(translatable) { detect!(translatable) }
+        translatable.detected_locale || translatable.set_detected_locale(detect!(translatable))
       end
 
       # Subclasses must implement this method to detect the text of a post or topic
@@ -94,29 +104,6 @@ def self.access_token
         raise "Not Implemented"
       end
 
-      def self.save_translation(translatable, target_locale_sym = I18n.locale)
-        begin
-          translation = yield
-        rescue Timeout::Error
-          raise TranslatorError.new(I18n.t("translator.api_timeout"))
-        end
-        translatable.set_translation(target_locale_sym, translation)
-        translation
-      end
-
-      def self.get_detected_locale(translatable)
-        translatable.detected_locale
-      end
-
-      def self.save_detected_locale(translatable)
-        # sometimes we may have a user post that is just an emoji
-        # in that case, we will just indicate the post is in the default locale
-        detected_locale = yield.presence || SiteSetting.default_locale
-        translatable.set_detected_locale(detected_locale)
-
-        detected_locale
-      end
-
       def self.language_supported?(detected_lang)
         raise NotImplementedError unless self.const_defined?(:SUPPORTED_LANG_MAPPING)
         supported_lang = const_get(:SUPPORTED_LANG_MAPPING)

app/services/discourse_translator/provider/discourse_ai.rb

@@ -15,33 +15,22 @@ def self.detect!(topic_or_post)
         ::DiscourseAi::LanguageDetector.new(text_for_detection(topic_or_post)).detect
       end
 
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        if (translatable.class.name == "Post")
-          translate_post!(translatable, target_locale_sym)
-        elsif (translatable.class.name == "Topic")
-          translate_topic!(translatable, target_locale_sym)
-        end
-      end
-
-      def self.translate_post!(post, target_locale_sym = I18n.locale)
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
         validate_required_settings!
 
-        text = text_for_translation(post, raw: true)
+        raw = opts.key?(:raw) ? opts[:raw] : !opts[:cooked]
+        text = text_for_translation(post, raw:)
         chunks = DiscourseTranslator::ContentSplitter.split(text)
-        translated =
-          chunks
-            .map { |chunk| ::DiscourseAi::PostTranslator.new(chunk, target_locale_sym).translate }
-            .join("")
-        DiscourseTranslator::TranslatedContentNormalizer.normalize(post, translated)
+        chunks
+          .map { |chunk| ::DiscourseAi::PostTranslator.new(chunk, target_locale_sym).translate }
+          .join("")
       end
 
       def self.translate_topic!(topic, target_locale_sym = I18n.locale)
         validate_required_settings!
 
         language = get_language_name(target_locale_sym)
-        translated =
-          ::DiscourseAi::TopicTranslator.new(text_for_translation(topic), language).translate
-        DiscourseTranslator::TranslatedContentNormalizer.normalize(topic, translated)
+        ::DiscourseAi::TopicTranslator.new(text_for_translation(topic), language).translate
       end
 
       def self.translate_text!(text, target_locale_sym = I18n.locale)

app/services/discourse_translator/provider/google.rb

@@ -96,14 +96,15 @@ def self.translate_supported?(source, target)
         end
       end
 
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        res =
-          result(
-            TRANSLATE_URI,
-            q: text_for_translation(translatable),
-            target: SUPPORTED_LANG_MAPPING[target_locale_sym],
-          )
-        res["translations"][0]["translatedText"]
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
+        raw = opts.key?(:raw) ? opts[:raw] : !opts[:cooked]
+        text = text_for_translation(post, raw:)
+        translate_text!(text, target_locale_sym)
+      end
+
+      def self.translate_topic!(topic, target_locale_sym = I18n.locale)
+        text = text_for_translation(topic)
+        translate_text!(text, target_locale_sym)
       end
 
       def self.translate_text!(text, target_locale_sym = I18n.locale)

app/services/discourse_translator/provider/libre_translate.rb

@@ -91,18 +91,19 @@ def self.translate_supported?(source, target)
         res.any? { |obj| obj["code"] == source } && res.any? { |obj| obj["code"] == lang }
       end
 
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        detected_lang = detect(translatable)
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
+        raw = opts.key?(:raw) ? opts[:raw] : !opts[:cooked]
+        text = text_for_translation(post, raw:)
 
-        res =
-          result(
-            translate_uri,
-            q: text_for_translation(translatable),
-            source: detected_lang,
-            target: SUPPORTED_LANG_MAPPING[target_locale_sym],
-            format: "html",
-          )
-        res["translatedText"]
+        detected_lang = detect(post)
+
+        send_for_translation(text, detected_lang, target_locale_sym)
+      end
+
+      def self.translate_topic!(topic, target_locale_sym = I18n.locale)
+        detected_lang = detect(topic)
+        text = text_for_translation(topic)
+        send_for_translation(text, detected_lang, target_locale_sym)
       end
 
       def self.translate_text!(text, target_locale_sym = I18n.locale)
@@ -154,6 +155,20 @@ def self.result(url, body)
           body
         end
       end
+
+      private
+
+      def self.send_for_translation(text, source_locale, target_locale)
+        res =
+          result(
+            translate_uri,
+            q: text,
+            source: source_locale,
+            target: SUPPORTED_LANG_MAPPING[target_locale],
+            format: "html",
+          )
+        res["translatedText"]
+      end
     end
   end
 end

app/services/discourse_translator/provider/microsoft.rb

@@ -154,27 +154,24 @@ def self.detect!(topic_or_post)
         result(uri.to_s, body, default_headers).first["language"]
       end
 
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        detected_lang = detect(translatable)
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
+        raw = opts.key?(:raw) ? opts[:raw] : !opts[:cooked]
+        text = text_for_translation(post, raw:)
 
-        if text_for_translation(translatable).length > LENGTH_LIMIT
-          raise TranslatorError.new(I18n.t("translator.too_long"))
-        end
-        locale =
-          SUPPORTED_LANG_MAPPING[target_locale_sym] || (raise I18n.t("translator.not_supported"))
+        raise TranslatorError.new(I18n.t("translator.too_long")) if text.length > LENGTH_LIMIT
 
-        query = default_query.merge("from" => detected_lang, "to" => locale, "textType" => "html")
-        body = [{ "Text" => text_for_translation(translatable) }].to_json
-        uri = URI(translate_endpoint)
-        uri.query = URI.encode_www_form(query)
-        response_body = result(uri.to_s, body, default_headers)
-        response_body.first["translations"].first["text"]
+        translate_text!(text, target_locale_sym)
       end
 
-      def self.translate_text!(text, target_locale_sym = I18n.locale)
-        locale =
-          SUPPORTED_LANG_MAPPING[target_locale_sym] || (raise I18n.t("translator.not_supported"))
+      def self.translate_topic!(topic, target_locale_sym = I18n.locale)
+        text = text_for_translation(topic)
+        raise TranslatorError.new(I18n.t("translator.too_long")) if text.length > LENGTH_LIMIT
+
+        translate_text!(text, target_locale_sym)
+      end
 
+      def self.translate_text!(text, target_locale_sym = I18n.locale)
+        locale = SUPPORTED_LANG_MAPPING[target_locale_sym]
         query = default_query.merge("to" => locale, "textType" => "html")
         body = [{ "Text" => text }].to_json
         uri = URI(translate_endpoint)

app/services/discourse_translator/provider/yandex.rb

@@ -129,26 +129,25 @@ def self.detect!(topic_or_post)
         result(uri.to_s, "", default_headers)["lang"]
       end
 
-      def self.translate_translatable!(translatable, target_locale_sym = I18n.locale)
-        detected_lang = detect(translatable)
-        locale =
-          SUPPORTED_LANG_MAPPING[target_locale_sym] || (raise I18n.t("translator.not_supported"))
+      def self.translate_post!(post, target_locale_sym = I18n.locale, opts = {})
+        raw = opts.key?(:raw) ? opts[:raw] : !opts[:cooked]
+        text = text_for_translation(post, raw:)
 
-        query =
-          default_query.merge(
-            "lang" => "#{detected_lang}-#{locale}",
-            "text" => text_for_translation(translatable),
-            "format" => "html",
-          )
+        detected_lang = detect(post)
+        locale = SUPPORTED_LANG_MAPPING[target_locale_sym]
 
-        uri = URI(TRANSLATE_URI)
-        uri.query = URI.encode_www_form(query)
+        send_for_translation(text, detected_lang, locale)
+      end
 
-        response_body = result(uri.to_s, "", default_headers)
-        response_body["text"][0]
+      def self.translate_topic!(topic, target_locale_sym = I18n.locale)
+        detected_lang = detect(topic)
+        locale = SUPPORTED_LANG_MAPPING[target_locale_sym]
+        text = text_for_translation(topic)
+
+        send_for_translation(text, detected_lang, locale)
       end
 
-      def self.translate_text!(translatable, target_locale_sym = I18n.locale)
+      def self.translate_text!(text, target_locale_sym = I18n.locale)
         # Not supported for v1.5 https://translate.yandex.com/developers
         raise TranslatorError.new(I18n.t("translator.not_supported"))
       end
@@ -160,6 +159,21 @@ def self.translate_supported?(detected_lang, target_lang)
 
       private
 
+      def self.send_for_translation(text, source_locale, target_locale)
+        query =
+          default_query.merge(
+            "lang" => "#{source_locale}-#{target_locale}",
+            "text" => text,
+            "format" => "html",
+          )
+
+        uri = URI(TRANSLATE_URI)
+        uri.query = URI.encode_www_form(query)
+
+        response_body = result(uri.to_s, "", default_headers)
+        response_body["text"][0]
+      end
+
       def self.post(uri, body, headers = {})
         Excon.post(uri, body: body, headers: headers)
       end