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

Localize the password-reset email #41627

Merged
merged 11 commits into from
Jul 28, 2021
Merged

Conversation

hacodeorg
Copy link
Contributor

@hacodeorg hacodeorg commented Jul 19, 2021

FND-1481
Localize the password-reset email, which is sent by Devise::Mailer. Strings in this email are very sensitive. Thus, they are entrusted to only approved translators in the Code.org - Restricted CrowdIn project.

This PR contains:

  • Localized password-reset email template dashboard/app/views/devise/mailer/reset_password_instructions.html.erb.
  • English strings from the password-reset email dashboard/config/locales/restricted.en.yml.
  • Configuration for the sync-in step bin/i18n-codeorg/in.sh.
  • Configuration for the sync-up step bin/i18n/i18n_script_utils.rb, bin/i18n/codeorg_restricted_crowdin.yml, and bin/i18n/codeorg_crowdin.yml.
  • Unit test in dashboard/test/models/user_test.rb.

Links

Testing story

  1. Run the full I18n pipeline sync-in, sync-up, sync-down, and sync-out locally.
  2. Test reseting password and verify that emails sent are localized.
  • Use mailcatcher to intercept emails sent on the local machine.

  • For regular account (user who doesn't have associated child accounts)

    • Password-reset email in English
      Screen Shot 2021-07-20 at 3 08 04 AM

    • Password-reset email in es-MX
      Screen Shot 2021-07-21 at 12 05 43 AM

  • For user with associated child accounts

    • Password-reset email in English
      Screen Shot 2021-07-20 at 3 07 40 AM

    • Password-reset email in es-MX
      Screen Shot 2021-07-21 at 12 05 53 AM

  1. Run unit tests: bundle exec rails test test/models/user_test.rb

Follow-up work

  • Make the sync-up step to upload i18n/locales/source/dashboard/restricted.yml to dasboard/restricted.yml in Crowdin.
  • Make the sync-down step to download translated dasboard/restricted.yml files to i18n/locales/<locale_code>/dashboard/restricted.yml. Currently, it downloads translated files to i18n/locales/<language_name>/dashboard/restricted.yml. See this line.
  • Delete subject string in reset_password_instructions in devise.en.yml
  • Add bin/i18n/codeorg_restricted_credentials.yml file to the i18n-dev server.
  • Delete test translations in Code.org - Restricted project.
  • Copy the list of languages from Code.org project to Code.org - Restricted project. (link)

Security

Only a few approved translators, likely all Code.org members, can join the Code.org - Restricted project.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@hacodeorg hacodeorg force-pushed the ha/translate-pwd-reset-email branch from 255650b to d81ae10 Compare July 19, 2021 19:55
@hacodeorg hacodeorg changed the title Translate password-reset email Localize the password-reset email Jul 19, 2021
@hacodeorg hacodeorg marked this pull request as ready for review July 19, 2021 21:28
@hacodeorg hacodeorg requested a review from a team as a code owner July 19, 2021 21:28
@annaxuphoto
Copy link
Contributor

I'm a bit confused why these strings have to be in a different Crowdin project. The strings don't contain PII, what are we worried about here?

@hacodeorg
Copy link
Contributor Author

I'm a bit confused why these strings have to be in a different Crowdin project. The strings don't contain PII, what are we worried about here?

To me, the restricted CrowdIn project is for strings from sensitive contexts that we want to control and maintain high translation quality. Direct email from Code.org is considered sensitive. Email related to password is even more sensitive.
@dju90 can explain other aspects of this decision.

@dju90
Copy link
Contributor

dju90 commented Jul 22, 2021

I'm a bit confused why these strings have to be in a different Crowdin project. The strings don't contain PII, what are we worried about here?

To me, the restricted CrowdIn project is for strings from sensitive contexts that we want to control and maintain high translation quality. Direct email from Code.org is considered sensitive. Email related to password is even more sensitive.
@dju90 can explain other aspects of this decision.

Yep, it's about us not wanting bad or malicious translations for these strings, more so than others. In the future, things like the privacy policy and terms of service might live in this project.

Copy link
Contributor

@annaxuphoto annaxuphoto left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it'd be nice if someone else can review the changes to bin/i18n/codeorg_restricted_crowdin.yml, as I'm not very familiar with that code

@@ -32,6 +32,7 @@ files: [
# hourofcode content is handled by the hourofcode-specific sync
"ignore" : [
"/source/hourofcode/**",
"/source/dashboard/restricted.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we care about generalizing the file names similar to the hourofcode path, for future proofing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, it's only 1 file; and I think we will use just this 1 file for a while. Later, if we want to break it into more files we can change this config easily.

In addition, I put this file under /source/dashboard/ path so it can be processed by the current in-up-down-out code without much changes.

Copy link
Member

@daynew daynew left a comment

Choose a reason for hiding this comment

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

The email template needs to avoid string concatenation, otherwise it will be very challenging to translate.

@hacodeorg hacodeorg requested a review from daynew July 28, 2021 06:05
@hacodeorg hacodeorg merged commit f2a2901 into staging Jul 28, 2021
@hacodeorg hacodeorg deleted the ha/translate-pwd-reset-email branch July 28, 2021 18:51
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.

None yet

4 participants