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

Add missing error messages to zxcvbn en.yml #5776

Merged
merged 3 commits into from
Jan 5, 2022

Conversation

nathanberg
Copy link
Contributor

@aduth Do we have existing spec tests that are related to this which would catch this type of missing error message in the yml file?

@mitchellhenke
Copy link
Contributor

There isn't a great way to test unfortunately: formigarafa/zxcvbn-rb#4

It looks like we were missing the suggestions here: https://github.com/formigarafa/zxcvbn-rb/blob/4258f8b97653e5a297fce2ef7d987aceebc3151e/lib/zxcvbn/feedback.rb#L7, but I think the other ones are covered?

@@ -17,6 +17,7 @@ en:
For a stronger password, use a few words separated by spaces, but avoid
common phrases
names_and_surnames_by_themselves_are_easy_to_guess: Names and surnames by themselves are easy to guess
no_need_for_symbols_digits_or_uppercase_letters: No need for symbols, digits, or uppercase letters
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll need to request spanish and fresh translations of these strings (which is why the specs are failing)

some info here: https://handbook.login.gov/articles/appdev-i18n.html#translating-content-guidance

@aduth
Copy link
Member

aduth commented Jan 4, 2022

@aduth Do we have existing spec tests that are related to this which would catch this type of missing error message in the yml file?

None that I'm aware of, and as @mitchellhenke mentioned, not much in the way of recommendations for testing. It would be nice if the full set of messages were at least catalogued somewhere in code, but as best I can tell in both the Ruby and JavaScript implementations, the messages are scattered throughout the logic.

@@ -30,4 +31,5 @@ en:
this_is_a_top_100_common_password: This is a top-100 common password
this_is_a_very_common_password: This is a very common password
this_is_similar_to_a_commonly_used_password: This is similar to a commonly used password
use_a_few_words_avoid_common_phrases: Use a few words, avoid common phrases
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this one is still missing in spanish, french:

> bundle exec i18n-tasks missing
Missing translations (2) | i18n-tasks v0.9.35
+--------+------------------------------------------------------+------------------------------------------+
| Locale | Key                                                  | Value in other locales or source         |
+--------+------------------------------------------------------+------------------------------------------+
|   es   | zxcvbn.feedback.use_a_few_words_avoid_common_phrases | en Use a few words, avoid common phrases |
|   fr   | zxcvbn.feedback.use_a_few_words_avoid_common_phrases | en Use a few words, avoid common phrases |
+--------+------------------------------------------------------+------------------------------------------+

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanberg nathanberg merged commit 873c71c into main Jan 5, 2022
@nathanberg nathanberg deleted the nathanberg-Fix-unreadable-password-error-(LG-5461) branch January 5, 2022 14:24
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.

4 participants