Opened 14 years ago

Closed 13 years ago

Last modified 12 years ago

#13618 closed Bug (fixed)

prepopulated_fields crashes with get_readonly_fields

Reported by: Artem Skoretskiy Owned by: Tobias McNulty
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: tonn81@…, Ben Spaulding, rm_ Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

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 Artem Skoretskiy 14 years ago.
Patch that fixed the crash
13618.diff (3.2 KB ) - added by Tobias McNulty 14 years ago.
slightly improved fix, with test
13618_updated.diff (3.5 KB ) - added by Artem Skoretskiy 13 years ago.
Fixed tobias patch to be applicable on trunk

Download all attachments as: .zip

Change History (20)

by Artem Skoretskiy, 14 years ago

Attachment: django.patch added

Patch that fixed the crash

comment:1 by Artem Skoretskiy, 14 years ago

Has patch: set

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 by Artem Skoretskiy, 14 years ago

Cc: tonn81@… added
milestone: 1.3
Version: 1.2SVN

comment:3 by Tobias McNulty, 14 years ago

Owner: changed from nobody to Tobias McNulty
Status: newassigned

by Tobias McNulty, 14 years ago

Attachment: 13618.diff added

slightly improved fix, with test

comment:4 by Tobias McNulty, 14 years ago

Triage Stage: UnreviewedAccepted

comment:5 by leanmeandonothingmachine, 13 years ago

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 by Ben Spaulding, 13 years ago

Cc: Ben Spaulding added

comment:7 by rm_, 13 years ago

Cc: rm_ added

comment:8 by Artem Skoretskiy, 13 years ago

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 by Russell Keith-Magee, 13 years ago

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.

by Artem Skoretskiy, 13 years ago

Attachment: 13618_updated.diff added

Fixed tobias patch to be applicable on trunk

comment:10 by Artem Skoretskiy, 13 years ago

Triage Stage: AcceptedReady 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 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

in reply to:  11 comment:12 by anonymous, 13 years ago

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 by v_peter, 13 years ago

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.

in reply to:  13 comment:14 by Artem Skoretskiy, 13 years ago

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

Severity: Normal
Type: Bug

comment:16 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: assignedclosed

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 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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