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

Correct untranslated messages #5881

Merged
merged 3 commits into from
Apr 1, 2021
Merged

Correct untranslated messages #5881

merged 3 commits into from
Apr 1, 2021

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Mar 26, 2021

Status

Ready for review

Description of Changes

Fixes #5755.

Corrects some misplaced parentheses in gettext/format calls. Marks some strings that were being presented untranslated.

Fixes locale/session handling on CSRF error. We were translating the CSRF error message, then clearing the session and showing the rest of the page in the default locale, almost certainly English. We need to reevaluate how we want to handle the locale when clearing the session, but for now, this just fixes the inconsistency.

Adds localization checks to basic source/journalist interface tests. We haven't really been testing translations. In the page layout tests, the content of strings is often only checked after making sure the page is not translated. This has led to us not catching text which didn't get marked for translation, thus being shown in English to users who are using a different locale.

Includes the following changes to make it easier to test that source strings are properly translated:

  • a context manager which will xfail a test for a locale if any of the message IDs passed to it are not present in the locale's message catalog
  • a pytest flaky filter which prevents retries of expected failures
  • turning off flaky's reporting of successful tests
  • functions to obtain test locales and counts to use in testing their pluralization rules, which can be used with pytest.parametrize to easily run a test in multiple locales, including fuzzing plural strings to check that translations are complete, while only trying as many variations as the rules need.

Adding locale parameterization to tests increases the overall time they take, of course: test_journalist.py went from around 2.5 minutes to 3.5 minutes.

Testing

Application string changes

  • git checkout develop
  • make dev

Journalist interface

  • Log in to the journalist interface as an admin.
  • Edit user "dellsberg".
  • Change the locale to "Français".
  • Resetting the user's password ("Réinitialiser le mot de passe") should present a translated message containing the new password in a <code> tag, so that it is displayed in a fixed-width typeface. (This is just checking that the formatting wasn't broken by reorganizing the translation.)
  • The following changes will all flash error messages in English:
    • Set the first name ("Prénom") to something longer than 100 characters.
    • Set the family name ("Nom de famille") to something longer than 100 characters.
    • Set the username ("Nom d’utilisateur") to a single character.
    • Set the username to "deleted".
  • Return to the main admin and try to add a user with username "dellsberg". The error about the username conflict will be in English.
  • In your SecureDrop repository root: chmod 555 securedrop/static/i
  • Visit the instance configuration page and upload a new site logo. The error shown because the directory is not writable will be in English.
  • Now check out this branch with git checkout -b fix-5755 origin/fix-5755.
  • The French message catalog needs to be edited to translate the new messages. Replace securedrop/translations/fr_FR/LC_MESSAGES/messages.po with the test translations in this gist.
  • Restart the dev server.
  • Repeat all the tests after the password reset test, verifying that the error messages are now in French.
  • Don't forget to restore the permissions on your image directory.

Source interface

  • Keep using the dev server.
  • This is another check that reorganized translation code hasn't broken an HTML message: running curl -sH "X-tor2web: encrypted" 'http://localhost:8080?l=fr_FR' | grep Tor2Web should produce:
<strong>AVERTISSEMENT</strong>&nbsp;Il semble que vous utilisez Tor2Web, qui ne garantit pas votre anonymat. &nbsp;<a href="/tor2web-warning">Pourquoi est-ce dangereux?</a></p>

Deployment

This is mostly changes to text or tests. The string changes should correct several cases of English messages being shown to people who've chosen another locale. There are a couple of changes to messages that wind up in logs; the new messages record less information.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

this looks great! i left some comments that i'll provide now. i still need to run through the test plan before i can approve.

securedrop/journalist_app/admin.py Show resolved Hide resolved
securedrop/journalist_app/admin.py Show resolved Hide resolved
securedrop/journalist_app/utils.py Show resolved Hide resolved
securedrop/models.py Show resolved Hide resolved
securedrop/models.py Show resolved Hide resolved
securedrop/models.py Outdated Show resolved Hide resolved
securedrop/source_app/__init__.py Outdated Show resolved Hide resolved
Correct some misplaced parentheses in gettext/format calls.

Mark some strings that were being presented untranslated.

Fix locale/session handling on CSRF error. We were translating the
CSRF error message, then clearing the session and showing the rest of
the page in the default locale, almost certainly English. We need to
reevaluate how we want to handle the locale when clearing the session,
but for now, this just fixes the inconsistency.
We haven't really been testing translations. In the page layout tests,
the content of strings is often only checked after making sure the
page is *not* translated. This has led to us not catching text which
didn't get marked for translation, thus being shown in English to
users who are using a different locale.

This commit includes the following changes to make it easier to test
that source strings are properly translated:

- a context manager which will xfail a test for a locale if any of the
message IDs passed to it are not present in the locale's message
catalog

- a pytest flaky filter which prevents retries of expected failures

- turning off flaky's reporting of successful tests

- functions to obtain test locales and counts to use in testing their
pluralization rules, which can be used with pytest.parametrize to
easily run a test in multiple locales, including fuzzing plural
strings to check that translations are complete, while only trying as
many variations as the rules need.

Adding locale parameterization to tests increases the overall time
they take, of course: test_journalist.py went from around 2.5 minutes
to 3.5 minutes.
@sssoleileraaa
Copy link
Contributor

@rmol this message "Name not updated: Name too long" should be in French, right?

Screenshot from 2021-03-31 18-22-45

@rmol
Copy link
Contributor Author

rmol commented Apr 1, 2021

Yes it should. I left a crucial step out of the test plan. The French message catalog needs to be edited to translate the new messages, and the dev server restarted. I've updated the test plan, sorry about that.

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

works great with the gist you provided for French translations:
Screenshot from 2021-04-01 08-56-16

@sssoleileraaa sssoleileraaa merged commit fd2f5f8 into develop Apr 1, 2021
@sssoleileraaa sssoleileraaa deleted the fix-5755 branch April 1, 2021 15:58
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.

gettext calls containing formatted strings produce untranslated text
2 participants