prepopulated_fields crashes with get_readonly_fields

[Artem Skoretskiy]

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).

[Artem Skoretskiy]

Patch that fixed the crash

[Artem Skoretskiy]

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

[Tobias McNulty]

slightly improved fix, with test

[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.

[Artem Skoretskiy]

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

[Russell Keith-Magee]

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.

[Artem Skoretskiy]

Fixed tobias patch to be applicable on trunk

[Artem Skoretskiy]

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

[Russell Keith-Magee]

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.

[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.

[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.

[Artem Skoretskiy]

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.

[Jannis Leidel]

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.

[Jacob]

Milestone 1.3 deleted