Allow ordering events by date

By default, the events API orders events by date in descending order
(newest first). However, it's useful to be able to order the events by
oldest events first. For example, when a client is polling the events
API for new events since a given date and wishes to process them in
chronological order.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
---
 patchwork/api/event.py                         |  2 +-
 patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
 ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
 3 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml

On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> By default, the events API orders events by date in descending order
> (newest first). However, it's useful to be able to order the events by
> oldest events first. For example, when a client is polling the events
> API for new events since a given date and wishes to process them in
> chronological order.
> 
> Signed-off-by: Jeremy Cline <jcline@redhat.com>

I'd purposefully avoided doing this initially because I wanted
'/events' to be thought of as a firehose that should be just consumed
as things were generated. We could have started deleting old events
after e.g. 4 weeks and kill pagination entirely. In hindsight though,
mistakes I made during implementation, such as the use of date-based
rather than cursor-based pagination, and the lack of webhooks or
another non-polling mechanism meant things couldn't _really_ work like
this. In addition, there's the series that aims to add an "actor" for
auditing purposes, meaning we probably should kill the idea of ever
deleting old events. So, overall, perhaps my original goal no longer
makes sense and we should just do this? Daniel - what are your
thoughts?

In any case, this unfortunately needs to be a little more complicated
than it is at the moment. Notes below.

> ---
>  patchwork/api/event.py                         |  2 +-
>  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
>  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> 
> diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> index c0d973d..e6d467d 100644
> --- a/patchwork/api/event.py
> +++ b/patchwork/api/event.py
> @@ -77,7 +77,7 @@ class EventList(ListAPIView):
>      serializer_class = EventSerializer
>      filter_class = filterset_class = EventFilterSet
>      page_size_query_param = None  # fixed page size
> -    ordering_fields = ()
> +    ordering_fields = ('date',)

This is going to apply to all API versions, from v1.0 to v1.2. However,
we actually want it to only apply to v1.2, just so API v1.0 behaves the
exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
I don't know if we've done versioning on fields before, but it should
be easy to override whatever method in 'ListAPIView' is responsible for
consuming 'ordering_field' from the querystring to ignore 'date' if API
version < 1.2. Let me know if you need help here.

>      ordering = '-date'
>  
>      def get_queryset(self):
> diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
> index 8816538..bff8f40 100644
> --- a/patchwork/tests/api/test_event.py
> +++ b/patchwork/tests/api/test_event.py
> @@ -149,6 +149,24 @@ class TestEventAPI(utils.APITestCase):
>          resp = self.client.get(self.api_url(), {'series': 999999})
>          self.assertEqual(0, len(resp.data))
>  
> +    def test_order_by_date_default(self):
> +        """Assert the default ordering is by date descending."""
> +        self._create_events()
> +
> +        resp = self.client.get(self.api_url())
> +        events = Event.objects.order_by("-date").all()
> +        for api_event, event in zip(resp.data, events):
> +            self.assertEqual(api_event["id"], event.id)
> +
> +    def test_order_by_date_ascending(self):
> +        """Assert the default ordering is by date descending."""
> +        self._create_events()
> +
> +        resp = self.client.get(self.api_url(), {'order': 'date'})
> +        events = Event.objects.order_by("date").all()
> +        for api_event, event in zip(resp.data, events):
> +            self.assertEqual(api_event["id"], event.id)
> +
>      def test_create(self):
>          """Ensure creates aren't allowed"""
>          user = create_maintainer()
> diff --git a/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> new file mode 100644
> index 0000000..5d5328d
> --- /dev/null
> +++ b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> @@ -0,0 +1,5 @@
> +---
> +features:

We have an 'api' section for this stuff which should be used here.

> +  - |
> +    Allow ordering events from the events API by date. This can be done by
> +    adding ``order=date`` or ``order=-date`` (the default) parameters.

On Thu, Oct 17, 2019 at 02:07:28PM +0100, Stephen Finucane wrote:
> On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> > By default, the events API orders events by date in descending order
> > (newest first). However, it's useful to be able to order the events by
> > oldest events first. For example, when a client is polling the events
> > API for new events since a given date and wishes to process them in
> > chronological order.
> > 
> > Signed-off-by: Jeremy Cline <jcline@redhat.com>
> 
> I'd purposefully avoided doing this initially because I wanted
> '/events' to be thought of as a firehose that should be just consumed
> as things were generated. We could have started deleting old events
> after e.g. 4 weeks and kill pagination entirely. In hindsight though,
> mistakes I made during implementation, such as the use of date-based
> rather than cursor-based pagination, and the lack of webhooks or
> another non-polling mechanism meant things couldn't _really_ work like
> this. In addition, there's the series that aims to add an "actor" for
> auditing purposes, meaning we probably should kill the idea of ever
> deleting old events. So, overall, perhaps my original goal no longer
> makes sense and we should just do this? Daniel - what are your
> thoughts?
> 

Interesting. To expand a little bit on why I want this, I'm writing a
mailing list <-> Git{Lab,Hub,Whatever} bridge. I'm just adding a Django
application that can run along side Patchwork to handle web hooks coming
from Git{Lab,Hub}, and toyed with the idea of just using a Django signal
to catch when incoming patch series are done, but opted to use this API
since that seemed like prone to breakage.

I ran into this particular chronological issue, but if this endpoint
isn't really intended to be used this way (or rather, folks don't want
this API to turn into that) I don't *need* this to do what I want.

> In any case, this unfortunately needs to be a little more complicated
> than it is at the moment. Notes below.
> 
> > ---
> >  patchwork/api/event.py                         |  2 +-
> >  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
> >  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> >  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > 
> > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > index c0d973d..e6d467d 100644
> > --- a/patchwork/api/event.py
> > +++ b/patchwork/api/event.py
> > @@ -77,7 +77,7 @@ class EventList(ListAPIView):
> >      serializer_class = EventSerializer
> >      filter_class = filterset_class = EventFilterSet
> >      page_size_query_param = None  # fixed page size
> > -    ordering_fields = ()
> > +    ordering_fields = ('date',)
> 
> This is going to apply to all API versions, from v1.0 to v1.2. However,
> we actually want it to only apply to v1.2, just so API v1.0 behaves the
> exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
> I don't know if we've done versioning on fields before, but it should
> be easy to override whatever method in 'ListAPIView' is responsible for
> consuming 'ordering_field' from the querystring to ignore 'date' if API
> version < 1.2. Let me know if you need help here.
> 

So, I'm happy to do this if that's what is required, but I must say I
don't see the value of it. This adds a completely optional query
parameter that defaults to the exact same thing it did before so the API
doesn't change unless the client is passing a bunch of nonsense
parameters that did nothing, but happened to include the ``order=date``
parameter. Since that's undocumented behavior I don't see this as
breaking anything.

Regardless, if that's what folks really want, that's what I'll do.

> >      ordering = '-date'
> >  
> >      def get_queryset(self):
> > diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
> > index 8816538..bff8f40 100644
> > --- a/patchwork/tests/api/test_event.py
> > +++ b/patchwork/tests/api/test_event.py
> > @@ -149,6 +149,24 @@ class TestEventAPI(utils.APITestCase):
> >          resp = self.client.get(self.api_url(), {'series': 999999})
> >          self.assertEqual(0, len(resp.data))
> >  
> > +    def test_order_by_date_default(self):
> > +        """Assert the default ordering is by date descending."""
> > +        self._create_events()
> > +
> > +        resp = self.client.get(self.api_url())
> > +        events = Event.objects.order_by("-date").all()
> > +        for api_event, event in zip(resp.data, events):
> > +            self.assertEqual(api_event["id"], event.id)
> > +
> > +    def test_order_by_date_ascending(self):
> > +        """Assert the default ordering is by date descending."""
> > +        self._create_events()
> > +
> > +        resp = self.client.get(self.api_url(), {'order': 'date'})
> > +        events = Event.objects.order_by("date").all()
> > +        for api_event, event in zip(resp.data, events):
> > +            self.assertEqual(api_event["id"], event.id)
> > +
> >      def test_create(self):
> >          """Ensure creates aren't allowed"""
> >          user = create_maintainer()
> > diff --git a/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > new file mode 100644
> > index 0000000..5d5328d
> > --- /dev/null
> > +++ b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > @@ -0,0 +1,5 @@
> > +---
> > +features:
> 
> We have an 'api' section for this stuff which should be used here.
> 

Ah, right.

> > +  - |
> > +    Allow ordering events from the events API by date. This can be done by
> > +    adding ``order=date`` or ``order=-date`` (the default) parameters.
> 


Thanks for the review!

- Jeremy

On Thu, 2019-10-17 at 09:35 -0400, Jeremy Cline wrote:
> On Thu, Oct 17, 2019 at 02:07:28PM +0100, Stephen Finucane wrote:
> > On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> > > By default, the events API orders events by date in descending order
> > > (newest first). However, it's useful to be able to order the events by
> > > oldest events first. For example, when a client is polling the events
> > > API for new events since a given date and wishes to process them in
> > > chronological order.
> > > 
> > > Signed-off-by: Jeremy Cline <jcline@redhat.com>
> > 
> > I'd purposefully avoided doing this initially because I wanted
> > '/events' to be thought of as a firehose that should be just consumed
> > as things were generated. We could have started deleting old events
> > after e.g. 4 weeks and kill pagination entirely. In hindsight though,
> > mistakes I made during implementation, such as the use of date-based
> > rather than cursor-based pagination, and the lack of webhooks or
> > another non-polling mechanism meant things couldn't _really_ work like
> > this. In addition, there's the series that aims to add an "actor" for
> > auditing purposes, meaning we probably should kill the idea of ever
> > deleting old events. So, overall, perhaps my original goal no longer
> > makes sense and we should just do this? Daniel - what are your
> > thoughts?
> > 
> 
> Interesting. To expand a little bit on why I want this, I'm writing a
> mailing list <-> Git{Lab,Hub,Whatever} bridge. I'm just adding a Django
> application that can run along side Patchwork to handle web hooks coming
> from Git{Lab,Hub}, and toyed with the idea of just using a Django signal
> to catch when incoming patch series are done, but opted to use this API
> since that seemed like prone to breakage.
> 
> I ran into this particular chronological issue, but if this endpoint
> isn't really intended to be used this way (or rather, folks don't want
> this API to turn into that) I don't *need* this to do what I want.

To be clear, I'm very much sitting on the fence about this rn and am
looking for input from others so I can get off said fence. It's
certainly not a definite no yet :)

> > In any case, this unfortunately needs to be a little more complicated
> > than it is at the moment. Notes below.
> > 
> > > ---
> > >  patchwork/api/event.py                         |  2 +-
> > >  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
> > >  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
> > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > >  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > 
> > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > > index c0d973d..e6d467d 100644
> > > --- a/patchwork/api/event.py
> > > +++ b/patchwork/api/event.py
> > > @@ -77,7 +77,7 @@ class EventList(ListAPIView):
> > >      serializer_class = EventSerializer
> > >      filter_class = filterset_class = EventFilterSet
> > >      page_size_query_param = None  # fixed page size
> > > -    ordering_fields = ()
> > > +    ordering_fields = ('date',)
> > 
> > This is going to apply to all API versions, from v1.0 to v1.2. However,
> > we actually want it to only apply to v1.2, just so API v1.0 behaves the
> > exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
> > I don't know if we've done versioning on fields before, but it should
> > be easy to override whatever method in 'ListAPIView' is responsible for
> > consuming 'ordering_field' from the querystring to ignore 'date' if API
> > version < 1.2. Let me know if you need help here.
> > 
> 
> So, I'm happy to do this if that's what is required, but I must say I
> don't see the value of it. This adds a completely optional query
> parameter that defaults to the exact same thing it did before so the API
> doesn't change unless the client is passing a bunch of nonsense
> parameters that did nothing, but happened to include the ``order=date``
> parameter. Since that's undocumented behavior I don't see this as
> breaking anything.

It's not so much that behavior will suddenly change behind people's
back, but rather avoiding confusion where this feature worked on one
instance but does nothing on another despite using the same API
version. I want to say "if you use API 1.2, this works and, if not, it
doesn't", rather than "this works on API 1.2 and also on other versions
but only if you use this PATCH version of Patchwork, which oh by the
way isn't discoverable via the API itself".

With that said, I haven't actually checked to see just how much effort
is involved here so it could be stupid big. If so, we can think about
ignoring it. I do think I'd like to try though, if that's okay?

Stephen

