yuuza/outline
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

chore: Refactor backlinks and revisions (#1611)

Browse Source 
* Update backlinks service to not rely on revisions

* fix: Add missing index for finding backlinks

* Debounce revision creation (#1616)

* refactor debounce logic to service

* Debounce slack notification

* Revisions created by service

* fix: Revision sidebar latest

* test: Add tests for notifications
This commit is contained in:
Tom Moor
2020-11-01 10:26:39 -08:00
committed by GitHub
parent 7735aa12d7
commit 3d09c8f655
20 changed files with 487 additions and 246 deletions
Download Patch File Download Diff File Expand all files Collapse all files

100
server/services/backlinks.js
View File

@@ -1,15 +1,17 @@
// @flow
import { difference } from "lodash";
import type { DocumentEvent } from "../events";
import { Document, Revision, Backlink } from "../models";
import type { DocumentEvent, RevisionEvent } from "../events";
import { Document, Backlink } from "../models";
import { Op } from "../sequelize";
import parseDocumentIds from "../utils/parseDocumentIds";
import slugify from "../utils/slugify";
export default class Backlinks {
async on(event: DocumentEvent) {
async on(event: DocumentEvent | RevisionEvent) {
switch (event.name) {
case "documents.publish": {
const document = await Document.findByPk(event.documentId);
if (!document) return;
const linkIds = parseDocumentIds(document.text);
await Promise.all(
@@ -32,36 +34,18 @@ export default class Backlinks {
break;
}
case "documents.update": {
// no-op for now
if (event.data.autosave) return;
// no-op for drafts
const document = await Document.findByPk(event.documentId);
if (!document) return;
// backlinks are only created for published documents
if (!document.publishedAt) return;
const [currentRevision, previousRevision] = await Revision.findAll({
where: { documentId: event.documentId },
order: [["createdAt", "desc"]],
limit: 2,
});
const linkIds = parseDocumentIds(document.text);
const linkedDocumentIds = [];
// before parsing document text we must make sure it's been migrated to
// the latest version or the parser may fail on version differences
await currentRevision.migrateVersion();
if (previousRevision) {
await previousRevision.migrateVersion();
}
const previousLinkIds = previousRevision
? parseDocumentIds(previousRevision.text)
: [];
const currentLinkIds = parseDocumentIds(currentRevision.text);
const addedLinkIds = difference(currentLinkIds, previousLinkIds);
const removedLinkIds = difference(previousLinkIds, currentLinkIds);
// add any new backlinks that were created
// create or find existing backlink records for referenced docs
await Promise.all(
addedLinkIds.map(async (linkId) => {
linkIds.map(async (linkId) => {
const linkedDocument = await Document.findByPk(linkId);
if (!linkedDocument || linkedDocument.id === event.documentId) {
return;
@@ -73,35 +57,31 @@ export default class Backlinks {
reverseDocumentId: event.documentId,
},
defaults: {
userId: currentRevision.userId,
userId: document.lastModifiedById,
},
});
linkedDocumentIds.push(linkedDocument.id);
})
);
// delete any backlinks that were removed
await Promise.all(
removedLinkIds.map(async (linkId) => {
const document = await Document.findByPk(linkId, {
paranoid: false,
});
if (document) {
await Backlink.destroy({
where: {
documentId: document.id,
reverseDocumentId: event.documentId,
},
});
}
})
);
// delete any backlinks that no longer exist
await Backlink.destroy({
where: {
documentId: {
[Op.notIn]: linkedDocumentIds,
},
reverseDocumentId: event.documentId,
},
});
break;
}
case "documents.title_change": {
const document = await Document.findByPk(event.documentId);
if (!document) return;
if (
!previousRevision ||
currentRevision.title === previousRevision.title
) {
break;
}
// might as well check
const { title, previousTitle } = event.data;
if (!previousTitle || title === previousTitle) break;
// update any link titles in documents that lead to this one
const backlinks = await Backlink.findAll({
@@ -113,7 +93,7 @@ export default class Backlinks {
await Promise.all(
backlinks.map(async (backlink) => {
const previousUrl = `/doc/${slugify(previousRevision.title)}-${
const previousUrl = `/doc/${slugify(previousTitle)}-${
document.urlId
}`;
@@ -121,8 +101,8 @@ export default class Backlinks {
// the old title as anchor text. Go ahead and update those to the
// new title automatically
backlink.reverseDocument.text = backlink.reverseDocument.text.replace(
`[${previousRevision.title}](${previousUrl})`,
`[${document.title}](${document.url})`
`[${previousTitle}](${previousUrl})`,
`[${title}](${document.url})`
);
await backlink.reverseDocument.save({
silent: true,
@@ -136,12 +116,10 @@ export default class Backlinks {
case "documents.delete": {
await Backlink.destroy({
where: {
reverseDocumentId: event.documentId,
},
});
await Backlink.destroy({
where: {
documentId: event.documentId,
[Op.or]: [
{ reverseDocumentId: event.documentId },
{ documentId: event.documentId },
],
},
});
break;

60
server/services/backlinks.test.js
View File

@@ -1,5 +1,5 @@
/* eslint-disable flowtype/require-valid-file-annotation */
import Backlink from "../models/Backlink";
import { Backlink } from "../models";
import { buildDocument } from "../test/factories";
import { flushdb } from "../test/support";
import BacklinksService from "./backlinks";
@@ -22,7 +22,6 @@ describe("documents.update", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
data: { autosave: false },
});
const backlinks = await Backlink.findAll({
@@ -48,7 +47,6 @@ describe("documents.update", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
data: { autosave: false },
});
const backlinks = await Backlink.findAll({
@@ -71,7 +69,6 @@ describe("documents.update", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
data: { autosave: false },
});
const backlinks = await Backlink.findAll({
@@ -83,8 +80,11 @@ describe("documents.update", () => {
test("should destroy removed backlink records", async () => {
const otherDocument = await buildDocument();
const yetAnotherDocument = await buildDocument();
const document = await buildDocument({
text: `[this is a link](${otherDocument.url})`,
text: `[this is a link](${otherDocument.url})
[this is a another link](${yetAnotherDocument.url})`,
});
await Backlinks.on({
@@ -93,10 +93,11 @@ describe("documents.update", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
data: { autosave: false },
});
document.text = "Link is gone";
document.text = `First link is gone
[this is a another link](${yetAnotherDocument.url})`;
await document.save();
await Backlinks.on({
@@ -105,7 +106,39 @@ describe("documents.update", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
data: { autosave: false },
});
const backlinks = await Backlink.findAll({
where: { reverseDocumentId: document.id },
});
expect(backlinks.length).toBe(1);
expect(backlinks[0].documentId).toBe(yetAnotherDocument.id);
});
});
describe("documents.delete", () => {
test("should destroy related backlinks", async () => {
const otherDocument = await buildDocument();
const document = await buildDocument();
document.text = `[this is a link](${otherDocument.url})`;
await document.save();
await Backlinks.on({
name: "documents.update",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
await Backlinks.on({
name: "documents.delete",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
const backlinks = await Backlink.findAll({
@@ -114,11 +147,14 @@ describe("documents.update", () => {
expect(backlinks.length).toBe(0);
});
});
describe("documents.title_change", () => {
test("should update titles in backlinked documents", async () => {
const newTitle = "test";
const document = await buildDocument();
const otherDocument = await buildDocument();
const previousTitle = otherDocument.title;
// create a doc with a link back
document.text = `[${otherDocument.title}](${otherDocument.url})`;
@@ -131,7 +167,6 @@ describe("documents.update", () => {
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
data: { autosave: false },
});
// change the title of the linked doc
@@ -140,12 +175,15 @@ describe("documents.update", () => {
// does the text get updated with the new title
await Backlinks.on({
name: "documents.update",
name: "documents.title_change",
documentId: otherDocument.id,
collectionId: otherDocument.collectionId,
teamId: otherDocument.teamId,
actorId: otherDocument.createdById,
data: { autosave: false },
data: {
previousTitle,
title: newTitle,
},
});
await document.reload();

44
server/services/debouncer.js Normal file
View File

@@ -0,0 +1,44 @@
// @flow
import events, { type Event } from "../events";
import { Document } from "../models";
export default class Debouncer {
async on(event: Event) {
switch (event.name) {
case "documents.update": {
events.add(
{
...event,
name: "documents.update.delayed",
},
{
delay: 5 * 60 * 1000,
removeOnComplete: true,
}
);
break;
}
case "documents.update.delayed": {
const document = await Document.findByPk(event.documentId);
// If the document has been deleted then prevent further processing
if (!document) return;
// If the document has been updated since we initially queued the delayed
// event then abort, there must be another updated event in the queue –
// this functions as a simple distributed debounce.
if (document.updatedAt > new Date(event.createdAt)) return;
events.add(
{
...event,
name: "documents.update.debounced",
},
{ removeOnComplete: true }
);
break;
}
default:
}
}
}

82
server/services/notifications.js
View File

@@ -1,8 +1,9 @@
// @flow
import * as Sentry from "@sentry/node";
import debug from "debug";
import type { DocumentEvent, CollectionEvent, Event } from "../events";
import mailer from "../mailer";
import {
View,
Document,
Team,
Collection,
@@ -10,21 +11,25 @@ import {
NotificationSetting,
} from "../models";
import { Op } from "../sequelize";
import { createQueue } from "../utils/queue";
const notificationsQueue = createQueue("notifications");
const log = debug("services");
notificationsQueue.process(async (job) => {
const event = job.data;
export default class Notifications {
async on(event: Event) {
switch (event.name) {
case "documents.publish":
case "documents.update.debounced":
return this.documentUpdated(event);
case "collections.create":
return this.collectionCreated(event);
default:
}
}
try {
async documentUpdated(event: DocumentEvent) {
const document = await Document.findByPk(event.documentId);
if (!document) return;
// If the document has been updated since we initially queued a notification
// abort sending a notification – this functions as a debounce.
if (document.updatedAt > new Date(event.createdAt)) return;
const { collection } = document;
if (!collection) return;
@@ -37,7 +42,10 @@ notificationsQueue.process(async (job) => {
[Op.ne]: document.lastModifiedById,
},
teamId: document.teamId,
event: event.name,
event:
event.name === "documents.publish"
? "documents.publish"
: "documents.update",
},
include: [
{
@@ -51,17 +59,36 @@ notificationsQueue.process(async (job) => {
const eventName =
event.name === "documents.publish" ? "published" : "updated";
notificationSettings.forEach((setting) => {
for (const setting of notificationSettings) {
// For document updates we only want to send notifications if
// the document has been edited by the user with this notification setting
// This could be replaced with ability to "follow" in the future
if (
event.name === "documents.update" &&
eventName === "updated" &&
!document.collaboratorIds.includes(setting.userId)
) {
return;
}
// If this user has viewed the document since the last update was made
// then we can avoid sending them a useless notification, yay.
const view = await View.findOne({
where: {
userId: setting.userId,
documentId: event.documentId,
updatedAt: {
[Op.gt]: document.updatedAt,
},
},
});
if (view) {
log(
`suppressing notification to ${setting.userId} because update viewed`
);
return;
}
mailer.documentNotification({
to: setting.user.email,
eventName,
@@ -71,37 +98,8 @@ notificationsQueue.process(async (job) => {
actor: document.updatedBy,
unsubscribeUrl: setting.unsubscribeUrl,
});
});
} catch (error) {
if (process.env.SENTRY_DSN) {
Sentry.withScope(function (scope) {
scope.setExtra("event", event);
Sentry.captureException(error);
});
} else {
throw error;
}
}
});
export default class Notifications {
async on(event: Event) {
switch (event.name) {
case "documents.publish":
case "documents.update":
return this.documentUpdated(event);
case "collections.create":
return this.collectionCreated(event);
default:
}
}
async documentUpdated(event: DocumentEvent) {
notificationsQueue.add(event, {
delay: 5 * 60 * 1000,
removeOnComplete: true,
});
}
async collectionCreated(event: CollectionEvent) {
const collection = await Collection.findByPk(event.collectionId, {

87
server/services/notifications.test.js Normal file
View File

@@ -0,0 +1,87 @@
/* eslint-disable flowtype/require-valid-file-annotation */
import mailer from "../mailer";
import { View, NotificationSetting } from "../models";
import { buildDocument, buildUser } from "../test/factories";
import { flushdb } from "../test/support";
import NotificationsService from "./notifications";
jest.mock("../mailer");
const Notifications = new NotificationsService();
beforeEach(() => flushdb());
beforeEach(jest.resetAllMocks);
describe("documents.update.debounced", () => {
test("should send a notification to other collaborator", async () => {
const document = await buildDocument();
const collaborator = await buildUser({ teamId: document.teamId });
document.collaboratorIds = [collaborator.id];
await document.save();
await NotificationSetting.create({
userId: collaborator.id,
teamId: collaborator.teamId,
event: "documents.update",
});
await Notifications.on({
name: "documents.update.debounced",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
expect(mailer.documentNotification).toHaveBeenCalled();
});
test("should not send a notification if viewed since update", async () => {
const document = await buildDocument();
const collaborator = await buildUser({ teamId: document.teamId });
document.collaboratorIds = [collaborator.id];
await document.save();
await NotificationSetting.create({
userId: collaborator.id,
teamId: collaborator.teamId,
event: "documents.update",
});
await View.touch(document.id, collaborator.id, true);
await Notifications.on({
name: "documents.update.debounced",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
expect(mailer.documentNotification).not.toHaveBeenCalled();
});
test("should not send a notification to last editor", async () => {
const user = await buildUser();
const document = await buildDocument({
teamId: user.teamId,
lastModifiedById: user.id,
});
await NotificationSetting.create({
userId: user.id,
teamId: user.teamId,
event: "documents.update",
});
await Notifications.on({
name: "documents.update.debounced",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
expect(mailer.documentNotification).not.toHaveBeenCalled();
});
});

32
server/services/revisions.js Normal file
View File

@@ -0,0 +1,32 @@
// @flow
import type { DocumentEvent, RevisionEvent } from "../events";
import { Revision, Document } from "../models";
export default class Revisions {
async on(event: DocumentEvent | RevisionEvent) {
switch (event.name) {
case "documents.publish":
case "documents.update.debounced": {
const document = await Document.findByPk(event.documentId);
if (!document) return;
const previous = await Revision.findLatest(document.id);
// we don't create revisions if identical to previous revision, this can
// happen if a manual revision was created from another service or user.
if (
previous &&
document.text === previous.text &&
document.title === previous.title
) {
return;
}
await Revision.createFromDocument(document);
break;
}
default:
}
}
}

61
server/services/revisions.test.js Normal file
View File

@@ -0,0 +1,61 @@
/* eslint-disable flowtype/require-valid-file-annotation */
import { Revision } from "../models";
import { buildDocument } from "../test/factories";
import { flushdb } from "../test/support";
import RevisionsService from "./revisions";
const Revisions = new RevisionsService();
beforeEach(() => flushdb());
beforeEach(jest.resetAllMocks);
describe("documents.publish", () => {
test("should create a revision", async () => {
const document = await buildDocument();
await Revisions.on({
name: "documents.publish",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
const amount = await Revision.count({ where: { documentId: document.id } });
expect(amount).toBe(1);
});
});
describe("documents.update.debounced", () => {
test("should create a revision", async () => {
const document = await buildDocument();
await Revisions.on({
name: "documents.update.debounced",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
const amount = await Revision.count({ where: { documentId: document.id } });
expect(amount).toBe(1);
});
test("should not create a revision if identical to previous", async () => {
const document = await buildDocument();
await Revision.createFromDocument(document);
await Revisions.on({
name: "documents.update.debounced",
documentId: document.id,
collectionId: document.collectionId,
teamId: document.teamId,
actorId: document.createdById,
});
const amount = await Revision.count({ where: { documentId: document.id } });
expect(amount).toBe(1);
});
});

22
server/services/slack.js
View File

@@ -7,7 +7,7 @@ export default class Slack {
async on(event: Event) {
switch (event.name) {
case "documents.publish":
case "documents.update":
case "documents.update.debounced":
return this.documentUpdated(event);
case "integrations.create":
return this.integrationCreated(event);
@@ -55,20 +55,6 @@ export default class Slack {
}
async documentUpdated(event: DocumentEvent) {
// lets not send a notification on every autosave update
if (
event.name === "documents.update" &&
event.data &&
event.data.autosave
) {
return;
}
// lets not send a notification on every CMD+S update
if (event.name === "documents.update" && event.data && !event.data.done) {
return;
}
const document = await Document.findByPk(event.documentId);
if (!document) return;
@@ -87,10 +73,10 @@ export default class Slack {
const team = await Team.findByPk(document.teamId);
let text = `${document.createdBy.name} published a new document`;
let text = `${document.updatedBy.name} updated a document`;
if (event.name === "documents.update") {
text = `${document.updatedBy.name} updated a document`;
if (event.name === "documents.publish") {
text = `${document.createdBy.name} published a new document`;
}
await fetch(integration.settings.url, {