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

Fix issue #261: Handle unique constraint validation error #777

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
24 changes: 21 additions & 3 deletions modeltranslation/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,27 @@ def _get_form_or_formset(
exclude.extend(self.form._meta.exclude)
# If exclude is an empty list we pass None to be consistent with the
# default on modelform_factory
exclude = self.replace_orig_field(exclude) or None
exclude = self._exclude_original_fields(exclude)
kwargs.update({"exclude": exclude})
updated_exclude = self.replace_orig_field(exclude) or None
# Copy default language field value into the original field.
if request.POST:
data = request.POST.copy()
for field in self.trans_opts.fields:
default_lang_field = build_localized_fieldname(field, mt_settings.DEFAULT_LANGUAGE)
if default_lang_field in data:
value = data.get(default_lang_field)
if field not in data.keys() and value:
data.appendlist(field, value)
request.POST = data # type: ignore[assignment]
# In order for BaseModelAdmin to show unique-constraint validation error,
# add the original translation fields to kwargs, and do not exclude the
# original translation fields.
if kwargs.get("fields"):
original_fields = [field for field in self.trans_opts.fields if field not in exclude]
kwargs["fields"].extend(original_fields)
kwargs.update({"exclude": (updated_exclude and list({*exclude, *updated_exclude}))})
Comment on lines +255 to +258
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned it up a bit, and fixed type errors.

And now we can discuss what's happening here.

This branch is where new code added, and it looks like we're adding "original fields" again ... after they were removed? Maybe it's better to look for a place where they were removed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what surprised me here is that we have added back original fields, but none of the tests failed.

I have added test_model_with_constraint_fields in master, and synced it here. But it still detects only title, sub_title_de, sub_title_en fields. This is good, because form looks like before, and bad, because I do not understand why :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning the code.
While debugging I noticed the _get_fieldsets_post_form_or_formset is where the translation fields get added (I could be wrong). Does it make sense to move the logic to add original fields here?

I am not entirely sure how this change fixed the issue. I made the changes purely based on my observation on how the code is executed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see there's replace_orig_field, and it looks like it replaces original fields with translation ones.

We can add option preserve_original_fields with default value False here. And now, instead of adding fields back - we just preserve it here.

I'm not sure if we need preserve_original_fields=False at all, maybe just make preserving originals default behaviour. Check how it works.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I will test it.

else:
updated_exclude = self._exclude_original_fields(updated_exclude)
kwargs.update({"exclude": updated_exclude})
Comment on lines +259 to +261
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the same as removed 243-244 lines. Right?

But, something feels not right here. If we have "fields" specified we're making changes to a code path, but when no fields specified - we're doing all like before?

(I may be wrong here, and maybe we're always have "fields" specified? But looking at the code coverage, this branch is also covered/executed, so it worth investigating)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While debugging I noticed the _get_form_or_formset is called twice, one with fields and one without fields (attached screenshot). If I remove the else block then the translation fields are displayed twice. So, I had to retain the existing logic in the else block.

mt_terminal_log


return kwargs

Expand Down
22 changes: 15 additions & 7 deletions modeltranslation/tests/test_admin_views.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
"""Tests for admin view."""

from django.test import Client
from django.urls import reverse
import pytest

from modeltranslation.tests.models import ModelWithConstraint
from django.db import IntegrityError


def test_create_duplicate(admin_client: Client):
"""Unique constraint error should be handled by TranslationAdmin."""
ModelWithConstraint.objects.create(title="1", sub_title="One")
url = reverse("admin:tests_modelwithconstraint_add")

with pytest.raises(IntegrityError):
response = admin_client.post(
url, {"title": "1", "sub_title_en": "One", "sub_title_de": "Ein"}
)
response = admin_client.post(
url,
{
"title": "1",
"sub_title_en": "One",
"sub_title_de": "Ein",
},
)

assert response.status_code == 302
error_msg = "Model with constraint with this Title and Sub title already exists."
assert error_msg in response.context["errors"][0]
assert response.status_code == 200