feat: improve the PIN Enter repetition validation rule and add relevant messages #1401
Conversation
…nt messages Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
| @@ -71,7 +71,7 @@ export const PINRules: PINValidationRules = { | |||
| only_numbers: true, | |||
| min_length: 6, | |||
| max_length: 6, | |||
| no_repeated_numbers: false, | |||
There was a problem hiding this comment.
I know this is in draft but if it's doable could we keep the defaults the same and just allow for opt-in behavior with the injectable PINSecurity.rules config we have currently?
There was a problem hiding this comment.
Put both PINSecurity and PINCreationValidation to IoC container, then the wallet variants can customize them without changinf Bifold default validating behavior. I will discuss with team. Thanks @bryce-mcmath
There was a problem hiding this comment.
Back to default, but change the validating logic for '0' as 'repeating times' instead of its boolean value, keep the extra message for numbers repeating allowed but the repeating times exceeding the max times rule
…refactored the messages Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
…lidation message and change the logic of '0' in no_repeated_behavior Signed-off-by: Jian Wang <[email protected]>
jian4on
changed the title
| noRepeatedNumbers = new RegExp(`(\\d)\\1{${PINRules.no_repeated_numbers - 1},}`) | ||
| } | ||
|
|
||
| if ('number' === typeof(PINRules.no_repeated_numbers) && PINRules.no_repeated_numbers > 2 ) { |
There was a problem hiding this comment.
When would the > 2 check be needed at the same time as the string interpolation below is? Wouldn't there only be one case (3) where that condition is satisfied?
| @@ -77,7 +77,7 @@ describe('PIN creation validations', () => { | |||
| test('PIN with repeated numbers and repeated numbers validation to true, so the validation use the default of two repeated numbers, should return NoRepetitionOfTheSameNumbersValidation as invalid', async () => { | |||
| const PINRulesWithRepeatedNumbers = { | |||
| ...defaultPINRules, | |||
| no_repeated_numbers: true, | |||
| no_repeated_numbers: 0, | |||
There was a problem hiding this comment.
I think no_ should be change to num_ if it now indicates the number of of repeating values allowed.
There was a problem hiding this comment.
@jleach Good point, It is type 'boolean | number' now. I changed it to an enum [1,2,3,4 ...] type in the Draft version for discussing and then changed it back. Actually I still think maxRepetition number is an option.
@bryce-mcmath If you think we need more discussion, I will pull this feat back to draft
There was a problem hiding this comment.
@jian4on ah right, the type changed. num is good I think, we can change the default since the type is changing. maybe just add a note in the "Breaking changes" section of the description of this PR so other teams know what to change to keep their existing behavior
There was a problem hiding this comment.
Not sure this will work well. There will be no way to disable the rule unless another flag is added. You can put in a high number but that will then be displayed during password selection, which would be confusing for the user.
…e rule 'max_repeated_numbers', and refactor the test cases relevently Signed-off-by: Jian Wang <[email protected]>
…on test' Signed-off-by: Jian Wang <[email protected]>
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1401 +/- ##
==========================================
+ Coverage 55.52% 55.61% +0.08%
==========================================
Files 223 227 +4
Lines 7985 8039 +54
Branches 2241 2273 +32
==========================================
+ Hits 4434 4471 +37
- Misses 3528 3544 +16
- Partials 23 24 +1 ☔ View full report in Codecov by Sentry. |
| no_repeated_numbers: number | ||
| no_repetition_of_the_two_same_numbers: boolean | number |
There was a problem hiding this comment.
We should probably make the same change for the rules below:
| no_repeated_numbers: number | |
| no_repetition_of_the_two_same_numbers: boolean | number | |
| no_repeated_numbers: number | |
| no_repetition_of_the_two_same_numbers: number |
|
@jleach I think your re-review will be required as well |
Summary of Changes
Change the PIN repetition validation to a number type rule 'no_repeated_numbers' to make the repeated-times validation setting more readable
Screenshots, videos, or gifs
N/A
Breaking change guide
The previous 'no_repeated_numbers' was an union type 'boolean | number', it is changed to a simple number. The default validation setting was '0' for allowing any times repeating.
Don't implement boolean value, the number value explanation:
0: Disable the repetition check, e.g. '111111' is allowed
1: Enable the repetition check, '123456' is allowed but '113456' forbidden
2: Enable the repetition check, '113456' is allowed but '111456' forbidden
...
Related Issues
N/A
Pull Request Checklist
Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.
Signed-off-byline (we use the DCO GitHub app to enforce this)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
Pro Tip 🤓