Repository navigation
Add opt-in threads and enterdate/leavedate to the per-commitfest patches endpoint - #115
Merged
Merged
Conversation
Threads for a whole commitfest in one prefetch query instead of a request per patch. enterdate/leavedate were already stored, just not exposed.
kjrocker
marked this pull request as draft
September 2, 2026 14:25
Helpers above the tests, create_thread called with keyword arguments, comments trimmed to the terser house style.
kjrocker
force-pushed
the
api-patches-include-threads
branch
from
September 2, 2026 14:44
5b184fa to
874c6a5
Compare
kjrocker
marked this pull request as ready for review
September 2, 2026 14:48
JelteF
approved these changes
Oct 18, 2026
The four separate tests for `?include=threads` each rebuilt the same patch/thread fixtures to check one property. They now share the setup of `test_commitfest_patches_endpoint`, which already has two patches on a commitfest: one gets a thread with an attachment, so the same test checks that threads are opt-in, that unknown include tokens are ignored, the inlined thread payload, the empty list for a patch without threads, and that including threads costs a single extra query rather than one per patch. The remaining naive `datetime.now()` in the file is replaced with a fixed aware datetime so the pre-commit ruff check passes.
`?include=bogus, threads` used to silently return no threads, because the token was compared as " threads". Strip whitespace from each token so a space after the comma works as people would expect.
This matches how `CommitFest` already provides its own API representation through `to_json`.
The per-commitfest patches endpoint built each patch entry inline. Like `CommitFest` and `MailThread`, the models now provide their own API representation: `Patch.to_json` has the patch fields, and `PatchOnCommitFest.to_json` adds the commitfest-specific status and enter/leave dates on top.
`MailThread.to_json` depends on the `has_attachment` annotation, so the helper that adds it belongs next to the model rather than in the API module. Now it's available as `MailThread.objects.with_has_attachment()` and on related managers like `patch.mailthread_set.with_has_attachment()`.
`to_json` raised an `AttributeError` for threads that weren't fetched through `with_has_attachment()`. Now `has_attachment` is a property that uses the annotation when it's there and otherwise checks the attachments with a query, so the annotation is only a performance optimization.
This reverts commit f6caf59.
Keeps the main patches endpoint test focused on the response contents, and makes it clear what the include parsing is expected to do.
`api_response` already calls `to_json` on objects that have it, so there's no need to do that explicitly for the threads.
The JSON serializer now turns querysets into lists itself, so views can put a queryset in the response without wrapping it in `list()`.
JelteF
approved these changes
Oct 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Getting the threads for all patches in a commitfest required one
GET /api/v1/patches/<id>/threadsrequest per patch, which makes fetching the history thousands of requests. With?include=threads,GET /api/v1/commitfests/<cfid>/patchesnow includes athreadslist for every patch, in the same shape as the per-patch endpoint, at the cost of a single extra query.The endpoint now also always returns
enterdateandleavedate. Likedraftfrom #114, these were already stored and shown in the UI, just not exposed in the API.