Fix fields bound to empty related fields being ignored #102
Conversation
…ly calling callable attributes
smartmin/views.py
Outdated
| @@ -141,10 +141,6 @@ def lookup_obj_attribute(self, obj, field): | |||
| # next up is the object itself | |||
| obj_field = getattr(obj, curr_field, None) | |||
|
|
|||
| # if it is callable, do so | |||
| if obj_field and getattr(obj_field, '__call__', None): | |||
There was a problem hiding this comment.
There's a chance someone is relying on this behaviour but I think it's better if views explicitly call object methods than doing it automatically here - as this doesn't take into account callables with parameters, or callables that we don't actually want to call.
There was a problem hiding this comment.
Isn't this used in say table views, where you can specify a column that is actually a method on the object? Or is this different? The former is used all over the place and is pretty useful. (it's also something the django-admin does as I recall so has precedent)
There was a problem hiding this comment.
You got an example of us using that - I honestly don't recall seeing that. What we do a lot is add a method to the view like get_foo.
Problem is things are callable that we don't actually want to call. Maybe we limit to bound methods with no params?
There was a problem hiding this comment.
Definitely something we used to use a ton when making dashboards and the like, don't know if we have any uses in RapidPro as the minute, but it sure is useful.
Here's what the Django admin allows:
https://docs.djangoproject.com/en/1.11/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_display
I don't think we need the second option, but 1, 3, 4 seems like reasonable options to have.
There was a problem hiding this comment.
Fair nuff - so what shall we do about m2m fields? They currently cause SmartReadView and SmartListView to blow up. We can either 1) fetch the related objects and generate a value from their csv'd str values, or 2) blow up with a more helpful message than https://sentry.io/nyaruka/casepro/issues/288024834/ so dev knows to add a method to the view or exclude that field.
There was a problem hiding this comment.
I guess what I'm not getting is why are they being included in the first place if they aren't valid? Are we including them automagically somewhere? Shouldn't it just be the fields specified in fields for the view?
There was a problem hiding this comment.
SmartReadView defaults to model._meta.fields but I just checked and that only includes concrete fields so I guess we can treat putting a m2m in fields of the view as a developer error. I'll put the calling code back in as is.
| @@ -6,7 +6,7 @@ | |||
| {% if form_field and form_field.is_hidden %} | |||
| {{ form_field }} | |||
| {% else %} | |||
| {% if form_field %} | |||
| {% if form_field != None %} | |||
There was a problem hiding this comment.
Looks like 1.9 has is but I guess we are not on that yet. Wonder where use of != with None breaks, either way better than it was.
There was a problem hiding this comment.
Actually our next release of Smartmin would be for 1.9, 1.10 and 1.11 so we can use that
There was a problem hiding this comment.
Argh PyCharm just informed me is was added in 1.10
smartmin/views.py
Outdated
| @@ -141,10 +141,6 @@ def lookup_obj_attribute(self, obj, field): | |||
| # next up is the object itself | |||
| obj_field = getattr(obj, curr_field, None) | |||
|
|
|||
| # if it is callable, do so | |||
| if obj_field and getattr(obj_field, '__call__', None): | |||
There was a problem hiding this comment.
Isn't this used in say table views, where you can specify a column that is actually a method on the object? Or is this different? The former is used all over the place and is pretty useful. (it's also something the django-admin does as I recall so has precedent)
If a form field is bound to related field on a model which is empty - it ends up being evaluated as falsey and ignored in our field template.
Then in addition because related fields have a call method, we try to call them and things blow up, e.g. https://sentry.io/nyaruka/casepro/issues/288024834/