fix: Support django CMS 5 data bridge for text-enabled plugins #87
Conversation
Reviewer's GuideCentralize and standardize Django CMS data bridge parsing to support CMS 5, refactor the clean_html API by deprecating the full parameter and update all callers and tests accordingly, and fix a plugin template reference bug with minor test cleanup. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
- Coverage 81.80% 81.61% -0.20%
==========================================
Files 14 14
Lines 940 941 +1
Branches 109 110 +1
==========================================
- Hits 769 768 -1
- Misses 129 130 +1
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
Undefined variable
bodyin processDataBridge (link) -
In processDataBridge you’re using an undefined
bodyvariable for the regex—make sure to pass in or derive the raw HTML string rather than referencingbody. -
Remove the
console.logcalls in processDataBridge once debugging is complete to avoid cluttering the console. -
The clean_html docstring still refers to
full: bool = False; please update it to reflect the newOptional[bool]signature and behavior.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!this.CMS.API.Helpers.dataBridge) { | ||
| // No databridge found | ||
| this.CMS.API.Helpers.reloadBrowser('REFRESH_PAGE'); | ||
| return; |
There was a problem hiding this comment.
issue (bug_risk): Undefined variable body in processDataBridge
Pass the HTML string (e.g., dom.innerHTML) into processDataBridge or rename the variable so that body is defined.
| this.CMS.API.Helpers.reloadBrowser('REFRESH_PAGE'); | ||
| return; | ||
| } | ||
| this.processDataBridge(dom); |
There was a problem hiding this comment.
issue (bug_risk): dataBridge fallback sets empty object, preventing reload
Assigning {} makes dataBridge truthy, so the if (!dataBridge) check never fires. Return a success flag or set dataBridge to null/undefined on failure to allow the reload path to run.
| @@ -497,6 +486,29 @@ | |||
| } | |||
| } | |||
|
|
|||
| processDataBridge(dom) { | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the dataBridge extraction logic into a single helper function that handles all cases, removes duplication, and restores immediate reload on failure.
Here’s one way to collapse the three “find‐then‐fallback” branches, kill the duplicated regex, remove the stray logs, and restore the original “reload on missing bridge” behavior—all while keeping every code‐path intact:
// call site
el.dataset.changed = 'false';
const dataBridge = this._fetchDataBridge(dom);
if (!dataBridge) {
this.CMS.API.Helpers.reloadBrowser('REFRESH_PAGE');
return;
}
delete dataBridge.structure?.content;
this.CMS.API.Helpers.dataBridge = dataBridge;
…// new helper
_fetchDataBridge(dom, body = dom.innerHTML) {
// 1) inline JSON <script id="data-bridge">…</script>
const script = dom.querySelector('script#data-bridge');
const txt = script?.textContent?.trim();
if (txt) {
return JSON.parse(txt);
}
// 2) legacy JS assignments in page HTML
const rx1 = /^\s*Window\.CMS\.API\.Helpers\.dataBridge\s*=\s*(.*?);/gm.exec(body);
const rx2 = /^\s*Window\.CMS\.API\.Helpers\.dataBridge\.structure\s*=\s*(.*?);/gm.exec(body);
if (rx1 && rx2) {
const bridge = JSON.parse(rx1[1]);
bridge.structure = JSON.parse(rx2[1]);
return bridge;
}
// 3) nothing found
return null;
}—This
- inlines your two regexes into one fall-through block
- returns
nullon failure so the caller can reload immediately - removes all
console.lognoise - keeps the
delete structure.contentstep right after you’ve parsed - never relies on the mysterious undefined
body—defaults todom.innerHTMLbut can be overridden (e.g. in the form callback)
This PR fixes #86 :
django CMS 5's data bridge (the data connection between JS frontend editor and the
cmsapp) has a new format. Now, djangocms-text accepts django CMS 3.11, 4.1, and 5.0 data bridge formats for both inline editing and adding text-enabled plugins.Summary by Sourcery
Add compatibility with django CMS 5 data bridge formats and refactor JS and Python code to streamline data bridge handling and HTML sanitization, including deprecation of the clean_html full flag and test adjustments.
New Features:
Bug Fixes:
Enhancements:
Tests: