From 777969b66707efec946ef7eed1f701d555978e68 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 22:24:53 +0100 Subject: [PATCH] Allow marking an already closed patch as committed When a patch in the open commitfest was already closed, for example as "Returned with feedback", and a committer then marked it as committed, this crashed with `UserInputError: Patch in state Returned with feedback cannot be moved.`. The "committed" action first moves the patch to the in progress commitfest (or from the draft to the regular open commitfest) so that it gets recorded in the commitfest it was actually committed in, but `Patch.move()` refused to move closed patches. `Patch.move()` now takes an `allow_move_closed` argument, which the commit action passes. Because closed statuses require a leavedate, the new `PatchOnCommitFest` gets one when the patch it was moved from was closed. --- pgcommitfest/commitfest/models.py | 20 ++++++----- .../commitfest/tests/test_close_committed.py | 36 +++++++++++++++++++ pgcommitfest/commitfest/views.py | 6 ++++ 3 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 pgcommitfest/commitfest/tests/test_close_committed.py diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index a088f90a..c7b8fb4f 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -598,7 +598,13 @@ def update_lastmail(self): self.lastmail = max(threads, key=lambda t: t.latestmessage).latestmessage def move( - self, from_cf, to_cf, by_user, allow_move_to_in_progress=False, by_cfbot=False + self, + from_cf, + to_cf, + by_user, + allow_move_to_in_progress=False, + allow_move_closed=False, + by_cfbot=False, ): """Returns the new PatchOnCommitFest object, or raises UserInputError""" @@ -609,11 +615,7 @@ def move( if from_cf.id == to_cf.id: raise UserInputError("Source and target commitfest are the same.") - if current_poc.status not in ( - PatchOnCommitFest.STATUS_REVIEW, - PatchOnCommitFest.STATUS_AUTHOR, - PatchOnCommitFest.STATUS_COMMITTER, - ): + if current_poc.is_closed and not allow_move_closed: raise UserInputError( f"Patch in state {current_poc.statusstring} cannot be moved." ) @@ -625,16 +627,18 @@ def move( raise UserInputError("Patch can only be moved to an open commitfest") old_status = current_poc.status + was_closed = current_poc.is_closed current_poc.set_status(PatchOnCommitFest.STATUS_MOVED) + now = datetime.now() new_poc, _ = PatchOnCommitFest.objects.update_or_create( patch=current_poc.patch, commitfest=to_cf, defaults=dict( status=old_status, - enterdate=datetime.now(), - leavedate=None, + enterdate=now, + leavedate=now if was_closed else None, ), ) new_poc.save() diff --git a/pgcommitfest/commitfest/tests/test_close_committed.py b/pgcommitfest/commitfest/tests/test_close_committed.py new file mode 100644 index 00000000..90bdd1d9 --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_close_committed.py @@ -0,0 +1,36 @@ +from datetime import datetime + +import pytest + +from pgcommitfest.commitfest.models import Committer, Patch, PatchOnCommitFest + +pytestmark = pytest.mark.django_db + + +@pytest.mark.parametrize( + "status", + [PatchOnCommitFest.STATUS_REVIEW, PatchOnCommitFest.STATUS_RETURNED], +) +def test_commit_moves_patch_to_in_progress_cf( + client, open_cf, in_progress_cf, alice, status +): + Committer.objects.create(user=alice) + patch = Patch.objects.create(name="Test patch") + poc = PatchOnCommitFest.objects.create( + patch=patch, + commitfest=open_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + poc.set_status(status) + + client.force_login(alice) + response = client.post(f"/patch/{patch.id}/close/committed/?c={alice.username}") + assert response.status_code == 302 + + old_poc = PatchOnCommitFest.objects.get(patch=patch, commitfest=open_cf) + assert old_poc.status == PatchOnCommitFest.STATUS_MOVED + new_poc = PatchOnCommitFest.objects.get(patch=patch, commitfest=in_progress_cf) + assert new_poc.status == PatchOnCommitFest.STATUS_COMMITTED + patch.refresh_from_db() + assert patch.committer.user == alice diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index d36ac503..b9466b5e 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -1179,6 +1179,10 @@ def close(request, patchid, status): if poc.commitfest.is_open: # Needs to be done before the next if condition, so the committer # that's set there is set on the correct poc. + # + # A patch that was already closed (e.g. returned with feedback) + # might still get committed later, and then it should also end up + # in the commitfest it was committed in. in_progress_cf = CommitFest.get_in_progress() if in_progress_cf is not None: poc = patch.move( @@ -1186,6 +1190,7 @@ def close(request, patchid, status): in_progress_cf, request.user, allow_move_to_in_progress=True, + allow_move_closed=True, ) elif poc.commitfest.draft: open_cf = CommitFest.get_open_regular() @@ -1193,6 +1198,7 @@ def close(request, patchid, status): poc.commitfest, open_cf, request.user, + allow_move_closed=True, ) committer_username = request.GET.get("c")