fix(draft): New Send Draft logic

Summary:
This is a WIP for the new send draft logic.

I'll add tests then update the diff

Test Plan: todo

Reviewers: bengotow, juan

Reviewed By: juan

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

spec/database-view-spec.coffee

@@ -129,6 +129,26 @@ describe "DatabaseView", ->
         expect(@view.retrievePageMetadata).toHaveBeenCalled()
         expect(@view.retrievePageMetadata.calls[0].args[0]).toEqual('1')
 
+    describe "invalidateAfterDatabaseChange with serverIds", ->
+      beforeEach ->
+        @inbox = new Label(id: 'l-1', name: 'inbox', displayName: 'Inbox')
+        @a = new Thread(clientId: 'client-a', serverId: null, subject: 'a', labels:[@inbox], lastMessageReceivedTimestamp: new Date(1428526885604))
+
+        @view = new DatabaseView Thread,
+          matchers: [Thread.attributes.labels.contains('l-1')]
+        @view._pages =
+          "0":
+            items: [@a]
+            metadata: {'a': 'a-metadata'}
+            loaded: true
+        spyOn(@view, 'invalidateRetainedRange')
+
+      it "should replace items even when their serverId changes", ->
+        a = new Thread(@a)
+        a.serverId = "server-a"
+        @view.invalidateAfterDatabaseChange({objects:[a], type: 'persist'})
+        expect(@view.invalidateRetainedRange).not.toHaveBeenCalled()
+
     describe "invalidateAfterDatabaseChange", ->
       beforeEach ->
         @inbox = new Label(id: 'l-1', name: 'inbox', displayName: 'Inbox')

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

@@ -50,6 +50,7 @@ describe "DraftChangeSet", ->
         expect(@changeSet.commit).toHaveBeenCalled()
 
   describe "commit", ->
+
     it "should resolve immediately if the pending set is empty", ->
       @changeSet._pending = {}
       waitsForPromise =>
@@ -214,6 +215,12 @@ describe "DraftStoreProxy", ->
           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))
+      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))

spec/stores/draft-store-spec.coffee

@@ -1,35 +1,35 @@
-File = require '../../src/flux/models/file'
-Thread = require '../../src/flux/models/thread'
-Message = require '../../src/flux/models/message'
-Contact = require '../../src/flux/models/contact'
+{File,
+ Utils,
+ Thread,
+ Actions,
+ Contact,
+ Message,
+ DraftStore,
+ AccountStore,
+ DatabaseStore,
+ SoundRegistry,
+ SendDraftTask,
+ DestroyDraftTask,
+ ComposerExtension,
+ ExtensionRegistry,
+ DatabaseTransaction,
+ SanitizeTransformer,
+ InlineStyleTransformer} = require 'nylas-exports'
+
 ModelQuery = require '../../src/flux/models/query'
-AccountStore = require '../../src/flux/stores/account-store'
-DatabaseStore = require '../../src/flux/stores/database-store'
-DatabaseTransaction = require '../../src/flux/stores/database-transaction'
-DraftStore = require '../../src/flux/stores/draft-store'
-ComposerExtension = require '../../src/extensions/composer-extension'
-SendDraftTask = require '../../src/flux/tasks/send-draft'
-DestroyDraftTask = require '../../src/flux/tasks/destroy-draft'
-SoundRegistry = require '../../src/sound-registry'
-Actions = require '../../src/flux/actions'
-Utils = require '../../src/flux/models/utils'
-ExtensionRegistry = require '../../src/extension-registry'
 
-InlineStyleTransformer = require '../../src/services/inline-style-transformer'
-SanitizeTransformer = require '../../src/services/sanitize-transformer'
-
-{ipcRenderer} = require 'electron'
 _ = require 'underscore'
+{ipcRenderer} = require 'electron'
 
+msgFromMe = null
 fakeThread = null
+fakeMessages = null
 fakeMessage1 = null
 fakeMessage2 = null
-msgFromMe = null
 msgWithReplyTo = null
-msgWithReplyToDuplicates = null
 messageWithStyleTags = null
-fakeMessages = null
 fakeMessageWithFiles = null
+msgWithReplyToDuplicates = null
 
 class TestExtension extends ComposerExtension
   @prepareNewDraft: (draft) ->
@@ -658,10 +658,11 @@ describe "DraftStore", ->
       DraftStore._draftSessions = {}
       DraftStore._draftsSending = {}
       @forceCommit = false
+      msg = new Message(clientId: draftClientId)
       proxy =
         prepare: -> Promise.resolve(proxy)
         teardown: ->
-        draft: -> {}
+        draft: -> msg
         changes:
           commit: ({force}={}) =>
             @forceCommit = force

spec/tasks/send-draft-spec.coffee

@@ -1,5 +1,4 @@
 _ = require 'underscore'
-
 {APIError,
  Actions,
  DatabaseStore,
@@ -13,12 +12,26 @@ _ = require 'underscore'
  NylasAPI,
  SoundRegistry} = require 'nylas-exports'
 
+DBt = DatabaseTransaction.prototype
+
 describe "SendDraftTask", ->
+  beforeEach ->
+    ## TODO FIXME: If we don't spy on DatabaseStore._query, then
+    # `DatabaseStore.inTransaction` will never complete and cause all
+    # tests that depend on transactions to hang.
+    #
+    # @_query("BEGIN IMMEDIATE TRANSACTION") never resolves because
+    # DatabaseStore._query never runs because the @_open flag is always
+    # false because we never setup the DB when `NylasEnv.inSpecMode` is
+    # true.
+    spyOn(DatabaseStore, '_query').andCallFake => Promise.resolve([])
+
   describe "isDependentTask", ->
-    it "should return true if there are SyncbackDraftTasks for the same draft", ->
+    it "is not dependent on any pending SyncbackDraftTasks", ->
       @draftA = new Message
         version: '1'
-        id: '1233123AEDF1'
+        clientId: 'localid-A'
+        serverId: '1233123AEDF1'
         accountId: 'A12ADE'
         subject: 'New Draft'
         draft: true
@@ -28,7 +41,8 @@ describe "SendDraftTask", ->
 
       @draftB = new Message
         version: '1'
-        id: '1233OTHERDRAFT'
+        clientId: 'localid-B'
+        serverId: '1233OTHERDRAFT'
         accountId: 'A12ADE'
         subject: 'New Draft'
         draft: true
@@ -40,128 +54,310 @@ describe "SendDraftTask", ->
       @saveB = new SyncbackDraftTask('localid-B')
       @sendA = new SendDraftTask('localid-A')
 
-      expect(@sendA.isDependentTask(@saveA)).toBe(true)
+      expect(@sendA.isDependentTask(@saveA)).toBe(false)
 
   describe "performLocal", ->
-    it "should throw an exception if the first parameter is not a clientId", ->
-      badTasks = [new SendDraftTask()]
-      goodTasks = [new SendDraftTask('localid-a')]
-      caught = []
-      succeeded = []
+    it "throws an error if we we don't pass a draftClientId", ->
+      badTask = new SendDraftTask()
+      badTask.performLocal()
+        .then ->
+          throw new Error("Shouldn't succeed")
+        .catch (err) ->
+          expect(err.message).toBe "Attempt to call SendDraftTask.performLocal without @draftClientId."
 
-      runs ->
-        [].concat(badTasks, goodTasks).forEach (task) ->
-          task.performLocal()
-          .then -> succeeded.push(task)
-          .catch (err) -> caught.push(task)
+    it "finds the message and saves a backup copy of it", ->
+      draft = new Message
+        clientId: "local-123"
+        serverId: "server-123"
+        draft: true
 
-      waitsFor ->
-        succeeded.length + caught.length == badTasks.length + goodTasks.length
-
-      runs ->
-        expect(caught).toEqual(badTasks)
-        expect(succeeded).toEqual(goodTasks)
+      spyOn(DatabaseStore, "findBy").andReturn Promise.resolve(draft)
+      task = new SendDraftTask('local-123')
+      waitsForPromise => task.performLocal().then =>
+        expect(task.backupDraft).toBeDefined()
+        expect(task.backupDraft.clientId).toBe "local-123"
+        expect(task.backupDraft.serverId).toBe "server-123"
+        expect(task.backupDraft).not.toBe draft # It's a clone
 
   describe "performRemote", ->
     beforeEach ->
+      @accountId = "a123"
       @draftClientId = "local-123"
       @serverMessageId = '1233123AEDF1'
-      @draft = new Message
-        version: 1
-        clientId: @draftClientId
-        accountId: 'A12ADE'
-        subject: 'New Draft'
-        draft: true
-        body: 'hello world'
-        to:
-          name: 'Dummy'
-          email: 'dummy@nylas.com'
 
-      response =
+      @response =
         version: 2
         id: @serverMessageId
-        account_id: 'A12ADE'
+        account_id: @accountId
         subject: 'New Draft'
         body: 'hello world'
         to:
           name: 'Dummy'
           email: 'dummy@nylas.com'
 
-      @task = new SendDraftTask(@draftClientId)
-
-
       spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
-        options.success?(response)
-        Promise.resolve(response)
-      spyOn(DatabaseStore, 'run').andCallFake (klass, id) =>
-        Promise.resolve(@draft)
-      spyOn(DatabaseTransaction.prototype, '_query').andCallFake ->
-        Promise.resolve([])
-      spyOn(DatabaseTransaction.prototype, 'unpersistModel').andCallFake (draft) ->
+        options.success?(@response)
+        Promise.resolve(@response)
+      spyOn(DBt, 'unpersistModel').andCallFake (draft) ->
         Promise.resolve()
-      spyOn(DatabaseTransaction.prototype, 'persistModel').andCallFake (draft) ->
+      spyOn(DBt, 'persistModel').andCallFake (draft) ->
         Promise.resolve()
       spyOn(SoundRegistry, "playSound")
       spyOn(Actions, "postNotification")
       spyOn(Actions, "sendDraftSuccess")
+      spyOn(NylasEnv, "emitError")
 
-    it "should notify the draft was sent", ->
-      waitsForPromise => @task.performRemote().then =>
-        args = Actions.sendDraftSuccess.calls[0].args[0]
-        expect(args.draftClientId).toBe @draftClientId
+    runFetchLatestDraftTests = ->
+      it "fetches the draft object from the DB", ->
+        waitsForPromise => @task._fetchLatestDraft().then (model) =>
+          expect(model).toBe @draft
+          expect(@task.draftAccountId).toBe @draft.accountId
+          expect(@task.draftServerId).toBe @draft.serverId
+          expect(@task.draftVersion).toBe @draft.version
 
-    it "get an object back on success", ->
-      waitsForPromise => @task.performRemote().then =>
-        args = Actions.sendDraftSuccess.calls[0].args[0]
-        expect(args.newMessage.id).toBe @serverMessageId
+      it "throws a `NotFoundError` if the model is blank", ->
+        spyOn(@task, "_notifyUserOfError")
+        spyOn(@task, "_permanentError").andCallThrough()
+        jasmine.unspy(DatabaseStore, "findBy")
+        spyOn(DatabaseStore, 'findBy').andReturn Promise.resolve(null)
+        waitsForPromise => @task.performRemote().then =>
+          expect(DBt.persistModel.callCount).toBe 1
+          expect(DBt.persistModel).toHaveBeenCalledWith(@backupDraft)
+          expect(@task._permanentError).toHaveBeenCalled()
 
-    it "should play a sound", ->
-      spyOn(NylasEnv.config, "get").andReturn true
-      waitsForPromise => @task.performRemote().then ->
-        expect(NylasEnv.config.get).toHaveBeenCalledWith("core.sending.sounds")
-        expect(SoundRegistry.playSound).toHaveBeenCalledWith("send")
+      it "throws a `NotFoundError` if findBy fails", ->
+        spyOn(@task, "_notifyUserOfError")
+        spyOn(@task, "_permanentError").andCallThrough()
+        jasmine.unspy(DatabaseStore, "findBy")
+        spyOn(DatabaseStore, 'findBy').andReturn Promise.reject(new Error("Problem"))
+        waitsForPromise => @task.performRemote().then =>
+          expect(DBt.persistModel.callCount).toBe 1
+          expect(DBt.persistModel).toHaveBeenCalledWith(@backupDraft)
+          expect(@task._permanentError).toHaveBeenCalled()
 
-    it "shouldn't play a sound if the config is disabled", ->
-      spyOn(NylasEnv.config, "get").andReturn false
-      waitsForPromise => @task.performRemote().then ->
-        expect(NylasEnv.config.get).toHaveBeenCalledWith("core.sending.sounds")
-        expect(SoundRegistry.playSound).not.toHaveBeenCalled()
+    # All of these are run in both the context of a saved draft and a new
+    # draft.
+    runMakeSendRequestTests = ->
+      it "makes a send request with the correct data", ->
+        @task.draftAccountId = @accountId
+        waitsForPromise => @task._makeSendRequest(@draft).then =>
+          expect(NylasAPI.makeRequest).toHaveBeenCalled()
+          reqArgs = NylasAPI.makeRequest.calls[0].args[0]
+          expect(reqArgs.accountId).toBe @accountId
+          expect(reqArgs.body).toEqual @draft.toJSON()
 
-    it "should start an API request to /send", ->
-      waitsForPromise =>
-        @task.performRemote().then =>
+      it "should pass returnsModel:false", ->
+        waitsForPromise => @task._makeSendRequest(@draft).then ->
+          expect(NylasAPI.makeRequest.calls.length).toBe(1)
+          options = NylasAPI.makeRequest.mostRecentCall.args[0]
+          expect(options.returnsModel).toBe(false)
+
+      it "should always send the draft body in the request body (joined attribute check)", ->
+        waitsForPromise =>
+          @task._makeSendRequest(@draft).then =>
+            expect(NylasAPI.makeRequest.calls.length).toBe(1)
+            options = NylasAPI.makeRequest.mostRecentCall.args[0]
+            expect(options.body.body).toBe('hello world')
+
+      it "should start an API request to /send", -> waitsForPromise =>
+        @task._makeSendRequest(@draft).then =>
           expect(NylasAPI.makeRequest.calls.length).toBe(1)
           options = NylasAPI.makeRequest.mostRecentCall.args[0]
           expect(options.path).toBe("/send")
-          expect(options.accountId).toBe(@draft.accountId)
           expect(options.method).toBe('POST')
 
-    it "should locally convert the draft to a message on send", ->
-      expect(@draft.clientId).toBe @draftClientId
-      expect(@draft.serverId).toBeUndefined()
-      waitsForPromise =>
-        @task.performRemote().then =>
-          expect(DatabaseTransaction.prototype.persistModel).toHaveBeenCalled()
-          model = DatabaseTransaction.prototype.persistModel.calls[0].args[0]
+      it "retries the task if 'Invalid message public id'", ->
+        jasmine.unspy(NylasAPI, "makeRequest")
+        spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+          if options.body.reply_to_message_id
+            err = new APIError(body: "Invalid message public id")
+            Promise.reject(err)
+          else
+            options.success?(@response)
+            Promise.resolve(@response)
+
+        @draft.replyToMessageId = "reply-123"
+        @draft.threadId = "thread-123"
+        waitsForPromise => @task._makeSendRequest(@draft).then =>
+          expect(NylasAPI.makeRequest).toHaveBeenCalled()
+          expect(NylasAPI.makeRequest.callCount).toEqual 2
+          req1 = NylasAPI.makeRequest.calls[0].args[0]
+          req2 = NylasAPI.makeRequest.calls[1].args[0]
+          expect(req1.body.reply_to_message_id).toBe "reply-123"
+          expect(req1.body.thread_id).toBe "thread-123"
+
+          expect(req2.body.reply_to_message_id).toBe null
+          expect(req2.body.thread_id).toBe "thread-123"
+
+      it "retries the task if 'Invalid message public id'", ->
+        jasmine.unspy(NylasAPI, "makeRequest")
+        spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+          if options.body.reply_to_message_id
+            err = new APIError(body: "Invalid thread")
+            Promise.reject(err)
+          else
+            options.success?(@response)
+            Promise.resolve(@response)
+
+        @draft.replyToMessageId = "reply-123"
+        @draft.threadId = "thread-123"
+        waitsForPromise => @task._makeSendRequest(@draft).then =>
+          expect(NylasAPI.makeRequest).toHaveBeenCalled()
+          expect(NylasAPI.makeRequest.callCount).toEqual 2
+          req1 = NylasAPI.makeRequest.calls[0].args[0]
+          req2 = NylasAPI.makeRequest.calls[1].args[0]
+          expect(req1.body.reply_to_message_id).toBe "reply-123"
+          expect(req1.body.thread_id).toBe "thread-123"
+
+          expect(req2.body.reply_to_message_id).toBe null
+          expect(req2.body.thread_id).toBe null
+
+    runSaveNewMessageTests = ->
+      it "should write the saved message to the database with the same client ID", ->
+        waitsForPromise =>
+          @task._saveNewMessage(@response).then =>
+            expect(DBt.persistModel).toHaveBeenCalled()
+            expect(DBt.persistModel.mostRecentCall.args[0].clientId).toEqual(@draftClientId)
+            expect(DBt.persistModel.mostRecentCall.args[0].serverId).toEqual(@serverMessageId)
+            expect(DBt.persistModel.mostRecentCall.args[0].draft).toEqual(false)
+
+    runNotifySuccess = ->
+      it "should notify the draft was sent", ->
+        waitsForPromise => @task.performRemote().then =>
+          args = Actions.sendDraftSuccess.calls[0].args[0]
+          expect(args.draftClientId).toBe @draftClientId
+
+      it "get an object back on success", ->
+        waitsForPromise => @task.performRemote().then =>
+          args = Actions.sendDraftSuccess.calls[0].args[0]
+          expect(args.newMessage.id).toBe @serverMessageId
+
+      it "should play a sound", ->
+        spyOn(NylasEnv.config, "get").andReturn true
+        waitsForPromise => @task.performRemote().then ->
+          expect(NylasEnv.config.get).toHaveBeenCalledWith("core.sending.sounds")
+          expect(SoundRegistry.playSound).toHaveBeenCalledWith("send")
+
+      it "shouldn't play a sound if the config is disabled", ->
+        spyOn(NylasEnv.config, "get").andReturn false
+        waitsForPromise => @task.performRemote().then ->
+          expect(NylasEnv.config.get).toHaveBeenCalledWith("core.sending.sounds")
+          expect(SoundRegistry.playSound).not.toHaveBeenCalled()
+
+    runIntegrativeWithErrors = ->
+      describe "when there are errors", ->
+        beforeEach ->
+          spyOn(@task, "_notifyUserOfError")
+          jasmine.unspy(NylasAPI, "makeRequest")
+
+        it "notifies of a permanent error of misc error types", ->
+          ## DB error
+          thrownError = null
+          jasmine.unspy(DBt, "persistModel")
+          spyOn(DBt, "persistModel").andCallFake =>
+            thrownError = new Error('db error')
+            throw thrownError
+          waitsForPromise =>
+            @task.performRemote().then (status) =>
+              expect(status[0]).toBe Task.Status.Failed
+              expect(status[1]).toBe thrownError
+              expect(@task._notifyUserOfError).toHaveBeenCalled()
+              expect(NylasEnv.emitError).toHaveBeenCalled()
+
+        it "notifies of a permanent error on 500 errors", ->
+          thrownError = new APIError(statusCode: 500, body: "err")
+          spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+            Promise.reject(thrownError)
+          waitsForPromise => @task.performRemote().then (status) =>
+            expect(status[0]).toBe Task.Status.Failed
+            expect(status[1]).toBe thrownError
+            expect(@task._notifyUserOfError).toHaveBeenCalled()
+            expect(NylasEnv.emitError).not.toHaveBeenCalled()
+
+        it "notifies us and users of a permanent error on 400 errors", ->
+          thrownError = new APIError(statusCode: 400, body: "err")
+          spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+            Promise.reject(thrownError)
+          waitsForPromise => @task.performRemote().then (status) =>
+            expect(status[0]).toBe Task.Status.Failed
+            expect(status[1]).toBe thrownError
+            expect(@task._notifyUserOfError).toHaveBeenCalled()
+            expect(NylasEnv.emitError).toHaveBeenCalled()
+
+        it "notifies of a permanent error on timeouts", ->
+          thrownError = new APIError(statusCode: NylasAPI.TimeoutErrorCode, body: "err")
+          spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+            Promise.reject(thrownError)
+          waitsForPromise => @task.performRemote().then (status) =>
+            expect(status[0]).toBe Task.Status.Failed
+            expect(status[1]).toBe thrownError
+            expect(@task._notifyUserOfError).toHaveBeenCalled()
+            expect(NylasEnv.emitError).not.toHaveBeenCalled()
+
+        it "retries for other error types", ->
+          thrownError = new APIError(statusCode: 402, body: "err")
+          spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+            Promise.reject(thrownError)
+          waitsForPromise => @task.performRemote().then (status) =>
+            expect(status).toBe Task.Status.Retry
+            expect(@task._notifyUserOfError).not.toHaveBeenCalled()
+            expect(NylasEnv.emitError).not.toHaveBeenCalled()
+
+        it "notifies the user that the required file upload failed", ->
+          fileUploadTask = new FileUploadTask('/dev/null', 'local-1234')
+          @task.onDependentTaskError(fileUploadTask, new Error("Oh no"))
+          expect(@task._notifyUserOfError).toHaveBeenCalled()
+          expect(@task._notifyUserOfError.calls.length).toBe 1
+
+    describe "with a new draft", ->
+      beforeEach ->
+        @draft = new Message
+          version: 1
+          clientId: @draftClientId
+          accountId: @accountId
+          subject: 'New Draft'
+          draft: true
+          body: 'hello world'
+        @task = new SendDraftTask(@draftClientId)
+        @backupDraft = @draft.clone()
+        @task.backupDraft = @backupDraft # Since performLocal doesn't run
+        spyOn(DatabaseStore, 'findBy').andReturn Promise.resolve(@draft)
+
+      it "can complete a full performRemote", -> waitsForPromise =>
+        @task.performRemote().then (status) ->
+          expect(status).toBe Task.Status.Success
+
+      runFetchLatestDraftTests.call(@)
+      runMakeSendRequestTests.call(@)
+      runSaveNewMessageTests.call(@)
+
+      it "shouldn't attempt to delete a draft", -> waitsForPromise =>
+        expect(@task.draftServerId).not.toBeDefined()
+        @task._deleteRemoteDraft().then =>
+          expect(NylasAPI.makeRequest).not.toHaveBeenCalled()
+
+      runNotifySuccess.call(@)
+      runIntegrativeWithErrors.call(@)
+
+      it "should locally convert the draft to a message on send", ->
+        expect(@draft.clientId).toBe @draftClientId
+        expect(@draft.serverId).toBeUndefined()
+        waitsForPromise => @task.performRemote().then =>
+          expect(DBt.persistModel).toHaveBeenCalled()
+          model = DBt.persistModel.calls[0].args[0]
           expect(model.clientId).toBe @draftClientId
           expect(model.serverId).toBe @serverMessageId
           expect(model.draft).toBe false
 
-    describe "when the draft has been saved", ->
-      it "should send the draft ID and version", ->
-        waitsForPromise =>
-          @task.performRemote().then =>
-            expect(NylasAPI.makeRequest.calls.length).toBe(1)
-            options = NylasAPI.makeRequest.mostRecentCall.args[0]
-            expect(options.body.version/1).toBe(1)
-            expect(options.body.draft_id).toBe(@draft.serverId)
 
