From 5e4fea01e592a2659bc677b9bf218bc0390cf6f0 Mon Sep 17 00:00:00 2001 From: Evan Morikawa Date: Thu, 21 May 2015 13:43:14 -0700 Subject: [PATCH] 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 | 30 +++++++----------- src/flux/actions.coffee | 15 +++++++-- src/flux/stores/draft-store-proxy.coffee | 5 ++- src/flux/stores/draft-store.coffee | 8 +---- src/flux/tasks/file-upload-task.coffee | 31 +++++++++---------- 5 files changed, 45 insertions(+), 44 deletions(-) diff --git a/spec-nylas/tasks/file-upload-task-spec.coffee b/spec-nylas/tasks/file-upload-task-spec.coffee index 2ea02b3c9..fbda1214c 100644 --- a/spec-nylas/tasks/file-upload-task-spec.coffee +++ b/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", -> - spyOn(@task, "_completedNotification").andReturn(100) + it "attaches the file to the draft", -> waitsForPromise => @task.performRemote().then => - expect(@task._completedNotification).toHaveBeenCalledWith(equivalentFile) + expect(@changes).toEqual [equivalentFile] describe "file upload notifications", -> beforeEach -> - @fileUploadFiredLast = false - spyOn(Actions, "attachFileComplete").andCallFake => - @fileUploadFiredLast = false - spyOn(Actions, "fileUploaded").andCallFake => - @fileUploadFiredLast = true + spyOn(Actions, "fileUploaded") 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 diff --git a/src/flux/actions.coffee b/src/flux/actions.coffee index f798e7b5c..e76b95a0c 100644 --- a/src/flux/actions.coffee +++ b/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 diff --git a/src/flux/stores/draft-store-proxy.coffee b/src/flux/stores/draft-store-proxy.coffee index 1ad42315a..fb7049903 100644 --- a/src/flux/stores/draft-store-proxy.coffee +++ b/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 diff --git a/src/flux/stores/draft-store.coffee b/src/flux/stores/draft-store.coffee index 8999700c9..d67c93f79 100644 --- a/src/flux/stores/draft-store.coffee +++ b/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) ? [] diff --git a/src/flux/tasks/file-upload-task.coffee b/src/flux/tasks/file-upload-task.coffee index e6c76ea15..89aeae068 100644 --- a/src/flux/tasks/file-upload-task.coffee +++ b/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 - # composers) that the file has finished uploading. - _completedNotification: (file) -> - setTimeout => - # We need these to be two separate actions in this sequence so - # 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}) + _onRemoteSuccess: (file, resolve, reject) => + clearInterval(@progress) + @req = null + @_attachFileToDraft(file).then => + Actions.uploadStateChanged @_uploadData("completed") 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