-
Notifications
You must be signed in to change notification settings - Fork 1
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
add event tracking #349
base: main
Are you sure you want to change the base?
add event tracking #349
Changes from 1 commit
51ffdb5
f97b2c5
4a2467c
dadbeae
99467b8
46e95e5
6be106a
a7b1db5
e85f605
e1c03fd
c59a387
51b7719
34bb90f
fdd182a
badcef5
a753311
dac5557
0a6e359
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import django_tables2 as tables | ||
from dal.autocomplete import ModelSelect2 | ||
from django.db import transaction | ||
from django.utils.decorators import method_decorator | ||
from django.views.decorators.csrf import csrf_exempt | ||
from django_filters import ChoiceFilter, FilterSet, ModelChoiceFilter | ||
|
@@ -34,9 +35,24 @@ def create(self, request, *args, **kwargs): | |
|
||
serializer = self.get_serializer(data=request.data, many=True) | ||
serializer.is_valid(raise_exception=True) | ||
|
||
event_objects = [Event(**item) for item in serializer.validated_data] | ||
Event.objects.bulk_create(event_objects) | ||
try: | ||
event_objects = [Event(**item) for item in serializer.validated_data] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its generally safer to be explicit about incoming fields in case the phone adds unexpected ones, and to allow it to intentionally send fields you are not including (like the event id we have discussed). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have listed the fields here explicitly https://github.com/dimagi/commcare-connect/pull/349/files#diff-a7c344d1bdc227452ddf03733e8218375041272c8e3c6157bb96c11a7fa195ebR23 (May be I am not getting your point) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might have been my ignorance about the serializer. What happens if there are unexpected fields sent up? Are they ignored or raise an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, they get ignored. |
||
Event.objects.bulk_create(event_objects) | ||
except Exception as e: | ||
# Bulk create failed, try saving each item individually | ||
failed_items = [] | ||
|
||
for item in serializer.validated_data: | ||
try: | ||
with transaction.atomic(): | ||
Event.objects.save(**item) | ||
except Exception: | ||
failed_items.append((item, e)) | ||
|
||
if failed_items: | ||
partial_error_response = {"error": "Some items could not be saved", "failed_items": failed_items} | ||
headers = self.get_success_headers(serializer.data) | ||
return Response(partial_error_response, status=status.HTTP_206_PARTIAL_CONTENT, headers=headers) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it looks like 206 is really intended for "range" requests. Since that isn't what this is, I think it's fine to use a more standard response like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, I will update it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you still plan to update this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I have updated it to 201 |
||
|
||
headers = self.get_success_headers(serializer.data) | ||
return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers) | ||
|
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 talked a few times about having these events include IDs that you could send back to the phone to indicate which ones succeeded. I don't see any code that could handle that here. Specifically it looks like the code will error if they send ids with these events (or it will try to set that id as the PK, which is even more dangerous).
I do see that you instead send down failures (though still not with uniquely identifiable IDs, so it will not necessarily be straightforward for the phone to know which events they match to). Successes are generally preferable because there are less ways for that to go wrong, and it doesn't require the phone to track which events it sent, and there is no possibility for mismatch, it just deletes the ones you say succeeded.
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.
Yea, I updated the code (and responded to that comment above https://github.com/dimagi/commcare-connect/pull/349/files#r1701902592). I took a simpler approach of sending down just the failed events. I don't understand how including IDs makes it any better for mobile to track what events failed.