DEV: Move language detection out of the post serializer (#193)

Language detection is added to the queue every time the post is serialized. We'll move this into the post_process_cooked event instead, since it is not a good idea to overload a serializer.

This is a follow-up from the move for queueing translations to redis here 8fce768.

Author: nattsw

lib/discourse_translator/guardian_extension.rb

@@ -11,4 +11,11 @@ def poster_group_allow_translate?(post)
     return false if post.user.nil?
     post.user.in_any_groups?(SiteSetting.restrict_translation_by_poster_group_map)
   end
+
+  def can_detect_language?(post)
+    (
+      SiteSetting.restrict_translation_by_poster_group_map.empty? ||
+        post&.user&.in_any_groups?(SiteSetting.restrict_translation_by_poster_group_map)
+    ) && post.raw.present? && post.post_type != Post.types[:small_action]
+  end
 end

plugin.rb

@@ -37,23 +37,21 @@ module ::DiscourseTranslator
     Topic.prepend(DiscourseTranslator::TopicExtension)
   end
 
-  add_to_serializer :post, :can_translate do
-    return false if !SiteSetting.translator_enabled
-    if !scope.user_group_allow_translate? || !scope.poster_group_allow_translate?(object)
-      return false
+  on(:post_process_cooked) do |_, post|
+    if Guardian.new.can_detect_language?(post)
+      Discourse.redis.sadd?(DiscourseTranslator::LANG_DETECT_NEEDED, post.id)
     end
-    return false if raw.blank? || post_type == Post.types[:small_action]
+  end
+
+  add_to_serializer :post, :can_translate do
+    return false if !scope.user_group_allow_translate?
 
     detected_lang = post_custom_fields[::DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD]
+    return false if detected_lang.blank?
 
-    if !detected_lang
-      Discourse.redis.sadd?(DiscourseTranslator::LANG_DETECT_NEEDED, object.id)
-      false
-    else
-      detected_lang.to_sym != I18n.locale &&
-        "DiscourseTranslator::#{SiteSetting.translator}".constantize.language_supported?(
-          detected_lang,
-        )
-    end
+    detected_lang.to_sym != I18n.locale &&
+      "DiscourseTranslator::#{SiteSetting.translator}".constantize.language_supported?(
+        detected_lang,
+      )
   end
 end

spec/lib/guardian_extension_spec.rb

@@ -77,4 +77,40 @@
       end
     end
   end
+
+  describe "#can_detect_language?" do
+    fab!(:group)
+    fab!(:user) { Fabricate(:user, groups: [group]) }
+    fab!(:post) { Fabricate(:post, user: user, raw: "Hello, world!") }
+    let(:guardian) { Guardian.new(user) }
+
+    it "returns false when the post user is not in restrict_translation_by_poster_group" do
+      SiteSetting.restrict_translation_by_poster_group = "#{Fabricate(:group).id}"
+
+      expect(guardian.can_detect_language?(post)).to eq(false)
+    end
+
+    context "when post author is in allowed groups" do
+      before do
+        SiteSetting.restrict_translation_by_group = "#{group.id}"
+        SiteSetting.restrict_translation_by_poster_group = "#{group.id}"
+      end
+
+      it "returns true when the post is not a small action post" do
+        expect(guardian.can_detect_language?(post)).to eq(true)
+      end
+
+      it "returns false when the post is a small action post" do
+        post.update!(post_type: Post.types[:small_action])
+
+        expect(guardian.can_detect_language?(post)).to eq(false)
+      end
+
+      it "returns false when the post raw is empty" do
+        expect { post.update(raw: "") }.to change { guardian.can_detect_language?(post) }.from(
+          true,
+        ).to(false)
+      end
+    end
+  end
 end

spec/models/post_spec.rb

@@ -27,4 +27,51 @@
       expect(post.custom_fields[::DiscourseTranslator::TRANSLATED_CUSTOM_FIELD]).to be_nil
     end
   end
+
+  describe "queueing post for language detection" do
+    fab!(:group)
+    fab!(:topic)
+    fab!(:user) { Fabricate(:user, groups: [group]) }
+
+    before do
+      Jobs.run_immediately!
+      SiteSetting.create_topic_allowed_groups = Group::AUTO_GROUPS[:everyone]
+    end
+
+    it "queues the post for language detection when user and posts are in the right group" do
+      SiteSetting.restrict_translation_by_poster_group = "#{group.id}"
+      post =
+        PostCreator.new(
+          user,
+          {
+            title: "a topic about cats",
+            raw: "tomtom is a cat",
+            category: Fabricate(:category).id,
+          },
+        ).create
+
+      expect(
+        Discourse.redis.sismember(DiscourseTranslator::LANG_DETECT_NEEDED, post.id),
+      ).to be_truthy
+    end
+
+    context "when user and posts are not in the right group" do
+      it "does not queue the post for language detection" do
+        SiteSetting.restrict_translation_by_poster_group = "#{group.id + 1}"
+        post =
+          PostCreator.new(
+            user,
+            {
+              title: "hello world topic",
+              raw: "my name is fred",
+              category: Fabricate(:category).id,
+            },
+          ).create
+
+        expect(
+          Discourse.redis.sismember(DiscourseTranslator::LANG_DETECT_NEEDED, post.id),
+        ).to be_falsey
+      end
+    end
+  end
 end

spec/serializers/post_serializer_spec.rb

@@ -12,34 +12,22 @@
 
   describe "#can_translate" do
     it "returns false when translator disabled" do
-      SiteSetting.translator_enabled = true
+      SiteSetting.translator_enabled = false
       serializer = PostSerializer.new(post, scope: Guardian.new)
 
       expect(serializer.can_translate).to eq(false)
     end
 
     describe "when translator enabled" do
-      before do
-        SiteSetting.translator_enabled = true
-        Jobs.run_later!
-      end
+      before { SiteSetting.translator_enabled = true }
 
-      describe "when small action post" do
-        fab!(:small_action)
-        let(:serializer) { PostSerializer.new(small_action, scope: Guardian.new) }
+      describe "anon user" do
+        let(:serializer) { PostSerializer.new(post, scope: Guardian.new) }
 
-        it "cannot translate" do
-          expect(serializer.can_translate).to eq(false)
+        before do
+          SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}"
+          SiteSetting.restrict_translation_by_poster_group = ""
         end
