Just modify the Django UUIDField for our 32 digit hex representation.#163
Just modify the Django UUIDField for our 32 digit hex representation.#163rtibbles wants to merge 3 commits intolearningequality:release-v0.6.xfrom
Conversation
jamalex
left a comment
jamalex
left a comment
There was a problem hiding this comment.
Probably a good move overall, but a few notes/concerns.
| 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.
|
|
||
| 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?
jamalex
left a comment
jamalex
left a comment
There was a problem hiding this comment.
Looks good to me, once it's been tested within Kolibri's test suite as well!
2 participants
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