Opened 16 years ago

Closed 16 years ago

#7682 closed (fixed)

edit_inline issues error(s) incorrectly when going from revision 7710 to TRUNK

Reported by: jleingang Owned by: Malcolm Tredinnick
Component: contrib.admin Version: dev
Severity: Keywords: edit_inline validation
Cc: ristretto.rb@…, puyb@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

models referenced here: http://dpaste.com/61474/

Users edit the memberpledge in the admin interface; when saving this validation error occurs:

{'billinginfo.0.id': [u'Billing Information with this ID already exists.']}

Attachments (1)

7682.diff (827 bytes ) - added by Karen Tracey <kmtracey@…> 16 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by anonymous, 16 years ago

Cc: ristretto.rb@… added

comment:2 by Karen Tracey <kmtracey@…>, 16 years ago

Triage Stage: UnreviewedAccepted

This bug seems to have been introduced in r7790. Specifically I think the changes to how unique is calculated in django.db.models.__init__.py around line 170 is causing the manipulator_validator_unique validator to be associated with the hidden input primary key fields for inline-edited objects, but that validator looks up the primary key in the parent object's table, not the inline-edited object's table, so its results are incorrect.

comment:3 by Karen Tracey <kmtracey@…>, 16 years ago

(Prior to r7790 these fields had no validators associated with them.)

comment:4 by puyb@…, 16 years ago

Cc: puyb@… added

As Karen Tracey say, the problem seems to be in db/models/fields/init.py at line 329

Before r7790, on the id field, the unique attribute return false even if the field was a primary key. r7790 correct this, but introduce the bug.
Am i wrong ?

I changed the condition on line 329 from :

if self.unique or (self.primary_key and not rel):

to :

if not rel and (self.unique or self.primary_key):

It work for me, but, i don't know if this may not create some problems when you have a related object with a unique field (other than the primary key). This may need more tests.

comment:5 by Karen Tracey <kmtracey@…>, 16 years ago

Having an inline-edited object with unique=True specified on a field is a known problem. See #565 and at least six other tickets marked as dups.

Another solution to this problem is to switch to newforms-admin, which doesn't use the old manipulator/validator framework. Ordinarily I'd say old admin problems aren't worth fixing but this is a recently introduced bug that entirely breaks inline editing on trunk, so unless newforms-admin is merging Today (which it might be, I thought the merge target was before the first sprint, which is coming up this weekend, right?) this seems worth fixing.

comment:6 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick

I'll look at this (although it will be 12 - 24 hours before I get to it), since it was my change that did it. My inclination, though, is that given the change in question fixed some major problems and given that newforms-admin is so close, people really needing edit_inline support can manage to not "svn update" for a few days without the universe grinding to a halt. So if it's not trivial to fix, it might just be hard cheese until the merge. That said, I will look at it. It might be as easy as indicated in the above comment.

by Karen Tracey <kmtracey@…>, 16 years ago

Attachment: 7682.diff added

comment:7 by Karen Tracey <kmtracey@…>, 16 years ago

Has patch: set

So I probably overstated how soon nfa needs to land to make this worthwhile fixing, but I'm impatient for that merge to happen and a bit at a loss to know how to help it along. FWIW the oneliner change (which I've attached as a patch) posted by puyb fixes the case I tracked down and allows me to freely switch my own app (which uses inlines and non-default-named ID fields, which caused a different error when the validator was attached to the hidden fields) between nfa and trunk admin as I've been doing for testing purposes for the last several months. It seems harmless but I'll admit it's in code that I've assiduously avoided learning anything about since it's supposed to be going away real soon now.

comment:8 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [7882]) Fixed #7682 -- Added a work around to keep existing admin alive until
newforms-admin merges (to handle changes in [7790]).

This code becomes irrelevant in a non-oldforms world, so even if it's not quite
correct, it will do for now. Based on a patch from puyb@… and Karen
Tracey.

Note: See TracTickets for help on using tickets.
Back to Top