fix: allow setting a list as a field value#21
fix: allow setting a list as a field value#21GitRon merged 3 commits intoambient-innovation:mainfrom
Conversation
|
the code looks good. Would you mind adding at least one test for the changes? Best regards |
|
Hi @GitRon ! |
|
@JBthePenguin That would be awesome. But having tests to ensure the new functionality works and keeps working is important, too 🙂 |
|
@GitRon Test added. |
GitRon
left a comment
GitRon
left a comment
There was a problem hiding this comment.
Hi @JBthePenguin! Thanks for the tests. I've asked the AI for a second opinion since I've took over this package and I'm not the one that built it in the first place.
What do you think?
django_dynamic_admin_forms/admin.py
Outdated
| bound_field.field.queryset = queryset | ||
| bound_field.form.data = bound_field.form.data.copy() | ||
| bound_field.form.data[field_name] = value | ||
| if isinstance(value, Iterable): |
There was a problem hiding this comment.
issue: I've asked the AI for a second opinion. Seems legit, doesn't it?
Potential Issue: str is Iterable
In Python, str is an Iterable. If a get_dynamic_*_field method returns a string as value, it will be passed to .setlist(), which would split the string into individual characters — almost certainly not the intended behavior.
For example, if value = "hello", then setlist(field_name, "hello") would set the field to ["h", "e", "l", "l", "o"].
The fix should explicitly exclude strings:
if isinstance(value, Iterable) and not isinstance(value, str):
Or alternatively, check specifically for list/tuple:
if isinstance(value, (list, tuple)):
| with self.assertRaises(PermissionDenied): | ||
| mixin.render_field(request) | ||
|
|
||
| def test_render_field_with_custom_method_for_many_to_many_field(self): |
There was a problem hiding this comment.
issue: Another valid AI suggestin IMHO
The new test (test_render_field_with_custom_method_for_many_to_many_field) verifies the happy path returns a 200 status. It's a reasonable integration-style test, but could be strengthened by:
- Asserting the response body contains the expected widget HTML with the pre-selected values ("1" and "2").
- Adding a test that a non-list value (e.g., a string) still works correctly after this change — to catch the str is Iterable edge case.
There was a problem hiding this comment.
@GitRon Thanks for comments! That seems legit, yes. I will post an update as soon as I find a moment to do so.
There was a problem hiding this comment.
@GitRon Two tests added. The first one to check if setlist method is called when value is a list, and the second one to check if setlist method is not called when value is a str.
There was a problem hiding this comment.
Thanks! ❤️ Will have a look once I find a minute 😅
|
@JBthePenguin Thanks for the work! Just published the new release: https://pypi.org/project/django-dynamic-admin-forms |
|
@GitRon Thanks! |
2 participants
To set form data, use the
setlistmethod ofQueryDictwhen the provided value isIterable.