fix(file-uploads): Smattering of bug fixes related to uploading & canceling uploads

Summary:
Cancelling a file upload properly makes the task return Task.Status.Finished (previously it was broken and returning Task.Status.Retry)

Dragging a directory now gives you a nice error message

Aborting an upload now looks for the task with the given ID, not a task with the given filepath since there could be identical uploads happening in other windows

Make it more explicit that the uploadId is not the task ID

Add a few tests

Test Plan: Run new tests

Reviewers: evan

Reviewed By: evan

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

internal_packages/composer/lib/composer-view.cjsx

@@ -299,7 +299,7 @@ class ComposerView extends React.Component
         if fileOrUpload instanceof File
           @_attachmentComponent(fileOrUpload, attachmentRole)
         else
-          <UploadComponent key={fileOrUpload.uploadId} uploadData={fileOrUpload} />
+          <UploadComponent key={fileOrUpload.uploadTaskId} uploadData={fileOrUpload} />
 
     <div className="attachments-area">
       {renderSubset(@_nonImages(), 'Attachment', FileUpload)}
@@ -340,7 +340,7 @@ class ComposerView extends React.Component
     if not uploadData
       sortOrder = 0
     else
-      sortOrder = uploadData.startedUploadingAt + (1 / +uploadData.uploadId)
+      sortOrder = (uploadData.startDate / 1) + 1.0 / (uploadData.startId/1)
 
     return sortOrder
 
@@ -359,7 +359,7 @@ class ComposerView extends React.Component
     uploads = _.filter @state.uploads, (upload) =>
       for file in @state.files
         linkedUpload = FileUploadStore.linkedUpload(file)
-        return false if linkedUpload and linkedUpload.uploadId is upload.uploadId
+        return false if linkedUpload and linkedUpload.uploadTaskId is upload.uploadTaskId
       return true
 
     _.compact(uploads.concat(@state.files))

internal_packages/composer/spec/composer-view-spec.cjsx

@@ -538,14 +538,14 @@ describe "populated composer", ->
         size: 7890
 
       @up1 =
-        uploadId: 4
+        uploadTaskId: 4
         messageLocalId: DRAFT_LOCAL_ID
         filePath: "/foo/bar/f4.bmp"
         fileName: "f4.bmp"
         fileSize: 1024
 
       @up2 =
-        uploadId: 5
+        uploadTaskId: 5
         messageLocalId: DRAFT_LOCAL_ID
         filePath: "/foo/bar/f5.zip"
         fileName: "f5.zip"

spec-nylas/stores/file-upload-store-spec.coffee

@@ -1,6 +1,7 @@
 File = require '../../src/flux/models/file'
 Actions = require '../../src/flux/actions'
 FileUploadStore = require '../../src/flux/stores/file-upload-store'
+fs = require 'fs'
 
 msgId = "local-123"
 fpath = "/foo/bar/test123.jpg"
@@ -12,7 +13,7 @@ describe 'FileUploadStore', ->
       filename: "test123.jpg"
       size: 12345
     @uploadData =
-      uploadId: 123
+      uploadTaskId: 123
       messageLocalId: msgId
       filePath: fpath
       fileSize: 12345
@@ -22,7 +23,7 @@ describe 'FileUploadStore', ->
 
     spyOn(Actions, "queueTask")
 
-  describe 'when a user wants to attach a file', ->
+  describe 'attachFile', ->
     it "throws if the message id is blank", ->
       expect( -> Actions.attachFile()).toThrow()
 
@@ -35,11 +36,13 @@ describe 'FileUploadStore', ->
       expect(args.messageLocalId).toBe msgId
       expect(args.path).toBe fpath
 
-  describe 'when a user selected the file to attach', ->
+  describe 'attachFilePath', ->
     it "throws if the message id is blank", ->
       expect( -> Actions.attachFilePath()).toThrow()
 
     it 'Creates a new file upload task', ->
+      spyOn(fs, 'stat').andCallFake (path, callback) ->
+        callback(null, {isDirectory: -> false})
       Actions.attachFilePath
         messageLocalId: msgId
         path: fpath
@@ -48,6 +51,16 @@ describe 'FileUploadStore', ->
       expect(t.filePath).toBe fpath
       expect(t.messageLocalId).toBe msgId
 
+    it 'displays an error if the file path given is a directory', ->
+      spyOn(FileUploadStore, '_onAttachFileError')
+      spyOn(fs, 'stat').andCallFake (path, callback) ->
+        callback(null, {isDirectory: -> true})
+      Actions.attachFilePath
+        messageLocalId: msgId
+        path: fpath
+      expect(Actions.queueTask).not.toHaveBeenCalled()
+      expect(FileUploadStore._onAttachFileError).toHaveBeenCalled()
+
   describe 'when an uploading file is aborted', ->
     it "dequeues the matching task", ->
       spyOn(Actions, "dequeueMatchingTask")
