Skip to content

LG-459 Clean up localizations#2333

Merged
monfresh merged 1 commit intomasterfrom
mb-update-i18n-tasks
Jul 24, 2018
Merged

LG-459 Clean up localizations#2333
monfresh merged 1 commit intomasterfrom
mb-update-i18n-tasks

Conversation

@monfresh
Copy link
Copy Markdown
Contributor

@monfresh monfresh commented Jul 18, 2018

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

  • For DB changes, check for missing indexes, check to see if the changes
    affect other apps (such as the dashboard), make sure the DB columns in the
    various environments are properly populated, coordinate with devops, plan
    migrations in separate steps.

  • For route changes, make sure GET requests don't change state or result in
    destructive behavior. GET requests should only result in information being
    read, not written.

  • For encryption changes, make sure it is compatible with data that was
    encrypted with the old code.

  • For secrets changes, make sure to update the S3 secrets bucket with the
    new configs in all environments.

  • Do not disable Rubocop or Reek offenses unless you are absolutely sure
    they are false positives. If you're not sure how to fix the offense, please
    ask a teammate.

  • When reading data, write tests for nil values, empty strings,
    and invalid formats.

  • When calling redirect_to in a controller, use _url, not _path.

  • When adding user data to the session, use the user_session helper
    instead of the session helper so the data does not persist beyond the user's
    session.

  • When adding a new controller that requires the user to be fully
    authenticated, make sure to add before_action :confirm_two_factor_authenticated.

@monfresh
Copy link
Copy Markdown
Contributor Author

This PR is best reviewed one commit at a time. The first one sorts the keys in all files alphabetically (which has been the desired state for a while but hasn't been enforced) using i18n-tasks normalize. The second one removes unused localizations and only ignores specific ones that are actually used but that i18n-tasks thinks are unused.

To keep this PR small, I will follow up with a second PR that moves all the 2FA stuff from config/locales/devise into config/locales/two_factor_authentication.

@monfresh monfresh force-pushed the mb-update-i18n-tasks branch from a867d3c to 105e76c Compare July 18, 2018 03:23
@monfresh monfresh changed the title Clean up localizations LG-459 Clean up localizations Jul 18, 2018
@mryenq
Copy link
Copy Markdown
Contributor

mryenq commented Jul 18, 2018

LGTM, do we want to elaborate in the comments on why we tell i18n-tasks to ignore certain keys? (For example because they're referenced as string values only in view models, or similar.)

In the context of preparing an automated process for translations, I wonder if there isn't a better way to manage the above that would eliminate this manual step and help the gem identify these. (Presumably it searches for translation method calls.)

@monfresh
Copy link
Copy Markdown
Contributor Author

I can add a comment explaining why they are being ignored. Basically, the gem is not able to detect that localizations with dynamic keys are being used: https://github.com/glebm/i18n-tasks#dynamic-keys

We are ignoring them because they are indeed used, but the gem thinks they are not, and if we didn't ignore them, the spec that makes sure that we don't have any unused translations would fail.

@jmhooper
Copy link
Copy Markdown
Contributor

For some of these dynamic ones, does it make sense to use a comment to ignore them in the code where they are used? Seems easier to remember to delete a comment in the code you're looking at than a config in a file you probably aren't.

@monfresh
Copy link
Copy Markdown
Contributor Author

I took a closer look, and there are several entries that are actually unused so this list will get smaller. I will clean them up.

@monfresh
Copy link
Copy Markdown
Contributor Author

Sure, I can try the magic comments.

@monfresh
Copy link
Copy Markdown
Contributor Author

This is what the difference is:

Using magic comments, we have to specify every single occurrence, like this:

  # i18n-tasks-use t('forms.piv_cac_setup.certificate.bad_html')
  # i18n-tasks-use t('forms.piv_cac_setup.certificate.expired_html')
  # i18n-tasks-use t('forms.piv_cac_setup.certificate.invalid_html')
  # i18n-tasks-use t('forms.piv_cac_setup.certificate.none_html')
  # i18n-tasks-use t('forms.piv_cac_setup.certificate.revoked_html')
  # i18n-tasks-use t('forms.piv_cac_setup.certificate.unverified_html')
  # i18n-tasks-use t('forms.piv_cac_setup.piv_cac.already_associated_html')
  # i18n-tasks-use t('forms.piv_cac_setup.token.bad_html')
  # i18n-tasks-use t('forms.piv_cac_setup.token.invalid_html')
  # i18n-tasks-use t('forms.piv_cac_setup.token.missing_html')

versus a single line in the config

 - 'forms.piv_cac_setup.*'

Which one do y'all prefer?

Also, for some reason, the gem also can't see that error messages are used. Do we really want to add a magic comment for every single validation error message (40+)?

@jmhooper
Copy link
Copy Markdown
Contributor

Ah, yeah that's gross. Maybe not for now. I wonder why the gem doesn't let you put the match string in the comments.

@monfresh
Copy link
Copy Markdown
Contributor Author

@mryenq I added comments and removed some more unused entries here: f565158

@monfresh
Copy link
Copy Markdown
Contributor Author

Thanks, @mryenq for the thumbs up. Could I get an official approval please, if everything looks good?

Copy link
Copy Markdown
Contributor

@mryenq mryenq left a comment

Choose a reason for hiding this comment

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

Sure, I thought you and @jmhooper were still discussing so held off.

LGTM.

Copy link
Copy Markdown
Contributor

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

LGTM2

@monfresh monfresh force-pushed the mb-update-i18n-tasks branch from f565158 to 98d291e Compare July 24, 2018 01:03
**Why**:
- We like to keep our keys sorted alphabetically, but many files were
not sorted
- The configuration for the `i18n-tasks` gem was ignoring all strings
in the `config/locales/devise` folder, and so we ended up with lots of
unused strings.

**How**:
- Run `i18n-tasks normalize` to sort the keys
- Add `i18n-tasks normalize` to the `make normalize_yaml` script so
that keys get sorted as part of our normalization
- Remove the `line-width` setting from the `i18n-tasks` config because
when you run `i18n-tasks normalize`, it also changes the formatting to
not wrap at 80 characters, but we expect our files to be wrapped at
80 characters (which is the default behavior of Psych, the Ruby YAML
parser).
- Move the non-Devise strings to their own section and ignore them
when `i18n-tasks` thinks they are unused (usually due to interpolation)
- Remove any remaining unused files using `i18n-tasks remove-unused`
@monfresh monfresh force-pushed the mb-update-i18n-tasks branch from 98d291e to f19f2d6 Compare July 24, 2018 01:24
@monfresh monfresh merged commit f4be781 into master Jul 24, 2018
@monfresh monfresh deleted the mb-update-i18n-tasks branch July 24, 2018 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants