-
Notifications
You must be signed in to change notification settings - Fork 10
Consider numbers in the required validator #92
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
base: master
Are you sure you want to change the base?
Conversation
@@ -63,6 +63,20 @@ describe('[required] validation rule tests =>', () => { | |||
expect(validationResult.errorMessage).to.be.empty; | |||
}); | |||
|
|||
it('should return true if typeof value is number', () => { | |||
// Arrange | |||
const value = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the validation if value is 0
or negative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talking with @brauliodiez comments that we should not consider negative values, nor zero. That is, the validation fail in these cases:
- Value equals undefined
- Value equals null
- Value equals ' '
For another value, should pass validation:
- 'some text'
- 0
- 1
- -1
- {}
- []
- { property: 'value' }
etc.
@v-borrego, what do you think? Could you add some unit tests for these cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will add the tests in the next pull request
@@ -17,7 +17,7 @@ export const required: FieldValidationFunction = (value, vm, customParams: Requi | |||
function isValidField(value, trim: boolean): boolean { | |||
return typeof value === 'string' ? | |||
isStringValid(value, trim) : | |||
value === true; | |||
value === true || typeof value === "number"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract value === true || typeof value === "number"
in a function like isNonStringValid
to check non string values. That is, is different than undefined
or null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evaluate if value is different than null or undefined collides with the "When validating a non string value should return false if value is false" test. What do you consider for a boolean with false value?
The data model binded to the form may contain numerical attributes that are not being considered in the "required" validator