From 9696c8e7ac3bafeb526ab5cedbf8d221b0e058dd Mon Sep 17 00:00:00 2001 From: Jonas Rocha Date: Sat, 17 Oct 2026 10:33:48 -0300 Subject: [PATCH 1/3] 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 f003ef20ac6d6e63bde354a198a876f0b1ef1cf9 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 09:58:40 +0200 Subject: [PATCH 2/3] Remove committer sort tests The tests were very specialised to this one sort method. A generic test covering all sort keys would be more useful and belongs in a separate PR. --- .../commitfest/tests/test_committer_sort.py | 189 ------------------ 1 file changed, 189 deletions(-) delete mode 100644 pgcommitfest/commitfest/tests/test_committer_sort.py diff --git a/pgcommitfest/commitfest/tests/test_committer_sort.py b/pgcommitfest/commitfest/tests/test_committer_sort.py deleted file mode 100644 index 0b5154ba..00000000 --- a/pgcommitfest/commitfest/tests/test_committer_sort.py +++ /dev/null @@ -1,189 +0,0 @@ -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 From 33cd08112fb67b774b8eae86d60548f2d7c1271b Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 11:35:32 +0200 Subject: [PATCH 3/3] Make descending committer sort the exact reverse of ascending Descending kept patches without a committer at the bottom and kept the `p.id` tiebreaker ascending, so clicking the header again didn't reverse the list. Now it does. Since PostgreSQL already sorts NULLs last when ascending and first when descending, the explicit `NULLS LAST` clauses are no longer needed. --- pgcommitfest/commitfest/views.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 25a953b7..38bb2f0f 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -508,14 +508,16 @@ def patchlist(request, cf, personalized=False): elif sortkey == -8: orderby_str = "poc.commitfest_id DESC, lastmail" elif sortkey == 9 and not personalized: + # PostgreSQL sorts NULLs last when ascending and first when + # descending, so patches without a committer end up at the bottom in + # this order and at the top in the reverse one. orderby_str = ( - "committer.last_name NULLS LAST, committer.first_name NULLS LAST, " - "committer.username NULLS LAST, p.id" + "committer.last_name, committer.first_name, committer.username, 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" + "committer.last_name DESC, committer.first_name DESC, " + "committer.username DESC, p.id DESC" ) else: if personalized: