Lint _html keys in YML files by zachmargolis · Pull Request #8713 · 18F/identity-idp

zachmargolis · 2023-07-03T18:28:59Z

I am trying to see what it would take to enforce:

that all i18n keys that have HTML inside them have a _html suffix
that all keys with an _html suffix actually contain HTML that needs escaping

zachmargolis · 2023-07-03T18:29:46Z

app/presenters/two_factor_login_options_presenter.rb

The copy for was like Some sentence. %{cancel_link} so I figured might as well split this out into two separate i18n keys

* Makes sure they actually contain HTML changelog: Internal, Source code, Make HTML escaping in translations consistent

zachmargolis · 2023-07-05T20:56:54Z

I think I got this working and cleaned up the whole codebase! PTAL!

zachmargolis · 2023-07-05T20:57:39Z

app/views/forgot_password/show.html.erb

  <p><%= t('notices.forgot_password.no_email_sent_explanation_start') %>
  <%= f.button :button, t('links.resend'), class: 'usa-button--unstyled margin-left-05' %></p>

-  <% link = link_to(


I chose to inline this assignment

zachmargolis · 2023-07-05T20:58:11Z

app/views/idv/gpo_verify/index.html.erb


 <%= render PageHeadingComponent.new.with_content(t('forms.verify_profile.welcome_back')) %>
-<%= sanitize t('forms.verify_profile.welcome_back_description'), tags: %w[p strong] %>
+<%= t('forms.verify_profile.welcome_back_description_html') %>


the _html does the same marking as HTML-safe that sanitize does basically

I guess the sanitize would have been stricter about the allowed tags? But I don't think we worry about that with non-user-provided strings like this, or at least we don't elsewhere.

Yeah with hard coded string I don't think we gain much benefit from sanitize

zachmargolis · 2023-07-05T20:58:42Z

app/views/user_mailer/phone_added.html.erb

-  <%= raw t(
-        '.help', disavowal_link: link_to(
+  <%= t(
+        '.help_html', disavowal_link_html: link_to(


ditto here, switching to an _html key is the same as raw

zachmargolis · 2023-07-05T20:59:11Z

app/views/users/emails/verify.html.erb

 <%= t('notices.signed_up_and_confirmed.no_email_sent_explanation_start') %>
 <%= button_to(add_email_resend_path, method: :post, class: 'usa-button usa-button--unstyled', form_class: 'display-inline-block padding-left-1') { t('links.resend') } %>

-<p><% link = link_to(t('notices.use_diff_email.link'), add_email_path) %></p>


this was an empty <p> tag because <% doesn't output anything, so I removed it when inlining the variable

zachmargolis · 2023-07-05T22:10:08Z

config/locales/two_factor_authentication/fr.yml

        de sauvegarde en lieu sûr. Si vous perdez vos codes de sauvegarde, vous
        ne pourrez plus vous connecter à %{app_name}.
-      new_backup_codes_html: J’ai besoin de nouveaux codes de sauvegarde
+      new_backup_codes_html: J’ai besoin de nouveaux codes de sauvegarde&ZeroWidthSpace;


ok so for this one, the English copy has   to control word wrapping, but I didn't know the appropriate place to put one in the other languages, so I added &ZeroWidthSpace; to the end because it should be invisible and not affect the wrapping

it's also the specific string from #8708

(Saw this comment after my previous remark)

aduth

Love this 👍

aduth · 2023-07-06T12:47:55Z

app/presenters/idv/otp_verification_presenter.rb

      t(
        "instructions.mfa.#{otp_delivery_preference}.number_message_html",
-        number: content_tag(:strong, phone_number),
+        number_html: content_tag(:strong, phone_number),


Just to clarify: Does the automated test catch that this should be _html-suffixed, or was it manual? Since the HTML only comes at the runtime code via content_tag

Or I suppose you were able to work your way backward on the parent string (e.g. number_message_html) that was otherwise flagged for an unnecessary _html to see why it was in-fact necessary.

Correct the parent tag had an un-necessary HTML without that

aduth · 2023-07-06T12:49:56Z

app/views/idv/gpo_verify/index.html.erb


 <%= render PageHeadingComponent.new.with_content(t('forms.verify_profile.welcome_back')) %>
-<%= sanitize t('forms.verify_profile.welcome_back_description'), tags: %w[p strong] %>
+<%= t('forms.verify_profile.welcome_back_description_html') %>


I guess the sanitize would have been stricter about the allowed tags? But I don't think we worry about that with non-user-provided strings like this, or at least we don't elsewhere.

aduth · 2023-07-06T12:55:10Z

config/locales/two_factor_authentication/es.yml

        respaldo. No podrá iniciar sesión en %{app_name} si pierde sus códigos
        de respaldo.
-      new_backup_codes_html: Necesito nuevos códigos de respaldo
+      new_backup_codes_html: Necesito nuevos códigos de respaldo&ZeroWidthSpace;


Was the &ZeroWidthSpace; added here to avoid having it be flagged?

I wonder if we should only check that at least one of the locales has HTML?

Alternatively, the idea with the   in the English version is to keep "backup codes" together on the same line, and I suppose we could do something similar in other languages like códigos de respaldo

If you think that's a better way to prevent wrapping, I'm all for it? I didn't have a sense of the font size/layout of this page

What do you think about the idea of the new test only running over the en.yml file, or just checking that at least one of the locales has HTML? So that we don't need to change this file.

I like the idea of checking that "at least one" has HTML... but the current structure of the assertions is each file independently, not grouped by locale. Let me noodle on that

Gave it a shot in a300659, PTAL!

I futzed with it, so a failure where one string has HTML but none of them should looks like this now:

Failures: 1) I18n config/locales/two_factor_authentication has HTML inside at least one locale string for all keys with .html or _html Failure/Error: expect(bad_keys).to be_empty expected `{"two_factor_authentication.account_reset.cancel_link"=>{"en"=>"Cancel your request", "es"=>"Cancelar su solicitud <STRONG>", "fr"=>"Annuler votre demande"}}.empty?` to be truthy, got false

aduth · 2023-07-06T12:55:32Z

config/locales/two_factor_authentication/fr.yml

        de sauvegarde en lieu sûr. Si vous perdez vos codes de sauvegarde, vous
        ne pourrez plus vous connecter à %{app_name}.
-      new_backup_codes_html: J’ai besoin de nouveaux codes de sauvegarde
+      new_backup_codes_html: J’ai besoin de nouveaux codes de sauvegarde&ZeroWidthSpace;


(Saw this comment after my previous remark)

Sgtpluck

sweet!

Sgtpluck · 2023-07-06T12:30:15Z

app/presenters/piv_cac_error_presenter.rb

+      t('instructions.mfa.piv_cac.no_certificate_html', try_again_html: try_again_link)
    when 'certificate.not_auth_cert'
-      t('instructions.mfa.piv_cac.not_auth_cert_html', please_try_again: please_try_again_link)
+      t('instructions.mfa.piv_cac.not_auth_cert_html', please_try_again_html: please_try_again_link)


[question, nonblocking] why are we sometimes saying please and sometimes not?

Are you asking about the specific copy? I'm not sure. Might be personal differences in how different engineers turn strings into keys

string needs the _html

aduth

LGTM 👍

zachmargolis requested review from aduth and jmdembe July 3, 2023 18:28

zachmargolis commented Jul 3, 2023

View reviewed changes

aduth changed the title ~~Link _html keys in YML files~~ Lint _html keys in YML files Jul 5, 2023

Add lint to standardize i18n keys named with HTML

dc13fd9

* Makes sure they actually contain HTML changelog: Internal, Source code, Make HTML escaping in translations consistent

zachmargolis force-pushed the margolis-lint-html-yml branch from afe3876 to dc13fd9 Compare July 5, 2023 20:55

zachmargolis marked this pull request as ready for review July 5, 2023 20:56

zachmargolis commented Jul 5, 2023

View reviewed changes

zachmargolis and others added 11 commits July 5, 2023 14:17

fix

445fa0b

fixes

e9f8afe

erblint

494d2e6

wip

a6abbd0

Merge branch 'main' into margolis-lint-html-yml

d26f03c

eslint --fix

50ad7df

wip

752d41c

fixes

2be5594

mailerspec

9d37b27

fixes

3f73ec1

one more

5ac1af1

zachmargolis commented Jul 5, 2023

View reviewed changes

aduth approved these changes Jul 6, 2023

View reviewed changes

Sgtpluck approved these changes Jul 6, 2023

View reviewed changes

zachmargolis added 2 commits July 6, 2023 09:32

Update HTML check to check across all locales, so that only one

a300659

string needs the _html

Shorten comment for 100c

2c0aa06

aduth approved these changes Jul 6, 2023

View reviewed changes

zachmargolis merged commit 6c75bb9 into main Jul 6, 2023

zachmargolis deleted the margolis-lint-html-yml branch July 6, 2023 18:05

This was referenced Jul 6, 2023

LG-10214 Suspended User Create Account Flow #8725

Merged

Deploy RC 295 #8746

Closed

Deploy RC 295 #8748

Merged

Conversation

zachmargolis commented Jul 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Jul 5, 2023

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!

aduth left a comment

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!

Sgtpluck left a comment

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!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants