-
Notifications
You must be signed in to change notification settings - Fork 8
Removed needsBus and added/fixed tests for travel #625
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
Conversation
| }) | ||
| .withMessage( | ||
| `${fieldname} must be between ${lowerBound} and ${upperBound}` | ||
| `${fieldname} must be between ${lowerBound} and ${upperBound}` |
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.
does this only validate that travel is an integer? or does it also cover the [0,100] inclusive bound?
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 made a new test all together that also validates the 0-100 bound
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.
does it validate float vs int? or is that being done on front end?
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.
@Ty-Won that would be a frontend validation, I'll have a PR for that tn 😉
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 we want to stay properly RESTful the int validation should be on the API
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.
Agreed we should have api validation
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.
The validator does check .isInt() which seems to work. If I input a float, like 10.5 for travel, then indeed the error appears saying that it should be an integer. Do we need more than this?
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.
Seems good to me, I also added int validation on the frontend: hackmcgill/dashboard#764
| }; | ||
|
|
||
| // duplicate of newHack0, but with 101 for travel | ||
| const invalidHacker2 = { |
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.
are we missing tests for impairments and barriers? I'll make a ticket if we are
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, there are no tests for those currently.
Tickets:
HCK-194, 176
List of changes:
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Questions for code reviewers?
Checklist: