Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ordering by custom field raises an error #413

Closed
deltadanger opened this issue Feb 8, 2017 · 4 comments · Fixed by mozilla/addons-server#4783 or drummonds/bene#50
Closed

Ordering by custom field raises an error #413

deltadanger opened this issue Feb 8, 2017 · 4 comments · Fixed by mozilla/addons-server#4783 or drummonds/bene#50

Comments

@deltadanger
Copy link

When defining a custom field in a table, as name = tables.Column() with methods to render and order render_name and order_name, sorting by this column causes an error if the custom field is not in last position:

  • Viewing /mytable?sort=name works
  • Viewing /mytable?sort=email&sort=name works
  • Viewing /mytable?sort=name&sort=email raises a FieldError "Cannot resolve keyword u'name' into field"

The cause of this problem is at https://github.com/bradleyayers/django-tables2/blob/master/django_tables2/tables.py#L108, this code is only called for the last column, whereas it should be checked for all columns.

@jieter
Copy link
Owner

jieter commented Feb 8, 2017

Thanks for the bug report, it seems you did look into it a bit, can you maybe provide a test to reproduce the problem reliably? And maybe also a fix?

@deltadanger
Copy link
Author

deltadanger commented Feb 8, 2017

Here is a simple test configuration to reproduce the problem:

models.py:

class Person(models.Model):
    first_name = models.CharField(max_length=50)
    last_name = models.CharField(max_length=50)

tables.py:

class PersonTable(tables.Table):
    first_name = tables.Column()
    last_name = tables.Column()
    full_name = tables.Column()

    def render_full_name(self, record):
        return record.last_name + ' ' + record.first_name

    def order_full_name(self, queryset, is_descending):
        queryset = queryset.annotate(
            full_name=Concat(F('last_name'), Value(' '), F('first_name'))
        ).order_by(('-' if is_descending else '') + 'full_name')
        return queryset, True

    class Meta:
        model = Person
        fields = (
            'first_name',
            'last_name',
            'full_name',
        )

tests.py:

class PersonTestCase(TestCase):
    def setUp(self):
        self.factory = RequestFactory()

        Person.objects.create(first_name="Alice", last_name='Beta')
        Person.objects.create(first_name="Bob", last_name='Alpha')

    def test_ordering_by_name(self):
        request = self.factory.get('/?sort=first_name')
        table = PersonTable(Person.objects.all())
        RequestConfig(request, paginate=False).configure(table)

        self.assertEqual(table.rows[0].record.first_name, 'Alice')

    def test_ordering_by_custom_field(self):
        request = self.factory.get('/?sort=full_name')
        table = PersonTable(Person.objects.all())
        RequestConfig(request, paginate=False).configure(table)

        self.assertEqual(table.rows[0].record.first_name, 'Bob')

    def test_ordering_by_both_with_custom_last(self):
        request = self.factory.get('/?sort=first_name&sort=full_name')
        table = PersonTable(Person.objects.all())
        RequestConfig(request, paginate=False).configure(table)

        self.assertEqual(table.rows[0].record.first_name, 'Bob')

    def test_ordering_by_both_with_custom_first(self):
        request = self.factory.get('/?sort=full_name&sort=first_name')
        table = PersonTable(Person.objects.all())
        RequestConfig(request, paginate=False).configure(table)

        self.assertEqual(table.rows[0].record.first_name, 'Bob')

The test test_ordering_by_both_with_custom_first raises a FieldError.

Changing the order_by function to the following fixes the problem:

    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.
        '''
        bound_column = None
        modified_by_custom_order = 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 hasattr(self, 'queryset') and bound_column:
                # Custom ordering
                self.queryset, modified = bound_column.order(self.queryset, alias[0] == '-')
                modified_by_custom_order = modified_by_custom_order or modified

        if modified_by_custom_order:
            return

        # Traditional ordering
        if accessors:
            order_by_accessors = (a.for_queryset() for a in accessors)
            self.queryset = self.queryset.order_by(*order_by_accessors)
        else:
            self.list.sort(key=OrderByTuple(accessors).key)

jieter added a commit that referenced this issue Feb 13, 2017
@jieter
Copy link
Owner

jieter commented Feb 13, 2017

@tjacoel thanks for your example.

Your fix however, doesn't pass some of the other unit tests with the error AttributeError: 'TableData' object has no attribute 'queryset'

@jieter jieter closed this as completed in 9f7eebd Feb 24, 2017
intiocean pushed a commit to intiocean/django-tables2 that referenced this issue Feb 27, 2017
* Added (failing) unit test for jieter#413

* Rough refactor of TableData into two different classes to declutter the ordering implementation

* Move ordering related tests to separate file

* Reorganize tests a bit

* Fix jieter#413, ordering by custum field raises error
@jieter
Copy link
Owner

jieter commented Feb 27, 2017

released 1.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment