Removed an incorrect blocking of plugin cleanup on cancellation (#537)

* Removed an incorrect blocking of plugin cleanup on cancellation

* Rebundle the js to be able to test the new changes.

* Woudl help if i added the bundlegit status

* Added a possibel solution to ensuring that a ghost plugin can never cause corruption issues for a page

* Handle the error when a bound plugin cannot be found

* Add a better comment to describe the new feature amd a better translatable error message

* Added tests to test all boundaries

* Added a note to check how the plugin position is generated and to think about cleaning ghosts

* Merged latest changes from upstream support/4 and cleaned new flake 8 and isort failures

* Added better comments

* Added test to highlight ghosting issue

* Remove any orphans hat hav ebeen left behind and reork logic to handle any in the future

* Clean up the errors in cms_plugins

* Bulk out tests to cover all eventualities, currently broken by the fact that we are catching an IntegrityError error

* clean out typos found in PR

* fix flake 8 errors in the tests

* Cleaned out test logic that incorrectly tries to add a new plugin when it should reuse the ghost plugins position

* Fixed the test on the transaction Index error

* Added additional method to handle the creation of the ghost to clean the add_view method

* Fix flke8 error

* Fix incorrect write up for the test method get_plugin_id_from_response

* Add additional edge checks to the tests and fix typos, and add additional information

* Renamed the clean ghosts method

djangocms_text_ckeditor/cms_plugins.py

@@ -6,8 +6,10 @@
 from django.conf.urls import url
 from django.contrib.admin.utils import unquote
 from django.core import signing
-from django.core.exceptions import PermissionDenied, ValidationError
-from django.db import transaction
+from django.core.exceptions import (
+    ObjectDoesNotExist, PermissionDenied, ValidationError,
+)
+from django.db import IntegrityError, transaction
 from django.forms.fields import CharField
 from django.http import (
     Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden,
@@ -291,6 +293,62 @@ def __init__(self, *args, **kwargs):
                 super(TextPluginForm, self).__init__(*args, initial=initial, **kwargs)
         return TextPluginForm
 
+    def _clean_orphaned_ghost_plugins(self, language, placeholder, plugin_type="TextPlugin"):
+        """
+        If any "ghost" plugins are left behind by a failed cancellation
+        the creation of a new plugin can be blocked by the fact that a new plugin is
+        trying to use the same position, to fix this we can clean any existing orphans
+        and recalculate the placeholder plugins positions
+        """
+        has_orphans = False
+        placeholder_plugins = CMSPlugin.objects.filter(
+            language=language,
+            plugin_type=plugin_type,
+            placeholder=placeholder,
+        )
+
+        for plugin in placeholder_plugins:
+            try:
+                plugin.get_bound_plugin()
+            except ObjectDoesNotExist:
+                has_orphans = True
+                plugin.delete()
+
+        if has_orphans:
+            # The positions are compromised, recalculate them
+            placeholder._recalculate_plugin_positions(language=language)
+
+    def _create_ghost_plugin(self, data):
+        """
+        Try and create a "ghost" plugin to be able to attach child plugins.
+        In the even that orphaned ghost plugins exist they must be cleaned!
+        """
+        try:
+            with transaction.atomic():
+                plugin = CMSPlugin.objects.create(
+                    language=data['plugin_language'],
+                    plugin_type=data['plugin_type'],
+                    position=data['position'],
+                    placeholder=data['placeholder_id'],
+                    parent=data.get('plugin_parent'),
+                )
+        except IntegrityError:
+            with transaction.atomic():
+                # Failed deletion of a ghost plugin in the placeholder
+                # could mean the position that we are trying to use is incorrect
+                # because ghost plugins may still exist, try and clean them
+                self._clean_orphaned_ghost_plugins(data['plugin_language'], data['placeholder_id'])
+                # We can now try adding the plugin again, if this fails then something else is wrong
+                # and the failure should throw the Integrity error, or any other error now occurring
+                plugin = CMSPlugin.objects.create(
+                    language=data['plugin_language'],
+                    plugin_type=data['plugin_type'],
+                    position=data['position'],
+                    placeholder=data['placeholder_id'],
+                    parent=data.get('plugin_parent'),
+                )
+        return plugin
+
     @xframe_options_sameorigin
     def add_view(self, request, form_url='', extra_context=None):
         if 'plugin' in request.GET:
@@ -339,13 +397,7 @@ def add_view(self, request, form_url='', extra_context=None):
         # Sadly we have to create the CMSPlugin record on add GET request
         # because we need this record in order to allow the user to add
         # child plugins to the text (image, link, etc..)
-        plugin = CMSPlugin.objects.create(
-            language=data['plugin_language'],
-            plugin_type=data['plugin_type'],
-            position=data['position'],
-            placeholder=data['placeholder_id'],
-            parent=data.get('plugin_parent'),
-        )
+        plugin = self._create_ghost_plugin(data)
 
         query = request.GET.copy()
         query['plugin'] = six.text_type(plugin.pk)

djangocms_text_ckeditor/static/djangocms_text_ckeditor/ckeditor_plugins/cmsplugins/plugin.js

@@ -404,36 +404,34 @@
                 if (!that.options.delete_on_cancel && !that.unsaved_child_plugins.length) {
                     return;
                 }
-                if (that.unsaved_child_plugins.length) {
-                    e.preventDefault();
-                    CMS.API.Toolbar.showLoader();
-                    var data = {
-                        token: that.options.action_token
-                    };
-
-                    if (!that.options.delete_on_cancel) {
-                        data.child_plugins = that.unsaved_child_plugins;
-                    }
+                e.preventDefault();
+                CMS.API.Toolbar.showLoader();
+                var data = {
+                    token: that.options.action_token
+                };
 
-                    $.ajax({
-                        method: 'POST',
-                        url: that.options.cancel_plugin_url,
-                        data: data,
-                        // use 'child_plugins' instead of default 'child_plugins[]'
-                        traditional: true
-                    }).done(function () {
-                        CMS.API.Helpers.removeEventListener(
-                            'modal-close.text-plugin.text-plugin-' + that.options.plugin_id
-                        );
-                        opts.instance.close();
-                    }).fail(function (res) {
-                        CMS.API.Messages.open({
-                            message: res.responseText + ' | ' + res.status + ' ' + res.statusText,
-                            delay: 0,
-                            error: true
-                        });
-                    });
+                if (!that.options.delete_on_cancel) {
+                    data.child_plugins = that.unsaved_child_plugins;
                 }
+
+                $.ajax({
+                    method: 'POST',
+                    url: that.options.cancel_plugin_url,
+                    data: data,
+                    // use 'child_plugins' instead of default 'child_plugins[]'
+                    traditional: true
+                }).done(function () {
+                    CMS.API.Helpers.removeEventListener(
+                        'modal-close.text-plugin.text-plugin-' + that.options.plugin_id
+                    );
+                    opts.instance.close();
+                }).fail(function (res) {
+                    CMS.API.Messages.open({
+                        message: res.responseText + ' | ' + res.status + ' ' + res.statusText,
+                        delay: 0,
+                        error: true
+                    });
+                });
             };
 
             CMS.API.Helpers.addEventListener(

djangocms_text_ckeditor/widgets.py

@@ -16,7 +16,7 @@
 
 
 # this path is changed automatically whenever you run `gulp bundle`
-PATH_TO_JS = 'djangocms_text_ckeditor/js/dist/bundle-807097e2af.cms.ckeditor.min.js'
+PATH_TO_JS = 'djangocms_text_ckeditor/js/dist/bundle-035eee7f1e.cms.ckeditor.min.js'
 
 
 class TextEditorWidget(forms.Textarea):

tests/base.py

@@ -1,7 +1,11 @@
 # -*- coding: utf-8 -*-
 from cms.test_utils.testcases import CMSTestCase
 
-from djangocms_helper.base_test import BaseTestCase as _BaseTestCase
+from djangocms_helper.base_test import (
+    BaseTestCase as _BaseTestCase,
+    BaseTransactionTestCase as _BaseTransactionTestCase
+)
 
 
 BaseTestCase = type('BaseTestCase', (CMSTestCase, _BaseTestCase), {})
+BaseTransactionTestCase = type('BaseTestCase', (CMSTestCase, _BaseTransactionTestCase), {})

tests/test_plugin.py

@@ -1,16 +1,18 @@
 # -*- coding: utf-8 -*-
 import copy
 import json
-import re
 import unittest
+from urllib.parse import parse_qs
+
 
 from django.contrib import admin
 from django.contrib.auth import get_permission_codename
 from django.contrib.auth.models import Permission
+from django.db import IntegrityError
 from django.template import RequestContext
 from django.utils.encoding import force_text
 from django.utils.html import escape
-from django.utils.http import urlencode, urlunquote
+from django.utils.http import urlencode
 
 from cms.api import add_plugin, create_page, create_title
 from cms.models import CMSPlugin, Page
@@ -111,10 +113,10 @@ def get_post_request(self, data):
         return self.get_request(post_data=data)
 
     def get_plugin_id_from_response(self, response):
-        url = urlunquote(response.url)
+        url = parse_qs(response.url)
         # Ideal case, this looks like:
-        # /en/admin/cms/page/edit-plugin/1/
-        return re.findall('\d+', url)[0]  # noqa
+        # /en/admin/cms/placeholder/edit-plugin/?.....&plugin=1
+        return url.get('plugin')[0] # noqa
 
     def test_add_and_edit_plugin(self):
         """
@@ -314,6 +316,148 @@ def test_add_and_cancel_child_plugin(self):
             self.assertObjectDoesNotExist(CMSPlugin.objects.all(), pk=child_plugin_2.pk)
             self.assertObjectDoesNotExist(CMSPlugin.objects.all(), pk=child_plugin_3.pk)
 
+    def test_add_and_cancel_plugin_on_failed_cancellation(self):
+        """
+        Cancelling a text plugin that doesn't successfully cancel does not leave the page
+        in a corrupt state and does not reuse any existing "ghost" plugins.
+        """
+        simple_page = create_page('test page', 'page.html', u'en')
+        simple_placeholder = simple_page.get_placeholders('en').get(slot='content')
+
+        endpoint = self.get_add_plugin_uri(simple_placeholder, 'TextPlugin')
+
+        with self.login_user_context(self.get_superuser()):
+            response = self.client.get(endpoint)
+
+        self.assertEqual(response.status_code, 302)
+
+        # Point to the newly created text plugin
+        text_plugin_pk = self.get_plugin_id_from_response(response)
+
+        # Assert "ghost" plugin has been created
+        self.assertObjectExist(CMSPlugin.objects.all(), pk=text_plugin_pk)
+        # Assert "real" plugin was never created
+        self.assertObjectDoesNotExist(Text.objects.all(), pk=text_plugin_pk)
+
+        # Simulating that the cancellation never succeeded and try to re-add plugin in the same position
+        # This should force a clean on any existing ghosts
+        with self.login_user_context(self.get_superuser()):
+            retry_response = self.client.get(endpoint)
+
+        self.assertEqual(retry_response.status_code, 302)
+
+        retry_text_plugin_pk = self.get_plugin_id_from_response(retry_response)
+
+        # Assert abandoned "ghost" plugin and the new plugin are not the same
+        self.assertNotEqual(text_plugin_pk, retry_text_plugin_pk)
+        # Assert existing "ghost" plugin no longer exists
+        self.assertObjectDoesNotExist(CMSPlugin.objects.all(), pk=text_plugin_pk)
+        # Assert a new "ghost" plugin has been created
+        self.assertObjectExist(CMSPlugin.objects.all(), pk=retry_text_plugin_pk)
+        # Assert "real" plugin was never created
+        self.assertObjectDoesNotExist(Text.objects.all(), pk=retry_text_plugin_pk)
+
+        # The retry "ghost" can be used still after an unsuccessful cancellation previously
+        with self.login_user_context(self.get_superuser()):
+            data = {'body': "Hello world"}
+            add_data_response = self.client.post(retry_response.url, data)
+
+        self.assertEqual(add_data_response.status_code, 200)
+        # Assert "real" plugin is now created
+        self.assertObjectExist(Text.objects.all(), pk=retry_text_plugin_pk)
+
+    def test_add_plugin_after_many_failed_cancellations_leaving_many_old_ghosts(self):
+        """
+        Adding a plugin should still be possible after many failed attempts leaves a
+        minefield of actual plugins and ghosts
+
+        2 Success plugins of TextPlugin
+        1 Failed cancellation plugin  (orphan ghost)
+        1 Success plugin of TextPlugin
+
+        The next position should be 5 but in reality the count is done by the FE which is not aware of the orphaned
+        ghosts so it will actually be 4 and a plugin exists here so there is nothing that we can do unless we clean any
+        orphaned ghosts that have been left behind
+        """
+        simple_page = create_page('test page', 'page.html', u'en')
+        simple_placeholder = simple_page.get_placeholders('en').get(slot='content')
+
+        # 2 Successes, position 1 and 2
+        add_plugin(
+            simple_placeholder,
+            "TextPlugin",
+            "en",
+            body="I'm the first",
+        )
+        add_plugin(
+            simple_placeholder,
+            "TextPlugin",
+            "en",
+            body="I'm the first",
+        )
+
+        # 1 Failure, position 3
+        endpoint = self.get_add_plugin_uri(simple_placeholder, 'TextPlugin')
+        with self.login_user_context(self.get_superuser()):
+            self.client.get(endpoint)
+
+        # 1 Success, position 4
+        add_plugin(
+            simple_placeholder,
+            "TextPlugin",
+            "en",
+            body="I'm the first",
+        )
+
+        # We simulate the fact that the FE can only count actual plugins and the 3rd is a ghost,
+        # The value calculated would be 3 actual plugins so the next position is 4, we know that 4 is actually
+        # populated in this test which is what we need to check i.e. the "ghost" in position 3 is cleaned up
+        # and recalculated leaving position 4 available
+        endpoint = endpoint.replace("plugin_position=3", "plugin_position=4")
+
+        self.assertEqual(CMSPlugin.objects.all().count(), 4)
+
+        with self.login_user_context(self.get_superuser()):
+            response = self.client.get(endpoint)
+
+        self.assertEqual(response.status_code, 302)
+        # The orphaned plugin in position 3 should have been deleted and a new plugin added!
+        self.assertEqual(CMSPlugin.objects.all().count(), 4)
+
+    def test_add_and_cancel_plugin_when_plugin_position_is_taken(self):
+        """
+        A text plugin that already exists in a position returns an error that the plugin
+        position is populated and conflicts i.e. a plugin cannot be created
+        """
+        simple_page = create_page('test page', 'page.html', u'en')
+        simple_placeholder = simple_page.get_placeholders('en').get(slot='content')
+        # Add a plugin
+        endpoint = self.get_add_plugin_uri(simple_placeholder, 'TextPlugin')
+        original_plugin = add_plugin(
+            simple_placeholder,
+            "TextPlugin",
+            "en",
+            body="I'm the first",
+        )
+
+        # Ensure the positions of the new plugin and the endpoint to add another are the same
+        self.assertEqual(original_plugin.position, 1)
+        self.assertTrue("plugin_position=1" in endpoint)
+
+        # Try and add the same plugin again
+        # Adding a plugin in the same location is not allowed
+        # Because it already exists and has contents i.e. it's not am orphaned "ghost"
+        with self.login_user_context(self.get_superuser()):
+
+            with self.assertRaises(Exception) as error:
+                self.client.get(endpoint)
+
+        # An error should be thrown because there is nothing that we can do to rectify this issue
+        self.assertEqual(IntegrityError, type(error.exception))
+        # Only one plugin still exists
+        self.assertEqual(CMSPlugin.objects.all().count(), 1)
+        self.assertObjectExist(Text.objects.all(), pk=original_plugin.pk)
+
     def test_action_token_per_session(self):
         # Assert that a cancel token for the same plugin
         # is different per user session.