fix #1527, more tests

[evildmp]

I'd appreciate a review before this is merged.

Fixes #1527 - when plugin trees were copied, their new structure was not always an accurate copy. In most cases this didn't actually have any effects, which is why this escaped so long.

It's likely relevant to #1463 also; one failure I had there was with copied plugin tree problems.

The problem was that in a copy, a plugin could be given the wrong parent - it would become the child of its sibling instead.

New test, test_plugin_deep_nesting_and_copying

Creates a structure that wouldn't be copied accurately before, and tests it thoroughly. It's a long test, but I can't see how to get around that, especially as it tests properties of the tree incrementally as more nodes are added, until finally it's copied.

CMSPlugin.copy_plugin()

This is where the heart of the problem was. The routine maintains a kind of breadcrumb trail though the plugin tree to work out a new plugin's parent, but would sometimes lose its way.

Some variables have been renamed to make it easier to understand their purpose; also lots of comments because I think the logic is not easy to follow unless you already know it.

Should be of interest to @adaptivelogic.

[digi604]

The test failed?

[evildmp]

That's odd, all the tests passed locally.

[evildmp]

I don't understand https://travis-ci.org/divio/django-cms/builds/3339563. The tests pass sometimes. What is the difference between all those jobs, and how can I do the same locally to test before I commit?

[evildmp]

OK, I see, it fails in Python 2.5 and 2.6, but is OK in Python 2.7. I will check that out.

[evildmp]

Tests pass now.

The issue was: in earlier Pythons (pre 2.7) there was a different implementation of assertItemsEqual, which would take a list like:

[
    CMSPlugin.objects.get(id=2),
    CMSPlugin.objects.get(id=4),
    CMSPlugin.objects.get(id=3),
]

and re-order it arbitrarily (in fact pretty much at random) because when it got hold of a model, it was like a monkey with a typewriter and really had no idea what to do with it.

Many thanks to @apollo13 and #django.

[digi604]

I am not sure about this line.... i think an ordering is needed here. If you want the same order as you inserted it in the test try: order_by('tree_id', 'lft')

[evildmp]

But according to the tests, this does get the ordering right, no?

[digi604]

No because the test build the tree in a optimal way... from the ground up.

[evildmp]

The test deliberately builds the tree in the 'wrong' order, to simulate things being added over time, not optimally - the nodes go: 1 2 4 10 8 3 9 5 6 7.

But perhaps it should be made even more difficult than that, to be a proper test.

[evildmp]

OK, all my attempts to make this break fail - it seems OK to me.

However, I would like to test moving plugins around as well, but as far as I can tell the CMS doesn't use CMSPlugin.move_to() at all, and I haven't been able to use it successfully to move a plugin. How does the CMS move plugins, if not using that? Surely it has to at some level, if it's using the MPTT base class for them?

[evildmp]

In fact, if the CMS never moves a plugin around in its tree, why should any of the tests? In which case, I don't see a need to make these tests do that.

[evildmp]

OK, apologies for the excess of commits.

What this latest patch (912803f is the important commit) does is:

• replace instances of placeholder.cmsplugin_set.all().order_by('tree_id', '-rght') and inconsistent variations with placeholder.get_plugins(), which already existed and can do this for us in a consistent way. For lists there's placeholder.get_plugins_list()
• changed the ordering in get_plugins() to ('tree_id', 'lft'); ('tree_id', '-rght') failed tests
• simplified the logic in lines 257-272 in https://github.com/evildmp/django-cms/blob/912803fa9e991781007cd02ac7cb2f3f445d02a3/cms/models/pluginmodel.py#L257 and fixed a bug exposed in the new tests
• new tests, to create a very out-of-sequence tree, then move bits of it all around

[ojii]

tests pass with

Python 2.5 and Django 1.3
Python 2.5 and Django 1.4
Python 2.6 and Django 1.3
Python 2.6 and Django 1.4
Python 2.7 and Django 1.3
Python 2.7 and Django 1.4

on my local machine

[digi604]

merge?

[ojii]

reviewing the actual code now

[ojii]

as noted on IRC: the current test tests the full page copy mechanism, not just CMSPlugin.copy_plugin, if possible page creation/copying should be avoided to make this test faster and more concise.

[evildmp]

Having just tried to avoid the full page create/copy routine, I remember why I didn't do it that way in the first place - I can't get it to work.

original_plugins = original_placeholder.get_plugins()
copied_placeholder = Placeholder()
copy_plugins_to(original_placeholder.get_plugins(), copied_placeholder)
copied_plugins = copied_placeholder.get_plugins()

What happens is that copied_placeholder.get_plugins() seems to return the original set of plugins, not new ones - unless the copied_placeholder belongs to a page.

[ojii]

that sounds like a huge problem that needs to be fixed first. placeholders need to work if not bound to pages.

[evildmp]

Now deep-nested plugin copy tests don't bother messing about with pages, and just work on placeholders. Simpler, and we save 0.8s in the tests.