Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#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: evan.reiser@… 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)

13696.diff (5.5 KB ) - added by Alexander Herrmann 13 years ago.
the patch for the issue

Download all attachments as: .zip

Change History (25)

comment:1 by Evan Reiser <evan.reiser@…>, 14 years ago

Cc: evan.reiser@… added

comment:2 by Karen Tracey, 14 years ago

Resolution: invalid
Status: newclosed

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?

comment:3 by Evan Reiser <evan.reiser@…>, 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 Karen Tracey, 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 David Wolever, 13 years ago

Resolution: invalid
Status: closedreopened

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 ids of the related Books (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 David Wolever, 13 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 Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

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 Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:9 by evan.reiser@…, 13 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 Alexander Herrmann, 13 years ago

Owner: changed from nobody to Alexander Herrmann
Status: reopenednew
UI/UX: unset

comment:11 by Alexander Herrmann, 13 years ago

Status: newassigned

by Alexander Herrmann, 13 years ago

Attachment: 13696.diff added

the patch for the issue

comment:12 by Alexander Herrmann, 13 years ago

Has patch: set
Resolution: fixed
Status: assignedclosed

in reply to:  12 ; comment:13 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: closedreopened

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

Last edited 13 years ago by Ramiro Morales (previous) (diff)

in reply to:  13 comment:14 by Alexander Herrmann, 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 Issac Kelly, 12 years ago

Cc: issac.kelly@… added
Triage Stage: AcceptedReady for checkin

Tested against 1.4-pre-alpha and still fixes this issue.

comment:16 by Ramiro Morales, 12 years ago

Summary: Admin Edit Inline templates only output hidden field for PK when inline_admin_form.has_auto_fieldAdmin Edit Inline templates don' t output hidden field for PK when it isn't an autoField and has editable=False
Triage Stage: Ready for checkinAccepted

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.

Last edited 12 years ago by Ramiro Morales (previous) (diff)

comment:17 by Alexander Herrmann, 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 brianglass, 11 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 evan.reiser@…, 11 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 Aymeric Augustin, 11 years ago

Status: reopenednew

comment:21 by anonymous, 11 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 Karen Tracey, 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 Karen Tracey <kmtracey@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 3aad955ea8db1592fad0012155eaa25b72e50dc5:

Fixed #13696 -- ensured inline pk field is rendered

comment:24 by Karen Tracey <kmtracey@…>, 11 years ago

In 706e542eb5db0d14277f34566d351b875e645bac:

[1.6.x] Fixed #13696 -- ensured inline pk field is rendered

Backport of 3aad955ea8db1592fad0012155eaa25b72e50dc5 from master.

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