fixes

Author: JelteF

pgcommitfest/commitfest/models.py

@@ -149,8 +149,6 @@ def auto_move_active_patches(self):
 
         A patch is moved if it has recent email activity and hasn't been
         failing CI for too long.
-
-        Returns a set of patch IDs that were moved.
         """
         current_date = datetime.now()
 
@@ -175,30 +173,17 @@ def auto_move_active_patches(self):
             status__in=PatchOnCommitFest.OPEN_STATUSES
         ).select_related("patch")
 
-        moved_patch_ids = set()
         for poc in open_pocs:
             if self._should_auto_move_patch(poc.patch, current_date):
                 poc.patch.move(self, next_cf, by_user=None, by_cfbot=True)
-                moved_patch_ids.add(poc.patch.id)
-
-        return moved_patch_ids
-
-    def send_closure_notifications(self, moved_patch_ids=None):
-        """Send email notifications to authors of patches that weren't auto-moved.
-
-        Args:
-            moved_patch_ids: Set of patch IDs that were auto-moved to the next commitfest.
-                           These patches are excluded since the move triggers its own notification.
-        """
-        if moved_patch_ids is None:
-            moved_patch_ids = set()
 
+    def send_closure_notifications(self):
+        """Send email notifications to authors of patches that are still open."""
         # Get patches that still need action (not moved, not closed)
         open_pocs = list(
             self.patchoncommitfest_set.filter(
                 status__in=PatchOnCommitFest.OPEN_STATUSES
             )
-            .exclude(patch_id__in=moved_patch_ids)
             .select_related("patch")
             .prefetch_related("patch__authors")
         )
@@ -292,19 +277,19 @@ def _refresh_relevant_commitfests(cls, for_update):
 
             inprogress_cf = cfs["in_progress"]
             if inprogress_cf and inprogress_cf.enddate < current_date:
-                moved_patch_ids = inprogress_cf.auto_move_active_patches()
+                inprogress_cf.auto_move_active_patches()
                 inprogress_cf.status = CommitFest.STATUS_CLOSED
                 inprogress_cf.save()
-                inprogress_cf.send_closure_notifications(moved_patch_ids)
+                inprogress_cf.send_closure_notifications()
 
             open_cf = cfs["open"]
 
             if open_cf.startdate <= current_date:
                 if open_cf.enddate < current_date:
-                    moved_patch_ids = open_cf.auto_move_active_patches()
+                    open_cf.auto_move_active_patches()
                     open_cf.status = CommitFest.STATUS_CLOSED
                     open_cf.save()
-                    open_cf.send_closure_notifications(moved_patch_ids)
+                    open_cf.send_closure_notifications()
                 else:
                     open_cf.status = CommitFest.STATUS_INPROGRESS
                     open_cf.save()
@@ -316,10 +301,10 @@ def _refresh_relevant_commitfests(cls, for_update):
                 cls.next_draft_cf(current_date).save()
             elif draft_cf.enddate < current_date:
                 # If the draft commitfest has started, we need to update it
-                moved_patch_ids = draft_cf.auto_move_active_patches()
+                draft_cf.auto_move_active_patches()
                 draft_cf.status = CommitFest.STATUS_CLOSED
                 draft_cf.save()
-                draft_cf.send_closure_notifications(moved_patch_ids)
+                draft_cf.send_closure_notifications()
                 cls.next_draft_cf(current_date).save()
 
             return cls.relevant_commitfests(for_update=for_update)

pgcommitfest/commitfest/tests/test_closure_notifications.py

@@ -93,10 +93,9 @@ def test_no_notification_for_withdrawn_patches(alice, in_progress_cf, open_cf, t
         status=PatchOnCommitFest.STATUS_WITHDRAWN,
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
-    in_progress_cf.send_closure_notifications(moved_patch_ids)
+    in_progress_cf.auto_move_active_patches()
+    in_progress_cf.send_closure_notifications()
 
-    assert patch.id not in moved_patch_ids
     assert QueuedMail.objects.count() == 0
 
 
@@ -261,8 +260,8 @@ def test_auto_move_patch_with_recent_email_activity(
         status=PatchOnCommitFest.STATUS_REVIEW,
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
-    in_progress_cf.send_closure_notifications(moved_patch_ids)
+    in_progress_cf.auto_move_active_patches()
+    in_progress_cf.send_closure_notifications()
 
     # Patch should be moved
     patch.refresh_from_db()
@@ -298,8 +297,8 @@ def test_no_auto_move_without_email_activity(alice, in_progress_cf, open_cf, top
         status=PatchOnCommitFest.STATUS_REVIEW,
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
-    in_progress_cf.send_closure_notifications(moved_patch_ids)
+    in_progress_cf.auto_move_active_patches()
+    in_progress_cf.send_closure_notifications()
 
     # Patch should NOT be moved
     patch.refresh_from_db()
@@ -339,8 +338,8 @@ def test_no_auto_move_when_failing_too_long(alice, in_progress_cf, open_cf, topi
         - timedelta(days=settings.AUTO_MOVE_MAX_FAILING_DAYS + 10),
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
-    in_progress_cf.send_closure_notifications(moved_patch_ids)
+    in_progress_cf.auto_move_active_patches()
+    in_progress_cf.send_closure_notifications()
 
     # Patch should NOT be moved
     patch.refresh_from_db()
@@ -373,8 +372,8 @@ def test_auto_move_when_failing_within_threshold(alice, in_progress_cf, open_cf,
         - timedelta(days=settings.AUTO_MOVE_MAX_FAILING_DAYS - 5),
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
-    in_progress_cf.send_closure_notifications(moved_patch_ids)
+    in_progress_cf.auto_move_active_patches()
+    in_progress_cf.send_closure_notifications()
 
     # Patch should be moved (failure is recent enough)
     patch.refresh_from_db()
@@ -399,9 +398,8 @@ def test_no_auto_move_with_null_lastmail(alice, in_progress_cf, open_cf, topic):
         status=PatchOnCommitFest.STATUS_REVIEW,
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
+    in_progress_cf.auto_move_active_patches()
 
-    assert patch.id not in moved_patch_ids
     patch.refresh_from_db()
     assert patch.current_commitfest().id == in_progress_cf.id
 
@@ -423,10 +421,9 @@ def test_auto_move_patch_without_cfbot_branch(alice, in_progress_cf, open_cf, to
 
     # No CfbotBranch created - CI never ran
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
-    in_progress_cf.send_closure_notifications(moved_patch_ids)
+    in_progress_cf.auto_move_active_patches()
+    in_progress_cf.send_closure_notifications()
 
-    assert patch.id in moved_patch_ids
     patch.refresh_from_db()
     assert patch.current_commitfest().id == open_cf.id
 
@@ -466,10 +463,9 @@ def test_regular_cf_does_not_move_to_draft_cf(alice, in_progress_cf, topic):
         status=PatchOnCommitFest.STATUS_REVIEW,
     )
 
-    moved_patch_ids = in_progress_cf.auto_move_active_patches()
+    in_progress_cf.auto_move_active_patches()
 
     # Should be moved to regular CF, not draft CF
-    assert patch.id in moved_patch_ids
     patch.refresh_from_db()
     assert patch.current_commitfest().id == regular_cf.id
     assert patch.current_commitfest().id != draft_cf.id
@@ -507,8 +503,8 @@ def test_draft_cf_moves_active_patches_to_next_draft(alice, bob, topic):
         status=PatchOnCommitFest.STATUS_REVIEW,
     )
 
-    moved_patch_ids = closing_draft_cf.auto_move_active_patches()
-    closing_draft_cf.send_closure_notifications(moved_patch_ids)
+    closing_draft_cf.auto_move_active_patches()
+    closing_draft_cf.send_closure_notifications()
 
     # Patch should be moved to the next draft CF
     patch.refresh_from_db()