[3/3] /api/events: Add 'user' field to generated JSON

Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 docs/api/schemas/latest/patchwork.yaml |  6 ++++++
 docs/api/schemas/patchwork.j2          |  6 ++++++
 docs/api/schemas/v1.0/patchwork.yaml   |  6 ++++++
 docs/api/schemas/v1.1/patchwork.yaml   |  6 ++++++
 docs/api/schemas/v1.2/patchwork.yaml   |  6 ++++++
 patchwork/api/event.py                 | 10 +++++++---
 patchwork/tests/api/test_event.py      |  7 +++++++
 7 files changed, 44 insertions(+), 3 deletions(-)

On Mon, 2019-10-07 at 00:57 +0200, Johan Herland wrote:
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Johan Herland <johan@herland.net>

This looks pretty good and should stay almost the same even if earlier
patches in the series change. If you can fix up that version check I
think we're good to go.

Stephen

> ---
>  docs/api/schemas/latest/patchwork.yaml |  6 ++++++
>  docs/api/schemas/patchwork.j2          |  6 ++++++
>  docs/api/schemas/v1.0/patchwork.yaml   |  6 ++++++
>  docs/api/schemas/v1.1/patchwork.yaml   |  6 ++++++
>  docs/api/schemas/v1.2/patchwork.yaml   |  6 ++++++
>  patchwork/api/event.py                 | 10 +++++++---
>  patchwork/tests/api/test_event.py      |  7 +++++++
>  7 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml
> index 45a6118..0fb2171 100644
> --- a/docs/api/schemas/latest/patchwork.yaml
> +++ b/docs/api/schemas/latest/patchwork.yaml
> @@ -1495,6 +1495,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        user:
> +          type: object
> +          title: The user responsible for 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 16d85a3..0acb665 100644
> --- a/docs/api/schemas/patchwork.j2
> +++ b/docs/api/schemas/patchwork.j2
> @@ -1510,6 +1510,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        user:
> +          type: object
> +          title: The user responsible for this event
> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'

You need to surround this with version checks to make sure this doesn't
get emitted for the v1.0 and v1.1 APIs as they won't have this field.

>          payload:
>            type: object
>      EventCoverCreated:
> diff --git a/docs/api/schemas/v1.0/patchwork.yaml b/docs/api/schemas/v1.0/patchwork.yaml
> index 02f3a15..a59c64c 100644
> --- a/docs/api/schemas/v1.0/patchwork.yaml
> +++ b/docs/api/schemas/v1.0/patchwork.yaml
> @@ -1460,6 +1460,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        user:
> +          type: object
> +          title: The user responsible for this event
> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'
>          payload:
>            type: object
>      EventCoverCreated:

So the above hunk would go...

> diff --git a/docs/api/schemas/v1.1/patchwork.yaml b/docs/api/schemas/v1.1/patchwork.yaml
> index 0c086ed..728b878 100644
> --- a/docs/api/schemas/v1.1/patchwork.yaml
> +++ b/docs/api/schemas/v1.1/patchwork.yaml
> @@ -1485,6 +1485,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        user:
> +          type: object
> +          title: The user responsible for this event
> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'
>          payload:
>            type: object
>      EventCoverCreated:

As would this one.

> diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml
> index 3a96aa3..9093bba 100644
> --- a/docs/api/schemas/v1.2/patchwork.yaml
> +++ b/docs/api/schemas/v1.2/patchwork.yaml
> @@ -1495,6 +1495,12 @@ components:
>            type: string
>            format: iso8601
>            readOnly: true
> +        user:
> +          type: object
> +          title: The user responsible for this event
> +          readOnly: true
> +          allOf:
> +            - $ref: '#/components/schemas/UserEmbedded'
>          payload:
>            type: object
>      EventCoverCreated:
> diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> index c0d973d..f1b46e5 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)
> +    user = 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', 'user']
>  
>          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', 'user', 'patch',
> +                  'series', 'cover', 'previous_state', 'current_state',
>                    'previous_delegate', 'current_delegate', 'created_check')
>          read_only_fields = fields
> +        versioned_fields = {
> +            '1.2': ('user', ),
> +        }
>  
>  
>  class EventList(ListAPIView):
> diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
> index 8816538..fd42e3d 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.user is None:
> +            self.assertIsNone(event_json['user'])
>  
>          # nested fields
>  
>          self.assertEqual(event_obj.project.id,
>                           event_json['project']['id'])
> +        if event_obj.user is not None:
> +            self.assertEqual(event_obj.user.id,
> +                             event_json['user']['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
> +        maintainer = 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(maintainer))
>          patch.save()
>  
>          return Event.objects.all()

It'd be good to have an actual check that the person that edited the
thing is set on the Event.user field.

On Mon, Oct 7, 2019 at 5:55 PM Stephen Finucane <stephen@that.guru> wrote:
> On Mon, 2019-10-07 at 00:57 +0200, Johan Herland wrote:
> > Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > Signed-off-by: Johan Herland <johan@herland.net>
>
> This looks pretty good and should stay almost the same even if earlier
> patches in the series change. If you can fix up that version check I
> think we're good to go.

Thanks again for reviewing.

> > diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
> > index 16d85a3..0acb665 100644
> > --- a/docs/api/schemas/patchwork.j2
> > +++ b/docs/api/schemas/patchwork.j2
> > @@ -1510,6 +1510,12 @@ components:
> >            type: string
> >            format: iso8601
> >            readOnly: true
> > +        user:
> > +          type: object
> > +          title: The user responsible for this event
> > +          readOnly: true
> > +          allOf:
> > +            - $ref: '#/components/schemas/UserEmbedded'
>
> You need to surround this with version checks to make sure this doesn't
> get emitted for the v1.0 and v1.1 APIs as they won't have this field.

Will fix.

> It'd be good to have an actual check that the person that edited the
> thing is set on the Event.user field.

Sure, I'll add a test that updates a patch via the REST API, and then
asserts that the resulting event has the correct actor set.

Have fun!
...Johan