patch(save): Only save drafts when necessary, avoid sync-engine issues

Summary:
Previously, we have saved drafts back to the user's provider through the sync engine. There are a handful of very serious edge case issues we're working to solve that are creating a bad user experience. (#933, #1175, #1504, #1237)

For now, we're going to change the behavior of N1 to mitagate these issues.

- If you create a draft in N1, we will not sync it to other mail clients while you're working on it.
- If you enable send later, we'll start syncing the draft to the server as before.
- If you created the draft in another client, we'll sync the draft to the server as before.

Fix specs

Test Plan: Run specs

Reviewers: evan, juan

Reviewed By: juan

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

internal_packages/send-later/lib/send-later-store.js

@@ -1,6 +1,12 @@
 /** @babel */
 import NylasStore from 'nylas-store'
-import {NylasAPI, Actions, Message, DatabaseStore} from 'nylas-exports'
+import {
+  NylasAPI,
+  Actions,
+  Message,
+  DatabaseStore,
+  SyncbackDraftTask,
+} from 'nylas-exports'
 import SendLaterActions from './send-later-actions'
 import {PLUGIN_ID, PLUGIN_NAME} from './send-later-constants'
 
@@ -28,6 +34,9 @@ class SendLaterStore extends NylasStore {
       return NylasAPI.authPlugin(this.pluginId, this.pluginName, accountId)
       .then(()=> {
         Actions.setMetadata(messages, this.pluginId, metadata);
+
+        // Important: Do not remove this unless N1 is syncing drafts by default.
+        Actions.queueTask(new SyncbackDraftTask(draftClientId));
       })
       .catch((error)=> {
         NylasEnv.reportError(error);

spec/stores/draft-store-proxy-spec.coffee

@@ -35,12 +35,6 @@ describe "DraftChangeSet", ->
       @changeSet.add(body: 'Hello World!')
       expect(@changeSet._pending.body).toEqual('Hello World!')
 
-    describe "when the immediate option is passed", ->
-      it "should commit", ->
-        spyOn(@changeSet, 'commit')
-        @changeSet.add({body: 'Hello World!'}, {immediate: true})
-        expect(@changeSet.commit).toHaveBeenCalled()
-
     describe "otherwise", ->
       it "should commit after thirty seconds", ->
         spyOn(@changeSet, 'commit')
@@ -50,7 +44,6 @@ describe "DraftChangeSet", ->
         expect(@changeSet.commit).toHaveBeenCalled()
 
   describe "commit", ->
-
     it "should resolve immediately if the pending set is empty", ->
       @changeSet._pending = {}
       waitsForPromise =>
@@ -193,53 +186,57 @@ describe "DraftStoreProxy", ->
       expect(@proxy.draft().subject).toEqual(updatedDraft.subject)
 
     it "atomically commits changes", ->
-      spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft))
+      spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft))
       spyOn(DatabaseStore, 'inTransaction').andCallThrough()
+      @proxy.changes.add({body: "123"})
       waitsForPromise =>
-        @proxy.changes.add({body: "123"}, {immediate: true}).then =>
+        @proxy.changes.commit().then =>
           expect(DatabaseStore.inTransaction).toHaveBeenCalled()
           expect(DatabaseStore.inTransaction.calls.length).toBe 1
 
     it "persist the applied changes", ->
-      spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft))
+      spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft))
+      @proxy.changes.add({body: "123"})
       waitsForPromise =>
-        @proxy.changes.add({body: "123"}, {immediate: true}).then =>
+        @proxy.changes.commit().then =>
           expect(DatabaseTransaction.prototype.persistModel).toHaveBeenCalled()
           updated = DatabaseTransaction.prototype.persistModel.calls[0].args[0]
           expect(updated.body).toBe "123"
 
-    it "queues a SyncbackDraftTask", ->
-      spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft))
-      waitsForPromise =>
-        @proxy.changes.add({body: "123"}, {immediate: true}).then =>
-          expect(Actions.queueTask).toHaveBeenCalled()
-          task = Actions.queueTask.calls[0].args[0]
-          expect(task.draftClientId).toBe "client-id"
+    # Note: Syncback temporarily disabled
+    #
+    # it "queues a SyncbackDraftTask", ->
+    #   spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft))
+    #   @proxy.changes.add({body: "123"})
+    #   waitsForPromise =>
+    #     @proxy.changes.commit().then =>
+    #       expect(Actions.queueTask).toHaveBeenCalled()
+    #       task = Actions.queueTask.calls[0].args[0]
+    #       expect(task.draftClientId).toBe "client-id"
 
     it "doesn't queues a SyncbackDraftTask if no Syncback is passed", ->
