From ebdf06b18331e3e52e8f6fa51a2f81b5a9d08f10 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Mon, 5 Oct 2015 17:57:24 -0700 Subject: [PATCH] fix(attachments): When downloads fail, don't resolve to chained actions Summary: Fixes Sentry 3319 and 3303 Test Plan: Run three new tests Reviewers: dillon, evan Reviewed By: dillon, evan Differential Revision: https://phab.nylas.com/D2118 --- spec/stores/file-download-store-spec.coffee | 63 ++++++++++++++++----- src/flux/stores/file-download-store.coffee | 32 +++++++---- 2 files changed, 70 insertions(+), 25 deletions(-) diff --git a/spec/stores/file-download-store-spec.coffee b/spec/stores/file-download-store-spec.coffee index 23c932224..4fb2349a8 100644 --- a/spec/stores/file-download-store-spec.coffee +++ b/spec/stores/file-download-store-spec.coffee @@ -42,6 +42,15 @@ describe "FileDownloadStore", -> spyOn(shell, 'showItemInFolder') spyOn(shell, 'openItem') @testfile = new File(filename: '123.png', contentType: 'image/png', id: "id", size: 100) + @testdownload = new Download({ + state : 'unknown', + fileId : 'id', + percent : 0, + filename : '123.png', + filesize : 100, + targetPath : '/Users/testuser/.nylas/downloads/id/123.png' + }) + FileDownloadStore._downloads = {} FileDownloadStore._downloadDirectory = "/Users/testuser/.nylas/downloads" @@ -83,24 +92,24 @@ describe "FileDownloadStore", -> FileDownloadStore._checkForDownloadedFile(f).then (downloaded) -> expect(downloaded).toBe(false) - describe "_startDownload", -> + describe "_runDownload", -> beforeEach -> - spyOn(FileDownloadStore.Download.prototype, 'run').andCallFake -> Promise.resolve() + spyOn(FileDownloadStore.Download.prototype, 'run').andCallFake -> Promise.resolve(@) spyOn(FileDownloadStore, '_prepareFolder').andCallFake -> Promise.resolve(true) spyOn(FileDownloadStore, '_cleanupDownload') it "should make sure that the download file path exists", -> - FileDownloadStore._startDownload(@testfile) + FileDownloadStore._runDownload(@testfile) expect(FileDownloadStore._prepareFolder).toHaveBeenCalled() it "should return the promise returned by download.run if the download already exists", -> existing = fileId: @testfile.id run: jasmine.createSpy('existing.run').andCallFake -> - Promise.resolve(true) + Promise.resolve(existing) FileDownloadStore._downloads[@testfile.id] = existing - promise = FileDownloadStore._startDownload(@testfile) + promise = FileDownloadStore._runDownload(@testfile) expect(promise instanceof Promise).toBe(true) waitsForPromise -> promise.then -> @@ -113,7 +122,7 @@ describe "FileDownloadStore", -> it "should resolve with a Download without calling download.run", -> waitsForPromise => - FileDownloadStore._startDownload(@testfile).then (download) -> + FileDownloadStore._runDownload(@testfile).then (download) -> expect(FileDownloadStore.Download.prototype.run).not.toHaveBeenCalled() expect(download instanceof FileDownloadStore.Download).toBe(true) expect(download.data()).toEqual({ @@ -131,7 +140,7 @@ describe "FileDownloadStore", -> Promise.resolve(false) it "should register the download with the right attributes", -> - FileDownloadStore._startDownload(@testfile) + FileDownloadStore._runDownload(@testfile) advanceClock(0) expect(FileDownloadStore.downloadDataForFile(@testfile.id)).toEqual({ state : 'unstarted',fileId : 'id', @@ -143,13 +152,13 @@ describe "FileDownloadStore", -> it "should call download.run", -> waitsForPromise => - FileDownloadStore._startDownload(@testfile) + FileDownloadStore._runDownload(@testfile) runs -> expect(FileDownloadStore.Download.prototype.run).toHaveBeenCalled() it "should resolve with a Download", -> waitsForPromise => - FileDownloadStore._startDownload(@testfile).then (download) -> + FileDownloadStore._runDownload(@testfile).then (download) -> expect(download instanceof FileDownloadStore.Download).toBe(true) expect(download.data()).toEqual({ state : 'unstarted', @@ -162,9 +171,17 @@ describe "FileDownloadStore", -> describe "_fetch", -> it "should call through to startDownload", -> - spyOn(FileDownloadStore, '_startDownload') + spyOn(FileDownloadStore, '_runDownload').andCallFake -> + Promise.resolve(@testdownload) FileDownloadStore._fetch(@testfile) - expect(FileDownloadStore._startDownload).toHaveBeenCalled() + expect(FileDownloadStore._runDownload).toHaveBeenCalled() + + it "should fail silently since it's called passively", -> + spyOn(FileDownloadStore, '_presentError') + spyOn(FileDownloadStore, '_runDownload').andCallFake => + Promise.reject(@testdownload) + FileDownloadStore._fetch(@testfile) + expect(FileDownloadStore._presentError).not.toHaveBeenCalled() describe "_fetchAndOpen", -> it "should open the file once it's been downloaded", -> @@ -172,7 +189,7 @@ describe "FileDownloadStore", -> download = {targetPath: @savePath} downloadResolve = null - spyOn(FileDownloadStore, '_startDownload').andCallFake => + spyOn(FileDownloadStore, '_runDownload').andCallFake => new Promise (resolve, reject) -> downloadResolve = resolve @@ -182,17 +199,25 @@ describe "FileDownloadStore", -> advanceClock(100) expect(shell.openItem).toHaveBeenCalledWith(@savePath) + it "should open an error if the download fails", -> + spyOn(FileDownloadStore, '_presentError') + spyOn(FileDownloadStore, '_runDownload').andCallFake => + Promise.reject(@testdownload) + FileDownloadStore._fetchAndOpen(@testfile) + advanceClock(1) + expect(FileDownloadStore._presentError).toHaveBeenCalled() + describe "_fetchAndSave", -> beforeEach -> @savePath = "/Users/imaginary/.nylas/Downloads/b.png" spyOn(atom, 'showSaveDialog').andCallFake (options, callback) => callback(@savePath) it "should open a save dialog and prompt the user to choose a download path", -> - spyOn(FileDownloadStore, '_startDownload').andCallFake => + spyOn(FileDownloadStore, '_runDownload').andCallFake => new Promise (resolve, reject) -> # never resolve FileDownloadStore._fetchAndSave(@testfile) expect(atom.showSaveDialog).toHaveBeenCalled() - expect(FileDownloadStore._startDownload).toHaveBeenCalledWith(@testfile) + expect(FileDownloadStore._runDownload).toHaveBeenCalledWith(@testfile) it "should copy the file to the download path after it's been downloaded and open it after the stream has ended", -> download = {targetPath: @savePath} @@ -202,7 +227,7 @@ describe "FileDownloadStore", -> on: (eventName, eventCallback) => onEndEventCallback = eventCallback - spyOn(FileDownloadStore, '_startDownload').andCallFake => + spyOn(FileDownloadStore, '_runDownload').andCallFake => Promise.resolve(download) spyOn(fs, 'createReadStream').andReturn(streamStub) spyOn(fs, 'createWriteStream') @@ -215,6 +240,14 @@ describe "FileDownloadStore", -> advanceClock(1) expect(shell.showItemInFolder).toHaveBeenCalledWith(download.targetPath) + it "should open an error if the download fails", -> + spyOn(FileDownloadStore, '_presentError') + spyOn(FileDownloadStore, '_runDownload').andCallFake => + Promise.reject(@testdownload) + FileDownloadStore._fetchAndSave(@testfile) + advanceClock(1) + expect(FileDownloadStore._presentError).toHaveBeenCalled() + describe "_abortFetchFile", -> beforeEach -> @download = diff --git a/src/flux/stores/file-download-store.coffee b/src/flux/stores/file-download-store.coffee index cda4d0657..201b4cc76 100644 --- a/src/flux/stores/file-download-store.coffee +++ b/src/flux/stores/file-download-store.coffee @@ -152,7 +152,7 @@ FileDownloadStore = Reflux.createStore # Returns a promise with a Download object, allowing other actions to be # daisy-chained to the end of the download operation. - _startDownload: (file) -> + _runDownload: (file) -> @_prepareFolder(file).then => targetPath = @pathForFile(file) @@ -172,18 +172,16 @@ FileDownloadStore = Reflux.createStore # Do we actually need to queue and run the download? Queuing a download # for an already-downloaded file has side-effects, like making the UI # flicker briefly. - @_checkForDownloadedFile(file).then (downloaded) => - if downloaded + @_checkForDownloadedFile(file).then (alreadyHaveFile) => + if alreadyHaveFile # If we have the file, just resolve with a resolved download representing the file. download.promise = Promise.resolve() return Promise.resolve(download) else - cleanup = => - @_cleanupDownload(download) - Promise.resolve(download) @_downloads[file.id] = download @trigger() - return download.run().catch(cleanup).then(cleanup) + return download.run().finally => + @_cleanupDownload(download) # Returns a promise that resolves with true or false. True if the file has # been downloaded, false if it should be downloaded. @@ -203,20 +201,25 @@ FileDownloadStore = Reflux.createStore mkdirpAsync(targetFolder) _fetch: (file) -> - @_startDownload(file) + @_runDownload(file).catch -> + # Passively ignore _fetchAndOpen: (file) -> - @_startDownload(file).then (download) -> + @_runDownload(file).then (download) -> shell.openItem(download.targetPath) + .catch => + @_presentError(file) _fetchAndSave: (file) -> atom.showSaveDialog @_defaultSavePath(file), (savePath) => return unless savePath - @_startDownload(file).then (download) -> + @_runDownload(file).then (download) -> stream = fs.createReadStream(download.targetPath) stream.pipe(fs.createWriteStream(savePath)) stream.on 'end', -> shell.showItemInFolder(savePath) + .catch => + @_presentError(file) _abortFetchFile: (file) -> download = @_downloads[file.id] @@ -243,5 +246,14 @@ FileDownloadStore = Reflux.createStore path.join(downloadDir, file.displayName()) + _presentError: (file) -> + dialog = require('remote').require('dialog') + dialog.showMessageBox + type: 'warning' + message: "Download Failed" + detail: "Unable to download #{file.displayName()}. + Check your network connection and try again." + buttons: ["OK"] + # Expose the Download class for our tests, and possibly for other things someday FileDownloadStore.Download = Download