@@ -56,7 +69,7 @@ describe 'FileUploadStore', ->
       arg = Actions.dequeueMatchingTask.calls[0].args[0]
       expect(arg).toEqual
         type: "FileUploadTask"
-        matching: filePath: fpath
+        matching: id: @uploadData.uploadTaskId
 
   describe 'when upload state changes', ->
     it 'updates the uploadData', ->

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

@@ -49,11 +49,9 @@ DATE = 1433963615918
 describe "FileUploadTask", ->
   beforeEach ->
     spyOn(Date, "now").andReturn DATE
-    spyOn(FileUploadTask, "idGen").andReturn 3
 
     @uploadData =
-      uploadId: 3
+      startDate: DATE
-      startedUploadingAt: DATE
       messageLocalId: localId
       filePath: test_file_paths[0]
       fileSize: 1234
@@ -89,13 +87,9 @@ describe "FileUploadTask", ->
       (new FileUploadTask(test_file_paths[0])).performLocal().catch (err) ->
         expect(err instanceof Error).toBe true
 
-  it 'initializes an uploadId', ->
-    task = new FileUploadTask(test_file_paths[0], localId)
-    expect(task._uploadId).toBeGreaterThan 2
-
   it 'initializes the upload start', ->
     task = new FileUploadTask(test_file_paths[0], localId)
-    expect(task._startedUploadingAt).toBe DATE
+    expect(task._startDate).toBe DATE
 
   it "notifies when the task locally starts", ->
     spyOn(Actions, "uploadStateChanged")
@@ -103,23 +97,60 @@ describe "FileUploadTask", ->
     waitsForPromise =>
       @task.performLocal().then =>
         data = _.extend @uploadData, state: "pending", bytesUploaded: 0
-        expect(Actions.uploadStateChanged).toHaveBeenCalledWith data
+        dataReceived = Actions.uploadStateChanged.calls[0].args[0]
+        expect(_.isMatch(dataReceived, data)).toBe(true)
 
   describe "when the remote API request fails with an API Error", ->
-    it "broadcasts uploadStateChanged", ->
+    beforeEach ->
-      runs ->
+      @taskExitStatus = null
-        @task.performRemote().catch (err) => console.log(err)
+      @runWithError = (simulatedError) =>
-      waitsFor ->
+        runs ->
-        @simulateRequestFailure
+          @task.performRemote().catch (err) ->
-      runs ->
+            console.log(err)
-        spyOn(@task, "_getBytesUploaded").andReturn(0)
+          .then (status) =>
-        spyOn(Actions, "uploadStateChanged")
+            @taskExitStatus = status
-        @simulateRequestFailure(new APIError())
+
-      waitsFor ->
+        waitsFor ->
-        Actions.uploadStateChanged.callCount > 0
+          @simulateRequestFailure
-      runs ->
+        runs ->
-        data = _.extend(@uploadData, {state: "failed", bytesUploaded: 0})
+          spyOn(@task, "_getBytesUploaded").andReturn(0)
-        expect(Actions.uploadStateChanged).toHaveBeenCalledWith(data)
+          spyOn(Actions, "uploadStateChanged")
+          @simulateRequestFailure(simulatedError)
+        waitsFor ->
+          Actions.uploadStateChanged.callCount > 0
+          advanceClock(100)
+
+    describe "if the error is permanent", ->
+      beforeEach ->
+        @runWithError(new APIError(statusCode: 400))
+
+      it "should broadcast `failed` if the error is permanent", ->
+        runs ->
+          data = _.extend(@uploadData, {state: "failed", bytesUploaded: 0})
+          dataReceived = Actions.uploadStateChanged.calls[0].args[0]
+          expect(_.isMatch(dataReceived, data)).toBe(true)
+
+      it "should resolve with `finished`", ->
+        runs ->
+          expect(@taskExitStatus).toBe(Task.Status.Finished)
+
+    describe "if the error is temporary", ->
+      beforeEach ->
+        @runWithError(new APIError(statusCode: 0))
+
+      it "should resolve with `retry`", ->
+        runs ->
+          expect(@taskExitStatus).toBe(Task.Status.Retry)
+
+    describe "if the request was cancelled", ->
+      beforeEach ->
+        @runWithError(new APIError(statusCode: NylasAPI.CancelledErrorCode))
+
+      it "should broadcast `aborted` if the upload was cancelled", ->
+        runs ->
+          data = _.extend(@uploadData, {state: "aborted", bytesUploaded: 0})
+          dataReceived = Actions.uploadStateChanged.calls[0].args[0]
+          expect(_.isMatch(dataReceived, data)).toBe(true)
 
   describe "when the remote API request succeeds", ->
     beforeEach ->
@@ -140,7 +171,8 @@ describe "FileUploadTask", ->
       waitsForPromise =>
         @task.performLocal().then =>
           data = _.extend @uploadData, state: "pending", bytesUploaded: 0
-          expect(Actions.uploadStateChanged).toHaveBeenCalledWith data
+          dataReceived = Actions.uploadStateChanged.calls[0].args[0]
+          expect(_.isMatch(dataReceived, data)).toBe(true)
 
     it "should start an API request", ->
       waitsForPromise => @task.performRemote().then ->
@@ -162,11 +194,14 @@ describe "FileUploadTask", ->
         @simulateRequestSuccess()
         advanceClock()
         Actions.fileUploaded.calls.length > 0
-        expect(Actions.fileUploaded).toHaveBeenCalledWith
+
-          file: equivalentFile
+        uploadDataExpected = _.extend {}, @uploadData,
-          uploadData: _.extend {}, @uploadData,
+          state: "completed"
-            state: "completed"
+          bytesUploaded: 1000
-            bytesUploaded: 1000
+
+        [{file, uploadData}] = Actions.fileUploaded.calls[0].args
+        expect(file).toEqual(equivalentFile)
+        expect(_.isMatch(uploadData, uploadDataExpected)).toBe(true)
 
     describe "when attaching a lot of files", ->
       it "attaches them all to the draft", ->
@@ -207,7 +242,3 @@ describe "FileUploadTask", ->
       advanceClock()
 
       expect(@req.abort).toHaveBeenCalled()
-      data = _.extend @uploadData,
-        state: "aborted"
-        bytesUploaded: 0
-      expect(Actions.uploadStateChanged).toHaveBeenCalledWith(data)

src/flux/nylas-api.coffee

@@ -10,6 +10,9 @@ NylasLongConnection = require './nylas-long-connection'
 {modelFromJSON, modelClassMap} = require './models/utils'
 async = require 'async'
 
+PermanentErrorCodes = [400, 404, 500]
+CancelledErrorCode = -123
+
 class NylasAPIOptimisticChangeTracker
   constructor: ->
     @_locks = {}
@@ -61,12 +64,18 @@ class NylasAPIRequest
             @options.success?(body)
             resolve(body)
       req.on 'abort', ->
-        reject(new APIError({statusCode: 0, body: 'Request Aborted'}))
+        cancelled = new APIError
+          statusCode: CancelledErrorCode,
+          body: 'Request Aborted'
+        reject(cancelled)
+
       @options.started?(req)
 
+
 class NylasAPI
 
-  PermanentErrorCodes: [400, 404, 500]
+  PermanentErrorCodes: PermanentErrorCodes
+  CancelledErrorCode: CancelledErrorCode
 
   constructor: ->
     @_workers = []

src/flux/stores/file-upload-store.coffee

@@ -1,5 +1,6 @@
 _ = require 'underscore'
 ipc = require 'ipc'
+fs = require 'fs'
 Reflux = require 'reflux'
 Actions = require '../actions'
 FileUploadTask = require '../tasks/file-upload-task'
@@ -44,18 +45,32 @@ FileUploadStore = Reflux.createStore
     atom.showOpenDialog {properties: ['openFile', 'multiSelections']}, (pathsToOpen) ->
       return if not pathsToOpen?
       pathsToOpen = [pathsToOpen] if _.isString(pathsToOpen)
-      for path in pathsToOpen
+
-        # When this task runs, we expect to hear `uploadStateChanged` actions.
+      pathsToOpen.forEach (path) ->
         Actions.attachFilePath({messageLocalId, path})
 
+  _onAttachFileError: (message) ->
+    remote = require('remote')
+    dialog = remote.require('dialog')
+    dialog.showMessageBox
+      type: 'info',
+      buttons: ['OK'],
+      message: 'Cannot Attach File',
+      detail: message
+
   _onAttachFilePath: ({messageLocalId, path}) ->
     @_verifyId(messageLocalId)
-    @task = new FileUploadTask(path, messageLocalId)
+    fs.stat path, (err, stats) =>
-    Actions.queueTask(@task)
+      return if err
+      if stats.isDirectory()
+        filename = require('path').basename(path)
+        @_onAttachFileError("#{filename} is a directory. Try compressing it and attaching it again.")
+      else
+        Actions.queueTask(new FileUploadTask(path, messageLocalId))
 
   # Receives:
   #   uploadData:
