diff mbox series

[v4,3/4] /api/events: Add 'actor' field to generated JSON

Message ID 20191201014955.1994-4-johan@herland.net
State Accepted
Headers show
Series Store the 'actor' responsible for events | expand

Commit Message

Johan Herland Dec. 1, 2019, 1:49 a.m. UTC
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Johan Herland <johan@herland.net>
Acked-by: Daniel Axtens <dja@axtens.net>
---
 docs/api/schemas/latest/patchwork.yaml |  6 ++++++
 docs/api/schemas/patchwork.j2          |  8 ++++++++
 docs/api/schemas/v1.2/patchwork.yaml   |  6 ++++++
 docs/usage/overview.rst                |  3 +++
 patchwork/api/event.py                 | 10 +++++++---
 patchwork/tests/api/test_event.py      | 24 ++++++++++++++++++++++++
 6 files changed, 54 insertions(+), 3 deletions(-)

Comments

Stephen Finucane Dec. 1, 2019, 12:14 p.m. UTC | #1
On Sun, 2019-12-01 at 02:49 +0100, Johan Herland wrote:
> Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Signed-off-by: Johan Herland <johan@herland.net>
> Acked-by: Daniel Axtens <dja@axtens.net>

Applied with the minor tweaks noted below.

Reviewed-by: Stephen Finucane <stephen@that.guru>

> ---
>  docs/api/schemas/latest/patchwork.yaml |  6 ++++++
>  docs/api/schemas/patchwork.j2          |  8 ++++++++
>  docs/api/schemas/v1.2/patchwork.yaml   |  6 ++++++
>  docs/usage/overview.rst                |  3 +++
>  patchwork/api/event.py                 | 10 +++++++---
>  patchwork/tests/api/test_event.py      | 24 ++++++++++++++++++++++++
>  6 files changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml
> index 6c7564b..35dda80 100644
> --- a/docs/api/schemas/latest/patchwork.yaml
> +++ b/docs/api/schemas/latest/patchwork.yaml
> @@ -1649,6 +1649,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        actor:
> +          type: object
> +          title: The user that caused/created this event

This is more a description than a title, so I've changed this to:

  title: Actor
  description: The user that caused/created this event.

in the template and updated the generated schemas.

> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'
>          payload:
>            type: object
>      EventCoverCreated:
> diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
> index e2c8a8c..0fbc243 100644
> --- a/docs/api/schemas/patchwork.j2
> +++ b/docs/api/schemas/patchwork.j2
> @@ -1679,6 +1679,14 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +{% if version >= (1, 2) %}
> +        actor:
> +          type: object
> +          title: The user that caused/created this event
> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'
> +{% endif %}
>          payload:
>            type: object
>      EventCoverCreated:
> diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml
> index 7dc9579..c186dc0 100644
> --- a/docs/api/schemas/v1.2/patchwork.yaml
> +++ b/docs/api/schemas/v1.2/patchwork.yaml
> @@ -1649,6 +1649,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        actor:
> +          type: object
> +          title: The user that caused/created this event
> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'
>          payload:
>            type: object
>      EventCoverCreated:
> diff --git a/docs/usage/overview.rst b/docs/usage/overview.rst
> index e84e13d..273c792 100644
> --- a/docs/usage/overview.rst
> +++ b/docs/usage/overview.rst
> @@ -228,6 +228,9 @@ properties:
>  ``date``
>    When this event was created
>  
> +``actor``
> +  The user, if any, that caused/created this event
> +
>  ``payload``
>    Additional information
>  
> diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> index e6d467d..e00ce05 100644
> --- a/patchwork/api/event.py
> +++ b/patchwork/api/event.py
> @@ -23,6 +23,7 @@ from patchwork.models import Event
>  class EventSerializer(ModelSerializer):
>  
>      project = ProjectSerializer(read_only=True)
> +    actor = UserSerializer()
>      patch = PatchSerializer(read_only=True)
>      series = SeriesSerializer(read_only=True)
>      cover = CoverLetterSerializer(read_only=True)
> @@ -50,7 +51,7 @@ class EventSerializer(ModelSerializer):
>          data = super(EventSerializer, self).to_representation(instance)
>          payload = OrderedDict()
>          kept_fields = self._category_map[instance.category] + [
> -            'id', 'category', 'project', 'date']
> +            'id', 'category', 'project', 'date', 'actor']
>  
>          for field in [x for x in data]:
>              if field not in kept_fields:
> @@ -65,10 +66,13 @@ class EventSerializer(ModelSerializer):
>  
>      class Meta:
>          model = Event
> -        fields = ('id', 'category', 'project', 'date', 'patch', 'series',
> -                  'cover', 'previous_state', 'current_state',
> +        fields = ('id', 'category', 'project', 'date', 'actor', 'patch',
> +                  'series', 'cover', 'previous_state', 'current_state',
>                    'previous_delegate', 'current_delegate', 'created_check')
>          read_only_fields = fields
> +        versioned_fields = {
> +            '1.2': ('actor', ),
> +        }
>  
>  
>  class EventList(ListAPIView):
> diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
> index bff8f40..456f2f5 100644
> --- a/patchwork/tests/api/test_event.py
> +++ b/patchwork/tests/api/test_event.py
> @@ -35,11 +35,16 @@ class TestEventAPI(utils.APITestCase):
>      def assertSerialized(self, event_obj, event_json):
>          self.assertEqual(event_obj.id, event_json['id'])
>          self.assertEqual(event_obj.category, event_json['category'])
> +        if event_obj.actor is None:
> +            self.assertIsNone(event_json['actor'])
>  
>          # nested fields
>  
>          self.assertEqual(event_obj.project.id,
>                           event_json['project']['id'])
> +        if event_obj.actor is not None:
> +            self.assertEqual(event_obj.actor.id,
> +                             event_json['actor']['id'])
>  
>          # TODO(stephenfin): Check other fields
>  
> @@ -66,10 +71,12 @@ class TestEventAPI(utils.APITestCase):
>          # check-created
>          create_check(patch=patch)
>          # patch-delegated, patch-state-changed
> +        actor = create_maintainer(project=patch.project)
>          user = create_maintainer(project=patch.project)
>          state = create_state()
>          patch.delegate = user
>          patch.state = state
> +        self.assertTrue(patch.is_editable(actor))
>          patch.save()
>  
>          return Event.objects.all()
> @@ -176,3 +183,20 @@ class TestEventAPI(utils.APITestCase):
>          self.client.force_authenticate(user=user)
>          resp = self.client.post(self.api_url(), {'category': 'patch-created'})
>          self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> +
> +    def test_change_delegate(self):
> +        """Ensure changing patch delegate via API produces expected event"""
> +        patch = create_patch()
> +        delegate = create_maintainer(project=patch.project)
> +        actor = create_maintainer(project=patch.project)
> +
> +        self.client.force_authenticate(user=actor)
> +        patch_url = reverse('api-patch-detail', kwargs={'pk': patch.id})
> +        resp = self.client.patch(patch_url, {'delegate': delegate.id})
> +        self.assertEqual(status.HTTP_200_OK, resp.status_code, resp)
> +
> +        events = Event.objects.all()
> +        delegation_event = events.get(category='patch-delegated')
> +        self.assertEqual(actor.id, delegation_event.actor.id)
> +        self.assertEqual(None, delegation_event.previous_delegate)
> +        self.assertEqual(delegate.id, delegation_event.current_delegate.id)

I dropped this because it felt unnecessary given we already have a
similar test in 'patchwork.tests.test_events'.
diff mbox series

Patch

diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml
index 6c7564b..35dda80 100644
--- a/docs/api/schemas/latest/patchwork.yaml
+++ b/docs/api/schemas/latest/patchwork.yaml
@@ -1649,6 +1649,12 @@  components:
           type: string
           format: iso8601
           readOnly: true
+        actor:
+          type: object
+          title: The user that caused/created this event
+          readOnly: true
+          allOf:
+            - $ref: '#/components/schemas/UserEmbedded'
         payload:
           type: object
     EventCoverCreated:
diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
index e2c8a8c..0fbc243 100644
--- a/docs/api/schemas/patchwork.j2
+++ b/docs/api/schemas/patchwork.j2
@@ -1679,6 +1679,14 @@  components:
           type: string
           format: iso8601
           readOnly: true
+{% if version >= (1, 2) %}
+        actor:
+          type: object
+          title: The user that caused/created this event
+          readOnly: true
+          allOf:
+            - $ref: '#/components/schemas/UserEmbedded'
+{% endif %}
         payload:
           type: object
     EventCoverCreated:
diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml
index 7dc9579..c186dc0 100644
--- a/docs/api/schemas/v1.2/patchwork.yaml
+++ b/docs/api/schemas/v1.2/patchwork.yaml
@@ -1649,6 +1649,12 @@  components:
           type: string
           format: iso8601
           readOnly: true
+        actor:
+          type: object
+          title: The user that caused/created this event
+          readOnly: true
+          allOf:
+            - $ref: '#/components/schemas/UserEmbedded'
         payload:
           type: object
     EventCoverCreated:
diff --git a/docs/usage/overview.rst b/docs/usage/overview.rst
index e84e13d..273c792 100644
--- a/docs/usage/overview.rst
+++ b/docs/usage/overview.rst
@@ -228,6 +228,9 @@  properties:
 ``date``
   When this event was created
 
+``actor``
+  The user, if any, that caused/created this event
+
 ``payload``
   Additional information
 
diff --git a/patchwork/api/event.py b/patchwork/api/event.py
index e6d467d..e00ce05 100644
--- a/patchwork/api/event.py
+++ b/patchwork/api/event.py
@@ -23,6 +23,7 @@  from patchwork.models import Event
 class EventSerializer(ModelSerializer):
 
     project = ProjectSerializer(read_only=True)
+    actor = UserSerializer()
     patch = PatchSerializer(read_only=True)
     series = SeriesSerializer(read_only=True)
     cover = CoverLetterSerializer(read_only=True)
@@ -50,7 +51,7 @@  class EventSerializer(ModelSerializer):
         data = super(EventSerializer, self).to_representation(instance)
         payload = OrderedDict()
         kept_fields = self._category_map[instance.category] + [
-            'id', 'category', 'project', 'date']
+            'id', 'category', 'project', 'date', 'actor']
 
         for field in [x for x in data]:
             if field not in kept_fields:
