Opened 6 years ago

Closed 4 years ago

Last modified 4 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: nessita 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@… 6 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@… 6 years ago.
Testcase testing django.contrib.admin
buggy-html-for-errors.png (69.0 KB) - added by nessita 5 years ago.
models.py (841 bytes) - added by nessita 5 years ago.
tests.py (1.1 KB) - added by nessita 5 years ago.
12749-tests.diff (2.6 KB) - added by jkocherhans 5 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 russellm 5 years ago.
Draft fix; passes nessita's test, but breaks others in model_formsets
t12749.diff (4.9 KB) - added by russellm 5 years ago.
Updated, fully working patch.

Download all attachments as: .zip

Change History (22)

Changed 6 years ago by Chris.Wesseling@…

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

Changed 6 years ago by Chris.Wesseling@…

Testcase testing django.contrib.admin

comment:1 Changed 5 years ago by jezdez

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by nessita

  • Owner changed from nobody to nessita
  • Status changed from new to assigned

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 follow-up: Changed 5 years ago by nessita

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 5 years ago by nessita

Changed 5 years ago by nessita

Changed 5 years ago by nessita

comment:5 Changed 5 years ago by nessita

Running tests with:

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

exercises this issue.

comment:6 Changed 5 years ago by nessita

  • Owner changed from nessita to nobody
  • Status changed from assigned to new

comment:7 Changed 5 years ago by nessita

  • Cc nessita added

Changed 5 years ago by jkocherhans

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

comment:8 Changed 5 years ago by jkocherhans

  • Owner changed from nobody to jkocherhans
  • Status changed from new to assigned

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 5 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 5 years ago by russellm

  • 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 5 years ago by russellm

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

Changed 5 years ago by russellm

Updated, fully working patch.

comment:11 Changed 5 years ago by russellm

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

(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 4 years ago by anonymous

  • Easy pickings unset
  • Resolution fixed deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to 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 4 years ago by lukeplant

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

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 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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