Opened 11 years ago

Closed 11 years ago

#20836 closed Bug (fixed)

to_field lost when adding via raw_id_fields

Reported by: Collin Anderson Owned by: Peter Sheats
Component: contrib.admin Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: Collin Anderson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using to_field with raw_id_fields, you get a popup to select an object to connect the ForeignKey to. This correctly handles to_field.

However, you also have the option of adding a new object from the popup. When you add the new object, the popup is closed the the primary key is sent back to the raw_id widget.

As far as I can tell, this has never worked correctly in django. (at least since 1.2)

I have a simple django app that can be used to reproduce the problem:
https://github.com/collinanderson/to_field/commit/0b913d4df7c8ab699aca2886ee619313373f4c8c

Change History (12)

comment:1 by Collin Anderson, 11 years ago

Has patch: set

Basically, TO_FIELD_VAR really needs to follow IS_POPUP_VAR everywhere it goes. Whenever it's a popup, there's always the possibility that there's a to_field.

pull request (against 1.6.x, sorry):
https://github.com/django/django/pull/1417

comment:2 by loic84, 11 years ago

I'm not sure I understand.

I tried your test project: clicked on the magnifying glass, clicked on "Add user", filled out the form, saved, then the popup closed and the PK was added to the raw_id field. Isn't that the expected behavior?

comment:3 by Collin Anderson, 11 years ago

I should be more clear. my field looks like this: user = models.ForeignKey('auth.User', to_field='username').

The to_field is the field that is referenced on the other model. Normally it is the primary key, but Django provides the option to override it. Really, any unique=True field should work in theory.

So the expected behavior is that when the popup is closed I get back the username instead of the PK, because that's what to_field is.

Again, it works correctly when selecting an existing object (username is returned), but does not work when adding. (pk is returned).

Last edited 11 years ago by Collin Anderson (previous) (diff)

comment:4 by loic84, 11 years ago

Triage Stage: UnreviewedAccepted

I think it's a reasonable request, marking it as accepted.

I didn't look into the rest of the patch into detail, but if this GET param is used on add_view and change_view we need to rename it (_tofield maybe?) otherwise it might shadow a field name.

#20288 and [7462a78c1b] may help on how to approach such rename.

comment:5 by Collin Anderson, 11 years ago

Yeah, I was thinking about that when I was creating the pull request. I'm personally not that worried about it because this is such a corner case as it is. I'm apparently the first one to ever notice the raw_id + to_field + adding case, and we would need a raw_id + to_field + adding + having a field named "t" before there would be an issue.

Though, yes, it's possible and it will only be harder to change in the future. And deprecating/changing this during the same release as _popup would make a lot of sense.

Actually, here's a proposal. How about instead of having a separate "t" variable, we simply have _popup=username or _popup=pk (or rename _popup again before 1.6 comes out). I think that would actually simplify the code quite a bit because it's one less thing we need to keep passing around. Currently is_popup is a boolean, and the "t" var only makes sense when _popup is true, so if we're changing things anyway, why not just combine the two into one?

comment:6 by Collin Anderson, 11 years ago

Owner: changed from nobody to Collin Anderson
Status: newassigned

comment:7 by Collin Anderson, 11 years ago

irc conversation:
<loic84> personally I'd keep the 2 variables separate as it's more explicit
<loic84> I think _tofield works well, it makes it easy to grep on the code to find out what it is

comment:8 by Tim Graham, 11 years ago

Needs tests: set

comment:9 by Collin Anderson, 11 years ago

Owner: Collin Anderson removed
Status: assignednew

comment:10 by Peter Sheats, 11 years ago

Owner: set to Peter Sheats
Status: newassigned

comment:11 by Peter Sheats, 11 years ago

Needs tests: unset

I added a selenium test for this issue and added a little to CollinAnderson's solution.

https://github.com/django/django/pull/1585

comment:12 by Julien Phalip <jphalip@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 55a11683f7b094ae4fd0b9fa030d18a12657ba98:

Fixed #20836 -- Ensure that the ForeignKey's to_field attribute is properly considered by the admin's interface when creating related objects.
Many thanks to Collin Anderson for the report and patch and to Peter Sheats for the test.

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