DEV: Move saving of translations into base class (#203)

The following changes allow the saving of translation metadata to be in the single base class instead of sprinkled in all the subclasses. This is to prepare for the move from custom fields to proper tables.

This PR does two things:
1. introduce a `detect!` (and also `translate!`) in translator subclasses (Amazon, Google, Microsoft, etc) which will set the value from the API all the time. The base class invokes `detect!`.
https://github.com/discourse/discourse-translator/blob/4c5d8e74dea4ff006706772a356f1c2efa1445fa/app/services/discourse_translator/google.rb#L77-L85
2. update `detect` to return the stored value or invoke the `!` variant to get the value if it does not exist.
https://github.com/discourse/discourse-translator/blob/4c5d8e74dea4ff006706772a356f1c2efa1445fa/app/services/discourse_translator/base.rb#L51-L57


There is already test coverage for this refactor.

app/jobs/scheduled/detect_posts_language.rb

@@ -26,10 +26,6 @@ def process_batch(post_ids)
           begin
             translator = "DiscourseTranslator::#{SiteSetting.translator}".constantize
             translator.detect(post)
-            if !post.custom_fields_clean?
-              post.save_custom_fields
-              post.publish_change_to_clients!(:revised)
-            end
           rescue ::DiscourseTranslator::ProblemCheckedTranslationError
             # problem-checked translation errors gracefully
           end

app/services/discourse_translator/amazon.rb

@@ -107,47 +107,41 @@ def self.access_token_key
       "aws-translator"
     end
 
-    def self.detect(topic_or_post)
-      text = truncate text_for_detection(topic_or_post)
-      return if text.blank?
-
-      topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] ||= (
+    def self.detect!(topic_or_post)
+      save_detected_locale(topic_or_post) do
         begin
           client.translate_text(
             {
-              text: text,
+              text: truncate(text_for_detection(topic_or_post)),
               source_language_code: "auto",
               target_language_code: SUPPORTED_LANG_MAPPING[I18n.locale],
             },
           )&.source_language_code
         rescue Aws::Errors::MissingCredentialsError
           raise I18n.t("translator.amazon.invalid_credentials")
         end
-      )
+      end
     end
 
-    def self.translate(topic_or_post)
+    def self.translate!(topic_or_post)
       detected_lang = detect(topic_or_post)
 
-      from_custom_fields(topic_or_post) do
+      save_translation(topic_or_post) do
         begin
-          result =
-            client.translate_text(
-              {
-                text: truncate(text_for_translation(topic_or_post)),
-                source_language_code: "auto",
-                target_language_code: SUPPORTED_LANG_MAPPING[I18n.locale],
-              },
-            )
+          client.translate_text(
+            {
+              text: truncate(text_for_translation(topic_or_post)),
+              source_language_code: "auto",
+              target_language_code: SUPPORTED_LANG_MAPPING[I18n.locale],
+            },
+          )
         rescue Aws::Translate::Errors::UnsupportedLanguagePairException
           raise I18n.t(
                   "translator.failed",
                   source_locale: detected_lang,
                   target_locale: I18n.locale,
                 )
         end
-
-        [detected_lang, result.translated_text]
       end
     end
 
@@ -170,12 +164,5 @@ def self.client
 
       @client ||= Aws::Translate::Client.new(opts)
     end
-
-    def self.assign_lang_custom_field(post, value)
-      if value.nil?
-        return post.custom_fields.delete(DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD)
-      end
-      post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] ||= value
-    end
   end
 end

app/services/discourse_translator/base.rb

@@ -24,19 +24,64 @@ def self.cache_key
       "#{key_prefix}#{access_token_key}"
     end
 
