Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13618 closed Bug (fixed)

prepopulated_fields crashes with get_readonly_fields

Reported by: tonnzor Owned by: tobias
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: tonn81@…, benspaulding, rm_ Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If prepopulated field is marked as readonly then admin crashes:

Traceback:
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/core/handlers/base.py" in get_response
  100.                     response = callback(request, *callback_args, **callback_kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/contrib/admin/options.py" in wrapper
  239.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/utils/decorators.py" in _wrapped_view
  76.                     response = view_func(request, *args, **kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/views/decorators/cache.py" in _wrapped_view_func
  69.         response = view_func(request, *args, **kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/contrib/admin/sites.py" in inner
  190.             return view(request, *args, **kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/utils/decorators.py" in _wrapper
  21.             return decorator(bound_func)(*args, **kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/utils/decorators.py" in _wrapped_view
  76.                     response = view_func(request, *args, **kwargs)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/utils/decorators.py" in bound_func
  17.                 return func(self, *args2, **kwargs2)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/db/transaction.py" in _commit_on_success
  299.                     res = func(*args, **kw)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/contrib/admin/options.py" in change_view
  918.             model_admin=self)
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/contrib/admin/helpers.py" in __init__
  32.         } for field_name, dependencies in prepopulated_fields.items()]
File "/home/tonnzor/projects/twangoo/twangoo-hg/django/forms/forms.py" in __getitem__
  106.             raise KeyError('Key %r not found in Form' % name)

Exception Type: KeyError at /admin/twangoo/deal/3/
Exception Value: "Key 'slug' not found in Form"

Admin class:

class DealAdmin(TwangooAdmin):
    prepopulated_fields = {"slug": ("title",)}
    readonly_fields = ("status", "preview_link", "image_height", "image_width", "currency", "newsletter_id",
                       "closed_at", "short_url")    # must be tuple

    def get_readonly_fields(self, request, obj=None):
        if obj.status != 'draft':
            return ('slug',) + DealAdmin.readonly_fields
        return DealAdmin.readonly_fields

We need to disable editing slug if deal has already published (changing slug will break the link).

Attachments (3)

django.patch (614 bytes) - added by tonnzor 5 years ago.
Patch that fixed the crash
13618.diff (3.2 KB) - added by tobias 5 years ago.
slightly improved fix, with test
13618_updated.diff (3.5 KB) - added by tonnzor 4 years ago.
Fixed tobias patch to be applicable on trunk

Download all attachments as: .zip

Change History (20)

Changed 5 years ago by tonnzor

Patch that fixed the crash

comment:1 Changed 5 years ago by tonnzor

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

Fixed Admin class (crashed on new object creation):

class DealAdmin(TwangooAdmin):
    prepopulated_fields = {"slug": ("title",)}
    readonly_fields = ("status", "preview_link", "image_height", "image_width", "currency", "newsletter_id",
                       "closed_at", "short_url")    # must be tuple

    def get_readonly_fields(self, request, obj=None):
        if obj and obj.status != 'draft':
            return ('slug',) + DealAdmin.readonly_fields
        return DealAdmin.readonly_fields

comment:2 Changed 5 years ago by tonnzor

  • Cc tonn81@… added
  • milestone set to 1.3
  • Version changed from 1.2 to SVN

comment:3 Changed 5 years ago by tobias

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

Changed 5 years ago by tobias

slightly improved fix, with test

comment:4 Changed 5 years ago by tobias

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by leanmeandonothingmachine

Another idea could be to combine this patch with the one in #11639. And have the proposed get_prepopulated_fields function do that calculation that this one does in init.

comment:6 Changed 5 years ago by benspaulding

  • Cc benspaulding added

comment:7 Changed 4 years ago by rm_

  • Cc rm_ added

comment:8 Changed 4 years ago by tonnzor

Current patch seems to be ready for merging into repository. Is anybody going to do that? That's the last thing to do here.

comment:9 Changed 4 years ago by russellm

Nobody is going to commit anything unless someone marks the ticket Ready For Checkin. If you think tobias' patch is ready for trunk, change the ticket state and it will be put on our todo list.

Changed 4 years ago by tonnzor

Fixed tobias patch to be applicable on trunk

comment:10 Changed 4 years ago by tonnzor

  • Triage Stage changed from Accepted to Ready for checkin

The patch is ready for check-in. I had just checked that test fails on unmodified trunk and passes on fixed version

comment:11 follow-up: Changed 4 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

To my eyes, This isn't RFC.

There is definitely a problem here, but this patch only partially addresses it it. This patch removes the validation error, but leaves the admin in a state where it isn't actually doing something useful -- it allows you to define a readonly propopulated field, but then doesn't actually provide any way to populate the slug. As a result, saving an object results in empty/default slug values. This will only ever work for the first object; subsequent slugs won't be unique.

Either:

1) Raise a validation error, prohibiting the use of a prepopulated value in a readonly.
2) The prepopulated field needs to be handled as a hidden field -- not user-modifiable, but modifiable by javascript, and displayed to the user a a fait accompli.

Doing (2) well will be extremely difficult. It requires a big change to readonly_fields, introducing a 'read-only, but submitted as form value' type; it also provides no easy way to avoid validation problems when the slug isn't unique.

comment:12 in reply to: ↑ 11 Changed 4 years ago by anonymous

Replying to russellm:

1) Raise a validation error, prohibiting the use of a prepopulated value in a readonly.

That already raises Exception when prepopulated and readonly are used together on the same field.

I have a strong usecase that is really demanded and I hoped that these changes would cover it. If you have a better idea how to handle it - it would be great.

Usecase: There's an object that has a slug. However, that slug field that shouldn't be changeable after an object becomes published (at it would break all the links). For that purpose I use static prepopulated_fields definition and generate readonly_fields on-fly (for needed items) by get_readonly_fields. Of course if I may generate prepopulated_fields on-fly with some method (like def get_prepopulated_fields(self, request, obj=None)) - I would simply remove that slug field from it to not conflict with readonly_fields.

In that usecase that is excellent that user cannot change or define slug field. If user didn't define it before - that would raise validation errors on previous save. Yes, developer may implement get_readonly_fields incorrectly - like slug field is not editable on object creation - then user wouldn't be able to save that object (as empty slug will generate validation error too). We cannot guarantee that this feature is used properly, but developer will face that issue from the very beginning.

In my point of view, field representation should not influence to readonly_fields property - it should be not allowed for editing (shown as plain text) in any case. Then how field look/work like (e.g. being prepopulated) should not make difference. Readonly works for all the fields and should work for prepopulated field too.

comment:13 follow-up: Changed 4 years ago by v_peter

Would you consider #11639 then instead? That one will allow the developer to work out the best solution for avoiding the error based on their exact needs.

comment:14 in reply to: ↑ 13 Changed 4 years ago by tonnzor

Replying to v_peter:

Would you consider #11639 then instead? That one will allow the developer to work out the best solution for avoiding the error based on their exact needs.

That patch is good for avoiding conflict of prepopulated and readonly and seems a fine solution. Having this conflict resolved (or making prepopulated implementation bullet-proof) would be great.

comment:15 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:16 Changed 4 years ago by jezdez

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

In [16069]:

Fixed #11639, #13618 -- Added get_prepopulated_fields method to ModelAdmin and InlineModelAdmin to be able to handle prepopulated fields on a case-by-case basis. Thanks, leanmeandonothingmachine.

comment:16 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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