feat(version): add HTML diff view for multiline field changes (#35837)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
Raffael Meyer
GitHub
parent
0522c40501
commit
345e9ed503
6 changed files with 268 additions and 14 deletions
|
|
@ -3,12 +3,137 @@
|
|||
import copy
|
||||
|
||||
import frappe
|
||||
from frappe.core.doctype.version.version import get_diff
|
||||
from frappe.tests import IntegrationTestCase
|
||||
from frappe.core.doctype.version.version import (
|
||||
_as_string,
|
||||
_generate_html_diff,
|
||||
_should_generate_html_diff,
|
||||
get_diff,
|
||||
)
|
||||
from frappe.tests import IntegrationTestCase, UnitTestCase
|
||||
from frappe.tests.utils import make_test_objects
|
||||
|
||||
|
||||
class TestHTMLDiff(UnitTestCase):
|
||||
def test_generate_html_diff_produces_table(self):
|
||||
"""Test HTML diff generates a table with content."""
|
||||
result = _generate_html_diff("line1\nline2", "line1\nmodified")
|
||||
|
||||
self.assertIsNotNone(result)
|
||||
self.assertIn("<table", result)
|
||||
self.assertIn("line1", result)
|
||||
|
||||
def test_generate_html_diff_escapes_html(self):
|
||||
"""Test HTML output is properly escaped and safe."""
|
||||
old_value = "<script>alert('xss')</script>\nline2"
|
||||
new_value = "<div>injected</div>\nline2"
|
||||
|
||||
result = _generate_html_diff(old_value, new_value)
|
||||
|
||||
self.assertIsNotNone(result)
|
||||
# Raw script/div tags should be escaped, not executable
|
||||
self.assertNotIn("<script>alert", result)
|
||||
self.assertNotIn("<div>injected", result)
|
||||
# Escaped versions should be present
|
||||
self.assertIn("<script>", result)
|
||||
self.assertIn("<div>", result)
|
||||
|
||||
def test_should_generate_html_diff_multiline(self):
|
||||
"""Test should_generate_html_diff returns True for multiline text."""
|
||||
self.assertTrue(_should_generate_html_diff("line1\nline2", "line1\nmodified"))
|
||||
self.assertTrue(_should_generate_html_diff("single", "multi\nline"))
|
||||
self.assertTrue(_should_generate_html_diff("multi\nline", "single"))
|
||||
|
||||
def test_should_generate_html_diff_long_text(self):
|
||||
"""Test should_generate_html_diff returns True for text > 80 characters."""
|
||||
self.assertTrue(_should_generate_html_diff("a" * 81, "b"))
|
||||
self.assertTrue(_should_generate_html_diff("a", "b" * 81))
|
||||
self.assertTrue(_should_generate_html_diff("a" * 81, "b" * 81))
|
||||
|
||||
def test_should_generate_html_diff_short_text(self):
|
||||
"""Test should_generate_html_diff returns False for short single-line text."""
|
||||
self.assertFalse(_should_generate_html_diff("short", "text"))
|
||||
self.assertFalse(_should_generate_html_diff("a" * 80, "b" * 80)) # Exactly 80 chars
|
||||
|
||||
def test_should_generate_html_diff_empty_values(self):
|
||||
"""Test should_generate_html_diff returns False when either value is empty."""
|
||||
self.assertFalse(_should_generate_html_diff("", "short"))
|
||||
self.assertFalse(_should_generate_html_diff("short", ""))
|
||||
self.assertFalse(_should_generate_html_diff("", ""))
|
||||
# Even long/multiline text returns False if the other value is empty
|
||||
self.assertFalse(_should_generate_html_diff("", "a" * 81))
|
||||
self.assertFalse(_should_generate_html_diff("multi\nline", ""))
|
||||
|
||||
def test_as_string_converts_values(self):
|
||||
"""Test _as_string converts values to strings correctly."""
|
||||
self.assertEqual(_as_string("text"), "text")
|
||||
self.assertEqual(_as_string(None), "")
|
||||
self.assertEqual(_as_string(""), "")
|
||||
self.assertEqual(_as_string(0), "0")
|
||||
|
||||
|
||||
class TestVersion(IntegrationTestCase):
|
||||
def test_onload_generates_html_diffs_for_multiline(self):
|
||||
"""Test onload generates HTML diffs for multiline changes."""
|
||||
version = frappe.get_doc(
|
||||
doctype="Version",
|
||||
ref_doctype="ToDo",
|
||||
docname="test-doc",
|
||||
data=frappe.as_json({"changed": [["description", "line1\nline2", "line1\nmodified"]]}),
|
||||
)
|
||||
|
||||
version.onload()
|
||||
|
||||
html_diffs = version.get_onload().get("html_diffs")
|
||||
self.assertIsNotNone(html_diffs)
|
||||
self.assertIn("description", html_diffs)
|
||||
self.assertIn("<table", html_diffs["description"])
|
||||
|
||||
def test_onload_generates_html_diffs_for_long_text(self):
|
||||
"""Test onload generates HTML diffs for text > 80 characters."""
|
||||
version = frappe.get_doc(
|
||||
doctype="Version",
|
||||
ref_doctype="ToDo",
|
||||
docname="test-doc",
|
||||
data=frappe.as_json({"changed": [["notes", "x" * 81, "y" * 81]]}),
|
||||
)
|
||||
|
||||
version.onload()
|
||||
|
||||
html_diffs = version.get_onload().get("html_diffs")
|
||||
self.assertIsNotNone(html_diffs)
|
||||
self.assertIn("notes", html_diffs)
|
||||
|
||||
def test_onload_no_html_diffs_for_simple_changes(self):
|
||||
"""Test onload doesn't generate HTML diffs for simple short changes."""
|
||||
version = frappe.get_doc(
|
||||
doctype="Version",
|
||||
ref_doctype="ToDo",
|
||||
docname="test-doc",
|
||||
data=frappe.as_json({"changed": [["status", "Open", "Closed"]]}),
|
||||
)
|
||||
|
||||
version.onload()
|
||||
|
||||
html_diffs = version.get_onload().get("html_diffs")
|
||||
self.assertIsNone(html_diffs)
|
||||
|
||||
def test_onload_handles_empty_data(self):
|
||||
"""Test onload handles empty or missing data gracefully."""
|
||||
version = frappe.get_doc(
|
||||
doctype="Version",
|
||||
ref_doctype="ToDo",
|
||||
docname="test-doc",
|
||||
data=None,
|
||||
)
|
||||
|
||||
# Should not raise an error
|
||||
version.onload()
|
||||
self.assertIsNone(version.get_onload().get("html_diffs"))
|
||||
|
||||
version.data = frappe.as_json({"changed": []})
|
||||
version.onload()
|
||||
self.assertIsNone(version.get_onload().get("html_diffs"))
|
||||
|
||||
def test_get_diff(self):
|
||||
frappe.set_user("Administrator")
|
||||
test_records = make_test_objects("Event", reset=True)
|
||||
|
|
|
|||
|
|
@ -1,12 +1,23 @@
|
|||
frappe.ui.form.on("Version", "refresh", function (frm) {
|
||||
$(
|
||||
frappe.render_template("version_view", { doc: frm.doc, data: JSON.parse(frm.doc.data) })
|
||||
).appendTo(frm.fields_dict.table_html.$wrapper.empty());
|
||||
|
||||
frm.add_custom_button(__("Show all Versions"), function () {
|
||||
frappe.set_route("List", "Version", {
|
||||
ref_doctype: frm.doc.ref_doctype,
|
||||
docname: frm.doc.docname,
|
||||
frappe.ui.form.on("Version", {
|
||||
refresh: function (frm) {
|
||||
frm.add_custom_button(__("Show all Versions"), function () {
|
||||
frappe.set_route("List", "Version", {
|
||||
ref_doctype: frm.doc.ref_doctype,
|
||||
docname: frm.doc.docname,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
frm.trigger("render_version_view");
|
||||
},
|
||||
|
||||
render_version_view: async function (frm) {
|
||||
await frappe.model.with_doctype(frm.doc.ref_doctype);
|
||||
|
||||
$(
|
||||
frappe.render_template("version_view", {
|
||||
doc: frm.doc,
|
||||
data: JSON.parse(frm.doc.data),
|
||||
})
|
||||
).appendTo(frm.fields_dict.table_html.$wrapper.empty());
|
||||
},
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
|
||||
# License: MIT. See LICENSE
|
||||
|
||||
import difflib
|
||||
import json
|
||||
|
||||
import frappe
|
||||
|
|
@ -74,6 +75,29 @@ class Version(Document):
|
|||
def get_data(self):
|
||||
return json.loads(self.data)
|
||||
|
||||
def onload(self):
|
||||
"""Generate HTML diffs for multiline changes on document load."""
|
||||
if not self.data:
|
||||
return
|
||||
|
||||
data = self.get_data()
|
||||
changed = data.get("changed", [])
|
||||
if not changed:
|
||||
return
|
||||
|
||||
html_diffs = {}
|
||||
for item in changed:
|
||||
if len(item) >= 3:
|
||||
fieldname, old_str, new_str = item[0], _as_string(item[1]), _as_string(item[2])
|
||||
if not _should_generate_html_diff(old_str, new_str):
|
||||
continue
|
||||
html_diff = _generate_html_diff(old_str, new_str)
|
||||
if html_diff:
|
||||
html_diffs[fieldname] = html_diff
|
||||
|
||||
if html_diffs:
|
||||
self.set_onload("html_diffs", html_diffs)
|
||||
|
||||
|
||||
def get_diff(old, new, for_child=False, compare_cancelled=False):
|
||||
"""Get diff between 2 document objects
|
||||
|
|
@ -203,3 +227,32 @@ def get_diff(old, new, for_child=False, compare_cancelled=False):
|
|||
|
||||
def on_doctype_update():
|
||||
frappe.db.add_index("Version", ["ref_doctype", "docname"])
|
||||
|
||||
|
||||
def _generate_html_diff(old_str: str, new_str: str) -> str | None:
|
||||
"""Generate HTML diff for the given old and new strings."""
|
||||
old_lines = old_str.splitlines(keepends=True)
|
||||
new_lines = new_str.splitlines(keepends=True)
|
||||
|
||||
differ = difflib.HtmlDiff(wrapcolumn=80)
|
||||
html_diff = differ.make_table(
|
||||
old_lines,
|
||||
new_lines,
|
||||
fromdesc=frappe._("Original"),
|
||||
todesc=frappe._("New"),
|
||||
context=True,
|
||||
numlines=3,
|
||||
)
|
||||
return html_diff
|
||||
|
||||
|
||||
def _should_generate_html_diff(old_str: str, new_str: str) -> bool:
|
||||
"""Determine if HTML diff should be generated for the given values."""
|
||||
return (
|
||||
old_str and new_str and ("\n" in old_str or "\n" in new_str or len(old_str) > 80 or len(new_str) > 80)
|
||||
)
|
||||
|
||||
|
||||
def _as_string(value: str | None) -> str:
|
||||
"""Convert the given value to a string."""
|
||||
return cstr(value) if value is not None else ""
|
||||
|
|
|
|||
|
|
@ -1,3 +1,52 @@
|
|||
<style>
|
||||
.version-diff-container {
|
||||
margin-bottom: 1rem;
|
||||
}
|
||||
.version-diff-container h5 {
|
||||
margin-bottom: 0.5rem;
|
||||
}
|
||||
.version-html-diff table.diff {
|
||||
width: 100%;
|
||||
border-collapse: collapse;
|
||||
font-family: monospace;
|
||||
font-size: 12px;
|
||||
}
|
||||
.version-html-diff table.diff td {
|
||||
padding: 2px 6px;
|
||||
border: 1px solid var(--border-color);
|
||||
vertical-align: top;
|
||||
}
|
||||
.version-html-diff table.diff .diff_header {
|
||||
background-color: var(--subtle-fg);
|
||||
text-align: right;
|
||||
padding: 2px 6px;
|
||||
color: var(--text-muted);
|
||||
font-weight: normal;
|
||||
width: 40px;
|
||||
}
|
||||
.version-html-diff table.diff .diff_next {
|
||||
background-color: var(--subtle-fg);
|
||||
width: 10px;
|
||||
}
|
||||
.version-html-diff table.diff .diff_add {
|
||||
background-color: var(--diff-added);
|
||||
}
|
||||
.version-html-diff table.diff .diff_chg {
|
||||
background-color: var(--diff-changed);
|
||||
}
|
||||
.version-html-diff table.diff .diff_sub {
|
||||
background-color: var(--diff-removed);
|
||||
}
|
||||
.version-html-diff table.diff th {
|
||||
background-color: var(--subtle-fg);
|
||||
padding: 6px;
|
||||
border: 1px solid var(--border-color);
|
||||
font-weight: 500;
|
||||
}
|
||||
.version-html-diff table.diff colgroup {
|
||||
display: none;
|
||||
}
|
||||
</style>
|
||||
<div class="version-info">
|
||||
{% if data.comment %}
|
||||
<h4>{{ __("Comment") + " (" + data.comment_type }})</h4>
|
||||
|
|
@ -5,8 +54,19 @@
|
|||
{% endif %}
|
||||
|
||||
{% const getEscapedValue = (v) => v === null ? "null" : frappe.utils.escape_html(v) %}
|
||||
{% const htmlDiffs = (doc.__onload && doc.__onload.html_diffs) || {} %}
|
||||
{% if data.changed && data.changed.length %}
|
||||
<h4>{{ __("Values Changed") }}</h4>
|
||||
{% for item in data.changed %}
|
||||
{% if htmlDiffs[item[0]] %}
|
||||
<div class="version-diff-container">
|
||||
<h5>{{ frappe.meta.get_label(doc.ref_doctype, item[0]) }}</h5>
|
||||
<div class="version-html-diff">{{ htmlDiffs[item[0]] }}</div>
|
||||
</div>
|
||||
{% endif %}
|
||||
{% endfor %}
|
||||
{% var hasSimpleChanges = data.changed.some(item => !htmlDiffs[item[0]]) %}
|
||||
{% if hasSimpleChanges %}
|
||||
<table class="table table-bordered">
|
||||
<thead>
|
||||
<tr>
|
||||
|
|
@ -17,15 +77,18 @@
|
|||
</thead>
|
||||
<tbody>
|
||||
{% for item in data.changed %}
|
||||
{% if !htmlDiffs[item[0]] %}
|
||||
<tr>
|
||||
<td>{{ frappe.meta.get_label(doc.ref_doctype, item[0]) }}</td>
|
||||
<td class="diff-remove">{{ getEscapedValue(item[1]) }}</td>
|
||||
<td class="diff-add">{{ getEscapedValue(item[2]) }}</td>
|
||||
</tr>
|
||||
{% endif %}
|
||||
{% endfor %}
|
||||
</tbody>
|
||||
</table>
|
||||
{% endif %}
|
||||
{% endif %}
|
||||
|
||||
{% var _keys = ["added", "removed"]; %}
|
||||
{% for key in _keys %}
|
||||
|
|
|
|||
|
|
@ -150,8 +150,9 @@ $disabled-input-height: 22px;
|
|||
--switch-bg: var(--gray-300);
|
||||
|
||||
// "diff" colors
|
||||
--diff-added: var(--green-100);
|
||||
--diff-removed: var(--red-100);
|
||||
--diff-added: var(--green-200);
|
||||
--diff-removed: var(--red-200);
|
||||
--diff-changed: var(--blue-200);
|
||||
|
||||
--right-arrow-svg: url("data: image/svg+xml;utf8, <svg width='6' height='8' viewBox='0 0 6 8' fill='none' xmlns='http://www.w3.org/2000/svg'><path d='M1.25 7.5L4.75 4L1.25 0.5' stroke='%231F272E' stroke-linecap='round' stroke-linejoin='round'/></svg>");
|
||||
--left-arrow-svg: url("data: image/svg+xml;utf8, <svg width='6' height='8' viewBox='0 0 6 8' fill='none' xmlns='http://www.w3.org/2000/svg'><path d='M7.5 9.5L4 6l3.5-3.5' stroke='%231F272E' stroke-linecap='round' stroke-linejoin='round'></path></svg>");
|
||||
|
|
|
|||
|
|
@ -116,6 +116,7 @@ $check-icon-dark: url("data:image/svg+xml, <svg viewBox='0 0 8 7' fill='none' xm
|
|||
// "diff" colors
|
||||
--diff-added: var(--green-800);
|
||||
--diff-removed: var(--red-800);
|
||||
--diff-changed: var(--blue-800);
|
||||
|
||||
// sidebar toggle
|
||||
.page-title .sidebar-toggle-btn {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue