Improve decidim's i18n by deivid-rodriguez · Pull Request #1555 · decidim/decidim

deivid-rodriguez · 2017-06-30T14:11:25Z

🎩 What? Why?

This PR fixes several things regarding decidim languages:

~~Adds docs! 🎉~~
Allows configuring locales. At the moment, by the time decidim's configuration is run, the config.available_locales = Decidim.available_locales assignment in application.rb has already been run. So custom locales are not picked up. I fixed it by changing the order these are run.
Changes generated application locales to be :en, :ca & :es, instead of all available locales. At the moment, playing around with the generated application is a PITA because until we fix Only require the default locale translations #1127, all languages are required. And the generated application uses all 8 languages by default! Even if Only require the default locale translations #1127 was no longer a problem, I'm not sure we want to generate applications with all languages enabled, since it would be a pretty unrealistic application. I think enabling English + decidim original languages is a good compromise.. I finally used those locales not globally but for the sample organizations as suggested by @mrcasals.
Makes it easier to remove locales from an app by providing a rake task, decidim:sanitize_locales, that cleans up DB from no longer available locales. Without running this task, decidim can crash after removing locales, because the locales available for each organization might no longer be available.

📌 Related Issues

~~Fixes Add docs about i18n #1439~~.
Related to Update README to document "How to add default languages" for developers. #1495. ~~Doesn't completely fix it, but gets it started.~~
~~Blocked on Fix decidim dev tasks not being exposed #1554, since decidim can't provide tasks to applications until that one is merged. It's currently built on top of it.~~ That PR was merged.

📋 Subtasks

None.

📷 Screenshots (optional)

None.

👻 GIF

codecov · 2017-06-30T14:11:29Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1555   +/-   ##
=======================================
  Coverage   96.78%   96.78%           
=======================================
  Files         511      511           
  Lines        8640     8640           
=======================================
  Hits         8362     8362           
  Misses        278      278

mrcasals · 2017-07-03T10:28:08Z

lib/generators/decidim/templates/initializer.rb

  config.authorization_handlers = [ExampleAuthorizationHandler]

  # Uncomment this lines to set your preferred locales
-  # config.available_locales = %i{en ca es}


Since Ruby 2.0 this is equivalent

It is. But we are enforcing the style I switched to via rubocop.

mrcasals · 2017-07-03T10:30:05Z

@deivid-rodriguez the second point can be related to #1532, if you want to link it :)

mrcasals · 2017-07-03T10:31:45Z

lib/generators/decidim/templates/initializer.rb

-  # Uncomment this lines to set your preferred locales
-  # config.available_locales = [:en, :ca, :es]
+  # Change this line to set your preferred locales
+  config.available_locales = [:en, :ca, :es]


Maybe the problems are the seeds, then? Since the organization created there uses Decidim.available_locales as its own available_locales. Maybe we can change it there?

You mean using the hardcoded list for the organizations locales, but leaving the full list enabled here? As long as I don't have to fill in 8 locales for each field on every form, sounds good to me! :)

Yep, I'd change the values in the seeds until we start working on not making all the languages compulsory

mrcasals · 2017-07-03T10:32:33Z

decidim-dev/lib/tasks/locale_checker.rake

+  task sanitize_locales: :environment do
+    Decidim::Organization.find_each do |organization|
+      sanitized_locales =
+        organization.available_locales & Decidim.available_locales.map(&:to_s)


I'm not sure about this. Shouldn't we update processes, etc in order to fix the locales there too?

I guess, I didn't know there were other locales in DB.

If we configure a reasonable set of locales per organization, I don't think we need this at the moment. I'll remove it, ok?

Now I see what you meant by updating processes. All translations for every text field are saved in DB, of course...

Yeah, that's what I meant. We would need to update every process/meeting/whatever and remove that language :/

Well, I'm not sure leaving the translations around could cause any problems. And they would still be there if you reenable the language... :) I can see how the opposite is problematic in the current situation though (adding a new language and every object lacking compulsory translations).

mrcasals · 2017-07-03T10:33:07Z

docs/getting_started.md

 - [Social providers integration](https://github.com/decidim/decidim/blob/master/docs/social_providers.md): Enable sign up from social networks.
 - [Analytics](https://github.com/decidim/decidim/blob/master/docs/analytics.md): How to enable analytics
 - [Geocoding](https://github.com/decidim/decidim/blob/master/docs/geocoding.md): How to enable geocoding for proposals and meetings
+- [Languages](https://github.com/decidim/decidim/blob/master/docs/languages.md): How to enable configure languages


How to configure languages?

deivid-rodriguez · 2017-07-03T10:45:10Z

@mrcasals Your comments make sense to me, it should make this much simpler. I'll update the PR & docs as you suggested.

deivid-rodriguez · 2017-07-03T10:59:44Z

Allows configuring locales. At the moment, by the time decidim's configuration is run, the config.available_locales = Decidim.available_locales assignment in application.rb has already been run. So custom locales are not picked up. I fixed it by changing the order these are run.

Should I keep 32c00fb? Removing locales globally is currently prone to errors (you have to re-seed or use a task like the one I provided here), and provides little value over configuring them per organization. So I'm not even sure about keeping that either, even if it fixes unexpected behavior.

mrcasals · 2017-07-03T11:08:03Z

We deliberately don't let system admins modify available locales for an already created organization, because we thought that modifying the locales could give some problems right now (until we do that PR about the locales).

As for that commit, I'd ping @beagleknight so he can chime in :)

deivid-rodriguez · 2017-07-03T11:25:06Z

Oh, I didn't know that... #1127 keeps biting! 😱

mention-bot · 2017-07-10T10:15:41Z

@deivid-rodriguez, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrcasals and @josepjaume to be potential reviewers.

deivid-rodriguez · 2017-07-24T21:45:54Z

I rebased this PR and removed the parts I was not sure about. Updated the PR description to reflect what's left in there. It's ready for another review pass!

Due to the order Rails loads things, locale were not actually configurable via `Decidim.available_locales` at the moment, because when the assignment to Rails native locales was done, customizations had not yet been loaded. This change fixes it by loading locale customizations _before_ setting `config.i18n.available_locales`.

Each test should be responsible for not leaking state.

deivid-rodriguez · 2017-07-28T11:43:23Z

Anything else left to do here? This makes manual testing a lot easier IMO.

beagleknight

Looks ok! Let's merge 😄

deivid-rodriguez · 2017-07-28T13:06:06Z

Yuhuuuuu! 🎉

* Fix code style of commented configuration * Fix locale configuration Due to the order Rails loads things, locale were not actually configurable via `Decidim.available_locales` at the moment, because when the assignment to Rails native locales was done, customizations had not yet been loaded. This change fixes it by loading locale customizations _before_ setting `config.i18n.available_locales`. * Fix typo * No need to reset locales for every test Each test should be responsible for not leaking state. * A a more reasonable set of locale for sample orgs

beagleknight · 2017-08-03T09:05:14Z

decidim-core/db/seeds.rb

    homepage_image: File.new(File.join(__dir__, "seeds", "homepage_image.jpg")),
    default_locale: I18n.default_locale,
-    available_locales: Decidim.available_locales,
+    available_locales: [:en, :ca, :es],


Hi @deivid-rodriguez ! I know this is merged but I am having a few problems because of that 😄 . Do you remember why you hardcoded this values?
The problem I found is when a installation doesn't have English for example, I cannot create a review app because the seeds are creating "invalid" organizations.

Ooops, that makes sense. The idea was not having 7 locales in the generated app by default. I initially changed the defaults in decidim's configuration. But @mrcasals suggested to do it per organization here. None of us realized of this problem... :(

No problem! 😄 I think I am going to change our app generator to include those locales as default because if we leave Decidim.available_locales commented it will use all the locales.

Sorry about that... 🙏 I can rework another PR that doesn't touch the seed process if you want.

Heheh, we replied at the same time :) Sounds good to me, sorry for the trouble 😅

Don't worry! I think I can handle it and it's not a big deal 😄

ItsGenis added the in-progress label Jun 30, 2017

deivid-rodriguez force-pushed the fix/simpler_dev_locales branch from 4dd1a76 to d4ce6ec Compare June 30, 2017 14:17

mrcasals reviewed Jul 3, 2017

View reviewed changes

beagleknight assigned deivid-rodriguez Jul 10, 2017

beagleknight added in-review and removed in-progress labels Jul 10, 2017

deivid-rodriguez force-pushed the fix/simpler_dev_locales branch from d4ce6ec to 98e3e0c Compare July 24, 2017 21:23

beagleknight force-pushed the fix/simpler_dev_locales branch from 98e3e0c to 9e9a371 Compare July 25, 2017 08:03

deivid-rodriguez force-pushed the fix/simpler_dev_locales branch from 9e9a371 to 19f5bb0 Compare July 25, 2017 14:45

deivid-rodriguez added 5 commits July 26, 2017 12:55

Fix code style of commented configuration

4e27d18

Fix typo

e41b4d3

No need to reset locales for every test

fc0495b

Each test should be responsible for not leaking state.

A a more reasonable set of locale for sample orgs

3628cdd

deivid-rodriguez force-pushed the fix/simpler_dev_locales branch from 19f5bb0 to 3628cdd Compare July 26, 2017 10:55

mrcasals approved these changes Jul 27, 2017

View reviewed changes

deivid-rodriguez mentioned this pull request Jul 27, 2017

Fix re-seeding problem #1649

Merged

decidim deleted a comment from codecov bot Jul 28, 2017

beagleknight approved these changes Jul 28, 2017

View reviewed changes

beagleknight merged commit 947bed6 into master Jul 28, 2017

beagleknight deleted the fix/simpler_dev_locales branch July 28, 2017 13:03

ghost removed the in-review label Jul 28, 2017

This was referenced Jul 28, 2017

Add docs about i18n #1439

Closed

Only require the default locale translations #1127

Closed

beagleknight reviewed Aug 3, 2017

View reviewed changes

mrcasals mentioned this pull request Aug 21, 2017

Bug with "survey" question creation #1739

Closed

Uh oh!

Conversation

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

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez commented Jul 3, 2017

Uh oh!

deivid-rodriguez commented Jul 3, 2017

Uh oh!

mrcasals commented Jul 3, 2017

Uh oh!

deivid-rodriguez commented Jul 3, 2017

Uh oh!

mention-bot commented Jul 10, 2017

Uh oh!

deivid-rodriguez commented Jul 24, 2017

Uh oh!

deivid-rodriguez commented Jul 28, 2017

Uh oh!

beagleknight left a comment

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

deivid-rodriguez commented Jun 30, 2017 •

edited

Loading

codecov bot commented Jun 30, 2017 •

edited

Loading