[local-sync] Make EnsureMessageInSentFolder save db changes after imap success

Summary:
This will solve T7579 when saving messages to the sent folder. I
attempted to clean up the references code but decided it was better left for a
new diff, so added a bunch of TODO's in this diff

Test Plan: manual

Reviewers: halla, spang, evan

Reviewed By: spang, evan

Differential Revision: https://phab.nylas.com/D3766

packages/local-sync/src/local-sync-worker/imap-helpers.js

@@ -88,53 +88,5 @@ const IMAPHelpers = {
     const labelIdentifiers = labels.map(label => label.imapLabelIdentifier());
     return box.setLabels(messages.map(m => m.folderImapUID), labelIdentifiers)
   },
-
-  async saveSentMessage({db, imap, provider, rawMime, headerMessageId}) {
-    if (provider !== 'gmail') {
-      const sentFolder = await db.Folder.find({where: {role: 'sent'}});
-      if (!sentFolder) { throw new APIError('Could not save message to sent folder.') }
-
-      const box = await imap.openBox(sentFolder.name);
-      return box.append(rawMime, {flags: 'SEEN'});
-    }
-
-    // Gmail, we need to add the message to all mail and add the sent label
-    const sentLabel = await db.Label.find({where: {role: 'sent'}});
-    const allMail = await db.Folder.find({where: {role: 'all'}});
-    if (sentLabel && allMail) {
-      const box = await imap.openBox(allMail.name);
-      await box.append(rawMime, {flags: 'SEEN'})
-      const uids = await box.search([['HEADER', 'Message-ID', headerMessageId]])
-      // There should only be one uid in the array
-      return box.setLabels(uids[0], '\\Sent');
-    }
-
-    throw new APIError('Could not save message to sent folder.')
-  },
-
-  async deleteGmailSentMessages({db, imap, provider, headerMessageId}) {
-    if (provider !== 'gmail') { return }
-
-    const trash = await db.Folder.find({where: {role: 'trash'}});
-    if (!trash) { throw new APIError(`Could not find folder with role 'trash'.`) }
-
-    const allMail = await db.Folder.find({where: {role: 'all'}});
-    if (!allMail) { throw new APIError(`Could not find folder with role 'all'.`) }
-
-    // Move the message from all mail to trash and then delete it from there
-    const steps = [
-      {folder: allMail, deleteFn: (box, uid) => box.moveFromBox(uid, trash.name)},
-      {folder: trash, deleteFn: (box, uid) => box.addFlags(uid, 'DELETED')},
-    ]
-
-    for (const {folder, deleteFn} of steps) {
-      const box = await imap.openBox(folder.name);
-      const uids = await box.search([['HEADER', 'Message-ID', headerMessageId]])
-      for (const uid of uids) {
-        await deleteFn(box, uid);
-      }
-      await box.closeBox();
-    }
-  },
 }
 module.exports = IMAPHelpers

packages/local-sync/src/local-sync-worker/syncback-tasks/ensure-message-in-sent-folder.imap.js

@@ -1,8 +1,159 @@
 const {SendmailClient, Provider, Errors: {APIError}} = require('isomorphic-core')
-const IMAPHelpers = require('../imap-helpers')
 const SyncbackTask = require('./syncback-task')
 const {getReplyHeaders} = require('../../shared/message-factory')
 
+
+async function deleteGmailSentMessages({db, imap, provider, headerMessageId}) {
+  if (provider !== 'gmail') { return }
+
+  const trash = await db.Folder.find({where: {role: 'trash'}});
+  if (!trash) { throw new APIError(`Could not find folder with role 'trash'.`) }
+
+  const allMail = await db.Folder.find({where: {role: 'all'}});
+  if (!allMail) { throw new APIError(`Could not find folder with role 'all'.`) }
+
+  // Move the message from all mail to trash and then delete it from there
+  const steps = [
+    {folder: allMail, deleteFn: (box, uid) => box.moveFromBox(uid, trash.name)},
+    {folder: trash, deleteFn: (box, uid) => box.addFlags(uid, 'DELETED')},
+  ]
+
+  for (const {folder, deleteFn} of steps) {
+    const box = await imap.openBox(folder.name);
+    const uids = await box.search([['HEADER', 'Message-ID', headerMessageId]])
+    for (const uid of uids) {
+      await deleteFn(box, uid);
+    }
+    await box.closeBox();
+  }
+}
+
+async function saveSentMessage({db, account, logger, imap, provider, sentPerRecipient, baseMessage}) {
+  const {sequelize, Folder, Label} = db
+  const thread = await baseMessage.getThread({
+    include: [{model: Folder, as: 'folders'}, {model: Label, as: 'labels'}],
+  })
+
+  // Case 1. If non gmail, save the message to the `sent` folder using IMAP
+  // Only gmail creates a sent message for us, so if we are using any other provider
+  // we need to save it manually ourselves.
+  if (provider !== 'gmail') {
+    const sentFolder = await Folder.find({where: {role: 'sent'}});
+    if (!sentFolder) { throw new APIError(`Can't find sent folder - could not save message to sent folder.`) }
+
+    const sender = new SendmailClient(account, logger);
+    const rawMime = await sender.buildMime(baseMessage);
+    const box = await imap.openBox(sentFolder.name);
+    await box.append(rawMime, {flags: 'SEEN'});
+
+    // Finally, if IMAP succeeds, save the model updates
+    await sequelize.transaction(async (transaction) => {
+      await baseMessage.setFolder(sentFolder, {transaction})
+      await baseMessage.save({transaction})
+      baseMessage.folder = sentFolder
+      if (thread) {
+        await thread.updateFromMessages({messages: [baseMessage], transaction})
+      }
+    })
+    return
+  }
+
+
+  // Showing as sent in gmail means adding the message to all mail and
+  // adding the sent label
+  const sentLabel = await Label.find({where: {role: 'sent'}});
+  const allMailFolder = await Folder.find({where: {role: 'all'}});
+  if (!sentLabel || !allMailFolder) {
+    throw new APIError('Could not save message to sent folder.')
+  }
+
+
+  // Case 2. If gmail, even though gmail saves sent messages automatically,
+  // if `sentPerRecipient` is true, it means we want to save the `baseMessage`
+  // as sent. This is because that means that we sent a message per recipient for
+  // tracking, but we actually /just/ want to show the baseMessage as sent
+  if (sentPerRecipient) {
+    const sender = new SendmailClient(account, logger);
+    const rawMime = await sender.buildMime(baseMessage);
+    const box = await imap.openBox(allMailFolder.name);
+
+    await box.append(rawMime, {flags: 'SEEN'})
+
+    const {headerMessageId} = baseMessage
+    const uids = await box.search([['HEADER', 'Message-ID', headerMessageId]])
+    // There should only be one uid in the array
+    await box.setLabels(uids[0], '\\Sent');
+  }
+
+  // Finally, if IMAP succeeds, save the model updates
+  await sequelize.transaction(async (transaction) => {
+    await baseMessage.setFolder(allMailFolder, {transaction})
+    await baseMessage.setLabels([sentLabel], {transaction})
+    await baseMessage.save({transaction})
+    baseMessage.folder = allMailFolder
+    baseMessage.labels = [sentLabel]
+    if (thread) {
+      await thread.updateFromMessages({messages: [baseMessage], transaction})
+    }
+  })
+}
+
+async function setThreadingReferences(db, baseMessage) {
+  const {Message, Reference} = db
+  // TODO When the message was created for sending, we set the
+  // `inReplyToLocalMessageId` if it exists, and we set the temporary properties
+  // `inReplyTo` and `references` for sending.
+  // Since these properties aren't saved to the model, we need to recreate
+  // them again because they are necessary for building the correct raw mime
+  // message to add to the sent folder
+  // We should clean this up
+  const replyToMessage = await Message.findById(
+    baseMessage.inReplyToLocalMessageId,
+    { include: [{model: Reference, as: 'references', attributes: ['id', 'rfc2822MessageId']}] }
+  )
+  if (replyToMessage) {
+    const {inReplyTo, references} = getReplyHeaders(replyToMessage);
+    baseMessage.inReplyTo = inReplyTo;
+    baseMessage.references = references;
+  }
+  // TODO If the thread exists, let's also optimistically create any Reference
+  // objects (these will also be detected when the message gets detected in
+  // the sync loop)
+  // We should clean this code, its duplicated from the message processor
+  const thread = await baseMessage.getThread()
+  if (thread) {
+    const references = Array.from(new Set([
+      ...baseMessage.references || [],
+      baseMessage.headerMessageId,
+      baseMessage.inReplyTo,
+    ]))
+
+    let existingReferences = [];
+    if (references.length > 0) {
+      existingReferences = await Reference.findAll({
+        where: {
+          rfc2822MessageId: baseMessage.references,
+        },
+      });
+    }
+
+    const refByMessageId = {};
+    for (const ref of existingReferences) {
+      refByMessageId[ref.rfc2822MessageId] = ref;
+    }
+    for (const mid of baseMessage.references) {
+      if (!refByMessageId[mid]) {
+        refByMessageId[mid] = await Reference.create({rfc2822MessageId: mid, threadId: thread.id});
+      }
+    }
+
+    const referencesInstances = baseMessage.references.map(mid => refByMessageId[mid]);
+    await baseMessage.addReferences(referencesInstances);
+    baseMessage.referencesOrder = referencesInstances.map(ref => ref.id);
+    await thread.addReferences(referencesInstances);
+  }
+}
+
 /**
  * Ensures that sent messages show up in the sent folder.
  *
@@ -25,33 +176,22 @@ class EnsureMessageInSentFolderIMAP extends SyncbackTask {
   }
 
   async run(db, imap) {
-    const {Message, Reference} = db
+    const {Message} = db
     const {messageId, sentPerRecipient} = this.syncbackRequestObject().props
     const {account, logger} = imap
     if (!account) {
       throw new APIError('EnsureMessageInSentFolder: Failed, account not available on imap connection')
     }
 
-    const baseMessage = await Message.findById(messageId,
+    const baseMessage = await Message.findById(messageId, {
-      {include: [{model: db.Folder}, {model: db.Label}, {model: db.File}]});
+      include: [{model: db.Folder}, {model: db.Label}, {model: db.File}],
+    });
 
     if (!baseMessage) {
       throw new APIError(`Couldn't find message ${messageId} to stuff in sent folder`, 500)
     }
 
-    // Since we store References in a separate table for indexing and don't
+    await setThreadingReferences(db, baseMessage)
-    // create these objects until the sent message is picked up via the
-    // account's sync (since that's when we create the Thread object and the
-    // references must be linked to the thread), we have to reconstruct the
-    // threading headers here before saving the message to the Sent folder.
-    const replyToMessage = await Message.findById(
-      baseMessage.inReplyToLocalMessageId,
-      { include: [{model: Reference, as: 'references', attributes: ['id', 'rfc2822MessageId']}] });
-    if (replyToMessage) {
-      const {inReplyTo, references} = getReplyHeaders(replyToMessage);
-      baseMessage.inReplyTo = inReplyTo;
-      baseMessage.references = references;
-    }
 
     const {provider} = account
     const {headerMessageId} = baseMessage
@@ -65,7 +205,7 @@ class EnsureMessageInSentFolderIMAP extends SyncbackTask {
     // sent messages and clean them up
     if (sentPerRecipient && provider === Provider.Gmail) {
       try {
-        await IMAPHelpers.deleteGmailSentMessages({db, imap, provider, headerMessageId})
+        await deleteGmailSentMessages({db, imap, provider, headerMessageId})
       } catch (err) {
         // Even if this fails, we need to finish attempting to save the
         // baseMessage to the sent folder
@@ -73,19 +213,7 @@ class EnsureMessageInSentFolderIMAP extends SyncbackTask {
       }
     }
 
-    /**
+    await saveSentMessage({db, account, logger, imap, provider, sentPerRecipient, baseMessage})
-     * If we've sentPerRecipient that means we need to always re-add the
-     * sent base message.
-     *
-     * Only gmail optimistically creates a sent message for us. We need to
-     * to it manually for all other providers
-     */
-    if (provider !== 'gmail' || sentPerRecipient) {
-      const sender = new SendmailClient(account, logger);
-      const rawMime = await sender.buildMime(baseMessage);
-      await IMAPHelpers.saveSentMessage({db, imap, provider, rawMime, headerMessageId})
-    }
-
     return baseMessage.toJSON()
   }
 }

