Comment notification emails (#4978)

* Comment notification emails

* fix links
fix threading in email inboxes
from is now commenter name

* fix

* refactor

* fix async filter

server/emails/mailer.tsx

@@ -1,3 +1,5 @@
+import addressparser from "addressparser";
+import invariant from "invariant";
 import nodemailer, { Transporter } from "nodemailer";
 import SMTPTransport from "nodemailer/lib/smtp-transport";
 import Oy from "oy-vey";
@@ -11,6 +13,7 @@ const useTestEmailService =
 
 type SendMailOptions = {
   to: string;
+  fromName?: string;
   replyTo?: string;
   subject: string;
   previewText?: string;
@@ -71,8 +74,21 @@ export class Mailer {
 
     try {
       Logger.info("email", `Sending email "${data.subject}" to ${data.to}`);
+
+      invariant(
+        env.SMTP_FROM_EMAIL,
+        "SMTP_FROM_EMAIL is required to send emails"
+      );
+
+      const from = addressparser(env.SMTP_FROM_EMAIL)[0];
+
       const info = await transporter.sendMail({
-        from: env.SMTP_FROM_EMAIL,
+        from: data.fromName
+          ? {
+              name: data.fromName,
+              address: from.address,
+            }
+          : env.SMTP_FROM_EMAIL,
         replyTo: data.replyTo ?? env.SMTP_REPLY_EMAIL ?? env.SMTP_FROM_EMAIL,
         to: data.to,
         subject: data.subject,

server/emails/templates/BaseEmail.tsx

@@ -18,6 +18,7 @@ export default abstract class BaseEmail<T extends EmailProps, S = unknown> {
    * Schedule this email type to be sent asyncronously by a worker.
    *
    * @param props Properties to be used in the email template
+   * @param metadata Optional metadata to be stored with the notification
    * @returns A promise that resolves once the email is placed on the task queue
    */
   public static schedule<T>(props: T, metadata?: NotificationMetadata) {
@@ -77,6 +78,7 @@ export default abstract class BaseEmail<T extends EmailProps, S = unknown> {
     try {
       await mailer.sendMail({
         to: this.props.to,
+        fromName: this.fromName?.(data),
         subject: this.subject(data),
         previewText: this.preview(data),
         component: this.render(data),
@@ -163,4 +165,9 @@ export default abstract class BaseEmail<T extends EmailProps, S = unknown> {
    * @returns A promise resolving to additional data
    */
   protected beforeSend?(props: T): Promise<S | false>;
+
+  /**
+   * fromName hook allows overriding the "from" name of the email.
+   */
+  protected fromName?(props: T): string | undefined;
 }

server/emails/templates/CommentCreatedEmail.tsx

@@ -0,0 +1,142 @@
+import inlineCss from "inline-css";
+import * as React from "react";
+import env from "@server/env";
+import { Comment, 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";
+import Header from "./components/Header";
+import Heading from "./components/Heading";
+
+type InputProps = {
+  to: string;
+  documentId: string;
+  actorName: string;
+  isReply: boolean;
+  commentId: string;
+  collectionName: string;
+  teamUrl: string;
+  unsubscribeUrl: string;
+  content: string;
+};
+
+type BeforeSend = {
+  document: Document;
+  body: string | undefined;
+  isFirstComment: boolean;
+};
+
+type Props = InputProps & BeforeSend;
+
+/**
+ * Email sent to a user when they are subscribed to a document and a new comment
+ * is created.
+ */
+export default class CommentCreatedEmail extends BaseEmail<
+  InputProps,
+  BeforeSend
+> {
+  protected async beforeSend({ documentId, commentId, content }: InputProps) {
+    const document = await Document.unscoped().findByPk(documentId);
+    if (!document) {
+      return false;
+    }
+
+    const firstComment = await Comment.findOne({
+      attributes: ["id"],
+      where: { documentId },
+      order: [["createdAt", "ASC"]],
+    });
+    const isFirstComment = firstComment?.id === commentId;
+
+    // inline all css so that it works in as many email providers as possible.
+    let body;
+    if (content) {
+      body = await inlineCss(content, {
+        url: env.URL,
+        applyStyleTags: true,
+        applyLinkTags: false,
+        removeStyleTags: true,
+      });
+    }
+
+    return { document, isFirstComment, body };
+  }
+
+  protected subject({ isFirstComment, document }: Props) {
+    return `${isFirstComment ? "" : "Re: "}New comment on “${document.title}”`;
+  }
+
+  protected preview({ isReply, actorName }: Props): string {
+    return isReply
+      ? `${actorName} replied in a thread`
+      : `${actorName} commented on the document`;
+  }
+
+  protected fromName({ actorName }: Props): string {
+    return actorName;
+  }
+
+  protected renderAsText({
+    actorName,
+    teamUrl,
+    isReply,
+    document,
+    commentId,
+    collectionName,
+  }: Props): string {
+    return `
+${actorName} ${isReply ? "replied in" : "commented on"} the document "${
+      document.title
+    }", in the ${collectionName} collection.
+
+Open Thread: ${teamUrl}${document.url}?commentId=${commentId}
+`;
+  }
+
+  protected render({
+    document,
+    actorName,
+    isReply,
+    collectionName,
+    teamUrl,
+    commentId,
+    unsubscribeUrl,
+    body,
+  }: Props) {
+    const link = `${teamUrl}${document.url}?commentId=${commentId}&ref=notification-email`;
+
+    return (
+      <EmailTemplate>
+        <Header />
+
+        <Body>
+          <Heading>{document.title}</Heading>
+          <p>
+            {actorName} {isReply ? "replied in" : "commented on"} the document{" "}
+            <a href={link}>{document.title}</a>, in the {collectionName}{" "}
+            collection.
+          </p>
+          {body && (
+            <>
+              <EmptySpace height={20} />
+              <Diff>
+                <div dangerouslySetInnerHTML={{ __html: body }} />
+              </Diff>
+              <EmptySpace height={20} />
+            </>
+          )}
+          <p>
+            <Button href={link}>Open Thread</Button>
+          </p>
+        </Body>
+
+        <Footer unsubscribeUrl={unsubscribeUrl} />
+      </EmailTemplate>
+    );
+  }
+}

server/models/helpers/DocumentHelper.tsx

@@ -4,25 +4,17 @@ import {
 } from "@getoutline/y-prosemirror";
 import { JSDOM } from "jsdom";
 import { escapeRegExp, startCase } from "lodash";
-import { Node, DOMSerializer } from "prosemirror-model";
-import * as React from "react";
-import { renderToString } from "react-dom/server";
-import styled, { ServerStyleSheet, ThemeProvider } from "styled-components";
+import { Node } from "prosemirror-model";
 import * as Y from "yjs";
-import EditorContainer from "@shared/editor/components/Styles";
 import textBetween from "@shared/editor/lib/textBetween";
-import GlobalStyles from "@shared/styles/globals";
-import light from "@shared/styles/theme";
 import {
   getCurrentDateAsString,
   getCurrentDateTimeAsString,
   getCurrentTimeAsString,
   unicodeCLDRtoBCP47,
 } from "@shared/utils/date";
-import { isRTL } from "@shared/utils/rtl";
 import unescape from "@shared/utils/unescape";
 import { parser, schema } from "@server/editor";
-import Logger from "@server/logging/Logger";
 import { trace } from "@server/logging/tracing";
 import type Document from "@server/models/Document";
 import type Revision from "@server/models/Revision";
@@ -31,6 +23,7 @@ import diff from "@server/utils/diff";
 import parseAttachmentIds from "@server/utils/parseAttachmentIds";
 import { getSignedUrl } from "@server/utils/s3";
 import Attachment from "../Attachment";
+import ProsemirrorHelper from "./ProsemirrorHelper";
 
 type HTMLOptions = {
   /** Whether to include the document title in the generated HTML (defaults to true) */
@@ -39,8 +32,11 @@ type HTMLOptions = {
   includeStyles?: boolean;
   /** Whether to include styles to center diff (defaults to true) */
   centered?: boolean;
-  /** Whether to replace attachment urls with pre-signed versions (defaults to false) */
-  signedUrls?: boolean;
+  /**
+   * Whether to replace attachment urls with pre-signed versions. If set to a
+   * number then the urls will be signed for that many seconds. (defaults to false)
+   */
+  signedUrls?: boolean | number;
 };
 
 @trace()
@@ -106,87 +102,17 @@ export default class DocumentHelper {
    */
   static async toHTML(document: Document | Revision, options?: HTMLOptions) {
     const node = DocumentHelper.toProsemirror(document);
-    const sheet = new ServerStyleSheet();
-    let html, styleTags;
-
-    const Centered = options?.centered
-      ? styled.article`
-          max-width: 46em;
-          margin: 0 auto;
-          padding: 0 1em;
-        `
-      : "article";
-
-    const rtl = isRTL(document.title);
-    const content = <div id="content" className="ProseMirror"></div>;
-    const children = (
-      <>
-        {options?.includeTitle !== false && (
-          <h1 dir={rtl ? "rtl" : "ltr"}>{document.title}</h1>
-        )}
-        {options?.includeStyles !== false ? (
-          <EditorContainer dir={rtl ? "rtl" : "ltr"} rtl={rtl}>
-            {content}
-          </EditorContainer>
-        ) : (
-          content
-        )}
-      </>
-    );
-
-    // First render the containing document which has all the editor styles,
-    // global styles, layout and title.
-    try {
-      html = renderToString(
-        sheet.collectStyles(
-          <ThemeProvider theme={light}>
-            <>
-              {options?.includeStyles === false ? (
-                <article>{children}</article>
-              ) : (
-                <>
-                  <GlobalStyles />
-                  <Centered>{children}</Centered>
-                </>
-              )}
-            </>
-          </ThemeProvider>
-        )
-      );
-      styleTags = sheet.getStyleTags();
-    } catch (error) {
-      Logger.error("Failed to render styles on document export", error, {
-        id: document.id,
-      });
-    } finally {
-      sheet.seal();
-    }
-
-    // Render the Prosemirror document using virtual DOM and serialize the
-    // result to a string
-    const dom = new JSDOM(
-      `<!DOCTYPE html>${
-        options?.includeStyles === false ? "" : styleTags
-      }${html}`
-    );
-    const doc = dom.window.document;
-    const target = doc.getElementById("content");
-
-    DOMSerializer.fromSchema(schema).serializeFragment(
-      node.content,
-      {
-        document: doc,
-      },
-      // @ts-expect-error incorrect library type, third argument is target node
-      target
-    );
-
-    let output = dom.serialize();
+    let output = ProsemirrorHelper.toHTML(node, {
+      title: options?.includeTitle !== false ? document.title : undefined,
+      includeStyles: options?.includeStyles,
+      centered: options?.centered,
+    });
 
     if (options?.signedUrls && "teamId" in document) {
       output = await DocumentHelper.attachmentsToSignedUrls(
         output,
-        document.teamId
+        document.teamId,
+        typeof options.signedUrls === "number" ? options.signedUrls : undefined
       );
     }
 

server/models/helpers/NotificationHelper.ts

@@ -0,0 +1,142 @@
+import { uniqBy } from "lodash";
+import { Op } from "sequelize";
+import {
+  Document,
+  Collection,
+  NotificationSetting,
+  Subscription,
+  Comment,
+} from "@server/models";
+
+export default class NotificationHelper {
+  /**
+   * Get the recipients of a notification for a collection event.
+   *
+   * @param collection The collection to get recipients for
+   * @param eventName The event name
+   * @returns A list of recipients
+   */
+  public static getCollectionNotificationRecipients = async (
+    collection: Collection,
+    eventName: string
+  ): Promise<NotificationSetting[]> => {
+    // First find all the users that have notifications enabled for this event
+    // type at all and aren't the one that performed the action.
+    const recipients = await NotificationSetting.scope("withUser").findAll({
+      where: {
+        userId: {
+          [Op.ne]: collection.createdById,
+        },
+        teamId: collection.teamId,
+        event: eventName,
+      },
+    });
+
+    // Ensure we only have one recipient per user as a safety measure
+    return uniqBy(recipients, "userId");
+  };
+
+  /**
+   * Get the recipients of a notification for a comment event.
+   *
+   * @param document The document associated with the comment
+   * @param comment The comment to get recipients for
+   * @param eventName The event name
+   * @returns A list of recipients
+   */
+  public static getCommentNotificationRecipients = async (
+    document: Document,
+    comment: Comment,
+    actorId: string
+  ): Promise<NotificationSetting[]> => {
+    const recipients = await this.getDocumentNotificationRecipients(
+      document,
+      "documents.update",
+      actorId
+    );
+
+    if (recipients.length > 0 && comment.parentCommentId) {
+      const contextComments = await Comment.findAll({
+        attributes: ["createdById"],
+        where: {
+          [Op.or]: [
+            { id: comment.parentCommentId },
+            { parentCommentId: comment.parentCommentId },
+          ],
+        },
+      });
+
+      const userIdsInThread = contextComments.map((c) => c.createdById);
+      return recipients.filter((r) => userIdsInThread.includes(r.userId));
+    }
+
+    return recipients;
+  };
+
+  /**
+   * Get the recipients of a notification for a document event.
+   *
+   * @param document The document to get recipients for
+   * @param eventName The event name
+   * @param actorId The id of the user that performed the action
+   * @returns A list of recipients
+   */
+  public static getDocumentNotificationRecipients = async (
+    document: Document,
+    eventName: string,
+    actorId: string
+  ): Promise<NotificationSetting[]> => {
+    // First find all the users that have notifications enabled for this event
+    // type at all and aren't the one that performed the action.
+    let recipients = await NotificationSetting.scope("withUser").findAll({
+      where: {
+        userId: {
+          [Op.ne]: actorId,
+        },
+        teamId: document.teamId,
+        event: eventName,
+      },
+    });
+
+    // If the event is a revision creation we can filter further to only those
+    // that have a subscription to the document…
+    if (eventName === "documents.update") {
+      const subscriptions = await Subscription.findAll({
+        attributes: ["userId"],
+        where: {
+          userId: recipients.map((recipient) => recipient.user.id),
+          documentId: document.id,
+          event: eventName,
+        },
+      });
+
+      const subscribedUserIds = subscriptions.map(
+        (subscription) => subscription.userId
+      );
+
+      recipients = recipients.filter((recipient) =>
+        subscribedUserIds.includes(recipient.user.id)
+      );
+    }
+
+    const filtered = [];
+
+    for (const recipient of recipients) {
+      const collectionIds = await recipient.user.collectionIds();
+
+      // Check the recipient has access to the collection this document is in. Just
+      // because they are subscribed doesn't meant they still have access to read
+      // the document.
+      if (
+        recipient.user.email &&
+        !recipient.user.isSuspended &&
+        collectionIds.includes(document.collectionId)
+      ) {
+        filtered.push(recipient);
+      }
+    }
+
+    // Ensure we only have one recipient per user as a safety measure
+    return uniqBy(filtered, "userId");
+  };
+}

server/models/helpers/ProsemirrorHelper.tsx

@@ -0,0 +1,107 @@
+import { JSDOM } from "jsdom";
+import { Node, DOMSerializer } from "prosemirror-model";
+import * as React from "react";
+import { renderToString } from "react-dom/server";
+import styled, { ServerStyleSheet, ThemeProvider } from "styled-components";
+import EditorContainer from "@shared/editor/components/Styles";
+import GlobalStyles from "@shared/styles/globals";
+import light from "@shared/styles/theme";
+import { isRTL } from "@shared/utils/rtl";
+import { schema } from "@server/editor";
+import Logger from "@server/logging/Logger";
+import { trace } from "@server/logging/tracing";
+
+export type HTMLOptions = {
+  /** A title, if it should be included */
+  title?: string;
+  /** 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;
+};
+
+@trace()
+export default class ProsemirrorHelper {
+  /**
+   * Returns the node as HTML. This is a lossy conversion and should only be used
+   * for export.
+   *
+   * @param node The node to convert to HTML
+   * @param options Options for the HTML output
+   * @returns The content as a HTML string
+   */
+  static toHTML(node: Node, options?: HTMLOptions) {
+    const sheet = new ServerStyleSheet();
+    let html, styleTags;
+
+    const Centered = options?.centered
+      ? styled.article`
+          max-width: 46em;
+          margin: 0 auto;
+          padding: 0 1em;
+        `
+      : "article";
+
+    const rtl = isRTL(node.textContent);
+    const content = <div id="content" className="ProseMirror"></div>;
+    const children = (
+      <>
+        {options?.title && <h1 dir={rtl ? "rtl" : "ltr"}>{options.title}</h1>}
+        {options?.includeStyles !== false ? (
+          <EditorContainer dir={rtl ? "rtl" : "ltr"} rtl={rtl}>
+            {content}
+          </EditorContainer>
+        ) : (
+          content
+        )}
+      </>
+    );
+
+    // First render the containing document which has all the editor styles,
+    // global styles, layout and title.
+    try {
+      html = renderToString(
+        sheet.collectStyles(
+          <ThemeProvider theme={light}>
+            <>
+              {options?.includeStyles === false ? (
+                <article>{children}</article>
+              ) : (
+                <>
+                  <GlobalStyles />
+                  <Centered>{children}</Centered>
+                </>
+              )}
+            </>
+          </ThemeProvider>
+        )
+      );
+      styleTags = sheet.getStyleTags();
+    } catch (error) {
+      Logger.error("Failed to render styles on node HTML conversion", error);
+    } finally {
+      sheet.seal();
+    }
+
+    // Render the Prosemirror document using virtual DOM and serialize the
+    // result to a string
+    const dom = new JSDOM(
+      `<!DOCTYPE html>${
+        options?.includeStyles === false ? "" : styleTags
+      }${html}`
+    );
+    const doc = dom.window.document;
+    const target = doc.getElementById("content");
+
+    DOMSerializer.fromSchema(schema).serializeFragment(
+      node.content,
+      {
+        document: doc,
+      },
+      // @ts-expect-error incorrect library type, third argument is target node
+      target
+    );
+
+    return dom.serialize();
+  }
+}

server/queues/processors/NotificationsProcessor.ts

@@ -1,6 +1,6 @@
 import { subHours } from "date-fns";
-import { uniqBy } from "lodash";
 import { Op } from "sequelize";
+import { Minute } from "@shared/utils/time";
 import subscriptionCreator from "@server/commands/subscriptionCreator";
 import { sequelize } from "@server/database/sequelize";
 import CollectionNotificationEmail from "@server/emails/templates/CollectionNotificationEmail";
@@ -8,23 +8,24 @@ import DocumentNotificationEmail from "@server/emails/templates/DocumentNotifica
 import env from "@server/env";
 import Logger from "@server/logging/Logger";
 import {
-  View,
   Document,
   Team,
   Collection,
-  User,
-  NotificationSetting,
-  Subscription,
   Notification,
   Revision,
+  User,
+  View,
 } from "@server/models";
 import DocumentHelper from "@server/models/helpers/DocumentHelper";
+import NotificationHelper from "@server/models/helpers/NotificationHelper";
 import {
   CollectionEvent,
   RevisionEvent,
   Event,
   DocumentEvent,
+  CommentEvent,
 } from "@server/types";
+import CommentCreatedNotificationTask from "../tasks/CommentCreatedNotificationTask";
 import BaseProcessor from "./BaseProcessor";
 
 export default class NotificationsProcessor extends BaseProcessor {
@@ -32,6 +33,7 @@ export default class NotificationsProcessor extends BaseProcessor {
     "documents.publish",
     "revisions.create",
     "collections.create",
+    "comments.create",
   ];
 
   async perform(event: Event) {
@@ -42,11 +44,18 @@ export default class NotificationsProcessor extends BaseProcessor {
         return this.revisionCreated(event);
       case "collections.create":
         return this.collectionCreated(event);
-
+      case "comments.create":
+        return this.commentCreated(event);
       default:
     }
   }
 
+  async commentCreated(event: CommentEvent) {
+    await CommentCreatedNotificationTask.schedule(event, {
+      delay: Minute,
+    });
+  }
+
   async documentPublished(event: DocumentEvent) {
     // never send notifications when batch importing documents
     if (
@@ -69,9 +78,10 @@ export default class NotificationsProcessor extends BaseProcessor {
 
     await this.createDocumentSubscriptions(document, event);
 
-    const recipients = await this.getDocumentNotificationRecipients(
+    const recipients = await NotificationHelper.getDocumentNotificationRecipients(
       document,
-      "documents.publish"
+      "documents.publish",
+      document.lastModifiedById
     );
 
     for (const recipient of recipients) {
@@ -115,27 +125,26 @@ export default class NotificationsProcessor extends BaseProcessor {
 
     await this.createDocumentSubscriptions(document, event);
 
-    const recipients = await this.getDocumentNotificationRecipients(
+    const recipients = await NotificationHelper.getDocumentNotificationRecipients(
       document,
-      "documents.update"
+      "documents.update",
+      document.lastModifiedById
     );
+    if (!recipients.length) {
+      return;
+    }
 
     // generate the diff html for the email
     const before = await revision.previous();
-    let content = await DocumentHelper.toEmailDiff(before, revision, {
+    const content = await DocumentHelper.toEmailDiff(before, revision, {
       includeTitle: false,
       centered: false,
+      signedUrls: 86400 * 4,
     });
     if (!content) {
       return;
     }
 
-    content = await DocumentHelper.attachmentsToSignedUrls(
-      content,
-      event.teamId,
-      86400 * 4
-    );
-
     for (const recipient of recipients) {
       const notify = await this.shouldNotify(document, recipient.user);
 
@@ -174,7 +183,7 @@ export default class NotificationsProcessor extends BaseProcessor {
       return;
     }
 
-    const recipients = await this.getCollectionNotificationRecipients(
+    const recipients = await NotificationHelper.getCollectionNotificationRecipients(
       collection,
       event.name
     );
@@ -194,128 +203,10 @@ export default class NotificationsProcessor extends BaseProcessor {
     }
   }
 
-  /**
-   * Create any new subscriptions that might be missing for collaborators in the
-   * document on publish and revision creation. This does mean that there is a
-   * short period of time where the user is not subscribed after editing until a
-   * revision is created.
-   *
-   * @param document The document to create subscriptions for
-   * @param event The event that triggered the subscription creation
-   */
-  private createDocumentSubscriptions = async (
-    document: Document,
-    event: DocumentEvent | RevisionEvent
-  ): Promise<void> => {
-    await sequelize.transaction(async (transaction) => {
-      const users = await document.collaborators({ transaction });
-
-      for (const user of users) {
-        await subscriptionCreator({
-          user,
-          documentId: document.id,
-          event: "documents.update",
-          resubscribe: false,
-          transaction,
-          ip: event.ip,
-        });
-      }
-    });
-  };
-
-  /**
-   * Get the recipients of a notification for a collection event.
-   *
-   * @param collection The collection to get recipients for
-   * @param eventName The event name
-   * @returns A list of recipients
-   */
-  private getCollectionNotificationRecipients = async (
-    collection: Collection,
-    eventName: string
-  ): Promise<NotificationSetting[]> => {
-    // First find all the users that have notifications enabled for this event
-    // type at all and aren't the one that performed the action.
-    const recipients = await NotificationSetting.scope("withUser").findAll({
-      where: {
-        userId: {
-          [Op.ne]: collection.createdById,
-        },
-        teamId: collection.teamId,
-        event: eventName,
-      },
-    });
-
-    // Ensure we only have one recipient per user as a safety measure
-    return uniqBy(recipients, "userId");
-  };
-
-  /**
-   * Get the recipients of a notification for a document event.
-   *
-   * @param document The document to get recipients for
-   * @param eventName The event name
-   * @returns A list of recipients
-   */
-  private getDocumentNotificationRecipients = async (
-    document: Document,
-    eventName: string
-  ): Promise<NotificationSetting[]> => {
-    // First find all the users that have notifications enabled for this event
-    // type at all and aren't the one that performed the action.
-    let recipients = await NotificationSetting.scope("withUser").findAll({
-      where: {
-        userId: {
-          [Op.ne]: document.lastModifiedById,
-        },
-        teamId: document.teamId,
-        event: eventName,
-      },
-    });
-
-    // If the event is a revision creation we can filter further to only those
-    // that have a subscription to the document…
-    if (eventName === "documents.update") {
-      const subscriptions = await Subscription.findAll({
-        attributes: ["userId"],
-        where: {
-          userId: recipients.map((recipient) => recipient.user.id),
-          documentId: document.id,
-          event: eventName,
-        },
-      });
-
-      const subscribedUserIds = subscriptions.map(
-        (subscription) => subscription.userId
-      );
-
-      recipients = recipients.filter((recipient) =>
-        subscribedUserIds.includes(recipient.user.id)
-      );
-    }
-
-    // Ensure we only have one recipient per user as a safety measure
-    return uniqBy(recipients, "userId");
-  };
-
   private shouldNotify = async (
     document: Document,
     user: User
   ): Promise<boolean> => {
-    // Suppress notifications for suspended and users with no email address
-    if (user.isSuspended || !user.email) {
-      return false;
-    }
-
-    // Check the recipient has access to the collection this document is in. Just
-    // because they are subscribed doesn't meant they still have access to read
-    // the document.
-    const collectionIds = await user.collectionIds();
-
-    if (!collectionIds.includes(document.collectionId)) {
-      return false;
-    }
-
     // Deliver only a single notification in a 12 hour window
     const notification = await Notification.findOne({
       order: [["createdAt", "DESC"]],
@@ -366,4 +257,33 @@ export default class NotificationsProcessor extends BaseProcessor {
 
     return true;
   };
+
+  /**
+   * Create any new subscriptions that might be missing for collaborators in the
+   * document on publish and revision creation. This does mean that there is a
+   * short period of time where the user is not subscribed after editing until a
+   * revision is created.
+   *
+   * @param document The document to create subscriptions for
+   * @param event The event that triggered the subscription creation
+   */
+  private createDocumentSubscriptions = async (
+    document: Document,
+    event: DocumentEvent | RevisionEvent
+  ): Promise<void> => {
+    await sequelize.transaction(async (transaction) => {
+      const users = await document.collaborators({ transaction });
+
+      for (const user of users) {
+        await subscriptionCreator({
+          user,
+          documentId: document.id,
+          event: "documents.update",
+          resubscribe: false,
+          transaction,
+          ip: event.ip,
+        });
+      }
+    });
+  };
 }

server/queues/tasks/CommentCreatedNotificationTask.ts

@@ -0,0 +1,99 @@
+import { Node } from "prosemirror-model";
+import subscriptionCreator from "@server/commands/subscriptionCreator";
+import { sequelize } from "@server/database/sequelize";
+import { schema } from "@server/editor";
+import CommentCreatedEmail from "@server/emails/templates/CommentCreatedEmail";
+import { Comment, Document, Notification, Team } from "@server/models";
+import DocumentHelper from "@server/models/helpers/DocumentHelper";
+import NotificationHelper from "@server/models/helpers/NotificationHelper";
+import ProsemirrorHelper from "@server/models/helpers/ProsemirrorHelper";
+import { CommentEvent } from "@server/types";
+import BaseTask, { TaskPriority } from "./BaseTask";
+
+export default class CommentCreatedNotificationTask extends BaseTask<
+  CommentEvent
+> {
+  public async perform(event: CommentEvent) {
+    const [document, comment, team] = await Promise.all([
+      Document.scope("withCollection").findOne({
+        where: {
+          id: event.documentId,
+        },
+      }),
+      Comment.findByPk(event.modelId),
+      Team.findByPk(event.teamId),
+    ]);
+    if (!document || !comment || !team) {
+      return;
+    }
+
+    // Commenting on a doc automatically creates a subscription to the doc
+    // if they haven't previously had one.
+    await sequelize.transaction(async (transaction) => {
+      await subscriptionCreator({
+        user: comment.createdBy,
+        documentId: document.id,
+        event: "documents.update",
+        resubscribe: false,
+        transaction,
+        ip: event.ip,
+      });
+    });
+
+    const recipients = await NotificationHelper.getCommentNotificationRecipients(
+      document,
+      comment,
+      comment.createdById
+    );
+    if (!recipients.length) {
+      return;
+    }
+
+    let content = ProsemirrorHelper.toHTML(
+      Node.fromJSON(schema, comment.data),
+      {
+        centered: false,
+      }
+    );
+    if (!content) {
+      return;
+    }
+
+    content = await DocumentHelper.attachmentsToSignedUrls(
+      content,
+      event.teamId,
+      86400 * 4
+    );
+
+    for (const recipient of recipients) {
+      const notification = await Notification.create({
+        event: event.name,
+        userId: recipient.user.id,
+        actorId: comment.createdById,
+        teamId: team.id,
+        documentId: document.id,
+      });
+      await CommentCreatedEmail.schedule(
+        {
+          to: recipient.user.email,
+          documentId: document.id,
+          teamUrl: team.url,
+          isReply: !!comment.parentCommentId,
+          actorName: comment.createdBy.name,
+          commentId: comment.id,
+          content,
+          collectionName: document.collection?.name,
+          unsubscribeUrl: recipient.unsubscribeUrl,
+        },
+        { notificationId: notification.id }
+      );
+    }
+  }
+
+  public get options() {
+    return {
+      attempts: 1,
+      priority: TaskPriority.Background,
+    };
+  }
+}