diff --git a/internal_packages/composer/lib/composer-view.cjsx b/internal_packages/composer/lib/composer-view.cjsx index 3248d92db..26ceded46 100644 --- a/internal_packages/composer/lib/composer-view.cjsx +++ b/internal_packages/composer/lib/composer-view.cjsx @@ -493,7 +493,11 @@ class ComposerView extends React.Component _sendDraft: (options = {}) => return unless @_proxy - return if @state.isSending + # We need to check the `DraftStore` instead of `@state.isSending` + # because the `DraftStore` is immediately and synchronously updated as + # soon as this function fires. Since `setState` is asynchronous, if we + # used that as our only check, then we might get a false reading. + return if DraftStore.isSendingDraft(@props.localId) draft = @_proxy.draft() remote = require('remote') @@ -570,7 +574,6 @@ class ComposerView extends React.Component @setState {showcc: true} @focus('textFieldCc') - _onSendingStateChanged: => @setState isSending: DraftStore.isSendingDraft(@props.localId) diff --git a/internal_packages/composer/spec/composer-view-spec.cjsx b/internal_packages/composer/spec/composer-view-spec.cjsx index 62af958f3..2542f1467 100644 --- a/internal_packages/composer/spec/composer-view-spec.cjsx +++ b/internal_packages/composer/spec/composer-view-spec.cjsx @@ -48,9 +48,10 @@ draftStoreProxyStub = (localId, returnedDraft) -> listen: -> -> draft: -> (returnedDraft ? new Message(draft: true)) draftLocalId: localId + cleanup: -> changes: add: -> - commit: -> + commit: -> Promise.resolve() applyToModel: -> searchContactStub = (email) -> @@ -100,33 +101,37 @@ describe "A blank composer view", -> it "shows and focuses on bcc field when already open", -> +# This will setup the mocks necessary to make the composer element (once +# mounted) think it's attached to the given draft. This mocks out the +# proxy system used by the composer. +DRAFT_LOCAL_ID = "local-123" +useDraft = (draftAttributes={}) -> + @draft = new Message _.extend({draft: true, body: ""}, draftAttributes) + draft = @draft + proxy = draftStoreProxyStub(DRAFT_LOCAL_ID, @draft) + spyOn(DraftStore, "sessionForLocalId").andCallFake -> new Promise (resolve, reject) -> resolve(proxy) + spyOn(ComposerView.prototype, "componentWillMount").andCallFake -> + @_prepareForDraft(DRAFT_LOCAL_ID) + @_setupSession(proxy) + +useFullDraft = -> + useDraft.call @, + from: [u1] + to: [u2] + cc: [u3, u4] + bcc: [u5] + subject: "Test Message 1" + body: "Hello World
This is a test" + +makeComposer = -> + @composer = ReactTestUtils.renderIntoDocument( + + ) + describe "populated composer", -> - # This will setup the mocks necessary to make the composer element (once - # mounted) think it's attached to the given draft. This mocks out the - # proxy system used by the composer. - DRAFT_LOCAL_ID = "local-123" - useDraft = (draftAttributes={}) -> - @draft = new Message _.extend({draft: true, body: ""}, draftAttributes) - draft = @draft - proxy = draftStoreProxyStub(DRAFT_LOCAL_ID, @draft) - spyOn(DraftStore, "sessionForLocalId").andCallFake -> new Promise (resolve, reject) -> resolve(proxy) - spyOn(ComposerView.prototype, "componentWillMount").andCallFake -> - @_prepareForDraft(DRAFT_LOCAL_ID) - @_setupSession(proxy) - - useFullDraft = -> - useDraft.call @, - from: [u1] - to: [u2] - cc: [u3, u4] - bcc: [u5] - subject: "Test Message 1" - body: "Hello World
This is a test" - - makeComposer = -> - @composer = ReactTestUtils.renderIntoDocument( - - ) + beforeEach -> + @isSending = {state: false} + spyOn(DraftStore, "isSendingDraft").andCallFake => @isSending.state describe "When displaying info from a draft", -> beforeEach -> @@ -299,13 +304,6 @@ describe "populated composer", -> spyOn(@dialog, "showMessageBox") spyOn(Actions, "sendDraft") - it "doesn't send twice", -> - useFullDraft.call(@) - makeComposer.call(@) - @composer._sendDraft() - @composer._sendDraft() - expect(Actions.sendDraft.calls.length).toBe 1 - it "shows a warning if there are no recipients", -> useDraft.call @, subject: "no recipients" makeComposer.call(@) @@ -430,15 +428,12 @@ describe "populated composer", -> expect(Actions.sendDraft).toHaveBeenCalledWith(DRAFT_LOCAL_ID) expect(Actions.sendDraft.calls.length).toBe 1 - simulateDraftStore = -> - spyOn(DraftStore, "isSendingDraft").andReturn true - DraftStore.trigger() - it "doesn't send twice if you double click", -> useFullDraft.apply(@); makeComposer.call(@) sendBtn = React.findDOMNode(@composer.refs.sendButton) ReactTestUtils.Simulate.click sendBtn - simulateDraftStore() + @isSending.state = true + DraftStore.trigger() ReactTestUtils.Simulate.click sendBtn expect(Actions.sendDraft).toHaveBeenCalledWith(DRAFT_LOCAL_ID) expect(Actions.sendDraft.calls.length).toBe 1 @@ -449,23 +444,23 @@ describe "populated composer", -> cover = ReactTestUtils.findRenderedDOMComponentWithClass(@composer, "composer-cover") expect(React.findDOMNode(cover).style.display).toBe "none" ReactTestUtils.Simulate.click sendBtn - simulateDraftStore() + @isSending.state = true + DraftStore.trigger() expect(React.findDOMNode(cover).style.display).toBe "block" expect(@composer.state.isSending).toBe true it "re-enables the composer if sending threw an error", -> - sending = null - spyOn(DraftStore, "isSendingDraft").andCallFake => return sending + @isSending.state = null useFullDraft.apply(@); makeComposer.call(@) sendBtn = React.findDOMNode(@composer.refs.sendButton) ReactTestUtils.Simulate.click sendBtn - sending = true + @isSending.state = true DraftStore.trigger() expect(@composer.state.isSending).toBe true - sending = false + @isSending.state = false DraftStore.trigger() expect(@composer.state.isSending).toBe false @@ -577,3 +572,26 @@ describe "populated composer", -> el = ReactTestUtils.findRenderedDOMComponentWithClass(@composer, 'image-file-upload') expect(el).toBeDefined() + +describe "when the DraftStore `isSending` isn't stubbed out", -> + beforeEach -> + DraftStore._pendingEnqueue = {} + + it "doesn't send twice in a popout", -> + spyOn(Actions, "queueTask") + spyOn(Actions, "sendDraft").andCallThrough() + useFullDraft.call(@) + makeComposer.call(@) + @composer._sendDraft() + @composer._sendDraft() + expect(Actions.sendDraft.calls.length).toBe 1 + + it "doesn't send twice in the main window", -> + spyOn(Actions, "queueTask") + spyOn(Actions, "sendDraft").andCallThrough() + spyOn(atom, "isMainWindow").andReturn true + useFullDraft.call(@) + makeComposer.call(@) + @composer._sendDraft() + @composer._sendDraft() + expect(Actions.sendDraft.calls.length).toBe 1 diff --git a/spec-nylas/stores/task-queue-spec.coffee b/spec-nylas/stores/task-queue-spec.coffee index ad84cf195..f4bf38e2d 100644 --- a/spec-nylas/stores/task-queue-spec.coffee +++ b/spec-nylas/stores/task-queue-spec.coffee @@ -72,6 +72,15 @@ describe "TaskQueue", -> TaskQueue.enqueue(@unstartedTask) expect(@unstartedTask.runLocal).toHaveBeenCalled() + it "add it to the queue after `performLocalComplete` has run", -> + task = new Task() + spyOn(atom, "isMainWindow").andReturn true + waitsForPromise -> + TaskQueue.enqueue(task) + task.waitForPerformLocal().then -> + expect(TaskQueue._queue.length).toBe 1 + expect(TaskQueue._queue[0]).toBe task + it "notifies the queue should be processed", -> spyOn(TaskQueue, "_processQueue").andCallThrough() spyOn(TaskQueue, "_processTask") diff --git a/src/flux/stores/draft-store.coffee b/src/flux/stores/draft-store.coffee index 6fd786beb..760079f6d 100644 --- a/src/flux/stores/draft-store.coffee +++ b/src/flux/stores/draft-store.coffee @@ -65,6 +65,22 @@ class DraftStore @_draftSessions = {} @_extensions = [] + # We would ideally like to be able to calculate the sending state + # declaratively from the existence of the SendDraftTask on the + # TaskQueue. + # + # Unfortunately it takes a while for the Task to end up on the Queue. + # Before it's there, the Draft session is fetched, changes are + # applied, it's saved to the DB, and performLocal is run. In the + # meantime, several triggers from the DraftStore may fire (like when + # it's saved to the DB). At the time of those triggers, the task is + # not yet on the Queue and the DraftStore incorrectly says + # `isSendingDraft` is false. + # + # As a result, we keep track of the intermediate time between when we + # request to queue something, and when it appears on the queue. + @_pendingEnqueue = {} + ipc.on 'mailto', @_onHandleMailtoLink # TODO: Doesn't work if we do window.addEventListener, but this is @@ -99,8 +115,8 @@ class DraftStore isSendingDraft: (draftLocalId) -> if atom.isMainWindow() task = TaskQueue.findTask(SendDraftTask, {draftLocalId}) - return task? - else return false + return task? or @_pendingEnqueue[draftLocalId] + else return @_pendingEnqueue[draftLocalId] ### Composer Extensions @@ -394,6 +410,7 @@ class DraftStore # The user request to send the draft _onSendDraft: (draftLocalId) => + @_pendingEnqueue[draftLocalId] = true @sessionForLocalId(draftLocalId).then (session) => @_runExtensionsBeforeSend(session) @@ -401,14 +418,22 @@ class DraftStore session.changes.commit().then => task = new SendDraftTask draftLocalId, {fromPopout: @_isPopout()} + + if atom.isMainWindow() + # We need to wait for performLocal to finish before `trigger`ing. + # Only when `performLocal` is done will the task be on the + # TaskQueue. When we `trigger` listeners should be able to call + # `isSendingDraft` and have it accurately return true. + task.waitForPerformLocal().then => + # As far as this window is concerned, we're not making any more + # edits and are destroying the session. If there are errors down + # the line, we'll make a new session and handle them later + @_doneWithSession(session) + @_pendingEnqueue[draftLocalId] = false + @trigger() + Actions.queueTask(task) - - # As far as this window is concerned, we're not making any more - # edits and are destroying the session. If there are errors down - # the line, we'll make a new session and handle them later @_doneWithSession(session) - @trigger() - atom.close() if @_isPopout() _isPopout: -> diff --git a/src/flux/stores/task-queue.coffee b/src/flux/stores/task-queue.coffee index 204cedcc3..2fc1fd69a 100644 --- a/src/flux/stores/task-queue.coffee +++ b/src/flux/stores/task-queue.coffee @@ -113,6 +113,13 @@ class TaskQueue @_dequeueObsoleteTasks(task) task.runLocal().then => @_queue.push(task) + + # We want to make sure the task has made it onto the queue before + # `performLocalComplete` runs. Code in the `performLocalComplete` + # callback might depend on knowing that the Task is present in the + # queue. For example, when we're sending a message I want to know if + # there's already a task on the queue so I don't double-send. + task.performLocalComplete() @_updateSoon() dequeue: (taskOrId) => diff --git a/src/flux/tasks/task.coffee b/src/flux/tasks/task.coffee index 1117dbf70..e8fa9d88e 100644 --- a/src/flux/tasks/task.coffee +++ b/src/flux/tasks/task.coffee @@ -51,7 +51,9 @@ class Task constructor: -> @_performLocalCompletePromise = new Promise (resolve, reject) => - @_performLocalComplete = resolve + # This is called by the `TaskQueue` immeidately after `performLocal` + # has finished and the task has been added to the Queue. + @performLocalComplete = resolve @id = generateTempId() @creationDate = new Date() @@ -70,7 +72,6 @@ class Task else @performLocal() .then => - @_performLocalComplete() @queueState.localComplete = true @queueState.localError = null return Promise.resolve()