Locale sanity by deivid-rodriguez · Pull Request #1330 · decidim/decidim

deivid-rodriguez · 2017-05-04T22:41:38Z

🎩 What? Why?

Just a quick proof of concept of how ensuring complete :es, :ca and :en would look. This would partially fix translation issues for the current installations, so we can deal with the versioning stuff discussed at a later stage.

Enabling the check for `:es` and `:ca` locales.

This change (ad8cbda) detected some missing translations in decidim-core. At a first glance, they look true positives to me, but I'm not sure:

Missing translations (16) | i18n-tasks v0.9.13
+--------+---------------------------------------+----------------------------------+
| Locale | Key                                   | Value in other locales or source |
+--------+---------------------------------------+----------------------------------+
|   ca   | pages.home.statistics.meetings_count  | en Meetings                      |
|   ca   | pages.home.statistics.processes_count | en Processes                     |
|   ca   | pages.home.statistics.projects_count  | en Projects                      |
|   ca   | pages.home.statistics.proposals_count | en Proposals                     |
|   ca   | pages.home.statistics.results_count   | en Results                       |
|   ca   | pages.home.statistics.users_count     | en Users                         |
|   ca   | pages.home.statistics.votes_count     | en Votes                         |
|   en   | pages.home.statistics.processes       | nl Processen                     |
|   en   | pages.home.statistics.users           | nl Gebruikers                    |
|   es   | pages.home.statistics.meetings_count  | en Meetings                      |
|   es   | pages.home.statistics.processes_count | en Processes                     |
|   es   | pages.home.statistics.projects_count  | en Projects                      |
|   es   | pages.home.statistics.proposals_count | en Proposals                     |
|   es   | pages.home.statistics.results_count   | en Results                       |
|   es   | pages.home.statistics.users_count     | en Users                         |
|   es   | pages.home.statistics.votes_count     | en Votes                         |
+--------+---------------------------------------+----------------------------------+

Enabling dynamic guessing.

This change (e454c48) led to nothing new, so I'm not sure whether it's a good change or not.

📌 Related Issues

Related to Improve translation missing tests #1309.

📋 Subtasks

None

📷 Screenshots (optional)

None

👻 GIF

deivid-rodriguez · 2017-05-05T01:26:23Z

If this was to be included, it'd be nice to have an easy way to push translations to crowdin from the dev environment. Is this something crowdin provides?

beagleknight · 2017-05-05T07:29:26Z

I summon @josepjaume because I'm not really sure about Crowdin 😄 !

josepjaume · 2017-05-05T07:33:26Z

I think so, but unsure of how it works with branches.

Maybe we could do this check prior to releasing a version? It's kinda nice being able to only include english translations and then translating them via crowdin. What we don't want is released versions with missing locales.

Does this make sense to you?

oriolgual

I'm sorry but I'm against this. Initially we had a similar config but we moved Catalan & Spanish two Crowdin. The reasoning behind this is that you can only have one source of truth, and that's the English locales.

I'd be OK with having a rake tasks that checks other locales, but that would be used in each installation, not in Decidim.

deivid-rodriguez · 2017-05-05T10:25:02Z

@josepjaume Makes sense to me! I can change the PR so that the full task is not run by default.

Ideally it would pickup locales from an env variable so it can be run against arbitrary locales, and it should be runnable inside the main repo (to ensure completeness of official locale before releasing) and from installations (so non official locales can do something about it as well).

deivid-rodriguez · 2017-05-05T10:28:40Z

@oriolgual So we wouldn't add nothing compulsory, just enhance the set of tools installations and core can use to enforce locale completeness. Makes sense?

josepjaume · 2017-05-16T08:41:24Z

Is this progressing or can we close it?

deivid-rodriguez · 2017-05-16T08:49:02Z

I was kind of expecting some more input from @oriolgual before proceeding further.

josepjaume · 2017-05-16T08:51:15Z

Okay sorry! I had my PR grooming hat on and didn't actually read the entire conversation.

oriolgual · 2017-05-16T10:53:20Z

Ooops, my bad, yes that makes sense @deivid-rodriguez!

codecov · 2017-05-16T13:13:07Z

Codecov Report

Merging #1330 into master will decrease coverage by 4.8%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1330      +/-   ##
==========================================
- Coverage   95.42%   90.62%   -4.81%     
==========================================
  Files         420      323      -97     
  Lines        7213     5588    -1625     
==========================================
- Hits         6883     5064    -1819     
- Misses        330      524     +194

