diff --git a/pgcommitfest/commitfest/apiv1.py b/pgcommitfest/commitfest/apiv1.py index a0efea18..9d6109ad 100644 --- a/pgcommitfest/commitfest/apiv1.py +++ b/pgcommitfest/commitfest/apiv1.py @@ -1,3 +1,4 @@ +from django.db.models import Prefetch, QuerySet from django.http import ( HttpResponse, ) @@ -8,6 +9,7 @@ from .models import ( CommitFest, + MailThread, Patch, PatchOnCommitFest, ) @@ -24,6 +26,9 @@ def datetime_serializer(obj): if hasattr(obj, "to_json"): return obj.to_json() + if isinstance(obj, QuerySet): + return list(obj) + raise TypeError(f"Type {type(obj)} not serializable to JSON") @@ -38,7 +43,7 @@ def api_response(payload, status=200, content_type="application/json"): def all_commitfests(request): """Return all commitfests, including closed ones.""" - return api_response({"commitfests": list(CommitFest.objects.order_by("id"))}) + return api_response({"commitfests": CommitFest.objects.order_by("id")}) def commitfestst_that_need_ci(request): @@ -60,9 +65,15 @@ def commitfest_patches(request, cfid): This endpoint provides the data that cfbot previously scraped from the commitfest HTML page. + + Threads for every patch can be included with ?include=threads. Unknown + tokens are ignored. """ cf = get_object_or_404(CommitFest, pk=cfid) + include = {i.strip() for i in request.GET.get("include", "").split(",")} + include_threads = "threads" in include + pocs = ( PatchOnCommitFest.objects.filter(commitfest=cf) .select_related("patch") @@ -70,19 +81,20 @@ def commitfest_patches(request, cfid): .order_by("patch__id") ) + if include_threads: + pocs = pocs.prefetch_related( + Prefetch( + "patch__mailthread_set", + queryset=MailThread.objects.with_has_attachment(), + ) + ) + patches = [] for poc in pocs: - patch = poc.patch - authors = [f"{a.first_name} {a.last_name}" for a in patch.authors.all()] - patches.append( - { - "id": patch.id, - "name": patch.name, - "status": poc.statusstring, - "authors": authors, - "last_email_time": patch.lastmail, - } - ) + entry = poc.to_json() + if include_threads: + entry["threads"] = poc.patch.mailthread_set.all() + patches.append(entry) return api_response( { @@ -100,18 +112,7 @@ def patch_threads(request, patch_id): """ patch = get_object_or_404(Patch, pk=patch_id) - threads = [] - for thread in patch.mailthread_set.all(): - latest_attachment = thread.mailthreadattachment_set.first() - threads.append( - { - "messageid": thread.messageid, - "subject": thread.subject, - "latest_message_id": thread.latestmsgid, - "latest_message_time": thread.latestmessage, - "has_attachment": latest_attachment is not None, - } - ) + threads = patch.mailthread_set.with_has_attachment() return api_response( { diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index 2d21a1ec..0f7da9e3 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -1,7 +1,7 @@ from django.conf import settings from django.contrib.auth.models import User from django.db import models, transaction -from django.db.models import Q +from django.db.models import Exists, OuterRef, Q from django.shortcuts import get_object_or_404 from datetime import datetime, timedelta, timezone @@ -637,6 +637,14 @@ def move( return new_poc + def to_json(self): + return { + "id": self.id, + "name": self.name, + "authors": [f"{a.first_name} {a.last_name}" for a in self.authors.all()], + "last_email_time": self.lastmail, + } + def __str__(self): return self.name @@ -714,6 +722,14 @@ def needs_committer(self): def statusstring(self): return [v for k, v in self._STATUS_CHOICES if k == self.status][0] + def to_json(self): + return { + **self.patch.to_json(), + "status": self.statusstring, + "enterdate": self.enterdate, + "leavedate": self.leavedate, + } + @classmethod def current_for_patch(cls, patch_id): return get_object_or_404( @@ -833,6 +849,18 @@ def save_and_notify( PendingNotification(history=self, user=u).save() +class MailThreadQuerySet(models.QuerySet): + def with_has_attachment(self): + # An annotation rather than checking each thread's attachments + # separately keeps the number of queries constant when listing many + # threads. + return self.annotate( + has_attachment=Exists( + MailThreadAttachment.objects.filter(mailthread=OuterRef("pk")) + ) + ) + + class MailThread(models.Model): # This class tracks mail threads from the main postgresql.org # mailinglist archives. For each thread, we store *one* messageid. @@ -852,9 +880,22 @@ class MailThread(models.Model): latestsubject = models.CharField(max_length=500, null=False, blank=False) latestmsgid = models.CharField(max_length=1000, null=False, blank=False) + objects = MailThreadQuerySet.as_manager() + def __str__(self): return self.subject + def to_json(self): + # Only works on threads fetched with with_has_attachment(), since + # has_attachment is an annotation, not a field. + return { + "messageid": self.messageid, + "subject": self.subject, + "latest_message_id": self.latestmsgid, + "latest_message_time": self.latestmessage, + "has_attachment": self.has_attachment, + } + class Meta: ordering = ("firstmessage",) diff --git a/pgcommitfest/commitfest/tests/test_apiv1.py b/pgcommitfest/commitfest/tests/test_apiv1.py index 5454da8a..539f29e1 100644 --- a/pgcommitfest/commitfest/tests/test_apiv1.py +++ b/pgcommitfest/commitfest/tests/test_apiv1.py @@ -1,3 +1,6 @@ +from django.db import connection +from django.test.utils import CaptureQueriesContext + import json from datetime import datetime, timezone @@ -5,6 +8,7 @@ from pgcommitfest.commitfest.models import ( MailThread, + MailThreadAttachment, Patch, PatchOnCommitFest, ) @@ -136,17 +140,42 @@ def test_commitfest_patches_endpoint(client, open_cf, alice, bob): PatchOnCommitFest.objects.create( patch=patch1, commitfest=open_cf, - enterdate=datetime.now(), + enterdate=datetime(2025, 1, 1, 12, 0, 0, tzinfo=timezone.utc), status=PatchOnCommitFest.STATUS_REVIEW, ) PatchOnCommitFest.objects.create( patch=patch2, commitfest=open_cf, - enterdate=datetime.now(), - status=PatchOnCommitFest.STATUS_AUTHOR, + enterdate=datetime(2025, 1, 2, 8, 0, 0, tzinfo=timezone.utc), + # leavedate is only allowed on statuses that close the patch out + leavedate=datetime(2025, 1, 20, 16, 45, 0, tzinfo=timezone.utc), + status=PatchOnCommitFest.STATUS_COMMITTED, ) - response = client.get(f"/api/v1/commitfests/{open_cf.id}/patches") + thread = MailThread.objects.create( + messageid="abc123@example.com", + subject="[PATCH] Add feature X", + firstmessage=datetime(2025, 1, 10, 9, 0, 0, tzinfo=timezone.utc), + firstauthor="alice@example.com", + latestmessage=datetime(2025, 1, 12, 14, 30, 0, tzinfo=timezone.utc), + latestauthor="bob@example.com", + latestsubject="Re: [PATCH] Add feature X", + latestmsgid="def456@example.com", + ) + patch1.mailthread_set.add(thread) + MailThreadAttachment.objects.create( + mailthread=thread, + messageid="abc123@example.com", + attachmentid=1, + filename="v1.patch", + date=datetime(2025, 1, 10, 9, 0, 0, tzinfo=timezone.utc), + author="alice@example.com", + ispatch=True, + ) + + url = f"/api/v1/commitfests/{open_cf.id}/patches" + with CaptureQueriesContext(connection) as queries_without_threads: + response = client.get(url) assert response.status_code == 200 assert response["Content-Type"] == "application/json" @@ -164,13 +193,55 @@ def test_commitfest_patches_endpoint(client, open_cf, alice, bob): assert p1["status"] == "Needs review" assert p1["authors"] == ["Alice Anderson"] assert p1["last_email_time"] == "2025-01-15T10:30:00+00:00" + assert p1["enterdate"] == "2025-01-01T12:00:00+00:00" + assert p1["leavedate"] is None p2 = data["patches"][1] assert p2["id"] == patch2.id assert p2["name"] == "Fix bug Y" - assert p2["status"] == "Waiting on Author" + assert p2["status"] == "Committed" assert sorted(p2["authors"]) == ["Alice Anderson", "Bob Brown"] assert p2["last_email_time"] is None + assert p2["enterdate"] == "2025-01-02T08:00:00+00:00" + assert p2["leavedate"] == "2025-01-20T16:45:00+00:00" + + # Threads are opt-in + assert "threads" not in p1 + + with CaptureQueriesContext(connection) as queries_with_threads: + response = client.get(f"{url}?include=threads") + + assert response.status_code == 200 + p1, p2 = json.loads(response.content)["patches"] + assert p1["threads"] == [ + { + "messageid": "abc123@example.com", + "subject": "[PATCH] Add feature X", + "latest_message_id": "def456@example.com", + "latest_message_time": "2025-01-12T14:30:00+00:00", + "has_attachment": True, + } + ] + assert p2["threads"] == [] + # A single prefetch query for all patches, not one per patch + assert len(queries_with_threads) == len(queries_without_threads) + 1 + + +def test_commitfest_patches_endpoint_include_parsing(client, open_cf): + """Unknown include tokens are ignored and spaces around tokens allowed.""" + PatchOnCommitFest.objects.create( + patch=Patch.objects.create(name="Add feature X"), + commitfest=open_cf, + enterdate=datetime(2025, 1, 1, 12, 0, 0, tzinfo=timezone.utc), + ) + url = f"/api/v1/commitfests/{open_cf.id}/patches" + + response = client.get(f"{url}?include=bogus") + assert response.status_code == 200 + assert "threads" not in json.loads(response.content)["patches"][0] + + response = client.get(f"{url}?include=bogus, threads") + assert json.loads(response.content)["patches"][0]["threads"] == [] def test_commitfest_patches_endpoint_not_found(client, commitfests): @@ -187,7 +258,7 @@ def test_patch_threads_endpoint(client, open_cf, alice): PatchOnCommitFest.objects.create( patch=patch, commitfest=open_cf, - enterdate=datetime.now(), + enterdate=datetime(2025, 1, 1, tzinfo=timezone.utc), status=PatchOnCommitFest.STATUS_REVIEW, )