-    describe "when the draft has not been saved", ->
+    describe "with an existing persisted draft", ->
       beforeEach ->
+        @draftServerId = 'server-123'
         @draft = new Message
-          serverId: null
+          version: 1
           clientId: @draftClientId
-          accountId: 'A12ADE'
+          serverId: @draftServerId
+          accountId: @accountId
           subject: 'New Draft'
           draft: true
           body: 'hello world'
@@ -169,134 +365,51 @@ describe "SendDraftTask", ->
             name: 'Dummy'
             email: 'dummy@nylas.com'
         @task = new SendDraftTask(@draftClientId)
+        @backupDraft = @draft.clone()
+        @task.backupDraft = @backupDraft # Since performLocal doesn't run
+        spyOn(DatabaseStore, 'findBy').andReturn Promise.resolve(@draft)
 
-      it "should send the draft JSON", ->
-        waitsForPromise =>
-          expectedJSON = @draft.toJSON()
-          @task.performRemote().then =>
-            expect(NylasAPI.makeRequest.calls.length).toBe(1)
-            options = NylasAPI.makeRequest.mostRecentCall.args[0]
-            expect(options.body).toEqual(expectedJSON)
+      it "can complete a full performRemote", -> waitsForPromise =>
+        @task.performRemote().then (status) ->
+          expect(status).toBe Task.Status.Success
 
-      it "should always send the draft body in the request body (joined attribute check)", ->
-        waitsForPromise =>
-          @task.performRemote().then =>
-            expect(NylasAPI.makeRequest.calls.length).toBe(1)
-            options = NylasAPI.makeRequest.mostRecentCall.args[0]
-            expect(options.body.body).toBe('hello world')
+      runFetchLatestDraftTests.call(@)
+      runMakeSendRequestTests.call(@)
+      runSaveNewMessageTests.call(@)
 
-    it "should pass returnsModel:false", ->
-      waitsForPromise =>
-        @task.performRemote().then ->
-          expect(NylasAPI.makeRequest.calls.length).toBe(1)
-          options = NylasAPI.makeRequest.mostRecentCall.args[0]
-          expect(options.returnsModel).toBe(false)
+      it "should make a request to delete a draft", ->
+        waitsForPromise => @task._fetchLatestDraft().then(@task._deleteRemoteDraft).then =>
+          expect(@task.draftServerId).toBe @draftServerId
+          expect(NylasAPI.makeRequest).toHaveBeenCalled()
+          expect(NylasAPI.makeRequest.callCount).toBe 1
+          req = NylasAPI.makeRequest.calls[0].args[0]
+          expect(req.path).toBe "/drafts/#{@draftServerId}"
+          expect(req.accountId).toBe @accountId
+          expect(req.method).toBe "DELETE"
+          expect(req.returnsModel).toBe false
 
-    it "should write the saved message to the database with the same client ID", ->
-      waitsForPromise =>
-        @task.performRemote().then =>
-          expect(DatabaseTransaction.prototype.persistModel).toHaveBeenCalled()
-          expect(DatabaseTransaction.prototype.persistModel.mostRecentCall.args[0].clientId).toEqual(@draftClientId)
-          expect(DatabaseTransaction.prototype.persistModel.mostRecentCall.args[0].serverId).toEqual('1233123AEDF1')
-          expect(DatabaseTransaction.prototype.persistModel.mostRecentCall.args[0].draft).toEqual(false)
+      it "should continue if the request failes", ->
+        jasmine.unspy(NylasAPI, "makeRequest")
+        spyOn(console, "error")
+        spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
+          err = new APIError(body: "Boo", statusCode: 500)
+          Promise.reject(err)
+        waitsForPromise => @task._fetchLatestDraft().then(@task._deleteRemoteDraft).then =>
+          expect(NylasAPI.makeRequest).toHaveBeenCalled()
+          expect(NylasAPI.makeRequest.callCount).toBe 1
+          expect(console.error).toHaveBeenCalled()
+        .catch =>
+          throw new Error("Shouldn't fail the promise")
 
-  describe "failing performRemote", ->
-    beforeEach ->
-      @draft = new Message
-        version: '1'
-        clientId: @draftClientId
-        accountId: 'A12ADE'
-        threadId: 'threadId'
-        replyToMessageId: 'replyToMessageId'
-        subject: 'New Draft'
-        body: 'body'
-        draft: true
-        to:
-          name: 'Dummy'
-          email: 'dummy@nylas.com'
-      @task = new SendDraftTask("local-1234")
-      spyOn(Actions, "dequeueTask")
-      spyOn(DatabaseTransaction.prototype, '_query').andCallFake ->
-        Promise.resolve([])
-      spyOn(DatabaseTransaction.prototype, 'unpersistModel').andCallFake (draft) ->
-        Promise.resolve()
-      spyOn(DatabaseTransaction.prototype, 'persistModel').andCallFake (draft) ->
-        Promise.resolve()
+      runNotifySuccess.call(@)
+      runIntegrativeWithErrors.call(@)
 
-    describe "when the server responds with `Invalid message public ID`", ->
-      it "should resend the draft without the reply_to_message_id key set", ->
-        spyOn(DatabaseStore, 'run').andCallFake =>
-          Promise.resolve(@draft)
-        spyOn(NylasAPI, 'makeRequest').andCallFake ({body, success, error}) =>
-          if body.reply_to_message_id
-            err = new APIError(body: "Invalid message public id", statusCode: 400)
-            error?(err)
-            return Promise.reject(err)
-          else
-            success?(body)
-            return Promise.resolve(body)
-
-        waitsForPromise =>
-          @task.performRemote().then =>
-            expect(NylasAPI.makeRequest.calls.length).toBe(2)
-            expect(NylasAPI.makeRequest.calls[1].args[0].body.thread_id).toBe('threadId')
-            expect(NylasAPI.makeRequest.calls[1].args[0].body.reply_to_message_id).toBe(null)
-
-    describe "when the server responds with `Invalid thread ID`", ->
-      it "should resend the draft without the thread_id or reply_to_message_id keys set", ->
-        spyOn(DatabaseStore, 'run').andCallFake => Promise.resolve(@draft)
-        spyOn(NylasAPI, 'makeRequest').andCallFake ({body, success, error}) =>
-          new Promise (resolve, reject) =>
-            if body.thread_id
-              err = new APIError(body: "Invalid thread public id", statusCode: 400)
-              error?(err)
-              reject(err)
-            else
-              success?(body)
-              resolve(body)
-
-        waitsForPromise =>
-          @task.performRemote().then =>
-            expect(NylasAPI.makeRequest.calls.length).toBe(2)
-            expect(NylasAPI.makeRequest.calls[1].args[0].body.thread_id).toBe(null)
-            expect(NylasAPI.makeRequest.calls[1].args[0].body.reply_to_message_id).toBe(null)
-          .catch (err) =>
-            console.log(err.trace)
-
-    it "throws an error if the draft can't be found", ->
-      spyOn(DatabaseStore, 'run').andCallFake (klass, clientId) ->
-        Promise.resolve()
-      waitsForPromise =>
-        @task.performRemote().catch (error) ->
-          expect(error.message).toBeDefined()
-
-    it "throws an error if the draft isn't saved", ->
-      spyOn(DatabaseStore, 'run').andCallFake (klass, clientId) ->
-        Promise.resolve(serverId: null)
-      waitsForPromise =>
-        @task.performRemote().catch (error) ->
-          expect(error.message).toBeDefined()
-
-    it "throws an error if the DB store has issues", ->
-      spyOn(DatabaseStore, 'run').andCallFake (klass, clientId) ->
-        Promise.reject("DB error")
-      waitsForPromise =>
-        @task.performRemote().catch (error) ->
-          expect(error).toBe "DB error"
-
-  describe "failing dependent task", ->
-    it "notifies the user that the required draft save failed", ->
-      task = new SendDraftTask("local-1234")
-      syncback = new SyncbackDraftTask('local-1234')
-      spyOn(task, "_notifyUserOfError")
-      task.onDependentTaskError(syncback, new Error("Oh no"))
-      expect(task._notifyUserOfError).toHaveBeenCalled()
-      expect(task._notifyUserOfError.calls.length).toBe 1
-
-    it "notifies the user that the required file upload failed", ->
-      task = new SendDraftTask("local-1234")
-      fileUploadTask = new FileUploadTask('/dev/null', 'local-1234')
-      spyOn(task, "_notifyUserOfError")
-      task.onDependentTaskError(fileUploadTask, new Error("Oh no"))
-      expect(task._notifyUserOfError).toHaveBeenCalled()
-      expect(task._notifyUserOfError.calls.length).toBe 1
+      it "should locally convert the existing draft to a message on send", ->
+        expect(@draft.clientId).toBe @draftClientId
+        expect(@draft.serverId).toBe "server-123"
+        waitsForPromise => @task.performRemote().then =>
+          expect(DBt.persistModel).toHaveBeenCalled()
+          model = DBt.persistModel.calls[0].args[0]
+          expect(model.clientId).toBe @draftClientId
+          expect(model.serverId).toBe @serverMessageId
+          expect(model.draft).toBe false

src/flux/stores/database-store.coffee

@@ -447,7 +447,8 @@ class DatabaseStore extends NylasStore
   inTransaction: (fn) ->
     t = new DatabaseTransaction(@)
     @_transactionQueue ?= new PromiseQueue(1, Infinity)
-    @_transactionQueue.add -> t.execute(fn)
+    @_transactionQueue.add ->
+      t.execute(fn)
 
   # _accumulateAndTrigger is a guarded version of trigger that can accumulate changes.
   # This means that even if you're a bad person and call `persistModel` 100 times

src/flux/stores/database-view.coffee

@@ -199,7 +199,15 @@ class DatabaseView extends ModelView
       didMakeOptimisticChange = true
 
     for item in items
-      idx = @indexOfId(item.id)
+
+      # It's important that we check against an item's clientId to
+      # determine if it's in the set. Some item persistModel mutations
+      # change the serverId (but leave the clientId intact). If keyed off
+      # of the `id` then we would erroneously say that the item isn't in
+      # the set. This happens frequently in the DraftListStore when Draft
+      # items persist on the server and/or turn into Message items.
+      idx = @indexOfId(item.clientId)
+
       itemIsInSet = idx isnt -1
       itemShouldBeInSet = item.matches(@_matchers) and change.type isnt 'unpersist'
       indexes.push(idx)

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

@@ -48,7 +48,7 @@ class DraftChangeSet
 
   # If force is true, then we'll always run the `_onCommit` callback
   # regardless if there are _pending changes or not
-  commit: ({force}={}) =>
+  commit: ({force, noSyncback}={}) =>
     @_commitChain = @_commitChain.finally =>
 
       if not force and Object.keys(@_pending).length is 0
@@ -56,7 +56,7 @@ class DraftChangeSet
 
       @_saving = @_pending
       @_pending = {}
-      return @_onCommit().then =>
+      return @_onCommit({noSyncback}).then =>
         @_saving = {}
 
     return @_commitChain
@@ -157,7 +157,7 @@ class DraftStoreProxy
       throw new Error("DraftChangeSet was modified before the draft was prepared.")
     @trigger()
 
-  _changeSetCommit: =>
+  _changeSetCommit: ({noSyncback}={}) =>
     if @_destroyed or not @_draft
       return Promise.resolve(true)
 
@@ -179,9 +179,9 @@ class DraftStoreProxy
         updatedDraft = @changes.applyToModel(draft)
         return t.persistModel(updatedDraft)
     .then =>
+      return if noSyncback
       Actions.queueTask(new SyncbackDraftTask(@draftClientId))
 
-
 DraftStoreProxy.DraftChangeSet = DraftChangeSet
 
 module.exports = DraftStoreProxy

src/flux/stores/draft-store.coffee

@@ -473,18 +473,31 @@ class DraftStore
     @sessionForClientId(draftClientId).then (session) =>
       @_runExtensionsBeforeSend(session)
 
-      # Immediately save any pending changes so we don't save after sending
+      # Immediately save any pending changes so we don't save after
+      # sending
       #
-      # It's important that we force commit the changes before sending.
-      # Once committed, we'll queue a `SyncbackDraftTask`. Since we may be
-      # sending a draft by its serverId, we need to make sure that the
-      # server has the latest changes. It's possible for the
-      # session.changes._pending to be empty if the last SyncbackDraftTask
-      # failed during its performRemote. When we send we should always try
-      # again.
-      session.changes.commit(force: true).then =>
+      # We do NOT queue a final {SyncbackDraftTask} before sending because
+      # we're going to send the full raw body with the Send are are about
+      # to delete the draft anyway.
+      #
+      # We do, however, need to ensure that all of the pending changes are
+      # committed to the Database since we'll look them up again just
+      # before send.
+      session.changes.commit(force: true, noSyncback: true).then =>
+
+        # We unfortunately can't give the SendDraftTask the raw draft JSON
+        # data because there may still be pending tasks (like a
+        # {FileUploadTask}) that will continue to update the draft data.
         task = new SendDraftTask(draftClientId, {fromPopout: @_isPopout()})
         Actions.queueTask(task)
+
+        # NOTE: We may be done with the session in this window, but there
+        # may still be {FileUploadTask}s and other pending draft mutations
+        # in the worker window.
+        #
+        # The send "pending" indicator in the main window is declaratively
+        # bound to the existence of a `@_draftSession`. We want to show
+        # the pending state immediately even as files are uploading.
         @_doneWithSession(session)
         NylasEnv.close() if @_isPopout()
 

src/flux/stores/model-view.coffee

@@ -61,7 +61,8 @@ class ModelView
     return -1 unless id
     for pageIdx, page of @_pages
       for item, itemIdx in page.items
-        return pageIdx * @_pageSize + itemIdx if item.id is id
+        if item.id is id or item.clientId is id
+          return pageIdx * @_pageSize + itemIdx
     return -1
 
   count: ->

src/flux/tasks/file-upload-task.coffee

@@ -107,8 +107,7 @@ class FileUploadTask extends Task
 
 
     DraftStore = require '../stores/draft-store'
-    # We have a `DatabaseStore.atomically` block surrounding the object
-    # right before we persist changes
+
     DraftStore.sessionForClientId(@messageClientId).then (session) =>
       files = _.clone(session.draft().files) ? []
       files.push(file)

src/flux/tasks/send-draft.coffee

@@ -1,13 +1,15 @@
-Actions = require '../actions'
-DatabaseStore = require '../stores/database-store'
-Message = require '../models/message'
-{APIError} = require '../errors'
+_ = require 'underscore'
 Task = require './task'
-TaskQueue = require '../stores/task-queue'
-SyncbackDraftTask = require './syncback-draft'
-FileUploadTask = require './file-upload-task'
+Actions = require '../actions'
+Message = require '../models/message'
 NylasAPI = require '../nylas-api'
+TaskQueue = require '../stores/task-queue'
+{APIError} = require '../errors'
 SoundRegistry = require '../../sound-registry'
+DatabaseStore = require '../stores/database-store'
+FileUploadTask = require './file-upload-task'
+class NotFoundError extends Error
+  constructor: -> super
 
 module.exports =
 class SendDraftTask extends Task
@@ -22,95 +24,126 @@ class SendDraftTask extends Task
     other instanceof SendDraftTask and other.draftClientId is @draftClientId
 
   isDependentTask: (other) ->
-    (other instanceof SyncbackDraftTask and other.draftClientId is @draftClientId) or
     (other instanceof FileUploadTask and other.messageClientId is @draftClientId)
 
   onDependentTaskError: (task, err) ->
-    if task instanceof SyncbackDraftTask
-      msg = "Your message could not be sent because we could not save your draft. Please check your network connection and try again soon."
-    else if task instanceof FileUploadTask
+    if task instanceof FileUploadTask
       msg = "Your message could not be sent because a file failed to upload. Please try re-uploading your file and try again."
     @_notifyUserOfError(msg) if msg
 
   performLocal: ->
-    # When we send drafts, we don't update anything in the app until
-    # it actually succeeds. We don't want users to think messages have
-    # already sent when they haven't!
     if not @draftClientId
       return Promise.reject(new Error("Attempt to call SendDraftTask.performLocal without @draftClientId."))
-    Promise.resolve()
+
+    # It's possible that between a user requesting the draft to send and
+    # the queue eventualy getting around to the `performLocal`, the Draft
+    # object may have been deleted. This could be caused by a user
+    # accidentally hitting "delete" on the same draft in another popout
+    # window. If this happens, `performRemote` will fail when we try and
+    # look up the draft by its clientId.
+    #
+    # In this scenario, we don't want to send, but want to restore the
+    # draft and notify the user to try again. In order to safely do this
+    # we need to keep a backup to restore.
+    DatabaseStore.findBy(Message, clientId: @draftClientId).then (draftModel) =>
+      @backupDraft = draftModel.clone()
 
   performRemote: ->
-    # Fetch the latest draft data to make sure we make the request with the most
-    # recent draft version
-    DatabaseStore.findBy(Message, clientId: @draftClientId).include(Message.attributes.body).then (draft) =>
-      # The draft may have been deleted by another task. Nothing we can do.
-      @draft = draft
-      if not draft
-        return Promise.reject(new Error("We couldn't find the saved draft."))
+    @_fetchLatestDraft()
+    .then(@_makeSendRequest)
+    .then(@_saveNewMessage)
+    .then(@_deleteRemoteDraft)
+    .then(@_notifySuccess)
+    .catch(@_onError)
 
