diff mbox series

[2/3] tests: Add tests for 'patch-relation-changed' events

Message ID 20201004111002.234895-2-stephen@that.guru
State Accepted
Headers show
Series [1/3] tests: Rework 'create_relation' helper | expand

Commit Message

Stephen Finucane Oct. 4, 2020, 11:10 a.m. UTC
This event is rather odd. If you have two patches then the way a
relation is created is by creating a 'PatchRelation' instance and then
setting the 'related' attribute on the first patch followed by the
second patch. Because the event uses the 'Patch' model's 'pre_save'
signal, we'll only see events for the patch being currently saved. This
means no event will be raised for the first patch and only one event,
the one for the second patch, will be raised when the second patch is
being added to the relationship.

In hindsight, the structure of the event is off. We should have had
something like a 'patch-added-to-relationship' and a
'patch-removed-from-relationship' event, both with the same fields:
'project', 'actor', 'patch' and 'related', the latter of which would
have listed all of the _other_ patches in the relationship. Sadly, this
is an API change which means we can't do it now. We may well wish to do
so in the future though.

Signed-off-by: Stephen Finucane <stephen@that.guru>
---
 patchwork/tests/test_events.py | 47 ++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Comments

Daniel Axtens Oct. 7, 2020, 12:34 p.m. UTC | #1
Stephen Finucane <stephen@that.guru> writes:

> This event is rather odd. If you have two patches then the way a
> relation is created is by creating a 'PatchRelation' instance and then
> setting the 'related' attribute on the first patch followed by the
> second patch. Because the event uses the 'Patch' model's 'pre_save'
> signal, we'll only see events for the patch being currently saved. This
> means no event will be raised for the first patch and only one event,
> the one for the second patch, will be raised when the second patch is
> being added to the relationship.
>
> In hindsight, the structure of the event is off. We should have had
> something like a 'patch-added-to-relationship' and a
> 'patch-removed-from-relationship' event, both with the same fields:
> 'project', 'actor', 'patch' and 'related', the latter of which would
> have listed all of the _other_ patches in the relationship. Sadly, this
> is an API change which means we can't do it now. We may well wish to do
> so in the future though.

D'oh.

Given that we're bumping the major version of patchwork, couldn't we
also bump the API version? I don't think there are many users of the
relations events so we might as well try to get this cleaned up before
they get popular.

Kind regards,
Daniel

> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---
>  patchwork/tests/test_events.py | 47 ++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git patchwork/tests/test_events.py patchwork/tests/test_events.py
> index 415237f9..5bac7789 100644
> --- patchwork/tests/test_events.py
> +++ patchwork/tests/test_events.py
> @@ -172,6 +172,53 @@ class PatchChangedTest(_BaseTestCase):
>                           Event.CATEGORY_PATCH_DELEGATED)
>          self.assertEventFields(events[3], previous_delegate=delegate_b)
>  
> +    def test_patch_relations_changed(self):
> +        # purposefully setting series to None to minimize additional events
> +        relation = utils.create_relation()
> +        patches = utils.create_patches(3, series=None)
> +
> +        # mark the first two patches as related; the second patch should be the
> +        # one that the event is raised for
> +
> +        patches[0].related = relation
> +        patches[0].save()
> +        patches[1].related = relation
> +        patches[1].save()
> +
> +        events = _get_events(patch=patches[1])
> +        self.assertEqual(events.count(), 2)
> +        self.assertEqual(
> +            events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED)
> +        self.assertEqual(events[1].project, patches[1].project)
> +        self.assertEqual(events[1].previous_relation, None)
> +        self.assertEqual(events[1].current_relation, relation)
> +
> +        # add the third patch
> +
> +        patches[2].related = relation
> +        patches[2].save()
> +
> +        events = _get_events(patch=patches[2])
> +        self.assertEqual(events.count(), 2)
> +        self.assertEqual(
> +            events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED)
> +        self.assertEqual(events[1].project, patches[1].project)
> +        self.assertEqual(events[1].previous_relation, None)
> +        self.assertEqual(events[1].current_relation, relation)
> +
> +        # drop the third patch
> +
> +        patches[2].related = None
> +        patches[2].save()
> +
> +        events = _get_events(patch=patches[2])
> +        self.assertEqual(events.count(), 3)
> +        self.assertEqual(
> +            events[2].category, Event.CATEGORY_PATCH_RELATION_CHANGED)
> +        self.assertEqual(events[2].project, patches[1].project)
> +        self.assertEqual(events[2].previous_relation, relation)
> +        self.assertEqual(events[2].current_relation, None)
> +
>  
>  class CheckCreatedTest(_BaseTestCase):
>  
> -- 
> 2.25.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Stephen Finucane Nov. 29, 2020, 1:11 p.m. UTC | #2
On Wed, 2020-10-07 at 23:34 +1100, Daniel Axtens wrote:
> Stephen Finucane <stephen@that.guru> writes:
> 
> > This event is rather odd. If you have two patches then the way a
> > relation is created is by creating a 'PatchRelation' instance and then
> > setting the 'related' attribute on the first patch followed by the
> > second patch. Because the event uses the 'Patch' model's 'pre_save'
> > signal, we'll only see events for the patch being currently saved. This
> > means no event will be raised for the first patch and only one event,
> > the one for the second patch, will be raised when the second patch is
> > being added to the relationship.
> > 
> > In hindsight, the structure of the event is off. We should have had
> > something like a 'patch-added-to-relationship' and a
> > 'patch-removed-from-relationship' event, both with the same fields:
> > 'project', 'actor', 'patch' and 'related', the latter of which would
> > have listed all of the _other_ patches in the relationship. Sadly, this
> > is an API change which means we can't do it now. We may well wish to do
> > so in the future though.
> 
> D'oh.
> 
> Given that we're bumping the major version of patchwork, couldn't we
> also bump the API version? I don't think there are many users of the
> relations events so we might as well try to get this cleaned up before
> they get popular.