@@ -65,10 +66,13 @@  class EventSerializer(ModelSerializer):
 
     class Meta:
         model = Event
-        fields = ('id', 'category', 'project', 'date', 'patch', 'series',
-                  'cover', 'previous_state', 'current_state',
+        fields = ('id', 'category', 'project', 'date', 'actor', 'patch',
+                  'series', 'cover', 'previous_state', 'current_state',
                   'previous_delegate', 'current_delegate', 'created_check')
         read_only_fields = fields
+        versioned_fields = {
+            '1.2': ('actor', ),
+        }
 
 
 class EventList(ListAPIView):
diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
index bff8f40..456f2f5 100644
--- a/patchwork/tests/api/test_event.py
+++ b/patchwork/tests/api/test_event.py
@@ -35,11 +35,16 @@  class TestEventAPI(utils.APITestCase):
     def assertSerialized(self, event_obj, event_json):
         self.assertEqual(event_obj.id, event_json['id'])
         self.assertEqual(event_obj.category, event_json['category'])
+        if event_obj.actor is None:
+            self.assertIsNone(event_json['actor'])
 
         # nested fields
 
         self.assertEqual(event_obj.project.id,
                          event_json['project']['id'])
+        if event_obj.actor is not None:
+            self.assertEqual(event_obj.actor.id,
+                             event_json['actor']['id'])
 
         # TODO(stephenfin): Check other fields
 
@@ -66,10 +71,12 @@  class TestEventAPI(utils.APITestCase):
         # check-created
         create_check(patch=patch)
         # patch-delegated, patch-state-changed
+        actor = create_maintainer(project=patch.project)
         user = create_maintainer(project=patch.project)
         state = create_state()
         patch.delegate = user
         patch.state = state
+        self.assertTrue(patch.is_editable(actor))
         patch.save()
 
         return Event.objects.all()
@@ -176,3 +183,20 @@  class TestEventAPI(utils.APITestCase):
         self.client.force_authenticate(user=user)
         resp = self.client.post(self.api_url(), {'category': 'patch-created'})
         self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
+
+    def test_change_delegate(self):
+        """Ensure changing patch delegate via API produces expected event"""
+        patch = create_patch()
+        delegate = create_maintainer(project=patch.project)
+        actor = create_maintainer(project=patch.project)
+
+        self.client.force_authenticate(user=actor)
+        patch_url = reverse('api-patch-detail', kwargs={'pk': patch.id})
+        resp = self.client.patch(patch_url, {'delegate': delegate.id})
+        self.assertEqual(status.HTTP_200_OK, resp.status_code, resp)
+
+        events = Event.objects.all()
+        delegation_event = events.get(category='patch-delegated')
+        self.assertEqual(actor.id, delegation_event.actor.id)
+        self.assertEqual(None, delegation_event.previous_delegate)
+        self.assertEqual(delegate.id, delegation_event.current_delegate.id)