Skip to content

Commit

Permalink
Fix garbled placeholder data in admin forms submitted with errors
Browse files Browse the repository at this point in the history
When a fluent contents or page admin change form is submitted with
validation errors, the form subsequently returned to the user for
correction can get garbled placeholder data causing the wrong
content to be shown in placeholder tabs, and probably other strange
side-effects.

This issue only applies to the form rendered as the result of a failed
submission, not to the initial form rendered when an admin change form
is first loaded.

The issue is caused by use of the `original` attribute of a formset's
`InlineAdminForm` form instance when rendering placeholder data inlines
in the admin. The `original` attribute for each form is assumed to have
the `Placeholder` instance related to the corresponding form, but there
is actually no guarantee this will be the case. On a form with multiple
placeholder inline tabs that is submitted with validation errors, there
may be no correspondence between the order of the forms' instances and
the value of the `original` forms' attribute.

The fix for the issue is to use the "inner" form's `instance` attribute
to get the related `Placeholder` instance, instead of relying on
`InlineAdminFormSet.original`. The instance attribute should be
guaranteed to always have the correct related placeholder.

To see why the `InlineAdminFormSet.original` attribute is unreliable,
note that Django's base `InlineAdminFormSet.__iter__` does this:

    for form, original in zip(
            self.formset.initial_forms, self.formset.get_queryset()):
        view_on_site_url = self.opts.get_view_on_site_url(original)
        yield InlineAdminForm(self.formset, form, self.fieldsets,
            self.prepopulated_fields, original, self.readonly_fields,
            model_admin=self.opts, view_on_site_url=view_on_site_url)

The assumption for the `zip` operation is that the set of
`initial_forms` will always be ordered in the same way as the queryset
from `get_queryset()`, or that this ordering is not very important.

However, for the `Placeholder` admin inlines this is not safe because
although the ordering matches on initial page loads with a GET request
-- due I believe to special handling in `PlaceholderEditorInline`s
`get_formset`? -- it will not necessarily match when the form is
submitted with an error and the ordering of `initial_forms` is set by
the display order in the admin page, which may not bear any
relationship to the ordering used by `self.formset.get_queryset()`.
  • Loading branch information
jmurty committed Mar 6, 2017
1 parent 03da447 commit c749e9c
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

inline_admin_formset = generated InlineAdminFormSet of the PlaceholderEditorInline. Each form holds a placeholder.
inline_admin_formset.opts = PlaceholderEditorInline object
inline_admin_formset.original = Placeholder object
inline_admin_formset.form.instance = Placeholder object
inline_admin_formset.form.'field'.value = value!

inline_admin_formset.__iter__ gives all forms + a template form
Expand All @@ -15,7 +15,7 @@
<script type="text/javascript">
// Pass database data to the cp_admin.js file.
// Each tab hosts a placeholder pane.
var placeholder_metadata = {% block js_placeholders %}[ {% for inline_admin_form in inline_admin_formset %}{% with form=inline_admin_form.form placeholder=inline_admin_form.original %}{% if not forloop.last %}{% if not forloop.first %}, {% endif %}{
var placeholder_metadata = {% block js_placeholders %}[ {% for inline_admin_form in inline_admin_formset %}{% with form=inline_admin_form.form placeholder=inline_admin_form.form.instance %}{% if not forloop.last %}{% if not forloop.first %}, {% endif %}{
id: {% if placeholder and placeholder.id %}{{ placeholder.id }}{% else %}''{% endif %},{# if test added to support TEMPLATE_STRING_IF_INVALID #}
slot: '{{ form.slot.value|default:forloop.counter0|escapejs }}',
title: '{{ form.title.value|default:"?"|escapejs }}',
Expand Down Expand Up @@ -69,8 +69,8 @@
<input type="checkbox" class="cp-placeholder-delete" name="{{ placeholder_form.DELETE.name }}" checked="checked" />
<input type="hidden" class="cp-placeholder-delete" name="{{ placeholder_form.id.name }}" value="{{ placeholder_form.id.value|default_if_none:'' }}" />
{% else %}
{% getfirstof inline_admin_form.original.get_allowed_plugins inline_admin_formset.opts.get_all_allowed_plugins as cp_plugin_list %}
{% getfirstof inline_admin_form.original.id '' as placeholder_id %}{# used by pane.html / controls.html #}
{% getfirstof inline_admin_form.form.instance.get_allowed_plugins inline_admin_formset.opts.get_all_allowed_plugins as cp_plugin_list %}
{% getfirstof inline_admin_form.form.instance.id '' as placeholder_id %}{# used by pane.html / controls.html #}
{% getfirstof placeholder_form.slot.value '__EMPTY__' as placeholder_slot %}
<div id="{{ inline_admin_formset.formset.prefix }}-{% if forloop.last %}empty{% else %}{{ forloop.counter0 }}{% endif %}" data-tab-region="{{ placeholder_slot|slugify }}" class="inline-placeholder cp-tab{% if forloop.last %} empty-form{% else %} cp-region-tab{% endif %}{% if forloop.first %} cp-tab-active{% endif %}">

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
<script type="text/javascript">
// Pass database data to the cp_admin.js file.
// Each tab hosts a placeholder pane.
var placeholder_metadata = {% block js_placeholders %}[ {% for inline_admin_form in inline_admin_formset %}{% with form=inline_admin_form.form placeholder=inline_admin_form.original %}{% if not forloop.last %}{% if not forloop.first %}, {% endif %}{
var placeholder_metadata = {% block js_placeholders %}[ {% for inline_admin_form in inline_admin_formset %}{% with form=inline_admin_form.form placeholder=inline_admin_form.form.instance %}{% if not forloop.last %}{% if not forloop.first %}, {% endif %}{
id: {% if placeholder and placeholder.id %}{{ placeholder.id }}{% else %}''{% endif %},{# if test added to support TEMPLATE_STRING_IF_INVALID #}
slot: '{{ form.slot.value|default:forloop.counter0|escapejs }}',
title: '{{ form.title.value|default:"?"|escapejs }}',
Expand Down

0 comments on commit c749e9c

Please sign in to comment.