signals: Fix 'series-completed' event

I'm not sure how I ever intended this to work and there were no tests
verifying things. Fix it now and add tests to prevent it regressing.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Fixes: 76505e91 ("models: Convert Series-Patch relationship to 1:N")

diff --git a/patchwork/signals.py b/patchwork/signals.py
index 536b177e37cfae1a02d9d6a38e5b53b888dcc3f8..666199b68161f8e63d4b258c62f961e7ba77e9ea 100644 (file)
--- a/patchwork/signals.py
+++ b/patchwork/signals.py
@@ -210,8 +210,8 @@ def create_series_created_event(sender, instance, created, raw, **kwargs):
     create_event(instance)
 
 
-@receiver(post_save, sender=Patch)
-def create_series_completed_event(sender, instance, created, raw, **kwargs):
+@receiver(pre_save, sender=Patch)
+def create_series_completed_event(sender, instance, raw, **kwargs):
 
     # NOTE(stephenfin): It's actually possible for this event to be fired
     # multiple times for a given series. To trigger this case, you would need
@@ -225,9 +225,20 @@ def create_series_completed_event(sender, instance, created, raw, **kwargs):
             project=series.project,
             series=series)
 
-    # don't trigger for items loaded from fixtures or existing items
-    if raw or not created:
+    # don't trigger for items loaded from fixtures, new items or items that
+    # (still) don't have a series
+    if raw or not instance.pk or not instance.series:
+        return
+
+    orig_patch = Patch.objects.get(pk=instance.pk)
+
+    # we don't currently allow users to change a series (though this might
+    # change in the future) meaning if the patch already had a series, there's
+    # nothing to notify about
+    if orig_patch.series:
         return
 
-    if instance.series and instance.series.received_all:
+    # we can't use "series.received_all" here since we haven't actually saved
+    # the instance yet so we duplicate that logic here but with an offset
+    if (instance.series.received_total + 1) >= instance.series.total:
         create_event(instance.series)

diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
index fd32bc8c58954b86a228b7e472076d7237eab09a..a2e89f53e0396a0369fc08208cbd4b914fdf0fa4 100644 (file)
--- a/patchwork/tests/api/test_event.py
+++ b/patchwork/tests/api/test_event.py
@@ -86,9 +86,7 @@ class TestEventAPI(APITestCase):
 
         resp = self.client.get(self.api_url())
         self.assertEqual(status.HTTP_200_OK, resp.status_code)
-        # FIXME(stephenfin): This should actually return 8 events but
-        # 'series-completed' events are not currently being generated
-        self.assertEqual(7, len(resp.data), [x['category'] for x in resp.data])
+        self.assertEqual(8, len(resp.data), [x['category'] for x in resp.data])
         for event_rsp in resp.data:
             event_obj = events.get(category=event_rsp['category'])
             self.assertSerialized(event_obj, event_rsp)
@@ -101,9 +99,7 @@ class TestEventAPI(APITestCase):
 
         resp = self.client.get(self.api_url(), {'project': project.pk})
         # All but one event belongs to the same project
-        # FIXME(stephenfin): This should actually return 8 events but
-        # 'series-completed' events are not currently being generated
-        self.assertEqual(7, len(resp.data))
+        self.assertEqual(8, len(resp.data))
 
         resp = self.client.get(self.api_url(), {'project': 'invalidproject'})
         self.assertEqual(0, len(resp.data))
@@ -153,9 +149,7 @@ class TestEventAPI(APITestCase):
         resp = self.client.get(self.api_url(), {'series': series.pk})
         # There should be three - series-created, patch-completed and
         # series-completed
-        # FIXME(stephenfin): This should actually return 3 events but
-        # 'series-completed' events are not currently being generated
-        self.assertEqual(2, len(resp.data))
+        self.assertEqual(3, len(resp.data))
 
         resp = self.client.get(self.api_url(), {'series': 999999})
         self.assertEqual(0, len(resp.data))

diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py
index 7d03d65db31537803f67e8e32369451bccd40c76..fc82022e175b9822b537c0d1c4d3720f5924aa63 100644 (file)
--- a/patchwork/tests/test_events.py
+++ b/patchwork/tests/test_events.py
@@ -28,7 +28,7 @@ class _BaseTestCase(TestCase):
                 self.assertIsNone(field)
 
 
-class PatchCreateTest(_BaseTestCase):
+class PatchCreatedTest(_BaseTestCase):
 
     def test_patch_created(self):
         """No series, so patch dependencies implicitly exist."""
@@ -165,7 +165,7 @@ class PatchChangedTest(_BaseTestCase):
         self.assertEventFields(events[3], previous_delegate=delegate_b)
 
 
-class CheckCreateTest(_BaseTestCase):
+class CheckCreatedTest(_BaseTestCase):
 
     def test_check_created(self):
         check = utils.create_check()
@@ -176,7 +176,7 @@ class CheckCreateTest(_BaseTestCase):
         self.assertEventFields(events[0])
 
 
-class CoverCreateTest(_BaseTestCase):
+class CoverCreatedTest(_BaseTestCase):
 
     def test_cover_created(self):
         cover = utils.create_cover()
@@ -187,7 +187,7 @@ class CoverCreateTest(_BaseTestCase):
         self.assertEventFields(events[0])
 
 
-class SeriesCreateTest(_BaseTestCase):
+class SeriesCreatedTest(_BaseTestCase):
 
     def test_series_created(self):
         series = utils.create_series()
@@ -196,3 +196,28 @@ class SeriesCreateTest(_BaseTestCase):
         self.assertEqual(events[0].category, Event.CATEGORY_SERIES_CREATED)
         self.assertEqual(events[0].project, series.project)
         self.assertEventFields(events[0])
+
+
+class SeriesChangedTest(_BaseTestCase):
+
+    def test_series_completed(self):
+        """Validate 'series-completed' events."""
+        series = utils.create_series(total=2)
+
+        # the series has no patches associated with it so it's not yet complete
+        events = _get_events(series=series)
+        self.assertNotIn(Event.CATEGORY_SERIES_COMPLETED,
+                         [x.category for x in events])
+
+        # create the second of two patches in the series; series is still not
+        # complete
+        utils.create_patch(series=series, number=2)
+        events = _get_events(series=series)
+        self.assertNotIn(Event.CATEGORY_SERIES_COMPLETED,
+                         [x.category for x in events])
+
+        # now create the first patch, which will "complete" the series
+        utils.create_patch(series=series, number=1)
+        events = _get_events(series=series)
+        self.assertIn(Event.CATEGORY_SERIES_COMPLETED,
+                      [x.category for x in events])