From 345e9ed503ba4978457b6f80829c4d84dee0dfe4 Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Fri, 16 Jan 2026 00:33:25 +0100 Subject: [PATCH] feat(version): add HTML diff view for multiline field changes (#35837) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- frappe/core/doctype/version/test_version.py | 129 +++++++++++++++++- frappe/core/doctype/version/version.js | 31 +++-- frappe/core/doctype/version/version.py | 53 +++++++ frappe/core/doctype/version/version_view.html | 63 +++++++++ frappe/public/scss/common/css_variables.scss | 5 +- frappe/public/scss/desk/dark.scss | 1 + 6 files changed, 268 insertions(+), 14 deletions(-) diff --git a/frappe/core/doctype/version/test_version.py b/frappe/core/doctype/version/test_version.py index b99dc6f046..e7f8829ef6 100644 --- a/frappe/core/doctype/version/test_version.py +++ b/frappe/core/doctype/version/test_version.py @@ -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("alert", result) + self.assertNotIn("
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(" 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) diff --git a/frappe/core/doctype/version/version.js b/frappe/core/doctype/version/version.js index 1e26e5f748..8fd83bc15f 100644 --- a/frappe/core/doctype/version/version.js +++ b/frappe/core/doctype/version/version.js @@ -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()); + }, }); diff --git a/frappe/core/doctype/version/version.py b/frappe/core/doctype/version/version.py index d3ee8ca22d..7ea6f2f039 100644 --- a/frappe/core/doctype/version/version.py +++ b/frappe/core/doctype/version/version.py @@ -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 "" diff --git a/frappe/core/doctype/version/version_view.html b/frappe/core/doctype/version/version_view.html index 7560118174..bbd63df849 100644 --- a/frappe/core/doctype/version/version_view.html +++ b/frappe/core/doctype/version/version_view.html @@ -1,3 +1,52 @@ +
{% if data.comment %}

{{ __("Comment") + " (" + data.comment_type }})

@@ -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 %}

{{ __("Values Changed") }}

+{% for item in data.changed %} + {% if htmlDiffs[item[0]] %} +
+
{{ frappe.meta.get_label(doc.ref_doctype, item[0]) }}
+
{{ htmlDiffs[item[0]] }}
+
+ {% endif %} +{% endfor %} +{% var hasSimpleChanges = data.changed.some(item => !htmlDiffs[item[0]]) %} +{% if hasSimpleChanges %} @@ -17,15 +77,18 @@ {% for item in data.changed %} + {% if !htmlDiffs[item[0]] %} + {% endif %} {% endfor %}
{{ frappe.meta.get_label(doc.ref_doctype, item[0]) }} {{ getEscapedValue(item[1]) }} {{ getEscapedValue(item[2]) }}
{% endif %} +{% endif %} {% var _keys = ["added", "removed"]; %} {% for key in _keys %} diff --git a/frappe/public/scss/common/css_variables.scss b/frappe/public/scss/common/css_variables.scss index 1ae33c1a01..0fd3fb902c 100644 --- a/frappe/public/scss/common/css_variables.scss +++ b/frappe/public/scss/common/css_variables.scss @@ -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, "); --left-arrow-svg: url("data: image/svg+xml;utf8, "); diff --git a/frappe/public/scss/desk/dark.scss b/frappe/public/scss/desk/dark.scss index bb0cf9c2e5..aa4aabf48c 100644 --- a/frappe/public/scss/desk/dark.scss +++ b/frappe/public/scss/desk/dark.scss @@ -116,6 +116,7 @@ $check-icon-dark: url("data:image/svg+xml,