Fix issue #261: Handle unique constraint validation error #777
Conversation
|
That's interesting issue. Can you please show how it's looking now on the screenshots? |
|
I have created a Django project to reproduce the issue. You can clone the repo to verify the fix. I have also attached the video for reference on how the error would look like.
Use the Checkout the model_translation_unique_contraint_fix.webm |
|
Thanks for detailed reproduction. I have updates our tests, and now you can modify Sync your branch with master, and try running tests locally. |
|
I have synced the branch with Thanks for adding tests. |
| 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}))}) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good idea. I will test it.
| else: | ||
| updated_exclude = self._exclude_original_fields(updated_exclude) | ||
| kwargs.update({"exclude": updated_exclude}) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
Also. maybe we don't need all this and can use another way. Validation errors for unique fields is collected at django I'll be grateful if you can check this idea. |
|
Sure. I will look into that. |
Issue: When a translation field is added to the unique constraint, the validation error is not captured by the TranslationAdmin causing 500 error.
Root cause: The TranslationAdmin removes the original field from the form and the BaseModelForm does not perform validation check for the fields that are not present in the form, resulting in unhandled exception when Django attempts to create or update the instance.
Fix: The original field is added back to the form fields after the form is rendered, allowing BaseModelForm to perform unique constraint validation for translation fields.