Resolve #427: Don't display empty resources table for locations

[seav]

No description provided.

[oliverroick]

get_context_data needs some improvement.

First, the database is hit twice when getting the relationship and then getting the number of relationships. context['relationships'] is a QuerySet so you can call exists() to find out if one record matches the filter. This is better written as:

context['relationships'] = TenureRelationship.objects.filter(
    spatial_unit=spatial_unit)
context['has_relationships'] = context['relationships'].exists()

Second, spatial_unit.resources is a QuerySet as well. Again, exists() is the better choice.

context['has_resources'] = spatial_unit.resources.exists()

I'm not sure if has_resources and has_relationships is necessary. In the template you can write {% if relationships %} or {% if location.resources %} as well.

[seav]

{% if relationships %} will not work because relationships refers to the spatial relationships of the location. I think {% if tenurerelationship_set %} should work.

I'll try that as well as the {% if location.resources %}. I'll update as needed.

[oliverroick]

In a template {% if relationships %} refers to what is in context['relationships']

[seav]

I'd like to clarify, I was thinking of also removing context['relationships'] and go straight to {% if location.tenurerelationship_set.all %}. If context['relationships'] is removed, then {% if location.relationships %} would now refer to the spatial relationships.

[seav]

I just tried using {% if location.tenurerelationship_set.all %} and {% if location.resources %} in the template, and completely deleting get_context_data() from the view. This worked.

[seav]

@ian-ross, I'd like to understand the tidying you did. Am I correct that both approaches are functionally the same and both hit the Tenure Relationships DB table only once, but the tidy approach avoids putting model logic into the view logic?

[oliverroick]

@seav Actually, your approach does hit the database twice. Every time you get a related QuerySet in a template (e.g. {% if location.tenurerelationship_set.all %}), a new query is sent to the database.

Ian's approach, on the other hand, caches the result and uses the same QuerySet in the template. You can read about it the Django docs.

[ian-ross]

@seav As Oliver says, your approach does two queries. More importantly though, my approach makes it explicit that the intent is for a single query to be performed and the same results to be used for both occurrences of location.tenurerelationship_set.all() in the template. Even though Django has caching for querysets, I always try to make the sharing of results explicit by assigning a name to query results. This makes it really clear when queries are being executed, and means that people reading your code can understand what its query behaviour is likely to be without understanding the details of the Django caching scheme (which could change in a subsequent Django release).

Not having model logic in the template is an additional benefit of assigning the queryset results to an explicit name in the context data.

I'm still working out the best way to do these things in Django, but I think that explicit names for shared results and avoiding complex logic in templates are two good principles to start with.