django-cms / djangocms-text-ckeditor

Text Plugin for django CMS using CKEditor 4
https://www.django-cms.org/en/repositories-plugins/
BSD 3-Clause "New" or "Revised" License
167 stars 185 forks source link

fix: Delay importing `html.clean_html` #636

Closed mbi closed 1 year ago

mbi commented 1 year ago

As mentioned in #468, Importing html.clean_html too early prevents registering custom user models containing an HTMLField.

This PR naively moves these imports into the two functions calling clean_html to delay the import and allow registering the models properly.

NOTE: This lacks a proper test, as I couldn't manage to register a custom user model and define it in settings.AUTH_USER_MODEL using django-cms's app_helper mechanism. If you absolutely need a test, can you please provide guidance on how to define a custom user model, that would trigger the error (when 172e1f0348415fa80ce4b622c34587460e162afd is not present)?

fsbraun commented 1 year ago

@mbi Thank you so much for this PR. I'll have to ponder how to create a HTMLField in a test when the apps are not initialized yet. Two things from my side:

  1. Could you add a changelog entry?
  2. For me the natural decoupling would be to move the CMSPlugin import in utils.py (imported in html.py) into the get_plugins_from_text function. Would that be an alternative?
codecov[bot] commented 1 year ago

Codecov Report

Merging #636 (172e1f0) into master (9ac4c15) will increase coverage by 0.06%. The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
+ Coverage   69.86%   69.93%   +0.06%     
==========================================
  Files          16       16              
  Lines         448      449       +1     
  Branches       49       49              
==========================================
+ Hits          313      314       +1     
  Misses        118      118              
  Partials       17       17              
Impacted Files Coverage Δ
djangocms_text_ckeditor/fields.py 68.88% <71.42%> (+0.70%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mbi commented 1 year ago

For me the natural decoupling would be to move the CMSPlugin import in utils.py (imported in html.py) into the get_plugins_from_text function. Would that be an alternative?

Yes, I just empirically tested this and it also seems to work. I'll update the PR.

mbi commented 1 year ago

@fsbraun I closed this by accident by force-pushing to remove 172e1f0348415fa80ce4b622c34587460e162afd Here is a new PR with the correct fix and changelog. #637.