From ce6bdca6fe36e6f998629157c5f36895d6c7198f Mon Sep 17 00:00:00 2001 From: Nat Date: Wed, 5 Feb 2025 22:53:10 +0800 Subject: [PATCH 1/7] DEV: Move translation custom fields into their own tables --- app/jobs/scheduled/detect_posts_language.rb | 6 +- .../discourse_translator/post_locale.rb | 12 +++ .../discourse_translator/post_translation.rb | 18 ++++ .../discourse_translator/topic_locale.rb | 12 +++ .../discourse_translator/topic_translation.rb | 18 ++++ ...0250205082400_create_translation_tables.rb | 31 +++++++ ...ove_translations_custom_fields_to_table.rb | 59 +++++++++++++ ...250205082402_create_translation_indexes.rb | 17 ++++ spec/models/translation_tables_spec.rb | 87 +++++++++++++++++++ 9 files changed, 257 insertions(+), 3 deletions(-) create mode 100644 app/models/discourse_translator/post_locale.rb create mode 100644 app/models/discourse_translator/post_translation.rb create mode 100644 app/models/discourse_translator/topic_locale.rb create mode 100644 app/models/discourse_translator/topic_translation.rb create mode 100644 db/migrate/20250205082400_create_translation_tables.rb create mode 100644 db/migrate/20250205082401_move_translations_custom_fields_to_table.rb create mode 100644 db/migrate/20250205082402_create_translation_indexes.rb create mode 100644 spec/models/translation_tables_spec.rb diff --git a/app/jobs/scheduled/detect_posts_language.rb b/app/jobs/scheduled/detect_posts_language.rb index 04a7f7c6..d7361b67 100644 --- a/app/jobs/scheduled/detect_posts_language.rb +++ b/app/jobs/scheduled/detect_posts_language.rb @@ -3,10 +3,10 @@ module ::Jobs class DetectPostsLanguage < ::Jobs::Scheduled sidekiq_options retry: false - every 5.minutes + every 1.minutes - BATCH_SIZE = 100 - MAX_QUEUE_SIZE = 1000 + BATCH_SIZE = 10 + MAX_QUEUE_SIZE = 100 def execute(args) return unless SiteSetting.translator_enabled diff --git a/app/models/discourse_translator/post_locale.rb b/app/models/discourse_translator/post_locale.rb new file mode 100644 index 00000000..b318504f --- /dev/null +++ b/app/models/discourse_translator/post_locale.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module DiscourseTranslator + class PostLocale < ActiveRecord::Base + self.table_name = "discourse_translator_post_locales" + + belongs_to :post + + validates :post_id, presence: true, uniqueness: true + validates :detected_locale, presence: true + end +end diff --git a/app/models/discourse_translator/post_translation.rb b/app/models/discourse_translator/post_translation.rb new file mode 100644 index 00000000..66be9c8b --- /dev/null +++ b/app/models/discourse_translator/post_translation.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module DiscourseTranslator + class PostTranslation < ActiveRecord::Base + self.table_name = "discourse_translator_post_translations" + + belongs_to :post + + validates :post_id, presence: true + validates :locale, presence: true + validates :translation, presence: true + validates :locale, uniqueness: { scope: :post_id } + + def self.translation_for(post_id, locale) + find_by(post_id: post_id, locale: locale)&.translation + end + end +end diff --git a/app/models/discourse_translator/topic_locale.rb b/app/models/discourse_translator/topic_locale.rb new file mode 100644 index 00000000..ccd09f52 --- /dev/null +++ b/app/models/discourse_translator/topic_locale.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module DiscourseTranslator + class TopicLocale < ActiveRecord::Base + self.table_name = "discourse_translator_topic_locales" + + belongs_to :topic + + validates :topic_id, presence: true, uniqueness: true + validates :detected_locale, presence: true + end +end diff --git a/app/models/discourse_translator/topic_translation.rb b/app/models/discourse_translator/topic_translation.rb new file mode 100644 index 00000000..001b2742 --- /dev/null +++ b/app/models/discourse_translator/topic_translation.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module DiscourseTranslator + class TopicTranslation < ActiveRecord::Base + self.table_name = "discourse_translator_topic_translations" + + belongs_to :topic + + validates :topic_id, presence: true + validates :locale, presence: true + validates :translation, presence: true + validates :locale, uniqueness: { scope: :topic_id } + + def self.translation_for(topic_id, locale) + find_by(topic_id: topic_id, locale: locale)&.translation + end + end +end diff --git a/db/migrate/20250205082400_create_translation_tables.rb b/db/migrate/20250205082400_create_translation_tables.rb new file mode 100644 index 00000000..c9a7037f --- /dev/null +++ b/db/migrate/20250205082400_create_translation_tables.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class CreateTranslationTables < ActiveRecord::Migration[7.2] + def change + create_table :discourse_translator_topic_locales do |t| + t.integer :topic_id, null: false + t.string :detected_locale, limit: 20, null: false + t.timestamps + end + + create_table :discourse_translator_topic_translations do |t| + t.integer :topic_id, null: false + t.string :locale, null: false + t.text :translation, null: false + t.timestamps + end + + create_table :discourse_translator_post_locales do |t| + t.integer :post_id, null: false + t.string :detected_locale, limit: 20, null: false + t.timestamps + end + + create_table :discourse_translator_post_translations do |t| + t.integer :post_id, null: false + t.string :locale, null: false + t.text :translation, null: false + t.timestamps + end + end +end diff --git a/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb b/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb new file mode 100644 index 00000000..d9494e4f --- /dev/null +++ b/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +class MoveTranslationsCustomFieldsToTable < ActiveRecord::Migration[7.2] # frozen_string_literal: true + BATCH_SIZE = 1000 + + def up + migrate_custom_fields("topic") + migrate_custom_fields("post") + end + + def down + execute "TRUNCATE discourse_translator_topic_locales" + execute "TRUNCATE discourse_translator_topic_translations" + execute "TRUNCATE discourse_translator_post_locales" + execute "TRUNCATE discourse_translator_post_translations" + end + + private + + def migrate_custom_fields(model) + start_id = 0 + loop do + last_id = DB.query_single(<<~SQL, model:, start_id:, limit: BATCH_SIZE) + WITH to_insert AS ( + SELECT id, #{model}_id, value + FROM #{model}_custom_fields + WHERE name = 'post_detected_lang' + AND id > :start_id + ORDER BY id + LIMIT :limit + ), + do_insert AS ( + INSERT INTO discourse_translator_#{model}_locales (#{model}_id, detected_locale, created_at, updated_at) + SELECT #{model}_id, value, NOW(), NOW() + FROM to_insert + ), + to_translate AS ( + SELECT id, #{model}_id, value::jsonb, created_at, updated_at + FROM #{model}_custom_fields + WHERE id > :start_id + AND name = 'translated_text' + AND value LIKE '{%}' + ORDER BY id + LIMIT :limit + ), + do_translate AS ( + INSERT INTO discourse_translator_#{model}_translations (#{model}_id, locale, translation, created_at, updated_at) + SELECT b.#{model}_id, jb.key as locale, jb.value as translation, b.created_at, b.updated_at + FROM to_translate b, jsonb_each_text(b.value) jb + WHERE LENGTH(jb.key) <= 20 + ), + max_value AS (SELECT COALESCE(GREATEST((SELECT MAX(id) FROM to_insert), (SELECT MAX(id) FROM to_translate) ), -1) as max_id) + SELECT max_id FROM max_value + SQL + start_id = last_id.last + break if start_id == -1 + end + end +end diff --git a/db/migrate/20250205082402_create_translation_indexes.rb b/db/migrate/20250205082402_create_translation_indexes.rb new file mode 100644 index 00000000..6855128e --- /dev/null +++ b/db/migrate/20250205082402_create_translation_indexes.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CreateTranslationIndexes < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def change + add_index :discourse_translator_topic_translations, + %i[topic_id locale], + unique: true, + algorithm: :concurrently + + add_index :discourse_translator_post_translations, + %i[post_id locale], + unique: true, + algorithm: :concurrently + end +end diff --git a/spec/models/translation_tables_spec.rb b/spec/models/translation_tables_spec.rb new file mode 100644 index 00000000..070d4909 --- /dev/null +++ b/spec/models/translation_tables_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require_relative "../../db/migrate/20250205082401_move_translations_custom_fields_to_table" + +module DiscourseTranslator + describe MoveTranslationsCustomFieldsToTable do + let!(:batch_size) { 5 } + + before { described_class.const_set(:BATCH_SIZE, batch_size) } + + def create_custom_fields(count) + count.times do + post = Fabricate(:post) + topic = Fabricate(:topic) + + post.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "pt" + post.save_custom_fields + + topic.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "es" + topic.save_custom_fields + + post.custom_fields[TRANSLATED_CUSTOM_FIELD] = { + en_GB: "The Romance of the Three Kingdoms", + de: "Die Romanze der Drei Königreiche", + } + post.save_custom_fields + + topic.custom_fields[TRANSLATED_CUSTOM_FIELD] = { + en_GB: "The Romance of the Three Kingdoms", + de: "Die Romanze der Drei Königreiche", + } + topic.save_custom_fields + end + end + + it "correctly migrates custom fields in batches" do + # batch size is 5 + create_custom_fields(12) + + migration = described_class.new + migration.up + + # 12 posts * 2 translations each + expect(PostLocale.count).to eq(12) + expect(PostTranslation.count).to eq(24) + + # 12 topics * 2 translations each + expect(TopicLocale.count).to eq(12) + expect(TopicTranslation.count).to eq(24) + + expect(PostLocale.first.detected_locale).to eq("pt") + + expect(PostTranslation.where(post_id: Post.first.id).pluck(:locale, :translation)).to include( + ["en_GB", "The Romance of the Three Kingdoms"], + ["de", "Die Romanze der Drei Königreiche"], + ) + + migration.down + expect(PostLocale.count).to eq(0) + expect(PostTranslation.count).to eq(0) + expect(TopicLocale.count).to eq(0) + expect(TopicTranslation.count).to eq(0) + end + + it "ignores invalid JSON in translated_text" do + post = Fabricate(:post) + post.custom_fields[TRANSLATED_CUSTOM_FIELD] = "invalid json" + post.save_custom_fields(true) + + migration = described_class.new + expect { migration.up }.not_to raise_error + expect(PostTranslation.count).to eq(0) + end + + it "ignores translations with locale longer than 20 chars" do + post = Fabricate(:post) + post.custom_fields[TRANSLATED_CUSTOM_FIELD] = { very_very_long_locale_name: "test" } + post.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "very_very_long_locale_name" + post.save_custom_fields(true) + + migration = described_class.new + migration.up + expect(PostLocale.count).to eq(0) + expect(PostTranslation.count).to eq(0) + end + end +end From 6c7e155a3346783625677b829be35e0182ad7eb0 Mon Sep 17 00:00:00 2001 From: Nat Date: Fri, 7 Feb 2025 03:14:19 +0800 Subject: [PATCH 2/7] Swap out new table --- .../discourse_translator/translatable.rb | 42 +++++++++ app/services/discourse_translator/base.rb | 90 +++++++------------ ...ove_translations_custom_fields_to_table.rb | 46 ++++++---- .../guardian_extension.rb | 11 +-- lib/discourse_translator/post_extension.rb | 11 +-- lib/discourse_translator/topic_extension.rb | 11 +-- plugin.rb | 5 -- spec/fabricators/topic_locale.rb | 5 ++ spec/fabricators/topic_translation.rb | 18 ++++ spec/models/post_spec.rb | 69 ++++++++++---- spec/models/topic_spec.rb | 73 +++++++++++---- spec/models/translation_tables_spec.rb | 55 +++++++----- 12 files changed, 272 insertions(+), 164 deletions(-) create mode 100644 app/models/concerns/discourse_translator/translatable.rb create mode 100644 spec/fabricators/topic_locale.rb create mode 100644 spec/fabricators/topic_translation.rb diff --git a/app/models/concerns/discourse_translator/translatable.rb b/app/models/concerns/discourse_translator/translatable.rb new file mode 100644 index 00000000..02948ecb --- /dev/null +++ b/app/models/concerns/discourse_translator/translatable.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module DiscourseTranslator + module Translatable + extend ActiveSupport::Concern + + prepended do + has_many :translations, + class_name: "DiscourseTranslator::#{name}Translation", + dependent: :destroy + has_one :content_locale, class_name: "DiscourseTranslator::#{name}Locale", dependent: :destroy + end + + 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) + end + + def set_translation(locale, text) + locale = locale.to_s.gsub("_", "-") + translations.find_or_initialize_by(locale: locale).update!(translation: text) + end + + def translation_for(locale) + translations.find_by(locale: locale)&.translation + end + + def detected_locale + content_locale&.detected_locale + end + + private + + def clear_translations + return if !SiteSetting.translator_enabled + + translations.delete_all + content_locale&.destroy + end + end +end diff --git a/app/services/discourse_translator/base.rb b/app/services/discourse_translator/base.rb index 6d927e22..d714825f 100644 --- a/app/services/discourse_translator/base.rb +++ b/app/services/discourse_translator/base.rb @@ -27,14 +27,14 @@ def self.cache_key # 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) + # @param translatable [Post|Topic] + def self.translate(translatable) + return if text_for_translation(translatable).blank? + detected_lang = detect(translatable) - return detected_lang, get_text(topic_or_post) if (detected_lang&.to_s == I18n.locale.to_s) + return detected_lang, get_text(translatable) if (detected_lang&.to_s == I18n.locale.to_s) - existing_translation = get_translation(topic_or_post) + existing_translation = get_translation(translatable) return detected_lang, existing_translation if existing_translation.present? unless translate_supported?(detected_lang, I18n.locale) @@ -46,27 +46,27 @@ def self.translate(topic_or_post) ), ) end - [detected_lang, translate!(topic_or_post)] + [detected_lang, translate!(translatable)] 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) + # @param translatable [Post|Topic] + def self.translate!(translatable) 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 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) + # @param translatable [Post|Topic] + def self.detect(translatable) + return if text_for_detection(translatable).blank? + get_detected_locale(translatable) || detect!(translatable) 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] + # Subclasses must implement this method to detect the text of a post or topic + # then use the save_detected_locale method to store the detected locale. + # @param translatable [Post|Topic] def self.detect!(post) raise "Not Implemented" end @@ -75,52 +75,33 @@ def self.access_token raise "Not Implemented" end - def self.get_translation(topic_or_post) - translated_custom_field = - topic_or_post.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] || {} - translated_custom_field[I18n.locale] + def self.get_translation(translatable) + translatable.translation_for(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] - - if translated_text.nil? - translated_text = yield - - topic_or_post.custom_fields[ - DiscourseTranslator::TRANSLATED_CUSTOM_FIELD - ] = translated_custom_field.merge(I18n.locale => translated_text) - - topic_or_post.save! - end - - translated_text + def self.save_translation(translatable) + translation = yield + translatable.set_translation(I18n.locale, translation) + translation end - def self.get_detected_locale(topic_or_post) - topic_or_post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] + def self.get_detected_locale(translatable) + translatable.detected_locale end - def self.save_detected_locale(topic_or_post) + def self.save_detected_locale(translatable) 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 + translatable.set_detected_locale(detected_locale) detected_locale end - def self.get_text(topic_or_post) - case topic_or_post.class.name + def self.get_text(translatable) + case translatable.class.name when "Post" - topic_or_post.cooked + translatable.cooked when "Topic" - topic_or_post.title + translatable.title end end @@ -143,15 +124,12 @@ def self.strip_tags_for_detection(detection_text) html_doc.to_html end - def self.text_for_detection(topic_or_post) - strip_tags_for_detection(get_text(topic_or_post)).truncate( - DETECTION_CHAR_LIMIT, - omission: nil, - ) + def self.text_for_detection(translatable) + strip_tags_for_detection(get_text(translatable)).truncate(DETECTION_CHAR_LIMIT, omission: nil) end - def self.text_for_translation(topic_or_post) - get_text(topic_or_post).truncate(SiteSetting.max_characters_per_translation, omission: nil) + def self.text_for_translation(translatable) + get_text(translatable).truncate(SiteSetting.max_characters_per_translation, omission: nil) end end end diff --git a/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb b/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb index d9494e4f..3ca4f0a3 100644 --- a/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb +++ b/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class MoveTranslationsCustomFieldsToTable < ActiveRecord::Migration[7.2] # frozen_string_literal: true +class MoveTranslationsCustomFieldsToTable < ActiveRecord::Migration[7.2] BATCH_SIZE = 1000 def up @@ -18,42 +18,50 @@ def down private def migrate_custom_fields(model) - start_id = 0 - loop do - last_id = DB.query_single(<<~SQL, model:, start_id:, limit: BATCH_SIZE) - WITH to_insert AS ( - SELECT id, #{model}_id, value + bounds = DB.query_single(<<~SQL, model:) + SELECT + COALESCE(MIN(id), 0) as min_id, + COALESCE(MAX(id), 0) as max_id + FROM #{model}_custom_fields + WHERE name IN ('post_detected_lang', 'translated_text') + SQL + + start_id = bounds[0] + max_id = bounds[1] + + while start_id < max_id + DB.exec(<<~SQL, model:, start_id:, end_id: start_id + BATCH_SIZE) + WITH to_detect AS ( + SELECT #{model}_id, value FROM #{model}_custom_fields WHERE name = 'post_detected_lang' - AND id > :start_id + AND id >= :start_id + AND id < :end_id ORDER BY id - LIMIT :limit ), - do_insert AS ( + do_detect AS ( INSERT INTO discourse_translator_#{model}_locales (#{model}_id, detected_locale, created_at, updated_at) SELECT #{model}_id, value, NOW(), NOW() - FROM to_insert + FROM to_detect ), to_translate AS ( - SELECT id, #{model}_id, value::jsonb, created_at, updated_at + SELECT #{model}_id, value::jsonb, created_at, updated_at FROM #{model}_custom_fields - WHERE id > :start_id - AND name = 'translated_text' + WHERE name = 'translated_text' AND value LIKE '{%}' + AND id >= :start_id + AND id < :end_id ORDER BY id - LIMIT :limit ), do_translate AS ( INSERT INTO discourse_translator_#{model}_translations (#{model}_id, locale, translation, created_at, updated_at) SELECT b.#{model}_id, jb.key as locale, jb.value as translation, b.created_at, b.updated_at FROM to_translate b, jsonb_each_text(b.value) jb WHERE LENGTH(jb.key) <= 20 - ), - max_value AS (SELECT COALESCE(GREATEST((SELECT MAX(id) FROM to_insert), (SELECT MAX(id) FROM to_translate) ), -1) as max_id) - SELECT max_id FROM max_value + ) + SELECT 1 SQL - start_id = last_id.last - break if start_id == -1 + start_id += BATCH_SIZE end end end diff --git a/lib/discourse_translator/guardian_extension.rb b/lib/discourse_translator/guardian_extension.rb index 1380651c..5ac094e8 100644 --- a/lib/discourse_translator/guardian_extension.rb +++ b/lib/discourse_translator/guardian_extension.rb @@ -22,14 +22,11 @@ def can_detect_language?(post) def can_translate?(post) return false if !user_group_allow_translate? - # we will deal with regionalized_strings (not syms) when comparing locales - # e.g. "en_GB" - # not "en-GB" - # nor :en_GB (I18n.locale) - detected_lang = - post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD].to_s.sub("-", "_") - return false if detected_lang.blank? + locale = post.detected_locale + return false if locale.nil? + # I18n.locale is a symbol e.g. :en_GB + detected_lang = locale.to_s.sub("-", "_") detected_lang != I18n.locale.to_s && "DiscourseTranslator::#{SiteSetting.translator}".constantize.language_supported?( detected_lang, diff --git a/lib/discourse_translator/post_extension.rb b/lib/discourse_translator/post_extension.rb index 08b87d0e..80b0b2c0 100644 --- a/lib/discourse_translator/post_extension.rb +++ b/lib/discourse_translator/post_extension.rb @@ -3,16 +3,7 @@ module DiscourseTranslator module PostExtension extend ActiveSupport::Concern - prepended { before_update :clear_translations, if: :raw_changed? } - - private - - def clear_translations - return if !SiteSetting.translator_enabled - - self.custom_fields.delete(DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD) - self.custom_fields.delete(DiscourseTranslator::TRANSLATED_CUSTOM_FIELD) - end + include Translatable end end diff --git a/lib/discourse_translator/topic_extension.rb b/lib/discourse_translator/topic_extension.rb index d0e3f4ec..03271c85 100644 --- a/lib/discourse_translator/topic_extension.rb +++ b/lib/discourse_translator/topic_extension.rb @@ -3,16 +3,7 @@ module DiscourseTranslator module TopicExtension extend ActiveSupport::Concern - prepended { before_update :clear_translations, if: :title_changed? } - - private - - def clear_translations - return if !SiteSetting.translator_enabled - - self.custom_fields.delete(DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD) - self.custom_fields.delete(DiscourseTranslator::TRANSLATED_CUSTOM_FIELD) - end + include Translatable end end diff --git a/plugin.rb b/plugin.rb index f74dc226..5dfdf8c0 100644 --- a/plugin.rb +++ b/plugin.rb @@ -27,11 +27,6 @@ module ::DiscourseTranslator register_problem_check ProblemCheck::MissingTranslatorApiKey register_problem_check ProblemCheck::TranslatorError - Post.register_custom_field_type(::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD, :json) - Topic.register_custom_field_type(::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD, :json) - - topic_view_post_custom_fields_allowlister { [::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] } - reloadable_patch do Guardian.prepend(DiscourseTranslator::GuardianExtension) Post.prepend(DiscourseTranslator::PostExtension) diff --git a/spec/fabricators/topic_locale.rb b/spec/fabricators/topic_locale.rb new file mode 100644 index 00000000..a868646b --- /dev/null +++ b/spec/fabricators/topic_locale.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true +Fabricator(:topic_locale, from: DiscourseTranslator::TopicLocale) do + topic + detected_locale { %w[en de es en-GB ja pt pt-BR].sample } +end diff --git a/spec/fabricators/topic_translation.rb b/spec/fabricators/topic_translation.rb new file mode 100644 index 00000000..cb75c4da --- /dev/null +++ b/spec/fabricators/topic_translation.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true +Fabricator(:topic_translation, from: DiscourseTranslator::TopicTranslation) do + topic + locale { %w[en de es en-GB ja pt pt-BR].sample } + translation do |attrs| + { + "en" => "Hello", + "de" => "Hallo", + "es" => "Hola", + "en-GB" => "Hello", + "ja" => "こんにちは", + "pt" => "Olá", + "pt-BR" => "Olá", + }[ + attrs[:locale] + ] + end +end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 3ee7b4cd..bae48a6f 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -5,26 +5,63 @@ RSpec.describe Post do before { SiteSetting.translator_enabled = true } - describe "translator custom fields" do - let(:post) do - Fabricate( - :post, - raw: "this is a sample post", - custom_fields: { - ::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD => "en", - ::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD => { - "en" => "lol", - }, - }, - ) + describe "translatable" do + fab!(:post) + + it "should reset translation data when post title has been updated" do + Fabricate(:post_translation, post:) + Fabricate(:post_locale, post:) + post.update!(title: "this is an updated title") + + expect(DiscourseTranslator::PostLocale.where(post:)).to be_empty + expect(DiscourseTranslator::PostLocale.find_by(post:)).to be_nil end - it "should reset custom fields when post has been updated" do - post.update!(raw: "this is an updated post") + describe "#set_translation" do + it "creates new translation" do + post.set_translation("en", "Hello") + + translation = post.translations.find_by(locale: "en") + expect(translation.translation).to eq("Hello") + end + + it "updates existing translation" do + post.set_translation("en", "Hello") + post.set_translation("en", "Updated hello") + + expect(post.translations.where(locale: "en").count).to eq(1) + expect(post.translation_for("en")).to eq("Updated hello") + end - expect(post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to be_nil + it "converts underscore to hyphen in locale" do + post.set_translation("en_US", "Hello") - expect(post.custom_fields[::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD]).to be_nil + expect(post.translations.find_by(locale: "en-US")).to be_present + expect(post.translations.find_by(locale: "en_US")).to be_nil + end + end + + describe "#translation_for" do + it "returns nil when translation doesn't exist" do + expect(post.translation_for("fr")).to be_nil + end + + it "returns translation when it exists" do + post.set_translation("es", "Hola") + expect(post.translation_for("es")).to eq("Hola") + end + end + + describe "#set_locale" do + it "creates new locale" do + post.set_detected_locale("en-US") + expect(post.content_locale.detected_locale).to eq("en-US") + end + + it "converts underscore to hyphen" do + post.set_detected_locale("en_US") + expect(post.content_locale.detected_locale).to eq("en-US") + end end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 9746e7d9..d88832ba 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -2,31 +2,66 @@ require "rails_helper" -RSpec.describe Topic do - describe "translator custom fields" do - fab!(:topic) do - Fabricate( - :topic, - title: "this is a sample title", - custom_fields: { - ::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD => "en", - ::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD => { - "en" => "lol", - }, - }, - ) - end +describe Topic do + describe "translatable" do + fab!(:topic) before { SiteSetting.translator_enabled = true } - after { SiteSetting.translator_enabled = false } - - it "should reset custom fields when topic title has been updated" do + it "should reset translation data when topic title has been updated" do + Fabricate(:topic_translation, topic:) + Fabricate(:topic_locale, topic:) topic.update!(title: "this is an updated title") - expect(topic.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to be_nil + expect(DiscourseTranslator::TopicLocale.where(topic:)).to be_empty + expect(DiscourseTranslator::TopicLocale.find_by(topic:)).to be_nil + end + + describe "#set_translation" do + it "creates new translation" do + topic.set_translation("en", "Hello") + + translation = topic.translations.find_by(locale: "en") + expect(translation.translation).to eq("Hello") + end + + it "updates existing translation" do + topic.set_translation("en", "Hello") + topic.set_translation("en", "Updated hello") + + expect(topic.translations.where(locale: "en").count).to eq(1) + expect(topic.translation_for("en")).to eq("Updated hello") + end + + it "converts underscore to hyphen in locale" do + topic.set_translation("en_US", "Hello") + + expect(topic.translations.find_by(locale: "en-US")).to be_present + expect(topic.translations.find_by(locale: "en_US")).to be_nil + end + end + + describe "#translation_for" do + it "returns nil when translation doesn't exist" do + expect(topic.translation_for("fr")).to be_nil + end + + it "returns translation when it exists" do + topic.set_translation("es", "Hola") + expect(topic.translation_for("es")).to eq("Hola") + end + end + + describe "#set_locale" do + it "creates new locale" do + topic.set_detected_locale("en-US") + expect(topic.content_locale.detected_locale).to eq("en-US") + end - expect(topic.custom_fields[::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD]).to be_nil + it "converts underscore to hyphen" do + topic.set_detected_locale("en_US") + expect(topic.content_locale.detected_locale).to eq("en-US") + end end end end diff --git a/spec/models/translation_tables_spec.rb b/spec/models/translation_tables_spec.rb index 070d4909..bb2b8200 100644 --- a/spec/models/translation_tables_spec.rb +++ b/spec/models/translation_tables_spec.rb @@ -4,53 +4,64 @@ module DiscourseTranslator describe MoveTranslationsCustomFieldsToTable do - let!(:batch_size) { 5 } + let!(:batch_size) { 3 } before { described_class.const_set(:BATCH_SIZE, batch_size) } - def create_custom_fields(count) + def create_translation_custom_fields(count) count.times do - post = Fabricate(:post) - topic = Fabricate(:topic) + t_post = Fabricate(:post) + t_topic = Fabricate(:topic) - post.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "pt" - post.save_custom_fields + t_post.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "pt" + t_post.save_custom_fields - topic.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "es" - topic.save_custom_fields + t_topic.custom_fields[DETECTED_LANG_CUSTOM_FIELD] = "es" + t_topic.save_custom_fields - post.custom_fields[TRANSLATED_CUSTOM_FIELD] = { + t_post.custom_fields[TRANSLATED_CUSTOM_FIELD] = { en_GB: "The Romance of the Three Kingdoms", de: "Die Romanze der Drei Königreiche", } - post.save_custom_fields + t_post.save_custom_fields - topic.custom_fields[TRANSLATED_CUSTOM_FIELD] = { + t_topic.custom_fields[TRANSLATED_CUSTOM_FIELD] = { en_GB: "The Romance of the Three Kingdoms", de: "Die Romanze der Drei Königreiche", } - topic.save_custom_fields + t_topic.save_custom_fields end end it "correctly migrates custom fields in batches" do - # batch size is 5 - create_custom_fields(12) + # batch size is 3 + create_translation_custom_fields(4) + # create some random custom fields in between + # to test the migrate loop doesn't end prematurely + 4.times do + post = Fabricate(:post) + post.custom_fields["x"] = "x" + post.save_custom_fields + + topic = Fabricate(:topic) + topic.custom_fields["x"] = "x" + topic.save_custom_fields + end + # another 4 + create_translation_custom_fields(4) migration = described_class.new migration.up - # 12 posts * 2 translations each - expect(PostLocale.count).to eq(12) - expect(PostTranslation.count).to eq(24) + expect(PostLocale.count).to eq(8) + expect(PostTranslation.count).to eq(16) - # 12 topics * 2 translations each - expect(TopicLocale.count).to eq(12) - expect(TopicTranslation.count).to eq(24) + expect(TopicLocale.count).to eq(8) + expect(TopicTranslation.count).to eq(16) - expect(PostLocale.first.detected_locale).to eq("pt") + expect(PostLocale.last.detected_locale).to eq("pt") - expect(PostTranslation.where(post_id: Post.first.id).pluck(:locale, :translation)).to include( + expect(PostTranslation.where(post_id: Post.last.id).pluck(:locale, :translation)).to include( ["en_GB", "The Romance of the Three Kingdoms"], ["de", "Die Romanze der Drei Königreiche"], ) From 069d0435542f6cc49556f611fecee8af3293c57b Mon Sep 17 00:00:00 2001 From: Nat Date: Fri, 7 Feb 2025 03:30:50 +0800 Subject: [PATCH 3/7] Update tests using custom fields --- ...ove_translations_custom_fields_to_table.rb | 1 + spec/fabricators/post_locale.rb | 5 +++++ spec/fabricators/post_translation.rb | 18 +++++++++++++++ spec/jobs/detect_posts_language_spec.rb | 2 +- spec/lib/guardian_extension_spec.rb | 6 ++--- spec/models/post_spec.rb | 2 +- spec/models/translation_tables_spec.rb | 3 ++- spec/serializers/post_serializer_spec.rb | 6 ++--- spec/services/amazon_spec.rb | 9 ++------ spec/services/base_spec.rb | 3 --- spec/services/discourse_ai_spec.rb | 14 +++++------- spec/services/google_spec.rb | 13 +++-------- spec/services/microsoft_spec.rb | 22 +++++-------------- spec/services/yandex_spec.rb | 6 +---- 14 files changed, 49 insertions(+), 61 deletions(-) create mode 100644 spec/fabricators/post_locale.rb create mode 100644 spec/fabricators/post_translation.rb diff --git a/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb b/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb index 3ca4f0a3..6f43d94e 100644 --- a/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb +++ b/db/migrate/20250205082401_move_translations_custom_fields_to_table.rb @@ -35,6 +35,7 @@ def migrate_custom_fields(model) SELECT #{model}_id, value FROM #{model}_custom_fields WHERE name = 'post_detected_lang' + AND length(value) <= 20 AND id >= :start_id AND id < :end_id ORDER BY id diff --git a/spec/fabricators/post_locale.rb b/spec/fabricators/post_locale.rb new file mode 100644 index 00000000..583c505e --- /dev/null +++ b/spec/fabricators/post_locale.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true +Fabricator(:post_locale, from: DiscourseTranslator::PostLocale) do + post + detected_locale { %w[en de es en-GB ja pt pt-BR].sample } +end diff --git a/spec/fabricators/post_translation.rb b/spec/fabricators/post_translation.rb new file mode 100644 index 00000000..4eb8cb24 --- /dev/null +++ b/spec/fabricators/post_translation.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true +Fabricator(:post_translation, from: DiscourseTranslator::PostTranslation) do + post + locale { %w[en de es en-GB ja pt pt-BR].sample } + translation do |attrs| + { + "en" => "Hello", + "de" => "Hallo", + "es" => "Hola", + "en-GB" => "Hello", + "ja" => "こんにちは", + "pt" => "Olá", + "pt-BR" => "Olá", + }[ + attrs[:locale] + ] + end +end diff --git a/spec/jobs/detect_posts_language_spec.rb b/spec/jobs/detect_posts_language_spec.rb index f0605c7b..1cd88017 100644 --- a/spec/jobs/detect_posts_language_spec.rb +++ b/spec/jobs/detect_posts_language_spec.rb @@ -23,7 +23,7 @@ posts.each do |post| post.reload - expect(post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).not_to be_nil + expect(post.detected_locale).not_to be_nil end expect(Discourse.redis.smembers(redis_key)).to be_empty diff --git a/spec/lib/guardian_extension_spec.rb b/spec/lib/guardian_extension_spec.rb index c9ac4b48..9560fadb 100644 --- a/spec/lib/guardian_extension_spec.rb +++ b/spec/lib/guardian_extension_spec.rb @@ -156,15 +156,13 @@ end it "cannot translate when post has DETECTED_LANG_CUSTOM_FIELD matches locale" do - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "pt" - post.save + post.set_detected_locale("pt") expect(guardian.can_translate?(post)).to eq(false) end it "can translate when post has DETECTED_LANG_CUSTOM_FIELD does not match locale" do - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "jp" - post.save + post.set_detected_locale("jp") expect(guardian.can_translate?(post)).to eq(true) end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index bae48a6f..7cf507ec 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -11,7 +11,7 @@ it "should reset translation data when post title has been updated" do Fabricate(:post_translation, post:) Fabricate(:post_locale, post:) - post.update!(title: "this is an updated title") + post.update!(raw: "this is an updated title") expect(DiscourseTranslator::PostLocale.where(post:)).to be_empty expect(DiscourseTranslator::PostLocale.find_by(post:)).to be_nil diff --git a/spec/models/translation_tables_spec.rb b/spec/models/translation_tables_spec.rb index bb2b8200..2dbcb03a 100644 --- a/spec/models/translation_tables_spec.rb +++ b/spec/models/translation_tables_spec.rb @@ -90,7 +90,8 @@ def create_translation_custom_fields(count) post.save_custom_fields(true) migration = described_class.new - migration.up + expect { migration.up }.not_to raise_error + expect(PostLocale.count).to eq(0) expect(PostTranslation.count).to eq(0) end diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 04d46b48..e2b95c80 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -63,15 +63,13 @@ end it "cannot translate when post has DETECTED_LANG_CUSTOM_FIELD matches locale" do - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "pt" - post.save + post.set_detected_locale("pt") expect(serializer.can_translate).to eq(false) end it "can translate when post has DETECTED_LANG_CUSTOM_FIELD does not match locale" do - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "jp" - post.save + post.set_detected_locale("jp") expect(serializer.can_translate).to eq(true) end diff --git a/spec/services/amazon_spec.rb b/spec/services/amazon_spec.rb index 87c26b18..d8b4e0fd 100644 --- a/spec/services/amazon_spec.rb +++ b/spec/services/amazon_spec.rb @@ -37,11 +37,7 @@ it "should store the detected language in a custom field" do expect(described_class.detect(post)).to eq(detected_lang) - 2.times do - expect(post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to eq( - detected_lang, - ) - end + expect(post.detected_locale).to eq(detected_lang) end it "should fail graciously when the cooked translated text is blank" do @@ -65,8 +61,7 @@ }, ) described_class.stubs(:client).returns(client) - post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "en" - post.save_custom_fields + post.set_detected_locale("en") I18n.stubs(:locale).returns(:es) end diff --git a/spec/services/base_spec.rb b/spec/services/base_spec.rb index 3d9aa5f0..0f224ddd 100644 --- a/spec/services/base_spec.rb +++ b/spec/services/base_spec.rb @@ -116,7 +116,6 @@ class EmptyTranslator < DiscourseTranslator::Base end it "performs detection if no cached result" do - TestTranslator.save_detected_locale(post) { nil } TestTranslator.expects(:detect!).with(post).returns("es") expect(TestTranslator.detect(post)).to eq("es") @@ -147,7 +146,6 @@ class EmptyTranslator < DiscourseTranslator::Base it "raises error when translation not supported" do TestTranslator.save_detected_locale(post) { "xx" } - TestTranslator.save_translation(post) { nil } TestTranslator.expects(:translate_supported?).with("xx", :en).returns(false) expect { TestTranslator.translate(post) }.to raise_error(DiscourseTranslator::TranslatorError) @@ -155,7 +153,6 @@ class EmptyTranslator < DiscourseTranslator::Base it "performs translation when needed" do TestTranslator.save_detected_locale(post) { "es" } - TestTranslator.save_translation(post) { nil } TestTranslator.expects(:translate!).returns("hello") expect(TestTranslator.translate(post)).to eq(%w[es hello]) diff --git a/spec/services/discourse_ai_spec.rb b/spec/services/discourse_ai_spec.rb index c7cf1188..e46b4b4b 100644 --- a/spec/services/discourse_ai_spec.rb +++ b/spec/services/discourse_ai_spec.rb @@ -26,23 +26,19 @@ end end - describe ".detect" do - it "stores the detected language in a custom field" do + describe ".detect!" do + it "stores the detected language" do locale = "de" DiscourseAi::Completions::Llm.with_prepared_responses(["de"]) do - DiscourseTranslator::DiscourseAi.detect(post) - post.save_custom_fields + DiscourseTranslator::DiscourseAi.detect!(post) end - expect(post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to eq locale + expect(post.detected_locale).to eq locale end end describe ".translate" do - before do - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "de" - post.save_custom_fields - end + before { post.set_detected_locale("de") } it "translates the post and returns [locale, translated_text]" do DiscourseAi::Completions::Llm.with_prepared_responses( diff --git a/spec/services/google_spec.rb b/spec/services/google_spec.rb index 746ff66a..01d92553 100644 --- a/spec/services/google_spec.rb +++ b/spec/services/google_spec.rb @@ -35,12 +35,7 @@ .once expect(described_class.detect(post)).to eq(detected_lang) - - 2.times do - expect(post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to eq( - detected_lang, - ) - end + expect(post.detected_locale).to eq(detected_lang) end it "should truncate string to 1000 characters" do @@ -168,8 +163,7 @@ end it "returns error with source and target locale when translation is not supported" do - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "cat" - post.save_custom_fields + post.set_detected_locale("cat") I18n.stubs(:locale).returns(:dog) Excon.expects(:post).returns( @@ -184,8 +178,7 @@ it "truncates text for translation to max_characters_per_translation setting" do SiteSetting.max_characters_per_translation = 50 post.cooked = "a" * 100 - post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "de" - post.save_custom_fields + post.set_detected_locale("de") body = { q: post.cooked.truncate(SiteSetting.max_characters_per_translation, omission: nil), source: "de", diff --git a/spec/services/microsoft_spec.rb b/spec/services/microsoft_spec.rb index 1ca1f22a..7c2cfb80 100644 --- a/spec/services/microsoft_spec.rb +++ b/spec/services/microsoft_spec.rb @@ -20,12 +20,10 @@ def detect_endpoint before { SiteSetting.translator_azure_subscription_key = "e1bba646088021aaf1ef972a48" } shared_examples "language detected" do - it "stores detected language in a custom field" do + it "stores detected language" do described_class.detect(post) - expect(post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to eq( - detected_lang, - ) + expect(post.detected_locale).to eq(detected_lang) end end @@ -131,8 +129,7 @@ def translate_endpoint end before do - post.custom_fields = { DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD => "en" } - post.save_custom_fields + post.set_detected_locale("en") SiteSetting.translator_azure_subscription_key = "e1bba646088021aaf1ef972a48" end @@ -161,21 +158,14 @@ def translate_endpoint it "returns stored translation if post has already been translated" do I18n.locale = "en" - post.custom_fields = { - DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD => "tr", - DiscourseTranslator::TRANSLATED_CUSTOM_FIELD => { - "en" => "some english text", - }, - } - - post.save_custom_fields + post.set_detected_locale("tr") + post.set_translation("en", "some english text") expect(described_class.translate(post)).to eq(["tr", "some english text"]) end it "raises an error if detected language of the post is not supported" do - post.custom_fields = { DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD => "donkey" } - post.save_custom_fields + post.set_detected_locale("donkey") expect { described_class.translate(post) }.to raise_error( DiscourseTranslator::TranslatorError, diff --git a/spec/services/yandex_spec.rb b/spec/services/yandex_spec.rb index 24ce0e41..3a59ecf1 100644 --- a/spec/services/yandex_spec.rb +++ b/spec/services/yandex_spec.rb @@ -28,11 +28,7 @@ .once expect(described_class.detect(post)).to eq(detected_lang) - 2.times do - expect(post.custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to eq( - detected_lang, - ) - end + expect(post.detected_locale).to eq(detected_lang) end end From e45e0b375d1a825bc221f2eb6414530f7f4b75cf Mon Sep 17 00:00:00 2001 From: Nat Date: Fri, 7 Feb 2025 13:58:54 +0800 Subject: [PATCH 4/7] Annotations for new tables --- app/models/discourse_translator/post_locale.rb | 11 +++++++++++ .../discourse_translator/post_translation.rb | 16 ++++++++++++++++ app/models/discourse_translator/topic_locale.rb | 11 +++++++++++ .../discourse_translator/topic_translation.rb | 16 ++++++++++++++++ 4 files changed, 54 insertions(+) diff --git a/app/models/discourse_translator/post_locale.rb b/app/models/discourse_translator/post_locale.rb index b318504f..305c7b62 100644 --- a/app/models/discourse_translator/post_locale.rb +++ b/app/models/discourse_translator/post_locale.rb @@ -10,3 +10,14 @@ class PostLocale < ActiveRecord::Base validates :detected_locale, presence: true end end + +# == Schema Information +# +# Table name: discourse_translator_post_locales +# +# id :bigint not null, primary key +# post_id :integer not null +# detected_locale :string(20) not null +# created_at :datetime not null +# updated_at :datetime not null +# diff --git a/app/models/discourse_translator/post_translation.rb b/app/models/discourse_translator/post_translation.rb index 66be9c8b..a13f8acb 100644 --- a/app/models/discourse_translator/post_translation.rb +++ b/app/models/discourse_translator/post_translation.rb @@ -16,3 +16,19 @@ def self.translation_for(post_id, locale) end end end + +# == Schema Information +# +# Table name: discourse_translator_post_translations +# +# id :bigint not null, primary key +# post_id :integer not null +# locale :string not null +# translation :text not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# idx_on_post_id_locale_0cc3d81e5b (post_id,locale) UNIQUE +# diff --git a/app/models/discourse_translator/topic_locale.rb b/app/models/discourse_translator/topic_locale.rb index ccd09f52..6646e763 100644 --- a/app/models/discourse_translator/topic_locale.rb +++ b/app/models/discourse_translator/topic_locale.rb @@ -10,3 +10,14 @@ class TopicLocale < ActiveRecord::Base validates :detected_locale, presence: true end end + +# == Schema Information +# +# Table name: discourse_translator_topic_locales +# +# id :bigint not null, primary key +# topic_id :integer not null +# detected_locale :string(20) not null +# created_at :datetime not null +# updated_at :datetime not null +# diff --git a/app/models/discourse_translator/topic_translation.rb b/app/models/discourse_translator/topic_translation.rb index 001b2742..07384e05 100644 --- a/app/models/discourse_translator/topic_translation.rb +++ b/app/models/discourse_translator/topic_translation.rb @@ -16,3 +16,19 @@ def self.translation_for(topic_id, locale) end end end + +# == Schema Information +# +# Table name: discourse_translator_topic_translations +# +# id :bigint not null, primary key +# topic_id :integer not null +# locale :string not null +# translation :text not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# idx_on_topic_id_locale_70b2f83213 (topic_id,locale) UNIQUE +# From 06c45b5f75ddd715d011a101e869e1d25e48c9e6 Mon Sep 17 00:00:00 2001 From: Nat Date: Fri, 7 Feb 2025 17:26:09 +0800 Subject: [PATCH 5/7] Prevent n+1 when loading topic post's translation metadata --- .../topic_view_serializer_extension.rb | 12 +++++++ plugin.rb | 1 + .../serializers/topic_view_serializer_spec.rb | 33 +++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 lib/discourse_translator/topic_view_serializer_extension.rb create mode 100644 spec/serializers/topic_view_serializer_spec.rb diff --git a/lib/discourse_translator/topic_view_serializer_extension.rb b/lib/discourse_translator/topic_view_serializer_extension.rb new file mode 100644 index 00000000..546772b8 --- /dev/null +++ b/lib/discourse_translator/topic_view_serializer_extension.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module DiscourseTranslator + module TopicViewSerializerExtension + def posts + if SiteSetting.translator_enabled? + object.instance_variable_set(:@posts, object.posts.includes(:content_locale)) + end + super + end + end +end diff --git a/plugin.rb b/plugin.rb index 5dfdf8c0..1b171cd8 100644 --- a/plugin.rb +++ b/plugin.rb @@ -31,6 +31,7 @@ module ::DiscourseTranslator Guardian.prepend(DiscourseTranslator::GuardianExtension) Post.prepend(DiscourseTranslator::PostExtension) Topic.prepend(DiscourseTranslator::TopicExtension) + TopicViewSerializer.prepend(DiscourseTranslator::TopicViewSerializerExtension) end on(:post_process_cooked) do |_, post| diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb new file mode 100644 index 00000000..58c1bc79 --- /dev/null +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe TopicViewSerializer do + fab!(:user) + fab!(:topic) + fab!(:post1) { Fabricate(:post, topic: topic).set_detected_locale("en") } + fab!(:post2) { Fabricate(:post, topic: topic).set_detected_locale("es") } + fab!(:post3) { Fabricate(:post, topic: topic).set_detected_locale("ja") } + + before do + SiteSetting.translator_enabled = true + SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}" + SiteSetting.restrict_translation_by_poster_group = "#{Group::AUTO_GROUPS[:everyone]}" + end + + it "preloads translations without N+1 queries" do + topic_view = TopicView.new(topic) + serializer = TopicViewSerializer.new(topic_view, scope: Guardian.new(user), root: false) + + # ensure translation data is included in the JSON + json = {} + queries = track_sql_queries { json = serializer.as_json } + posts_json = json[:post_stream][:posts] + expect(posts_json.map { |p| p[:can_translate] }).to eq([false, true, true]) + + translation_queries = queries.count { |q| q.include?("discourse_translator_post_locales") } + expect(translation_queries).to eq(1) # would be 3 (posts) if not preloaded + + expect(topic_view.posts.first.association(:content_locale)).to be_loaded + end +end From 33aab2b754143122fb5b31556deff61c1fd75cde Mon Sep 17 00:00:00 2001 From: Nat Date: Fri, 7 Feb 2025 17:30:48 +0800 Subject: [PATCH 6/7] Undo job frequency changes (put in separate pr) --- app/jobs/scheduled/detect_posts_language.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/jobs/scheduled/detect_posts_language.rb b/app/jobs/scheduled/detect_posts_language.rb index d7361b67..04a7f7c6 100644 --- a/app/jobs/scheduled/detect_posts_language.rb +++ b/app/jobs/scheduled/detect_posts_language.rb @@ -3,10 +3,10 @@ module ::Jobs class DetectPostsLanguage < ::Jobs::Scheduled sidekiq_options retry: false - every 1.minutes + every 5.minutes - BATCH_SIZE = 10 - MAX_QUEUE_SIZE = 100 + BATCH_SIZE = 100 + MAX_QUEUE_SIZE = 1000 def execute(args) return unless SiteSetting.translator_enabled From 723402b00734a0862e14fe404f339611fd47e593 Mon Sep 17 00:00:00 2001 From: Nat Date: Fri, 7 Feb 2025 18:05:17 +0800 Subject: [PATCH 7/7] Remove all traces of CUSTOM_FIELD except in the migration spec --- plugin.rb | 2 -- spec/jobs/detect_posts_language_spec.rb | 2 +- spec/lib/guardian_extension_spec.rb | 7 ++++--- spec/models/translation_tables_spec.rb | 3 +++ spec/serializers/post_serializer_spec.rb | 7 ++++--- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/plugin.rb b/plugin.rb index 1b171cd8..7e927c71 100644 --- a/plugin.rb +++ b/plugin.rb @@ -16,8 +16,6 @@ module ::DiscourseTranslator PLUGIN_NAME = "discourse-translator".freeze - DETECTED_LANG_CUSTOM_FIELD = "post_detected_lang".freeze - TRANSLATED_CUSTOM_FIELD = "translated_text".freeze LANG_DETECT_NEEDED = "lang_detect_needed".freeze end diff --git a/spec/jobs/detect_posts_language_spec.rb b/spec/jobs/detect_posts_language_spec.rb index 1cd88017..6dfd1f3c 100644 --- a/spec/jobs/detect_posts_language_spec.rb +++ b/spec/jobs/detect_posts_language_spec.rb @@ -35,7 +35,7 @@ posts.each do |post| post.reload - expect(post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]).to be_nil + expect(post.detected_locale).to be_nil end expect(Discourse.redis.smembers(redis_key)).to match_array(posts.map(&:id).map(&:to_s)) diff --git a/spec/lib/guardian_extension_spec.rb b/spec/lib/guardian_extension_spec.rb index 9560fadb..8dafd241 100644 --- a/spec/lib/guardian_extension_spec.rb +++ b/spec/lib/guardian_extension_spec.rb @@ -151,17 +151,18 @@ describe "locale is :xx" do before { I18n.stubs(:locale).returns(:pt) } - it "cannot translate when post does not have DETECTED_LANG_CUSTOM_FIELD" do + it "cannot translate when post does not have detected locale" do + expect(post.detected_locale).to eq(nil) expect(guardian.can_translate?(post)).to eq(false) end - it "cannot translate when post has DETECTED_LANG_CUSTOM_FIELD matches locale" do + it "cannot translate when post detected locale matches i18n locale" do post.set_detected_locale("pt") expect(guardian.can_translate?(post)).to eq(false) end - it "can translate when post has DETECTED_LANG_CUSTOM_FIELD does not match locale" do + it "can translate when post detected locale does not match i18n locale" do post.set_detected_locale("jp") expect(guardian.can_translate?(post)).to eq(true) diff --git a/spec/models/translation_tables_spec.rb b/spec/models/translation_tables_spec.rb index 2dbcb03a..091e4253 100644 --- a/spec/models/translation_tables_spec.rb +++ b/spec/models/translation_tables_spec.rb @@ -4,6 +4,9 @@ module DiscourseTranslator describe MoveTranslationsCustomFieldsToTable do + DETECTED_LANG_CUSTOM_FIELD = "post_detected_lang".freeze + TRANSLATED_CUSTOM_FIELD = "translated_text".freeze + let!(:batch_size) { 3 } before { described_class.const_set(:BATCH_SIZE, batch_size) } diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index e2b95c80..c3b230cb 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -58,17 +58,18 @@ I18n.stubs(:locale).returns(:pt) end - it "cannot translate when post does not have DETECTED_LANG_CUSTOM_FIELD" do + it "cannot translate when post does not have detected locale" do + expect(post.detected_locale).to eq(nil) expect(serializer.can_translate).to eq(false) end - it "cannot translate when post has DETECTED_LANG_CUSTOM_FIELD matches locale" do + it "cannot translate when post detected locale matches i18n locale" do post.set_detected_locale("pt") expect(serializer.can_translate).to eq(false) end - it "can translate when post has DETECTED_LANG_CUSTOM_FIELD does not match locale" do + it "can translate when post detected locale does not match i18n locale" do post.set_detected_locale("jp") expect(serializer.can_translate).to eq(true)