packages/local-sync/src/message-processor/index.js

@@ -139,7 +139,7 @@ class MessageProcessor {
       console.log(`🔃 ✉️ "${messageValues.subject}" - ${messageValues.date}`)
       return processedMessage
     } catch (err) {
-      console.error(`FetchMessagesInFolder: Could not build message`, {
+      console.error(`MessageProcessor: Could not build message`, {
         err,
         imapMessage,
         desiredParts,
@@ -191,9 +191,9 @@ class MessageProcessor {
     }
 
     const referencesInstances = references.map(mid => refByMessageId[mid]);
-    message.addReferences(referencesInstances);
+    await message.addReferences(referencesInstances);
     message.referencesOrder = referencesInstances.map(ref => ref.id);
-    thread.addReferences(referencesInstances);
+    await thread.addReferences(referencesInstances);
   }
 
   async _processNewMessage(messageValues, struct) {

packages/local-sync/src/models/thread.js

@@ -66,10 +66,11 @@ module.exports = (sequelize, Sequelize) => {
 
         return this.save();
       },
+
       // Updates the attributes that don't require an external set to prevent
       // duplicates. Currently includes starred/unread counts, various date
       // values, and snippet. Does not save the thread.
-      async _updateSimpleMessageAttributes(message) {
+      _updateSimpleMessageAttributes(message) {
         // Update starred/unread counts
         this.starredCount += message.starred ? 1 : 0;
         this.unreadCount += message.unread ? 1 : 0;
@@ -96,13 +97,16 @@ module.exports = (sequelize, Sequelize) => {
           this.lastMessageReceivedDate = message.date;
         }
       },
-      async updateFromMessages({messages, recompute, db} = {}) {
+
+      async updateFromMessages({db, messages, recompute, transaction} = {}) {
+        if (!(messages instanceof Array)) {
+          throw new Error('Thread.updateFromMessages() expected an array of messages')
+        }
         if (!(this.folders instanceof Array) || !(this.labels instanceof Array)) {
           throw new Error('Thread.updateFromMessages() expected .folders and .labels to be inflated arrays')
         }
 
         let _messages = messages;
-        let threadMessageIds;
         if (recompute) {
           if (!db) {
             throw new Error('Cannot recompute thread attributes without a database reference.')
@@ -115,7 +119,6 @@ module.exports = (sequelize, Sequelize) => {
           if (_messages.length === 0) {
             return this.destroy();
           }
-          threadMessageIds = new Set(_messages.map(m => m.id))
 
           this.folders = [];
           this.labels = [];
@@ -128,9 +131,6 @@ module.exports = (sequelize, Sequelize) => {
           this.firstMessageDate = null;
           this.lastMessageSentDate = null;
           this.lastMessageReceivedDate = null;
-        } else {
-          const threadMessages = await this.getMessages({attributes: ['id']})
-          threadMessageIds = new Set(threadMessages.map(m => m.id))
         }
 
         const folders = new Set(this.folders);
@@ -170,13 +170,14 @@ module.exports = (sequelize, Sequelize) => {
         // Setting folders and labels cannot be done on a thread without an id
         let thread = this;
         if (!this.id) {
-          thread = await this.save();
+          thread = await this.save({transaction});
         }
 
-        thread.setFolders(Array.from(folders))
+        await thread.setFolders(Array.from(folders), {transaction})
-        thread.setLabels(Array.from(labels))
+        await thread.setLabels(Array.from(labels), {transaction})
-        return thread.save();
+        return thread.save({transaction});
       },
+
       toJSON() {
         if (!(this.labels instanceof Array)) {
           throw new Error("Thread.toJSON called on a thread where labels were not eagerly loaded.")

packages/local-sync/src/shared/message-factory.js

@@ -282,8 +282,11 @@ async function parseFromImap(imapMessage, desiredParts, {db, accountId, folder})
     // separately, and can simply associate them all in the same way.
     // Generally, References already contains the Message-IDs in In-Reply-To,
     // but we concat and dedupe just in case.
-    references: parseReferences((parsedHeaders.references || []).concat(
+    references: parseReferences(
-      (parsedHeaders['in-reply-to'] || []), (parsedHeaders['message-id'] || []))),
+      (parsedHeaders.references || []).concat(
+        (parsedHeaders['in-reply-to'] || []), (parsedHeaders['message-id'] || [])
+      )
+    ),
     gMsgId: parsedHeaders['x-gm-msgid'],
     gThrId: parsedHeaders['x-gm-thrid'],
     subject: parsedHeaders.subject ? parsedHeaders.subject[0] : '(no subject)',
@@ -332,8 +335,10 @@ async function buildForSend(db, json) {
   }
 
   if (json.reply_to_message_id != null) {
-    replyToMessage = await Message.findById(json.reply_to_message_id,
+    replyToMessage = await Message.findById(
-      { include: [{model: Reference, as: 'references', attributes: ['id', 'rfc2822MessageId']}] });
+      json.reply_to_message_id,
+      { include: [{model: Reference, as: 'references', attributes: ['id', 'rfc2822MessageId']}] }
+    )
   }
 
   if (replyToThread && replyToMessage) {
@@ -390,8 +395,11 @@ async function buildForSend(db, json) {
   message.id = Message.hash(messageForHashing)
   message.body = replaceMessageIdInBodyTrackingLinks(message.id, message.body)
   const instance = Message.build(message)
-  // we don't store these fields in the db, so if we set them on `message`
+
-  // beforehand Message.build() would drop them---this is just a convenience
+  // TODO we set these temporary properties which aren't stored in the database
+  // model because SendmailClient requires them to send the message with the
+  // correct headers.
+  // This should be cleaned up
   instance.inReplyTo = inReplyTo;
   instance.references = references;
   return instance;