More robust displaycondition checking

userdefinedfields/fields.py

@@ -1,7 +1,9 @@
 from functools import partialmethod
 
 from django.contrib.contenttypes.models import ContentType
+from django.core.exceptions import FieldDoesNotExist
 from django.db.models import JSONField
+from django.db.models.fields.related import ForeignKey
 
 from .models import ExtraField
 
@@ -22,16 +24,44 @@ def _get_EXTRAFIELD_fieldlist(self, obj, field):
             .prefetch_related('displaycondition_set')
         )
 
-        # use the displaycondition_set to limit the fields
+        # include only the fields that aren't rejected by their display conditions.
         filtered_fields = []
         for f in relevant_fields:
-            dcset = f.displaycondition_set.all()
-            if dcset:
-                for dc in dcset:
-                    if str(getattr(obj, f'{dc.key}_id')) in dc.values.split(','):
-                        filtered_fields.append(f)
+            for dc in f.displaycondition_set.all():
+                try:
+                    dc_field = type(obj)._meta.get_field(dc.key)
+                except FieldDoesNotExist:
+                    # Well this won't do. Poorly defined field. Since this'll be commonly used
+                    # in templates though, we opt to squelch the error, rather than be noisy about
+                    # it. Users will quickly notice their extra field is not coming up.
+                    break
+
+                if not isinstance(dc_field, ForeignKey):
+                    # We only support FK-based display conditions, by design! Here's why:
+                    # - Other attributes on the object -- Having business logic based on random
+                    #    fields like, eg. the objects name, promotes bad design. Exception could
+                    #    perhaps be choice fields, but let's just reject attribs across the board
+                    #    for now.
+                    # - M2Ms -- These are generally less persistent than FKs, and we generally want
+                    #    extra fields to stay visible once set up. Also, certain M2Ms, such as tags,
+                    #    already cover a similar space to what extra fields are trying to achieve.
+                    #    Also, most importantly, checking against these would incur an extra db hit.
+                    # - Attributes on related objects -- Would incur additional db hits too. This'd
+                    #    be quite costly when trying to display a whole bunch of objects and their
+                    #    extra fields in a list. So we opt to avoid this headache altogether.
+                    break
+
+                # To avoid letting users have multiple ways of doing the same thing, we reject
+                # conditions that have tried to supply the `_id` suffix themselves.
+                # (The earlier get_field call tolerates for both with and without.)
+                if dc.key != dc_field.name:
+                    break
+
+                # Check to see whether the condition is satisfied.
+                if str(getattr(obj, f'{dc_field.name}_id')) not in dc.values.split(','):
+                    break
             else:
-                # if there's no display conditions, include this field
+                # We didn't trip any display condition failures; field is good to add.
                 filtered_fields.append(f)
 
         # loop through and create a list of field info and values from the JSON blob
@@ -42,7 +72,7 @@ def _get_EXTRAFIELD_fieldlist(self, obj, field):
 
             # overwrite val with the pretty value if it's a choice field
             if 'choices' in f.field_settings:
-                choices = dict([(choice.get('value'), choice.get('label')) for choice in f.field_settings['choices']])
+                choices = {choice.get('value'): choice.get('label') for choice in f.field_settings['choices']}
                 val = choices.get(val) or val
             fieldlist.append((f.group, f.name, f.label, val))
 
@@ -55,9 +85,9 @@ def _get_EXTRAFIELD_display(self, obj, field):
         {{ asset.get_extra_fields_display.field_name }}
         """
         fieldlist = self._get_EXTRAFIELD_fieldlist(obj, field)
-        return dict([(d[1], d[3]) for d in fieldlist])
+        return {d[1]: d[3] for d in fieldlist}
 
     def contribute_to_class(self, cls, name, **kwargs):
-        setattr(cls, 'get_%s_display' % name, partialmethod(self._get_EXTRAFIELD_display, field=self))
-        setattr(cls, 'get_%s_fieldlist' % name, partialmethod(self._get_EXTRAFIELD_fieldlist, field=self))
+        setattr(cls, f'get_{name}_display', partialmethod(self._get_EXTRAFIELD_display, field=self))
+        setattr(cls, f'get_{name}_fieldlist', partialmethod(self._get_EXTRAFIELD_fieldlist, field=self))
         super().contribute_to_class(cls, name)

userdefinedfields/migrations/0001_initial.py

@@ -1,8 +1,6 @@
 # Generated by Django 2.0.3 on 2018-06-14 11:03
-
 import django.db.models.deletion
 from django.db import migrations, models
-from django.conf import settings
 
 
 class Migration(migrations.Migration):

userdefinedfields/migrations/0002_auto_20200923_0122.py

@@ -1,7 +1,3 @@
-# -*- coding: utf-8 -*-
-# Generated by Django 1.11.29 on 2020-09-23 01:22
-from __future__ import unicode_literals
-
 from django.db import migrations
 
 

userdefinedfields/migrations/0003_auto_20201120_0209.py

@@ -0,0 +1,16 @@
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('userdefinedfields', '0002_auto_20200923_0122'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='displaycondition',
+            name='values',
+            field=models.CharField(blank=True, max_length=500),
+        ),
+    ]

userdefinedfields/models.py

@@ -34,7 +34,7 @@ class Meta:
 class DisplayCondition(models.Model):
     field = models.ForeignKey(ExtraField, on_delete=models.CASCADE)
     key = models.CharField(max_length=32, blank=True)
-    values = models.CharField(max_length=200, blank=True)
+    values = models.CharField(max_length=500, blank=True)
 
     def __str__(self):
         return f'{self.field} {self.key} {self.values}'