Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#12749 closed Uncategorized (fixed)

"Please correct the error below." when saving add model form with inline formset and no auto primary key.

Reported by: Chris.Wesseling@… Owned by: jkocherhans
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Natalia Bidart Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX:

Description

When a Model has a field with primary_key=True and the Admin has an Inline ManyToMany, trying to save will result in "Please correct the error below." with no errors shown.

This has been introduced in [12206].
Just the changes in django/contrib/admin/options.py to be precise.
I expected reverting the patch would result in failing tests. But no tests failed.
So either [12206] contained noise code that was doing nothing or it needed better tests.

I've written a test for my usecase that was broken by [12206] and I suggest that someone that knows the purpose
of the options.py part of 12206 expresses that purpose in a test.

I'll add both my test and a patch containing part of the faulty changeset, but triagers, this ticket might need a better patch.

Attachments (8)

12206.patch (2.1 KB) - added by Chris.Wesseling@… 7 years ago.
Patch containing faulty bits of changeset 12206. patch using -R or --reverse
primary_key_inline.tgz (2.6 KB) - added by Chris.Wesseling@… 7 years ago.
Testcase testing django.contrib.admin
buggy-html-for-errors.png (69.0 KB) - added by Natalia Bidart 7 years ago.
models.py (841 bytes) - added by Natalia Bidart 7 years ago.
tests.py (1.1 KB) - added by Natalia Bidart 7 years ago.
12749-tests.diff (2.6 KB) - added by jkocherhans 7 years ago.
Pull the code from nessita's patches into an extra test case for admin_inlines.
t12749.draft-fix.diff (4.7 KB) - added by Russell Keith-Magee 7 years ago.
Draft fix; passes nessita's test, but breaks others in model_formsets
t12749.diff (4.9 KB) - added by Russell Keith-Magee 7 years ago.
Updated, fully working patch.

Download all attachments as: .zip

Change History (22)

Changed 7 years ago by Chris.Wesseling@…

Attachment: 12206.patch added

Patch containing faulty bits of changeset 12206. patch using -R or --reverse

Changed 7 years ago by Chris.Wesseling@…

Attachment: primary_key_inline.tgz added

Testcase testing django.contrib.admin

comment:1 Changed 7 years ago by Jannis Leidel

milestone: 1.2

comment:2 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by Natalia Bidart

Owner: changed from nobody to Natalia Bidart
Status: newassigned

This ticket has two sub-problems in it:

  • django/contrib/admin/templates/admin/change_form.html should not iterate over the adminform.form.non_field_errors and should not build the <ul> by its own, but use the {{ non_field_errors }} directly instead. We may need to check the rest of the forms for this problem. I'll be doing the follow up and submitting patches to ticket #11681.
  • For this particular example, I'm also getting the error "Model fashionista with pk 1 does not exist.", I have to investigate this further.

comment:4 Changed 7 years ago by Natalia Bidart

I've reproduced the problem using the attached models.py and tests.py files.
After a lot of (a little bit frustrating) debugging, I've come up to the following conclusions:

  • The design presented by the reporter doesn't seem to make sense, since it generates a mutual dependency between the model Fashionista and ShoppingWeakness: to create a Fashionista, an instance of ShoppingWeakness is needed; and to create the latter, an instance of Fashionista is needed.
  • The attempt to create a Fashionista ends up with the error Model fashionista with pk 1 does not exist but this error is never shown in the admin's change_form.html, so the user only sees the Please correct the error below message and nothing else.
  • When printing the {{ errors }} value in the change_form.html template, there seems to be a buggy generated HTML, as per the screenshot attached.

Changed 7 years ago by Natalia Bidart

Attachment: buggy-html-for-errors.png added

Changed 7 years ago by Natalia Bidart

Attachment: models.py added

Changed 7 years ago by Natalia Bidart

Attachment: tests.py added

comment:5 Changed 7 years ago by Natalia Bidart

Running tests with:

(django-sprint)nessita@miro:~/pycon/fix_12749$ ./manage.py test primary_key_inline

exercises this issue.

comment:6 Changed 7 years ago by Natalia Bidart

Owner: changed from Natalia Bidart to nobody
Status: assignednew

comment:7 Changed 7 years ago by Natalia Bidart

Cc: Natalia Bidart added

Changed 7 years ago by jkocherhans

Attachment: 12749-tests.diff added

Pull the code from nessita's patches into an extra test case for admin_inlines.

comment:8 Changed 7 years ago by jkocherhans

Owner: changed from nobody to jkocherhans
Status: newassigned

I'm not quite sure how to fix this yet, but the fix should probably be in ForeignKey.validate(). Generally, when we're adding inline objects, the value of the FK to the parent object will be None since the parent hasn't been saved yet. If the value is None, the ForeignKey validation is skipped. However, in this case, the ForeignKey has a value of 1 since the Person object has already been saved. Person isn't the *real* parent object, but the real parent's primary key is a ForeignKey to Person.

[12206] just rolled back part of model validation, but it was the part of model validation that allowed cases like this to pass. Reverting that isn't an option.

comment:9 in reply to:  4 Changed 7 years ago by anonymous

Replying to nessita:

  • The design presented by the reporter doesn't seem to make sense, since it generates a mutual dependency between the model Fashionista and ShoppingWeakness: to create a Fashionista, an instance of ShoppingWeakness is needed; and to create the latter, an instance of Fashionista is needed.

It's legal to create a Fashionista without ShoppingWeakness (blank=True). In fact, that's the only way to do it now: save the Fashionista, then apply Weaknesses. The UI shortcut of having inline forms for the Weaknesses is just broken.

This Unittest was "designed" just to point that out.

As for your little frustration.. I really relate to that. I bumped into this
after some time without updating trunk. After a lot of weeding and searching, I could boil it down to this example and part of changeset.

Fixing it was difficult cause the intention of [12206] wasn't really clear to
me, and not fully documented/locked in a unittest.

It would still be very helpful if all of the model validation would have a
proper unittest so a rollback would break tests.

comment:10 Changed 7 years ago by Russell Keith-Magee

Patch needs improvement: set

@jkocherhans - I'm not completely certain ForeignKey.validate() is at fault here.

You can make the reported failure go away by adding the following check:

if self.rel.to._meta.get_field(self.rel.field_name).rel:
    return

to ForeignKey.validate(). The #12507-related comment indicates that the 'value not supplied' case is usually when saving new inlines; this snippet catches the case of an inline where the inlined foreign key points to an object whose primary key is a OneToOneField or ForeignKey.

However, I think this reveals a deeper problem with validation. The current if value is None: return is applied to *every* foreign key, not just the ones in inlines. This isn't an immediate problem for the Null case, since the only additional validation that is performed is that the related object actually exists. However, it isn't true to say that every foreign key to a RelatedField can be handled in the same way. The need to skip validation for inlines is the exception not the rule.

It seems to me that the problem is a layer higher up. Regardless of the value, it's the inlined foreign key that shouldn't ever be validated. I think the problem lies in _get_validation_exclusions() not identifying an inlined foreign key as something that should be excluded from validation when creating a new object. I've attached a sample implementation that passes the provided test case; however, this breaks 3 other cases in model_formset. I need to dig deeper to work out why, but it's getting late and I can't see the problem right now.

Changed 7 years ago by Russell Keith-Magee

Attachment: t12749.draft-fix.diff added

Draft fix; passes nessita's test, but breaks others in model_formsets

Changed 7 years ago by Russell Keith-Magee

Attachment: t12749.diff added

Updated, fully working patch.

comment:11 Changed 7 years ago by Russell Keith-Magee

Resolution: fixed
Status: assignedclosed

(In [13034]) Fixed #12749 -- Corrected a problem with validation of inline primary keys. Thanks to Chris.Wesseling@… for the report, and nessita for the test case.

comment:12 Changed 6 years ago by anonymous

Easy pickings: unset
Resolution: fixed
Severity: Normal
Status: closedreopened
Type: Uncategorized

i'm using Django 1.2.5 and experiencing the same. My model consists of:

Name = models.CharField(unique=True, db_index=True, max_length=255)
Outdoor = models.BooleanField(default=False, blank=True)
Floor = models.IntegerField(null=True, blank=True)
Disabled = models.BooleanField(default=False, blank=True)

my LocationAdmin(admin.ModelAdmin) consists of:

list_display = ('Name','Floor','Outdoor','Disabled')
list_editable = ('Name','Floor','Outdoor','Disabled')

When I try to tick Disabled or edit anything else in the Admin interface, clicking on save just shows: "Please correct the errors below." and no errors are given

Wasn't this supposed to have been fixed for 1.2? :/

comment:13 Changed 6 years ago by Luke Plant

Resolution: fixed
Status: reopenedclosed

Your bug does not sound like the original bug, which was about models with no auto primary key, and with inlines in the admin - neither of which appear to describe your situation. Please open a new bug with enough information to reproduce your problem.

comment:14 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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