more localization for two-factor #885

gissimo · 2023-11-27T09:34:02Z

I have added more localization strings in template two_factor_setup.html, and updated accordingly ES and IT translations.

codecov · 2023-11-27T09:37:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c8062d) 98.33% compared to head (a22bb98) 98.33%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #885   +/-   ##
=======================================
  Coverage   98.33%   98.33%           
=======================================
  Files          34       34           
  Lines        4452     4453    +1     
=======================================
+ Hits         4378     4379    +1     
  Misses         74       74

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jwag956 · 2023-12-04T17:57:11Z

Thanks - and sorry for taking a while to respond. I'd like to try to get the translations for 'primary_method' up into the form/response - rather than in the template itself - we have those strings already localized - just not a reverse look up. And there are 2 places these exist - two-factor and unified signin.
If you have time/inclination to do that - please do - otherwise - let me know and i will work on that - and let you then just add appropriate translations.

…-factor

gissimo · 2023-12-05T18:24:37Z

Hi! Thanks for feedback. I see your point. I've moved localization to views.py and created there a dictionary (like the one in forms.py) because what is shown now for primary method is a code and not really a string. Also, already existing localization strings are similar ones but not exact ones, so they need to be created anew. How do you feel about it?

jwag956

Thanks for working on this - a couple small comments.

jwag956 · 2023-12-06T03:50:33Z

flask_security/views.py

+    "email": _("Email"),
+    "mail": _("Email"),
+    "sms": _("SMS"),
+    None: _("None"),


I think you want "None" here - not pythons None!

getattr(user, "tf_primary_method", None) in views.py#L884 can return indeed a python None, according to what's stored in user attribute. OK to have both str and None as dictionary keys?

jwag956 · 2023-12-06T03:53:05Z

flask_security/views.py

@@ -126,6 +127,15 @@
    webauthn_verify_response,
 )

+_tf_methods = {


I would like to see this in forms.py - that's where all the other labels are - and reference that from views. This should also make it easier to re-use in unified_signin.py which has a very similar localization issue.

Sure thing, done!

jwag956

Thanks for the changes.
I think there are a few more places this same translation is needed.
In views::two_factor_token_validation - both render_template set 'chosen_method' - those I think should use the new translation table.

Same in views::two_factor_rescue:render_template

Also - in two_factor_setup - higher up in the code there is a render_template that sets chosen_method for the two_factor_verify_form - this also I think should be translated.

gissimo · 2023-12-11T20:17:06Z

Hi!
I have added translation for chosen_method when it is used as string. However, sometimes it is then used as key inside the template. In these cases, I have left it as it is. To avoid confusion, I have added suffix _xlated for when it is a string and translated.
What do you think?

jwag956

Thanks for sticking with this. This is a great improvement.
See comments about maintaining backwards compatibility for templaes.

jwag956 · 2023-12-13T14:54:41Z

flask_security/templates/security/two_factor_setup.html

@@ -4,7 +4,7 @@
  On GET:
    choices: Value of SECURITY_TWO_FACTOR_ENABLED_METHODS (with possible addition of 'delete'
    two_factor_required: Value of SECURITY_TWO_FACTOR_REQUIRED
-    primary_method: if a two-factor method has already been set up.
+    primary_method_xlated: if a two-factor method has already been set up.


While I agree this is confusing - making this change now will break backward compatibility for applications with their own templates. From what I can see - there are no forms where the same value (primary_method) is used both as a key and as an informational piece of info (in other words - from your changes - we never have BOTH primary_method AND primary_method_xlated. So - lets favor keeping things compatible...

jwag956 · 2023-12-13T14:55:04Z

flask_security/templates/security/two_factor_verify_code.html

@@ -4,7 +4,7 @@
 {% block content %}
  {% include "security/_messages.html" %}
  <h1>{{ _fsdomain("Two-factor Authentication") }}</h1>
-  <h2>{{ _fsdomain("Please enter your authentication code generated via: %(method)s", method=chosen_method) }}</h2>
+  <h2>{{ _fsdomain("Please enter your authentication code generated via: %(method)s", method=chosen_method_xlated) }}</h2>


same here - lets keep things compatible.

flask_security/views.py

jwag956 · 2023-12-13T14:57:13Z

flask_security/views.py

@@ -880,7 +881,9 @@ def two_factor_setup():
        two_factor_verify_code_form=code_form,
        choices=choices,
        chosen_method=form.setup.data,
-        primary_method=getattr(user, "tf_primary_method", "None"),
+        primary_method_xlated=_tf_methods_xlate[


to keep compatibility - lets change all these back to what they were (without the _xlated)

gissimo · 2023-12-13T21:54:59Z

Hi, all change requests applied! Let me know if I can be of any help anymore.

jwag956 · 2023-12-14T04:32:09Z

Thanks!

more localization for two-factor

2f6019a

gissimo and others added 2 commits December 5, 2023 18:53

Merge branch 'Flask-Middleware:master' into more-localization-for-two…

a627c47

…-factor

moved localizable strings from template to view

e865437

jwag956 requested changes Dec 6, 2023

View reviewed changes

moved localizable strings from view to forms

80df761

jwag956 requested changes Dec 11, 2023

View reviewed changes

more translations and more clarity about what not to translate

f540171

jwag956 requested changes Dec 13, 2023

View reviewed changes

maintain backwards compatibility for templates

a22bb98

jwag956 approved these changes Dec 14, 2023

View reviewed changes

jwag956 merged commit 3018dde into pallets-eco:master Dec 14, 2023
17 checks passed

gissimo deleted the more-localization-for-two-factor branch December 20, 2023 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more localization for two-factor #885

more localization for two-factor #885

gissimo commented Nov 27, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading

jwag956 commented Dec 4, 2023

gissimo commented Dec 5, 2023

jwag956 left a comment

jwag956 Dec 6, 2023

gissimo Dec 6, 2023

jwag956 Dec 6, 2023

gissimo Dec 6, 2023

jwag956 left a comment

gissimo commented Dec 11, 2023

jwag956 left a comment

jwag956 Dec 13, 2023

jwag956 Dec 13, 2023

jwag956 Dec 13, 2023

gissimo commented Dec 13, 2023

jwag956 commented Dec 14, 2023

more localization for two-factor #885

more localization for two-factor #885

Conversation

gissimo commented Nov 27, 2023

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

jwag956 commented Dec 4, 2023

gissimo commented Dec 5, 2023

jwag956 left a comment

Choose a reason for hiding this comment

jwag956 Dec 6, 2023

Choose a reason for hiding this comment

gissimo Dec 6, 2023

Choose a reason for hiding this comment

jwag956 Dec 6, 2023

Choose a reason for hiding this comment

gissimo Dec 6, 2023

Choose a reason for hiding this comment

jwag956 left a comment

Choose a reason for hiding this comment

gissimo commented Dec 11, 2023

jwag956 left a comment

Choose a reason for hiding this comment

jwag956 Dec 13, 2023

Choose a reason for hiding this comment

jwag956 Dec 13, 2023

Choose a reason for hiding this comment

jwag956 Dec 13, 2023

Choose a reason for hiding this comment

gissimo commented Dec 13, 2023

jwag956 commented Dec 14, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading