Multi-column sorting order ignores some columns and overwrites order_by values

[boatcoder]

There are 2 problems with the current implementation and I think my changes fix both of them, but there are some parts of the code I'm not 100% sure of.

You can make methods:
def order_FOO(self, queryset, is_descending):
....

def order_FOO(self, queryset, is_descending):
....

Then if you order_by('FOO', 'ordinary_field', 'BAR') your order_FOO and order_BARmethods will both get called but they will replace the sort order of the preceding methods because django'sorder_by` nukes the order list every time it is called.

I think a better way to do this would be to build a list of orderables and then call order_by ONCE with that list vs having each order_XYZ method update the queryset.order_by and return True if they do.

I've just run into this and ended up doing some shenanigans to get it to work where I grab the existing queryset.query.order_by tuple and concat it with a new value so that in the end the order_by is correct but this seems wrong and is very prone to unintentional side effects (I think).

The second problem I just ran into was mixing order_FOO and regular field names. In that case the only sorting is done on the order_FOO functions and the regular field names are ignored in the sorting because Column.order returns False for them, so they are not added to the sort (the problem mentioned above makes this impossible to see until you work around maintaining the existing sort order as you add more sorting. My solution seems to fix both of the problems but it could use some more experienced eyes on a review. If this kind of fix would be something you would consider, I'll build a PR for it. I haven't decided if I need to fork the repo and put in my fix or just leave my workaround. Both of them require extra effort but if you are ok with the fix then I can just make a PR for you.

My change unfortunately breaks the documentation and the def order_FOO api but that is unavoidable since the existing API doesn't really work.

def order_do_not_renew(self, queryset, is_descending):
        queryset = queryset.order_by(
            *(queryset.query.order_by + (("-" if is_descending else "") + "do_not_renew",))
        )
        return (queryset, True)

def order_paid_until(self, queryset, is_descending):
    paid_until = OrderBy(F("paid_until"), descending=is_descending, nulls_last=True)
    queryset = queryset.order_by(*(queryset.query.order_by + (paid_until,)))
    return (queryset, True)

This is the method where the problem lies:

def order_by(self, aliases):
        """
        Order the data based on order by aliases (prefixed column names) in the
        table.
        Arguments:
            aliases (`~.utils.OrderByTuple`): optionally prefixed names of
                columns ('-' indicates descending order) in order of
                significance with regard to data ordering.
        """
        modified_any = False
        accessors = []
        for alias in aliases:
            bound_column = self.table.columns[OrderBy(alias).bare]
            # bound_column.order_by reflects the current ordering applied to
            # the table. As such we need to check the current ordering on the
            # column and use the opposite if it doesn't match the alias prefix.
            if alias[0] != bound_column.order_by_alias[0]:
                accessors += bound_column.order_by.opposite
            else:
                accessors += bound_column.order_by

            if bound_column:
                queryset, modified = bound_column.order(self.data, alias[0] == "-") # This is where the bug is!

                if modified:
                    self.data = queryset
                    modified_any = True

        # custom ordering
        if modified_any:
            return

        # Traditional ordering
        if accessors:
            order_by_accessors = (a.for_queryset() for a in accessors)
            self.data = self.data.order_by(*order_by_accessors)

I think it should look more like this:

    def order_by(self, aliases):
        """
        Order the data based on order by aliases (prefixed column names) in the
        table.

        Arguments:
            aliases (`~.utils.OrderByTuple`): optionally prefixed names of
                columns ('-' indicates descending order) in order of
                significance with regard to data ordering.
        """
        accessors = []
        orderings = tuple()
        for alias in aliases:
            bound_column = self.table.columns[OrderBy(alias).bare]
            # bound_column.order_by reflects the current ordering applied to
            # the table. As such we need to check the current ordering on the
            # column and use the opposite if it doesn't match the alias prefix.
            if alias[0] != bound_column.order_by_alias[0]:
                accessors += bound_column.order_by.opposite
            else:
                accessors += bound_column.order_by

            if bound_column:
                # We have to pass queryset because ordering could be on an annotation that was
                #  added for sorting
                self.data, ordering = bound_column.order(self.data, alias[0] == "-")

                orderings = orderings + (ordering,)

        # custom ordering
        if orderings:
            self.data = self.data.order_by(*orderings)
            return

        # Traditional ordering
        if accessors:
            order_by_accessors = (a.for_queryset() for a in accessors)
            self.data = self.data.order_by(*order_by_accessors)

And the Column class needs this change:

    def order(self, queryset, is_descending):
        """
        Order the QuerySet of the table.

        This method can be overridden by :ref:`table.order_FOO` methods on the
        table or by subclassing `.Column`; but only overrides if second element
        in return tuple is True.

        returns:
            Tuple (QuerySet, boolean)
        """
        from django.db.models import F, OrderBy

        if self.order_by or self.accessor:  # TODO: self.orderable
            return queryset, OrderBy(F(self.order_by or self.accessor), descending=is_descending)

        return queryset, None

[boatcoder]

Built a PR just to make it easier to understand, and like I said I'm not 100% confident of all the ramifications (and I'm very fuzzy about Column.orderable defaulting to None......