Code

Opened 9 months ago

Closed 7 months ago

#20836 closed Bug (fixed)

to_field lost when adding via raw_id_fields

Reported by: CollinAnderson Owned by: sheats
Component: contrib.admin Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: CollinAnderson 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

Attachments (0)

Change History (12)

comment:1 Changed 9 months ago by CollinAnderson

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 9 months 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 9 months ago by CollinAnderson

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 9 months ago by CollinAnderson (previous) (diff)

comment:4 Changed 9 months ago by loic84

  • Triage Stage changed from Unreviewed to Accepted

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 9 months ago by CollinAnderson

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 9 months ago by CollinAnderson

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

comment:7 Changed 9 months ago by CollinAnderson

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 9 months ago by timo

  • Needs tests set

comment:9 Changed 8 months ago by CollinAnderson

  • Owner CollinAnderson deleted
  • Status changed from assigned to new

comment:10 Changed 7 months ago by sheats

  • Owner set to sheats
  • Status changed from new to assigned

comment:11 Changed 7 months ago by sheats

  • 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 Changed 7 months ago by Julien Phalip <jphalip@…>

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.