🔊 [#4927] Add system check for missing allow_blank on non-required charfields #4989
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4989 +/- ##
==========================================
- Coverage 96.66% 96.65% -0.02%
==========================================
Files 761 761
Lines 25957 25983 +26
Branches 3393 3402 +9
==========================================
+ Hits 25092 25113 +21
- Misses 601 603 +2
- Partials 264 267 +3 ☔ View full report in Codecov by Sentry. |
…elds because in most cases it is desired to allow empty strings as well
stevenbal
force-pushed
the
feature/4927-serializer-allow-blank-system-check
branch
from
8fa38e2 to
ff6cc90
Compare
|
@sergei-maertens should I write a test for this check? |
No, I don't think so, So you can |
| @register | ||
| def check_serializer_non_required_charfield_allow_blank_true(app_configs, **kwargs): | ||
| """ | ||
| https://github.com/open-formulieren/open-forms/issues/4927 |
There was a problem hiding this comment.
Let's not make the summary line of a docstring a link to a github issue, always provide a description summary, and you can include the link in the body. But even better, the issue reference in the commit message should be sufficient.
| """ | ||
| https://github.com/open-formulieren/open-forms/issues/4927 | ||
|
|
||
| Check for serializers.CharFields that have required=False, but not `allow_blank=True` |
There was a problem hiding this comment.
| Check for serializers.CharFields that have required=False, but not `allow_blank=True` | |
| Check for serializers.CharFields that have ``required=False``, but not ``allow_blank=True`` |
| https://github.com/open-formulieren/open-forms/issues/4927 | ||
|
|
||
| Check for serializers.CharFields that have required=False, but not `allow_blank=True` | ||
| to avoid errors occurring when empty strings are passed from the frontend. |
There was a problem hiding this comment.
| to avoid errors occurring when empty strings are passed from the frontend. | |
| to avoid bogus validation errors occurring when empty strings are provided by the frontend. |
| errors = [] | ||
| serializers = get_subclasses(Serializer) | ||
| for serializer in serializers: | ||
| # TODO not sure if this issue can also occur for fields defined in Meta.fields? |
There was a problem hiding this comment.
I think it respects the model field and blank=False will result in required=True
| serializers = get_subclasses(Serializer) | ||
| for serializer in serializers: | ||
| # TODO not sure if this issue can also occur for fields defined in Meta.fields? | ||
| for field_name, field in serializer._declared_fields.items(): |
There was a problem hiding this comment.
you could consider using drf_spectacular.plumbing.force_instance to get serializer instances, and then you can operate on serializer.fields instead?
| file_path = inspect.getfile(serializer) | ||
| line_number = get_field_line_number(serializer, field_name) |
There was a problem hiding this comment.
maybe we can use serializer_class.__module__ and serializer_class.__qualname__ to get the full python path instead to point to it, instead of trying to figure out line numbers etc.?
| # No line number indicates that the field is inherited from another class | ||
| if not file_path or not line_number: | ||
| continue |
Closes #4927
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh./bin/compilemessages_js.shDockerfile/scripts
./binfolderCommit hygiene