-
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 all commits
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 |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from django.apps import AppConfig | ||
|
||
|
||
class EventsAppConfig(AppConfig): | ||
default_auto_field = "django.db.models.BigAutoField" | ||
name = "commcare_connect.events" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# Generated by Django 4.2.5 on 2024-07-13 14:45 | ||
|
||
from django.conf import settings | ||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
|
||
|
||
class Migration(migrations.Migration): | ||
initial = True | ||
|
||
dependencies = [ | ||
migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
("opportunity", "0044_opportunityverificationflags"), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name="Event", | ||
fields=[ | ||
("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), | ||
("date_created", models.DateTimeField(db_index=True)), | ||
( | ||
"event_type", | ||
models.CharField( | ||
max_length=40, | ||
db_index=True | ||
), | ||
), | ||
( | ||
"opportunity", | ||
models.ForeignKey( | ||
null=True, on_delete=django.db.models.deletion.PROTECT, to="opportunity.opportunity" | ||
), | ||
), | ||
( | ||
"user", | ||
models.ForeignKey( | ||
null=True, on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL | ||
), | ||
), | ||
( | ||
"metadata", | ||
models.JSONField(default=dict), | ||
), | ||
], | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
from datetime import datetime | ||
|
||
from django.db import models | ||
|
||
from commcare_connect.cache import quickcache | ||
from commcare_connect.users.models import User | ||
|
||
from . import types | ||
|
||
|
||
class Event(models.Model): | ||
from commcare_connect.opportunity.models import Opportunity | ||
|
||
# this allows referring to event types in this style: Event.Type.INVITE_SENT | ||
Type = types | ||
|
||
date_created = models.DateTimeField(db_index=True) | ||
event_type = models.CharField(max_length=40, db_index=True) | ||
user = models.ForeignKey(User, on_delete=models.CASCADE, null=True) | ||
opportunity = models.ForeignKey(Opportunity, on_delete=models.PROTECT, null=True) | ||
metadata = models.JSONField(default=dict) | ||
|
||
@classmethod | ||
@quickcache([], timeout=60 * 60) | ||
def get_all_event_types(cls): | ||
return set(cls.objects.values_list("event_type", flat=True).distinct()) | set(types.EVENT_TYPES) | ||
|
||
def save(self, *args, **kwargs): | ||
if not self.date_created: | ||
self.date_created = datetime.utcnow() | ||
super().save(*args, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
from datetime import datetime | ||
|
||
from rest_framework.test import APIClient | ||
|
||
from commcare_connect.opportunity.models import Opportunity | ||
from commcare_connect.users.models import User | ||
|
||
from .models import Event | ||
|
||
|
||
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": Event.Type.INVITE_SENT, | ||
"user": mobile_user_with_connect_link.pk, | ||
"opportunity": opportunity.pk, | ||
"date_created": datetime.utcnow(), | ||
"uid": "1", | ||
}, | ||
{ | ||
"event_type": Event.Type.RECORDS_APPROVED, | ||
"user": mobile_user_with_connect_link.pk, | ||
"opportunity": opportunity.pk, | ||
"date_created": datetime.utcnow(), | ||
"metadata": {"extra": "test"}, | ||
"uid": "2", | ||
}, | ||
], | ||
format="json", | ||
) | ||
assert response.status_code == 201 | ||
assert Event.objects.count() == 2 | ||
response = api_client.post( | ||
"/api/events/", | ||
data=[ | ||
{ | ||
"event_type": Event.Type.INVITE_SENT, | ||
"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 == 400 | ||
assert Event.objects.count() == 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
from django.utils.translation import gettext as _ | ||
|
||
# Server/Web events | ||
INVITE_SENT = _("Invite Sent") | ||
INVITE_ACCEPTED = _("Invite Accepted") | ||
RECORDS_APPROVED = _("Records Approved") | ||
RECORDS_REJECTED = _("Records Rejected") | ||
PAYMENT_APPROVED = _("Payment Approved") | ||
PAYMENT_ACCRUED = _("Payment Accrued") | ||
PAYMENT_TRANSFERRED = _("Payment Transferred") | ||
NOTIFICATIONS_SENT = _("Notifications Sent") | ||
ADDITIONAL_BUDGET_ADDED = _("Additional Budget Added") | ||
|
||
MODULE_COMPLETED = _("Module Completed") | ||
ALL_MODULES_COMPLETED = _("All Modules Completed") | ||
ASSESSMENT_PASSED = _("Assessment Passed") | ||
ASSESSMENT_FAILED = _("Assessment Failed") | ||
|
||
JOB_CLAIMED = _("Job Claimed Successfully") | ||
DELIVERY_FORM_SUBMITTED = _("Delivery Form Submitted") | ||
PAYMENT_ACKNOWLEDGED = _("Payment Acknowledged") | ||
|
||
|
||
EVENT_TYPES = [ | ||
INVITE_SENT, | ||
INVITE_ACCEPTED, | ||
JOB_CLAIMED, | ||
MODULE_COMPLETED, | ||
ALL_MODULES_COMPLETED, | ||
ASSESSMENT_PASSED, | ||
ASSESSMENT_FAILED, | ||
DELIVERY_FORM_SUBMITTED, | ||
RECORDS_APPROVED, | ||
RECORDS_REJECTED, | ||
PAYMENT_APPROVED, | ||
PAYMENT_ACCRUED, | ||
PAYMENT_TRANSFERRED, | ||
PAYMENT_ACKNOWLEDGED, | ||
NOTIFICATIONS_SENT, | ||
ADDITIONAL_BUDGET_ADDED, | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
from django.urls import path | ||
|
||
from .views import EventListView | ||
|
||
app_name = "events" | ||
|
||
urlpatterns = [ | ||
path("", view=EventListView.as_view(), name="event_htmx"), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
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 | ||
from django_filters.views import FilterView | ||
from rest_framework import serializers, status | ||
from rest_framework.generics import ListCreateAPIView | ||
from rest_framework.permissions import IsAuthenticated | ||
from rest_framework.response import Response | ||
|
||
from commcare_connect.opportunity.forms import DateRanges | ||
from commcare_connect.opportunity.models import Opportunity | ||
from commcare_connect.opportunity.views import OrganizationUserMixin | ||
from commcare_connect.users.models import User | ||
|
||
from .models import Event | ||
|
||
|
||
class EventSerializer(serializers.ModelSerializer): | ||
uid = serializers.JSONField(write_only=True, required=False) | ||
|
||
class Meta: | ||
model = Event | ||
fields = ["date_created", "event_type", "user", "opportunity", "metadata", "uid"] | ||
|
||
def to_internal_value(self, data): | ||
# Extract the 'meta' field if present and remove it from the data | ||
uid = data.pop("uid", None) | ||
|
||
internal_value = super().to_internal_value(data) | ||
internal_value["uid"] = uid | ||
|
||
return internal_value | ||
|
||
|
||
@method_decorator(csrf_exempt, name="dispatch") | ||
class EventListCreateView(ListCreateAPIView): | ||
queryset = Event.objects.all() | ||
serializer_class = EventSerializer | ||
permission_classes = [IsAuthenticated] | ||
|
||
def create(self, request, *args, **kwargs): | ||
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. What happens if one item passed by the phone is invalid? Does the whole request fail or are the rest created? If the first, I think that could cause issues because then one bad event will make all subsequent requests from the phone fail as it will be included in each one. If the second, I think we need to let the phone know which succeeded and which failed so it knows which records to retry and which it can delete 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 is a great point, from your above comment maintaining some sort of ID would address this. I will add that. 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 this change is still pending? 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. Instead of having an ID, I have gone with a simpler approach of just sending back the rows that fail. |
||
if not isinstance(request.data, list): | ||
return Response({"error": "Expected a list of items"}, status=status.HTTP_400_BAD_REQUEST) | ||
|
||
serializer = self.get_serializer(data=request.data, many=True) | ||
serializer.is_valid(raise_exception=True) | ||
try: | ||
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. 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 commentThe 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] | ||
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: | ||
import sentry_sdk | ||
|
||
sentry_sdk.capture_exception(e) | ||
# Bulk create failed, try saving each item individually | ||
failed_items = [] | ||
|
||
for item in serializer.validated_data: | ||
uid = item.pop("uid") | ||
try: | ||
with transaction.atomic(): | ||
Event(**item).save() | ||
except Exception as e: | ||
sentry_sdk.capture_exception(e) | ||
failed_items.append(uid) | ||
|
||
if failed_items: | ||
partial_error_response = {"success": False, "failed_items": failed_items} | ||
headers = self.get_success_headers(serializer.data) | ||
return Response(partial_error_response, status=status.HTTP_201_CREATED, headers=headers) | ||
|
||
headers = self.get_success_headers(serializer.data) | ||
return Response({"success": True}, status=status.HTTP_201_CREATED, headers=headers) | ||
|
||
|
||
class EventTable(tables.Table): | ||
date_created = tables.Column(verbose_name="Time") | ||
metadata = tables.Column(verbose_name="Metadata", orderable=False) | ||
|
||
class Meta: | ||
model = Event | ||
template_name = "events/htmx_table.html" | ||
fields = ("user", "opportunity", "event_type", "date_created", "metadata") | ||
|
||
|
||
class EventFilter(FilterSet): | ||
date_range = ChoiceFilter(choices=DateRanges.choices, method="filter_by_date_range", label="Date Range") | ||
user = ModelChoiceFilter( | ||
queryset=User.objects.all(), | ||
widget=ModelSelect2( | ||
url="users:search", | ||
attrs={ | ||
"data-placeholder": "All", | ||
}, | ||
), | ||
) | ||
event_type = ChoiceFilter(choices=lambda: [(_type, _type) for _type in Event.get_all_event_types()]) | ||
|
||
class Meta: | ||
model = Event | ||
fields = ["opportunity", "user", "event_type"] | ||
|
||
def filter_by_date_range(self, queryset, name, value): | ||
if not value: | ||
return queryset | ||
|
||
try: | ||
date_range = DateRanges(value) | ||
return queryset.filter( | ||
date_created__gte=date_range.get_cutoff_date(), | ||
) | ||
except ValueError: | ||
return queryset | ||
|
||
|
||
class EventListView(tables.SingleTableMixin, OrganizationUserMixin, FilterView): | ||
table_class = EventTable | ||
queryset = Event.objects.all() | ||
filterset_class = EventFilter | ||
paginate_by = 20 | ||
|
||
def get_template_names(self): | ||
if self.request.htmx: | ||
template_name = "events/event_table_partial.html" | ||
else: | ||
template_name = "events/event_table_htmx.html" | ||
|
||
return template_name | ||
|
||
def get_queryset(self): | ||
return Event.objects.filter(opportunity__in=Opportunity.objects.filter(organization=self.request.org)) |
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 will add again that I think a metadata field will be very beneficial. Even with the existing set of events we have, data like which record was approved, or how much the payment was could be very useful, and I know that mobile was also hoping to include additional metadata.