From c749e9c0184702d318abbfd3e4f3557cf251bc12 Mon Sep 17 00:00:00 2001
From: James Murty <james@murty.co>
Date: Mon, 6 Mar 2017 14:32:11 +1100
Subject: [PATCH] Fix garbled placeholder data in admin forms submitted with
 errors

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()`.
---
 .../admin/fluent_contents/placeholder/inline_tabs.html    | 8 ++++----
 .../fluent_contents/placeholderfield/inline_init.html     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fluent_contents/templates/admin/fluent_contents/placeholder/inline_tabs.html b/fluent_contents/templates/admin/fluent_contents/placeholder/inline_tabs.html
index 8a986775..f3085384 100644
--- a/fluent_contents/templates/admin/fluent_contents/placeholder/inline_tabs.html
+++ b/fluent_contents/templates/admin/fluent_contents/placeholder/inline_tabs.html
@@ -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
@@ -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 }}',
@@ -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 %}">
 
diff --git a/fluent_contents/templates/admin/fluent_contents/placeholderfield/inline_init.html b/fluent_contents/templates/admin/fluent_contents/placeholderfield/inline_init.html
index 6aaec0ee..de3df8ca 100644
--- a/fluent_contents/templates/admin/fluent_contents/placeholderfield/inline_init.html
+++ b/fluent_contents/templates/admin/fluent_contents/placeholderfield/inline_init.html
@@ -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 }}',