Thinking about this more, even that wouldn't work. The linked PatchRelation
entry isn't idempotent. If you create a two patch relation, an event will be
created for the second patch and the the 'current_relation' entry will show the
two patches. If you then add a third patch to this relation, another event entry
will be created but the previous one will also have its 'current_relation' value
updated to show three patches. There isn't really any way around this other than
embedding the information in a 'JSONField', which is not supported by MySQL-
based deployments without an extension. I think we should probably null this out
and remove the fields in a future major version bump of the API.

Stephen

> Kind regards,
> Daniel
diff mbox series

Patch

diff --git patchwork/tests/test_events.py patchwork/tests/test_events.py
index 415237f9..5bac7789 100644
--- patchwork/tests/test_events.py
+++ patchwork/tests/test_events.py
@@ -172,6 +172,53 @@  class PatchChangedTest(_BaseTestCase):
                          Event.CATEGORY_PATCH_DELEGATED)
         self.assertEventFields(events[3], previous_delegate=delegate_b)
 
+    def test_patch_relations_changed(self):
+        # purposefully setting series to None to minimize additional events
+        relation = utils.create_relation()
+        patches = utils.create_patches(3, series=None)
+
+        # mark the first two patches as related; the second patch should be the
+        # one that the event is raised for
+
+        patches[0].related = relation
+        patches[0].save()
+        patches[1].related = relation
+        patches[1].save()
+
+        events = _get_events(patch=patches[1])
+        self.assertEqual(events.count(), 2)
+        self.assertEqual(
+            events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED)
+        self.assertEqual(events[1].project, patches[1].project)
+        self.assertEqual(events[1].previous_relation, None)
+        self.assertEqual(events[1].current_relation, relation)
+
+        # add the third patch
+
+        patches[2].related = relation
+        patches[2].save()
+
+        events = _get_events(patch=patches[2])
+        self.assertEqual(events.count(), 2)
+        self.assertEqual(
+            events[1].category, Event.CATEGORY_PATCH_RELATION_CHANGED)
+        self.assertEqual(events[1].project, patches[1].project)
+        self.assertEqual(events[1].previous_relation, None)
+        self.assertEqual(events[1].current_relation, relation)
+
+        # drop the third patch
+
+        patches[2].related = None
+        patches[2].save()
+
+        events = _get_events(patch=patches[2])
+        self.assertEqual(events.count(), 3)
+        self.assertEqual(
+            events[2].category, Event.CATEGORY_PATCH_RELATION_CHANGED)
+        self.assertEqual(events[2].project, patches[1].project)
+        self.assertEqual(events[2].previous_relation, relation)
+        self.assertEqual(events[2].current_relation, None)
+
 
 class CheckCreatedTest(_BaseTestCase):