-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
Fixed #28950 -- Fixed ArrayField.has_changed() for empty values. #9520
Conversation
tests/postgres_tests/test_array.py
Outdated
@@ -737,6 +737,14 @@ def test_already_converted_value(self): | |||
vals = ['a', 'b', 'c'] | |||
self.assertEqual(field.clean(vals), vals) | |||
|
|||
def test_has_changed(self): | |||
field = SimpleArrayField(forms.CharField()) | |||
self.assertFalse(field.has_changed(None, None)) |
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.
use self.assertIs(..., False)
(assertFalse
checks bool(val) is False
)
tests/postgres_tests/test_array.py
Outdated
field = SimpleArrayField(forms.CharField()) | ||
self.assertFalse(field.has_changed(None, None)) | ||
self.assertFalse(field.has_changed(None, '')) | ||
self.assertFalse(field.has_changed(None, [])) |
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 [], None, and '' all realistic values for data
? Generally I would expect only string values from POST data.
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.
for an empty field ''
is passed in POST data.
if you are referring to pure html form implementation, then None
and []
are not encountered. but if the form is used otherwise (validating api requests) you might expect to see these values.
try: | ||
data_value = self.to_python(data) | ||
except ValidationError: | ||
pass |
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.
Test coverage is missing for this branch. You might need to add some initial test coverage for has_changed()
in a separate commit.
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.
added testcase for existing implementation
https://code.djangoproject.com/ticket/28950