-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cancel plugin shouldn't leave db in inconsistent state #515
Conversation
@@ -83,6 +83,8 @@ def get_child_plugins(self): | |||
excluded_plugins = [] | |||
|
|||
queryset = self.text_plugin.get_descendants() | |||
if not queryset.exists(): | |||
queryset = CMSPlugin.objects.all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the text_plugin
contains no descendants the queryset will be empty. DeleteOnCancelForm
will thus be invalid when it is checked. So instead of getting a 200 error you'll get a 400 Bad Request.
token: that.options.action_token | ||
}; | ||
|
||
if (!that.options.delete_on_cancel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here seems wrong in the first place. delete_on_cancel
should be True
if we intend to delete on cancel. With the !
we check the opposite. So this has been changed further down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete_on_cancel
is false if the TextPlugin is being created. this whole thing covered the case when the plugin didn't yet exist but we are creating the TextPlugin and immediately adding children to it.
when delete_on_cancel
is true - that means that the TextPlugin already exists and we need to handle cancelling the children that were created during the current editing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or at least that was the idea, i haven't seen this code in a while)
method: 'POST', | ||
headers: { | ||
'X-CSRFToken': CKEDITOR.tools.getCookie('csrftoken') | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A CSRFToken was missing so the request never succeeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's weird. ckeditor uses https://github.com/divio/django-cms/blob/develop/cms/static/cms/js/modules/cms.base.js#L210 to set csrftoken for every request via djangocms_text_ckeditor/templates/cms/plugins/text_plugin_change_form.html
it could be that the wrong jquery is now being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that could also be the source of the problems (wrong jquery) cause the listener that runs the cancelModalCallback
is attached via django-cms's jquery
59764ce
to
565ab4e
Compare
565ab4e
to
29cd71c
Compare
Closing as this issue is resolved in this PR, thank you @jonathan-s the changes in this PR helped us get moving on a fix that everyone could agree on. 1a16c43 |
Glad it's finally resolved! |
In short, when you click cancel after adding a text plugin and then try to add a new text plugin you'll get an integrity error.
The code prior to this patch seems to have been more concerned with the children of the plugin rather than instance itself.
In
plugin.js
,unsaved_plugin_children
was always empty. There is a function that adds children so it is not empty on line 221, but I was never able to trigger that.Note: Tests are broken since before. This PR #486 was merged 20 August 2018 to support CMS4. 2 days later this PR (django-cms/django-cms#6486) was merged which broke tests for ckeditor.
Fixing the tests should therefore be a separate task.Once #515 is merged we can rebase this again to get rid of the commit that shows that this is passing with the tests changes.