WIP: Handle placeholders being added and removed from templates when …

[mrmachine]

…saving existing objects.

• Don't assume the queryset order matches the formset initial data order, which
  can change when placeholders are moved in a template. Match initial data to
  placeholder instance by slot name instead of index.
• If a placeholder exists in the database but no longer exists in the template,
  and therefore has no initial data, get the slot name from orphaned
  placeholders by index. This is a best guess and might be wrong?
• Add a sanity check (raises KeyError). We should always be able to find
  initial data for a new placeholder. We can probably remove this check?

[mrmachine]

This still doesn't quite work, but I found some more issues while implementing your suggested changes (loop to cache queryset, remove .save()).

Another edge case I discovered is when placeholders exist in the database and are removed from the template. I've tried to handle that with a __queryset_minus_initial() method, which falls back to guessing which placeholder to get by index from the list of orphaned placeholders.

Unfortunately, I think the original problem ValidationError: '' value must be an integer." is still present if we don't call .save() as you suggested. In fact even when we do call save, it seems to only work after I submit, refresh, and submit again.

The core problem is that when editing an existing object, some placeholders will exist in the database and some will not. When the formset is rendered for placeholders that are not in the database, they have no pk value set, and then when the form is saved, we get that validation error.

I don't much time to dig much deeper right now, but if you can further point me in the right direction I can keep trying.

If we're not going to call .save() in __get_form_instance(), where will the placeholder be created?

Is it OK to get orphaned placeholders by index, like I am doing here?

[vdboor]

Just to answer a few of your questions quickly:

• the placeholders are saved through the standard Django formset/inline admin mechanism. That is PlaceholderEditorInline and PlaceholderInlineFormSet.
• FYI, in BaseContentItemFormSet, the save_new() and save_existing() are overwritten to deal with placeholders too. Those calls are initiated by the standard BaseModelFormSet.save().
• When you change the layout in the client-side (e.g. using the layout box), the client-side will detect which placeholders will become orphaned, and mark those for deletion. It adds a placeholder-fs-#-DELETE field so the object is removed on save. That is picked up server-side with standard Django code, hence you didn't find that in django-fluent-contents.
• Right now, layout changes are completely handled client-side. Hence, switching layouts twice (or saving again) fixes the placeholders because they get marked for deletion in the client-side code.
• The standard Django delete mechanism works as follows: The Django formset code calls BaseModelFormSet.save() which calls BaseModelFormSet.save_existing_objects(). It loops through BaseFormSet.deleted_forms. You may want to override either of these to remove orphaned objects.

In standard Django, get_queryset() shows what's currently there, and that is updated on save(). I'm OK with hacking get_queryset() to return only what we want to show - meaning we should improve the save() to correct the orphaned objects.

When writing this code, I've uses step-through debugging in PyCharm to get this code working, because the inner workings of the formset/inlineadmin code is complex. By walking through the formset code - even through django.forms.models - this allowed implementing all the save logic without having to resort to completely rewriting ModelAdmin.change_view() like others do, since that is horrible to begin with.

[mrmachine]

Is changing the layout client side the same as changing the template used by a layout? If not, should it be? I haven't tested for this problem when switching layouts, only when altering the actual template that defines placeholders for a given layout.

If this "marked for deletion" thing happens via client side JS when layout is switched, I guess it won't happen when a template that defines layouts is altered and the page is reloaded. This is causing us problems both when new placeholders are added, and existing placeholders are removed.

So, how and where would I attempt to imitate this "on layout switch" behaviour on every page load, when the number of placeholders and slot name for placeholders defined in the template no longer match existing placeholders in the database?

[vdboor]

Good points.

The server-side support is indeed lacking, and the client-side hid that mostly because layout switching was the only option at first. I would vote for getting server-support proper too.

When opening a page, I'd expect all placeholders to look good, and in the right order. That would imply that get_queryset() or the initial forms return what the situation should actually be.

When saving, the situation should be saved, and any old orphaned placeholders can be cleaned up.

This seems like to cleanest route to me. You never expect a save using a GET call, only when the user actually saves something.

[mrmachine]

Thanks. I'll take another look at updating get_queryset() as you suggest.

One potential issue though is that the class is named BaseInitialGenericInlineFormSet and it's in fluent_contents.admin.genericextensions. But it already contains implementation details that are specific to placeholders, which appear to be the only subclass. And I imagine with these changes, it will only become more specific to placeholders.

1. Is it correct that this class is only used by placeholders?
2. Is it OK for this code to become even more coupled to placeholders?

[vdboor]

Yes, I'm fine with merging that class with the PlaceholderInlineFormSet. It doesn't make sense to keep these two separate anymore. It was done as a separation of concerns, but it would only add to code bloat now. Merging is fine! :)