Merge pull request #2075 from noirbizarre/model-field-errors

Improve ModelField errors

CHANGELOG.md

@@ -5,7 +5,7 @@
 - Automatically loads default settings from plugins (if `plugin.settings` module exists) [#2058](https://github.com/opendatateam/udata/pull/2058)
 - Fixes some memory leaks on reindexing [#2070](https://github.com/opendatateam/udata/pull/2070)
 - Prevent ExtrasField failure on null value [#2074](https://github.com/opendatateam/udata/pull/2074)
-
+- Improve ModelField errors handling [#2075](https://github.com/opendatateam/udata/pull/2075)
 
 ## 1.6.5 (2019-02-27)
 

udata/forms/fields.py

@@ -21,7 +21,7 @@
 from udata.core.organization.permissions import OrganizationPrivatePermission
 from udata.i18n import lazy_gettext as _
 from udata import tags, uris
-from udata.forms import ModelForm, Form
+from udata.forms import ModelForm
 from udata.utils import to_iso_date, get_by
 
 
@@ -353,8 +353,8 @@ def clean_oid(oid, model):
             # and returns the original value on exception
             model.id.validate(oid)
             return model.id.to_python(oid)
-        except:  # Catch all exceptions as model.type is not predefined
-            raise ValueError('Unsupported identifier: ' + oid)
+        except Exception:  # Catch all exceptions as model.type is not predefined
+            raise ValueError('Unsupported identifier: {0}'.format(oid))
 
 
 class ModelFieldMixin(object):
@@ -381,30 +381,62 @@ def process_formdata(self, valuelist):
 
 
 class ModelField(Field):
+    def process(self, formdata, data=unset_value):
+        if formdata:
+            # Process prefixed values as in FormField
+            newdata = {}
+            prefix = self.short_name + '-'
+            for key in formdata.keys():
+                if key.startswith(prefix):
+                    value = formdata.pop(key)
+                    newdata[key.replace(prefix, '')] = value
+            if newdata:
+                formdata.add(self.short_name, newdata)
+        super(ModelField, self).process(formdata, data)
+
     def process_formdata(self, valuelist):
-        if valuelist and len(valuelist) == 1 and valuelist[0]:
-            specs = valuelist[0]
-            model_field = getattr(self._form.model_class, self.name)
-            if isinstance(specs, basestring):
-                if isinstance(model_field, db.ReferenceField):
-                    specs = {'class': str(model_field.document_type.__name__), 'id': specs}
-                elif isinstance(model_field, db.GenericReferenceField):
-                    message = _('Expect both class and identifier')
-                    raise validators.ValidationError(message)
-
-            # No try/except required
-            # In case of error, ValueError is raised
-            # and is properly handled as form validation error
-            model = db.resolve_model(specs['class'])
+        if not valuelist or len(valuelist) != 1 or not valuelist[0]:
+            return
+        specs = valuelist[0]
+        model_field = getattr(self._form.model_class, self.name)
+        if isinstance(specs, basestring):
+            specs = {'id': specs}
+        elif not specs.get('id', None):
+            raise validators.ValidationError('Missing "id" field')
+
+        if isinstance(model_field, db.ReferenceField):
+            expected_model = str(model_field.document_type.__name__)
+            if 'class' not in specs:
+                specs['class'] = expected_model
+            elif specs['class'] != expected_model:
+                msg = 'Expect a "{0}" class but "{1}" was found'.format(
+                    expected_model, specs['class']
+                )
+                raise validators.ValidationError(msg)
+        elif isinstance(model_field, db.GenericReferenceField):
+            if 'class' not in specs:
+                msg = _('Expect both class and identifier')
+                raise validators.ValidationError(msg)
+
+        # No try/except required
+        # In case of error, ValueError is raised
+        # and is properly handled as form validation error
+        model = db.resolve_model(specs['class'])
+        oid = clean_oid(specs, model)
 
-            try:
-                self.data = model.objects.only('id').get(id=clean_oid(specs, model))
-            except db.DoesNotExist:
-                message = _('{0} does not exists').format(model.__name__)
-                raise validators.ValidationError(message)
-            except db.ValidationError:
-                message = _('{0} is not a valid identifier').format(specs['id'])
-                raise validators.ValidationError(message)
+        try:
+            self.data = model.objects.only('id').get(id=oid)
+        except db.DoesNotExist:
+            label = '{0}({1})'.format(model.__name__, oid)
+            msg = _('{0} does not exists').format(label)
+            raise validators.ValidationError(msg)
+
+    def pre_validate(self, form):
+        # If any error happen during process, we raise StopValidation here
+        # to prevent "DataRequired" validator from clearing errors
+        if self.errors:
+            raise validators.StopValidation()
+        super(ModelField, self).pre_validate(form)
 
 
 class ModelChoiceField(StringField):

udata/models/__init__.py

@@ -75,7 +75,7 @@ def resolve_model(self, model):
         try:
             return get_document(classname)
         except self.NotRegistered:
-            message = '{0} does not exists'.format(classname)
+            message = 'Model "{0}" does not exist'.format(classname)
             raise ValueError(message)