feat(draft): drafts that fail to send throw better errors

Summary: Also enhancements to the developer toolbar

Test Plan: edgehill --test

Reviewers: dillon, bengotow

Reviewed By: bengotow

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

internal_packages/message-list/lib/message-controls.cjsx

@@ -75,17 +75,17 @@ class MessageControls extends React.Component
 
   _onShowActionsMenu: =>
     remote = require('remote')
-    Menu = remote.require('menu')
-    MenuItem = remote.require('menu-item')
+    SystemMenu = remote.require('menu')
+    SystemMenuItem = remote.require('menu-item')
 
     # Todo: refactor this so that message actions are provided
     # dynamically. Waiting to see if this will be used often.
-    menu = new Menu()
-    menu.append(new MenuItem({ label: 'Report Issue: Quoted Text', click: => @_onReport('Quoted Text')}))
-    menu.append(new MenuItem({ label: 'Report Issue: Rendering', click: => @_onReport('Rendering')}))
-    menu.append(new MenuItem({ type: 'separator'}))
-    menu.append(new MenuItem({ label: 'Show Original', click: => @_onShowOriginal()}))
-    menu.append(new MenuItem({ label: 'Log Data', click: => @_onLogData()}))
+    menu = new SystemMenu()
+    menu.append(new SystemMenuItem({ label: 'Report Issue: Quoted Text', click: => @_onReport('Quoted Text')}))
+    menu.append(new SystemMenuItem({ label: 'Report Issue: Rendering', click: => @_onReport('Rendering')}))
+    menu.append(new SystemMenuItem({ type: 'separator'}))
+    menu.append(new SystemMenuItem({ label: 'Show Original', click: => @_onShowOriginal()}))
+    menu.append(new SystemMenuItem({ label: 'Log Data', click: => @_onLogData()}))
     menu.popup(remote.getCurrentWindow())
 
   _onReport: (issueType) =>

internal_packages/worker-ui/lib/developer-bar-curl-item.cjsx

@@ -6,6 +6,7 @@ class DeveloperBarCurlItem extends React.Component
   render: =>
     <div className={"item status-code-#{@props.item.statusCode}"}>
       <div className="code">{@props.item.statusCode}</div>
+      <span className="timestamp">{@props.item.startMoment.format("HH:mm:ss")}&nbsp;&nbsp;</span>
       <a onClick={@_onRunCommand}>Run</a>
       <a onClick={@_onCopyCommand}>Copy</a>
       {@props.item.command}

internal_packages/worker-ui/lib/developer-bar-store.coffee

@@ -1,18 +1,18 @@
-Reflux = require 'reflux'
+NylasStore = require 'nylas-store'
 {Actions} = require 'nylas-exports'
 qs = require 'querystring'
 _ = require 'underscore'
+moment = require 'moment'
 
-curlItemId = 0
-
-DeveloperBarStore = Reflux.createStore
-  init: ->
+class DeveloperBarStore extends NylasStore
+  constructor: ->
     @_setStoreDefaults()
     @_registerListeners()
 
   ########### PUBLIC #####################################################
 
-  curlHistory: -> @_curlHistory
+  curlHistory: -> _.sortBy _.values(@_curlHistory), (item) ->
+    item.startMoment.valueOf()
 
   longPollState: -> @_longPollState
 
@@ -30,13 +30,14 @@ DeveloperBarStore = Reflux.createStore
     @_triggerThrottled()
 
   _setStoreDefaults: ->
-    @_curlHistory = []
+    @_curlHistory = {}
     @_longPollHistory = []
     @_longPollState = {}
     @_visible = atom.inDevMode()
 
   _registerListeners: ->
-    @listenTo Actions.didMakeAPIRequest, @_onAPIRequest
+    @listenTo Actions.willMakeAPIRequest, @_onWillMakeAPIRequest
+    @listenTo Actions.didMakeAPIRequest, @_onDidMakeAPIRequest
     @listenTo Actions.longPollReceivedRawDeltas, @_onLongPollDeltas
     @listenTo Actions.longPollProcessedDeltas, @_onLongPollProcessedDeltas
     @listenTo Actions.longPollStateChanged, @_onLongPollStateChange
@@ -49,7 +50,7 @@ DeveloperBarStore = Reflux.createStore
     @trigger(@)
 
   _onClear: ->
-    @_curlHistory = []
+    @_curlHistory = {}
     @_longPollHistory = []
     @trigger(@)
 
@@ -72,7 +73,17 @@ DeveloperBarStore = Reflux.createStore
     @_longPollState[accountId] = state
     @triggerThrottled(@)
 
-  _onAPIRequest: ({request, response}) ->
+  _onWillMakeAPIRequest: ({requestId, request}) =>
+    item = @_generateCurlItem({requestId, request})
+    @_curlHistory[requestId] = item
+    @triggerThrottled(@)
+
+  _onDidMakeAPIRequest: ({requestId, request, response, error}) =>
+    item = @_generateCurlItem({requestId, request, response, error})
+    @_curlHistory[requestId] = item
+    @triggerThrottled(@)
+
+  _generateCurlItem: ({requestId, request, response, error}) ->
     url = request.url
     if request.auth
       url = url.replace('://', "://#{request.auth.user}:#{request.auth.pass}@")
@@ -83,15 +94,20 @@ DeveloperBarStore = Reflux.createStore
     data = ""
     data = "-d '#{postBody}'" unless request.method == 'GET'
 
+    headers = ""
+    if request.headers
+      for k,v of request.headers
+        headers += "-H \"#{k}: #{v}\" "
+
+    statusCode = response?.statusCode ? error?.code ? "pending"
+
     item =
-      id: "curlitemId:#{curlItemId}"
-      command: "curl -X #{request.method} #{data} \"#{url}\""
-      statusCode: response?.statusCode || 0
+      id: "curlitemId:#{requestId}"
+      command: "curl -X #{request.method} #{headers}#{data} \"#{url}\""
+      statusCode: statusCode
+      startMoment: moment(request.startTime)
 
-    @_curlHistory.unshift(item)
-    curlItemId += 1
-
-    @triggerThrottled(@)
+    return item
 
   _onSendFeedback: ->
     {AccountStore,
@@ -102,7 +118,7 @@ DeveloperBarStore = Reflux.createStore
     user = AccountStore.current().name
 
     debugData = JSON.stringify({
-      queries: @_curlHistory
+      queries: _.values(@curlHistory())
     }, null, '\t')
 
     # Remove API tokens from URLs included in the debug data
@@ -142,4 +158,4 @@ DeveloperBarStore = Reflux.createStore
     DatabaseStore.persistModel(draft).then ->
       Actions.composePopoutDraft(draft.clientId)
 
-module.exports = DeveloperBarStore
+module.exports = new DeveloperBarStore()

internal_packages/worker-ui/lib/developer-bar.cjsx

@@ -127,7 +127,7 @@ class DeveloperBar extends React.Component
   _getStateFromStores: =>
     queue: TaskQueue._queue
     completed: TaskQueue._completed
-    curlHistory: DeveloperBarStore.curlHistory()
+    curlHistory: DeveloperBarStore.curlHistory().reverse()
     longPollHistory: DeveloperBarStore.longPollHistory()
     longPollState: DeveloperBarStore.longPollState()
 

internal_packages/worker-ui/stylesheets/worker-ui.less

@@ -115,6 +115,9 @@
         padding-right:8px;
         padding-bottom:3px;
       }
+      .timestamp {
+        color: rgba(255,255,255,0.5);
+      }
       .item.status-code-500,
       .item.status-code-501,
       .item.status-code-502,

spec-nylas/stores/draft-store-spec.coffee

@@ -708,6 +708,27 @@ describe "DraftStore", ->
         task = Actions.queueTask.calls[0].args[0]
         expect(task.fromPopout).toBe true
 
+    it "resets the sending state if there's an error", ->
+      spyOn(atom, "isMainWindow").andReturn false
+      DraftStore._draftsSending[draftClientId] = true
+      Actions.draftSendingFailed({errorMessage: "boohoo", draftClientId})
+      expect(DraftStore.isSendingDraft(draftClientId)).toBe false
+      expect(DraftStore.trigger).toHaveBeenCalledWith(draftClientId)
+
+    it "displays a popup in the main window if there's an error", ->
+      spyOn(atom, "isMainWindow").andReturn true
+      remote = require('remote')
+      dialog = remote.require('dialog')
+      spyOn(dialog, "showMessageBox")
+      DraftStore._draftsSending[draftClientId] = true
+      Actions.draftSendingFailed({errorMessage: "boohoo", draftClientId})
+      advanceClock(10)
+      expect(DraftStore.isSendingDraft(draftClientId)).toBe false
+      expect(DraftStore.trigger).toHaveBeenCalledWith(draftClientId)
+      expect(dialog.showMessageBox).toHaveBeenCalled()
+      dialogArgs = dialog.showMessageBox.mostRecentCall.args[1]
+      expect(dialogArgs.detail).toEqual("boohoo")
+
   describe "session teardown", ->
     beforeEach ->
       @draftTeardown = jasmine.createSpy('draft teardown')

spec-nylas/tasks/change-mail-task-spec.coffee

@@ -226,7 +226,7 @@ describe "ChangeMailTask", ->
     describe "if performRequests rejects with a temporary network error", ->
       beforeEach ->
         @task = new ChangeMailTask()
-        spyOn(@task, 'performRequests').andReturn(Promise.reject(new APIError(statusCode: 0)))
+        spyOn(@task, 'performRequests').andReturn(Promise.reject(new APIError(statusCode: NylasAPI.SampleTemporaryErrorCode)))
         spyOn(@task, 'performLocal').andReturn(Promise.resolve())
 
       it "should not revert", ->
@@ -320,7 +320,7 @@ describe "ChangeMailTask", ->
           promises[0].resolve()
           advanceClock()
           expect(err).toBe(null)
-          apiError = new APIError(statusCode: 0)
+          apiError = new APIError(statusCode: NylasAPI.SampleTemporaryErrorCode)
           promises[1].reject(apiError)
           advanceClock()
           expect(err).toBe(apiError)
@@ -405,7 +405,7 @@ describe "ChangeMailTask", ->
 
     describe "when performRemote is returning Task.Status.Retry", ->
       it "should not clean up locks", ->
-        spyOn(@task, 'performRequests').andReturn(Promise.reject(new APIError(statusCode: 0)))
+        spyOn(@task, 'performRequests').andReturn(Promise.reject(new APIError(statusCode: NylasAPI.SampleTemporaryErrorCode)))
         spyOn(@task, '_ensureLocksRemoved')
         waitsForPromise =>
           @task.performRemote().then =>

spec-nylas/tasks/file-upload-task-spec.coffee

@@ -146,7 +146,7 @@ describe "FileUploadTask", ->
 
     describe "if the error is temporary", ->
       beforeEach ->
-        @runWithError(new APIError(statusCode: 0))
+        @runWithError(new APIError(statusCode: NylasAPI.SampleTemporaryErrorCode))
 
       it "should resolve with `retry`", ->
         runs ->

src/flux/actions.coffee

@@ -90,6 +90,7 @@ class Actions
   @multiWindowNotification: ActionScopeGlobal
   @sendDraftSuccess: ActionScopeGlobal
   @sendToAllWindows: ActionScopeGlobal
+  @draftSendingFailed: ActionScopeGlobal
 
   ###
   Public: Queue a {Task} object to the {TaskQueue}.
@@ -120,6 +121,7 @@ class Actions
   @longPollProcessedDeltas: ActionScopeWorkWindow
   @longPollConnected: ActionScopeWorkWindow
   @longPollOffline: ActionScopeWorkWindow
+  @willMakeAPIRequest: ActionScopeWorkWindow
   @didMakeAPIRequest: ActionScopeWorkWindow
   @sendFeedback: ActionScopeWorkWindow
 

src/flux/edgehill-api.coffee

@@ -1,5 +1,6 @@
 _ = require 'underscore'
 nodeRequest = require 'request'
+Utils = require './models/utils'
 Actions = require './actions'
 {APIError} = require './errors'
 DatabaseStore = require './stores/database-store'
@@ -49,13 +50,18 @@ class EdgehillAPI
 
     options.error ?= @_defaultErrorCallback
 
-    nodeRequest options, (error, response, body) ->
-      PriorityUICoordinator.settle.then ->
-        Actions.didMakeAPIRequest({request: options, response: response})
-        if error? or response.statusCode > 299
-          options.error(new APIError({error:error, response:response, body:body, requestOptions: options}))
-        else
-          options.success(body) if options.success
+    # This is to provide functional closure for the variable.
+    rid = Utils.generateTempId()
+    [rid].forEach (requestId) ->
+      options.startTime = Date.now()
+      Actions.willMakeAPIRequest({request: options, requestId: requestId})
+      nodeRequest options, (error, response, body) ->
+        Actions.didMakeAPIRequest({request: options, response: response, error: error, requestId: requestId})
+        PriorityUICoordinator.settle.then ->
+          if error? or response.statusCode > 299
+            options.error(new APIError({error:error, response:response, body:body, requestOptions: options}))
+          else
+            options.success(body) if options.success
 
   urlForConnecting: (provider, email_address = '') ->
     auth = @_getCredentials()

src/flux/nylas-api.coffee

@@ -1,13 +1,18 @@
 _ = require 'underscore'
 request = require 'request'
+Utils = require './models/utils'
 Actions = require './actions'
 {APIError} = require './errors'
 PriorityUICoordinator = require '../priority-ui-coordinator'
 DatabaseStore = require './stores/database-store'
 async = require 'async'
 
-PermanentErrorCodes = [400, 404, 500]
+# A 0 code is when an error returns without a status code. These are
+# things like "ESOCKETTIMEDOUT"
+TimeoutErrorCode = 0
+PermanentErrorCodes = [400, 404, 500, TimeoutErrorCode]
 CancelledErrorCode = -123
+SampleTemporaryErrorCode = 504
 
 # This is lazy-loaded
 AccountStore = null
@@ -43,6 +48,8 @@ class NylasAPIRequest
     @options.url ?= "#{@api.APIRoot}#{@options.path}" if @options.path
     @options.json ?= true
 
+    @options.timeout ?= 15000
+
     unless @options.method is 'GET' or @options.formData
       @options.body ?= {}
     @
@@ -63,12 +70,22 @@ class NylasAPIRequest
         pass: ''
         sendImmediately: true
 
+    requestId = Utils.generateTempId()
     new Promise (resolve, reject) =>
+      @options.startTime = Date.now()
+      Actions.willMakeAPIRequest({request: @options, requestId: requestId})
       req = request @options, (error, response, body) =>
+        Actions.didMakeAPIRequest({request: @options, response: response, error: error, requestId: requestId})
         PriorityUICoordinator.settle.then =>
-          Actions.didMakeAPIRequest({request: @options, response: response})
-
           if error or response.statusCode > 299
+            # Some errors (like socket errors and some types of offline
+            # errors) return with a valid `error` object but no `response`
+            # object (and therefore no `statusCode`. To normalize all of
+            # this, we inject our own offline status code so people down
+            # the line can have a more consistent interface.
+            if not response?.statusCode
+              response ?= {}
+              response.statusCode = TimeoutErrorCode
             apiError = new APIError({error, response, body, requestOptions: @options})
             @options.error?(apiError)
             reject(apiError)
@@ -86,8 +103,10 @@ class NylasAPIRequest
 
 class NylasAPI
 
+  TimeoutErrorCode: TimeoutErrorCode
   PermanentErrorCodes: PermanentErrorCodes
   CancelledErrorCode: CancelledErrorCode
+  SampleTemporaryErrorCode: SampleTemporaryErrorCode
 
   constructor: ->
     @_workers = []

src/flux/stores/draft-store.coffee

@@ -52,6 +52,7 @@ class DraftStore
     @listenTo Actions.composeReplyAll, @_onComposeReplyAll
     @listenTo Actions.composePopoutDraft, @_onPopoutDraftClientId
     @listenTo Actions.composeNewBlankDraft, @_onPopoutBlankDraft
+    @listenTo Actions.draftSendingFailed, @_onDraftSendingFailed
 
     atom.commands.add 'body',
       'application:new-message': => @_onPopoutBlankDraft()
@@ -122,7 +123,7 @@ class DraftStore
   # Public: Look up the sending state of the given draftClientId.
   # In popout windows the existance of the window is the sending state.
   isSendingDraft: (draftClientId) ->
-    return @_draftsSending[draftClientId]?
+    return @_draftsSending[draftClientId] ? false
 
   ###
   Composer Extensions
@@ -483,5 +484,18 @@ class DraftStore
       files = _.reject files, (f) -> f.id is file.id
       session.changes.add({files}, immediate: true)
 
+  _onDraftSendingFailed: ({draftClientId, errorMessage}) ->
+    @_draftsSending[draftClientId] = false
+    @trigger(draftClientId)
+    if atom.isMainWindow()
+      _.defer ->
+        remote = require('remote')
+        dialog = remote.require('dialog')
+        dialog.showMessageBox remote.getCurrentWindow(), {
+          type: 'warning'
+          buttons: ['Okay'],
+          message: "Error"
+          detail: errorMessage
+        }
 
 module.exports = new DraftStore()

src/flux/tasks/send-draft.coffee

@@ -87,8 +87,11 @@ class SendDraftTask extends Task
         body.reply_to_message_id = null
         return @_send(body)
       else if err.statusCode in NylasAPI.PermanentErrorCodes
-        msg = err.message ? "Your draft could not be sent."
-        Actions.composePopoutDraft(@draftClientId, {errorMessage: msg})
+        msg = "Your draft could not be sent. Please check your network connection and try again."
+        if @fromPopout
+          Actions.composePopoutDraft(@draftClientId, {errorMessage: msg})
+        else
+          Actions.draftSendingFailed({draftClientId: @draftClientId, errorMessage: msg})
         return Promise.resolve(Task.Status.Finished)
       else
         return Promise.resolve(Task.Status.Retry)