#13696 closed Bug (fixed)
Admin Edit Inline templates don' t output hidden field for PK when it isn't an autoField and has editable=False
Reported by: | Owned by: | Alexander Herrmann | |
---|---|---|---|
Component: | contrib.admin | Version: | 1.2 |
Severity: | Normal | Keywords: | inline templates |
Cc: | evan.reiser@…, issac.kelly@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
In my project I'm using a UUID field where I generate the uuid myself in python, and don't rely on any sequences on the database side. Because of this I just use a CharField field, and when used i set it to be the primary key. However because of this usage the field does not have 'has_auto_field' == True, This causes the edit inline forms for existing objects to not output the hidden field to hold the PK for the existing object.
Right now this template requires the inline_admin_form.has_auto_field == True, for it to output the pk_field.field, however I think this should always output the primary key for the object.
This line occurs on line 30 of contrib/admin/templates/admin/edit_inline/tabular.html in django 1.2.1
{% if inline_admin_form.has_auto_field %}{{ inline_admin_form.pk_field.field }}{% endif %}
contrib/admin/templates/admin/edit_inline/tabular.html
<td class="original"> {% if inline_admin_form.original or inline_admin_form.show_url %}<p> {% if inline_admin_form.original %} {{ inline_admin_form.original }}{% endif %} {% if inline_admin_form.show_url %}<a href="../../../r/{{ inline_admin_form.original_content_type_id }}/{{ inline_admin_form.original.id }}/">{% trans "View on site" %}</a>{% endif %} </p>{% endif %} {% if inline_admin_form.has_auto_field %}{{ inline_admin_form.pk_field.field }}{% endif %} {{ inline_admin_form.fk_field.field }} {% spaceless %} {% for fieldset in inline_admin_form %} {% for line in fieldset %} {% for field in line %} {% if field.is_hidden %} {{ field.field }} {% endif %} {% endfor %} {% endfor %} {% endfor %} {% endspaceless %} </td>
Attachments (1)
Change History (25)
comment:1 by , 14 years ago
Cc: | added |
---|
comment:2 by , 14 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:3 by , 14 years ago
It seems to be that this this problem happens for models which have a primary key field that is not based on the AutoField (such as a CharField).
When the primary key field is not based on an AutoField 'has_auto_field' is not set to True, therefore it will not output the primary key in the form. (as you can see on line 30 of the tabular.html
{% if inline_admin_form.has_auto_field %}{{ inline_admin_form.pk_field.field }}{% endif %}
These inline forms should still work for any primary key (in my opinion), not only when the field is based on an AutoField.
You said there area already tests in place to check for this, but according to the template, I'm not sure how this would be possible unless the field is set to be "has_auto_field" = True.
It seems that changing line 30 to this fixes the problem:
{% if inline_admin_form.pk_field %}{{ inline_admin_form.pk_field.field }}{% endif %}
comment:4 by , 14 years ago
But...it is only AutoFields that are specifically not part of the form by default, so it is only in that case that the admin has to do something special to ensure they are on the form. I'm still missing the problem you are actually observing, vs. the specifics of the reasons for it that you seem to have diagnosed. Could you step back and describe the problem with the admin behavior that you are actually seeing? As I said, we have tests that verify that inlines with non-auto-field PKs can be added and updated. I've also manually verified that myself. I can add and update inlines with explicit non-auto character field PKs. I don't see any exceptions raised or incorrect data saved. So I am still struggling to understand what problem you are observing that would require a change in the admin template here.
comment:5 by , 14 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
For what it's worth, I've been burned by this too.
To clarify a bit: it occurs when the id
field is not an AutoField
and editable=False
. To use the example from the documentation:
import random from django.db import models as m class Author(m.Model): name = m.CharField(max_length=100) class Book(m.Model): id = m.CharField(max_length=32, primary_key=True, editable=False) author = m.ForeignKey(Author) title = m.CharField(max_length=100) def save(self, *args, **kwargs): if not self.id: self.id = str(random.random()) super(Book, self).save(*args, **kwargs)
In the Author
admin page, there is no reference to the id
s of the related Book
s (clean up a bit):
<td class="original"> <p>Book object</p> <!-- this line *should* be here, but isn't: <input type="hidden" name="book_set-0-id" value="0.23411" id="id_book_set-0-id" /> --> <input type="hidden" name="book_set-0-author" value="1" id="id_book_set-0-author" /> </td>
If this can be confirmed, I'll supply tests.
comment:6 by , 14 years ago
As a warning: the “obvious” work around for this is to have custom ID fields “pretend” to be auto fields by doing something like this:
def contribute_to_class(...): # XXX BAD XXX cls._meta.has_auto_field = True ls._meta.auto_field = self
However, this doesn't quite work either, because line 525 of /Users/wolever/Verso/Repos/web/env/massuni-web/lib/python2.6/site-packages/django/db/models/base.py
in save_base
(update_pk = bool(meta.has_auto_field and not pk_set)
), sets the update_pk
flag, which essentially causes:
if update_pk: instance.id = get_last_inserted_id()
And, at least with the SQLite backend, the result of get_last_inserted_id
is the row number (or similar), *not* the value of the id
field.
So, in short, this issue can't be hacked around by setting has_auto_field
.
comment:7 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
The combination of editable=False and a non-auto primary key field appears to be the key here. On first inspection, it looks to me like the inlines should be looking for a "is primary key and not editable", rather than "is AutoField", as the condition for displaying the hidden ID column.
comment:8 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:9 by , 14 years ago
Easy pickings: | set |
---|
This is a very easy fix, all you need to do is change this line:
{% if inline_admin_form.has_auto_field %}{{ inline_admin_form.pk_field.field }}{% endif %}
to this:
{% if inline_admin_form.pk_field %}{{ inline_admin_form.pk_field.field }}{% endif %}
comment:10 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
UI/UX: | unset |
comment:11 by , 13 years ago
Status: | new → assigned |
---|
follow-up: 13 comment:12 by , 13 years ago
Has patch: | set |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
follow-up: 14 comment:13 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Replying to dArignac:
Status changed from assigned to closed
Thanks for the patch! But there is no need to close the ticket as fixed, the has patch one is the way to mark it has a fix a fix ready for review.
Fixed is used when/if the issue is actually fixed either by applying it (or another fix) to the code (by a core committer) . Otherwise it will be completely under the radar for people looking for unsolved tickets or patches to review and commit, causing effectively the exact contrary effect to the one you intended when you worked in the issue in the first place.
See https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/#closing-tickets
comment:14 by , 13 years ago
Oh ok, sorry, so I got it wrong. Will not happen next time!
Replying to ramiro:
Replying to dArignac:
Status changed from assigned to closed
Thanks for the patch! But there is no need to close the ticket as fixed, the has patch one is the way to mark it has a fix a fix ready for review.
Fixed is used when/if the issue is actually fixed either by applying it (or another fix) to the code (by a core committer) . Otherwise it will be completely under the radar for people looking for unsolved tickets or patches to review and commit, causing effectively the exact contrary effect to the one you intended when you worked in the issue in the first place.
See https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/#closing-tickets
comment:15 by , 13 years ago
Cc: | added |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Tested against 1.4-pre-alpha and still fixes this issue.
comment:16 by , 13 years ago
Summary: | Admin Edit Inline templates only output hidden field for PK when inline_admin_form.has_auto_field → Admin Edit Inline templates don' t output hidden field for PK when it isn't an autoField and has editable=False |
---|---|
Triage Stage: | Ready for checkin → Accepted |
The note in this comment: https://code.djangoproject.com/browser/django/trunk/django/forms/models.py?rev=17373#L637 seems to indicate that AutoField's editable value isn't False so I don't think this patch is ready for commit. The condition in the if test would need to be a bit more complex than the two proposed so far: Checking for the field being a PK field (as implemented in the patch) or checking for it being a PK and not editable.
comment:17 by , 12 years ago
I don't get where exactly the problem now is. If I include an AutoField setting its property "editable" to True, it still cannot be edited within the AdminSite, there is also no problem saving this. For all custom Fields being primary keys (primary_key=True) the current fix does work. Please explain, thanks!
comment:18 by , 12 years ago
I am affected by a very similar problem (though technically unrelated) in the same template. When I have a model that has a field that is not named id, the "View on site" link in the inline editing section is missing the pk in the url. So the link ends up going to nowhere. The relevant code is:
{% if inline_admin_form.show_url %}<a href="../../../r/{{ inline_admin_form.original_content_type_id }}/{{ inline_admin_form.original.id }}/">{% trans "View on site" %}</a>{% endif %}
Instead of {{ inline_admin_form.original.id }}, I think it should be {{ inline_admin_form.original.pk }}.
comment:19 by , 12 years ago
Brianglass: Yes. I agree. It's such an obvious and easy fix, its too bad that there is so much bureaucracy around changing it.
The underlying problem is that the code is assuming you always have primary keys named "id" that are auto incrementing, which obviously isn't the case.
comment:20 by , 12 years ago
Status: | reopened → new |
---|
comment:21 by , 12 years ago
We've been bitten by this as well. This bug has been replicated in Django grappelli. So for anyone that bumps into this bug you can override admin/edit_inline/tabular.html and/or admin/edit_inline/stacked.html similarly to the patch that's been posted here.
Also note that django_extensions' UUIDField contains a hack to work around this bug. It pretends to be an auto field, which doesn't work well, as pointed out by wolever above. I'm trying to get that hack removed from django-extensions: https://github.com/django-extensions/django-extensions/pull/332
comment:22 by , 11 years ago
Pull request with slightly altered fix: https://github.com/django/django/pull/1381
The proposed fix duplicated the pk field on the inlines if there was a non-auto EDITABLE pk field already rendered. The pull request tests for that (previous fix would fail that test) in addition to testing for the non-auto non-editable pk.
comment:23 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
The problem description does not mention what the problem is, exactly. It states how some code behaves in a particular circumstance, but leaves out what the negative effect of that behavior might be? We have tests that verify that inlines with explicit non-auto PK fields can be created, saved, edited and saved again in the admin (see http://code.djangoproject.com/ticket/13696), so I'm struggling to understand what exactly the problem reported here is? What exactly is not working properly?