Code

Opened 4 years ago

Closed 12 months ago

Last modified 12 months 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: dArignac
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 dArignac 3 years ago.
the patch for the issue

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by Evan Reiser <evan.reiser@…>

  • Cc evan.reiser@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by kmtracey

  • Resolution set to invalid
  • Status changed from new to 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?

comment:3 Changed 4 years ago by Evan Reiser <evan.reiser@…>

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 Changed 4 years ago by kmtracey

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 Changed 4 years ago by wolever

  • Resolution invalid deleted
  • Status changed from closed to 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 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 Changed 4 years ago by wolever

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 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to 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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 3 years ago by evan.reiser@…

  • 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 Changed 3 years ago by dArignac

  • Owner changed from nobody to dArignac
  • Status changed from reopened to new
  • UI/UX unset

comment:11 Changed 3 years ago by dArignac

  • Status changed from new to assigned

Changed 3 years ago by dArignac

the patch for the issue

comment:12 follow-up: Changed 3 years ago by dArignac

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

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by ramiro

  • Resolution fixed deleted
  • Status changed from closed to 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 time.

See https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/#closing-tickets

Version 0, edited 3 years ago by ramiro (next)

comment:14 in reply to: ↑ 13 Changed 3 years ago by dArignac

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 Changed 3 years ago by issackelly

  • Cc issac.kelly@… added
  • Triage Stage changed from Accepted to Ready for checkin

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

comment:16 Changed 2 years ago by ramiro

  • Summary changed from Admin Edit Inline templates only output hidden field for PK when inline_admin_form.has_auto_field to Admin Edit Inline templates don' t output hidden field for PK when it isn't an autoField and has editable=False
  • Triage Stage changed from Ready for checkin to 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.

Last edited 2 years ago by ramiro (previous) (diff)

comment:17 Changed 2 years ago by dArignac

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 Changed 21 months ago by brianglass

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 Changed 21 months ago by evan.reiser@…

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 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

comment:21 Changed 14 months ago by anonymous

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 Changed 12 months ago by kmtracey

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 Changed 12 months ago by Karen Tracey <kmtracey@…>

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

In 3aad955ea8db1592fad0012155eaa25b72e50dc5:

Fixed #13696 -- ensured inline pk field is rendered

comment:24 Changed 12 months ago by Karen Tracey <kmtracey@…>

In 706e542eb5db0d14277f34566d351b875e645bac:

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

Backport of 3aad955ea8db1592fad0012155eaa25b72e50dc5 from master.

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.