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")