Code

Opened 6 years ago

Closed 6 years ago

#7682 closed (fixed)

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

Reported by: jleingang Owned by: mtredinnick
Component: contrib.admin Version: master
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: UI/UX:

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@…> 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by anonymous

  • Cc ristretto.rb@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by Karen Tracey <kmtracey@…>

  • Triage Stage changed from Unreviewed to Accepted

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 Changed 6 years ago by Karen Tracey <kmtracey@…>

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

comment:4 Changed 6 years ago by puyb@…

  • 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 Changed 6 years ago by Karen Tracey <kmtracey@…>

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 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

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.

Changed 6 years ago by Karen Tracey <kmtracey@…>

comment:7 Changed 6 years ago by Karen Tracey <kmtracey@…>

  • 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 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.