From 9696c8e7ac3bafeb526ab5cedbf8d221b0e058dd Mon Sep 17 00:00:00 2001 From: Jonas Rocha Date: Sat, 17 Oct 2026 10:33:48 -0300 Subject: [PATCH 1/2] Allow sorting patches by committer On the page of a commitfest, the Committer column header can now be clicked to sort the patch list by committer, just like the other sortable columns. Combined with the Committed status filter, this makes it easy to see who committed what in a commitfest. Committers are sorted by last name, then first name and username, which is the same order that the Committer model uses. Patches without a committer are always shown last, and the patch ID is used as a tiebreaker. Like with the other sort orders, open patches are still shown before closed ones. The dashboard is left unchanged, since this was only requested for the commitfest page. Fixes #112 --- .../commitfest/templates/commitfest.html | 2 +- .../commitfest/tests/test_committer_sort.py | 189 ++++++++++++++++++ pgcommitfest/commitfest/views.py | 10 + 3 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 pgcommitfest/commitfest/tests/test_committer_sort.py diff --git a/pgcommitfest/commitfest/templates/commitfest.html b/pgcommitfest/commitfest/templates/commitfest.html index 2e71784b..2549d2d5 100644 --- a/pgcommitfest/commitfest/templates/commitfest.html +++ b/pgcommitfest/commitfest/templates/commitfest.html @@ -41,7 +41,7 @@

{{p.is_open|yesno:"Active patches,Closed patches"}}

