From patchwork Tue Oct 30 11:31:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 990769 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 42kqFp0Qj1z9s7W for ; Tue, 30 Oct 2018 22:37:34 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="aTJaI0MD"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 42kqFn5s2MzF1Pt for ; Tue, 30 Oct 2018 22:37:33 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="aTJaI0MD"; dkim-atps=neutral X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=none (mailfrom) smtp.mailfrom=that.guru (client-ip=185.234.75.12; helo=relay012.mxrelay.co; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="aTJaI0MD"; dkim-atps=neutral Received: from relay012.mxrelay.co (relay012.mxrelay.co [185.234.75.12]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 42kq862FhdzF1QC for ; Tue, 30 Oct 2018 22:32:38 +1100 (AEDT) Received: from filter002.mxroute.com (unknown [185.133.192.179]) by relay012.mxrelay.co (Postfix) with ESMTP id CDBD33F9FF for ; Tue, 30 Oct 2018 11:32:05 +0000 (UTC) Received: from one.mxroute.com (one.mxroute.com [195.201.59.211]) by filter002.mxroute.com (Postfix) with ESMTPS id B470C3F3DB for ; Tue, 30 Oct 2018 11:32:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=default; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From: Sender:Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=m2onPd1b12aUPWN4UBYgI1nVAoe6qvIayMkpkMRrKyQ=; b=aTJaI0MDwDqce7P998e79WKCDT IMMd+4U2nizakqOt0mrQH7AUk26G96WsZTRdVeDa6wsBpH0HnHtEgdsi05XZNAeT9TRytLt4KGEEp 8f4gxhw9py+uxXIbJAxfmWKOVDoYmRceR92kqpQaRmDENU3mQxJNYJxkKOeT45yxNKjIB//c8Tk8P gp3+tDRPs24Ygb1l8xZoVQYHarc9+ESZNXuJBPl14Yb/MgVkRIfl+rsljb4d0otdXLVPqWL+kEwXj oL5j6Wg8Hs16nF8DXI9rVDiSA8ALjEej9GkIQ85my5rLGY40WO7ippgSkip2+hIa46mF/DycsGNrd 11yVYpBQ==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [PATCH 13/13] signals: Fix 'series-completed' event Date: Tue, 30 Oct 2018 11:31:53 +0000 Message-Id: <20181030113153.7855-14-stephen@that.guru> X-Mailer: git-send-email 2.17.2 In-Reply-To: <20181030113153.7855-1-stephen@that.guru> References: <20181030113153.7855-1-stephen@that.guru> X-AuthUser: stephen@that.guru X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" 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 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])