[Django] #20836: to_field lost when adding via raw_id_fields
Django
--------------------------------+------------------------
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20836>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by CollinAnderson):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:1>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:2>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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 doesn't not work when adding. (pk is returned).
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:3>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:4>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:5>
Django
Reporter: CollinAnderson | Owner: CollinAnderson
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.6-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by CollinAnderson):
* status: new => assigned
* owner: nobody => CollinAnderson
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:6>
Django
Reporter: CollinAnderson | Owner: CollinAnderson
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:7>
Django
Reporter: CollinAnderson | Owner: CollinAnderson
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:8>
Django
Reporter: CollinAnderson | Owner:
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by CollinAnderson):
* owner: CollinAnderson =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:9>
Django
Reporter: CollinAnderson | Owner: sheats
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.6-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sheats):
* owner: => sheats
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:10>
Django
Reporter: CollinAnderson | Owner: sheats
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sheats):
* needs_tests: 1 => 0
Comment:
I added a selenium test for this issue and added a little to
CollinAnderson's solution.
https://github.com/django/django/pull/1585
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:11>
Django
Reporter: CollinAnderson | Owner: sheats
Component: contrib.admin | Version: 1.6-beta-1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Julien Phalip <jphalip@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"55a11683f7b094ae4fd0b9fa030d18a12657ba98"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20836#comment:12>