Just modify the Django UUIDField for our 32 digit hex representation. #163
Conversation
| def prepare_value(self, value): | ||
| if isinstance(value, uuid.UUID): | ||
| return value.hex | ||
| return value |
There was a problem hiding this comment.
This one doesn't seem to be captured in the parent class.
There was a problem hiding this comment.
(and is an important part of having it return the 32 char value rather than a UUID object, from what I recall)
There was a problem hiding this comment.
(I think maybe specifically for cases where someone sets it on there as a UUID object and then reads it back, without it going through the database yet)
But it could probably call to_python here to be DRYer.
There was a problem hiding this comment.
prepare_value is a form field method though, not a regular field: https://github.com/django/django/blob/stable/1.11.x/django/forms/fields.py#L129
I do wonder if that's where some of the original implementation errors came from, as it seems the original implementation here was a copy of the Django form field UUIDField: https://github.com/django/django/blob/stable/1.11.x/django/forms/fields.py#L1206 but then wrapped around the model CharField implementation instead of the form field.
There was a problem hiding this comment.
There is definitely something missing though, because during the serialization tests, it is returning as a UUID.
There was a problem hiding this comment.
The default Django serialization framework uses the value_to_string method, which we seem to have not defined here.
See: https://github.com/django/django/blob/stable/1.11.x/django/db/models/fields/__init__.py#L834
Although we're then not using that in the serialize method: https://github.com/learningequality/morango/blob/release-v0.6.x/morango/models/core.py#L939
There was a problem hiding this comment.
Have now updated to add a value_from_object override that properly coerces the value to a 32 digit hex string.
| value = super(UUIDField, self).to_python(value) | ||
|
|
||
| if connection.features.has_native_uuid_field: | ||
| return value | ||
| return value.hex |
There was a problem hiding this comment.
I'm now not seeing a difference between this and the parent class implementation, other than the use of super, which doesn't seem necessary, since to_python isn't defined in this class, so it'll use the super's anyway. So I'm guessing this can be removed?
Summary
TODO
Reviewer guidance
Don't think I've missed anything but I'd appreciate @jamalex's eyes to be sure.
Issues addressed
Fixes #89