Impacted Files	Coverage Δ
...mments/lib/decidim/comments/mutation_extensions.rb	`25% <0%> (-75%)`	⬇️
...-comments/lib/decidim/comments/query_extensions.rb	`36.36% <0%> (-63.64%)`	⬇️
.../app/services/decidim/proposals/proposal_search.rb	`33.33% <0%> (-62.97%)`	⬇️
...trollers/decidim/proposals/proposals_controller.rb	`36.53% <0%> (-59.62%)`	⬇️
...dim/proposals/admin/proposal_answers_controller.rb	`42.85% <0%> (-57.15%)`	⬇️
...helpers/decidim/proposals/proposal_votes_helper.rb	`43.75% <0%> (-56.25%)`	⬇️
.../decidim/proposals/abilities/process_admin_user.rb	`48.27% <0%> (-51.73%)`	⬇️
...p/commands/decidim/meetings/admin/close_meeting.rb	`50% <0%> (-50%)`	⬇️
...p/models/decidim/proposals/abilities/admin_user.rb	`50% <0%> (-50%)`	⬇️
...ers/decidim/proposals/proposal_votes_controller.rb	`52.63% <0%> (-47.37%)`	⬇️
... and 139 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29be570...e097f17. Read the comment docs.

codecov · 2017-05-16T13:13:08Z

Codecov Report

Merging #1330 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1330   +/-   ##
=======================================
  Coverage   95.42%   95.42%           
=======================================
  Files         426      426           
  Lines        7342     7342           
=======================================
  Hits         7006     7006           
  Misses        336      336

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0a9496...2abc01f. Read the comment docs.

deivid-rodriguez · 2017-05-16T13:13:57Z

I pushed a proof of concept of what I'm proposing to do!

deivid-rodriguez · 2017-05-19T12:19:46Z

@josepjaume Did you use this task before the last release? When I pushed the last changes, the task was detecting some missing translations, but now it's all greeny! 🎉 🎉

$ bundle exec rake check_locale_completeness
warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 2.47 seconds (files took 0.48515 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.34599 seconds (files took 0.493 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 2.59 seconds (files took 0.51189 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.02507 seconds (files took 0.4772 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.1163 seconds (files took 0.51795 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.41022 seconds (files took 0.43737 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.91211 seconds (files took 0.4801 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.41916 seconds (files took 0.43802 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.57575 seconds (files took 0.44598 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.73293 seconds (files took 0.57483 seconds to load)
3 examples, 0 failures

warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.0-compliant syntax, but you are running 2.4.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
...

Finished in 0.01993 seconds (files took 0.44594 seconds to load)
3 examples, 0 failures

oriolgual · 2017-05-22T10:46:55Z

@deivid-rodriguez I'm not sure I've understand this: do you run the rake task from decidim root or in a decidim installation like Barcelona? I meant to do it in a installation, I don't think we should block a release because some language is missing a locale (even catalan or spanish).

deivid-rodriguez · 2017-05-22T10:54:59Z

It can be run from both. If run from decidim's root, it checks official locales (en, ca, es). If run from an installation, it checks application locales.

I added the "root check" to the release task because I think that's what @josepjaume suggested. But I can remove that bit.

oriolgual · 2017-05-22T11:05:48Z

@deivid-rodriguez can you document how to run the task from an installation? Maybe we can add it to the getting started guide.

deivid-rodriguez · 2017-05-22T11:54:17Z

@oriolgual I added some docs in 1a4009c!

oriolgual · 2017-05-22T13:10:20Z

Rakefile

+task :check_locale_completeness do
+  DECIDIM_GEMS.each do |gem_name|
+    Dir.chdir("#{File.dirname(__FILE__)}/decidim-#{gem_name}") do
+      system({ "ENFORCED_LOCALES" => "en,ca,es" }, "rspec spec/i18n_spec.rb")


Shouldn't we make this configurable? Either via params to the task or using the app configured locales?

This is the task supposed to be run from decidim's root. In that case, I wanted to hardcode it to just check the "officially supported" locales. I could make it configurable but I'm not sure if it's worth the effort...

Note that if the task is run from an app, the app locales are used instead, not via this task, but via https://github.com/decidim/decidim/pull/1330/files#diff-c3eb346890858ca91acb2f677044cdd1.

Oh, sorry about that, I didn't realise it was two different things! About the official maintained locales: I'm not sure if we should even include Catalan & Spanish, isn't it a bit arbitrary?

No problem! Yes, it's arbitrary. So there's two ideas here:

One is trying to keep locales in core that are currently 100% complete, 100% complete. This would account for the concept of "officially supported locales". Since I guess most core developers right now speak all three languages and most current installations include ca and es, choosing es, ca and en as official locales seems natural. But yes, this concept of official locales is a bit arbitrary and would probably no longer make sense once/if we extract decidim-i18n to a separate library.

The other one is to allow installations to check their own locales. I think there's no doubts about this, we all want it, right?

Yes, all makes sense. Let's go with it now and if we need to adjust in the future we'll do it!

With colors and all.

Currently `rake development_app` generates a development app with all locales supported by decidim. Since at the moment is compulsory to introduce all locale information in forms, it's very tedious to make manual tests using this app. So I changed it to use the same the test app uses.

As a release pre-requisite.

deivid-rodriguez · 2017-05-29T11:35:37Z

I rebased this PR on top of latest master. I'd like to get this in, specifically for 8688bfc. I usually play around with the example app, but at the moment it is generated with 7 different locales. That, in combination with #1127, makes admin forms pretty unusable.

Also fixed a new codeclimate style issue. That, and the fact that the hound check is stuck leads to the question: do we even need/want hound at all?

oriolgual · 2017-05-29T13:36:42Z

We don't need Hound with CodeClimate now, we're actually talking about this!

josepjaume · 2017-05-30T12:01:22Z

Nice! Merging this in!

oriolgual suggested changes May 5, 2017

View reviewed changes

mrcasals mentioned this pull request May 11, 2017

Missing i18n remove avatars on user profile #1348

Closed

oriolgual approved these changes May 22, 2017

View reviewed changes

oriolgual suggested changes May 22, 2017

View reviewed changes

oriolgual previously approved these changes May 22, 2017

View reviewed changes

deivid-rodriguez mentioned this pull request May 22, 2017

Translation typo #1388

Closed

deivid-rodriguez added 7 commits May 29, 2017 07:49

Allow checking i18n against non English locales

574c325

Try out dynamic missing locale guessing

a28bb2e

Move i18n specs to shared examples

fbe87f6

More readable test failures

ed260b7

With colors and all.

Implement locale checking from installations

810f370

Task to enforce completeness of official locales

7cd1ae3

As a release pre-requisite.

Add some docs to the decidim:check_locales task

0b3140f

deivid-rodriguez dismissed oriolgual’s stale review via 0b3140f May 29, 2017 10:51

Fix style issue detected by Codeclimate

2abc01f

oriolgual approved these changes May 29, 2017

View reviewed changes

josepjaume merged commit e8bdafe into decidim:master May 30, 2017

deivid-rodriguez deleted the locale_sanity branch May 30, 2017 13:06

deivid-rodriguez mentioned this pull request Jun 8, 2017

Improve translation missing tests #1309

Closed

Uh oh!

Conversation

deivid-rodriguez commented May 4, 2017

🎩 What? Why?

Enabling the check for :es and :ca locales.

Enabling dynamic guessing.

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Uh oh!

deivid-rodriguez commented May 5, 2017

Uh oh!

beagleknight commented May 5, 2017

Uh oh!

josepjaume commented May 5, 2017

Uh oh!

oriolgual left a comment

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez commented May 5, 2017

Uh oh!

deivid-rodriguez commented May 5, 2017

Uh oh!

josepjaume commented May 16, 2017

Uh oh!

deivid-rodriguez commented May 16, 2017

Uh oh!

josepjaume commented May 16, 2017

Uh oh!

oriolgual commented May 16, 2017

Uh oh!

codecov bot commented May 16, 2017

Codecov Report

Uh oh!

codecov bot commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

deivid-rodriguez commented May 16, 2017

Uh oh!

deivid-rodriguez commented May 19, 2017

Uh oh!

oriolgual commented May 22, 2017

Uh oh!

deivid-rodriguez commented May 22, 2017

Uh oh!

oriolgual commented May 22, 2017

Uh oh!

deivid-rodriguez commented May 22, 2017

Uh oh!

oriolgual May 22, 2017

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oriolgual May 22, 2017

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez May 22, 2017

Choose a reason for hiding this comment

Uh oh!

oriolgual May 22, 2017

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez commented May 29, 2017

Uh oh!

oriolgual commented May 29, 2017

Uh oh!

josepjaume commented May 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Enabling the check for `:es` and `:ca` locales.

codecov bot commented May 16, 2017 •

edited

Loading

deivid-rodriguez May 22, 2017 •

edited

Loading