FIX: Ensure old feature works with new and show translate button in correct scenarios (#215)

In the older implementation of translation, there exists these two site settings are part of the criteria that determines if 🌐 shows up per post

* `restrict translation by group` - Only allowed groups can translate
* `restrict translation by poster group` - Only allow translation of posts made by users in allowed groups. If empty, allow translations of posts from all users.

In a recent update with experimental features, we updated the cases where 🌐 will show up and caused a regression where the 🌐 shows up for everyone in `restrict translation by group`, ignoring `restrict translation by poster group`.

This commit makes sure these two site settings are adhered to when the experimental topic setting is turned off.

Author: nattsw

lib/discourse_translator/guardian_extension.rb

@@ -22,14 +22,13 @@ def can_detect_language?(post)
   def can_translate?(post)
     return false if !user_group_allow_translate?
 
-    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,
-      )
+    if SiteSetting.experimental_topic_translation
+      return false if post.locale_matches?(I18n.locale)
+      return false if post.translation_for(I18n.locale).present?
+      true
+    else
+      return false if post.locale_matches?(I18n.locale)
+      poster_group_allow_translate?(post)
+    end
   end
 end

spec/lib/guardian_extension_spec.rb

@@ -130,42 +130,111 @@
     describe "when translator enabled" do
       before { SiteSetting.translator_enabled = true }
 
-      describe "anon user" do
-        before { SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}" }
+      describe "when experimental_topic_translation enabled" do
+        before { SiteSetting.experimental_topic_translation = true }
 
-        it "cannot translate" do
-          expect(Guardian.new.can_translate?(post)).to eq(false)
+        describe "anon user" do
+          before { SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}" }
+
+          it "cannot translate" do
+            expect(Guardian.new.can_translate?(post)).to eq(false)
+          end
+        end
+
+        describe "logged in user" do
+          it "cannot translate when user is not in restrict_translation_by_group" do
+            SiteSetting.restrict_translation_by_group = "#{group.id + 1}"
+
+            expect(guardian.can_translate?(post)).to eq(false)
+          end
+
+          describe "user is in restrict_translation_by_group" do
+            before { SiteSetting.restrict_translation_by_group = "#{group.id}" }
+
+            describe "locale is :pt" do
+              before { I18n.stubs(:locale).returns(:pt) }
+
+              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's detected locale does not match i18n locale" do
+                post.set_detected_locale("jp")
+
+                expect(guardian.can_translate?(post)).to eq(true)
+              end
+
+              it "cannot translate when post has translation for user locale" do
+                post.set_detected_locale("jp")
+                post.set_translation("pt", "Olá, mundo!")
+
+                expect(guardian.can_translate?(post)).to eq(false)
+              end
+
+              it "can translate when post does not have translation for user locale" do
+                post.set_detected_locale("jp")
+
+                expect(guardian.can_translate?(post)).to eq(true)
+              end
+            end
+          end
         end
       end
 
-      describe "logged in user" do
-        it "cannot translate when user is not in restrict_translation_by_group" do
-          SiteSetting.restrict_translation_by_group = "#{group.id + 1}"
+      describe "when experimental topic translation disabled" do
+        before { SiteSetting.experimental_topic_translation = false }
+
+        describe "anon user" do
+          before { SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}" }
 
-          expect(guardian.can_translate?(post)).to eq(false)
+          it "cannot translate" do
+            expect(Guardian.new.can_translate?(post)).to eq(false)
+          end
         end
 
-        describe "user is in restrict_translation_by_group" do
-          before { SiteSetting.restrict_translation_by_group = "#{group.id}" }
+        describe "logged in user" do
+          it "cannot translate when user is not in restrict_translation_by_group" do
+            SiteSetting.restrict_translation_by_group = "#{group.id + 1}"
 
-          describe "locale is :xx" do
-            before { I18n.stubs(:locale).returns(:pt) }
+            expect(guardian.can_translate?(post)).to eq(false)
+          end
 
-            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
+          describe "user is in restrict_translation_by_group" do
+            before { SiteSetting.restrict_translation_by_group = "#{group.id}" }
 
-            it "cannot translate when post detected locale matches i18n locale" do
-              post.set_detected_locale("pt")
+            describe "locale is :pt" do
+              before { I18n.stubs(:locale).returns(:pt) }
 
-              expect(guardian.can_translate?(post)).to eq(false)
-            end
+              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's detected locale does not match i18n locale" do
+                post.set_detected_locale("jp")
+
+                expect(guardian.can_translate?(post)).to eq(true)
+              end
+
+              it "cannot translate if poster is not in restrict_translation_by_poster_group" do
+                SiteSetting.restrict_translation_by_poster_group = "#{Group::AUTO_GROUPS[:staff]}"
+
+                expect(guardian.can_translate?(post)).to eq(false)
+              end
+
+              it "can translate if poster is in restrict_translation_by_poster_group" do
+                poster = post.user
+                poster_group = Fabricate(:group, users: [poster])
 
-            it "can translate when post detected locale does not match i18n locale" do
-              post.set_detected_locale("jp")
+                SiteSetting.restrict_translation_by_poster_group = "#{poster_group.id}"
+                expect(guardian.can_translate?(post)).to eq(true)
 
-              expect(guardian.can_translate?(post)).to eq(true)
+                SiteSetting.restrict_translation_by_poster_group = ""
+                expect(guardian.can_translate?(post)).to eq(true)
+              end
             end
           end
         end

spec/serializers/post_serializer_spec.rb

@@ -19,19 +19,15 @@
     end
 
     describe "when translator enabled" do
-      before { SiteSetting.translator_enabled = true }
-
-      describe "anon user" do
-        let(:serializer) { PostSerializer.new(post, scope: Guardian.new) }
-
-        before do
-          SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}"
-          SiteSetting.restrict_translation_by_poster_group = ""
-        end
+      before do
+        SiteSetting.translator_enabled = true
+        SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}"
+        SiteSetting.restrict_translation_by_poster_group = ""
+      end
+      let(:serializer) { PostSerializer.new(post, scope: Guardian.new) }
 
-        it "cannot translate" do
-          expect(serializer.can_translate).to eq(false)
-        end
+      it "cannot translate for anon" do
+        expect(serializer.can_translate).to eq(false)
       end
 
       describe "logged in user" do
@@ -44,32 +40,12 @@
         end
 
         describe "user is in restrict_translation_by_group" do
-          before { SiteSetting.restrict_translation_by_group = "#{group.id}" }
-
-          it "cannot translate when post author is not in restrict_translation_by_poster_group" do
-            SiteSetting.restrict_translation_by_poster_group = "#{group.id}"
-
-            expect(serializer.can_translate).to eq(false)
-          end
-
           describe "post author in restrict_translation_by_poster_group and locale is :xx" do
-            before do
+            it "can translate when post detected locale does not match i18n locale" do
+              SiteSetting.restrict_translation_by_group = "#{group.id}"
               SiteSetting.restrict_translation_by_poster_group = "#{post_user_group.id}"
               I18n.stubs(:locale).returns(:pt)
-            end
-
-            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 detected locale matches i18n locale" do
-              post.set_detected_locale("pt")
-
-              expect(serializer.can_translate).to eq(false)
-            end
-
-            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)