Skip to content
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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
24 changes: 18 additions & 6 deletions commcare_connect/events/tests.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from datetime import datetime

from rest_framework.test import APIClient

from commcare_connect.opportunity.models import Opportunity
Expand All @@ -8,34 +10,44 @@

def test_post_events(mobile_user_with_connect_link: User, api_client: APIClient, opportunity: Opportunity):
api_client.force_authenticate(mobile_user_with_connect_link)
assert Event.objects.count() == 0
response = api_client.post(
"/api/events/",
data=[
{
"event_type": "invalid_event_name",
"event_type": Event.Type.INVITE_SENT,
"user": mobile_user_with_connect_link.pk,
"opportunity": opportunity.pk,
"date_created": datetime.utcnow(),
},
{
"event_type": Event.Type.RECORDS_APPROVED,
"user": mobile_user_with_connect_link.pk,
"opportunity": opportunity.pk,
}
"date_created": datetime.utcnow(),
},
],
format="json",
)
assert response.status_code == 400
assert Event.objects.count() == 0
assert response.status_code == 201
assert Event.objects.count() == 2
response = api_client.post(
"/api/events/",
data=[
{
"event_type": Event.Type.INVITE_SENT,
"user": mobile_user_with_connect_link.pk,
"user": -1,
"opportunity": opportunity.pk,
"date_created": datetime.utcnow(),
},
{
"event_type": Event.Type.RECORDS_APPROVED,
"user": mobile_user_with_connect_link.pk,
"opportunity": opportunity.pk,
"date_created": datetime.utcnow(),
},
],
format="json",
)
assert response.status_code == 201
assert response.status_code == 400
assert Event.objects.count() == 2
22 changes: 19 additions & 3 deletions commcare_connect/events/views.py
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
Expand Down Expand Up @@ -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:
Copy link
Collaborator

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.

Copy link
Member Author

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.

event_objects = [Event(**item) for item in serializer.validated_data]
Copy link
Collaborator

Choose a reason for hiding this comment

The 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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 200, especially since that is still distinguishable from a full success.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I will update it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still plan to update this?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand Down
Loading