REST: Null out previous, current relation info

These fields don't work like we expect them to. Because we're linking to
a non-idempotent entity, an instance of 'relation', what we're storing
in either of these fields is subject to change as patches are added and
removed. This makes the information pretty much useless after the fact.
It's best to just state the patch and request that people query the
information themselves if necessary. We don't want to remove the field
entirely from the API - that would be a truly breaking change - so
instead we null it out like we do for patch tags. In a v2 API (i.e. a
major version bump) we can remove this entirely.

A small bug with the schema generation is corrected.

Signed-off-by: Stephen Finucane <[email protected]>
Related: #379
(cherry picked from commit 646a2f2)

Author: stephenfin

docs/api/schemas/patchwork.j2

@@ -1821,7 +1821,7 @@ components:
                 current_state:
                   title: Current state
                   type: string
-{% if version >= (1, 1) %}
+{% if version >= (1, 2) %}
     EventPatchRelationChanged:
       allOf:
         - $ref: '#/components/schemas/EventBase'
@@ -1837,9 +1837,11 @@ components:
                 previous_relation:
                   title: Previous relation
                   type: string
+                  nullable: true
                 current_relation:
                   title: Current relation
                   type: string
+                  nullable: true
 {% endif %}
     EventPatchDelegated:
       allOf:

docs/api/schemas/v1.1/patchwork.yaml

@@ -1551,24 +1551,6 @@ components:
                 current_state:
                   title: Current state
                   type: string
-    EventPatchRelationChanged:
-      allOf:
-        - $ref: '#/components/schemas/EventBase'
-        - type: object
-          properties:
-            category:
-              enum:
-                - patch-relation-changed
-            payload:
-              properties:
-                patch:
-                  $ref: '#/components/schemas/PatchEmbedded'
-                previous_relation:
-                  title: Previous relation
-                  type: string
-                current_relation:
-                  title: Current relation
-                  type: string
     EventPatchDelegated:
       allOf:
         - $ref: '#/components/schemas/EventBase'

docs/api/schemas/v1.2/patchwork.yaml

@@ -1768,9 +1768,11 @@ components:
                 previous_relation:
                   title: Previous relation
                   type: string
+                  nullable: true
                 current_relation:
                   title: Current relation
                   type: string
+                  nullable: true
     EventPatchDelegated:
       allOf:
         - $ref: '#/components/schemas/EventBase'

patchwork/api/event.py

@@ -33,10 +33,8 @@ class EventSerializer(ModelSerializer):
     current_delegate = UserSerializer()
     created_check = SerializerMethodField()
     created_check = CheckSerializer()
-    previous_relation = PatchSerializer(
-        source='previous_relation.patches', many=True, default=None)
-    current_relation = PatchSerializer(
-        source='current_relation.patches', many=True, default=None)
+    previous_relation = SerializerMethodField()
+    current_relation = SerializerMethodField()
 
     _category_map = {
         Event.CATEGORY_COVER_CREATED: ['cover'],
@@ -53,6 +51,12 @@ class EventSerializer(ModelSerializer):
         Event.CATEGORY_SERIES_COMPLETED: ['series'],
     }
 
+    def get_previous_relation(self, instance):
+        return None
+
+    def get_current_relation(self, instance):
+        return None
+
     def to_representation(self, instance):
         data = super(EventSerializer, self).to_representation(instance)
         payload = OrderedDict()
@@ -72,10 +76,12 @@ def to_representation(self, instance):
 
     class Meta:
         model = Event
-        fields = ('id', 'category', 'project', 'date', 'actor', 'patch',
-                  'series', 'cover', 'previous_state', 'current_state',
-                  'previous_delegate', 'current_delegate', 'created_check',
-                  'previous_relation', 'current_relation',)
+        fields = (
+            'id', 'category', 'project', 'date', 'actor', 'patch',
+            'series', 'cover', 'previous_state', 'current_state',
+            'previous_delegate', 'current_delegate', 'created_check',
+            'previous_relation', 'current_relation',
+        )
         read_only_fields = fields
         versioned_fields = {
             '1.2': ('actor', ),

patchwork/signals.py

@@ -137,14 +137,12 @@ def create_event(patch, before, after):
 @receiver(pre_save, sender=Patch)
 def create_patch_relation_changed_event(sender, instance, raw, **kwargs):
 
-    def create_event(patch, before, after):
+    def create_event(patch):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_RELATION_CHANGED,
             project=patch.project,
             actor=getattr(patch, '_edited_by', None),
-            patch=patch,
-            previous_relation=before,
-            current_relation=after)
+            patch=patch)
 
     # don't trigger for items loaded from fixtures or new items
     if raw or not instance.pk:
@@ -155,7 +153,7 @@ def create_event(patch, before, after):
     if orig_patch.related == instance.related:
         return
 
-    create_event(instance, orig_patch.related, instance.related)
+    create_event(instance)
 
 
 @receiver(pre_save, sender=Patch)

patchwork/tests/test_events.py

@@ -190,8 +190,8 @@ def test_patch_relations_changed(self):
         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)
+        self.assertIsNone(events[1].previous_relation)
+        self.assertIsNone(events[1].current_relation)
 
         # add the third patch
 
@@ -203,8 +203,8 @@ def test_patch_relations_changed(self):
         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)
+        self.assertIsNone(events[1].previous_relation)
+        self.assertIsNone(events[1].current_relation)
 
         # drop the third patch
 
@@ -216,8 +216,8 @@ def test_patch_relations_changed(self):
         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)
+        self.assertIsNone(events[2].previous_relation)
+        self.assertIsNone(events[2].current_relation)
 
 
 class CheckCreatedTest(_BaseTestCase):