Skip to content
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

504 copy paste plugins inside ckeditor refer to same instance #572

Conversation

nichoski
Copy link
Contributor

This is fixing a severe bug caused by pull request 569. In pull request 569, after the referenced plugin is copied successfully, the post_copy method is removing the other child plugins of the plugin we are pasting into, because the other child plugins are not inside the plugins_ziplist provided in the arguments of the post_copy method. This fix is including the existing plugins as well, so they are not touched by the post_copy method.

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2021

Codecov Report

Merging #572 (5d68827) into master (1221448) will increase coverage by 0.49%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   72.72%   73.22%   +0.49%     
==========================================
  Files          19       19              
  Lines         825      829       +4     
  Branches       90       91       +1     
==========================================
+ Hits          600      607       +7     
+ Misses        200      198       -2     
+ Partials       25       24       -1     
Impacted Files Coverage Δ
djangocms_text_ckeditor/models.py 87.50% <100.00%> (+4.60%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1221448...5d68827. Read the comment docs.

Copy link
Member

@vinitkumar vinitkumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@gabrielcandrade
Copy link
Member

Looks Good! 👍

@marksweb
Copy link
Member

@nichoski do you think, given what we've seen happen here, that a test case could be written for this?

@nichoski
Copy link
Contributor Author

@marksweb yes, it's the least I could do. I just added a test case to this.

@vinitkumar vinitkumar merged commit d08a6e4 into django-cms:master Jun 24, 2021
@nichoski
Copy link
Contributor Author

This was merged into master, but I don't see it in any of the releases. Is this planned to be added in a new release? Or is it already added and I have issues finding it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants