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 <step...@that.guru>
Fixes: 76505e91 ("models: Convert Series-Patch relationship to 1:N")
---
patchwork/signals.py | 21 +++++++++++++++-----
patchwork/tests/api/test_event.py | 12 +++--------
patchwork/tests/test_events.py | 33 +++++++++++++++++++++++++++----
3 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/patchwork/signals.py b/patchwork/signals.py
index 536b177e..666199b6 100644
--- 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 fd32bc8c..a2e89f53 100644
--- 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 7d03d65d..fc82022e 100644
--- 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])
--
2.17.2
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
