diff --git a/docs/admin_integration.rst b/docs/admin_integration.rst index 24926986b..0ae2c943d 100644 --- a/docs/admin_integration.rst +++ b/docs/admin_integration.rst @@ -37,7 +37,7 @@ dropdown in the UI. .. figure:: _static/images/django-import-export-change.png A screenshot of the change view with Import and Export buttons. - + Importing --------- @@ -137,6 +137,14 @@ For example, if using django-storages, you can configure s3 as a temporary stora }, } +.. _format_ui_error_messages: + +How to format UI error messages +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Admin UI import error messages can be formatted using the :attr:`~import_export.admin.ImportMixin.import_error_display` +attribute. + Exporting --------- diff --git a/docs/changelog.rst b/docs/changelog.rst index 16fa9844e..4192378fb 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -1,7 +1,9 @@ Changelog ========= -Please refer to :doc:`release notes`. +.. warning:: + + Version 4 introduces breaking changes. Please refer to :doc:`release notes`. 4.0.0-beta.3 (unreleased) ------------------------- @@ -10,6 +12,7 @@ Please refer to :doc:`release notes`. - Added customizable ``MediaStorage`` (#1708) - Relocated admin integration section from advanced_usage.rst into new file (#1713) - Fix slow export with ForeignKey id (#1717) +- Added customization of Admin UI import error messages (#1727) 4.0.0-beta.2 (2023-12-09) ------------------------- @@ -19,7 +22,7 @@ Please refer to :doc:`release notes`. - Support export from model change form (#1687) - Updated Admin UI to track deleted and skipped Imports (#1691) - Import form defaults to read-only field if only one format defined (#1690) -- Refactored :module:`~import_export.resources` into separate modules for ``declarative`` and ``options`` (#1695) +- Refactored :mod:`~import_export.resources` into separate modules for ``declarative`` and ``options`` (#1695) - fix multiple inheritance not setting options (#1696) - Refactored tests to remove dependencies between tests (#1703) - Handle python3.12 datetime deprecations (#1705) diff --git a/docs/faq.rst b/docs/faq.rst index d491b09bb..d3dbbb9db 100755 --- a/docs/faq.rst +++ b/docs/faq.rst @@ -104,7 +104,7 @@ Please refer to `this issue `_. +Please refer to :ref:`format_ui_error_messages`. Ids incremented twice during import ----------------------------------- diff --git a/docs/release_notes.rst b/docs/release_notes.rst index b21681bfb..671efd5a4 100644 --- a/docs/release_notes.rst +++ b/docs/release_notes.rst @@ -70,6 +70,8 @@ Deprecations * Use of ``ExportViewFormMixin`` is deprecated. See `this issue `_. +* See :ref:`renamed_methods`. + Admin UI ======== @@ -97,6 +99,21 @@ Success message The success message shown on successful import has been updated to include the number of 'deleted' and 'skipped' rows. See `this PR `_. +Import error messages +--------------------- + +The default error message for import errors has been modified to simplify the format. +Error messages now contain the error message only by default. The row and traceback are not presented. + +The original format can be restored by setting :attr:`~import_export.admin.ImportMixin.import_error_display` on the +Admin class definition. For example:: + + class BookAdmin(ImportExportModelAdmin): + import_error_display = ("message", "row", "traceback") + + +See `this issue `_. + API changes =========== @@ -121,6 +138,8 @@ method calls, and to allow easier extensibility. :class:`import_export.resources.Resource` ----------------------------------------- +.. _renamed_methods: + Renamed methods ^^^^^^^^^^^^^^^ @@ -185,41 +204,41 @@ This section describes methods in which the parameters have changed. * - ``save_m2m(self, obj, data, using_transactions, dry_run)`` - ``save_m2m(self, instance, row, **kwargs)`` - - * ``dry_run`` param now in ``kwargs`` - * ``using_transactions`` param now in ``kwargs`` - * ``row`` added as mandatory arg + - * ``row`` added as mandatory arg * ``obj`` renamed to ``instance`` * ``data`` renamed to ``row`` + * ``dry_run`` param now in ``kwargs`` + * ``using_transactions`` param now in ``kwargs`` * - ``before_save_instance(self, instance, using_transactions, dry_run)`` - ``before_save_instance(self, instance, row, **kwargs)`` - - * ``dry_run`` param now in ``kwargs`` + - * ``row`` added as mandatory arg + * ``dry_run`` param now in ``kwargs`` * ``using_transactions`` param now in ``kwargs`` - * ``row`` added as mandatory arg * - ``after_save_instance(self, instance, using_transactions, dry_run)`` - ``after_save_instance(self, instance, row, **kwargs)`` - - * ``dry_run`` param now in ``kwargs`` + - * ``row`` added as mandatory arg + * ``dry_run`` param now in ``kwargs`` * ``using_transactions`` param now in ``kwargs`` - * ``row`` added as mandatory arg * - ``delete_instance(self, instance, using_transactions=True, dry_run=False)`` - ``delete_instance(self, instance, row, **kwargs)`` - - * ``dry_run`` param now in ``kwargs`` + - * ``row`` added as mandatory arg + * ``dry_run`` param now in ``kwargs`` * ``using_transactions`` param now in ``kwargs`` - * ``row`` added as mandatory arg * - ``before_delete_instance(self, instance, dry_run)`` - ``before_delete_instance(self, instance, row, **kwargs)`` - - * ``dry_run`` param now in ``kwargs`` + - * ``row`` added as mandatory arg + * ``dry_run`` param now in ``kwargs`` * ``using_transactions`` param now in ``kwargs`` - * ``row`` added as mandatory arg * - ``after_delete_instance(self, instance, dry_run)`` - ``after_delete_instance(self, instance, row, **kwargs)`` - - * ``dry_run`` param now in ``kwargs`` + - * ``row`` added as mandatory arg + * ``dry_run`` param now in ``kwargs`` * ``using_transactions`` param now in ``kwargs`` - * ``row`` added as mandatory arg * - ``before_export(self, queryset, *args, **kwargs)`` - ``before_export(self, queryset, **kwargs)`` diff --git a/import_export/admin.py b/import_export/admin.py index 6bd8ff4a4..19592bd0d 100644 --- a/import_export/admin.py +++ b/import_export/admin.py @@ -83,6 +83,10 @@ class ImportMixin(BaseImportMixin, ImportExportMixinBase): confirm_form_class = ConfirmImportForm #: import data encoding from_encoding = "utf-8-sig" + #: control which UI elements appear when import errors are displayed. + #: Available options: 'message', 'row', 'traceback' + import_error_display = ("message",) + skip_admin_log = None # storage class for saving temporary files tmp_storage_class = None @@ -535,6 +539,7 @@ def import_action(self, request, *args, **kwargs): ) for resource in resources ] + context["import_error_display"] = self.import_error_display request.current_app = self.admin_site.name return TemplateResponse(request, [self.import_template_name], context) diff --git a/import_export/resources.py b/import_export/resources.py index 1c0564b83..14eecf782 100644 --- a/import_export/resources.py +++ b/import_export/resources.py @@ -4,6 +4,7 @@ from collections import OrderedDict from copy import deepcopy from html import escape +from warnings import warn import tablib from diff_match_patch import diff_match_patch @@ -423,6 +424,18 @@ def import_field(self, field, instance, row, is_m2m=False, **kwargs): def get_import_fields(self): return [self.fields[f] for f in self.get_import_order()] + def import_obj(self, obj, data, dry_run, **kwargs): + warn( + "The 'import_obj' method is deprecated and will be replaced " + "with 'import_instance(self, instance, row, **kwargs)' " + "in a future release. Refer to Release Notes for details.", + DeprecationWarning, + stacklevel=2, + ) + if dry_run is True: + kwargs.update({"dry_run": dry_run}) + self.import_instance(obj, data, **kwargs) + def import_instance(self, instance, row, **kwargs): r""" Traverses every field in this Resource and calls @@ -615,6 +628,18 @@ def after_import_row(self, row, row_result, **kwargs): """ pass + def after_import_instance(self, instance, new, row_number=None, **kwargs): + warn( + "The 'after_import_instance' method is deprecated and will be replaced " + "with 'after_init_instance(self, instance, new, row, **kwargs)' " + "in a future release. Refer to Release Notes for details.", + DeprecationWarning, + stacklevel=2, + ) + if row_number is not None: + kwargs.update({"row_number": row_number}) + self.after_init_instance(instance, new, None, **kwargs) + def after_init_instance(self, instance, new, row, **kwargs): r""" Override to add additional logic. Does nothing by default. diff --git a/import_export/templates/admin/import_export/import.html b/import_export/templates/admin/import_export/import.html index 03f0cb6d7..20cf17dec 100644 --- a/import_export/templates/admin/import_export/import.html +++ b/import_export/templates/admin/import_export/import.html @@ -85,15 +85,25 @@

{% translate "Errors" %}

{{ error.traceback|linebreaks }}
{% endfor %} + {% block import_error_list %} {% for line, errors in result.row_errors %} {% for error in errors %} -
  • - {% translate "Line number" %}: {{ line }} - {{ error.error }} -
    {{ error.row.values|join:", " }}
    -
    {{ error.traceback|linebreaks }}
    + {% block import_error_list_item %} +
  • + {% if "message" in import_error_display %} +
    {% translate "Line number" %}: {{ line }} - {{ error.error }}
    + {% endif %} + {% if "row" in import_error_display %} +
    {{ error.row.values|join:", " }}
    + {% endif %} + {% if "traceback" in import_error_display %} +
    {{ error.traceback|linebreaks }}
    + {% endif %}
    • + {% endblock %} {% endfor %} {% endfor %} + {% endblock %} {% endblock %} diff --git a/tests/core/tests/admin_integration/test_import.py b/tests/core/tests/admin_integration/test_import.py index b1a34d84b..879cc3721 100644 --- a/tests/core/tests/admin_integration/test_import.py +++ b/tests/core/tests/admin_integration/test_import.py @@ -1,5 +1,6 @@ import os import warnings +from io import StringIO from unittest import mock from unittest.mock import MagicMock, patch @@ -9,7 +10,10 @@ from core.tests.admin_integration.mixins import AdminTestMixin from core.tests.utils import ignore_widget_deprecation_warning from django.contrib.admin.models import DELETION, LogEntry +from django.contrib.admin.sites import AdminSite +from django.contrib.auth.models import User from django.http import HttpRequest +from django.test import RequestFactory from django.test.testcases import TestCase, TransactionTestCase from django.test.utils import override_settings from django.utils.translation import gettext_lazy as _ @@ -480,6 +484,75 @@ def test_export_empty_import_formats(self): self.client.get(self.book_import_url) +class TestImportErrorMessageFormat(AdminTestMixin, TestCase): + # issue 1724 + + def setUp(self): + super().setUp() + self.csvdata = "id,name,author\r\n" "1,Ulysses,666\r\n" + self.filedata = StringIO(self.csvdata) + self.data = {"format": "0", "import_file": self.filedata} + self.model_admin = BookAdmin(Book, AdminSite()) + + factory = RequestFactory() + self.request = factory.post(self.book_import_url, self.data, follow=True) + self.request.user = User.objects.create_user("admin1") + + @ignore_widget_deprecation_warning + def test_result_error_display_default(self): + response = self.model_admin.import_action(self.request) + response.render() + self.assertIn("import-error-display-message", str(response.content)) + self.assertIn( + "Line number: 1 - Author matching query does not exist.", + str(response.content), + ) + self.assertNotIn("import-error-display-row", str(response.content)) + self.assertNotIn("import-error-display-traceback", str(response.content)) + + @ignore_widget_deprecation_warning + def test_result_error_display_message_only(self): + self.model_admin.import_error_display = ("message",) + + response = self.model_admin.import_action(self.request) + response.render() + self.assertIn( + "Line number: 1 - Author matching query does not exist.", + str(response.content), + ) + self.assertIn("import-error-display-message", str(response.content)) + self.assertNotIn("import-error-display-row", str(response.content)) + self.assertNotIn("import-error-display-traceback", str(response.content)) + + @ignore_widget_deprecation_warning + def test_result_error_display_row_only(self): + self.model_admin.import_error_display = ("row",) + + response = self.model_admin.import_action(self.request) + response.render() + self.assertNotIn( + "Line number: 1 - Author matching query does not exist.", + str(response.content), + ) + self.assertNotIn("import-error-display-message", str(response.content)) + self.assertIn("import-error-display-row", str(response.content)) + self.assertNotIn("import-error-display-traceback", str(response.content)) + + @ignore_widget_deprecation_warning + def test_result_error_display_traceback_only(self): + self.model_admin.import_error_display = ("traceback",) + + response = self.model_admin.import_action(self.request) + response.render() + self.assertNotIn( + "Line number: 1 - Author matching query does not exist.", + str(response.content), + ) + self.assertNotIn("import-error-display-message", str(response.content)) + self.assertNotIn("import-error-display-row", str(response.content)) + self.assertIn("import-error-display-traceback", str(response.content)) + + class ConfirmImportEncodingTest(AdminTestMixin, TestCase): """Test handling 'confirm import' step using different file encodings and storage types. diff --git a/tests/core/tests/test_resources/test_modelresource/test_deprecated_fields.py b/tests/core/tests/test_resources/test_modelresource/test_deprecated_fields.py new file mode 100644 index 000000000..22085a500 --- /dev/null +++ b/tests/core/tests/test_resources/test_modelresource/test_deprecated_fields.py @@ -0,0 +1,64 @@ +import warnings + +import tablib +from core.models import Book +from core.tests.resources import BookResource +from django.test import TestCase + +from import_export import resources + + +class DeprecatedMethodTest(TestCase): + """ + These tests relate to renamed methods in v4. + The tests can be removed when the deprecated methods are removed. + """ + + def setUp(self): + rows = [ + ["1", "Ulysses"], + ] + self.dataset = tablib.Dataset(*rows, headers=["id", "name"]) + self.obj = Book.objects.create(id=1, name="Ulysses") + + def test_import_obj_renamed(self): + resource = BookResource() + with self.assertWarns( + DeprecationWarning, + ): + resource.import_obj(self.obj, self.dataset, dry_run=True) + + def test_import_obj_passes_params(self): + class MyBookResource(resources.ModelResource): + def import_instance(self, instance, row, **kwargs): + self.kwargs = kwargs + + class Meta: + model = Book + + resource = MyBookResource() + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=DeprecationWarning) + resource.import_obj(self.obj, self.dataset, True) + self.assertTrue(resource.kwargs["dry_run"]) + + def test_after_import_instance_renamed(self): + resource = BookResource() + with self.assertWarns( + DeprecationWarning, + ): + resource.after_import_instance(self.obj, True, row_number=1) + + def test_after_import_instance_passes_params(self): + class MyBookResource(resources.ModelResource): + def after_init_instance(self, instance, new, row, **kwargs): + self.kwargs = kwargs + + class Meta: + model = Book + + resource = MyBookResource() + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=DeprecationWarning) + resource.after_import_instance(self.obj, True, row_number=1) + self.assertEqual(1, resource.kwargs["row_number"])