-    def self.translate(post)
+    # 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.
+    # If the detected language is the same as the target language, the original text will be returned.
+    # @param topic_or_post [Post|Topic]
+    def self.translate(topic_or_post)
+      return if text_for_translation(topic_or_post).blank?
+      detected_lang = detect(topic_or_post)
+
+      return detected_lang, get_text(topic_or_post) if (detected_lang&.to_s == I18n.locale.to_s)
+
+      existing_translation = get_translation(topic_or_post)
+      return detected_lang, existing_translation if existing_translation.present?
+
+      unless translate_supported?(detected_lang, I18n.locale)
+        raise TranslatorError.new(
+                I18n.t(
+                  "translator.failed",
+                  source_locale: detected_lang,
+                  target_locale: I18n.locale,
+                ),
+              )
+      end
+      [detected_lang, translate!(topic_or_post)]
+    end
+
+    # Subclasses must implement this method to translate the text of a post or topic
+    # then use the save_translation method to store the translated text.
+    # @param topic_or_post [Post|Topic]
+    def self.translate!(topic_or_post)
       raise "Not Implemented"
     end
 
-    def self.detect(post)
+    # 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 topic_or_post [Post|Topic]
+    def self.detect(topic_or_post)
+      return if text_for_detection(topic_or_post).blank?
+      get_detected_locale(topic_or_post) || detect!(topic_or_post)
+    end
+
+    # Subclasses must implement this method to translate the text of a post or topic
+    # then use the save_translation method to store the translated text.
+    # @param topic_or_post [Post|Topic]
+    def self.detect!(post)
       raise "Not Implemented"
     end
 
     def self.access_token
       raise "Not Implemented"
     end
 
-    def self.from_custom_fields(topic_or_post)
+    def self.get_translation(topic_or_post)
+      translated_custom_field =
+        topic_or_post.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] || {}
+      translated_custom_field[I18n.locale]
+    end
+
+    def self.save_translation(topic_or_post)
       translated_custom_field =
         topic_or_post.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] || {}
       translated_text = translated_custom_field[I18n.locale]
@@ -54,6 +99,22 @@ def self.from_custom_fields(topic_or_post)
       translated_text
     end
 
+    def self.get_detected_locale(topic_or_post)
+      topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]
+    end
+
+    def self.save_detected_locale(topic_or_post)
+      detected_locale = yield
+      topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = detected_locale
+
+      if !topic_or_post.custom_fields_clean?
+        topic_or_post.save_custom_fields
+        topic_or_post.publish_change_to_clients!(:revised) if topic_or_post.class.name == "Post"
+      end
+
+      detected_locale
+    end
+
     def self.get_text(topic_or_post)
       case topic_or_post.class.name
       when "Post"
@@ -70,6 +131,10 @@ def self.language_supported?(detected_lang)
       detected_lang != supported_lang[I18n.locale]
     end
 
+    def self.translate_supported?(detected_lang, target_lang)
+      true
+    end
+
     private
 
     def self.strip_tags_for_detection(detection_text)

app/services/discourse_translator/discourse_ai.rb

@@ -11,24 +11,20 @@ def self.language_supported?(detected_lang)
       detected_lang != locale_without_region
     end
 
-    def self.detect(topic_or_post)
+    def self.detect!(topic_or_post)
       return unless required_settings_enabled
 
-      topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] ||= begin
+      save_detected_locale(topic_or_post) do
         ::DiscourseAi::LanguageDetector.new(text_for_detection(topic_or_post)).detect
       end
-    rescue => e
-      Rails.logger.warn(
-        "#{::DiscourseTranslator::PLUGIN_NAME}: Failed to detect language for #{topic_or_post.class.name} #{topic_or_post.id}: #{e}",
-      )
     end
 
     def self.translate(topic_or_post)
       return unless required_settings_enabled
 
       detected_lang = detect(topic_or_post)
       translated_text =
-        from_custom_fields(topic_or_post) do
+        save_translation(topic_or_post) do
           ::DiscourseAi::Translator.new(text_for_translation(topic_or_post), I18n.locale).translate
         end
 

app/services/discourse_translator/google.rb

@@ -74,54 +74,33 @@ def self.access_token
       raise ProblemCheckedTranslationError.new("NotFound: Google Api Key not set.")
     end
 
