feat: Render diffs in email notifications (#4164)

* deps

* diffCompact

* Diffs in email

* test

* fix: Fade deleted images
fix: Don't include empty paragraphs as context
fix: Allow for same image multiple times and refactor

* Remove target _blank

* fix: Table heading incorrect color
This commit is contained in:
Tom Moor
2022-09-24 23:29:11 +02:00
committed by GitHub
parent 0c5859222f
commit 91d8d27f2d
15 changed files with 396 additions and 85 deletions

View File

@@ -16,7 +16,6 @@ declare module "styled-components" {
tableDivider: string;
tableSelected: string;
tableSelectedBackground: string;
tableHeaderBackground: string;
quote: string;
codeBackground: string;
codeBorder: string;

View File

@@ -10,10 +10,7 @@ export default async function revisionCreator({
user: User;
ip?: string;
}) {
let transaction;
try {
transaction = await sequelize.transaction();
return sequelize.transaction(async (transaction) => {
const revision = await Revision.createFromDocument(document, {
transaction,
});
@@ -32,13 +29,6 @@ export default async function revisionCreator({
transaction,
}
);
await transaction.commit();
return revision;
} catch (err) {
if (transaction) {
await transaction.rollback();
}
throw err;
}
});
}

View File

@@ -3,6 +3,7 @@ import { Document } from "@server/models";
import BaseEmail from "./BaseEmail";
import Body from "./components/Body";
import Button from "./components/Button";
import Diff from "./components/Diff";
import EmailTemplate from "./components/EmailLayout";
import EmptySpace from "./components/EmptySpace";
import Footer from "./components/Footer";
@@ -17,6 +18,7 @@ type InputProps = {
eventName: string;
teamUrl: string;
unsubscribeUrl: string;
content: string;
};
type BeforeSend = {
@@ -73,25 +75,34 @@ Open Document: ${teamUrl}${document.url}
eventName = "published",
teamUrl,
unsubscribeUrl,
content,
}: Props) {
const link = `${teamUrl}${document.url}?ref=notification-email`;
return (
<EmailTemplate>
<Header />
<Body>
<Heading>
"{document.title}" {eventName}
{document.title} {eventName}
</Heading>
<p>
{actorName} {eventName} the document "{document.title}", in the{" "}
{collectionName} collection.
{actorName} {eventName} the document{" "}
<a href={link}>{document.title}</a>, in the {collectionName}{" "}
collection.
</p>
<hr />
<EmptySpace height={10} />
<p>{document.getSummary()}</p>
<EmptySpace height={10} />
{content && (
<>
<EmptySpace height={20} />
<Diff>
<div dangerouslySetInnerHTML={{ __html: content }} />
</Diff>
<EmptySpace height={20} />
</>
)}
<p>
<Button href={`${teamUrl}${document.url}`}>Open Document</Button>
<Button href={link}>Open Document</Button>
</p>
</Body>

View File

@@ -57,7 +57,7 @@ Join now: ${teamUrl}
</p>
<EmptySpace height={10} />
<p>
<Button href={teamUrl}>Join now</Button>
<Button href={`${teamUrl}?ref=invite-email`}>Join now</Button>
</p>
</Body>

View File

@@ -59,7 +59,9 @@ If you haven't signed up yet, you can do so here: ${teamUrl}
<p>If you haven't signed up yet, you can do so here:</p>
<EmptySpace height={10} />
<p>
<Button href={teamUrl}>Join now</Button>
<Button href={`${teamUrl}?ref=invite-reminder-email`}>
Join now
</Button>
</p>
</Body>

View File

@@ -59,7 +59,9 @@ ${teamUrl}/home
</p>
<EmptySpace height={10} />
<p>
<Button href={`${teamUrl}/home`}>Open Outline</Button>
<Button href={`${teamUrl}/home?ref=welcome-email`}>
Open Outline
</Button>
</p>
</Body>

View File

@@ -0,0 +1,25 @@
import * as React from "react";
import theme from "@shared/styles/theme";
type Props = {
children: React.ReactNode;
href?: string;
};
export default ({ children, ...rest }: Props) => {
const style = {
borderRadius: "4px",
background: theme.secondaryBackground,
padding: ".75em 1em",
color: theme.text,
display: "block",
textDecoration: "none",
width: "100%",
};
return (
<div style={style} className="content-diff" {...rest}>
{children}
</div>
);
};

View File

@@ -0,0 +1,83 @@
import Revision from "@server/models/Revision";
import DocumentHelper from "./DocumentHelper";
describe("toEmailDiff", () => {
test("toEmailDiff", () => {
const before = new Revision({
title: "Title",
text: `
This is a test paragraph
- list item 1
- list item 2
:::info
Content in an info block
:::
!!This is a placeholder!!
==this is a highlight==
- [ ] checklist item 1
- [ ] checklist item 2
- [x] checklist item 3
same on both sides
same on both sides
same on both sides`,
});
const after = new Revision({
title: "Title",
text: `
This is a test paragraph
A new paragraph
- list item 1
This is a new paragraph.
!!This is a placeholder!!
==this is a highlight==
- [x] checklist item 1
- [x] checklist item 2
- [ ] checklist item 3
- [ ] checklist item 4
- [x] checklist item 5
same on both sides
same on both sides
same on both sides`,
});
const html = 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");
});
});

View File

@@ -3,6 +3,7 @@ import {
yDocToProsemirrorJSON,
} from "@getoutline/y-prosemirror";
import { JSDOM } from "jsdom";
import { escapeRegExp } from "lodash";
import diff from "node-htmldiff";
import { Node, DOMSerializer } from "prosemirror-model";
import * as React from "react";
@@ -18,12 +19,17 @@ import { parser, schema } from "@server/editor";
import Logger from "@server/logging/Logger";
import Document from "@server/models/Document";
import type Revision from "@server/models/Revision";
import parseAttachmentIds from "@server/utils/parseAttachmentIds";
import { getSignedUrl } from "@server/utils/s3";
import Attachment from "../Attachment";
type HTMLOptions = {
/** Whether to include the document title in the generated HTML (defaults to true) */
includeTitle?: boolean;
/** Whether to include style tags in the generated HTML (defaults to true) */
includeStyles?: boolean;
/** Whether to include styles to center diff (defaults to true) */
centered?: boolean;
};
export default class DocumentHelper {
@@ -73,11 +79,13 @@ export default class DocumentHelper {
const sheet = new ServerStyleSheet();
let html, styleTags;
const Centered = styled.article`
max-width: 46em;
margin: 0 auto;
padding: 0 1em;
`;
const Centered = options?.centered
? styled.article`
max-width: 46em;
margin: 0 auto;
padding: 0 1em;
`
: "article";
const rtl = isRTL(document.title);
const children = (
@@ -142,7 +150,7 @@ export default class DocumentHelper {
}
/**
* Generates a HTML diff between after documents or revisions.
* Generates a HTML diff between documents or revisions.
*
* @param before The before document
* @param after The after document
@@ -172,12 +180,133 @@ export default class DocumentHelper {
// Inject the diffed content into the original document with styling and
// serialize back to a string.
beforeDOM.window.document.getElementsByTagName(
"article"
)[0].innerHTML = diffedContentAsHTML;
const article = beforeDOM.window.document.querySelector("article");
if (article) {
article.innerHTML = diffedContentAsHTML;
}
return beforeDOM.serialize();
}
/**
* Generates a compact HTML diff between documents or revisions, the
* diff is reduced up to show only the parts of the document that changed and
* the immediate context. Breaks in the diff are denoted with
* "div.diff-context-break" nodes.
*
* @param before The before document
* @param after The after document
* @param options Options passed to HTML generation
* @returns The diff as a HTML string
*/
static toEmailDiff(
before: Document | Revision | null,
after: Revision,
options?: HTMLOptions
) {
if (!before) {
return "";
}
const html = DocumentHelper.diff(before, after, options);
const dom = new JSDOM(html);
const doc = dom.window.document;
const containsDiffElement = (node: Element | null) => {
return node && node.innerHTML.includes("data-operation-index");
};
// We use querySelectorAll to get a static NodeList as we'll be modifying
// it as we iterate, rather than getting content.childNodes.
const contents = doc.querySelectorAll("#content > *");
let previousNodeRemoved = false;
let previousDiffClipped = false;
const br = doc.createElement("div");
br.innerHTML = "…";
br.className = "diff-context-break";
for (const childNode of contents) {
// If the block node contains a diff tag then we want to keep it
if (containsDiffElement(childNode as Element)) {
if (previousNodeRemoved && previousDiffClipped) {
childNode.parentElement?.insertBefore(br.cloneNode(true), childNode);
}
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
);
}
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");
const body = doc.querySelector("body");
return `${head?.innerHTML} ${body?.innerHTML}`;
}
/**
* Converts attachment urls in documents to signed equivalents that allow
* direct access without a session cookie
*
* @param text The text either html or markdown which contains urls to be converted
* @param teamId The team context
* @param expiresIn The time that signed urls should expire in (ms)
* @returns The replaced text
*/
static async attachmentsToSignedUrls(
text: string,
teamId: string,
expiresIn = 3000
) {
const attachmentIds = parseAttachmentIds(text);
await Promise.all(
attachmentIds.map(async (id) => {
const attachment = await Attachment.findOne({
where: {
id,
teamId,
},
});
if (attachment) {
const signedUrl = await getSignedUrl(attachment.key, expiresIn);
text = text.replace(
new RegExp(escapeRegExp(attachment.redirectUrl), "g"),
signedUrl
);
}
})
);
return text;
}
/**
* Applies the given Markdown to the document, this essentially creates a
* single change in the collaborative state that makes all the edits to get

View File

@@ -1,35 +1,12 @@
import { escapeRegExp } from "lodash";
import { APM } from "@server/logging/tracing";
import { Document } from "@server/models";
import Attachment from "@server/models/Attachment";
import parseAttachmentIds from "@server/utils/parseAttachmentIds";
import { getSignedUrl } from "@server/utils/s3";
import DocumentHelper from "@server/models/helpers/DocumentHelper";
import presentUser from "./user";
type Options = {
isPublic?: boolean;
};
// replaces attachments.redirect urls with signed/authenticated url equivalents
async function replaceImageAttachments(text: string) {
const attachmentIds = parseAttachmentIds(text);
await Promise.all(
attachmentIds.map(async (id) => {
const attachment = await Attachment.findByPk(id);
if (attachment) {
const signedUrl = await getSignedUrl(attachment.key, 3600);
text = text.replace(
new RegExp(escapeRegExp(attachment.redirectUrl), "g"),
signedUrl
);
}
})
);
return text;
}
async function present(
document: Document,
options: Options | null | undefined = {}
@@ -40,7 +17,10 @@ async function present(
};
await document.migrateVersion();
const text = options.isPublic
? await replaceImageAttachments(document.text)
? await DocumentHelper.attachmentsToSignedUrls(
document.text,
document.teamId
)
: document.text;
const data: Record<string, any> = {

View File

@@ -18,7 +18,7 @@ export default class DebounceProcessor extends BaseProcessor {
{
// speed up revision creation in development, we don't have all the
// time in the world.
delay: (env.ENVIRONMENT === "development" ? 1 : 5) * 60 * 1000,
delay: (env.ENVIRONMENT === "development" ? 0.5 : 5) * 60 * 1000,
}
);
break;

View File

@@ -5,6 +5,7 @@ import {
Subscription,
Event,
Notification,
Revision,
} from "@server/models";
import {
buildDocument,
@@ -156,6 +157,7 @@ describe("documents.publish", () => {
describe("revisions.create", () => {
test("should send a notification to other collaborators", async () => {
const document = await buildDocument();
const revision = await Revision.createFromDocument(document);
const collaborator = await buildUser({ teamId: document.teamId });
document.collaboratorIds = [collaborator.id];
await document.save();
@@ -171,7 +173,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
expect(DocumentNotificationEmail.schedule).toHaveBeenCalled();
@@ -179,6 +181,7 @@ describe("revisions.create", () => {
test("should not send a notification if viewed since update", async () => {
const document = await buildDocument();
const revision = await Revision.createFromDocument(document);
const collaborator = await buildUser({ teamId: document.teamId });
document.collaboratorIds = [collaborator.id];
await document.save();
@@ -196,7 +199,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
expect(DocumentNotificationEmail.schedule).not.toHaveBeenCalled();
@@ -208,6 +211,7 @@ describe("revisions.create", () => {
teamId: user.teamId,
lastModifiedById: user.id,
});
const revision = await Revision.createFromDocument(document);
await NotificationSetting.create({
userId: user.id,
teamId: user.teamId,
@@ -220,7 +224,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: user.id,
modelId: document.id,
modelId: revision.id,
ip,
});
expect(DocumentNotificationEmail.schedule).not.toHaveBeenCalled();
@@ -228,6 +232,7 @@ describe("revisions.create", () => {
test("should send a notification for subscriptions, even to collaborator", async () => {
const document = await buildDocument();
const revision = await Revision.createFromDocument(document);
const collaborator = await buildUser({ teamId: document.teamId });
const subscriber = await buildUser({ teamId: document.teamId });
@@ -256,7 +261,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -268,6 +273,7 @@ describe("revisions.create", () => {
const collaborator1 = await buildUser({ teamId: collaborator0.teamId });
const collaborator2 = await buildUser({ teamId: collaborator0.teamId });
const document = await buildDocument({ userId: collaborator0.id });
const revision = await Revision.createFromDocument(document);
await document.update({
collaboratorIds: [collaborator0.id, collaborator1.id, collaborator2.id],
@@ -281,7 +287,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator0.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -312,6 +318,7 @@ describe("revisions.create", () => {
teamId: collaborator0.teamId,
userId: collaborator0.id,
});
const revision = await Revision.createFromDocument(document);
await document.update({
collaboratorIds: [collaborator0.id, collaborator1.id, collaborator2.id],
@@ -338,7 +345,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator0.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -355,6 +362,7 @@ describe("revisions.create", () => {
teamId: collaborator0.teamId,
userId: collaborator0.id,
});
const revision = await Revision.createFromDocument(document);
await document.update({
collaboratorIds: [collaborator0.id, collaborator1.id, collaborator2.id],
@@ -378,7 +386,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator0.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -406,6 +414,7 @@ describe("revisions.create", () => {
const document = await buildDocument();
const collaborator = await buildUser({ teamId: document.teamId });
const subscriber = await buildUser({ teamId: document.teamId });
const revision = await Revision.createFromDocument(document);
// `subscriber` hasn't collaborated on `document`.
document.collaboratorIds = [collaborator.id];
@@ -435,7 +444,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -444,6 +453,7 @@ describe("revisions.create", () => {
test("should not send a notification for subscriptions to collaborators if unsubscribed", async () => {
const document = await buildDocument();
const revision = await Revision.createFromDocument(document);
const collaborator = await buildUser({ teamId: document.teamId });
const subscriber = await buildUser({ teamId: document.teamId });
@@ -477,7 +487,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -487,6 +497,7 @@ describe("revisions.create", () => {
test("should not send a notification for subscriptions to members outside of the team", async () => {
const document = await buildDocument();
const revision = await Revision.createFromDocument(document);
const collaborator = await buildUser({ teamId: document.teamId });
// `subscriber` *does not* belong
@@ -523,7 +534,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
@@ -533,6 +544,7 @@ describe("revisions.create", () => {
test("should not send a notification if viewed since update", async () => {
const document = await buildDocument();
const revision = await Revision.createFromDocument(document);
const collaborator = await buildUser({ teamId: document.teamId });
document.collaboratorIds = [collaborator.id];
await document.save();
@@ -551,7 +563,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: collaborator.id,
modelId: document.id,
modelId: revision.id,
ip,
});
expect(DocumentNotificationEmail.schedule).not.toHaveBeenCalled();
@@ -563,6 +575,8 @@ describe("revisions.create", () => {
teamId: user.teamId,
lastModifiedById: user.id,
});
const revision = await Revision.createFromDocument(document);
await NotificationSetting.create({
userId: user.id,
teamId: user.teamId,
@@ -575,7 +589,7 @@ describe("revisions.create", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: user.id,
modelId: document.id,
modelId: revision.id,
ip,
});
expect(DocumentNotificationEmail.schedule).not.toHaveBeenCalled();

View File

@@ -5,6 +5,7 @@ import subscriptionCreator from "@server/commands/subscriptionCreator";
import { sequelize } from "@server/database/sequelize";
import CollectionNotificationEmail from "@server/emails/templates/CollectionNotificationEmail";
import DocumentNotificationEmail from "@server/emails/templates/DocumentNotificationEmail";
import env from "@server/env";
import Logger from "@server/logging/Logger";
import {
View,
@@ -15,7 +16,9 @@ import {
NotificationSetting,
Subscription,
Notification,
Revision,
} from "@server/models";
import DocumentHelper from "@server/models/helpers/DocumentHelper";
import {
CollectionEvent,
RevisionEvent,
@@ -34,9 +37,9 @@ export default class NotificationsProcessor extends BaseProcessor {
async perform(event: Event) {
switch (event.name) {
case "documents.publish":
return this.documentPublished(event);
case "revisions.create":
return this.documentUpdated(event);
return this.revisionCreated(event);
case "collections.create":
return this.collectionCreated(event);
@@ -44,10 +47,13 @@ export default class NotificationsProcessor extends BaseProcessor {
}
}
async documentUpdated(event: DocumentEvent | RevisionEvent) {
async documentPublished(event: DocumentEvent) {
// never send notifications when batch importing documents
// @ts-expect-error ts-migrate(2339) FIXME: Property 'data' does not exist on type 'DocumentEv... Remove this comment to see the full error message
if (event.data?.source === "import") {
if (
"data" in event &&
"source" in event.data &&
event.data.source === "import"
) {
return;
}
@@ -65,9 +71,7 @@ export default class NotificationsProcessor extends BaseProcessor {
const recipients = await this.getDocumentNotificationRecipients(
document,
event.name === "documents.publish"
? "documents.publish"
: "documents.update"
"documents.publish"
);
for (const recipient of recipients) {
@@ -84,8 +88,7 @@ export default class NotificationsProcessor extends BaseProcessor {
await DocumentNotificationEmail.schedule(
{
to: recipient.user.email,
eventName:
event.name === "documents.publish" ? "published" : "updated",
eventName: "published",
documentId: document.id,
teamUrl: team.url,
actorName: document.updatedBy.name,
@@ -98,6 +101,66 @@ export default class NotificationsProcessor extends BaseProcessor {
}
}
async revisionCreated(event: RevisionEvent) {
const [collection, document, revision, team] = await Promise.all([
Collection.findByPk(event.collectionId),
Document.findByPk(event.documentId),
Revision.findByPk(event.modelId),
Team.findByPk(event.teamId),
]);
if (!document || !team || !revision || !collection) {
return;
}
await this.createDocumentSubscriptions(document, event);
const recipients = await this.getDocumentNotificationRecipients(
document,
"documents.update"
);
// generate the diff html for the email
const before = await revision.previous();
let content = DocumentHelper.toEmailDiff(before, revision, {
includeTitle: false,
centered: false,
});
content = await DocumentHelper.attachmentsToSignedUrls(
content,
event.teamId,
86400 * 4
);
for (const recipient of recipients) {
const notify = await this.shouldNotify(document, recipient.user);
if (notify) {
const notification = await Notification.create({
event: event.name,
userId: recipient.user.id,
actorId: document.updatedBy.id,
teamId: team.id,
documentId: document.id,
});
await DocumentNotificationEmail.schedule(
{
to: recipient.user.email,
eventName: "updated",
documentId: document.id,
teamUrl: team.url,
actorName: document.updatedBy.name,
collectionName: collection.name,
unsubscribeUrl: recipient.unsubscribeUrl,
content,
},
{ notificationId: notification.id }
);
}
}
}
async collectionCreated(event: CollectionEvent) {
const collection = await Collection.scope("withUser").findByPk(
event.collectionId
@@ -263,7 +326,18 @@ export default class NotificationsProcessor extends BaseProcessor {
});
if (notification) {
return false;
if (env.ENVIRONMENT === "development") {
Logger.info(
"processor",
`would have suppressed notification to ${user.id}, but not in development`
);
} else {
Logger.info(
"processor",
`suppressing notification to ${user.id} as recently notified`
);
return false;
}
}
// If this recipient has viewed the document since the last update was made

View File

@@ -1035,7 +1035,7 @@ table {
}
th {
background: ${props.theme.tableHeaderBackground};
background: transparent;
}
td,
@@ -1284,6 +1284,10 @@ del {
text-decoration: strikethrough;
}
del img {
opacity: .5;
}
@media print {
.placeholder:before,
.block-menu-trigger,

View File

@@ -130,7 +130,6 @@ export const light = {
tableDivider: colors.smokeDark,
tableSelected: colors.primary,
tableSelectedBackground: "#E5F7FF",
tableHeaderBackground: colors.white,
buttonNeutralBackground: colors.white,
buttonNeutralText: colors.almostBlack,
buttonNeutralBorder: darken(0.15, colors.white),
@@ -188,7 +187,6 @@ export const dark = {
tableDivider: colors.lightBlack,
tableSelected: colors.primary,
tableSelectedBackground: "#002333",
tableHeaderBackground: colors.almostBlack,
buttonNeutralBackground: colors.almostBlack,
buttonNeutralText: colors.white,
buttonNeutralBorder: colors.slateDark,