diff --git a/app/models/concerns/discourse_translator/translatable.rb b/app/models/concerns/discourse_translator/translatable.rb index ad8a202a..1a2dbaf1 100644 --- a/app/models/concerns/discourse_translator/translatable.rb +++ b/app/models/concerns/discourse_translator/translatable.rb @@ -30,7 +30,10 @@ def set_translation(locale, text) def translation_for(locale) locale = locale.to_s.gsub("_", "-") - translations.find_by(locale: locale)&.translation + # this is a tricky perf balancing act when loading translations for a topic with many posts. + # the topic_view_serializer includes(:translations) for posts in the topic, + # the alternative is to do a find_by(locale: locale) which would result in a query per post. + translations.to_a.find { |t| t.locale == locale }&.translation end def detected_locale diff --git a/app/models/discourse_translator/post_translation.rb b/app/models/discourse_translator/post_translation.rb index a13f8acb..cec77815 100644 --- a/app/models/discourse_translator/post_translation.rb +++ b/app/models/discourse_translator/post_translation.rb @@ -10,10 +10,6 @@ class PostTranslation < ActiveRecord::Base 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_translation.rb b/app/models/discourse_translator/topic_translation.rb index 07384e05..da67bfaf 100644 --- a/app/models/discourse_translator/topic_translation.rb +++ b/app/models/discourse_translator/topic_translation.rb @@ -10,10 +10,6 @@ class TopicTranslation < ActiveRecord::Base 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/assets/javascripts/discourse/components/show-original-content.gjs b/assets/javascripts/discourse/components/show-original-content.gjs index ce8e19a0..e9f2e4e0 100644 --- a/assets/javascripts/discourse/components/show-original-content.gjs +++ b/assets/javascripts/discourse/components/show-original-content.gjs @@ -6,6 +6,13 @@ import DButton from "discourse/components/d-button"; import concatClass from "discourse/helpers/concat-class"; export default class ShowOriginalContent extends Component { + static shouldRender(args) { + return ( + args.topic.is_translated || + args.topic.postStream.posts.some(({ is_translated }) => is_translated) + ); + } + @service router; @tracked isTranslated = true; diff --git a/lib/discourse_translator/guardian_extension.rb b/lib/discourse_translator/guardian_extension.rb index ae80d777..aecd3940 100644 --- a/lib/discourse_translator/guardian_extension.rb +++ b/lib/discourse_translator/guardian_extension.rb @@ -24,15 +24,17 @@ def can_detect_language?(post) def can_translate?(post) return false if post.user&.bot? return false if !user_group_allow_translate? - return false if post.locale_matches?(I18n.locale) # we want to return false if the post is created within a short buffer ago, # this prevents the 🌐from appearing and then disappearing if the lang is same as user's lang return false if post.created_at > POST_DETECTION_BUFFER.ago && post.detected_locale.blank? if SiteSetting.experimental_inline_translation - post.translation_for(I18n.locale).nil? + locale = DiscourseTranslator::InlineTranslation.effective_locale + return false if post.locale_matches?(locale) + post.translation_for(locale).nil? else + return false if post.locale_matches?(I18n.locale) poster_group_allow_translate?(post) end end diff --git a/lib/discourse_translator/inline_translation.rb b/lib/discourse_translator/inline_translation.rb index 4eec745f..6aec3998 100644 --- a/lib/discourse_translator/inline_translation.rb +++ b/lib/discourse_translator/inline_translation.rb @@ -2,41 +2,69 @@ module DiscourseTranslator class InlineTranslation + def self.effective_locale + if LocaleMatcher.user_locale_is_default? || LocaleMatcher.user_locale_in_target_languages? + I18n.locale + else + SiteSetting.default_locale + end + end + def inject(plugin) + # since locales are eager loaded but translations may not, + # always return early if topic and posts are in the user's effective_locale. + # this prevents the need to load translations. + plugin.register_modifier(:basic_post_serializer_cooked) do |cooked, serializer| if !SiteSetting.experimental_inline_translation || - serializer.scope&.request&.params&.[]("show") == "original" || - serializer.object.detected_locale == I18n.locale.to_s.gsub("_", "-") + serializer.object.locale_matches?(InlineTranslation.effective_locale) || + serializer.scope&.request&.params&.[]("show") == "original" cooked else - serializer.object.translation_for(I18n.locale).presence + serializer.object.translation_for(InlineTranslation.effective_locale).presence end end plugin.register_modifier(:topic_serializer_fancy_title) do |fancy_title, serializer| if !SiteSetting.experimental_inline_translation || - serializer.scope&.request&.params&.[]("show") == "original" || - serializer.object.locale_matches?(I18n.locale) + serializer.object.locale_matches?(InlineTranslation.effective_locale) || + serializer.scope&.request&.params&.[]("show") == "original" fancy_title else - serializer.object.translation_for(I18n.locale).presence&.then { |t| Topic.fancy_title(t) } + serializer + .object + .translation_for(InlineTranslation.effective_locale) + .presence + &.then { |t| Topic.fancy_title(t) } end end plugin.register_modifier(:topic_view_serializer_fancy_title) do |fancy_title, serializer| if !SiteSetting.experimental_inline_translation || - serializer.scope&.request&.params&.[]("show") == "original" || - serializer.object.topic.locale_matches?(I18n.locale) + serializer.object.topic.locale_matches?(InlineTranslation.effective_locale) || + serializer.scope&.request&.params&.[]("show") == "original" fancy_title else serializer .object .topic - .translation_for(I18n.locale) + .translation_for(InlineTranslation.effective_locale) .presence &.then { |t| Topic.fancy_title(t) } end end + + plugin.add_to_serializer(:basic_post, :is_translated) do + SiteSetting.experimental_inline_translation && + !object.locale_matches?(InlineTranslation.effective_locale) && + object.translation_for(InlineTranslation.effective_locale).present? + end + + plugin.add_to_serializer(:topic_view, :is_translated) do + SiteSetting.experimental_inline_translation && + !object.topic.locale_matches?(InlineTranslation.effective_locale) && + object.topic.translation_for(InlineTranslation.effective_locale).present? + end end end end diff --git a/lib/discourse_translator/locale_matcher.rb b/lib/discourse_translator/locale_matcher.rb new file mode 100644 index 00000000..f5786b2b --- /dev/null +++ b/lib/discourse_translator/locale_matcher.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module DiscourseTranslator + class LocaleMatcher + def self.user_locale_in_target_languages? + # e.g. "en|es|pt_BR" vs :en_UK + SiteSetting.automatic_translation_target_languages.split("|").include?(I18n.locale.to_s) + end + + def self.user_locale_is_default? + # e.g. :en_UK vs "en_UK" + I18n.locale.to_s == SiteSetting.default_locale + end + end +end diff --git a/lib/discourse_translator/topic_view_serializer_extension.rb b/lib/discourse_translator/topic_view_serializer_extension.rb index 546772b8..a98b4724 100644 --- a/lib/discourse_translator/topic_view_serializer_extension.rb +++ b/lib/discourse_translator/topic_view_serializer_extension.rb @@ -4,7 +4,20 @@ module DiscourseTranslator module TopicViewSerializerExtension def posts if SiteSetting.translator_enabled? - object.instance_variable_set(:@posts, object.posts.includes(:content_locale)) + posts_query = object.posts.includes(:content_locale) + # this is kind of a micro-optimization. + # we do not want to eager load translations if the user is using the site's language. + # we will only load them if the user is using a different language that is supported by the site. + if SiteSetting.experimental_inline_translation && !LocaleMatcher.user_locale_is_default? && + LocaleMatcher.user_locale_in_target_languages? + locale = InlineTranslation.effective_locale.to_s.gsub("_", "-") + posts_query = + posts_query + .includes(:translations) + .references(:translations) + .where(translations: { locale: [nil, locale] }) + end + object.instance_variable_set(:@posts, posts_query) end super end diff --git a/spec/lib/guardian_extension_spec.rb b/spec/lib/guardian_extension_spec.rb index 1b643f75..33f248b5 100644 --- a/spec/lib/guardian_extension_spec.rb +++ b/spec/lib/guardian_extension_spec.rb @@ -177,7 +177,12 @@ end describe "when experimental_inline_translation enabled" do - before { SiteSetting.experimental_inline_translation = true } + before do + SiteSetting.experimental_inline_translation = true + + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 + SiteSetting.automatic_translation_target_languages = "pt" + end describe "logged in user" do it "cannot translate when user is not in restrict_translation_by_group" do @@ -190,7 +195,7 @@ before { SiteSetting.restrict_translation_by_group = "#{group.id}" } it "cannot translate when post has translation for user locale" do - post.set_detected_locale("jp") + post.set_detected_locale("ja") post.set_translation("pt", "Olá, mundo!") expect(guardian.can_translate?(post)).to eq(false) diff --git a/spec/serializers/basic_topic_serializer_spec.rb b/spec/serializers/basic_topic_serializer_spec.rb index 616eb527..9a64fc46 100644 --- a/spec/serializers/basic_topic_serializer_spec.rb +++ b/spec/serializers/basic_topic_serializer_spec.rb @@ -20,6 +20,9 @@ topic.title = original_title SiteSetting.experimental_inline_translation = true I18n.locale = "ja" + + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 + SiteSetting.automatic_translation_target_languages = "ja" end def serialize_topic(guardian_user: user, params: {}) @@ -53,6 +56,14 @@ def serialize_topic(guardian_user: user, params: {}) expect(serialize_topic.fancy_title).to eq(topic.fancy_title) end + it "does not replace fancy_title when user's locale is not in target languages" do + I18n.locale = "es" + topic.set_detected_locale("en") + topic.set_translation("es", jap_title) + + expect(serialize_topic.fancy_title).to eq(topic.fancy_title) + end + it "returns translated title in fancy_title when translation exists for current locale" do topic.set_translation("ja", jap_title) expect(serialize_topic.fancy_title).to eq("<h1>フス・ロ・ダ・ア</h1>") diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 20697951..0ee949f3 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -56,6 +56,63 @@ end end + describe "#is_translated" do + fab!(:post) + + it "returns false when translator disabled" do + SiteSetting.translator_enabled = false + serializer = PostSerializer.new(post, scope: Guardian.new) + + expect(serializer.is_translated).to eq(false) + end + + it "returns false when experimental inline translation disabled" do + SiteSetting.translator_enabled = true + SiteSetting.experimental_inline_translation = false + serializer = PostSerializer.new(post, scope: Guardian.new) + + expect(serializer.is_translated).to eq(false) + end + + it "returns true when there is a translation for the user's locale in target languages" do + SiteSetting.translator_enabled = true + SiteSetting.experimental_inline_translation = true + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 + SiteSetting.automatic_translation_target_languages = "ja" + I18n.locale = "ja" + post.set_detected_locale("en") + post.set_translation("ja", "こんにちは") + serializer = PostSerializer.new(post, scope: Guardian.new) + + expect(serializer.is_translated).to eq(true) + end + + it "returns false when there is a translation for the user's locale not in target languages" do + SiteSetting.translator_enabled = true + SiteSetting.experimental_inline_translation = true + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 + SiteSetting.automatic_translation_target_languages = "es" + I18n.locale = "ja" + post.set_detected_locale("en") + post.set_translation("ja", "こんにちは") + serializer = PostSerializer.new(post, scope: Guardian.new) + + expect(serializer.is_translated).to eq(false) + end + + it "returns false when there is no translation for the current locale in target languages" do + SiteSetting.translator_enabled = true + SiteSetting.experimental_inline_translation = true + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 + SiteSetting.automatic_translation_target_languages = "ja" + I18n.locale = "ja" + post.set_translation("es", "Hola") + serializer = PostSerializer.new(post, scope: Guardian.new) + + expect(serializer.is_translated).to eq(false) + end + end + describe "#cooked" do def serialize_post(guardian_user: user, params: {}) env = { "action_dispatch.request.parameters" => params, "REQUEST_METHOD" => "GET" } @@ -76,6 +133,8 @@ def serialize_post(guardian_user: user, params: {}) it "does not return translated_cooked when show=original param is present" do I18n.locale = "ja" + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 + SiteSetting.automatic_translation_target_languages = "ja" post.set_translation("ja", "こんにちは") expect(serialize_post(params: { "show" => "original" }).cooked).to eq(post.cooked) @@ -90,11 +149,17 @@ def serialize_post(guardian_user: user, params: {}) expect(serialize_post.cooked).to eq(post.cooked) end - it "returns translated content based on locale" do - I18n.locale = "ja" + it "returns translated content based on locale presence in target languages" do + SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 1 post.set_translation("ja", "こんにちは") post.set_translation("es", "Hola") + I18n.locale = "ja" + + SiteSetting.automatic_translation_target_languages = "ja" expect(serialize_post.cooked).to eq("こんにちは") + + SiteSetting.automatic_translation_target_languages = "es" + expect(serialize_post.cooked).to eq(post.cooked) end it "does not return translated_cooked when plugin is disabled" do diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index e618ed37..d23294f0 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -3,11 +3,7 @@ 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 @@ -15,70 +11,198 @@ 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) + describe "preloading" do + fab!(:user) + fab!(:en_post) do + post = Fabricate(:post, topic: topic) + post.set_detected_locale("en") + post + end + fab!(:es_post) do + post = Fabricate(:post, topic: topic) + post.set_detected_locale("es") + post + end + fab!(:ja_post) do + post = Fabricate(:post, topic: topic) + post.set_detected_locale("ja") + post + end - # 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]) + it "always preloads locale without N+1 queries" do + topic_view = TopicView.new(topic) + serializer = TopicViewSerializer.new(topic_view, scope: Guardian.new(user), root: false) - translation_queries = queries.count { |q| q.include?("discourse_translator_post_locales") } - expect(translation_queries).to eq(1) # would be 3 (posts) if not preloaded + json = {} + queries = track_sql_queries { json = serializer.as_json } + expect(json[:post_stream][:posts].map { |p| p[:can_translate] }).to eq([false, true, true]) - expect(topic_view.posts.first.association(:content_locale)).to be_loaded - end + translation_queries = queries.count { |q| q.include?("discourse_translator_post_locales") } + expect(translation_queries).to eq(1) # would be 3 (posts) if not preloaded - describe "#fancy_title" do - fab!(:user) { Fabricate(:user, locale: "ja") } - fab!(:topic) + expect(topic_view.posts.first.association(:content_locale)).to be_loaded + end - let!(:guardian) { Guardian.new(user) } - let!(:original_title) { "