Add opt-in threads and enterdate/leavedate to the per-commitfest patches endpoint

pgcommitfest/commitfest/apiv1.py

@@ -1,3 +1,4 @@
+from django.db.models import Exists, OuterRef, Prefetch
 from django.http import (
     HttpResponse,
 )
@@ -8,6 +9,8 @@
 
 from .models import (
     CommitFest,
+    MailThread,
+    MailThreadAttachment,
     Patch,
     PatchOnCommitFest,
 )
@@ -55,34 +58,74 @@ def commitfestst_that_need_ci(request):
     return api_response({"commitfests": cfs})
 
 
+def with_has_attachment(threads):
+    """Annotate a MailThread queryset with has_attachment.
+
+    Doing this as an annotation rather than checking each thread's attachments
+    separately keeps the number of queries constant.
+    """
+    return threads.annotate(
+        has_attachment=Exists(
+            MailThreadAttachment.objects.filter(mailthread=OuterRef("pk"))
+        )
+    )
+
+
+def mailthread_json(thread):
+    return {
+        "messageid": thread.messageid,
+        "subject": thread.subject,
+        "latest_message_id": thread.latestmsgid,
+        "latest_message_time": thread.latestmessage,
+        "has_attachment": thread.has_attachment,
+    }
+
+
 def commitfest_patches(request, cfid):
     """Return all patches for a commitfest.
 
     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 = set(request.GET.get("include", "").split(","))
+    include_threads = "threads" in include
+
     pocs = (
         PatchOnCommitFest.objects.filter(commitfest=cf)
         .select_related("patch")
         .prefetch_related("patch__authors")
         .order_by("patch__id")
     )
 
+    if include_threads:
+        pocs = pocs.prefetch_related(
+            Prefetch(
+                "patch__mailthread_set",
+                queryset=with_has_attachment(MailThread.objects.all()),
+            )
+        )
+
     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 = {
+            "id": patch.id,
+            "name": patch.name,
+            "status": poc.statusstring,
+            "authors": authors,
+            "last_email_time": patch.lastmail,
+            "enterdate": poc.enterdate,
+            "leavedate": poc.leavedate,
+        }
+        if include_threads:
+            entry["threads"] = [mailthread_json(t) for t in patch.mailthread_set.all()]
+        patches.append(entry)
 
     return api_response(
         {
@@ -100,18 +143,9 @@ 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 = [
+        mailthread_json(t) for t in with_has_attachment(patch.mailthread_set.all())
+    ]
 
     return api_response(
         {

pgcommitfest/commitfest/tests/test_apiv1.py

@@ -1,17 +1,48 @@
+from django.db import connection
+from django.test.utils import CaptureQueriesContext
+
 import json
 from datetime import datetime, timezone
 
 import pytest
 
 from pgcommitfest.commitfest.models import (
     MailThread,
+    MailThreadAttachment,
     Patch,
     PatchOnCommitFest,
 )
 
 pytestmark = pytest.mark.django_db
 
 
+def create_patch_on_cf(commitfest, name, author):
+    """Create a patch and put it on a commitfest."""
+    patch = Patch.objects.create(name=name)
+    patch.authors.add(author)
+    PatchOnCommitFest.objects.create(
+        patch=patch,
+        commitfest=commitfest,
+        enterdate=datetime(2025, 1, 1, tzinfo=timezone.utc),
+        status=PatchOnCommitFest.STATUS_REVIEW,
+    )
+    return patch
+
+
+def create_thread(messageid, subject, firstmessage, latestmessage, latestmsgid):
+    """Create a mail thread."""
+    return MailThread.objects.create(
+        messageid=messageid,
+        subject=subject,
+        firstmessage=firstmessage,
+        firstauthor="alice@example.com",
+        latestmessage=latestmessage,
+        latestauthor="bob@example.com",
+        latestsubject=f"Re: {subject}",
+        latestmsgid=latestmsgid,
+    )
+
+
 def test_commitfests_endpoint(client, commitfests):
     """Test the /api/v1/commitfests endpoint returns all commitfests."""
     response = client.get("/api/v1/commitfests")
@@ -136,14 +167,16 @@ 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")
@@ -164,13 +197,17 @@ 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"
 
 
 def test_commitfest_patches_endpoint_not_found(client, commitfests):
@@ -227,3 +264,148 @@ def test_patch_threads_endpoint_not_found(client, commitfests):
     """Test the threads endpoint returns 404 for non-existent patch."""
     response = client.get("/api/v1/patches/99999/threads")
     assert response.status_code == 404
+
+
+def test_commitfest_patches_include_threads(client, open_cf, alice):
+    """Test ?include=threads inlines the same thread data as the patch endpoint."""
+    patch1 = create_patch_on_cf(open_cf, "Add feature X", alice)
+    patch2 = create_patch_on_cf(open_cf, "Fix bug Y", alice)
+
+    # Created out of order; they come back oldest-first
+    thread2 = create_thread(
+        messageid="second@example.com",
+        subject="[PATCH] Add feature X v2",
+        firstmessage=datetime(2025, 1, 10, 9, 0, 0, tzinfo=timezone.utc),
+        latestmessage=datetime(2025, 1, 12, 14, 30, 0, tzinfo=timezone.utc),
+        latestmsgid="second-latest@example.com",
+    )
+    thread1 = create_thread(
+        messageid="first@example.com",
+        subject="[PATCH] Add feature X v1",
+        firstmessage=datetime(2025, 1, 5, 9, 0, 0, tzinfo=timezone.utc),
+        latestmessage=datetime(2025, 1, 6, 11, 0, 0, tzinfo=timezone.utc),
+        latestmsgid="first-latest@example.com",
+    )
+    patch1.mailthread_set.add(thread1, thread2)
+
+    MailThreadAttachment.objects.create(
+        mailthread=thread2,
+        messageid="second@example.com",
+        attachmentid=1,
+        filename="v2.patch",
+        date=datetime(2025, 1, 10, 9, 0, 0, tzinfo=timezone.utc),
+        author="alice@example.com",
+        ispatch=True,
+    )
+
+    response = client.get(f"/api/v1/commitfests/{open_cf.id}/patches?include=threads")
+
+    assert response.status_code == 200
+
+    data = json.loads(response.content)
+
+    p1, p2 = data["patches"]
+    assert p1["id"] == patch1.id
+    assert p1["threads"] == [
+        {
+            "messageid": "first@example.com",
+            "subject": "[PATCH] Add feature X v1",
+            "latest_message_id": "first-latest@example.com",
+            "latest_message_time": "2025-01-06T11:00:00+00:00",
+            "has_attachment": False,
+        },
+        {
+            "messageid": "second@example.com",
+            "subject": "[PATCH] Add feature X v2",
+            "latest_message_id": "second-latest@example.com",
+            "latest_message_time": "2025-01-12T14:30:00+00:00",
+            "has_attachment": True,
+        },
+    ]
+
+    # A patch without threads still gets the key
+    assert p2["id"] == patch2.id
+    assert p2["threads"] == []
+
+
+def test_commitfest_patches_no_threads_by_default(client, open_cf, alice):
+    """Test threads are only included when asked for."""
+    patch = create_patch_on_cf(open_cf, "Add feature X", alice)
+    patch.mailthread_set.add(
+        create_thread(
+            messageid="abc123@example.com",
+            subject="[PATCH] Add feature X",
+            firstmessage=datetime(2025, 1, 5, 9, 0, 0, tzinfo=timezone.utc),
+            latestmessage=datetime(2025, 1, 6, 11, 0, 0, tzinfo=timezone.utc),
+            latestmsgid="def456@example.com",
+        )
+    )
+
+    for url in (
+        f"/api/v1/commitfests/{open_cf.id}/patches",
+        f"/api/v1/commitfests/{open_cf.id}/patches?include=bogus",
+    ):
+        response = client.get(url)
+
+        assert response.status_code == 200
+
+        data = json.loads(response.content)
+
+        assert "threads" not in data["patches"][0]
+
+
+def test_commitfest_patches_include_unknown_token(client, open_cf, alice):
+    """Test unknown include tokens are ignored, rather than rejecting the request."""
+    patch = create_patch_on_cf(open_cf, "Add feature X", alice)
+    patch.mailthread_set.add(
+        create_thread(
+            messageid="abc123@example.com",
+            subject="[PATCH] Add feature X",
+            firstmessage=datetime(2025, 1, 5, 9, 0, 0, tzinfo=timezone.utc),
+            latestmessage=datetime(2025, 1, 6, 11, 0, 0, tzinfo=timezone.utc),
+            latestmsgid="def456@example.com",
+        )
+    )
+
+    response = client.get(
+        f"/api/v1/commitfests/{open_cf.id}/patches?include=bogus,threads"
+    )
+
+    assert response.status_code == 200
+
+    data = json.loads(response.content)
+
+    assert len(data["patches"][0]["threads"]) == 1
+
+
+def test_commitfest_patches_include_threads_query_count(client, open_cf, alice):
+    """Test including threads costs a fixed number of queries, not one per patch."""
+    for i in range(5):
+        patch = create_patch_on_cf(open_cf, f"Patch {i}", alice)
+        patch.mailthread_set.add(
+            create_thread(
+                messageid=f"thread{i}@example.com",
+                subject=f"[PATCH] Patch {i}",
+                firstmessage=datetime(2025, 1, 5, 9, 0, 0, tzinfo=timezone.utc),
+                latestmessage=datetime(2025, 1, 6, 11, 0, 0, tzinfo=timezone.utc),
+                latestmsgid=f"thread{i}-latest@example.com",
+            )
+        )
+
+    url = f"/api/v1/commitfests/{open_cf.id}/patches"
+
+    with CaptureQueriesContext(connection) as without_threads:
+        client.get(url)
+
+    with CaptureQueriesContext(connection) as with_threads:
+        response = client.get(f"{url}?include=threads")
+
+    assert response.status_code == 200
+
+    data = json.loads(response.content)
+
+    assert len(data["patches"]) == 5
+    assert all(len(p["threads"]) == 1 for p in data["patches"])
+
+    # One extra query for all patches, not one per patch
+    assert len(with_threads) == len(without_threads) + 1