-    def self.detect(topic_or_post)
-      topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] ||= result(
-        DETECT_URI,
-        q: text_for_detection(topic_or_post),
-      )[
-        "detections"
-      ][
-        0
-      ].max { |a, b| a.confidence <=> b.confidence }[
-        "language"
-      ]
+    def self.detect!(topic_or_post)
+      save_detected_locale(topic_or_post) do
+        result(DETECT_URI, q: text_for_detection(topic_or_post))["detections"][0].max do |a, b|
+          a.confidence <=> b.confidence
+        end[
+          "language"
+        ]
+      end
     end
 
     def self.translate_supported?(source, target)
       res = result(SUPPORT_URI, target: SUPPORTED_LANG_MAPPING[target])
       res["languages"].any? { |obj| obj["language"] == source }
     end
 
-    def self.translate(topic_or_post)
-      detected_lang = detect(topic_or_post)
-
-      # the translate button appears if a given post is in a foreign language.
-      # however the title of the topic may be in a different language, and may be in the user's language.
-      # if this is the case, when this is called for a topic, the detected_lang will be the user's language,
-      # so the user's language and the detected language will be the same. For example, both could be "en"
-      # google will choke on this and return an error instead of gracefully handling it by returning the original
-      # string.
-      # ---
-      # here we handle that situation by returning the original string if the source and target lang are the same.
-      return detected_lang, get_text(topic_or_post) if (detected_lang&.to_s.eql? I18n.locale.to_s)
-
-      unless translate_supported?(detected_lang, I18n.locale)
-        raise I18n.t("translator.failed", source_locale: detected_lang, target_locale: I18n.locale)
+    def self.translate!(topic_or_post)
+      detected_locale = detect(topic_or_post)
+      save_translation(topic_or_post) do
+        res =
+          result(
+            TRANSLATE_URI,
+            q: text_for_translation(topic_or_post),
+            source: detected_locale,
+            target: SUPPORTED_LANG_MAPPING[I18n.locale],
+          )
+        res["translations"][0]["translatedText"]
       end
-
-      translated_text =
-        from_custom_fields(topic_or_post) do
-          res =
-            result(
-              TRANSLATE_URI,
-              q: text_for_translation(topic_or_post),
-              source: detected_lang,
-              target: SUPPORTED_LANG_MAPPING[I18n.locale],
-            )
-          res["translations"][0]["translatedText"]
-        end
-
-      [detected_lang, translated_text]
     end
 
     def self.result(url, body)

app/services/discourse_translator/libre_translate.rb

@@ -78,19 +78,14 @@ def self.access_token
       SiteSetting.translator_libretranslate_api_key
     end
 
-    def self.detect(topic_or_post)
-      res =
-        result(
-          detect_uri,
-          q: ActionController::Base.helpers.strip_tags(text_for_detection(topic_or_post)),
-        )
-
-      if !res.empty?
-        topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] ||= res[0][
-          "language"
-        ]
-      else
-        topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] ||= "en"
+    def self.detect!(topic_or_post)
+      save_detected_locale(topic_or_post) do
+        res =
+          result(
+            detect_uri,
+            q: ActionController::Base.helpers.strip_tags(text_for_detection(topic_or_post)),
+          )
+        !res.empty? ? res[0]["language"] : "en"
       end
     end
 
@@ -100,27 +95,20 @@ def self.translate_supported?(source, target)
       res.any? { |obj| obj["code"] == source } && res.any? { |obj| obj["code"] == lang }
     end
 
-    def self.translate(topic_or_post)
+    def self.translate!(topic_or_post)
       detected_lang = detect(topic_or_post)
 
-      unless translate_supported?(detected_lang, I18n.locale)
-        raise I18n.t("translator.failed", source_locale: detected_lang, target_locale: I18n.locale)
+      save_translation(topic_or_post) do
+        res =
+          result(
+            translate_uri,
+            q: text_for_translation(topic_or_post),
+            source: detected_lang,
+            target: SUPPORTED_LANG_MAPPING[I18n.locale],
+            format: "html",
+          )
+        res["translatedText"]
       end
-
-      translated_text =
-        from_custom_fields(topic_or_post) do
-          res =
-            result(
-              translate_uri,
-              q: text_for_translation(topic_or_post),
-              source: detected_lang,
-              target: SUPPORTED_LANG_MAPPING[I18n.locale],
-              format: "html",
-            )
-          res["translatedText"]
-        end
-
-      [detected_lang, translated_text]
     end
 
     def self.get(url)