-  #     uploadId - A unique id
+  #     uploadTaskId - A unique id
   #     messageLocalId - The localId of the message (draft) we're uploading to
   #     filePath - The full absolute local system file path
   #     fileSize - The size in bytes
@@ -63,27 +78,25 @@ FileUploadStore = Reflux.createStore
   #     bytesUploaded - Current number of bytes uploaded
   #     state - one of "pending" "started" "progress" "completed" "aborted" "failed"
   _onUploadStateChanged: (uploadData) ->
-    @_fileUploads[uploadData.uploadId] = uploadData
+    @_fileUploads[uploadData.uploadTaskId] = uploadData
     @trigger()
 
   _onAbortUpload: (uploadData) ->
-    Actions.dequeueMatchingTask({
+    Actions.dequeueMatchingTask
       type: 'FileUploadTask',
-      matching: {
+      matching:
-        filePath: uploadData.filePath
+        id: uploadData.uploadTaskId
-      }
-    })
 
   _onLinkFileToUpload: ({file, uploadData}) ->
     @_linkedFiles[file.id] = uploadData
     @trigger()
 
   _onFileUploaded: ({file, uploadData}) ->
-    delete @_fileUploads[uploadData.uploadId]
+    delete @_fileUploads[uploadData.uploadTaskId]
     @trigger()
 
   _onFileAborted: (uploadData) ->
-    delete @_fileUploads[uploadData.uploadId]
+    delete @_fileUploads[uploadData.uploadTaskId]
     @trigger()
 
   _verifyId: (messageLocalId) ->

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

@@ -1,5 +1,6 @@
 fs = require 'fs'
 _ = require 'underscore'
+crypto = require 'crypto'
 pathUtils = require 'path'
 Task = require './task'
 {APIError} = require '../errors'
@@ -12,20 +13,17 @@ DatabaseStore = require '../stores/database-store'
 NylasAPI = require '../nylas-api'
 Utils = require '../models/utils'
 
-idGen = 2
+UploadCounter = 0
 
 class FileUploadTask extends Task
 
-  # Necessary so that tasks always get the same ID during specs
-  @idGen: -> idGen
-
   constructor: (@filePath, @messageLocalId) ->
     super
-    @_startedUploadingAt = Date.now()
+    @_startDate = Date.now()
-    @progress = null # The progress checking timer.
+    @_startId = UploadCounter
+    UploadCounter += 1
 
-    @_uploadId = FileUploadTask.idGen()
+    @progress = null # The progress checking timer.
-    idGen += 1
 
   performLocal: ->
     return Promise.reject(new Error("Must pass an absolute path to upload")) unless @filePath?.length
@@ -62,11 +60,15 @@ class FileUploadTask extends Task
       return Promise.resolve(Task.Status.Finished)
 
     .catch APIError, (err) =>
-      Actions.uploadStateChanged(@_uploadData("failed"))
       if err.statusCode in NylasAPI.PermanentErrorCodes
         msg = "There was a problem uploading this file. Please try again later."
+        Actions.uploadStateChanged(@_uploadData("failed"))
         Actions.postNotification({message: msg, type: "error"})
-        return Promise.reject(err)
+        return Promise.resolve(Task.Status.Finished)
+      else if err.statusCode is NylasAPI.CancelledErrorCode
+        Actions.uploadStateChanged(@_uploadData("aborted"))
+        Actions.fileAborted(@_uploadData("aborted"))
+        return Promise.resolve(Task.Status.Finished)
       else
         return Promise.resolve(Task.Status.Retry)
 
@@ -108,11 +110,6 @@ class FileUploadTask extends Task
     # NylasAPI.makeRequest to reject with an error.
     return unless @req
     @req.abort()
-    clearInterval(@progress)
-
-    # To see the aborted state for a little bit
-    Actions.uploadStateChanged(@_uploadData("aborted"))
-    setTimeout(( =>  Actions.fileAborted(@_uploadData("aborted"))), 1000)
 
   # Helper Methods
 
@@ -131,8 +128,9 @@ class FileUploadTask extends Task
   #   state - one of "pending" "started" "progress" "completed" "aborted" "failed"
   _uploadData: (state) ->
     @_memoUploadData ?=
-      uploadId: @_uploadId
+      uploadTaskId: @id
-      startedUploadingAt: @_startedUploadingAt
+      startDate: @_startDate
+      startId: @_startId
       messageLocalId: @messageLocalId
       filePath: @filePath
       fileSize: @_getFileSize(@filePath)