Skip to content

DEV: Simplify if-statements and test #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 26, 2025
Merged

DEV: Simplify if-statements and test #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions lib/discourse_translator/guardian_extension.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@ def can_detect_language?(post)

def can_translate?(post)
return false if !user_group_allow_translate?
return false if post.locale_matches?(I18n.locale)

if SiteSetting.experimental_topic_translation
return false if post.locale_matches?(I18n.locale)
return false if post.translation_for(I18n.locale).present?
true
post.translation_for(I18n.locale).nil?
Copy link
Contributor

@tyb-talks tyb-talks Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was going to say .blank? is the direct complement of .present? but I guess there's enough safeguards against this being something like empty string? (I see the presence: true validation for translation)

else
return false if post.locale_matches?(I18n.locale)
Comment on lines -26 to -30
Copy link
Contributor Author

@nattsw nattsw Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the only real change is the common condition is brought up top.

poster_group_allow_translate?(post)
end
end
Expand Down
127 changes: 55 additions & 72 deletions spec/lib/guardian_extension_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,35 @@
end

describe "when translator enabled" do
before { SiteSetting.translator_enabled = true }
before do
SiteSetting.translator_enabled = true
I18n.locale = :pt
end

describe "when experimental_topic_translation enabled" do
before { SiteSetting.experimental_topic_translation = true }
describe "anon user" do
before { SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}" }

describe "anon user" do
before { SiteSetting.restrict_translation_by_group = "#{Group::AUTO_GROUPS[:everyone]}" }
it "cannot translate" do
SiteSetting.experimental_topic_translation = true
expect(Guardian.new.can_translate?(post)).to eq(false)

it "cannot translate" do
expect(Guardian.new.can_translate?(post)).to eq(false)
end
SiteSetting.experimental_topic_translation = false
expect(Guardian.new.can_translate?(post)).to eq(false)
end
end

it "cannot translate when post detected locale matches i18n locale" do
post.set_detected_locale("pt")

SiteSetting.experimental_topic_translation = true
expect(guardian.can_translate?(post)).to eq(false)

SiteSetting.experimental_topic_translation = false
expect(guardian.can_translate?(post)).to eq(false)
end

describe "when experimental_topic_translation enabled" do
before { SiteSetting.experimental_topic_translation = true }

describe "logged in user" do
it "cannot translate when user is not in restrict_translation_by_group" do
Expand All @@ -151,33 +168,17 @@
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!")
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
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")
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
expect(guardian.can_translate?(post)).to eq(true)
end
end
end
Expand All @@ -186,56 +187,38 @@
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]}" }
it "cannot translate when user is not in restrict_translation_by_group" do
SiteSetting.restrict_translation_by_group = "#{group.id + 1}"

it "cannot translate" do
expect(Guardian.new.can_translate?(post)).to eq(false)
end
expect(guardian.can_translate?(post)).to eq(false)
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
describe "user is in restrict_translation_by_group" do
before { SiteSetting.restrict_translation_by_group = "#{group.id}" }

it "can translate when post's detected locale does not match i18n locale" do
post.set_detected_locale("jp")
it "can translate when post's detected locale does not match i18n locale, regardless of translation presence" do
post.set_detected_locale("jp")
expect(guardian.can_translate?(post)).to eq(true)

expect(guardian.can_translate?(post)).to eq(true)
end
post.set_translation("pt", "Olá, mundo!")
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]}"
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
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 if poster is in restrict_translation_by_poster_group" do
poster = post.user
poster_group = Fabricate(:group, users: [poster])

SiteSetting.restrict_translation_by_poster_group = "#{poster_group.id}"
expect(guardian.can_translate?(post)).to eq(true)
SiteSetting.restrict_translation_by_poster_group = "#{poster_group.id}"
expect(guardian.can_translate?(post)).to eq(true)

SiteSetting.restrict_translation_by_poster_group = ""
expect(guardian.can_translate?(post)).to eq(true)
end
end
SiteSetting.restrict_translation_by_poster_group = ""
expect(guardian.can_translate?(post)).to eq(true)
end
end
end
Expand Down
Loading