-      end
-
-      describe "when post raw is empty" do
-        fab!(:empty_post) do
-          empty_post = Fabricate.build(:post, raw: "")
-          empty_post.save!(validate: false)
-          empty_post
-        end
-        let(:serializer) { PostSerializer.new(empty_post, scope: Guardian.new) }
 
         it "cannot translate" do
           expect(serializer.can_translate).to eq(false)
@@ -49,61 +37,44 @@
       describe "logged in user" do
         let(:serializer) { PostSerializer.new(post, scope: Guardian.new(user)) }
 
-        describe "when user is not in restrict_translation_by_group" do
-          it "cannot translate" do
-            SiteSetting.restrict_translation_by_group = ""
+        it "cannot translate when user is not in restrict_translation_by_group" do
+          SiteSetting.restrict_translation_by_group = "#{group.id + 1}"
 
-            expect(serializer.can_translate).to eq(false)
-          end
+          expect(serializer.can_translate).to eq(false)
         end
 
-        describe "when post is not in restrict_translation_by_poster_group" do
-          it "cannot translate" do
-            SiteSetting.restrict_translation_by_group = "#{group.id}"
-            SiteSetting.restrict_translation_by_poster_group = ""
+        describe "user is in restrict_translation_by_group" do
+          before { SiteSetting.restrict_translation_by_group = "#{group.id}" }
 
-            expect(serializer.can_translate).to eq(false)
-          end
-        end
+          it "cannot translate when post author is not in restrict_translation_by_poster_group" do
+            SiteSetting.restrict_translation_by_poster_group = "#{group.id}"
 
-        describe "when user is in restrict_translation_by_group and poster in restrict_translation_by_poster_group" do
-          before do
-            SiteSetting.restrict_translation_by_group = "#{group.id}"
-            SiteSetting.restrict_translation_by_poster_group = "#{post_user_group.id}"
+            expect(serializer.can_translate).to eq(false)
           end
 
-          describe "when post does not have DETECTED_LANG_CUSTOM_FIELD" do
-            it "cannot translate" do
-              expect(serializer.can_translate).to eq(false)
+          describe "post author in restrict_translation_by_poster_group and locale is :xx" do
+            before do
+              SiteSetting.restrict_translation_by_poster_group = "#{post_user_group.id}"
+              I18n.stubs(:locale).returns(:pt)
             end
 
-            it "adds post id to redis if detected_language is blank" do
-              post.custom_fields["detected_language"] = nil
-              post.save_custom_fields
-
-              expect { serializer.can_translate }.to change {
-                Discourse.redis.sismember(DiscourseTranslator::LANG_DETECT_NEEDED, post.id)
-              }.from(false).to(true)
+            it "cannot translate when post does not have DETECTED_LANG_CUSTOM_FIELD" do
+              expect(serializer.can_translate).to eq(false)
             end
-          end
 
-          describe "when post has DETECTED_LANG_CUSTOM_FIELD matches user locale" do
-            before do
-              I18n.stubs(:locale).returns(:xx)
-              post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "xx"
+            it "cannot translate when post has DETECTED_LANG_CUSTOM_FIELD matches locale" do
+              post.custom_fields[DiscourseTranslator::DETECTED_LANG_CUSTOM_FIELD] = "pt"
               post.save
-            end
 
-            it { expect(serializer.can_translate).to eq(false) }
-          end
+              expect(serializer.can_translate).to eq(false)
+            end
 
-          describe "when post has DETECTED_LANG_CUSTOM_FIELD does not match user locale" do
-            before do
+            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
-            end
 
-            it { expect(serializer.can_translate).to eq(true) }
+              expect(serializer.can_translate).to eq(true)
+            end
           end
         end
       end