> Regardless, if that's what folks really want, that's what I'll do.
> 
> > >      ordering = '-date'
> > >  
> > >      def get_queryset(self):
> > > diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
> > > index 8816538..bff8f40 100644
> > > --- a/patchwork/tests/api/test_event.py
> > > +++ b/patchwork/tests/api/test_event.py
> > > @@ -149,6 +149,24 @@ class TestEventAPI(utils.APITestCase):
> > >          resp = self.client.get(self.api_url(), {'series': 999999})
> > >          self.assertEqual(0, len(resp.data))
> > >  
> > > +    def test_order_by_date_default(self):
> > > +        """Assert the default ordering is by date descending."""
> > > +        self._create_events()
> > > +
> > > +        resp = self.client.get(self.api_url())
> > > +        events = Event.objects.order_by("-date").all()
> > > +        for api_event, event in zip(resp.data, events):
> > > +            self.assertEqual(api_event["id"], event.id)
> > > +
> > > +    def test_order_by_date_ascending(self):
> > > +        """Assert the default ordering is by date descending."""
> > > +        self._create_events()
> > > +
> > > +        resp = self.client.get(self.api_url(), {'order': 'date'})
> > > +        events = Event.objects.order_by("date").all()
> > > +        for api_event, event in zip(resp.data, events):
> > > +            self.assertEqual(api_event["id"], event.id)
> > > +
> > >      def test_create(self):
> > >          """Ensure creates aren't allowed"""
> > >          user = create_maintainer()
> > > diff --git a/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > new file mode 100644
> > > index 0000000..5d5328d
> > > --- /dev/null
> > > +++ b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > @@ -0,0 +1,5 @@
> > > +---
> > > +features:
> > 
> > We have an 'api' section for this stuff which should be used here.
> > 
> 
> Ah, right.
> 
> > > +  - |
> > > +    Allow ordering events from the events API by date. This can be done by
> > > +    adding ``order=date`` or ``order=-date`` (the default) parameters.
> 
> Thanks for the review!
> 
> - Jeremy
>

On Thu, Oct 17, 2019 at 03:09:16PM +0100, Stephen Finucane wrote:
> On Thu, 2019-10-17 at 09:35 -0400, Jeremy Cline wrote:
> > On Thu, Oct 17, 2019 at 02:07:28PM +0100, Stephen Finucane wrote:
> > > On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> > > > By default, the events API orders events by date in descending order
> > > > (newest first). However, it's useful to be able to order the events by
> > > > oldest events first. For example, when a client is polling the events
> > > > API for new events since a given date and wishes to process them in
> > > > chronological order.
> > > > 
> > > > Signed-off-by: Jeremy Cline <jcline@redhat.com>
> > > 
> > > I'd purposefully avoided doing this initially because I wanted
> > > '/events' to be thought of as a firehose that should be just consumed
> > > as things were generated. We could have started deleting old events
> > > after e.g. 4 weeks and kill pagination entirely. In hindsight though,
> > > mistakes I made during implementation, such as the use of date-based
> > > rather than cursor-based pagination, and the lack of webhooks or
> > > another non-polling mechanism meant things couldn't _really_ work like
> > > this. In addition, there's the series that aims to add an "actor" for
> > > auditing purposes, meaning we probably should kill the idea of ever
> > > deleting old events. So, overall, perhaps my original goal no longer
> > > makes sense and we should just do this? Daniel - what are your
> > > thoughts?
> > > 
> > 
> > Interesting. To expand a little bit on why I want this, I'm writing a
> > mailing list <-> Git{Lab,Hub,Whatever} bridge. I'm just adding a Django
> > application that can run along side Patchwork to handle web hooks coming
> > from Git{Lab,Hub}, and toyed with the idea of just using a Django signal
> > to catch when incoming patch series are done, but opted to use this API
> > since that seemed like prone to breakage.
> > 
> > I ran into this particular chronological issue, but if this endpoint
> > isn't really intended to be used this way (or rather, folks don't want
> > this API to turn into that) I don't *need* this to do what I want.
> 
> To be clear, I'm very much sitting on the fence about this rn and am
> looking for input from others so I can get off said fence. It's
> certainly not a definite no yet :)
> 

Yeah, I mostly provided that so it was clear that it was just a
nice-to-have for me.

