Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8241 closed (fixed)

Primary ForeignKeys don't work with FormSets

Reported by: Samuel Cormier-Iijima Owned by: Brian Rosner
Component: contrib.admin Version: dev
Severity: Keywords:
Cc: sciyoshi@…, ville@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using this UserProfile as an example:

class UserProfile(models.Model):
    user = models.OneToOneField(primary_key=True)
    nickname = models.CharField(max_length=30)

from django.contrib.auth.models import User
from django.contrib.auth.admin import UserAdmin
from django.contrib.admin import site, StackedInline

class UserProfileAdmin(StackedInline)
    model = UserProfile
    max_num = 1

UserAdmin.inlines = UserAdmin.inlines + [UserProfileAdmin]

site.unregister(User)
site.register(User, UserAdmin)

Trying to save a User from the admin causes a KeyError: 'user_id' because the primary key doesn't get rendered in the form.

Attachments (3)

8241.patch (2.2 KB ) - added by Samuel Cormier-Iijima 16 years ago.
onetoone_admin.8461.diff (2.7 KB ) - added by Ilya Semenov 16 years ago.
onetoone_admin.8541.diff (3.2 KB ) - added by Ilya Semenov 16 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by Samuel Cormier-Iijima, 16 years ago

That should be

user = models.OneToOneField(User, primary_key=True)

comment:2 by Jacob, 16 years ago

Component: UncategorizedAdmin interface
milestone: 1.0
Triage Stage: UnreviewedAccepted

comment:3 by Marc Remolt, 16 years ago

Isn't this a duplicate for or at least related to #7947 ?

in reply to:  3 comment:4 by Samuel Cormier-Iijima, 16 years ago

Replying to mremolt:

Isn't this a duplicate for or at least related to #7947 ?

I don't think so; applying the patch from that ticket doesn't fix the bug, and the same behavior also occurs if we have

user = models.ForeignKey(User, primary_key=True)

I think the problem could be fixed somewhere around add_fields in BaseModelFormSet:

    def add_fields(self, form, index):
        """Add a hidden field for the object's primary key."""
        if self.model._meta.has_auto_field:
            self._pk_field_name = self.model._meta.pk.attname
            form.fields[self._pk_field_name] = IntegerField(required=False, widget=HiddenInput)
        super(BaseModelFormSet, self).add_fields(form, index)

If the primary key isn't an AutoField it doesn't get added to the fields here...

comment:5 by Samuel Cormier-Iijima, 16 years ago

The following patch fixes it for me (for OneToOneFields the patch from #7949 needs to be applied first), but probably breaks other things; namely, model_formsets fails. If anybody has ideas on how to better implement this let me know...

by Samuel Cormier-Iijima, 16 years ago

Attachment: 8241.patch added

comment:6 by Samuel Cormier-Iijima, 16 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:7 by Ilya Semenov, 16 years ago

Patch needs improvement: unset

sciyoshi, first of all, thanks for your patch. It inspired me for further analysis of the problem. :)

From what I've discovered, your patch suffers from the following:

  1. It curcumvents into the naming convention of form fields and adds a form field named user_id (i.e., taken as field.attname, not as field.name). At first glance, that seems to be ok: the field is filled by model_to_dict(), passed thru browser POST, but then ignored by save_instance(). Alas, that would break things if a model had TWO fields like user=OneToOneField() and user_id=CharField(). We shouldn't be messing with the naming convention: all form fields should always use field.name as their names.
  1. The patch breaks BaseModelFormSet.add_fields() contract, which currently adds a new field if (and only if) a primary key is an AutoField -- what I consider to be a desired behaviour.

I'm attaching the updated patch (vs [8461]), which resolves the both problems.

by Ilya Semenov, 16 years ago

Attachment: onetoone_admin.8461.diff added

comment:8 by Ilya Semenov, 16 years ago

Also, I didn't get the idea behind parent_link, which was in your patch, and also was discussed in #7947. I replaced that with:

if not kwargs.get('primary_key', False): 
    kwargs['editable'] = False 

which I admit might be wrong for some obscure reason...

comment:9 by Ilya Semenov, 16 years ago

If anybody cares, I'm attaching the updated patch against [8541], since [8528] broke things again (but in a different way).

by Ilya Semenov, 16 years ago

Attachment: onetoone_admin.8541.diff added

comment:10 by Ilya Semenov, 16 years ago

Note: I really don't like my drill down until we find a "real" pk approach; but I don't have enough deep knowledge of django/db internals, so that was the only way that seemed correct to me (more or less).

comment:11 by Ville Säävuori, 16 years ago

Cc: ville@… added

comment:12 by Brian Rosner, 16 years ago

Resolution: invalid
Status: newclosed

It appears this ticket has been caught in the middle of some commit rapid fire. In the fire the issue reported here is fixed. The issue that semenov brings up is not related, but I think should be fixed. Can you please open a ticket talking about the namespace stomping. Please reopen if the issue reported has not been fixed on trunk with detailed and concise use cases.

comment:13 by Brian Rosner, 16 years ago

Resolution: invalid
Status: closedreopened

Ok. I take back that this is fixed. I had a bad test case. Reopening.

comment:14 by Brian Rosner, 16 years ago

Owner: changed from nobody to Brian Rosner
Status: reopenednew

comment:15 by Brian Rosner, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in [8756].

comment:16 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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