-
Notifications
You must be signed in to change notification settings - Fork 26
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
[#72] Add backend validation of form rules #4029
[#72] Add backend validation of form rules #4029
Conversation
52ee39e
to
69cdcc9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4029 +/- ##
==========================================
+ Coverage 96.07% 96.10% +0.02%
==========================================
Files 729 730 +1
Lines 22980 23094 +114
Branches 2674 2698 +24
==========================================
+ Hits 22078 22194 +116
+ Misses 639 638 -1
+ Partials 263 262 -1 ☔ View full report in Codecov by Sentry. |
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'm not seeing the matching e2e tests yet? These are important to ensure that the frontend and backend behaviour are aligned:
This commit handles the backend validation for the following components: - textarea - time - postcode - bsn (11-check) - select - checkbox - currency - signature - map
842d756
to
a68c8fd
Compare
* Refactored and moved validator * Added more tests * Fixed warning in playwright test
I tested some more complex forms that revealed some validation issues before, and as far as I can tell this doesn't introduce new problems yet, but not all component types were used in there 😬 |
Use accurate keys/labels for the components being tested.
* Especially text-based fields with multiple: true are problematic * Fixed textarea
4f27452
to
bc7fcfe
Compare
* Ensure multiple false|true is handled appropriately * Model current behaviour of frontend data in tests * Ensure that empty options are treated correctly depending on required/optional config
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.
@vaszig do you mind sanity checking my changes?
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 would approve this but github doesn't let me to do so..
The only thing, as mentioned in slack as well, is something like splitting some things like the TimeBetweenValidator
and the custom field, but I guess this can be done in the next-relative PR which is open.
|
||
is_valid, _ = validate_formio_data(component, {"time": "15:36"}) | ||
|
||
self.assertTrue(is_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 am missing something here. Could you please explain why we have this behaviour here?
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.
coverage reporting complained about missing branch coverage on the match
statement, so this is what it addresses - it happens that invalid configuration ends up in the JSON and this ensures it is logged/covered by tests :)
Partly closes #72
This PR handles the following components, the rest of them will be handled in an another PR: