diff --git a/pgcommitfest/commitfest/apiv1.py b/pgcommitfest/commitfest/apiv1.py index a0efea18..ffd00a33 100644 --- a/pgcommitfest/commitfest/apiv1.py +++ b/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,14 +58,43 @@ 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") @@ -70,19 +102,30 @@ def commitfest_patches(request, cfid): .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( { diff --git a/pgcommitfest/commitfest/tests/test_apiv1.py b/pgcommitfest/commitfest/tests/test_apiv1.py index 5454da8a..8d53c1bb 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, ) @@ -12,6 +16,33 @@ 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