> > > In any case, this unfortunately needs to be a little more complicated
> > > than it is at the moment. Notes below.
> > > 
> > > > ---
> > > >  patchwork/api/event.py                         |  2 +-
> > > >  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
> > > >  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
> > > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > > >  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > > 
> > > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > > > index c0d973d..e6d467d 100644
> > > > --- a/patchwork/api/event.py
> > > > +++ b/patchwork/api/event.py
> > > > @@ -77,7 +77,7 @@ class EventList(ListAPIView):
> > > >      serializer_class = EventSerializer
> > > >      filter_class = filterset_class = EventFilterSet
> > > >      page_size_query_param = None  # fixed page size
> > > > -    ordering_fields = ()
> > > > +    ordering_fields = ('date',)
> > > 
> > > This is going to apply to all API versions, from v1.0 to v1.2. However,
> > > we actually want it to only apply to v1.2, just so API v1.0 behaves the
> > > exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
> > > I don't know if we've done versioning on fields before, but it should
> > > be easy to override whatever method in 'ListAPIView' is responsible for
> > > consuming 'ordering_field' from the querystring to ignore 'date' if API
> > > version < 1.2. Let me know if you need help here.
> > > 
> > 
> > So, I'm happy to do this if that's what is required, but I must say I
> > don't see the value of it. This adds a completely optional query
> > parameter that defaults to the exact same thing it did before so the API
> > doesn't change unless the client is passing a bunch of nonsense
> > parameters that did nothing, but happened to include the ``order=date``
> > parameter. Since that's undocumented behavior I don't see this as
> > breaking anything.
> 
> It's not so much that behavior will suddenly change behind people's
> back, but rather avoiding confusion where this feature worked on one
> instance but does nothing on another despite using the same API
> version. I want to say "if you use API 1.2, this works and, if not, it
> doesn't", rather than "this works on API 1.2 and also on other versions
> but only if you use this PATCH version of Patchwork, which oh by the
> way isn't discoverable via the API itself".
> 
> With that said, I haven't actually checked to see just how much effort
> is involved here so it could be stupid big. If so, we can think about
> ignoring it. I do think I'd like to try though, if that's okay?
> 

Ah, okay. Yeah, I don't think it'll be terribly hard, it just means more
tests and so on. It seems to me that the "real" problem is that there's
not an endpoint to retrieve the server patchwork version from? Or a
header with the version included, or something.

Like I said, I don't mind doing it, it just seemed like an odd thing to
do and I've never seen a REST API do v1.y versions. I'm still in the
"it's an odd thing to do" camp, but there's a reason for it so that's
okay.

If I get the itch to do some Patchwork work, would you welcome such an
introspection API? And totally unrelated to this patch, would you accept
a setup.py to make Patchwork installable with pip/setuptools?

- Jeremy

On Thu, 2019-10-17 at 14:19 +0000, Jeremy Cline wrote:
> On Thu, Oct 17, 2019 at 03:09:16PM +0100, Stephen Finucane wrote:
> > On Thu, 2019-10-17 at 09:35 -0400, Jeremy Cline wrote:
> > > On Thu, Oct 17, 2019 at 02:07:28PM +0100, Stephen Finucane wrote:
> > > > On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> > > > > By default, the events API orders events by date in descending order
> > > > > (newest first). However, it's useful to be able to order the events by
> > > > > oldest events first. For example, when a client is polling the events
> > > > > API for new events since a given date and wishes to process them in
> > > > > chronological order.
> > > > > 
> > > > > Signed-off-by: Jeremy Cline <jcline@redhat.com>
> > > > 
> > > > I'd purposefully avoided doing this initially because I wanted
> > > > '/events' to be thought of as a firehose that should be just consumed
> > > > as things were generated. We could have started deleting old events
> > > > after e.g. 4 weeks and kill pagination entirely. In hindsight though,
> > > > mistakes I made during implementation, such as the use of date-based
> > > > rather than cursor-based pagination, and the lack of webhooks or
> > > > another non-polling mechanism meant things couldn't _really_ work like
> > > > this. In addition, there's the series that aims to add an "actor" for
> > > > auditing purposes, meaning we probably should kill the idea of ever
> > > > deleting old events. So, overall, perhaps my original goal no longer
> > > > makes sense and we should just do this? Daniel - what are your
> > > > thoughts?
> > > > 
> > > 
> > > Interesting. To expand a little bit on why I want this, I'm writing a
> > > mailing list <-> Git{Lab,Hub,Whatever} bridge. I'm just adding a Django
> > > application that can run along side Patchwork to handle web hooks coming
> > > from Git{Lab,Hub}, and toyed with the idea of just using a Django signal
> > > to catch when incoming patch series are done, but opted to use this API
> > > since that seemed like prone to breakage.
> > > 
> > > I ran into this particular chronological issue, but if this endpoint
> > > isn't really intended to be used this way (or rather, folks don't want
> > > this API to turn into that) I don't *need* this to do what I want.
> > 
> > To be clear, I'm very much sitting on the fence about this rn and am
> > looking for input from others so I can get off said fence. It's
> > certainly not a definite no yet :)
> > 
> 
> Yeah, I mostly provided that so it was clear that it was just a
> nice-to-have for me.
> 
> > > > In any case, this unfortunately needs to be a little more complicated
> > > > than it is at the moment. Notes below.
> > > > 
> > > > > ---
> > > > >  patchwork/api/event.py                         |  2 +-
> > > > >  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
> > > > >  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
> > > > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > > > 
> > > > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > > > > index c0d973d..e6d467d 100644
> > > > > --- a/patchwork/api/event.py
> > > > > +++ b/patchwork/api/event.py
> > > > > @@ -77,7 +77,7 @@ class EventList(ListAPIView):
> > > > >      serializer_class = EventSerializer
> > > > >      filter_class = filterset_class = EventFilterSet
> > > > >      page_size_query_param = None  # fixed page size
> > > > > -    ordering_fields = ()
> > > > > +    ordering_fields = ('date',)
> > > > 
> > > > This is going to apply to all API versions, from v1.0 to v1.2. However,
> > > > we actually want it to only apply to v1.2, just so API v1.0 behaves the
> > > > exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
> > > > I don't know if we've done versioning on fields before, but it should
> > > > be easy to override whatever method in 'ListAPIView' is responsible for
> > > > consuming 'ordering_field' from the querystring to ignore 'date' if API
> > > > version < 1.2. Let me know if you need help here.
> > > > 
> > > 
> > > So, I'm happy to do this if that's what is required, but I must say I
> > > don't see the value of it. This adds a completely optional query
> > > parameter that defaults to the exact same thing it did before so the API
> > > doesn't change unless the client is passing a bunch of nonsense
> > > parameters that did nothing, but happened to include the ``order=date``
> > > parameter. Since that's undocumented behavior I don't see this as
> > > breaking anything.
> > 
> > It's not so much that behavior will suddenly change behind people's
> > back, but rather avoiding confusion where this feature worked on one
> > instance but does nothing on another despite using the same API
> > version. I want to say "if you use API 1.2, this works and, if not, it
> > doesn't", rather than "this works on API 1.2 and also on other versions
> > but only if you use this PATCH version of Patchwork, which oh by the
> > way isn't discoverable via the API itself".
> > 
> > With that said, I haven't actually checked to see just how much effort
> > is involved here so it could be stupid big. If so, we can think about
> > ignoring it. I do think I'd like to try though, if that's okay?
> > 
> 
> Ah, okay. Yeah, I don't think it'll be terribly hard, it just means more
> tests and so on. It seems to me that the "real" problem is that there's
> not an endpoint to retrieve the server patchwork version from? Or a
> header with the version included, or something.

It's less that an more that our API versions are separated from our
main version, so we need to do things to ensure behavior in the former
is consistent across multiple releases of the latter.

> Like I said, I don't mind doing it, it just seemed like an odd thing to
> do and I've never seen a REST API do v1.y versions. I'm still in the
> "it's an odd thing to do" camp, but there's a reason for it so that's
> okay.
> 
> If I get the itch to do some Patchwork work, would you welcome such an
> introspection API? And totally unrelated to this patch, would you accept
> a setup.py to make Patchwork installable with pip/setuptools?

For the former, sure. I've considered exposing an 'X-Patchwork-API-
Version' header in responses previously, so doing something similar but
for server version would probably make sense. For the latter, we can do
that but we will need to shuffle some things around before we do. To
the best of my knowledge, distributable Django applications shouldn't
include things like a 'manage.py' file. Mailman [1], with its
'example_project' folder, is probably a good template to follow. I also
think this should wait until 3.0 rather than 2.2 since it will require
a few changes to deployment tooling people have built up.

Stephen

[1] https://gitlab.com/mailman/hyperkitty/

> - Jeremy

Stephen Finucane <stephen@that.guru> writes:

> On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
>> By default, the events API orders events by date in descending order
>> (newest first). However, it's useful to be able to order the events by
>> oldest events first. For example, when a client is polling the events
>> API for new events since a given date and wishes to process them in
>> chronological order.
>> 
>> Signed-off-by: Jeremy Cline <jcline@redhat.com>
>
> I'd purposefully avoided doing this initially because I wanted
> '/events' to be thought of as a firehose that should be just consumed
> as things were generated. We could have started deleting old events
> after e.g. 4 weeks and kill pagination entirely. In hindsight though,
> mistakes I made during implementation, such as the use of date-based
> rather than cursor-based pagination, and the lack of webhooks or
> another non-polling mechanism meant things couldn't _really_ work like
> this. In addition, there's the series that aims to add an "actor" for
> auditing purposes, meaning we probably should kill the idea of ever
> deleting old events. So, overall, perhaps my original goal no longer
> makes sense and we should just do this? Daniel - what are your
> thoughts?

I think date ordering is probably fine. We can try again for
cursor-based pagination and so on in a future version of the API.

FWIW, I think having an undocumented field in old versions of the API is
probably not the end of the world.

Regards,
Daniel

> In any case, this unfortunately needs to be a little more complicated
> than it is at the moment. Notes below.
>
>> ---
>>  patchwork/api/event.py                         |  2 +-
>>  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
>>  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
>>  3 files changed, 24 insertions(+), 1 deletion(-)
>>  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
>> 
>> diff --git a/patchwork/api/event.py b/patchwork/api/event.py
>> index c0d973d..e6d467d 100644
>> --- a/patchwork/api/event.py
>> +++ b/patchwork/api/event.py
>> @@ -77,7 +77,7 @@ class EventList(ListAPIView):
>>      serializer_class = EventSerializer
>>      filter_class = filterset_class = EventFilterSet
>>      page_size_query_param = None  # fixed page size
>> -    ordering_fields = ()
>> +    ordering_fields = ('date',)
>
> This is going to apply to all API versions, from v1.0 to v1.2. However,
> we actually want it to only apply to v1.2, just so API v1.0 behaves the
> exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
> I don't know if we've done versioning on fields before, but it should
> be easy to override whatever method in 'ListAPIView' is responsible for
> consuming 'ordering_field' from the querystring to ignore 'date' if API
> version < 1.2. Let me know if you need help here.
>
>>      ordering = '-date'
>>  
>>      def get_queryset(self):
>> diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
>> index 8816538..bff8f40 100644
>> --- a/patchwork/tests/api/test_event.py
>> +++ b/patchwork/tests/api/test_event.py
>> @@ -149,6 +149,24 @@ class TestEventAPI(utils.APITestCase):
>>          resp = self.client.get(self.api_url(), {'series': 999999})
>>          self.assertEqual(0, len(resp.data))
>>  
>> +    def test_order_by_date_default(self):
>> +        """Assert the default ordering is by date descending."""
>> +        self._create_events()
>> +
>> +        resp = self.client.get(self.api_url())
>> +        events = Event.objects.order_by("-date").all()
>> +        for api_event, event in zip(resp.data, events):
>> +            self.assertEqual(api_event["id"], event.id)
>> +
>> +    def test_order_by_date_ascending(self):
>> +        """Assert the default ordering is by date descending."""
>> +        self._create_events()
>> +
>> +        resp = self.client.get(self.api_url(), {'order': 'date'})
>> +        events = Event.objects.order_by("date").all()
>> +        for api_event, event in zip(resp.data, events):
>> +            self.assertEqual(api_event["id"], event.id)
>> +
>>      def test_create(self):
>>          """Ensure creates aren't allowed"""
>>          user = create_maintainer()
>> diff --git a/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
>> new file mode 100644
>> index 0000000..5d5328d
>> --- /dev/null
>> +++ b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
>> @@ -0,0 +1,5 @@
>> +---
>> +features:
>
> We have an 'api' section for this stuff which should be used here.
>
>> +  - |
>> +    Allow ordering events from the events API by date. This can be done by
>> +    adding ``order=date`` or ``order=-date`` (the default) parameters.

On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> By default, the events API orders events by date in descending order
> (newest first). However, it's useful to be able to order the events by
> oldest events first. For example, when a client is polling the events
> API for new events since a given date and wishes to process them in
> chronological order.
> 
> Signed-off-by: Jeremy Cline <jcline@redhat.com>

I've gone ahead and applied this. It was possible to ignore the 'order'
key by subclassing 'rest_framework.filters.OrderingFilter' but that
change invovled way more boilerplate than I was comfortable with.
Thanks for the change and sorry it took so long to merge.

Stephen