Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Admin UI import error messages #1727

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion docs/admin_integration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
---------

Expand Down Expand Up @@ -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
---------

Expand Down
1 change: 1 addition & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Please refer to :doc:`release notes<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)
-------------------------
Expand Down
2 changes: 1 addition & 1 deletion docs/faq.rst
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ Please refer to `this issue <https://github.com/django-import-export/django-impo
How to hide stack trace in UI error messages
--------------------------------------------

Please refer to `this issue <https://github.com/django-import-export/django-import-export/issues/1257#issuecomment-952276485>`_.
Please refer to :ref:`format_ui_error_messages`.

Ids incremented twice during import
-----------------------------------
Expand Down
15 changes: 15 additions & 0 deletions docs/release_notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,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 <https://github.com/django-import-export/django-import-export/issues/1691>`_.

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 <https://github.com/django-import-export/django-import-export/issues/1724>`_.

API changes
===========

Expand Down
5 changes: 5 additions & 0 deletions import_export/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
18 changes: 14 additions & 4 deletions import_export/templates/admin/import_export/import.html
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,25 @@ <h2>{% translate "Errors" %}</h2>
<div class="traceback">{{ error.traceback|linebreaks }}</div>
</li>
{% endfor %}
{% block import_error_list %}
{% for line, errors in result.row_errors %}
{% for error in errors %}
<li>
{% translate "Line number" %}: {{ line }} - {{ error.error }}
<div><code>{{ error.row.values|join:", " }}</code></div>
<div class="traceback">{{ error.traceback|linebreaks }}</div>
{% block import_error_list_item %}
<li class="import-error-li">
{% if "message" in import_error_display %}
<div class="import-error-display-message">{% translate "Line number" %}: {{ line }} - {{ error.error }}</div>
{% endif %}
{% if "row" in import_error_display %}
<div class="import-error-display-row">{{ error.row.values|join:", " }}</div>
{% endif %}
{% if "traceback" in import_error_display %}
<div class="import-error-display-traceback">{{ error.traceback|linebreaks }}</div>
{% endif %}
</li>
{% endblock %}
{% endfor %}
{% endfor %}
{% endblock %}
</ul>
{% endblock %}

Expand Down
73 changes: 73 additions & 0 deletions tests/core/tests/admin_integration/test_import.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import warnings
from io import StringIO
from unittest import mock
from unittest.mock import MagicMock, patch

Expand All @@ -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 _
Expand Down Expand Up @@ -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.
Expand Down