Skip to content
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.

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

Improve remote translations #3917

Merged

Conversation

taitus
Copy link
Member

@taitus taitus commented Feb 10, 2020

References

Related PR's:

Objectives

  • Avoid curl requests to the application by requesting unavailable languages.
  • Avoid curl requests to the application by requesting resources already translated

Visual Changes

None

Notes

None

@taitus taitus self-assigned this Feb 10, 2020
@taitus taitus requested a review from javierm February 10, 2020 10:28
spec/controllers/rack_attack_spec.rb Outdated Show resolved Hide resolved
spec/controllers/rack_attack_spec.rb Outdated Show resolved Hide resolved
spec/controllers/rack_attack_spec.rb Outdated Show resolved Hide resolved
config/initializers/rack_attack.rb Outdated Show resolved Hide resolved
config/initializers/rack_attack.rb Outdated Show resolved Hide resolved
config/initializers/rack_attack.rb Outdated Show resolved Hide resolved
app/models/remote_translation.rb Outdated Show resolved Hide resolved
@taitus taitus force-pushed the improve-remote-translations branch 2 times, most recently from 2942caf to 910cad8 Compare February 11, 2020 11:15
@taitus taitus force-pushed the improve-remote-translations branch 2 times, most recently from 3365836 to 2b99a00 Compare February 25, 2020 12:41
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

If you don't mind, I'll be doing the review in two parts 🤣.

Part one (this review) deals with everything except rack-attack.

Part two (coming soon?) will deal with rack-attack.

To be continued...

spec/controllers/rack_attack_spec.rb Outdated Show resolved Hide resolved
spec/features/stats_spec.rb Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved

def already_translated_resource
if remote_translatable&.translations&.where(locale: locale).present?
errors.add(:locale, I18n.t("remote_translations.already_translated_resource"))
Copy link
Member

Choose a reason for hiding this comment

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

How about errors.add(:locale, :already_translated)? We could then configure it like we configure other translation messages, adding translations to activerecord.yml. I think the translation key would be something like activerecord.errors.models.remote_translation.attributes.locale.already_translated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Solved in the commit: cdb2937

@taitus taitus changed the title Improve remote translations [WIP] Improve remote translations Feb 26, 2020
@taitus taitus force-pushed the improve-remote-translations branch 2 times, most recently from a747c92 to bdbc924 Compare February 26, 2020 11:16
- Do not display remote translations button when API key is not configured
@taitus taitus changed the title [WIP] Improve remote translations Improve remote translations Feb 26, 2020
it "When defined in current locale should not detect remote_translations" do
proposal = create(:proposal)
comment = create(:comment, commentable: proposal)

expect(detect_remote_translations([proposal, comment])).to eq []
end

it "When resource class has not translates should not detect remote_translations" do
Copy link
Member

Choose a reason for hiding this comment

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

"has not translates" seems to be a typo 🤔. How about "is not translatable"?

app/models/remote_translation.rb Outdated Show resolved Hide resolved
@javierm javierm self-assigned this Feb 26, 2020
@javierm javierm added this to Reviewing in Roadmap via automation Feb 26, 2020
@javierm javierm moved this from Reviewing to Doing in Roadmap Feb 26, 2020
- Validate that locale is a valid locale for RemoteTranslation Client.
- RemoteTranslation can only be created for resources that do not have the requested
language translated
Legislation::Proposal is not Globalize model but use CommentableActions and try
detect remote translations. Add new condition to discard Non Globalize models.
This fix is necessary since the following commit was included: c1f3a4a.
Roadmap automation moved this from Doing to Testing Feb 26, 2020
@javierm javierm merged commit 9d750dd into consuldemocracy:master Feb 26, 2020
Roadmap automation moved this from Testing to Release 1.1.0 Feb 26, 2020
smarques pushed a commit to venetochevogliamo/consul that referenced this pull request Apr 29, 2020
…e-translations

Improve remote translations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Roadmap
  
Release 1.1.0
Development

Successfully merging this pull request may close these issues.

None yet

3 participants