fix(uploads): Consolidate logic, use Promisify, path.join

internal_packages/attachments/lib/image-attachment-component.cjsx

@@ -32,13 +32,12 @@ class ImageAttachmentComponent extends AttachmentComponent
     <RetinaImg name="image-download-button.png" mode={RetinaImg.Mode.ContentPreserve} />
 
   _imgOrLoader: ->
-    if @props.download
-      if @props.download.percent <= 5
-        <div style={width: "100%", height: "100px"}>
-          <Spinner visible={true} />
-        </div>
-      else
-        <DraggableImg src={"#{@props.targetPath}?percent=#{@props.download.percent}"} />
+    if @props.download and @props.download.percent <= 5
+      <div style={width: "100%", height: "100px"}>
+        <Spinner visible={true} />
+      </div>
+    else if @props.download and @props.download.percent < 100
+      <DraggableImg src={"#{@props.targetPath}?percent=#{@props.download.percent}"} />
     else
       <DraggableImg src={@props.targetPath} />
 

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

@@ -53,7 +53,7 @@ describe 'FileUploadStore', ->
       spyOn(FileUploadStore, '_verifyUpload').andCallFake -> Promise.resolve()
       spyOn(FileUploadStore, '_prepareTargetDir').andCallFake -> Promise.resolve()
       spyOn(FileUploadStore, '_copyUpload').andCallFake => Promise.resolve(@upload)
-      spyOn(FileUploadStore, '_saveUpload').andCallThrough()
+      spyOn(FileUploadStore, '_applySessionChanges').andCallThrough()
 
     it "throws if no messageClientId or path is provided", ->
       expect(-> Actions.addAttachment()).toThrow()
@@ -68,7 +68,7 @@ describe 'FileUploadStore', ->
         expect(FileUploadStore._verifyUpload).toHaveBeenCalled()
         expect(FileUploadStore._prepareTargetDir).toHaveBeenCalled()
         expect(FileUploadStore._copyUpload).toHaveBeenCalled()
-        expect(FileUploadStore._saveUpload).toHaveBeenCalled()
+        expect(FileUploadStore._applySessionChanges).toHaveBeenCalled()
         expect(@session.changes.add).toHaveBeenCalledWith({uploads: [@upload]})
 
 
@@ -78,7 +78,7 @@ describe 'FileUploadStore', ->
       spyOn(FileUploadStore, '_deleteUpload').andCallFake => Promise.resolve(@upload)
       spyOn(fs, 'rmdir')
 
-    it 'removes upload correctly', ->
+    it 'removes the upload from the draft', ->
       @draft.uploads = [{id: 'u2'}, @upload]
       waitsForPromise =>
         FileUploadStore._onRemoveAttachment(@upload)
@@ -86,13 +86,12 @@ describe 'FileUploadStore', ->
           expect(@session.changes.add).toHaveBeenCalledWith uploads: [{id: 'u2'}]
           expect(fs.rmdir).not.toHaveBeenCalled()
 
-    it 'removes upload and removes directory if no more uploads left dor message', ->
+    it 'calls deleteUpload to clean up the filesystem', ->
       @draft.uploads = [@upload]
       waitsForPromise =>
         FileUploadStore._onRemoveAttachment(@upload)
         .then =>
-          expect(@session.changes.add).toHaveBeenCalledWith uploads: []
-          expect(fs.rmdir).toHaveBeenCalled()
+          expect(FileUploadStore._deleteUpload).toHaveBeenCalled()
 
   describe '_getFileStats', ->
 
@@ -111,8 +110,8 @@ describe 'FileUploadStore', ->
       waitsForPromise ->
         FileUploadStore._getFileStats(argsObj)
         .then -> throw new Error('It should fail.')
-        .catch (msg) ->
-          expect(msg.indexOf(fpath)).toBe 0
+        .catch (error) ->
+          expect(error.message.indexOf(fpath)).toBe 0
 
 
   describe '_verifyUpload', ->
@@ -122,16 +121,16 @@ describe 'FileUploadStore', ->
       waitsForPromise ->
         FileUploadStore._verifyUpload(upload)
         .then -> throw new Error('It should fail.')
-        .catch (msg) ->
-          expect(msg.indexOf(filename + ' is a directory')).toBe 0
+        .catch (error) ->
+          expect(error.message.indexOf(filename + ' is a directory')).toBe 0
 
     it 'throws if the file is more than 25MB', ->
       upload = new Upload(msgId, fpath, {size: 25*1000000+1, isDirectory: -> false})
       waitsForPromise ->
         FileUploadStore._verifyUpload(upload)
         .then -> throw new Error('It should fail.')
-        .catch (msg) ->
-          expect(msg.indexOf(filename + ' cannot')).toBe 0
+        .catch (error) ->
+          expect(error.message.indexOf(filename + ' cannot')).toBe 0
 
     it 'resolves otherwise', ->
       upload = new Upload(msgId, fpath, {size: 1234, isDirectory: -> false})

src/flux/actions.coffee

@@ -445,9 +445,11 @@ class Actions
 
   # File Actions
   # Some file actions only need to be processed in their current window
-  @addAttachment: ActionScopeGlobal
+  @addAttachment: ActionScopeWindow
   @selectAttachment: ActionScopeWindow
-  @removeAttachment: ActionScopeGlobal
+  @removeAttachment: ActionScopeWindow
+  @attachmentUploaded: ActionScopeWindow
+
   @fetchAndOpenFile: ActionScopeWindow
   @fetchAndSaveFile: ActionScopeWindow
   @fetchFile: ActionScopeWindow

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

@@ -12,11 +12,7 @@ NylasAPI = require '../nylas-api'
 RegExpUtils = require '../../regexp-utils'
 
 Promise.promisifyAll(fs)
-
-mkdirpAsync = (folder) ->
-  new Promise (resolve, reject) ->
-    mkdirp folder, (err) ->
-      if err then reject(err) else resolve(folder)
+mkdirpAsync = Promise.promisify(mkdirp)
 
 State =
   Unstarted: 'unstarted'

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

@@ -9,6 +9,8 @@ Message = require '../models/message'
 DraftStore = require './draft-store'
 DatabaseStore = require './database-store'
 
+Promise.promisifyAll(fs)
+mkdirpAsync = Promise.promisify(mkdirp)
 
 UPLOAD_DIR = path.join(NylasEnv.getConfigDirPath(), 'uploads')
 
@@ -27,9 +29,10 @@ class FileUploadStore extends NylasStore
   Upload: Upload
 
   constructor: ->
-    @listenTo Actions.selectAttachment, @_onSelectAttachment
     @listenTo Actions.addAttachment, @_onAddAttachment
+    @listenTo Actions.selectAttachment, @_onSelectAttachment
     @listenTo Actions.removeAttachment, @_onRemoveAttachment
+    @listenTo Actions.attachmentUploaded, @_onAttachmentUploaded
     @listenTo DatabaseStore, @_onDataChanged
 
     mkdirp.sync(UPLOAD_DIR)
@@ -39,63 +42,49 @@ class FileUploadStore extends NylasStore
   _onDataChanged: (change) =>
     return unless NylasEnv.isMainWindow()
     return unless change.objectClass is Message.name and change.type is 'unpersist'
-    change.objects.forEach (message) =>
-      messageDir = "#{UPLOAD_DIR}/#{message.clientId}"
-      uploads = message.uploads
 
-      if uploads and uploads.length > 0
-        Promise.all(uploads.map (upload) => @_deleteUpload(upload))
-        .then ->
-          fs.rmdir(messageDir)
-        .catch (err) ->
-          console.warn(err)
+    change.objects.forEach (message) =>
+      messageDir = path.join(UPLOAD_DIR, message.clientId)
+      for upload in message.uploads
+        @_deleteUpload(upload)
 
   _onSelectAttachment: ({messageClientId}) ->
     @_verifyId(messageClientId)
+
     # When the dialog closes, it triggers `Actions.addAttachment`
     NylasEnv.showOpenDialog {properties: ['openFile', 'multiSelections']}, (pathsToOpen) ->
-      return if not pathsToOpen?
+      return unless pathsToOpen?
       pathsToOpen = [pathsToOpen] if _.isString(pathsToOpen)
 
       pathsToOpen.forEach (filePath) ->
         Actions.addAttachment({messageClientId, filePath})
 
   _onAddAttachment: ({messageClientId, filePath}) ->
-    return unless NylasEnv.isMainWindow()
     @_verifyId(messageClientId)
     @_getFileStats({messageClientId, filePath})
     .then(@_makeUpload)
     .then(@_verifyUpload)
     .then(@_prepareTargetDir)
     .then(@_copyUpload)
-    .then(@_saveUpload)
+    .then (upload) =>
+      @_applySessionChanges upload.messageClientId, (uploads) ->
+        uploads.concat([upload])
     .catch(@_onAttachFileError)
 
   _onRemoveAttachment: (upload) ->
-    return unless NylasEnv.isMainWindow()
     return Promise.resolve() unless upload
-    {messageClientId} = upload
+
+    @_applySessionChanges upload.messageClientId, (uploads) ->
+      _.reject(uploads, _.matcher({id: upload.id}))
+
+    @_deleteUpload(upload).catch(@_onAttachFileError)
+
+  _onAttachmentUploaded: (upload) ->
+    return Promise.resolve() unless upload
     @_deleteUpload(upload)
-    .then (upload) =>
-      DraftStore.sessionForClientId(messageClientId)
-    .then (session) =>
-      uploads = session.draft().uploads
-      uploads = _.reject(uploads, ({id}) -> id is upload.id)
-      if uploads.length is 0
-        fs.rmdir("#{UPLOAD_DIR}/#{messageClientId}")
-      session.changes.add({uploads})
-    .catch(@_onAttachFileError)
-
-  _onAttachFileError: (message) ->
-    {remote} = require('electron')
-    dialog = remote.require('dialog')
-    console.error(message)
-    dialog.showMessageBox
-      type: 'info',
-      buttons: ['OK'],
-      message: 'Cannot Attach File',
-      detail: message
 
+  _onAttachFileError: (error) ->
+    NylasEnv.showErrorDialog(error.message)
 
   # Helpers
 
@@ -104,12 +93,10 @@ class FileUploadStore extends NylasStore
       throw new Error "You need to pass the ID of the message (draft) this Action refers to"
 
   _getFileStats: ({messageClientId, filePath}) ->
-    return new Promise (resolve, reject) ->
-      fs.stat filePath, (err, stats) =>
-        if err
-          reject("#{filePath} could not be found, or has invalid file permissions.")
-        else
-          resolve({messageClientId, filePath, stats})
+    fs.statAsync(filePath).then (stats) =>
+      Promise.resolve({messageClientId, filePath, stats})
+    .catch (err) ->
+      Promise.reject(new Error("#{filePath} could not be found, or has invalid file permissions."))
 
   _makeUpload: ({messageClientId, filePath, stats}) ->
     Promise.resolve(new Upload(messageClientId, filePath, stats))
@@ -117,19 +104,14 @@ class FileUploadStore extends NylasStore
   _verifyUpload: (upload) ->
     {filename, stats} = upload
     if stats.isDirectory()
-      Promise.reject("#{filename} is a directory. Try compressing it and attaching it again.")
+      Promise.reject(new Error("#{filename} is a directory. Try compressing it and attaching it again."))
     else if stats.size > 25 * 1000000
-      Promise.reject("#{filename} cannot be attached because it is larger than 25MB.")
+      Promise.reject(new Error("#{filename} cannot be attached because it is larger than 25MB."))
     else
       Promise.resolve(upload)
 
   _prepareTargetDir: (upload) =>
-    return new Promise (resolve, reject) ->
-      mkdirp upload.targetDir, (err) ->
-        if err
-          reject("Error creating folder for upload: `#{upload.filename}`")
-        else
-          resolve(upload)
+    mkdirpAsync(upload.targetDir).thenReturn(upload)
 
   _copyUpload: (upload) ->
     return new Promise (resolve, reject) =>
@@ -138,25 +120,25 @@ class FileUploadStore extends NylasStore
       writeStream = fs.createWriteStream(targetPath)
 
       readStream.on 'error', ->
-        reject("Error while reading file from #{originPath}")
+        reject(new Error("Could not read file at path: #{originPath}"))
       writeStream.on 'error', ->
-        reject("Error while writing file #{upload.filename}")
+        reject(new Error("Could not write #{upload.filename} to uploads directory."))
       readStream.on 'end', ->
         resolve(upload)
       readStream.pipe(writeStream)
 
-  _deleteUpload: (upload) ->
-    return new Promise (resolve, reject) ->
-      fs.unlink upload.targetPath, (err) ->
-        reject("Error removing file #{upload.filename}") if err
-        fs.rmdir upload.targetDir, (err) ->
-          reject("Error removing directory for file #{upload.filename}") if err
-          resolve(upload)
+  _deleteUpload: (upload) =>
+    fs.unlinkAsync(upload.targetPath).then ->
+      fs.rmdirAsync(upload.targetDir).then ->
+        fs.rmdir path.join(UPLOAD_DIR, upload.messageClientId), (err) ->
+          # Will fail if it's not empty, which is fine.
+        Promise.resolve(upload)
+    .catch ->
+      Promise.reject(new Error("Error cleaning up file #{upload.filename}"))
 
-  _saveUpload: (upload) =>
-    DraftStore.sessionForClientId(upload.messageClientId)
-    .then (session) =>
-      uploads = session.draft().uploads.concat [upload]
+  _applySessionChanges: (messageClientId, changeFunction) =>
+    DraftStore.sessionForClientId(messageClientId).then (session) =>
+      uploads = changeFunction(session.draft().uploads)
       session.changes.add({uploads})
 
 module.exports = new FileUploadStore()

src/flux/tasks/send-draft.coffee

@@ -111,6 +111,9 @@ class SendDraftTask extends Task
         @draft.uploads.splice(@draft.uploads.indexOf(upload), 1)
         @draft.files.push(file)
 
+        # Deletes the attachment from the uploads folder
+        Actions.attachmentUploaded(upload)
+
   _sendAndCreateMessage: =>
     NylasAPI.makeRequest
       path: "/send"
@@ -170,11 +173,6 @@ class SendDraftTask extends Task
     if NylasEnv.config.get("core.sending.sounds")
       SoundRegistry.playSound('send')
 
-    # Remove attachments we were waiting to upload
-    # Call the Action to do this
-    for upload in @draft.uploads
-      Actions.removeAttachment(upload)
-
     return Promise.resolve(Task.Status.Success)
 
   _onError: (err) =>