[PATCH] Allow ordering events by date

[Jeremy Cline]

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 at 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