fix(composer): fix double-sending issue

Summary:
Fixes T2275

Messages were still double sending sometimes because it took a while for
the Task to end up on the TaskQueue, which is what we were using to
determine re-sendability

Test Plan: edgehill --test

Reviewers: bengotow

Reviewed By: bengotow

Maniphest Tasks: T2275

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

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)
 

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 <b>World</b><br/> This is a test"
+
+makeComposer = ->
+  @composer = ReactTestUtils.renderIntoDocument(
+    <ComposerView localId={DRAFT_LOCAL_ID} />
+  )
+
 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 <b>World</b><br/> This is a test"
-
-  makeComposer = ->
-    @composer = ReactTestUtils.renderIntoDocument(
-      <ComposerView localId={DRAFT_LOCAL_ID} />
-    )
+  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

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")

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: ->

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) =>

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()