jieter / django-tables2

django-tables2 - An app for creating HTML tables
https://django-tables2.readthedocs.io/en/latest/
Other
1.88k stars 426 forks source link

ManyToManyFields not shown with Django 1.7 #211

Closed victor-baumann closed 8 years ago

victor-baumann commented 10 years ago

I tracked down the cause to utils.Accessor https://github.com/bradleyayers/django-tables2/blob/master/django_tables2/utils.py#L376-L380

            if callable(current):
                ...
                current = current()

The problem is, that managers are callable in Django 1.7 and will raise an error when called without arguments. https://github.com/django/django/blob/master/django/db/models/fields/related.py#L654 https://docs.djangoproject.com/en/dev/releases/1.7/#s-using-a-custom-manager-when-traversing-reverse-relations

I kinda fixed it for me with:

                if not isinstance(current, BaseManager):
                    current = current()

But I don't know if this is a good solution, since I don't have a very good understanding of the inner workings of tables2 nor django:)

brianmay commented 9 years ago

I encountered the same issue and tracked it down to the same code. Although I don't seem to get an exception, I get None returned instead.

I don't like the solution proposed in #214 as it means putting tables specific attributes in the db model (If I understand it correctly).

For me, the solution proposed by @smolotov is a better solution, as no changes are required to code that uses django-tables2.

brianmay commented 9 years ago

Here is a patch that seems to work. If this looks good, can submit a merge request.

--- /usr/lib/python3/dist-packages/django_tables2/utils.py      2014-02-02 09:07:17.000000000 +0000
+++ /usr/lib/python2.7/dist-packages/django_tables2/utils.py    2014-10-16 05:33:24.000000000 +0000
@@ -4,6 +4,7 @@
 from django.utils.html import escape
 from django.utils.safestring import mark_safe
 from django.test.client import FakePayload
+from django.db.models.manager import BaseManager
 from itertools import chain
 import inspect
 import six
@@ -377,7 +378,8 @@
                     if safe and getattr(current, 'alters_data', False):
                         raise ValueError('refusing to call %s() because `.alters_data = True`'
                                          % repr(current))
-                    current = current()
+                    if not isinstance(current, BaseManager):
+                       current = current()
                 # important that we break in None case, or a relationship
                 # spanning across a null-key will raise an exception in the
                 # next iteration, instead of defaulting.
mbertheau commented 9 years ago

@brianmay No code change is necessary as django adds that attribute already, see https://github.com/django/django/blob/ed37f7e979186c99a6f351c289eb486461601d80/django/db/models/fields/related.py#L676 and https://github.com/django/django/blob/ed37f7e979186c99a6f351c289eb486461601d80/django/db/models/fields/related.py#L881

brianmay commented 9 years ago

@mbertheau Thanks for the correction; contrary to what I said before, your patch is probably the better patch, and is more likely to catch other classes that have similar issues.

victor-baumann commented 9 years ago

Agreed. I think @mbertheau is the better patch and more likely to catch other cases

dyve commented 9 years ago

We are experiencing this problem too. A patch/updated release on PyPI would be much appreciated.

Nekmo commented 9 years ago

+1

hdmaker commented 9 years ago

Apparently @mbertheau pull request #214 fixes the related fields access. Integrating that pull request or providing an updated version that fixes that issue would be of great use.

elmcrest commented 9 years ago

+1

thebarty commented 9 years ago

+1

thebarty commented 9 years ago

Hi guys,

I tried this fix, but it did not work for me - it still fails and shows me "--" in the table-data.

My code example is in https://github.com/bradleyayers/django-tables2/issues/256

dyve commented 9 years ago

This bug is really bugging us. Polite request to @bradleyayers to check #214 and se if a Django 1.7+ compatible version (for M2M) can be published to PyPI.

dyve commented 8 years ago

@bradleyayers Any ETA on this? Still looks broken to us.

brianmay commented 8 years ago

Think somebody needs to look at @thebarty's code and try to work out why @mbertheau pull request #214 doesn't fix the problem.

brianmay commented 8 years ago

...Assuming of course #256 is related to this bug.

brianmay commented 8 years ago

Actually #256 looks different, it isn't a ManyToMany relationship.

brianmay commented 8 years ago

Suspect the problem in #256 is that the column resolves to a RelatedManager, and Django Tables doesn't know how to deal with it. So you need to tell it how, by overriding the appropriate render function.

I am having a memory problem. I can't remember how to reproduce the ManyToMany issue. Can somebody please provide a sample db model and table that doesn't work unless #214 is applied? If so, I might try writing a proper test case.

Mental note for future: including test cases before you forget is important!

ghost commented 8 years ago

+1 for @mbertheau 's solution. It fixed the problem for me too.

bradleyayers commented 8 years ago

I'm not maintaining django-tables2, but I'm happy to give admin to someone else that's interested in maintaining it.

dyve commented 8 years ago

@jieter This is the single most annoying thing in tables2 right now :-)

jieter commented 8 years ago

Copied the patch in #214 to a new branch (rebase-214), which passes the test suite.

@brianmay did you manage to create a proper test for this?

brianmay commented 8 years ago

No, sorry, no test case for this. Basically I am no longer involved in the project that triggered the problem :-(