Orderable list mixin #45

Merged
merged 20 commits into from Jul 18, 2013

Projects

None yet

3 participants

@centralniak
Contributor

Hey django-braces maintainers!

I've added a mixin that allows easy ordering of the queryset basing on the GET params that I found handy for one of my projects.

The concept is explained in the docs; the addition is also covered by the unit tests.

Hoping you'll find it good enough to be a part of django-braces!

Cheers,
Piotr

@kennethlove kennethlove commented on an outdated diff May 22, 2013
braces/views.py
+ order_by = None
+ ordering = None
+
+ def get_context_data(self, **kwargs):
+ """
+ Augments context with:
+
+ * ``order_by`` - name of the field
+ * ``ordering`` - order of ordering, either ``asc`` or ``desc``
+ """
+ ctx = super(OrderableListMixin, self).get_context_data(**kwargs)
+ ctx['order_by'] = self.order_by
+ ctx['ordering'] = self.ordering
+ return ctx
+
+ def get_ordered_queryset(self, qs=None):
@kennethlove
kennethlove May 22, 2013 Member

Use queryset, not qs.

@kennethlove kennethlove and 1 other commented on an outdated diff May 22, 2013
braces/views.py
+ * ``order_by`` - name of the field
+ * ``ordering`` - order of ordering, either ``asc`` or ``desc``
+ """
+ ctx = super(OrderableListMixin, self).get_context_data(**kwargs)
+ ctx['order_by'] = self.order_by
+ ctx['ordering'] = self.ordering
+ return ctx
+
+ def get_ordered_queryset(self, qs=None):
+ """
+ Augments ``QuerySet`` with order_by statement if possible
+
+ :param QuerySet qs: ``QuerySet`` to ``order_by``
+ :return: QuerySet
+ """
+ if self.request.GET.get('order_by', None) \
@kennethlove
kennethlove May 22, 2013 Member

Try not to continue lines with \. It's ugly and most lines can be continued in containers. This one, you could set self.request.GET.get("order_by", None) to a variable and then check that.

Also, notice we've used " through the entire file, not '. Please match our style.

@chrisjones-brack3t
chrisjones-brack3t May 22, 2013 Member

Also, the default on a .get is to return None, so unless you need to return something other than None it does not need to be specified.

@kennethlove kennethlove commented on an outdated diff May 22, 2013
braces/views.py
+ """
+ ctx = super(OrderableListMixin, self).get_context_data(**kwargs)
+ ctx['order_by'] = self.order_by
+ ctx['ordering'] = self.ordering
+ return ctx
+
+ def get_ordered_queryset(self, qs=None):
+ """
+ Augments ``QuerySet`` with order_by statement if possible
+
+ :param QuerySet qs: ``QuerySet`` to ``order_by``
+ :return: QuerySet
+ """
+ if self.request.GET.get('order_by', None) \
+ in self.model.Orderable.columns:
+ order_by = self.request.GET.get('order_by')
@kennethlove
kennethlove May 22, 2013 Member

Having it as a variable saves you from fetching it twice.

@kennethlove kennethlove commented on an outdated diff May 22, 2013
braces/views.py
+ return ctx
+
+ def get_ordered_queryset(self, qs=None):
+ """
+ Augments ``QuerySet`` with order_by statement if possible
+
+ :param QuerySet qs: ``QuerySet`` to ``order_by``
+ :return: QuerySet
+ """
+ if self.request.GET.get('order_by', None) \
+ in self.model.Orderable.columns:
+ order_by = self.request.GET.get('order_by')
+ elif getattr(self.model.Orderable, 'default', None):
+ order_by = self.model.Orderable.default
+ else:
+ raise ImproperlyConfigured\
@kennethlove
kennethlove May 22, 2013 Member

Again with the \.

@kennethlove kennethlove and 1 other commented on an outdated diff May 22, 2013
braces/views.py
+ ('Please define default ordering column in your model')
+
+ self.order_by = order_by
+ self.ordering = 'asc'
+
+ if order_by and self.request.GET.get('ordering', 'asc') == 'desc':
+ order_by = '-' + order_by
+ self.ordering = 'desc'
+
+ return qs.order_by(order_by)
+
+ def get_queryset(self):
+ """
+ Returns ordered ``QuerySet``
+ """
+ return self.get_ordered_queryset(super(OrderableListMixin, self).
@kennethlove
kennethlove May 22, 2013 Member

I'm not sure exactly why, but this bothers me. I'd almost rather see you get the default queryset and then return the results of the method on that queryset, explicitly as two steps.

@centralniak
centralniak May 23, 2013 Contributor

Fine, what do you think of 792b69a ?

@kennethlove kennethlove commented on an outdated diff May 22, 2013
braces/views.py
+class OrderableListMixin(object):
+ """
+ Mixin allows your users to order records using GET parameters
+ """
+
+ order_by = None
+ ordering = None
+
+ def get_context_data(self, **kwargs):
+ """
+ Augments context with:
+
+ * ``order_by`` - name of the field
+ * ``ordering`` - order of ordering, either ``asc`` or ``desc``
+ """
+ ctx = super(OrderableListMixin, self).get_context_data(**kwargs)
@kennethlove
kennethlove May 22, 2013 Member

context, please, not ctx.

@kennethlove
Member

I like the idea of this mixin but there are a couple of things, aside from style, that I have a problem with.

I don't like that it requires a model change that doesn't actually affect the model. Why should I change my model for a view constraint? Seems like it would be smarter to have both an orderable_fields and orderable_default attributes on the view mixin, much like Django's standard View class gives http_method_names (http://ccbv.co.uk/projects/Django/1.5/django.views.generic.base/View/) and then check against those.

@centralniak
Contributor

WOW, thanks guys for the feedback. Will get through it tomorrow, fix the style and come back to you.

@centralniak
Contributor

Work in progress... ;-)

@centralniak
Contributor

OK, in b4299cb it uses view attributes,
rather than model subclass. I can rename them if you want.
Actually my idea was, that these ordering attributes on the model could
be shared among many views, but they can be just as well using another
mixin if needed. Another argument for it was that Django stores default
ordering in the meta class - but that rather affects whole the ORM, not
the view layer.

Let me know what you think.

Cheers,
Piotr

@kennethlove kennethlove commented on an outdated diff May 31, 2013
braces/views.py
+ """
+ context = super(OrderableListMixin, self).get_context_data(**kwargs)
+ context["order_by"] = self.order_by
+ context["ordering"] = self.ordering
+ return context
+
+ def get_ordered_queryset(self, queryset=None):
+ """
+ Augments ``QuerySet`` with order_by statement if possible
+
+ :param QuerySet queryset: ``QuerySet`` to ``order_by``
+ :return: QuerySet
+ """
+ get_order_by = self.request.GET.get("order_by")
+
+ if get_order_by in self.orderable_columns:
@kennethlove
kennethlove May 31, 2013 Member

orderable_columns should be an attribute on the model that is set to None by default and there should be a get_orderable_columns method that returns the attribute by default and throws an exception if it's still set to None. The reasoning behind this is that I might want to programmatically define my orderable columns instead of hard coding them.

@kennethlove kennethlove and 1 other commented on an outdated diff May 31, 2013
braces/views.py
+ context["order_by"] = self.order_by
+ context["ordering"] = self.ordering
+ return context
+
+ def get_ordered_queryset(self, queryset=None):
+ """
+ Augments ``QuerySet`` with order_by statement if possible
+
+ :param QuerySet queryset: ``QuerySet`` to ``order_by``
+ :return: QuerySet
+ """
+ get_order_by = self.request.GET.get("order_by")
+
+ if get_order_by in self.orderable_columns:
+ order_by = get_order_by
+ elif getattr(self, "orderable_columns_default", None):
@kennethlove
kennethlove May 31, 2013 Member

Same as above.

@centralniak
centralniak Jun 4, 2013 Contributor

Yep, makes sense. Will do this soon.

@kennethlove
Member

Sorry for the delay in time. I'll be testing this soon and, if it succeeds, I'll merge it in. Thanks for all your hard work!

@kennethlove kennethlove merged commit cf0ac83 into brack3t:master Jul 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment