Opened 5 years ago

Closed 2 years ago

#18355 closed New feature (fixed)

Add ordering mixin for class based generic views

Reported by: Aymeric Augustin Owned by: pjrharley
Component: Generic views Version: master
Severity: Normal Keywords:
Cc: marc.tamlyn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, if the model used by by a date-based generic view doesn't have an ordering and pagination is enabled, results will be random. Defining an ordering on the model isn't totally satisfying because it has global effects, and it adds an overhead to every query involving the model.

Another workaround is to define queryset = MyModel.objects.order_by('-<date_field>') in every view. But CBVs are precisely about removing as much boilerplate as possible.

So here's a draft of an API for ordering:

class BaseDateListView(...):

    ordering = None

    def get_ordering(self):
        return self.get_date_field() if self.ordering is None else self.ordering
            
    def get_dated_queryset(self, **lookups):

    ...

    if not qs.ordered():
        qs = qs.order_by(self.get_ordering())

    ...

self.get_ordering() could return '-<date_field>' for the archive view for backwards compatibility.

This would be a better fix for #18354.

Change History (12)

comment:1 Changed 5 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

comment:2 Changed 5 years ago by Jannis Leidel

Triage Stage: UnreviewedAccepted

I accept the problem in general, but don't see how this is limited to date based views. This is perfect for a mixin, so let's write a OrderingViewMixin or similar.

comment:3 Changed 5 years ago by Jannis Leidel

Summary: Date-based generic views should enforce orderingAdd ordering mixin for class based generic views

comment:4 Changed 4 years ago by Marc Tamlyn

Cc: marc.tamlyn@… added

Would this functionality apply only to list views? This would certainly give a good advantage as one's instinct is to apply the get_queryset changes to every view with the same Model, but ordering is rather useless when we're simply doing a DetailView (and could impose a performance problem). So the code would perhaps live in BaseListView rather than MultipleObjectMixin.

That said, if this functionality is only hitting list views, it actually fits in with a larger subject area of search/ordering/filtering as defined by the user. In reality this is probably just one of the first hooks we might need to implement that kind of functionality well, but it may be worth considering the larger list-view-customisation issue.

comment:5 Changed 4 years ago by Aymeric Augustin

The goals were clear to me when I was working on the date-based views, but unfortunately I've lost track of it since then...

I think it's just about implementing the API in the ticket description, documenting it, and taking advantage of it in the date-based views while preserving backwards-compat.

You're very welcome to take over on this ticket if you'd like to as it's pretty low on my priority list.

comment:6 Changed 3 years ago by Aymeric Augustin

Status: newassigned

comment:7 Changed 3 years ago by Aymeric Augustin

Owner: Aymeric Augustin deleted
Status: assignednew

comment:8 Changed 3 years ago by anonymous

Has patch: set
Owner: set to anonymous
Status: newassigned

I've added a PR for this here:

https://github.com/django/django/pull/2097

Not sure if the approach is right - feedback welcome. I suspect the docs need improvement - more than happy to do that if the code is looking good.

comment:9 Changed 3 years ago by pjrharley

Owner: changed from anonymous to pjrharley

Try assigning to myself while signed in instead...

comment:10 Changed 3 years ago by Tim Graham

Patch needs improvement: set

I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.

comment:11 Changed 2 years ago by pjrharley

Patch needs improvement: unset

Patch updated. Thanks for the review.

comment:12 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 2724cdbff635a40819f206411de23e9b14867a58:

Fixed #18355 -- Added ordering options to list based generic views.

Added MultipleObjectMixin.ordering and get_ordering().

Refs #21450.

Note: See TracTickets for help on using tickets.
Back to Top