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

Avoid missing i18n translations #2626

Closed
kristinalim opened this issue Aug 31, 2018 · 6 comments
Closed

Avoid missing i18n translations #2626

kristinalim opened this issue Aug 31, 2018 · 6 comments
Assignees

Comments

@kristinalim
Copy link
Member

Description

There doesn't seem to be an easy way right now to make sure that translation keys we use indeed have corresponding entries in the main language file.

There's a chance to miss it if a developer introduces a new translation key but forgets to add it to the language file.

And if a translation is missing, the non-underscored version of the key is used, so a missing "new_feature" appears as "New Feature" (wrapped in span.translation_missing). This translation often still makes sense for the development language en, so you would think everything is okay when manually testing in the browser.

These are small enough changes that can help us avoid these missing translations:

  • For translations used in the Ruby code, in automated tests (only test environment), raise error when encountering a missing translation key. We have good test coverage, so I think this would be very helpful.
  • For translations used in the JS, warn in the browser console if a translation is missing.
  • Make it more obvious in the UI if a certain text is only from the non-underscored version of the translation key. As mentioned, these are wrapped in span.translation_missing so can easily be styled.

Expected Behavior

Warnings when using a translation key that is missing from the language file

Actual Behavior

No changes in actual behaviour if translations are okay

Steps to Reproduce

  1. Add t("missing_key") to a page, and check page. The page would not complain, and render this as "Missing Key".
  2. The following test would not complain:
require 'spec_helper'

describe "Missing translation" do
  it "raises error when a translation is missing" do
    I18n.t("missing_key")
  end
end

Context

Lack of confidence when adding/updating translation keys

@kristinalim
Copy link
Member Author

When I submitted this issue, I was only thinking about how this would affect application code, but it turns out this is also relevant in the decision of whether we should use I18n.t in specs or not. There is a discussion on this here.

About Task 1, @sauloperez recommended this article which has code for this. The code sets this up not just for the test environment but also for development. Should we do the same?

If we also do Task 1 for development, Task 3 will no longer be necessary because then span.translation_missing would never be generated there.

@mkllnk
Copy link
Member

mkllnk commented Oct 26, 2018

I ran a CI run with exceptions on missing translations. 1c92847

Here are the results:

Failures:

  1) Account and Billing Settings updating as an admin user attributes can be changed
     Failure/Error: raise "missing translation: #{key}"
     
     RuntimeError:
       missing translation: billing_and_account_settings
     # ./spec/support/i18n_error_raising.rb:4:in `block in <top (required)>'
     # ./app/controllers/admin/accounts_and_billing_settings_controller.rb:12:in `update'
     # ./lib/open_food_network/rack_request_blocker.rb:37:in `call'
     # ------------------
     # --- Caused by: ---
     # Capybara::CapybaraError:
     #   Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
     #   ./spec/spec_helper.rb:92:in `block (2 levels) in <top (required)>'

  2) Admin::AccountsAndBillingSettingsController update as super admin when required settings have values sets global config to the specified values
     Failure/Error: raise "missing translation: #{key}"
     
     RuntimeError:
       missing translation: billing_and_account_settings
     # ./spec/support/i18n_error_raising.rb:4:in `block in <top (required)>'
     # ./app/controllers/admin/accounts_and_billing_settings_controller.rb:12:in `update'
     # ./spec/controllers/admin/accounts_and_billing_settings_controller_spec.rb:94:in `block (5 levels) in <top (required)>'

  3) UserConfirmationsController requesting confirmation instructions to be resent redirects the user to login
     Failure/Error: raise "missing translation: #{key}"
     
     RuntimeError:
       missing translation: spree_user.send_instructions
     # ./spec/support/i18n_error_raising.rb:4:in `block in <top (required)>'
     # ./app/controllers/user_confirmations_controller.rb:15:in `create'
     # ./spec/controllers/user_confirmations_controller_spec.rb:63:in `block (3 levels) in <top (required)>'

  4) UserConfirmationsController requesting confirmation instructions to be resent sends the confirmation email
     Failure/Error: raise "missing translation: #{key}"
     
     RuntimeError:
       missing translation: spree_user.send_instructions
     # ./spec/support/i18n_error_raising.rb:4:in `block in <top (required)>'
     # ./app/controllers/user_confirmations_controller.rb:15:in `create'
     # ./spec/controllers/user_confirmations_controller_spec.rb:70:in `block (4 levels) in <top (required)>'
     # ./spec/support/matchers/email_confirmation_matchers.rb:3:in `block (2 levels) in <top (required)>'
     # ./spec/controllers/user_confirmations_controller_spec.rb:69:in `block (3 levels) in <top (required)>'

Failures:

  1) Spree::Payment applying transaction fees to Stripe payments when the payment information is invalid makes the transaction fee ineligible and finalizes it
     Failure/Error: raise "missing translation: #{key}"
     
     RuntimeError:
       missing translation: payment_method_not_supported
     # ./spec/support/i18n_error_raising.rb:4:in `block in <top (required)>'
     # ./app/models/spree/order_decorator.rb:339:in `block in process_payments!'
     # ./app/models/spree/order_decorator.rb:336:in `each'
     # ./app/models/spree/order_decorator.rb:336:in `process_payments!'
     # ./spec/models/spree/payment_spec.rb:178:in `block (5 levels) in <module:Spree>'

  2) Managing users as super-admin resending confirmation email displays success
     Failure/Error: raise "missing translation: #{key}"
     
     RuntimeError:
       missing translation: spree_user.send_instructions
     # ./spec/support/i18n_error_raising.rb:4:in `block in <top (required)>'
     # ./app/controllers/user_confirmations_controller.rb:15:in `create'
     # ./lib/open_food_network/rack_request_blocker.rb:37:in `call'
     # ------------------
     # --- Caused by: ---
     # Capybara::ExpectationNotMet:
     #   expected to find text "Resend done" in "Logged in as : grant_gutkowski@zemlak.info Account Logout Store DASHBOARD ORDERS PRODUCTS REPORTS CONFIGURATION USERS ORDER CYCLES GROUPS ENTERPRISES CUSTOMERS Editing User BACK TO USERS LIST GENERAL SETTINGS Email confirmation is pending. We've sent a confirmation email to scotty@mcdermott.co.uk. Resend... EMAIL ROLES Admin ENTERPRISE LIMIT PASSWORD CONFIRM PASSWORD UPDATE or CANCEL API ACCESS NO KEY GENERATE API KEY"
     #   ./spec/features/admin/users_spec.rb:40:in `block (5 levels) in <top (required)>'

These may be covered in other issues: #2874, #2374, #1829. We should fix them.

@luisramos0
Copy link
Contributor

I wonder what else we have to do here after #3006

@Matt-Yorkley
Copy link
Contributor

I ran a CI run with exceptions on missing translations

Integrating this into the CI build would be great!

@luisramos0
Copy link
Contributor

This is already there Matt, I wonder if it is working correclty though:
https://github.com/openfoodfoundation/openfoodnetwork/pull/3006/files

@filipefurtad0
Copy link
Contributor

Closing as per comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Welcome New QAs!
To triage (by maintainers)
Development

Successfully merging a pull request may close this issue.

6 participants