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

Fix broken tests caused by locale configuration #1875

Merged
merged 5 commits into from
Sep 20, 2018
Merged

Conversation

Bodacious
Copy link
Contributor

  • Add LocaleSet
  • Add LocaleFormatter to force proper string formats
  • Rename I18n formats to use hyphen
  • Add rake task for updating existing languages on DB
  • Fix instances of US english in the templates

Locale formats

FastGettext expects locales to be in the format 'en_GB'. I18n uses the ISO standard for language formats 'en-GB' (a hyphen instead of an underscore). FastGettext does re-format locale strings when setting locale (e.g. FastGettext.locale = "en-GB" # => "en_GB"), but the lookups were tending to fail, causing lots of unpredictable bugs in testing.

See this issue for more info: grosser/fast_gettext#81

This PR adds a couple of helper classes to mediate between I18n expectations and FastGettext expectations. Namely, LocaleFormatter and LocaleSet.

By forcing the use of these classes throughout the code, it seems to have sorted most of the sporadic failures in the tests.

Default locale

There was another issue that was happening sporadically: Some tests would fail because the text on the page was the wrong locale. I think this was some sort of race condition, in which the test was inspecting the page before strings had been updated in the browser by FastGetext. This meant that strings like "Customize" were being loaded in the page in US English, whereas the tests were searching for GB English strings "Customise". Since en-GB is the default locale, I rewrote those strings to use GB spelling.

Migration

I've added a rake task rake upgrade:normalize_language_formats to change the locale formats for existing Languages, Themes, and Templates on the DB.

- Add LocaleSet
- Add LocaleFormatter to force proper string formats
- Rename I18n formats to use hyphen
- Add rake task for updating existing languages on DB
- Fix instances of US english in the templates
@Bodacious Bodacious force-pushed the bug/locale-formats branch 2 times, most recently from dc3d2c4 to 419e365 Compare September 14, 2018 11:36
@Bodacious Bodacious requested review from xsrust and briri September 14, 2018 11:47
Was breaking rake db:seed file
Copy link
Contributor

@xsrust xsrust left a comment

Choose a reason for hiding this comment

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

Can you re-add the pt-BR.yml file with another PR?

FastGettext.default_available_locales = available_locales

I18n.default_locale = available_locales.for(:i18n).first
FastGettext.default_locale = available_locales.for(:fast_gettext).first
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a clean way of managing the fastGettext vs I18n expectations.

@@ -1,25 +0,0 @@
# Sample localization file for English. Add more files in this directory for other locales.
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a full translation for Brazillian due to their translation work with CDL so we'll actually want to keep and update this file rather than remove it.

@@ -1,6 +1,21 @@
require 'set'
namespace :upgrade do

desc "Update Language abbreviations to use ISO format"
task :normalize_language_formats => :environment do
Copy link
Contributor

Choose a reason for hiding this comment

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

We can flag this needs to be run with the release

@xsrust xsrust merged commit 9d3df90 into development Sep 20, 2018
@briri briri deleted the bug/locale-formats branch January 22, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants