From d911793befc8b183f48b0d9f930db801f771bdc3 Mon Sep 17 00:00:00 2001
From: Ben Gotow <bengotow@gmail.com>
Date: Mon, 28 Dec 2015 18:10:17 -0800
Subject: [PATCH] fix(mail-rules): Catch and disable mail rules building bad
 actions

---
 .../lib/notifications-store.coffee            |  5 +-
 .../lib/tabs/preferences-mail-rules.cjsx      | 20 ++++-
 .../stylesheets/preferences-mail-rules.less   | 18 +++++
 .../lib/developer-bar-long-poll-item.cjsx     |  4 +-
 .../worker-ui/lib/developer-bar-store.coffee  |  7 +-
 .../worker-ui/lib/developer-bar.cjsx          |  2 +-
 spec/mail-rules-processor-spec.coffee         | 10 ++-
 src/components/scenario-editor-row.cjsx       | 13 ++-
 src/flux/actions.coffee                       |  4 +-
 src/flux/stores/mail-rules-store.coffee       | 45 +++++++++++
 src/flux/tasks/change-folder-task.coffee      |  3 +
 src/flux/tasks/change-labels-task.coffee      |  7 +-
 src/mail-rules-processor.coffee               | 81 ++++++++++++-------
 13 files changed, 169 insertions(+), 50 deletions(-)

diff --git a/internal_packages/notifications/lib/notifications-store.coffee b/internal_packages/notifications/lib/notifications-store.coffee
index 74da856c4..05b703de8 100644
--- a/internal_packages/notifications/lib/notifications-store.coffee
+++ b/internal_packages/notifications/lib/notifications-store.coffee
@@ -47,8 +47,9 @@ NotificationStore = Reflux.createStore
       @_removeNotification(notification)() if action.dismisses
     @listenTo Actions.postNotification, (data) =>
       @_postNotification(new Notification(data))
-    @listenTo Actions.multiWindowNotification, (data={}, context={}) =>
-      @_postNotification(new Notification(data)) if @_inWindowContext(context)
+    @listenTo Actions.dismissNotificationsMatching, (criteria) =>
+      @_notifications = _.reject @_notifications, (n) -> _.isMatch(n, criteria)
+      @trigger()
 
   ######### PUBLIC #######################################################
 
diff --git a/internal_packages/preferences/lib/tabs/preferences-mail-rules.cjsx b/internal_packages/preferences/lib/tabs/preferences-mail-rules.cjsx
index 8d074ef77..7d920db81 100644
--- a/internal_packages/preferences/lib/tabs/preferences-mail-rules.cjsx
+++ b/internal_packages/preferences/lib/tabs/preferences-mail-rules.cjsx
@@ -84,7 +84,12 @@ class PreferencesMailRules extends React.Component
       className="rule-list"
       showEditIcon={true}
       items={@state.rules}
-      itemContent={ (rule) -> rule.name }
+      itemContent={ (rule) ->
+        if rule.disabled
+          return <div className="item-rule-disabled">{rule.name}</div>
+        else
+          return rule.name
+      }
       onCreateItem={@_onAddRule}
       onDeleteItem={@_onDeleteRule}
       onItemEdited={@_onRuleNameEdited}
@@ -96,6 +101,7 @@ class PreferencesMailRules extends React.Component
 
     if rule
       <ScrollRegion className="rule-detail">
+        {@_renderDetailDisabledNotice()}
         <div className="inner">
           <span>If </span>
           <select value={rule.conditionMode} onChange={@_onRuleConditionModeEdited}>
@@ -122,6 +128,15 @@ class PreferencesMailRules extends React.Component
         <div className="no-selection">Create a rule or select one to get started</div>
       </div>
 
+  _renderDetailDisabledNotice: =>
+    return false unless @state.selectedRule.disabled
+    <div className="disabled-reason">
+      <button className="btn" onClick={@_onRuleEnabled}>Enable</button>
+      This rule has been disabled. Make sure the actions below are valid
+      and re-enable the rule.
+      <div>({@state.selectedRule.disabledReason})</div>
+    </div>
+
   _renderTasks: =>
     return false if @state.tasks.length is 0
 
@@ -170,6 +185,9 @@ class PreferencesMailRules extends React.Component
   _onRuleConditionModeEdited: (event) =>
     Actions.updateMailRule(@state.selectedRule.id, {conditionMode: event.target.value})
 
+  _onRuleEnabled: =>
+    Actions.updateMailRule(@state.selectedRule.id, {disabled: false, disabledReason: null})
+
   _onRulesChanged: =>
     next = @stateForAccount(@props.accountId)
     nextRules = next.rules
diff --git a/internal_packages/preferences/stylesheets/preferences-mail-rules.less b/internal_packages/preferences/stylesheets/preferences-mail-rules.less
index 7556e307b..4d3c50776 100644
--- a/internal_packages/preferences/stylesheets/preferences-mail-rules.less
+++ b/internal_packages/preferences/stylesheets/preferences-mail-rules.less
@@ -10,6 +10,14 @@
       min-width:200px;
       height: 350px;
     }
+    .item-rule-disabled {
+      color: @error-color;
+      padding: 4px 10px;
+      border-bottom: 1px solid @border-color-divider;
+    }
+    .selected .item-rule-disabled {
+      color: @component-active-bg;
+    }
   }
   .rule-detail {
     flex: 1;
@@ -18,6 +26,16 @@
     border: 1px solid @border-color-divider;
     border-left: 0;
 
+    .disabled-reason {
+      padding: @padding-base-vertical * 2 @padding-base-vertical * 2;
+      background-color: fade(@background-color-error, 30%);
+      border-bottom: 1px solid @background-color-error;
+      margin-bottom: @padding-base-vertical;
+      .btn {
+        margin-left:@padding-base-horizontal * 2;
+        float:right;
+      }
+    }
     .inner {
       padding: @padding-base-vertical @padding-base-horizontal;
     }
diff --git a/internal_packages/worker-ui/lib/developer-bar-long-poll-item.cjsx b/internal_packages/worker-ui/lib/developer-bar-long-poll-item.cjsx
index be31531fb..84690be23 100644
--- a/internal_packages/worker-ui/lib/developer-bar-long-poll-item.cjsx
+++ b/internal_packages/worker-ui/lib/developer-bar-long-poll-item.cjsx
@@ -26,9 +26,9 @@ class DeveloperBarLongPollItem extends React.Component
     classname = "item"
     right = @props.item.cursor
 
-    if @props.item.ignoredBecause
+    if @props.ignoredBecause
       classname += " ignored"
-      right = @props.item.ignoredBecause + " - " + right
+      right = @props.ignoredBecause + " - " + right
 
     <div className={classname} onClick={ => @setState expanded: not @state?.expanded}>
       <div className="cursor">{right}</div>
diff --git a/internal_packages/worker-ui/lib/developer-bar-store.coffee b/internal_packages/worker-ui/lib/developer-bar-store.coffee
index dd22ce0ab..49f924d6a 100644
--- a/internal_packages/worker-ui/lib/developer-bar-store.coffee
+++ b/internal_packages/worker-ui/lib/developer-bar-store.coffee
@@ -40,15 +40,12 @@ class DeveloperBarStore extends NylasStore
 
   longPollState: -> @_longPollState
 
-  longPollHistory: ->
-    # We can't use Utils.deepClone because the deltas contain circular references
-    # See delta.attributes._delta = delta
-    JSON.parse(JSON.stringify(@_longPollHistory))
+  longPollHistory: -> @_longPollHistory
 
   ########### PRIVATE ####################################################
 
   triggerThrottled: ->
-    @_triggerThrottled ?= _.throttle(@trigger, 100)
+    @_triggerThrottled ?= _.throttle(@trigger, 150)
     @_triggerThrottled()
 
   _setStoreDefaults: ->
diff --git a/internal_packages/worker-ui/lib/developer-bar.cjsx b/internal_packages/worker-ui/lib/developer-bar.cjsx
index aad65867b..2a03cf4a2 100644
--- a/internal_packages/worker-ui/lib/developer-bar.cjsx
+++ b/internal_packages/worker-ui/lib/developer-bar.cjsx
@@ -77,7 +77,7 @@ class DeveloperBar extends React.Component
 
     else if @state.section == 'long-polling'
       itemDivs = @state.longPollHistory.filter(matchingFilter).map (item) ->
-        <DeveloperBarLongPollItem item={item} key={"#{item.cursor}-#{item.timestamp}"}/>
+        <DeveloperBarLongPollItem item={item} ignoredBecause={item.ignoredBecause} key={"#{item.cursor}-#{item.timestamp}"}/>
       expandedDiv = <div className="expanded-section long-polling">{itemDivs}</div>
 
     else if @state.section == 'queue'
diff --git a/spec/mail-rules-processor-spec.coffee b/spec/mail-rules-processor-spec.coffee
index b2837e098..79d1de778 100644
--- a/spec/mail-rules-processor-spec.coffee
+++ b/spec/mail-rules-processor-spec.coffee
@@ -1,7 +1,9 @@
 _ = require 'underscore'
 {Message,
  Contact,
+ Thread,
  File,
+ DatabaseStore,
  TaskQueueStatusStore,
  Actions} = require 'nylas-exports'
 
@@ -142,14 +144,14 @@ describe "MailRulesProcessor", ->
     it "should queue tasks for messages", ->
       spyOn(TaskQueueStatusStore, 'waitForPerformLocal')
       spyOn(Actions, 'queueTask')
-
+      spyOn(DatabaseStore, 'findBy').andReturn(Promise.resolve({}))
       Tests.forEach ({rule}) =>
         TaskQueueStatusStore.waitForPerformLocal.reset()
         Actions.queueTask.reset()
 
-        messageSpy = jasmine.createSpy('message')
-        threadSpy = jasmine.createSpy('thread')
-        response = MailRulesProcessor._applyRuleToMessage(rule, messageSpy, threadSpy)
+        message = new Message({accountId: rule.accountId})
+        thread = new Thread({accountId: rule.accountId})
+        response = MailRulesProcessor._applyRuleToMessage(rule, message, thread)
         expect(response instanceof Promise).toBe(true)
 
         waitsForPromise =>
diff --git a/src/components/scenario-editor-row.cjsx b/src/components/scenario-editor-row.cjsx
index b2933b437..f15166741 100644
--- a/src/components/scenario-editor-row.cjsx
+++ b/src/components/scenario-editor-row.cjsx
@@ -6,6 +6,7 @@ Rx = require 'rx-lite'
 
 {Comparator, Template} = require './scenario-editor-models'
 
+SOURCE_SELECT_NULL = 'NULL'
 
 class SourceSelect extends React.Component
   @displayName: 'SourceSelect'
@@ -40,12 +41,22 @@ class SourceSelect extends React.Component
   render: =>
     options = @state.options
 
-    <select value={@props.value} onChange={@props.onChange}>
+    # The React <select> component won't select the correct option if the value
+    # is null or undefined - it just leaves the selection whatever it was in the
+    # previous render. To work around this, we coerce null/undefined to SOURCE_SELECT_NULL.
+
+    <select value={@props.value || SOURCE_SELECT_NULL} onChange={@_onChange}>
+      <option key={SOURCE_SELECT_NULL} value={SOURCE_SELECT_NULL}></option>
       { @state.options.map ({value, name}) =>
         <option key={value} value={value}>{name}</option>
       }
     </select>
 
+  _onChange: (event) =>
+    value = event.target.value
+    value = null if value is SOURCE_SELECT_NULL
+    @props.onChange(target: {value})
+
 class ScenarioEditorRow extends React.Component
   @displayName: 'ScenarioEditorRow'
   @propTypes:
diff --git a/src/flux/actions.coffee b/src/flux/actions.coffee
index 2afbf1790..5a998c1bd 100644
--- a/src/flux/actions.coffee
+++ b/src/flux/actions.coffee
@@ -79,7 +79,6 @@ class Actions
   @downloadStateChanged: ActionScopeGlobal
   @linkFileToUpload: ActionScopeGlobal
   @fileUploaded: ActionScopeGlobal
-  @multiWindowNotification: ActionScopeGlobal
   @sendDraftSuccess: ActionScopeGlobal
   @sendToAllWindows: ActionScopeGlobal
   @draftSendingFailed: ActionScopeGlobal
@@ -424,6 +423,8 @@ class Actions
   ###
   @postNotification: ActionScopeGlobal
 
+  @dismissNotificationsMatching: ActionScopeGlobal
+
   ###
   Public: Listen to this action to handle user interaction with notifications you
   published via `postNotification`.
@@ -511,6 +512,7 @@ class Actions
   @addMailRule: ActionScopeWindow
   @updateMailRule: ActionScopeWindow
   @deleteMailRule: ActionScopeWindow
+  @disableMailRule: ActionScopeWindow
 
 # Read the actions we declared on the dummy Actions object above
 # and translate them into Reflux Actions
diff --git a/src/flux/stores/mail-rules-store.coffee b/src/flux/stores/mail-rules-store.coffee
index 02b16f3db..e9145124b 100644
--- a/src/flux/stores/mail-rules-store.coffee
+++ b/src/flux/stores/mail-rules-store.coffee
@@ -1,7 +1,9 @@
 NylasStore = require 'nylas-store'
 _ = require 'underscore'
 Rx = require 'rx-lite'
+AccountStore = require './account-store'
 DatabaseStore = require './database-store'
+TaskQueueStatusStore = require './task-queue-status-store'
 Utils = require '../models/utils'
 Actions = require '../actions'
 
@@ -19,6 +21,8 @@ class MailRulesStore extends NylasStore
     @listenTo Actions.addMailRule, @_onAddMailRule
     @listenTo Actions.deleteMailRule, @_onDeleteMailRule
     @listenTo Actions.updateMailRule, @_onUpdateMailRule
+    @listenTo Actions.disableMailRule, @_onDisableMailRule
+    @listenTo Actions.notificationActionTaken, @_onNotificationActionTaken
 
   rules: =>
     @_rules
@@ -38,6 +42,7 @@ class MailRulesStore extends NylasStore
       conditionMode: ConditionMode.All
       conditions: [ConditionTemplates[0].createDefaultInstance()]
       actions: [ActionTemplates[0].createDefaultInstance()]
+      disabled: false
 
     unless properties.accountId
       throw new Error("AddMailRule: you must provide an account id.")
@@ -52,10 +57,50 @@ class MailRulesStore extends NylasStore
     @_saveMailRules()
     @trigger()
 
+  _onDisableMailRule: (id, reason) =>
+    existing = _.find @_rules, (f) -> id is f.id
+    return if not existing or existing.disabled is true
+
+    Actions.postNotification
+      message: "We were unable to run your mail rules - one or more rules have been disabled."
+      type: "error"
+      tag: 'mail-rule-failure'
+      sticky: true
+      actions: [{
+        label: 'Hide'
+        dismisses: true
+        id: 'hide'
+      },{
+        label: 'View Rules'
+        dismisses: true
+        default: true
+        id: 'mail-rule-failure:view-rules'
+      }]
+
+    # Disable the task
+    existing.disabled = true
+    existing.disabledReason = reason
+    @_saveMailRules()
+
+    # Cancel all bulk processing jobs
+    for task in TaskQueueStatusStore.tasksMatching(ReprocessMailRulesTask, {})
+      Actions.dequeueTask(task.id)
+
+    @trigger()
+
+  _onNotificationActionTaken: ({notification, action}) =>
+    return unless NylasEnv.isMainWindow()
+    if action.id is 'mail-rule-failure:view-rules'
+      Actions.switchPreferencesTab('Mail Rules', {accountId: AccountStore.current().id})
+      Actions.openPreferences()
+
   _saveMailRules: =>
     @_saveMailRulesDebounced ?= _.debounce =>
       DatabaseStore.inTransaction (t) =>
         t.persistJSONBlob(RulesJSONBlobKey, @_rules)
+
+      if not _.findWhere(@_rules, {disabled: true})
+        Actions.dismissNotificationsMatching({tag: 'mail-rule-failure'})
     ,1000
     @_saveMailRulesDebounced()
 
diff --git a/src/flux/tasks/change-folder-task.coffee b/src/flux/tasks/change-folder-task.coffee
index dd9b51051..83daad5ac 100644
--- a/src/flux/tasks/change-folder-task.coffee
+++ b/src/flux/tasks/change-folder-task.coffee
@@ -69,6 +69,9 @@ class ChangeFolderTask extends ChangeMailTask
       @threads = _.compact(threads)
       @messages = _.compact(messages)
 
+      if not @folder
+        return Promise.reject(new Error("The specified folder could not be found."))
+
       # The base class does the heavy lifting and calls changesToModel
       return super
 
diff --git a/src/flux/tasks/change-labels-task.coffee b/src/flux/tasks/change-labels-task.coffee
index 195769ed1..2439beb71 100644
--- a/src/flux/tasks/change-labels-task.coffee
+++ b/src/flux/tasks/change-labels-task.coffee
@@ -56,10 +56,13 @@ class ChangeLabelsTask extends ChangeMailTask
       messages: DatabaseStore.modelify(Message, @messages)
 
     ).then ({labelsToAdd, labelsToRemove, threads, messages}) =>
+      if _.any([].concat(labelsToAdd, labelsToRemove), _.isUndefined)
+        return Promise.reject(new Error("One or more of the specified labels could not be found."))
+
       # Remove any objects we weren't able to find. This can happen pretty easily
       # if you undo an action and other things have happened.
-      @labelsToAdd = _.compact(labelsToAdd)
-      @labelsToRemove = _.compact(labelsToRemove)
+      @labelsToAdd = labelsToAdd
+      @labelsToRemove = labelsToRemove
       @threads = _.compact(threads)
       @messages = _.compact(messages)
 
diff --git a/src/mail-rules-processor.coffee b/src/mail-rules-processor.coffee
index ee8808ad7..9ae3279d3 100644
--- a/src/mail-rules-processor.coffee
+++ b/src/mail-rules-processor.coffee
@@ -29,44 +29,51 @@ MailRulesActions =
       name: 'important',
       accountId: thread.accountId
     }).then (important) ->
-      new ChangeLabelsTask(labelsToAdd: [important], threads: [thread])
+      return Promise.reject(new Error("Could not find `important` label")) unless important
+      return new ChangeLabelsTask(labelsToAdd: [important], threads: [thread])
 
   moveToTrash: (message, thread) ->
-    account = AccountStore.itemWithId(thread.accountId)
-    CategoryClass = account.categoryClass()
-    TaskClass = if CategoryClass is Label then ChangeLabelsTask else ChangeFolderTask
+    if AccountStore.itemWithId(thread.accountId).categoryClass() is Label
+      return MailRulesActions._applyStandardLabelRemovingInbox(message, thread, 'trash')
+    else
+      DatabaseStore.findBy(Folder, { name: 'trash', accountId: thread.accountId }).then (folder) ->
+        return Promise.reject(new Error("The folder could not be found.")) unless folder
+        return new ChangeFolderTask(folder: folder, threads: [thread])
 
-    Promise.props(
-      inbox: DatabaseStore.findBy(CategoryClass, { name: 'inbox', accountId: thread.accountId })
-      trash: DatabaseStore.findBy(CategoryClass, { name: 'trash', accountId: thread.accountId })
-    ).then ({inbox, trash}) ->
-      new TaskClass
-        labelsToRemove: [inbox]
-        labelsToAdd: [trash]
-        threads: [thread]
-
-  markAsRead: (message, thread, value) ->
+  markAsRead: (message, thread) ->
     new ChangeUnreadTask(unread: false, threads: [thread])
 
-  star: (message, thread, value) ->
+  star: (message, thread) ->
     new ChangeStarredTask(starred: true, threads: [thread])
 
-  applyLabel: (message, thread, value) ->
-    new ChangeLabelsTask(labelsToAdd: [value], threads: [thread])
+  changeFolder: (message, thread, value) ->
+    return Promise.reject(new Error("A folder is required.")) unless value
+    DatabaseStore.findBy(Folder, { id: value, accountId: thread.accountId }).then (folder) ->
+      return Promise.reject(new Error("The folder could not be found.")) unless folder
+      return new ChangeFolderTask(folder: folder, threads: [thread])
 
-  applyLabelArchive: (message, thread, value) ->
+  applyLabel: (message, thread, value) ->
+    return Promise.reject(new Error("A label is required.")) unless value
+    DatabaseStore.findBy(Label, { id: value, accountId: thread.accountId }).then (label) ->
+      return Promise.reject(new Error("The label could not be found.")) unless label
+      return new ChangeLabelsTask(labelsToAdd: [label], threads: [thread])
+
+  applyLabelArchive: (message, thread) ->
+    return MailRulesActions._applyStandardLabelRemovingInbox(message, thread, 'all')
+
+  # Helpers for other actions
+
+  _applyStandardLabelRemovingInbox: (message, thread, value) ->
     Promise.props(
       inbox: DatabaseStore.findBy(Label, { name: 'inbox', accountId: thread.accountId })
-      all: DatabaseStore.findBy(Label, { name: 'all', accountId: thread.accountId })
-    ).then ({inbox, all}) ->
-      new ChangeLabelsTask
+      newLabel: DatabaseStore.findBy(Label, { name: value, accountId: thread.accountId })
+    ).then ({inbox, newLabel}) ->
+      return Promise.reject(new Error("Could not find `inbox` or `#{value}` label")) unless inbox and newLabel
+      return new ChangeLabelsTask
         labelsToRemove: [inbox]