-      spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft))
+      spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft))
       waitsForPromise =>
         @proxy.changes.commit({noSyncback: true}).then =>
           expect(Actions.queueTask).not.toHaveBeenCalled()
 
     describe "when findBy does not return a draft", ->
       it "continues and persists it's local draft reference, so it is resaved and draft editing can continue", ->
-        spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(null))
+        spyOn(DatabaseStore, "run").andReturn(Promise.resolve(null))
+        @proxy.changes.add({body: "123"})
         waitsForPromise =>
-          @proxy.changes.add({body: "123"}, {immediate: true}).then =>
+          @proxy.changes.commit().then =>
             expect(DatabaseTransaction.prototype.persistModel).toHaveBeenCalled()
             updated = DatabaseTransaction.prototype.persistModel.calls[0].args[0]
             expect(updated.body).toBe "123"
-            expect(Actions.queueTask).toHaveBeenCalled()
-            task = Actions.queueTask.calls[0].args[0]
-            expect(task.draftClientId).toBe "client-id"
 
     it "does nothing if the draft is marked as destroyed", ->
-      spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft))
+      spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft))
       spyOn(DatabaseStore, 'inTransaction').andCallThrough()
       waitsForPromise =>
         @proxy._destroyed = true
-        @proxy.changes.add({body: "123"}, {immediate: true}).then =>
+        @proxy.changes.add({body: "123"})
+        @proxy.changes.commit().then =>
           expect(DatabaseStore.inTransaction).not.toHaveBeenCalled()
 
   describe "draft pristine body", ->

src/flux/stores/draft-store-proxy.coffee

@@ -36,19 +36,16 @@ class DraftChangeSet
       clearTimeout(@_timer)
       @_timer = null
 
-  add: (changes, {immediate, silent}={}) =>
+  add: (changes, {silent} = {}) =>
     @_pending = _.extend(@_pending, changes)
     @_pending['pristine'] = false
     @_onTrigger() unless silent
-    if immediate
-      @commit()
-    else
-      clearTimeout(@_timer) if @_timer
-      @_timer = setTimeout(@commit, 10000)
+
+    clearTimeout(@_timer) if @_timer
+    @_timer = setTimeout(@commit, 10000)
 
   commit: ({noSyncback}={}) =>
     @_commitChain = @_commitChain.finally =>
-
       if Object.keys(@_pending).length is 0
         return Promise.resolve(true)
 
@@ -165,7 +162,7 @@ class DraftStoreProxy
     inMemoryDraft = @_draft
 
     DatabaseStore.inTransaction (t) =>
-      t.findBy(Message, clientId: inMemoryDraft.clientId).then (draft) =>
+      t.findBy(Message, clientId: inMemoryDraft.clientId).include(Message.attributes.body).then (draft) =>
         # This can happen if we get a "delete" delta, or something else
         # strange happens. In this case, we'll use the @_draft we have in
         # memory to apply the changes to. On the `persistModel` in the
@@ -173,14 +170,24 @@ class DraftStoreProxy
         # `SyncbackDraftTask` may then fail due to differing Ids not
         # existing, but if this happens it'll 404 and recover gracefully
         # by creating a new draft
-        if not draft then draft = inMemoryDraft
-
+        draft ?= inMemoryDraft
         updatedDraft = @changes.applyToModel(draft)
         return t.persistModel(updatedDraft)
+
     .then =>
       return if noSyncback
+      # We have temporarily disabled the syncback of most drafts to user's mail
+      # providers, due to a number of issues in the sync-engine that we're still
+      # firefighting.
+      #
+      # For now, drafts are only synced when you choose "Send Later", and then
+      # once they have a serverId we sync them periodically here.
+      #
+      return unless @_draft.serverId
+
       Actions.queueTask(new SyncbackDraftTask(@draftClientId))
 
+
 DraftStoreProxy.DraftChangeSet = DraftChangeSet
 
 module.exports = DraftStoreProxy

src/flux/stores/draft-store.coffee

@@ -546,7 +546,8 @@ class DraftStore
     @sessionForClientId(messageClientId).then (session) ->
       files = _.clone(session.draft().files) ? []
       files = _.reject files, (f) -> f.id is file.id
-      session.changes.add({files}, immediate: true)
+      session.changes.add({files})
+      session.changes.commit()
 
   _onDraftSendingFailed: ({draftClientId, threadId, errorMessage}) ->
     @_draftsSending[draftClientId] = false

src/flux/tasks/syncback-draft-task.coffee

@@ -12,7 +12,6 @@ SyncbackMetadataTask = require './syncback-metadata-task'
 Message = require '../models/message'
 Account = require '../models/account'
 
-# MutateDraftTask
 
 module.exports =
 class SyncbackDraftTask extends Task