Opened 6 years ago

Closed 6 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


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:

Change History (12)

comment:1 Changed 6 years ago by Collin Anderson

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):

comment:2 Changed 6 years ago by loic84

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

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 6 years ago by Collin Anderson (previous) (diff)

comment:4 Changed 6 years ago by loic84

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

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

Owner: changed from nobody to Collin Anderson
Status: newassigned

comment:7 Changed 6 years ago by Collin Anderson

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

Needs tests: set

comment:9 Changed 6 years ago by Collin Anderson

Owner: Collin Anderson deleted
Status: assignednew

comment:10 Changed 6 years ago by Peter Sheats

Owner: set to Peter Sheats
Status: newassigned

comment:11 Changed 6 years ago by Peter Sheats

Needs tests: unset

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

comment:12 Changed 6 years ago by Julien Phalip <jphalip@…>

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