-        labelsToAdd: [all]
+        labelsToAdd: [newLabel]
         threads: [thread]
 
-  changeFolder: (message, thread, value) ->
-    new ChangeFolderTask(folder: value, threads: [thread])
-
 
 class MailRulesProcessor
   constructor: ->
@@ -74,10 +81,12 @@ class MailRulesProcessor
   processMessages: (messages) =>
     return Promise.resolve() unless messages.length > 0
 
+    enabledRules = MailRulesStore.rules().filter (r) -> not r.disabled
+
     # When messages arrive, we process all the messages in parallel, but one
     # rule at a time. This is important, because users can order rules which
     # may do and undo a change. Ie: "Star if from Ben, Unstar if subject is "Bla"
-    Promise.each MailRulesStore.rules(), (rule) =>
+    Promise.each enabledRules, (rule) =>
       matching = messages.filter (message) =>
         @_checkRuleForMessage(rule, message)
 
@@ -107,17 +116,27 @@ class MailRulesProcessor
       template.evaluate(condition, value)
 
   _applyRuleToMessage: (rule, message, thread) =>
-    results = rule.actions.map (action) =>
-      MailRulesActions[action.templateKey](message, thread, action.value)
+    actionPromises = rule.actions.map (action) =>
+      actionFunction = MailRulesActions[action.templateKey]
+      if not actionFunction
+        return Promise.reject(new Error("#{action.templateKey} is not a supported action."))
+      return actionFunction(message, thread, action.value)
 
-    Promise.all(results).then (results) ->
+    Promise.all(actionPromises).then (actionResults) ->
       performLocalPromises = []
 
-      tasks = results.filter (r) -> r instanceof Task
-      tasks.forEach (task) ->
+      actionTasks = actionResults.filter (r) -> r instanceof Task
+      actionTasks.forEach (task) ->
         performLocalPromises.push TaskQueueStatusStore.waitForPerformLocal(task)
         Actions.queueTask(task)
 
       Promise.all(performLocalPromises)
 
+    .catch (err) ->
+      # Errors can occur if a mail rule specifies an invalid label or folder, etc.
+      # Disable the rule. Disable the mail rule so the failure is reflected in the
+      # interface.
+      Actions.disableMailRule(rule.id, err.toString())
+      return Promise.resolve()
+
 module.exports = new MailRulesProcessor