Stats{%if sortkey == 6%}
{%elif sortkey == -6%}
{%endif%} Author Reviewers - Committer + Committer{%if sortkey == 9%}
{%elif sortkey == -9%}
{%endif%} Num cfs{%if sortkey == 3%}
{%elif sortkey == -3%}
{%endif%} Latest mail{%if sortkey == 2%}
{%elif sortkey == -2%}
{%endif%} {%if user.is_staff%} diff --git a/pgcommitfest/commitfest/tests/test_committer_sort.py b/pgcommitfest/commitfest/tests/test_committer_sort.py new file mode 100644 index 00000000..0b5154ba --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_committer_sort.py @@ -0,0 +1,189 @@ +from django.contrib.auth.models import User + +from datetime import datetime + +import pytest + +from pgcommitfest.commitfest.models import Committer, Patch, PatchOnCommitFest + +# The commitfest views start with SET TRANSACTION ISOLATION LEVEL, which must +# be the first statement in a transaction, so the test can't wrap them in one. +pytestmark = pytest.mark.django_db(transaction=True, serialized_rollback=True) + + +def make_committer(username, first_name, last_name): + user = User.objects.create_user( + username=username, first_name=first_name, last_name=last_name + ) + return Committer.objects.create(user=user) + + +def make_patch(cf, name, committer=None, status=PatchOnCommitFest.STATUS_REVIEW): + patch = Patch.objects.create(name=name, committer=committer) + is_open = status in PatchOnCommitFest.OPEN_STATUSES + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=cf, + enterdate=datetime.now(), + leavedate=None if is_open else datetime.now(), + status=status, + ) + return patch + + +def patch_names(client, cf, **params): + response = client.get(f"/{cf.id}/", params) + assert response.status_code == 200 + return [p["name"] for p in response.context["patches"]] + + +@pytest.fixture +def cf(commitfests): + # Rendering pages needs the full set of relevant commitfests to exist. + return commitfests["open"] + + +@pytest.fixture +def committers(): + # Surnames are chosen so that sorting by first name would give the + # reverse of sorting by surname. + return { + "adams": make_committer("zadams", "Zed", "Adams"), + "moore": make_committer("mmoore", "Mia", "Moore"), + "young": make_committer("ayoung", "Amy", "Young"), + } + + +@pytest.fixture +def sort_patches(cf, committers): + # Created in an order that matches neither sort direction. + make_patch(cf, "moore", committers["moore"]) + make_patch(cf, "none", None) + make_patch(cf, "young", committers["young"]) + make_patch(cf, "adams", committers["adams"]) + + +def test_sort_by_committer_ascending(client, cf, sort_patches): + assert patch_names(client, cf, sortkey=9) == [ + "adams", + "moore", + "young", + "none", + ] + + +def test_sort_by_committer_descending(client, cf, sort_patches): + assert patch_names(client, cf, sortkey=-9) == [ + "young", + "moore", + "adams", + "none", + ] + + +def test_sort_by_committer_tiebreakers(client, cf): + bea = make_committer("bsmith", "Bea", "Smith") + ann2 = make_committer("asmith2", "Ann", "Smith") + ann1 = make_committer("asmith1", "Ann", "Smith") + + make_patch(cf, "bea", bea) + first = make_patch(cf, "ann2-a", ann2) + second = make_patch(cf, "ann2-b", ann2) + make_patch(cf, "ann1", ann1) + assert first.id < second.id + + assert patch_names(client, cf, sortkey=9) == [ + "ann1", + "ann2-a", + "ann2-b", + "bea", + ] + # The names are reversed, but the patch id stays ascending as a tiebreaker + assert patch_names(client, cf, sortkey=-9) == [ + "bea", + "ann2-a", + "ann2-b", + "ann1", + ] + + +def test_sort_by_committer_keeps_open_patches_first(client, cf, committers): + make_patch( + cf, + "adams-committed", + committers["adams"], + status=PatchOnCommitFest.STATUS_COMMITTED, + ) + make_patch(cf, "young-open", committers["young"]) + make_patch(cf, "none-open", None) + make_patch( + cf, + "moore-committed", + committers["moore"], + status=PatchOnCommitFest.STATUS_COMMITTED, + ) + + assert patch_names(client, cf, sortkey=9) == [ + "young-open", + "none-open", + "adams-committed", + "moore-committed", + ] + assert patch_names(client, cf, sortkey=-9) == [ + "young-open", + "none-open", + "moore-committed", + "adams-committed", + ] + + +def test_sort_by_committer_with_committed_status_filter(client, cf, committers): + for name in ("young", "adams", "moore"): + make_patch( + cf, name, committers[name], status=PatchOnCommitFest.STATUS_COMMITTED + ) + make_patch(cf, "open", committers["adams"]) + + assert patch_names( + client, cf, sortkey=9, status=PatchOnCommitFest.STATUS_COMMITTED + ) == ["adams", "moore", "young"] + + +def test_default_order_is_unchanged(client, cf, sort_patches): + assert patch_names(client, cf) == ["moore", "none", "young", "adams"] + + +@pytest.mark.parametrize("sortkey", ["0", "99", "abc"]) +def test_unknown_sortkey_redirects_to_default(client, cf, sortkey): + response = client.get(f"/{cf.id}/", {"sortkey": sortkey}) + assert response.status_code == 302 + assert response["Location"] == f"/{cf.id}/" + + +def test_committer_header_is_sortable(client, cf, sort_patches): + response = client.get(f"/{cf.id}/") + assert b'onclick="return sortpatches(9);">Committer' in response.content + + response = client.get(f"/{cf.id}/", {"sortkey": 9}) + assert response.context["sortkey"] == 9 + assert b"bi-sort-alpha-down" in response.content + + response = client.get(f"/{cf.id}/", {"sortkey": -9}) + assert response.context["sortkey"] == -9 + assert b"bi-sort-alpha-up" in response.content + + +def test_dashboard_does_not_sort_by_committer(client, cf, alice, committers): + patch = make_patch(cf, "mine", committers["adams"]) + patch.authors.add(alice) + client.force_login(alice) + + # Without other filters, an unsupported sortkey redirects to the plain page + response = client.get("/", {"sortkey": 9}) + assert response.status_code == 302 + + response = client.get( + "/", {"sortkey": 9, "status": PatchOnCommitFest.STATUS_REVIEW} + ) + assert response.status_code == 200 + assert response.context["sortkey"] == 0 diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 34553217..25a953b7 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -507,6 +507,16 @@ def patchlist(request, cf, personalized=False): orderby_str = "poc.commitfest_id, lastmail DESC" elif sortkey == -8: orderby_str = "poc.commitfest_id DESC, lastmail" + elif sortkey == 9 and not personalized: + orderby_str = ( + "committer.last_name NULLS LAST, committer.first_name NULLS LAST, " + "committer.username NULLS LAST, p.id" + ) + elif sortkey == -9 and not personalized: + orderby_str = ( + "committer.last_name DESC NULLS LAST, committer.first_name DESC NULLS LAST, " + "committer.username DESC NULLS LAST, p.id" + ) else: if personalized: # First we sort by group_name, to have the grouping work. From 59331294482192f5b59bc1320d8575a7ceafa5dd Mon Sep 17 00:00:00 2001 From: Jonas Rocha Date: Sat, 17 Oct 2026 10:37:34 -0300 Subject: [PATCH 2/2] Allow filtering patches by committer This adds a Committer filter to the page of a commitfest. It lists the committers of the patches in that commitfest, including committers that are no longer active. Since that list is short, it's a plain dropdown instead of a searchable one. It's sorted by first name, like the other user dropdowns, because that's how the names are shown. The filter uses the current committer of a patch. Values that are not a number, or that are not one of the committers in the dropdown, are ignored. The dashboard and global search use the same filter form, but they don't get this filter: they don't show it and ignore the committer parameter. --- pgcommitfest/commitfest/forms.py | 34 ++- .../commitfest/tests/test_committer_filter.py | 246 ++++++++++++++++++ pgcommitfest/commitfest/views.py | 25 +- 3 files changed, 300 insertions(+), 5 deletions(-) create mode 100644 pgcommitfest/commitfest/tests/test_committer_filter.py diff --git a/pgcommitfest/commitfest/forms.py b/pgcommitfest/commitfest/forms.py index a01b437e..25fa9f9a 100644 --- a/pgcommitfest/commitfest/forms.py +++ b/pgcommitfest/commitfest/forms.py @@ -5,7 +5,14 @@ from django.http import Http404 from .ajax import _archivesAPI -from .models import MailThread, Patch, PatchOnCommitFest, Tag, TargetVersion +from .models import ( + Committer, + MailThread, + Patch, + PatchOnCommitFest, + Tag, + TargetVersion, +) from .widgets import ThreadPickWidget @@ -22,9 +29,12 @@ class CommitFestFilterForm(forms.Form): tag = forms.MultipleChoiceField(required=False, label="Tag (type to search)") author = forms.ChoiceField(required=False, label="Author (type to search)") reviewer = forms.ChoiceField(required=False, label="Reviewer (type to search)") + committer = forms.ChoiceField(required=False, label="Committer") sortkey = forms.IntegerField(required=False) - def __init__(self, data, commitfest=None, *args, **kwargs): + def __init__( + self, data, commitfest=None, *args, include_committer_filter=False, **kwargs + ): super(CommitFestFilterForm, self).__init__(data, *args, **kwargs) self.commitfest = commitfest @@ -79,6 +89,26 @@ def __init__(self, data, commitfest=None, *args, **kwargs): ): self.fields[f].widget.attrs = {"class": "input-medium"} + # The committer filter is only supported on the page of a single + # commitfest, so other pages that share this form don't get it. The + # list of committers in a commitfest is short, so a plain dropdown is + # fine. It's sorted by first name, to match how the names are shown. + if include_committer_filter: + committers = ( + Committer.objects.filter( + patch__patchoncommitfest__commitfest=commitfest + ) + .select_related("user") + .distinct() + .order_by("user__first_name", "user__last_name", "user__username") + ) + self.fields["committer"].choices = [(-1, "* All")] + [ + (c.user_id, c.fullname) for c in committers + ] + self.fields["committer"].widget.attrs = {"class": "input-medium"} + else: + del self.fields["committer"] + # Tags that get their own checkbox buttons as shortcuts (also appear in selectize dropdown) CHECKBOX_TAG_NAMES = ["Bugfix", "Security", "Performance"] diff --git a/pgcommitfest/commitfest/tests/test_committer_filter.py b/pgcommitfest/commitfest/tests/test_committer_filter.py new file mode 100644 index 00000000..906d5d09 --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_committer_filter.py @@ -0,0 +1,246 @@ +from django.contrib.auth.models import User + +from datetime import datetime + +import pytest + +from pgcommitfest.commitfest.models import Committer, Patch, PatchOnCommitFest, Tag + +# The commitfest views start with SET TRANSACTION ISOLATION LEVEL, which must +# be the first statement in a transaction, so the test can't wrap them in one. +pytestmark = pytest.mark.django_db(transaction=True, serialized_rollback=True) + + +def make_committer(username, first_name, last_name, active=True): + user = User.objects.create_user( + username=username, first_name=first_name, last_name=last_name + ) + return Committer.objects.create(user=user, active=active) + + +def make_patch(cf, name, committer=None, status=PatchOnCommitFest.STATUS_REVIEW): + patch = Patch.objects.create(name=name, committer=committer) + is_open = status in PatchOnCommitFest.OPEN_STATUSES + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=cf, + enterdate=datetime.now(), + leavedate=None if is_open else datetime.now(), + status=status, + ) + return patch + + +def get_patch_names(client, cf, **params): + response = client.get(f"/{cf.id}/", params) + assert response.status_code == 200 + return [p["name"] for p in response.context["patches"]] + + +@pytest.fixture +def cf(commitfests): + # Rendering pages needs the full set of relevant commitfests to exist. + return commitfests["open"] + + +@pytest.fixture +def committers(): + return { + "adams": make_committer("zadams", "Zed", "Adams"), + "young": make_committer("ayoung", "Amy", "Young"), + } + + +@pytest.fixture +def filter_patches(cf, committers): + make_patch(cf, "adams-open", committers["adams"]) + make_patch( + cf, + "adams-committed", + committers["adams"], + status=PatchOnCommitFest.STATUS_COMMITTED, + ) + make_patch( + cf, + "young-committed", + committers["young"], + status=PatchOnCommitFest.STATUS_COMMITTED, + ) + make_patch(cf, "none-open", None) + + +def test_filter_by_committer(client, cf, committers, filter_patches): + adams = committers["adams"].user_id + assert get_patch_names(client, cf, committer=adams) == [ + "adams-open", + "adams-committed", + ] + + +def test_filter_by_committer_and_committed_status( + client, cf, committers, filter_patches +): + adams = committers["adams"].user_id + assert get_patch_names( + client, cf, committer=adams, status=PatchOnCommitFest.STATUS_COMMITTED + ) == ["adams-committed"] + + +def test_filter_by_committer_combined_with_sort(client, cf, committers): + adams = committers["adams"] + other = make_committer("bsmith", "Bea", "Smith") + make_patch(cf, "adams-1", adams, status=PatchOnCommitFest.STATUS_COMMITTED) + make_patch(cf, "other", other, status=PatchOnCommitFest.STATUS_COMMITTED) + make_patch(cf, "adams-2", adams, status=PatchOnCommitFest.STATUS_COMMITTED) + + response = client.get( + f"/{cf.id}/", + { + "committer": adams.user_id, + "status": PatchOnCommitFest.STATUS_COMMITTED, + "sortkey": -9, + }, + ) + assert response.status_code == 200 + assert response.context["sortkey"] == -9 + assert [p["name"] for p in response.context["patches"]] == [ + "adams-1", + "adams-2", + ] + + +def test_filter_all_committers(client, cf, filter_patches): + # "* All" with no other filters is the same as the plain page + response = client.get(f"/{cf.id}/", {"committer": "-1"}) + assert response.status_code == 302 + assert response["Location"] == f"/{cf.id}/" + + assert get_patch_names(client, cf, committer="-1", sortkey=9) == [ + "adams-open", + "none-open", + "adams-committed", + "young-committed", + ] + + +def test_filter_combines_with_author_and_tag(client, cf, committers, alice): + tag = Tag.objects.create(name="Committer test", color="#000000") + adams = committers["adams"] + match = make_patch(cf, "match", adams) + match.authors.add(alice) + match.tags.add(tag) + no_tag = make_patch(cf, "no-tag", adams) + no_tag.authors.add(alice) + wrong_committer = make_patch(cf, "wrong-committer", committers["young"]) + wrong_committer.authors.add(alice) + wrong_committer.tags.add(tag) + + assert get_patch_names( + client, cf, committer=adams.user_id, author=alice.id, tag=tag.id + ) == ["match"] + + +@pytest.fixture +def other_cf_committer(commitfests): + committer = make_committer("bsmith", "Bea", "Smith") + make_patch(commitfests["in_progress"], "elsewhere", committer) + return committer + + +@pytest.mark.parametrize( + "value", + ["abc", "", "1.5", "other-cf", "no-patches", "999999", str(2**40), str(10**30)], +) +def test_invalid_committer_is_ignored( + client, cf, committers, filter_patches, other_cf_committer, value +): + if value == "other-cf": + value = other_cf_committer.user_id + elif value == "no-patches": + value = make_committer("nnone", "Ned", "None").user_id + + # Without other filters, the parameter is ignored and stripped + response = client.get(f"/{cf.id}/", {"committer": value}) + assert response.status_code == 302 + assert response["Location"] == f"/{cf.id}/" + + assert get_patch_names(client, cf, committer=value, sortkey=4) == [ + "adams-open", + "none-open", + "adams-committed", + "young-committed", + ] + + +def test_committer_choices(client, commitfests, cf, committers, filter_patches): + inactive = make_committer("iold", "Ian", "Old", active=False) + make_patch(cf, "inactive", inactive, status=PatchOnCommitFest.STATUS_COMMITTED) + # Committers without patches in this commitfest are not offered + elsewhere = make_committer("eelse", "Eve", "Elsewhere") + make_patch(commitfests["in_progress"], "elsewhere", elsewhere) + make_committer("nnone", "Ned", "None") + # Users that are not committers are not offered either + author = User.objects.create_user(username="author", last_name="Author") + make_patch(cf, "authored", None).authors.add(author) + + response = client.get(f"/{cf.id}/") + form = response.context["form"] + # Sorted by first name, like the names are shown + assert form.fields["committer"].choices == [ + (-1, "* All"), + (committers["young"].user_id, "Amy Young (ayoung)"), + (inactive.user_id, "Ian Old (iold)"), + (committers["adams"].user_id, "Zed Adams (zadams)"), + ] + # A plain dropdown, not a searchable one + assert b'