Fixes T1363: move file success to task and listen to DB event

Test Plan: edgehill --test

Reviewers: bengotow

Reviewed By: bengotow

Maniphest Tasks: T1363

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

spec-nylas/tasks/file-upload-task-spec.coffee

@@ -6,6 +6,7 @@ Message = require '../../src/flux/models/message'
 Actions = require '../../src/flux/actions'
 
 NamespaceStore = require "../../src/flux/stores/namespace-store"
+DraftStore = require "../../src/flux/stores/draft-store"
 
 FileUploadTask = proxyquire "../../src/flux/tasks/file-upload-task",
   fs:
@@ -79,6 +80,14 @@ describe "FileUploadTask", ->
       spyOn(NylasAPI, 'makeRequest').andCallFake (reqParams) =>
         reqParams.success(testResponse) if reqParams.success
         return @req
+      @testFiles = []
+      @changes = []
+      spyOn(DraftStore, "sessionForLocalId").andCallFake =>
+        Promise.resolve(
+          draft: => files: @testFiles
+          changes:
+            add: ({files}) => @changes = files
+        )
 
     it "notifies when the task starts remote", ->
       waitsForPromise =>
@@ -93,18 +102,13 @@ describe "FileUploadTask", ->
         expect(options.method).toBe('POST')
         expect(options.formData.file.value).toBe("Read Stream")
 
-    it "passes the file to the completed notification function", ->
+    it "attaches the file to the draft", ->
-      spyOn(@task, "_completedNotification").andReturn(100)
       waitsForPromise => @task.performRemote().then =>
-        expect(@task._completedNotification).toHaveBeenCalledWith(equivalentFile)
+        expect(@changes).toEqual [equivalentFile]
 
     describe "file upload notifications", ->
       beforeEach ->
-        @fileUploadFiredLast = false
+        spyOn(Actions, "fileUploaded")
-        spyOn(Actions, "attachFileComplete").andCallFake =>
-          @fileUploadFiredLast = false
-        spyOn(Actions, "fileUploaded").andCallFake =>
-          @fileUploadFiredLast = true
         spyOn(@task, "_getBytesUploaded").andReturn(1000)
 
         runs =>
@@ -113,16 +117,6 @@ describe "FileUploadTask", ->
         waitsFor ->
           Actions.fileUploaded.calls.length > 0
 
-      it "calls the `attachFileComplete` action first", ->
-        runs ->
-          expect(@fileUploadFiredLast).toBe true
-
-      it "correctly fires the attachFileComplete action", ->
-        runs ->
-          expect(Actions.attachFileComplete).toHaveBeenCalledWith
-            file: equivalentFile
-            messageLocalId: localId
-
       it "correctly fires the fileUploaded action", ->
         runs ->
           expect(Actions.fileUploaded).toHaveBeenCalledWith

src/flux/actions.coffee

@@ -93,7 +93,6 @@ class Actions
   @fileAborted: ActionScopeGlobal
   @downloadStateChanged: ActionScopeGlobal
   @fileUploaded: ActionScopeGlobal
-  @attachFileComplete: ActionScopeGlobal
   @multiWindowNotification: ActionScopeGlobal
   @sendDraftSuccess: ActionScopeGlobal
   @sendToAllWindows: ActionScopeGlobal
@@ -400,12 +399,24 @@ class Actions
   @createTemplate: ActionScopeWindow
   @showTemplates: ActionScopeWindow
 
+  ###
+  Public: Remove a file from a draft.
+
+  *Scope: Window*
+
+  ```
+  Actions.removeFile
+    file: fileObject
+    messageLocalId: draftLocalId
+  ```
+  ###
+  @removeFile: ActionScopeWindow
+
   # File Actions
   # Some file actions only need to be processed in their current window
   @attachFile: ActionScopeWindow
   @attachFilePath: ActionScopeWindow
   @abortUpload: ActionScopeWindow
-  @removeFile: ActionScopeWindow
   @fetchAndOpenFile: ActionScopeWindow
   @fetchAndSaveFile: ActionScopeWindow
   @fetchFile: ActionScopeWindow

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

@@ -45,6 +45,8 @@ class DraftChangeSet
 
     DatabaseStore = require './database-store'
     DatabaseStore.findByLocalId(Message, @localId).then (draft) =>
+      if not draft
+        throw new Error("Tried to commit a draft that had already been removed from the database. DraftId: #{@localId}")
       draft = @applyToModel(draft)
       DatabaseStore.persistModel(draft).then =>
         @_pending = {}
@@ -123,7 +125,8 @@ class DraftStoreProxy
     # Is this change an update to our draft?
     myDraft = _.find(change.objects, (obj) => obj.id == @_draft.id)
     if myDraft
-      @_setDraft(myDraft)
+      @_draft = _.extend @_draft, myDraft
+      @trigger()
 
   _onDraftSwapped: (change) ->
     # A draft was saved with a new ID. Since we use the draft ID to

src/flux/stores/draft-store.coffee

@@ -59,7 +59,6 @@ class DraftStore
     @listenTo Actions.destroyDraft, @_onDestroyDraft
 
     @listenTo Actions.removeFile, @_onRemoveFile
-    @listenTo Actions.attachFileComplete, @_onAttachFileComplete
 
     atom.onBeforeUnload @_onBeforeUnload
 
@@ -165,6 +164,7 @@ class DraftStore
     return unless change.objectClass is Message.name
     containsDraft = _.some(change.objects, (msg) -> msg.draft)
     return unless containsDraft
+    @trigger(change)
 
   _isMe: (contact={}) =>
     contact.email is NamespaceStore.current().me().email
@@ -390,12 +390,6 @@ class DraftStore
       continue unless extension.finalizeSessionBeforeSending
       extension.finalizeSessionBeforeSending(session)
 
-  _onAttachFileComplete: ({file, messageLocalId}) =>
-    @sessionForLocalId(messageLocalId).then (session) ->
-      files = _.clone(session.draft().files) ? []
-      files.push(file)
-      session.changes.add({files}, true)
-
   _onRemoveFile: ({file, messageLocalId}) =>
     @sessionForLocalId(messageLocalId).then (session) ->
       files = _.clone(session.draft().files) ? []

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

@@ -41,14 +41,9 @@ class FileUploadTask extends Task
           try
             json = JSON.parse(rawResponseString)
             file = (new File).fromJSON(json[0])
-            Actions.uploadStateChanged @_uploadData("completed")
-            @_completedNotification(file)
-
-            clearInterval(@progress)
-            @req = null
-            resolve()
           catch error
             reject(error)
+          @_onRemoteSuccess(file, resolve, reject)
 
       @progress = setInterval =>
         Actions.uploadStateChanged(@_uploadData("progress"))
@@ -90,17 +85,21 @@ class FileUploadTask extends Task
     Actions.postNotification({message: msg, type: "error"})
     Actions.uploadStateChanged @_uploadData("failed")
 
-  # The `fileUploaded` action is needed to notify all other windows (like
+  _onRemoteSuccess: (file, resolve, reject) =>
-  # composers) that the file has finished uploading.
+    clearInterval(@progress)
-  _completedNotification: (file) ->
+    @req = null
-    setTimeout =>
+    @_attachFileToDraft(file).then =>
-      # We need these to be two separate actions in this sequence so
+      Actions.uploadStateChanged @_uploadData("completed")
-      # stores (like the DrafStore) can attach the file to their objects
-      # before we take action (like sending and closing the message) upon
-      # upload completion
-      Actions.attachFileComplete({file, @messageLocalId})
       Actions.fileUploaded(uploadData: @_uploadData("completed"))
-    , 1000 # To see the success state for a little bit
+      resolve()
+    .catch(reject)
+
+  _attachFileToDraft: (file) ->
+    DraftStore = require '../stores/draft-store'
+    DraftStore.sessionForLocalId(@messageLocalId).then (session) =>
+      files = _.clone(session.draft().files) ? []
+      files.push(file)
+      return session.changes.add({files}, true)
 
   _formData: ->
     file: # Must be named `file` as per the Nylas API spec