-
Notifications
You must be signed in to change notification settings - Fork 132
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
Return flow data results for merged datasets #1773
Conversation
Side effects of implementing the PR as is
|
57c4c86
to
ea543e9
Compare
response.data['attributes']['responses']) | ||
# The transportation form(self.xform) contains 11 responses | ||
# Assert that the responses are returned | ||
self.assertEqual(len(response.data['attributes']['responses']), 11) |
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.
Chose to use the number of responses returned(11) for this test due to erratic time values returned in the responses
.
ea543e9
to
10d560d
Compare
Using a data migration to solve the missing Other options we have is to set the values lazily either:
Opinions on a favorable way to proceed ? |
10d560d
to
614ab84
Compare
614ab84
to
200aefe
Compare
I will recommend the data migration as well as the default value for the UUID field. This will be the data migration first and a second migration that sets the default value. |
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.
can you please add a test for the generation of default IDs
20c3a0c
to
a3f7fd2
Compare
# Always have uuids. | ||
# Assert that set_uuid is called with an object | ||
# that already has a uuid. | ||
self.assertEqual(set_uuid_mock.call_count, 1) |
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.
The DataDictionary
model always sets uuid for XForms. Sadly, since the MergedXForm
model inherits from the XForm
model directly it doesn't have the uuid set...(And probably a few other custom processing steps ?)
I wonder if we should just modify the save
method of the MergedXForm model to set the uuid ? I prefer having the uuid set directly from the parent class(XForm)
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.
Yes, let's have a UUID for the merged form.
I've added a test to test the generation of the default uuids in the latest commits. Please have a look. |
c6b2676
to
84c7033
Compare
# Assert that the uuid is not empty and is | ||
# 32 characters in length | ||
self.assertNotEqual(uuid, '') | ||
self.assertEqual(len(uuid), 32) |
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.
uuid4 is always 36 characters (32 hex digits + 4 dashes).
Decided to use the default way the uuid
field is set for DataDictionary
objects to not diverge from the norm; Generates uuids without the dashes(only hex)
b97e3cd
to
a1ef783
Compare
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.
a1ef783
to
e816e11
Compare
33e1398
to
dd535d0
Compare
Add the 'uuid' field from the MergedXForm model to the tuple of fields to serialize in the MergedXFormSerializer. - Limit 'uuid' field to write only
Set the 'uuid' field on MergedXForms on save
da3520e
to
8820db3
Compare
a1b7221
to
8a2a4a2
Compare
@DavisRayM Flow data results for merged datasets now have data. |
8a2a4a2
to
6e10034
Compare
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.
Generally looks good to me. I have one question though
def generate_uuid_if_missing(apps, schema_editor): | ||
""" | ||
Generate uuids for XForms without them | ||
""" | ||
XForm = apps.get_model('logger', 'XForm') | ||
|
||
for form in XForm.objects.filter(uuid=''): | ||
form.uuid = get_uuid() | ||
form.save() |
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.
Won't this be expensive when running migrations - even though it might be one time? Can we do this in the XForm's save
function; check if that form has a uuid and set one if it doesn't exist.
I guess my worry is just the app going through all XForms checking which one doesn't have a uuid
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.
May be not check on the XForm's instance save function but where the uuid is required
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.
I'm not entirely sure if this query would be expensive to run(I'll try analyzing the query on a large database and get back to you). The XForm
save function has a check to set the UUID when missing through the DataDictionary
model.
The main reason this was included was because of MergedXForms
since they're not created through the DataDictionary
model. But, I've also modified the MergedXForm
save to set it when missing...
I'm not sure at the moment about when MergedXForms
are updated/saved again after creation. I'll update this once I research a bit more
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.
Cool. I guess i'm fine if retrieving xforms without uuid won't be expensive
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.
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.
Cool
""" | ||
XForm = apps.get_model('logger', 'XForm') | ||
|
||
for form in XForm.objects.filter(uuid=''): |
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.
We tend to use xform
instead of form as a variable of an XForm
model object. Can we keep the same for consistency?
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.
Yes, I've changed this to xform
in the latest commits
Add a data migration that generates 'uuid' values for XForms that do not have a uuid
Return FLOIP results for merged datasets; Retrieve all instances attached to the forms that make up the MergedXForm
Bump pyfloip version to commit 3c980eb184069ae7c3c9136b18441978237cd41d
Support both uuid4 and uuid4 hex string on FLOIP Endpoint; Allow users to use the string to retrieve and publish FLOIP Responses & Results
Utilize uuid4 string when generating 'Location' header values for the FLOIP endpoint
Add ReadOnlyUUIDField custom serializer field to serializer UUID fields
6e10034
to
bcbf0b6
Compare
Changes / Features implemented
MergedXForm
on savetest_retrieve_responses_merged_dataset
uuid4
string or its hex versionSteps taken to verify this change does what is intended
QA Checklist
uuid4
strings are used to identify packages(All ids returned by the endpoint should be uuid4). A uuid4 string can be identified by its 32 hex characters and 4 dashesCloses #1643