-      # Just before sending we ask the {DraftStoreProxy} to commit its
-      # changes. This will fire a {SyncbackDraftTask}. Since we will be
-      # sending the draft by its serverId, we must be ABSOLUTELY sure that
-      # the {SyncbackDraftTask} succeeded otherwise we will send an
-      # incomplete or obsolete message.
-      if draft.serverId
-        body =
-          draft_id: draft.serverId
-          version: draft.version
-      else
-        body = draft.toJSON()
+  _fetchLatestDraft: ->
+    DatabaseStore.findBy(Message, clientId: @draftClientId).then (draftModel) =>
+      @draftAccountId = draftModel.accountId
+      @draftServerId = draftModel.serverId
+      @draftVersion = draftModel.version
+      if not draftModel
+        throw new NotFoundError("#{@draftClientId} not found")
+      return draftModel
+    .catch (err) =>
+      throw new NotFoundError("#{@draftClientId} not found")
 
-      return @_send(body)
-
-  # Returns a promise which resolves when the draft is sent. There are several
-  # failure cases where this method may call itself, stripping bad fields out of
-  # the body. This promise only rejects when these changes have been tried.
-  _send: (body) ->
+  _makeSendRequest: (draftModel) =>
     NylasAPI.makeRequest
       path: "/send"
-      accountId: @draft.accountId
+      accountId: @draftAccountId
       method: 'POST'
-      body: body
+      body: draftModel.toJSON()
       timeout: 1000 * 60 * 5 # We cannot hang up a send - won't know if it sent
       returnsModel: false
-
-    .then (json) =>
-      # The JSON returned from the server will be the new Message.
-      #
-      # Our old draft may or may not have a serverId. We update the draft
-      # with whatever the server returned (which includes a serverId).
-      #
-      # We then save the model again (keyed by its clientId) to indicate
-      # that it is no longer a draft, but rather a Message (draft: false)
-      # with a valid serverId.
-      @draft = @draft.clone().fromJSON(json)
-      @draft.draft = false
-      DatabaseStore.inTransaction (t) =>
-        t.persistModel(@draft)
-      .then =>
-        if NylasEnv.config.get("core.sending.sounds")
-          SoundRegistry.playSound('send')
-        Actions.sendDraftSuccess
-          draftClientId: @draftClientId
-          newMessage: @draft
-
-        return Promise.resolve(Task.Status.Success)
-      .catch @_permanentError
-
-    .catch APIError, (err) =>
+    .catch (err) =>
+      tryAgainDraft = draftModel.clone()
+      # If the message you're "replying to" were deleted
       if err.message?.indexOf('Invalid message public id') is 0
-        body.reply_to_message_id = null
-        return @_send(body)
+        tryAgainDraft.replyToMessageId = null
+        return @_makeSendRequest(tryAgainDraft)
       else if err.message?.indexOf('Invalid thread') is 0
-        body.thread_id = null
-        body.reply_to_message_id = null
-        return @_send(body)
-      else if err.statusCode is 500
-        msg = "Your message could not be sent at this time. Please try again soon."
+        tryAgainDraft.threadId = null
+        tryAgainDraft.replyToMessageId = null
+        return @_makeSendRequest(tryAgainDraft)
+      else return Promise.reject(err)
+
+  # The JSON returned from the server will be the new Message.
+  #
+  # Our old draft may or may not have a serverId. We update the draft with
+  # whatever the server returned (which includes a serverId).
+  #
+  # We then save the model again (keyed by its client_id) to indicate that
+  # it is no longer a draft, but rather a Message (draft: false) with a
+  # valid serverId.
+  _saveNewMessage: (newMessageJSON) =>
+    @message = new Message().fromJSON(newMessageJSON)
+    @message.clientId = @draftClientId
+    @message.draft = false
+    return DatabaseStore.inTransaction (t) =>
+      t.persistModel(@message)
+
+  # We DON'T need to delete the local draft because we actually transmute
+  # it into a {Message} by setting the `draft` flat to `true` in the
+  # `_saveNewMessage` method.
+  #
+  # We DO, however, need to make sure that the remote draft has been
+  # cleaned up.
+  #
+  # Not all drafts will have a server component. Only those that have been
+  # persisted by a {SyncbackDraftTask} will have a `serverId`.
+  _deleteRemoteDraft: =>
+    return Promise.resolve() unless @draftServerId
+    NylasAPI.makeRequest
+      path: "/drafts/#{@draftServerId}"
+      accountId: @draftAccountId
+      method: "DELETE"
+      body: version: @draftVersion
+      returnsModel: false
+    .catch APIError, (err) =>
+      # If the draft failed to delete remotely, we don't really care. It
+      # shouldn't stop the send draft task from continuing.
+      console.error("Deleting the draft remotely failed", err)
+
+  _notifySuccess: =>
+    Actions.sendDraftSuccess
+      draftClientId: @draftClientId
+      newMessage: @message
+    if NylasEnv.config.get("core.sending.sounds")
+      SoundRegistry.playSound('send')
+    return Task.Status.Success
+
+  _onError: (err) =>
+    msg = "Your message could not be sent at this time. Please try again soon."
+    if err instanceof NotFoundError
+      msg = "The draft you are trying to send has been deleted. We have restored your draft. Please try and send again."
+      DatabaseStore.inTransaction (t) =>
+        t.persistModel(@backupDraft)
+      .then =>
+        return @_permanentError(err, msg)
+    else if err instanceof APIError
+      if err.statusCode is 500
         return @_permanentError(err, msg)
       else if err.statusCode in [400, 404]
-        msg = "Your message could not be sent at this time. Please try again soon."
         NylasEnv.emitError(new Error("Sending a message responded with #{err.statusCode}!"))
         return @_permanentError(err, msg)
       else if err.statusCode is NylasAPI.TimeoutErrorCode
@@ -118,6 +151,9 @@ class SendDraftTask extends Task
         return @_permanentError(err, msg)
       else
         return Promise.resolve(Task.Status.Retry)
+    else
+      NylasEnv.emitError(err)
+      return @_permanentError(err, msg)
 
   _permanentError: (err, msg) =>
     @_notifyUserOfError(msg)