FIX: Ensure old feature works with new and show translate button in correct scenarios #215
Conversation
spec/lib/guardian_extension_spec.rb
Outdated
| expect(guardian.can_translate?(post)).to eq(true) | ||
| end | ||
|
|
||
| it "can translate when post does not have translation" do |
There was a problem hiding this comment.
The setup for this test is exactly the same as the test before it. I think the set up for this test should be clearing out the post's translation?
There was a problem hiding this comment.
The post will not have translation if I don't explicitly add post.set_translation(locale, text)
It's exactly the same setup, but my intent is that the test description is the documentation of the behaviour not the implementation.
There was a problem hiding this comment.
Thank you for clarifying 👍 Perhaps we can just combine the two test cases into one? Technically we can only translate when post's detected locale does not match i18n locale and also when the post does not have translation.
Might also be useful to assert that the post cannot be translated when the post's translation is present for the given locale.
There was a problem hiding this comment.
the post cannot be translated when the post's translation is present for the given locale
Actually, "can_translate" is the bool that determines if the 🌐 appears on the post.
(Old feature) If the translation exists for the locale, the value should still be true, since the user will click on 🌐 and we will return the existing translation to the user. This is the old feature that we need to maintain, where viewing translations are manual, not the new feature where it is automatically rendered by default.
old feature
new experimental feature
| return false if post.translation_for(I18n.locale).present? | ||
| true | ||
| else | ||
| return false if post.locale_matches?(I18n.locale) |
There was a problem hiding this comment.
Small one but this check is the same regardless of the conditional so we can technically pull this out of the conditional.
This is a refactor to prepare for a fix, so that the fix PR is less messy. Related comment here: #215 (comment)
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 translaterestrict 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, ignoringrestrict translation by poster group.This PR makes sure these two site settings are adhered to when the experimental topic setting is turned off.