diff --git a/app/scenes/Document/components/DataLoader.js b/app/scenes/Document/components/DataLoader.js index 03d97ce06..6aceca7c8 100644 --- a/app/scenes/Document/components/DataLoader.js +++ b/app/scenes/Document/components/DataLoader.js @@ -64,7 +64,11 @@ class DataLoader extends React.Component { // Also need to load the revision if it changes const { revisionId } = this.props.match.params; - if (prevProps.match.params.revisionId !== revisionId && revisionId) { + if ( + prevProps.match.params.revisionId !== revisionId && + revisionId && + revisionId !== "latest" + ) { this.loadRevision(); } } @@ -152,7 +156,7 @@ class DataLoader extends React.Component { shareId, }); - if (revisionId) { + if (revisionId && revisionId !== "latest") { await this.loadRevision(); } else { this.revision = undefined; diff --git a/app/stores/RevisionsStore.js b/app/stores/RevisionsStore.js index c975dcade..348083cd2 100644 --- a/app/stores/RevisionsStore.js +++ b/app/stores/RevisionsStore.js @@ -16,7 +16,31 @@ export default class RevisionsStore extends BaseStore { } getDocumentRevisions(documentId: string): Revision[] { - return filter(this.orderedData, { documentId }); + let revisions = filter(this.orderedData, { documentId }); + const latestRevision = revisions[0]; + const document = this.rootStore.documents.get(documentId); + + // There is no guarantee that we have a revision that represents the latest + // state of the document. This pushes a fake revision in at the top if there + // isn't one + if ( + latestRevision && + document && + latestRevision.createdAt !== document.updatedAt + ) { + revisions.unshift( + new Revision({ + id: "latest", + documentId: document.id, + title: document.title, + text: document.text, + createdAt: document.updatedAt, + createdBy: document.createdBy, + }) + ); + } + + return revisions; } @action diff --git a/server/api/documents.js b/server/api/documents.js index 749f7082f..b40a80ea2 100644 --- a/server/api/documents.js +++ b/server/api/documents.js @@ -870,6 +870,8 @@ router.post("documents.update", auth(), async (ctx) => { throw new InvalidRequestError("Document has changed since last revision"); } + const previousTitle = document.title; + // Update document if (title) document.title = title; if (editorVersion) document.editorVersion = editorVersion; @@ -926,6 +928,21 @@ router.post("documents.update", auth(), async (ctx) => { }); } + if (document.title !== previousTitle) { + Event.add({ + name: "documents.title_change", + documentId: document.id, + collectionId: document.collectionId, + teamId: document.teamId, + actorId: user.id, + data: { + previousTitle, + title: document.title, + }, + ip: ctx.request.ip, + }); + } + document.updatedBy = user; document.collection = collection; diff --git a/server/api/documents.test.js b/server/api/documents.test.js index ff20f03cc..4283e6b43 100644 --- a/server/api/documents.test.js +++ b/server/api/documents.test.js @@ -1369,9 +1369,7 @@ describe("#documents.restore", () => { it("should restore the document to a previous version", async () => { const { user, document } = await seed(); - const revision = await Revision.findOne({ - where: { documentId: document.id }, - }); + const revision = await Revision.createFromDocument(document); const previousText = revision.text; const revisionId = revision.id; @@ -1391,9 +1389,7 @@ describe("#documents.restore", () => { it("should not allow restoring a revision in another document", async () => { const { user, document } = await seed(); const anotherDoc = await buildDocument(); - const revision = await Revision.findOne({ - where: { documentId: anotherDoc.id }, - }); + const revision = await Revision.createFromDocument(anotherDoc); const revisionId = revision.id; const res = await server.post("/api/documents.restore", { @@ -1421,9 +1417,7 @@ describe("#documents.restore", () => { it("should require authorization", async () => { const { document } = await seed(); - const revision = await Revision.findOne({ - where: { documentId: document.id }, - }); + const revision = await Revision.createFromDocument(document); const revisionId = revision.id; const user = await buildUser(); @@ -1684,31 +1678,6 @@ describe("#documents.update", () => { expect(res.status).toEqual(403); }); - it("should not create new version when autosave=true", async () => { - const { user, document } = await seed(); - - const res = await server.post("/api/documents.update", { - body: { - token: user.getJwtToken(), - id: document.id, - title: "Updated title", - text: "Updated text", - lastRevision: document.revision, - autosave: true, - }, - }); - - const prevRevisionRecords = await Revision.count(); - const body = await res.json(); - - expect(res.status).toEqual(200); - expect(body.data.title).toBe("Updated title"); - expect(body.data.text).toBe("Updated text"); - - const revisionRecords = await Revision.count(); - expect(revisionRecords).toBe(prevRevisionRecords); - }); - it("should fail if document lastRevision does not match", async () => { const { user, document } = await seed(); diff --git a/server/api/revisions.test.js b/server/api/revisions.test.js index 2af42b274..e49294d02 100644 --- a/server/api/revisions.test.js +++ b/server/api/revisions.test.js @@ -1,7 +1,7 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import TestServer from "fetch-test-server"; import app from "../app"; -import Revision from "../models/Revision"; +import { Revision } from "../models"; import { buildDocument, buildUser } from "../test/factories"; import { flushdb, seed } from "../test/support"; @@ -13,11 +13,8 @@ afterAll(() => server.close()); describe("#revisions.info", () => { it("should return a document revision", async () => { const { user, document } = await seed(); - const revision = await Revision.findOne({ - where: { - documentId: document.id, - }, - }); + const revision = await Revision.createFromDocument(document); + const res = await server.post("/api/revisions.info", { body: { token: user.getJwtToken(), @@ -33,11 +30,8 @@ describe("#revisions.info", () => { it("should require authorization", async () => { const document = await buildDocument(); - const revision = await Revision.findOne({ - where: { - documentId: document.id, - }, - }); + const revision = await Revision.createFromDocument(document); + const user = await buildUser(); const res = await server.post("/api/revisions.info", { body: { @@ -52,6 +46,8 @@ describe("#revisions.info", () => { describe("#revisions.list", () => { it("should return a document's revisions", async () => { const { user, document } = await seed(); + await Revision.createFromDocument(document); + const res = await server.post("/api/revisions.list", { body: { token: user.getJwtToken(), @@ -68,6 +64,8 @@ describe("#revisions.list", () => { it("should not return revisions for document in collection not a member of", async () => { const { user, document, collection } = await seed(); + await Revision.createFromDocument(document); + collection.private = true; await collection.save(); diff --git a/server/events.js b/server/events.js index a72166581..5ed9786b4 100644 --- a/server/events.js +++ b/server/events.js @@ -1,11 +1,14 @@ // @flow import * as Sentry from "@sentry/node"; +import debug from "debug"; import services from "./services"; import { createQueue } from "./utils/queue"; +const log = debug("services"); + export type UserEvent = | { - name: | 'users.create' // eslint-disable-line + name: | "users.create" // eslint-disable-line | "users.update" | "users.suspend" | "users.activate" @@ -26,7 +29,7 @@ export type UserEvent = export type DocumentEvent = | { - name: | 'documents.create' // eslint-disable-line + name: | "documents.create" // eslint-disable-line | "documents.publish" | "documents.delete" | "documents.pin" @@ -53,20 +56,43 @@ export type DocumentEvent = }, } | { - name: "documents.update", + name: | "documents.update" // eslint-disable-line + | "documents.update.delayed" + | "documents.update.debounced", documentId: string, collectionId: string, + createdAt: string, teamId: string, actorId: string, data: { + title: string, autosave: boolean, done: boolean, }, + } + | { + name: "documents.title_change", + documentId: string, + collectionId: string, + createdAt: string, + teamId: string, + actorId: string, + data: { + title: string, + previousTitle: string, + }, }; +export type RevisionEvent = { + name: "revisions.create", + documentId: string, + collectionId: string, + teamId: string, +}; + export type CollectionEvent = | { - name: | 'collections.create' // eslint-disable-line + name: | "collections.create" // eslint-disable-line | "collections.update" | "collections.delete", collectionId: string, @@ -120,7 +146,8 @@ export type Event = | DocumentEvent | CollectionEvent | IntegrationEvent - | GroupEvent; + | GroupEvent + | RevisionEvent; const globalEventsQueue = createQueue("global events"); const serviceEventsQueue = createQueue("service events"); @@ -132,7 +159,7 @@ globalEventsQueue.process(async (job) => { const service = services[name]; if (service.on) { serviceEventsQueue.add( - { service: name, ...job.data }, + { ...job.data, service: name }, { removeOnComplete: true } ); } @@ -145,6 +172,8 @@ serviceEventsQueue.process(async (job) => { const service = services[event.service]; if (service.on) { + log(`${event.service} processing ${event.name}`); + service.on(event).catch((error) => { if (process.env.SENTRY_DSN) { Sentry.withScope(function (scope) { diff --git a/server/migrations/20201028043021-reverse-document-id-index.js b/server/migrations/20201028043021-reverse-document-id-index.js new file mode 100644 index 000000000..470fe9060 --- /dev/null +++ b/server/migrations/20201028043021-reverse-document-id-index.js @@ -0,0 +1,11 @@ +'use strict'; + +module.exports = { + up: async (queryInterface, Sequelize) => { + await queryInterface.addIndex("backlinks", ["reverseDocumentId"]); + }, + + down: async (queryInterface, Sequelize) => { + await queryInterface.removeIndex("backlinks", ["reverseDocumentId"]); + } +}; diff --git a/server/models/Document.js b/server/models/Document.js index 512076021..6d5700cf1 100644 --- a/server/models/Document.js +++ b/server/models/Document.js @@ -19,32 +19,6 @@ const serializer = new MarkdownSerializer(); export const DOCUMENT_VERSION = 2; -const createRevision = async (doc, options = {}) => { - // we don't create revisions for autosaves - if (options.autosave) return; - - const previous = await Revision.findLatest(doc.id); - - // we don't create revisions if identical to previous - if (previous && doc.text === previous.text && doc.title === previous.title) { - return; - } - - return Revision.create( - { - title: doc.title, - text: doc.text, - userId: doc.lastModifiedById, - editorVersion: doc.editorVersion, - version: doc.version, - documentId: doc.id, - }, - { - transaction: options.transaction, - } - ); -}; - const createUrlId = (doc) => { return (doc.urlId = doc.urlId || randomstring.generate(10)); }; @@ -118,8 +92,6 @@ const Document = sequelize.define( beforeValidate: createUrlId, beforeCreate: beforeCreate, beforeUpdate: beforeSave, - afterCreate: createRevision, - afterUpdate: createRevision, }, getterMethods: { url: function () { diff --git a/server/models/Document.test.js b/server/models/Document.test.js index 3befac7c5..6ecdb7747 100644 --- a/server/models/Document.test.js +++ b/server/models/Document.test.js @@ -1,5 +1,5 @@ /* eslint-disable flowtype/require-valid-file-annotation */ -import { Document, Revision } from "../models"; +import { Document } from "../models"; import { buildDocument, buildCollection, @@ -11,37 +11,6 @@ import { flushdb } from "../test/support"; beforeEach(() => flushdb()); beforeEach(jest.resetAllMocks); -describe("#createRevision", () => { - test("should create revision on document creation", async () => { - const document = await buildDocument(); - - document.title = "Changed"; - await document.save({ autosave: true }); - - const amount = await Revision.count({ where: { documentId: document.id } }); - expect(amount).toBe(1); - }); - - test("should create revision on document update identical to previous autosave", async () => { - const document = await buildDocument(); - - document.title = "Changed"; - await document.save({ autosave: true }); - - document.title = "Changed"; - await document.save(); - - const amount = await Revision.count({ where: { documentId: document.id } }); - expect(amount).toBe(2); - }); - - test("should not create revision if autosave", async () => { - const document = await buildDocument(); - const amount = await Revision.count({ where: { documentId: document.id } }); - expect(amount).toBe(1); - }); -}); - describe("#getSummary", () => { test("should strip markdown", async () => { const document = await buildDocument({ diff --git a/server/models/Event.js b/server/models/Event.js index 7c1cfdc67..d5421e3a6 100644 --- a/server/models/Event.js +++ b/server/models/Event.js @@ -48,6 +48,12 @@ Event.afterCreate((event) => { events.add(event, { removeOnComplete: true }); }); +// add can be used to send events into the event system without recording them +// in the database / audit trail +Event.add = (event) => { + events.add(Event.build(event), { removeOnComplete: true }); +}; + Event.ACTIVITY_EVENTS = [ "users.create", "documents.publish", diff --git a/server/models/Revision.js b/server/models/Revision.js index 46ad2cc57..707be02cd 100644 --- a/server/models/Revision.js +++ b/server/models/Revision.js @@ -49,6 +49,21 @@ Revision.findLatest = function (documentId) { }); }; +Revision.createFromDocument = function (document) { + return Revision.create({ + title: document.title, + text: document.text, + userId: document.lastModifiedById, + editorVersion: document.editorVersion, + version: document.version, + documentId: document.id, + + // revision time is set to the last time document was touched as this + // handler can be debounced in the case of an update + createdAt: document.updatedAt, + }); +}; + Revision.prototype.migrateVersion = function () { let migrated = false; diff --git a/server/models/Revision.test.js b/server/models/Revision.test.js index 6bab258b5..ada543b44 100644 --- a/server/models/Revision.test.js +++ b/server/models/Revision.test.js @@ -12,12 +12,15 @@ describe("#findLatest", () => { title: "Title", text: "Content", }); + await Revision.createFromDocument(document); document.title = "Changed 1"; await document.save(); + await Revision.createFromDocument(document); document.title = "Changed 2"; await document.save(); + await Revision.createFromDocument(document); const revision = await Revision.findLatest(document.id); diff --git a/server/services/backlinks.js b/server/services/backlinks.js index 56547b9a3..74c915b5d 100644 --- a/server/services/backlinks.js +++ b/server/services/backlinks.js @@ -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; diff --git a/server/services/backlinks.test.js b/server/services/backlinks.test.js index 4335bb35e..83c09f670 100644 --- a/server/services/backlinks.test.js +++ b/server/services/backlinks.test.js @@ -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(); diff --git a/server/services/debouncer.js b/server/services/debouncer.js new file mode 100644 index 000000000..cf6cf07be --- /dev/null +++ b/server/services/debouncer.js @@ -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: + } + } +} diff --git a/server/services/notifications.js b/server/services/notifications.js index d8e11e512..c37aa6f32 100644 --- a/server/services/notifications.js +++ b/server/services/notifications.js @@ -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, { diff --git a/server/services/notifications.test.js b/server/services/notifications.test.js new file mode 100644 index 000000000..1a1cb3863 --- /dev/null +++ b/server/services/notifications.test.js @@ -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(); + }); +}); diff --git a/server/services/revisions.js b/server/services/revisions.js new file mode 100644 index 000000000..6508d02eb --- /dev/null +++ b/server/services/revisions.js @@ -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: + } + } +} diff --git a/server/services/revisions.test.js b/server/services/revisions.test.js new file mode 100644 index 000000000..7723cde64 --- /dev/null +++ b/server/services/revisions.test.js @@ -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); + }); +}); diff --git a/server/services/slack.js b/server/services/slack.js index fb012b459..3b552eb6c 100644 --- a/server/services/slack.js +++ b/server/services/slack.js @@ -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, {