-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
src/shared/Validation.js
Outdated
const validInt = validator.isInt(input); | ||
|
||
let validIntType = true; | ||
if (parseInt(input, 10) < -32768 || parseInt(input, 10) > 32767) { |
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.
Avoid magic numbers. Create constants for these...
const INT_SHORT_SIGNED_MAX_VALUE = 32767;
const INT_SHORT_SIGNED_MIN_VALUE = -32768;
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.
Fixed
src/shared/Validation.js
Outdated
const idx = formatErrors.indexOf(FORM_ERRORS.INVALID_FORMAT); | ||
if (input && !validator.isInt(input)) { | ||
const validInt = validator.isInt(input); |
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.
validator
only validates strings, and will throw an error otherwise, which could potentially cause a lot of problems. Either surround this with a try-catch
, or ensure that input
will be a string.
https://github.com/chriso/validator.js/blob/master/src/lib/util/assertString.js
Further, isInt
takes a second optional parameter where you can specify a min and a max:
https://github.com/chriso/validator.js/blob/master/src/lib/isInt.js
I'd recommend removing the check below, and replace it with a single validator
check:
const isValid = validator.isInt(input, {
max: INT_SHORT_SIGNED_MAX_VALUE,
min: INT_SHORT_SIGNED_MIN_VALUE
})
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.
Fixed
src/shared/Validation.js
Outdated
validIntType = false; | ||
} | ||
|
||
if ((input && !validInt) || (input && !validIntType)) { |
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.
If input
is falsy, then validator.isInt
will either throw an Error or return false. Either way, this check seems redundant.
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.
Fixed
src/shared/Validation.js
Outdated
} | ||
case 'int64': { | ||
const idx = formatErrors.indexOf(FORM_ERRORS.INVALID_FORMAT); | ||
const validInt = validator.isInt(input); |
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.
Same comment about isInt
taking a second optional parameter where you can specify max
and min
.
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.
Fixed
src/shared/Validation.js
Outdated
|
||
let validIntType = true; | ||
if ( | ||
parseInt(input, 10) < -9223372036854775808 |
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.
Avoid magic numbers. Additionally, I recommend using Number.MAX_SAFE_INTEGER
and Number.MIN_SAFE_INTEGER
instead since we're validating JavaScript. They are not quite 64 bits, but are probably good enough for our use.
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.
Fixed
src/shared/Validation.js
Outdated
const idx = formatErrors.indexOf(FORM_ERRORS.INVALID_FORMAT); | ||
const currentDate = (new Date()).toISOString(); | ||
const validDate = validator.isISO8601(input) | ||
&& validator.isBefore(input, currentDate); |
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.
'date'
is generic, but validator.isBefore
is a restriction that makes using 'date'
not make much sense.
Enforcing isBefore
belongs in the date picker widget, where it disables the ability to choose any date after "today", but I'm not sure if the widget supports it.
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.
Theoretically, but there's a bug with it. pushtell/react-bootstrap-date-picker#139
src/shared/Validation.js
Outdated
const idx = formatErrors.indexOf(FORM_ERRORS.INVALID_FORMAT); | ||
const valid = validator.isAlphanumeric(input); | ||
|
||
if (input && !valid) { |
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.
I think checking valid
is enough since validator
will already have checked input
.
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.
Fixed
src/containers/form/FormContainer.js
Outdated
const input = e; | ||
handleDateInput = (e, section, name, formatErrors, setErrorsFn) => { | ||
let input = e; | ||
input = input.replace(/T(.*)$/g, 'T00:00:00.000Z'); |
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.
This is hacky, but we're stuck dealing with a bad dependency.
However, I believe all of the properties that are Date
only care about the day+month+year.
Can you try just removing everything after and including the T
, basically submitting only 2017-11-20
, and seeing if that works?
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.
Fixed. The shorter date submits fine.
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.
I found a bug outside of this PR. The DOB
in ConsumerInfoView
doesn't pass the format errors parameter to handleDateInput
, so there's errors being logged:
Uncaught TypeError: Cannot read property 'slice' of undefined
src/containers/form/FormContainer.js
Outdated
const input = e; | ||
handleDateInput = (e, section, name, formatErrors, setErrorsFn) => { | ||
let input = e; | ||
input = input.replace(/T(.*)$/g, 'T00:00:00.000Z'); |
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.
You need to handle the case where the "clear" button was clicked, which means input
will be 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.
Fixed
Includes:
Note: Phone validation is NOT included for this PR b/c the validator module's validation requires a country code and the design / implementation to make this happen is more intensive than is worth getting ready for the demo.