From bd5ce92d059f1eec94f05ff6a6266ad07cc447b4 Mon Sep 17 00:00:00 2001 From: Andy Babic Date: Fri, 2 Nov 2018 11:35:05 +0000 Subject: [PATCH] feat: Better surfacing of validation errors in UI / optional model instance validationA (#852) Fixes: #64, #176, #415, #601 --- import_export/admin.py | 2 +- import_export/fields.py | 6 +- import_export/resources.py | 80 +++++- import_export/results.py | 75 +++++- import_export/static/import_export/import.css | 66 +++++ .../templates/admin/import_export/import.html | 248 +++++++++++------- tests/core/models.py | 10 + tests/core/tests/test_invalidrow.py | 52 ++++ tests/core/tests/test_resources.py | 114 +++++++- 9 files changed, 528 insertions(+), 125 deletions(-) create mode 100644 import_export/static/import_export/import.css create mode 100644 tests/core/tests/test_invalidrow.py diff --git a/import_export/admin.py b/import_export/admin.py index 51fc6a3..c51617d 100644 --- a/import_export/admin.py +++ b/import_export/admin.py @@ -281,7 +281,7 @@ class ImportMixin(ImportExportMixinBase): context['result'] = result - if not result.has_errors(): + if not result.has_errors() and not result.has_validation_errors(): context['confirm_form'] = ConfirmImportForm(initial={ 'import_file_name': tmp_storage.name, 'original_file_name': import_file.name, diff --git a/import_export/fields.py b/import_export/fields.py index 8b481ae..208a6a0 100644 --- a/import_export/fields.py +++ b/import_export/fields.py @@ -65,10 +65,8 @@ class Field(object): raise KeyError("Column '%s' not found in dataset. Available " "columns are: %s" % (self.column_name, list(data))) - try: - value = self.widget.clean(value, row=data) - except ValueError as e: - raise ValueError("Column '%s': %s" % (self.column_name, e)) + # If ValueError is raised here, import_obj() will handle it + value = self.widget.clean(value, row=data) if value in self.empty_values and self.default != NOT_PROVIDED: if callable(self.default): diff --git a/import_export/resources.py b/import_export/resources.py index 152222b..75bb495 100644 --- a/import_export/resources.py +++ b/import_export/resources.py @@ -10,7 +10,7 @@ from copy import deepcopy from diff_match_patch import diff_match_patch from django.conf import settings -from django.core.exceptions import ImproperlyConfigured +from django.core.exceptions import ImproperlyConfigured, ValidationError from django.core.management.color import no_style from django.db import connections, DEFAULT_DB_ALIAS from django.db.models.fields import FieldDoesNotExist @@ -114,6 +114,13 @@ class ResourceOptions(object): Controls if the result reports skipped rows Default value is True """ + clean_model_instances = False + """ + Controls whether ``instance.full_clean()`` is called during the import + process to identify potential validation errors for each (non skipped) row. + The default value is False. + """ + class DeclarativeMetaclass(type): @@ -265,6 +272,31 @@ class Resource(six.with_metaclass(DeclarativeMetaclass)): else: return (self.init_instance(row), True) + def validate_instance(self, instance, import_validation_errors={}, validate_unique=True): + """ + Takes any validation errors that were raised by + :meth:`~import_export.resources.Resource.import_obj`, and combines them + with validation errors raised by the instance's ``full_clean()`` + method. The combined errors are then re-raised as single, multi-field + ValidationError. + + If the ``clean_model_instances`` option is False, the instances's + ``full_clean()`` method is not called, and only the errors raised by + ``import_obj()`` are re-raised. + """ + errors = import_validation_errors.copy() + if self._meta.clean_model_instances: + try: + instance.full_clean( + exclude=errors.keys(), + validate_unique=validate_unique, + ) + except ValidationError as e: + errors = e.update_error_dict(errors) + + if errors: + raise ValidationError(errors) + def save_instance(self, instance, using_transactions=True, dry_run=False): """ Takes care of saving the object to the database. @@ -330,12 +362,21 @@ class Resource(six.with_metaclass(DeclarativeMetaclass)): def import_obj(self, obj, data, dry_run): """ Traverses every field in this Resource and calls - :meth:`~import_export.resources.Resource.import_field`. - """ + :meth:`~import_export.resources.Resource.import_field`. If + ``import_field()`` results in a ``ValueError`` being raised for + one of more fields, those errors are captured and reraised as a single, + multi-field ValidationError.""" + errors = {} for field in self.get_import_fields(): if isinstance(field.widget, widgets.ManyToManyWidget): continue - self.import_field(field, obj, data) + try: + self.import_field(field, obj, data) + except ValueError as e: + errors[field.attribute] = ValidationError( + force_text(e), code="invalid") + if errors: + raise ValidationError(errors) def save_m2m(self, obj, data, using_transactions, dry_run): """ @@ -463,19 +504,31 @@ class Resource(six.with_metaclass(DeclarativeMetaclass)): self.delete_instance(instance, using_transactions, dry_run) diff.compare_with(self, None, dry_run) else: - self.import_obj(instance, row, dry_run) + import_validation_errors = {} + try: + self.import_obj(instance, row, dry_run) + except ValidationError as e: + # Validation errors from import_obj() are passed on to + # validate_instance(), where they can be combined with model + # instance validation errors if necessary + import_validation_errors = e.update_error_dict(import_validation_errors) if self.skip_row(instance, original): row_result.import_type = RowResult.IMPORT_TYPE_SKIP else: + self.validate_instance(instance, import_validation_errors) self.save_instance(instance, using_transactions, dry_run) self.save_m2m(instance, row, using_transactions, dry_run) + # Add object info to RowResult for LogEntry + row_result.object_id = instance.pk + row_result.object_repr = force_text(instance) diff.compare_with(self, instance, dry_run) + row_result.diff = diff.as_html() - # Add object info to RowResult for LogEntry - if row_result.import_type != RowResult.IMPORT_TYPE_SKIP: - row_result.object_id = instance.pk - row_result.object_repr = force_text(instance) self.after_import_row(row, row_result, **kwargs) + + except ValidationError as e: + row_result.import_type = RowResult.IMPORT_TYPE_INVALID + row_result.validation_error = e except Exception as e: row_result.import_type = RowResult.IMPORT_TYPE_ERROR # There is no point logging a transaction error for each row @@ -549,7 +602,7 @@ class Resource(six.with_metaclass(DeclarativeMetaclass)): if collect_failed_rows: result.add_dataset_headers(dataset.headers) - for row in dataset.dict: + for i, row in enumerate(dataset.dict, 1): with atomic_if_using_transaction(using_transactions): row_result = self.import_row( row, @@ -559,11 +612,18 @@ class Resource(six.with_metaclass(DeclarativeMetaclass)): **kwargs ) result.increment_row_result_total(row_result) + if row_result.errors: if collect_failed_rows: result.append_failed_row(row, row_result.errors[0]) if raise_errors: raise row_result.errors[-1].error + elif row_result.validation_error: + result.append_invalid_row(i, row, row_result.validation_error) + if collect_failed_rows: + result.append_failed_row(row, row_result.validation_error) + if raise_errors: + raise row_result.validation_error if (row_result.import_type != RowResult.IMPORT_TYPE_SKIP or self._meta.report_skipped): result.append_row_result(row_result) diff --git a/import_export/results.py b/import_export/results.py index d7303f4..8cd8f84 100644 --- a/import_export/results.py +++ b/import_export/results.py @@ -2,6 +2,8 @@ from __future__ import unicode_literals from collections import OrderedDict +from django.core.exceptions import NON_FIELD_ERRORS + from tablib import Dataset @@ -18,11 +20,55 @@ class RowResult(object): IMPORT_TYPE_DELETE = 'delete' IMPORT_TYPE_SKIP = 'skip' IMPORT_TYPE_ERROR = 'error' + IMPORT_TYPE_INVALID = 'invalid' + + valid_import_types = frozenset([ + IMPORT_TYPE_NEW, + IMPORT_TYPE_UPDATE, + IMPORT_TYPE_DELETE, + IMPORT_TYPE_SKIP, + ]) def __init__(self): self.errors = [] + self.validation_error = None self.diff = None self.import_type = None + self.raw_values = {} + + +class InvalidRow(object): + """A row that resulted in one or more ``ValidationError`` being raised during import.""" + + def __init__(self, number, validation_error, values): + self.number = number + self.error = validation_error + self.values = values + try: + self.error_dict = validation_error.message_dict + except AttributeError: + self.error_dict = {NON_FIELD_ERRORS: validation_error.messages} + + @property + def field_specific_errors(self): + """Returns a dictionary of field-specific validation errors for this row.""" + return { + key: value for key, value in self.error_dict.items() + if key != NON_FIELD_ERRORS + } + + @property + def non_field_specific_errors(self): + """Returns a list of non field-specific validation errors for this row.""" + return self.error_dict.get(NON_FIELD_ERRORS, []) + + @property + def error_count(self): + """Returns the total number of validation errors for this row.""" + count = 0 + for error_list in self.error_dict.values(): + count += len(error_list) + return count class Result(object): @@ -31,14 +77,22 @@ class Result(object): self.base_errors = [] self.diff_headers = [] self.rows = [] # RowResults + self.invalid_rows = [] # InvalidRow self.failed_dataset = Dataset() self.totals = OrderedDict([(RowResult.IMPORT_TYPE_NEW, 0), (RowResult.IMPORT_TYPE_UPDATE, 0), (RowResult.IMPORT_TYPE_DELETE, 0), (RowResult.IMPORT_TYPE_SKIP, 0), - (RowResult.IMPORT_TYPE_ERROR, 0)]) + (RowResult.IMPORT_TYPE_ERROR, 0), + (RowResult.IMPORT_TYPE_INVALID, 0)]) self.total_rows = 0 + def valid_rows(self): + return [ + r for r in self.rows + if r.import_type in RowResult.valid_import_types + ] + def append_row_result(self, row_result): self.rows.append(row_result) @@ -50,9 +104,19 @@ class Result(object): def append_failed_row(self, row, error): row_values = [v for (k, v) in row.items()] - row_values.append(str(error.error)) + try: + row_values.append(str(error.error)) + except AttributeError: + row_values.append(str(error)) self.failed_dataset.append(row_values) + def append_invalid_row(self, number, row, validation_error): + self.invalid_rows.append(InvalidRow( + number=number, + validation_error=validation_error, + values=row.values(), + )) + def increment_row_result_total(self, row_result): if row_result.import_type: self.totals[row_result.import_type] += 1 @@ -62,7 +126,14 @@ class Result(object): for i, row in enumerate(self.rows) if row.errors] def has_errors(self): + """Returns a boolean indicating whether the import process resulted in + any critical (non-validation) errors for this result.""" return bool(self.base_errors or self.row_errors()) + def has_validation_errors(self): + """Returns a boolean indicating whether the import process resulted in + any validation errors for this result.""" + return bool(self.invalid_rows) + def __iter__(self): return iter(self.rows) diff --git a/import_export/static/import_export/import.css b/import_export/static/import_export/import.css new file mode 100644 index 0000000..b1045f5 --- /dev/null +++ b/import_export/static/import_export/import.css @@ -0,0 +1,66 @@ +.import-preview .errors { + position: relative; +} + +.validation-error-count { + display: inline-block; + background-color: #e40000; + border-radius: 6px; + color: white; + font-size: 0.9em; + position: relative; + font-weight: bold; + margin-top: -2px; + padding: 0.2em 0.4em; +} + +.validation-error-container { + position: absolute; + opacity: 0; + pointer-events: none; + background-color: #ffc1c1; + padding: 14px 15px 10px; + left: 16px; + top: 25px; + margin: 0 0 20px 0; + width: 200px; + z-index: 2; +} + +.import-preview td:hover .validation-error-count { + z-index: 3; +} +.import-preview td:hover .validation-error-container { + opacity: 1; + pointer-events: auto; +} + +.validation-error-list { + margin: 0; + padding: 0; +} + +.validation-error-list li { + list-style: none; + margin: 0; +} + +.validation-error-list > li > ul { + margin: 8px 0; + padding: 0; +} + +.validation-error-list > li > ul > li { + padding: 0; + margin: 0 0 10px; + line-height: 1.28em; +} + +.validation-error-field-label { + display: block; + border-bottom: 1px solid #e40000; + color: #e40000; + text-transform: uppercase; + font-weight: bold; + font-size: 0.85em; +} diff --git a/import_export/templates/admin/import_export/import.html b/import_export/templates/admin/import_export/import.html index 024bfd7..e7a24b8 100644 --- a/import_export/templates/admin/import_export/import.html +++ b/import_export/templates/admin/import_export/import.html @@ -2,112 +2,170 @@ {% load i18n %} {% load admin_urls %} {% load import_export_tags %} +{% load static %} + +{% block extrastyle %}{{ block.super }}{% endblock %} {% block breadcrumbs_last %} {% trans "Import" %} {% endblock %} {% block content %} -{% if confirm_form %} -
- {% csrf_token %} - {{ confirm_form.as_p }} -

- {% trans "Below is a preview of data to be imported. If you are satisfied with the results, click 'Confirm import'" %} -

-
- -
-
-{% else %} -
- {% csrf_token %} - -

- {% trans "This importer will import the following fields: " %} - {{ fields|join:", " }} -

- -
- {% for field in form %} -
- {{ field.errors }} - - {{ field.label_tag }} - - {{ field }} - - {% if field.field.help_text %} -

{{ field.field.help_text|safe }}

- {% endif %} -
- {% endfor %} -
- -
- -
-
-{% endif %} - -{% if result %} - - {% if result.has_errors %} -

{% trans "Errors" %}

- + {% if confirm_form %} +
+ {% csrf_token %} + {{ confirm_form.as_p }} +

+ {% trans "Below is a preview of data to be imported. If you are satisfied with the results, click 'Confirm import'" %} +

+
+ +
+
{% else %} +
+ {% csrf_token %} -

- {% trans "Preview" %} -

- - - - - {% for field in result.diff_headers %} - +

+ {% trans "This importer will import the following fields: " %} + {{ fields|join:", " }} +

+ +
+ {% for field in form %} +
+ {{ field.errors }} + + {{ field.label_tag }} + + {{ field }} + + {% if field.field.help_text %} +

{{ field.field.help_text|safe }}

+ {% endif %} +
{% endfor %} -
- - {% for row in result.rows %} - - - {% for field in row.diff %} - - {% endfor %} - - {% endfor %} -
{{ field }}
- {% if row.import_type == 'new' %} - {% trans "New" %} - {% elif row.import_type == 'skip' %} - {% trans "Skipped" %} - {% elif row.import_type == 'delete' %} - {% trans "Delete" %} - {% elif row.import_type == 'update' %} - {% trans "Update" %} - {% endif %} - - {{ field }} -
+ + +
+ +
+
{% endif %} + {% if result %} + + {% if result.has_errors %} + +

{% trans "Errors" %}

+ + + {% elif result.has_validation_errors %} + +

{% trans "Some rows failed to validate" %}

+ +

{% trans "Please correct these errors in your data where possible, then reupload it using the form above." %}

+ + + + + + + {% for field in result.diff_headers %} + + {% endfor %} + + + + {% for row in result.invalid_rows %} + + + + {% for field in row.values %} + + {% endfor %} + + {% endfor %} + +
{% trans "Row" %}{% trans "Errors" %}{{ field }}
{{ row.number }} + {{ row.error_count }} +
+
    + {% for field_name, error_list in row.field_specific_errors.items %} +
  • + {{ field_name }} +
      + {% for error in error_list %} +
    • {{ error }}
      • + {% endfor %} +
    +
    • + {% endfor %} + {% if row.non_field_specific_errors %} +
  • + {% trans "Non field specific" %} +
      + {% for error in row.non_field_specific_errors %} +
    • {{ error }}
      • + {% endfor %} +
    +
    • + {% endif %} +
+
+ 		{{ field }}
+ + {% else %} + +

{% trans "Preview" %}

+ + + + + + {% for field in result.diff_headers %} + + {% endfor %} + + + {% for row in result.valid_rows %} + + + {% for field in row.diff %} + + {% endfor %} + + {% endfor %} +
{{ field }}
+ {% if row.import_type == 'new' %} + {% trans "New" %} + {% elif row.import_type == 'skip' %} + {% trans "Skipped" %} + {% elif row.import_type == 'delete' %} + {% trans "Delete" %} + {% elif row.import_type == 'update' %} + {% trans "Update" %} + {% endif %} + {{ field }}
+ + {% endif %} + {% endif %} {% endblock %} diff --git a/tests/core/models.py b/tests/core/models.py index 6e34dab..816346d 100644 --- a/tests/core/models.py +++ b/tests/core/models.py @@ -2,6 +2,7 @@ from __future__ import unicode_literals import random import string +from django.core.exceptions import ValidationError from django.db import models from django.utils.encoding import python_2_unicode_compatible @@ -14,6 +15,15 @@ class Author(models.Model): def __str__(self): return self.name + def full_clean(self, exclude=None, validate_unique=True): + super(Author, self).full_clean(exclude, validate_unique) + if exclude is None: + exclude = [] + else: + exclude = list(exclude) + if 'name' not in exclude and self.name == '123': + raise ValidationError({'name': "'123' is not a valid value"}) + @python_2_unicode_compatible class Category(models.Model): diff --git a/tests/core/tests/test_invalidrow.py b/tests/core/tests/test_invalidrow.py new file mode 100644 index 0000000..43507be --- /dev/null +++ b/tests/core/tests/test_invalidrow.py @@ -0,0 +1,52 @@ +from __future__ import unicode_literals + +from django.core.exceptions import NON_FIELD_ERRORS, ValidationError +from django.test import TestCase + +from import_export.results import InvalidRow + + +class InvalidRowTest(TestCase): + + def setUp(self): + # Create a ValidationEror with a mix of field-specific and non-field-specific errors + self.non_field_errors = ValidationError(['Error 1', 'Error 2', 'Error 3']) + self.field_errors = ValidationError({ + 'name': ['Error 4', 'Error 5'], + 'birthday': ['Error 6', 'Error 7'], + }) + combined_error_dict = self.non_field_errors.update_error_dict( + self.field_errors.error_dict.copy() + ) + e = ValidationError(combined_error_dict) + # Create an InvalidRow instance to use in tests + self.obj = InvalidRow( + number=1, + validation_error=e, + values={'name': 'ABC', 'birthday': '123'} + ) + + def test_error_count(self): + self.assertEqual(self.obj.error_count, 7) + + def test_non_field_specific_errors(self): + result = self.obj.non_field_specific_errors + self.assertIsInstance(result, list) + self.assertEqual(result, ['Error 1', 'Error 2', 'Error 3']) + + def test_field_specific_errors(self): + result = self.obj.field_specific_errors + self.assertIsInstance(result, dict) + self.assertEqual(len(result), 2) + self.assertEqual(result['name'], ['Error 4', 'Error 5']) + self.assertEqual(result['birthday'], ['Error 6', 'Error 7']) + + def test_creates_error_dict_from_error_list_if_validation_error_only_has_error_list(self): + obj = InvalidRow( + number=1, + validation_error=self.non_field_errors, + values={} + ) + self.assertIsInstance(obj.error_dict, dict) + self.assertIn(NON_FIELD_ERRORS, obj.error_dict) + self.assertEqual(obj.error_dict[NON_FIELD_ERRORS], ['Error 1', 'Error 2', 'Error 3']) diff --git a/tests/core/tests/test_resources.py b/tests/core/tests/test_resources.py index 9b0e342..5fdb2b3 100644 --- a/tests/core/tests/test_resources.py +++ b/tests/core/tests/test_resources.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- from __future__ import unicode_literals import tablib @@ -13,6 +14,7 @@ from django.db import IntegrityError, DatabaseError from django.db.models import Count from django.db.models.fields import FieldDoesNotExist from django.test import TestCase, TransactionTestCase, skipUnlessDBFeature +from django.utils import six from django.utils.html import strip_tags from django.utils.encoding import force_text @@ -159,6 +161,29 @@ class WithDefaultResource(resources.ModelResource): fields = ('name',) +class HarshRussianWidget(widgets.CharWidget): + def clean(self, value, row=None, *args, **kwargs): + raise ValueError("Ова вриједност је страшна!") + + +class AuthorResourceWithCustomWidget(resources.ModelResource): + + class Meta: + model = Author + + @classmethod + def widget_from_django_field(cls, f, default=widgets.Widget): + if f.name == 'name': + return HarshRussianWidget + result = default + internal_type = f.get_internal_type() if callable(getattr(f, "get_internal_type", None)) else "" + if internal_type in cls.WIDGETS_MAP: + result = cls.WIDGETS_MAP[internal_type] + if isinstance(result, six.string_types): + result = getattr(cls, result)(f) + return result + + class ModelResourceTest(TestCase): def setUp(self): self.resource = BookResource() @@ -288,23 +313,87 @@ class ModelResourceTest(TestCase): self.assertEqual(instance.author_email, 'test@example.com') self.assertEqual(instance.price, Decimal("10.25")) - def test_import_data_value_error_includes_field_name(self): - class AuthorResource(resources.ModelResource): - class Meta: - model = Author - + def test_import_data_raises_field_specific_validation_errors(self): resource = AuthorResource() dataset = tablib.Dataset(headers=['id', 'name', 'birthday']) dataset.append(['', 'A.A.Milne', '1882test-01-18']) result = resource.import_data(dataset, raise_errors=False) - self.assertTrue(result.has_errors()) - self.assertTrue(result.rows[0].errors) - msg = ("Column 'birthday': Enter a valid date/time.") - actual = result.rows[0].errors[0].error - self.assertIsInstance(actual, ValueError) - self.assertEqual(msg, str(actual)) + self.assertTrue(result.has_validation_errors()) + self.assertIs(result.rows[0].import_type, results.RowResult.IMPORT_TYPE_INVALID) + self.assertIn('birthday', result.invalid_rows[0].field_specific_errors) + + def test_import_data_handles_widget_valueerrors_with_unicode_messages(self): + resource = AuthorResourceWithCustomWidget() + dataset = tablib.Dataset(headers=['id', 'name', 'birthday']) + dataset.append(['', 'A.A.Milne', '1882-01-18']) + + result = resource.import_data(dataset, raise_errors=False) + + self.assertTrue(result.has_validation_errors()) + self.assertIs(result.rows[0].import_type, results.RowResult.IMPORT_TYPE_INVALID) + self.assertEqual( + result.invalid_rows[0].field_specific_errors['name'], + ["Ова вриједност је страшна!"] + ) + + def test_model_validation_errors_not_raised_when_clean_model_instances_is_false(self): + + class TestResource(resources.ModelResource): + class Meta: + model = Author + clean_model_instances = False + + resource = TestResource() + dataset = tablib.Dataset(headers=['id', 'name']) + dataset.append(['', '123']) + + result = resource.import_data(dataset, raise_errors=False) + self.assertFalse(result.has_validation_errors()) + self.assertEqual(len(result.invalid_rows), 0) + + def test_model_validation_errors_raised_when_clean_model_instances_is_true(self): + + class TestResource(resources.ModelResource): + class Meta: + model = Author + clean_model_instances = True + + resource = TestResource() + dataset = tablib.Dataset(headers=['id', 'name']) + dataset.append(['', '123']) + + result = resource.import_data(dataset, raise_errors=False) + self.assertTrue(result.has_validation_errors()) + self.assertEqual(result.invalid_rows[0].error_count, 1) + self.assertEqual( + result.invalid_rows[0].field_specific_errors, + {'name': ["'123' is not a valid value"]} + ) + + def test_known_invalid_fields_are_excluded_from_model_instance_cleaning(self): + + # The custom widget on the parent class should complain about + # 'name' first, preventing Author.full_clean() from raising the + # error as it does in the previous test + + class TestResource(AuthorResourceWithCustomWidget): + class Meta: + model = Author + clean_model_instances = True + + resource = TestResource() + dataset = tablib.Dataset(headers=['id', 'name']) + dataset.append(['', '123']) + + result = resource.import_data(dataset, raise_errors=False) + self.assertTrue(result.has_validation_errors()) + self.assertEqual(result.invalid_rows[0].error_count, 1) + self.assertEqual( + result.invalid_rows[0].field_specific_errors, + {'name': ["Ова вриједност је страшна!"]} + ) def test_import_data_error_saving_model(self): row = list(self.dataset.pop()) @@ -317,8 +406,7 @@ class ModelResourceTest(TestCase): self.assertTrue(result.rows[0].errors) actual = result.rows[0].errors[0].error self.assertIsInstance(actual, ValueError) - self.assertIn("Column 'id': could not convert string to float", - str(actual)) + self.assertIn("could not convert string to float", str(actual)) def test_import_data_delete(self):