#13091: admin list_editable with unique_together raises Integrity Error
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: contrib.admin
Version: SVN | Severity: Normal
Resolution: | Keywords: list_editable
Triage Stage: Design | unique_together IntegrityError
decision needed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by carljm):
* needs_better_patch: 0 => 1
Comment:
Latest patch is much closer. Thanks for the patch! Some comments:
* The versionadded note in the validate_unique docs should probably go
right below the modified paragraph, not at the top of the section.
* The "as of Django 1.2" note in the unique_together docs should use the
versionadded marker.
* This patch doesn't require adding another method to model _meta; it's
already bloated. Use the existing .get_field_by_name() method and discard
the values you don't need in the returned tuple.
* The use of check_count is a bit convoluted and hard to understand. Given
the new logic, I think it would be clearer to drop the use of for...else
and just have a boolean perform_check that gets flipped True if any field
in the check is found to not be excluded and not have a default (and when
its flipped to True you can break from the loop) and then after the loop
use the boolean to decide whether to add the check to the unique_checks
list.
* Why does the admin list_editable test post values for t3 in the POST
data when that field is not in list_editable? Does the admin actually post
values from hidden inputs for all fields not included in list_editable?
This test should be checked to make sure it matches what the admin
actually does.
* I'm not sure checking if default is NOT_PROVIDED is actually sufficient.
In particular, I think a CharField blank=True but with no default
specified could still break if there's a record in the database with that
field blank, because empty string is the implicit default. We need a test
for this case.
* The test where values are set on form.instance prior to validation isn't
really relevant, because that's not something Django does internally (e.g.
in the admin), nor is it a documented pattern we need to ensure works. It
would be better to replace that test with a case like the one in the
#15326 report, a real-world case that is currently broken.
It's late so I won't guarantee there isn't more that I'm missing, but
that's what I see on first review.
--
Ticket URL: <http://code.djangoproject.com/ticket/13091#comment:14>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
