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

Allow default MFA help text to be translated along with the rest of the application's translatable strings #4549

Merged
merged 3 commits into from Jul 26, 2023

Conversation

jnm
Copy link
Member

@jnm jnm commented Jul 22, 2023

No description provided.

…with the rest of the application's translatable strings
Comment on lines +238 to +240
# It's terrible, but if you change this, you must also change
# `MFA_DEFAULT_HELP_TEXT` in `static_lists.py` so that
# translators receive the new string
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps a more sane approach would be to have superusers leave the default value empty when they do not want to override it. However, doing that would take away our ability to use the default string to show an instructional example. Copying and pasting the string seemed like the lesser evil 🙃

Comment on lines -59 to -67
'mfa_localized_help_text': lambda i18n_texts: {
lang: markdown(text)
for lang, text in json.loads(
constance.config.MFA_LOCALIZED_HELP_TEXT.replace(
'##support email##',
constance.config.SUPPORT_EMAIL
)
).items()
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, it was impossible for this to fail. Always make sure your tests can fail! I'll take some blame here, too, because making values automatically get assertEqual()ed while requiring callables to do their own assertions leaves some clarity to be desired.

@@ -33,41 +35,44 @@ def setUp(self):
'community_url': constance.config.COMMUNITY_URL,
'frontend_min_retry_time': constance.config.FRONTEND_MIN_RETRY_TIME,
'frontend_max_retry_time': constance.config.FRONTEND_MAX_RETRY_TIME,
'project_metadata_fields': lambda x: \
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about the quantity of formatting changes. The smallest unit of code that I could invoke python-black on was the dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Who am I to complain about receiving few lines of new formatting changes in a review? 😇

Comment on lines 159 to 175
queries = 19
queries = 20
Copy link
Member Author

Choose a reason for hiding this comment

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

This query is being executed twice for some reason:

SELECT "constance_config"."id", "constance_config"."key", "constance_config"."value" FROM "constance_config" WHERE "constance_config"."key" = 'MFA_LOCALIZED_HELP_TEXT' LIMIT 21

I had to give up caring about this for now. I have another branch that I'm about to PR anyway that reduces (or at least was reducing) the number of queries from 19 to 18, so… 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

jnm and others added 2 commits July 22, 2023 00:46
Everyone should do it!
Clean up the environment (API endpoint)
@noliveleger noliveleger merged commit e9e6b04 into beta Jul 26, 2023
4 checks passed
@noliveleger noliveleger deleted the translate-default-mfa-help-text branch July 26, 2023 19:17
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

2 participants