fix: CMS v4: Moving plugins between placeholders, plugin api

[fsbraun]

Description

This PR removes some bugs from handling the plugin tree within a placeholder:

• Moving plugins between placeholder now works reliably (fixing [BUG] CMS v4: Moving plugins between placeholders broken. #7392). The position moving logic required for CTE to not fail because of violated uniqueness constraints [test added]
• adding a plugin as first child now works if the plugin tree contains more than 3 plugins already [test added]
• fix .get_last_plugin_position to give the position of the last child or grand-child. This caused the cms.api.add_plugin function to add a plugin before grand children of the last child [test added]
• fix .get_last_plugin_position to always give the largest position number and not the one retrieved from the database in last position (though they apparently regularly coincide) by adding .order_py('position')
• adding a .check_tree and .fix_tree method to the Placeholder model to at least be able to fix broken trees ([BUG] CMS v4: Moving plugins between placeholders broken. #7392 )

Related resources

It has been observed (though exact circumstances are unclear) that when the plugin tree in the js frontend and in the database get out of sync (e.g., by a bug like #7392) further editing commands can lead to unexpected results to the plugin tree, potentially even a plugin tree spanning placeholders (e.g., the clipboard).

To this end the PR includes a .check_tree and .fix_tree methods which will check and fix 3 plugin tree properties

1. Plugin positions do not include gaps (should have been removed by ._recalculate_plugin_positions)
2. Child plugins always have a higher position than their parent plugins
3. Plugins do not refer to parent plugins which are assigned to a different placeholder (attn: The fix breaks links to other placeholders by moving the plugin to the top-level (parent = None)

Checklist

• I have opened this pull request against develop-4
• I have added or modified the tests when changing logic
• I have followed the conventional commits guidelines to add meaningful information into the changelog
• I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

[Aiky30]

I'm genuinely concerned by the amount of changes to the plugin tree here.

Do we have a scenario as a unit test that currently fails without any changes?

I was under the impression that the issue was with shifting the plugins. Playing with the plugin tree is extremely risky.

My attention would have been here:

django-cms/cms/models/placeholdermodel.py

Line 611 in 28d3fa5

self._shift_plugin_positions(

[Aiky30]

I fear that this will just end up like django-cms v3 where production container spin up always runs this command. It's not very efficient and people with versioning will want this to work very differently.

[fsbraun]

So far this is only a method for the Placeholder model. Not a management command. If added as a management command it would need to be only invoked on placeholders in draft models. If we fix the plugin trees we might never need it as a management command. Right now, @marksweb at least will need the fix for his site to work.

I suggest to add a management command for the develop-4. branch. We can decide to remove it before publishing v4. Clearly it should not be needed in a published version.

[Aiky30]

The design principle of the core is to be as simple as possible, temporary logic should be a 3rd party package, not part of the core. It also allows changes to be optional and provides a much faster turn around.

I agree Mark needs a fast solution, this can be a management command in the djangocms-frontend package because it is this package that is seeing these issues. The only time I saw the same issue is with the text editor logic I keep referring to. It had exactly the same side affects and it was because the cms wasn't used to get the position. Once the text editor got the position properly no position issues were seen again.

        last_position = placeholder.get_last_plugin_position(language) or 0
        # A shift is only needed if the distance between the new plugin
        # and the last plugin is greater than 1 position.
        needs_shift = (position - last_position) < 1

        if needs_shift:
            # shift to the right to make space for the new ghost plugin
            placeholder._shift_plugin_positions(
                language,
                start=position,
                offset=last_position,
            )

[Aiky30]

The fix tre command you are creating probably only needs to be this method:

placeholder._shift_plugin_positions(
    language,
    start=position,
    offset=last_position,
)

[Aiky30]

@fsbraun A versioning example, versioning code can never go in core.

from djangocms_versioning.helpers import remove_published_where


# Get all draft PageContents (Ensure that we don't change any previously published pages, allows us to use compare
page_contents = PageContents.objects.all()
page_contents = remove_published_where(page_contents)
page_contents = page_contents.filter(versions__state="DRAFT")

# may be able to do: page_contents = remove_published_where(PageContents.objects.filter(versions__state="DRAFT"))

for page_content in page_contents:
    for placeholder in page_content.placeholders:
         placeholder._shift_plugin_positions(
            page_content.language,
            start=1
        )

[Aiky30]

You can use version compare manually to see if it has reordered anything unintentionally. Compare is useless if all versions have been manipulated.

[fsbraun]

For me it's also fine to have a fix-tree management command in a separate mini app or even script. Thanks for the code snippet to get the draft pages. I was thinking of let it run over all placeholders that are assigned to a ContentType object with "draft" status in versioning and, e.g., the clipboard.

[marksweb]

@Aiky30 Yeah need to give it a run. That earlier version that just went over all placeholders did resolve it, but obviously wasn't the correct approach so need to run this now.

[marksweb]

@Aiky30 I've tried to run your example and it didn't change what's happening.

[Aiky30]

Sorry guys, I saw what u thought was this just being merged without any approvals hence why I threw my toys out of the pram.

I do have genuine concerns at the scale if the changes here.

If you're blocked then I guess you have to do something.

@fsbraun The tests will need to create multiple trees in different languages.

Does there need to be the amount if changes made to the tree or can this be made less intrusive?

@marksweb it's a shame that the script I sent didn't work, I imagine it could with some tweaking but I'm away all weekend so not going to be much help on getting something fixed fast.

I would still lean towards making sure any fixing of previous trees is handled outside of the cms, this is very hacky and I'm pretty sure will crash on large projects.

[Aiky30]

@fsbraun Now you understand how the plugins are being ordered can you now see that the way the position is being created here in the front end could be creating them incorrectly? The cms always goes and gets the highest number: https://github.com/django-cms/djangocms-frontend/blob/f5691769b7d1d5c3e2a0cdd9d7162bd3f9bdbe41/djangocms_frontend/contrib/grid/cms_plugins.py#L125

You're making the core cms fit djangocms-frontend rather than make djangocms-frontend fit the cms.

This could have unforeseen side affects that I would not want to be responsible for in production projects.

I would advise having any fixing of trees done in a 3rd party package until it is seen to be stable, the same for any plugin tree changes. Just bringing them in could have devastating consequences, especially by modifying all placeholders in the system. I feel so strongly about this this change, that if it is just forced in then I will need to consider how much value I can add going forwards.

[fsbraun]

@Aiky30 I understand your concerns about changes to the plugin tree at the very heart of v4. Obviously, you have by far the most massive experience with v4.

Nevertheless, all three new tests

• test_inter_placeholder_nested_plugin_move

  django-cms/cms/tests/test_placeholder.py

  Line 196 in 94c929e

  def test_inter_placeholder_nested_plugin_move(self):

• test_get_last_plugin_position

  django-cms/cms/tests/test_placeholder.py

  Line 230 in 94c929e

  def test_get_last_plugin_position(self):

• test_get_last_plugin_position_order

  django-cms/cms/tests/test_placeholder.py

  Line 270 in 94c929e

  def test_get_last_plugin_position_order(self):

will fail/throw an exception without this PR. To me these address genuine bugs. Don't kill the messenger.

This PR proposes no changes in the way the plugin tree works. It's just that you actually can now move plugins across placeholders by creating enough temporary space between plugin positions so that they can be moved to where they need to go. Currently, cms v4 in some cases only creates insufficient temporary space and moving the plugins to the new placeholder make them collide with an existing plugin there.

These bugs have nothing to do with djangocms-frontend and exist w/o it being installed. Here's a djangocms-frontend "free" example of #7392 - try it on any cms v4 installation and move any two+ plugins in front of a single plugin in a different placeholder:

Just to be clear, I do not want to force anything on anybody. I just want to bring v4 forward. This is why I actively asked you to be involved and review in this PR. I hope you understand.

[Aiky30]

I do intend to pull this PR / branch down and check it over. I will at the very least have a lot of time this week to go over this, I'll also see Mark this week too so should have enough background to be of use with this change.

[Aiky30]

The logic to moving the plugins looks sane.

One comment on the add plugin offset that I don't understand.

Asked for some documentation on the tests for the plugin tree to help anyone in the future,

Is this change to the djangocms-text-ckeditor (https://github.com/django-cms/djangocms-text-ckeditor/pull/629/files) a side affect of the changes being made in this PR?

[Aiky30]

@fsbraun I've found it took a lot of my time to read the tests and understand what they are doing, can you please add more detail / comments to allow anyone in the future who breaks this to understand what they broke. This is the same for all of the tests created for this change. Put yourself in the position of someone who broke the tests.

I wouldn't expect this level of detail but this is a really good example of how it can be done: https://github.com/django-cms/django-cms/blob/develop-4/cms/tests/test_menu.py#L1685

[Aiky30]

Wondering if these changes for & are actually a separate bug / PR?

[fsbraun]

Right. This is not connected to the rest. It just came to mind that Django since 2.2.19 does not support semicolons as separators in urls any more (https://docs.djangoproject.com/en/4.1/releases/2.2.19/) Hence it does not understand & as a separator. However, this seems not critical for the test. It passes w/o change.

[Aiky30]

I can't see the need for this logic, because the "last" plugin should be the end, if we are adding a plugin we need to shift the tree behind where we are inserting all of the way to the end.

[fsbraun]

Like for the move between placeholders you have to both consider to be able to move out the tail (as you assume) but then also be able to move the tail back in after the new plugin has been added. For this you need one more spare position, since the gap has been reduced by the newly created plugin.

Moving the tail to the end amounts to an offset of last_position - instance.position + 1, then add 1 for the new plugin.

Consider your want to insert Plugin 0 at position 1 (positions in the first column):

1 Plugin 1
2 Plugin 2

First you move the tail out and put the new plugin into the database. Take as offset last_position(2) - instance.position(1) + 2 = 3:

1 Plugin 0
4 Plugin 1 
5 Plugin 2

Now you have the minimum position slots between 1 and 4 to move the tail back in (position 2 and up).

If you had just moved by last_position(2) the gap would not have been big enough to move back the tail.

[fsbraun]

The logic to moving the plugins looks sane.

Yes, it's good to have some sane logic there.

One comment on the add plugin offset that I don't understand.
Asked for some documentation on the tests for the plugin tree to help anyone in the future,

I've added an elaborate explanation why the add plugin offset has to be what it is. The general issue for the offsets in both cases seems that not only they have to consider moving the remaining part (tail) out far enough to create space for new plugins and not overlap with the original plugin positions but also far enough out to after adding the plugins close the remaining gaps and move the tail back in.

I invested in some comments for the tests. I guess the new tests are above par with respect to comments and reflecting languages now. 😉

Is this change to the djangocms-text-ckeditor (https://github.com/django-cms/djangocms-text-ckeditor/pull/629/files) a side affect of the changes being made in this PR?

At this point I have identified two totally independent ways of getting the plugin tree into a inconsistent state:

1. By moving a big number of plugins from one placeholder to another with a small number of plugins (this PR)
2. By generating a gap in the position field and moving plugins after the gap. Only one placeholder needed for this and this might be @marksweb case in [BUG] CMS v4: Inconsistent behaviour when re-ordering plugins #7391. The gap in the position field originates from djangocms-text-ckeditor. Hence this PR is not a side effect of the changes here.