From 80a8f5b7e271ce339526e8c91f2dd3ae796d9db2 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 29 Jan 2023 11:48:37 -0800 Subject: [PATCH] feat: For changes in long tables do not print the entire table in the emailed diff (#4800) --- server/models/helpers/DocumentHelper.test.ts | 103 +++++++++++++------ server/models/helpers/DocumentHelper.tsx | 97 ++++++++++++----- shared/editor/nodes/Table.ts | 2 +- 3 files changed, 139 insertions(+), 63 deletions(-) diff --git a/server/models/helpers/DocumentHelper.test.ts b/server/models/helpers/DocumentHelper.test.ts index ef5107e83..b61920781 100644 --- a/server/models/helpers/DocumentHelper.test.ts +++ b/server/models/helpers/DocumentHelper.test.ts @@ -2,10 +2,11 @@ import Revision from "@server/models/Revision"; import DocumentHelper from "./DocumentHelper"; describe("DocumentHelper", () => { - test("toEmailDiff", async () => { - const before = new Revision({ - title: "Title", - text: ` + describe("toEmailDiff", () => { + it("should render a compact diff", async () => { + const before = new Revision({ + title: "Title", + text: ` This is a test paragraph - list item 1 @@ -28,11 +29,11 @@ same on both sides same on both sides same on both sides`, - }); + }); - const after = new Revision({ - title: "Title", - text: ` + const after = new Revision({ + title: "Title", + text: ` This is a test paragraph A new paragraph @@ -56,35 +57,68 @@ same on both sides same on both sides same on both sides`, + }); + + const html = await DocumentHelper.toEmailDiff(before, after); + + // marks breaks in diff + expect(html).toContain("diff-context-break"); + + // changed list + expect(html).toContain("checklist item 1"); + expect(html).toContain("checklist item 5"); + + // added + expect(html).toContain("A new paragraph"); + + // Retained for context above added paragraph + expect(html).toContain("This is a test paragraph"); + + // removed + expect(html).toContain("Content in an info block"); + + // unchanged + expect(html).not.toContain("same on both sides"); + expect(html).not.toContain("this is a highlight"); }); - const html = await DocumentHelper.toEmailDiff(before, after); + it("should trim table rows to show minimal diff including header", async () => { + const before = new Revision({ + title: "Title", + text: ` +| Syntax | Description | +| ----------- | ----------- | +| Header | Title | +| Paragraph | Text | +| Content | Another | +| More | Content | +| Long | Table |`, + }); - // marks breaks in diff - expect(html).toContain("diff-context-break"); + const after = new Revision({ + title: "Title", + text: ` +| Syntax | Description | +| ----------- | ----------- | +| Header | Title | +| Paragraph | Text | +| Content | Changed | +| More | Content | +| Long | Table |`, + }); - // changed list - expect(html).toContain("checklist item 1"); - expect(html).toContain("checklist item 5"); + const html = await DocumentHelper.toEmailDiff(before, after); - // added - expect(html).toContain("A new paragraph"); - - // Retained for context above added paragraph - expect(html).toContain("This is a test paragraph"); - - // removed - expect(html).toContain("Content in an info block"); - - // unchanged - expect(html).not.toContain("same on both sides"); - expect(html).not.toContain("this is a highlight"); + expect(html).toContain("Changed"); + expect(html).not.toContain("Long"); + }); }); - test("toPlainText", async () => { - const revision = new Revision({ - title: "Title", - text: ` + describe("toPlainText", () => { + it("should return only plain text", async () => { + const revision = new Revision({ + title: "Title", + text: ` This is a test paragraph A new [link](https://www.google.com) @@ -107,12 +141,12 @@ This is a new paragraph. |----|----|----| | Multiple \n Lines \n In a cell | | | | | | |`, - }); + }); - const text = await DocumentHelper.toPlainText(revision); + const text = await DocumentHelper.toPlainText(revision); - // Strip all formatting - expect(text).toEqual(`This is a test paragraph + // Strip all formatting + expect(text).toEqual(`This is a test paragraph A new link @@ -147,5 +181,6 @@ Lines In a cell `); + }); }); }); diff --git a/server/models/helpers/DocumentHelper.tsx b/server/models/helpers/DocumentHelper.tsx index bafefef92..668fcff79 100644 --- a/server/models/helpers/DocumentHelper.tsx +++ b/server/models/helpers/DocumentHelper.tsx @@ -124,7 +124,7 @@ export default class DocumentHelper { {options?.includeTitle !== false && (

{document.title}

)} - {options?.includeStyles ? ( + {options?.includeStyles !== false ? ( {content} @@ -278,36 +278,77 @@ export default class DocumentHelper { previousNodeRemoved = false; previousDiffClipped = true; - // If the block node does not contain a diff tag and the previous - // block node did not contain a diff tag then remove the previous. - } else { - if ( - childNode.nodeName === "P" && - childNode.textContent && - childNode.nextElementSibling?.nodeName === "P" && - containsDiffElement(childNode.nextElementSibling) - ) { - if (previousDiffClipped) { - childNode.parentElement?.insertBefore( - br.cloneNode(true), - childNode - ); + // Special case for largetables, as this block can get very large we + // want to clip it to only the changed rows and surrounding context. + if (childNode.classList.contains("table-wrapper")) { + const rows = childNode.querySelectorAll("tr"); + if (rows.length < 3) { + continue; + } + + let previousRowRemoved = false; + let previousRowDiffClipped = false; + + for (const row of rows) { + if (containsDiffElement(row)) { + const cells = row.querySelectorAll("td"); + if (previousRowRemoved && previousRowDiffClipped) { + const tr = doc.createElement("tr"); + const br = doc.createElement("td"); + br.colSpan = cells.length; + br.innerHTML = "…"; + br.className = "diff-context-break"; + tr.appendChild(br); + childNode.parentElement?.insertBefore(tr, childNode); + } + previousRowRemoved = false; + previousRowDiffClipped = true; + continue; + } + + if (containsDiffElement(row.nextElementSibling)) { + previousRowRemoved = false; + continue; + } + + if (containsDiffElement(row.previousElementSibling)) { + previousRowRemoved = false; + continue; + } + + previousRowRemoved = true; + row.remove(); } - previousNodeRemoved = false; - continue; } - if ( - childNode.nodeName === "P" && - childNode.textContent && - childNode.previousElementSibling?.nodeName === "P" && - containsDiffElement(childNode.previousElementSibling) - ) { - previousNodeRemoved = false; - continue; - } - previousNodeRemoved = true; - childNode.remove(); + + continue; } + + // If the block node does not contain a diff tag and the previous + // block node did not contain a diff tag then remove the previous. + if ( + childNode.nodeName === "P" && + childNode.textContent && + childNode.nextElementSibling?.nodeName === "P" && + containsDiffElement(childNode.nextElementSibling) + ) { + if (previousDiffClipped) { + childNode.parentElement?.insertBefore(br.cloneNode(true), childNode); + } + previousNodeRemoved = false; + continue; + } + if ( + childNode.nodeName === "P" && + childNode.textContent && + childNode.previousElementSibling?.nodeName === "P" && + containsDiffElement(childNode.previousElementSibling) + ) { + previousNodeRemoved = false; + continue; + } + previousNodeRemoved = true; + childNode.remove(); } const head = doc.querySelector("head"); diff --git a/shared/editor/nodes/Table.ts b/shared/editor/nodes/Table.ts index b6635f3e5..797e1abc5 100644 --- a/shared/editor/nodes/Table.ts +++ b/shared/editor/nodes/Table.ts @@ -43,7 +43,7 @@ export default class Table extends Node { toDOM() { return [ "div", - { class: "scrollable-wrapper" }, + { class: "scrollable-wrapper table-wrapper" }, [ "div", { class: "scrollable" },