Fixed issue #874 - Misconfigured Trigger can cause silent failures/backtrace.

app/models/concerns/validates_condition.rb

@@ -0,0 +1,34 @@
+# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
+module ValidatesCondition
+  extend ActiveSupport::Concern
+
+  included do
+    before_create :validate_condition
+    before_update :validate_condition
+  end
+
+  def validate_condition
+    # use Marshal to do a deep copy of the condition hash
+    validate_condition = Marshal.load(Marshal.dump(condition))
+
+    # check if a valid condition got inserted.
+    validate_condition.delete('ticket.action')
+    validate_condition.each do |key, value|
+      next if !value
+      next if !value['operator']
+      next if !value['operator']['has changed']
+
+      validate_condition.delete(key)
+    end
+
+    validate_condition['ticket.id'] = {
+      operator: 'is',
+      value: 1,
+    }
+
+    ticket_count, tickets = Ticket.selectors(validate_condition, 1, User.find(1))
+    return if ticket_count.present?
+
+    raise Exceptions::UnprocessableEntity, 'Invalid ticket selector conditions'
+  end
+end

app/models/job.rb

@@ -2,6 +2,7 @@
 
 class Job < ApplicationModel
   include NotifiesClients
+  include ValidatesCondition
 
   load 'job/assets.rb'
   include Job::Assets

app/models/overview.rb

@@ -3,6 +3,7 @@
 class Overview < ApplicationModel
   include NotifiesClients
   include LatestChangeObserved
+  include ValidatesCondition
 
   load 'overview/assets.rb'
   include Overview::Assets

app/models/sla.rb

@@ -2,6 +2,7 @@
 
 class Sla < ApplicationModel
   include NotifiesClients
+  include ValidatesCondition
 
   load 'sla/assets.rb'
   include Sla::Assets

app/models/transaction/trigger.rb

@@ -130,6 +130,7 @@ class Transaction::Trigger
 
         # verify is condition is matching
         ticket_count, tickets = Ticket.selectors(condition, 1)
+        next if ticket_count.blank?
         next if ticket_count.zero?
         next if tickets.first.id != ticket.id
         user_id = ticket.updated_by_id

app/models/trigger.rb

@@ -1,6 +1,8 @@
 # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
 
 class Trigger < ApplicationModel
+  include ValidatesCondition
+
   store     :condition
   store     :perform
   validates :name, presence: true

test/unit/job_test.rb

@@ -330,7 +330,7 @@ class JobTest < ActiveSupport::TestCase
         },
       },
       condition: {
-        'ticket.state_id' => { 'operator' => 'is', 'value' => '' },
+        'ticket.state_id' => { 'operator' => 'is', 'value' => '9999' },
         'ticket.created_at' => { 'operator' => 'before (relative)', 'value' => '2', 'range' => 'day' },
       },
       perform: {
@@ -355,81 +355,6 @@ class JobTest < ActiveSupport::TestCase
     ticket2_later = Ticket.find(ticket2.id)
     assert_equal('new', ticket2_later.state.name)
     assert_equal(ticket2.updated_at.to_s, ticket2_later.updated_at.to_s)
-
-    job1 = Job.create_or_update(
-      name: 'Test Job1',
-      timeplan: {
-        days: {
-          Mon: true,
-          Tue: true,
-          Wed: true,
-          Thu: true,
-          Fri: true,
-          Sat: true,
-          Sun: true,
-        },
-        hours: {
-          0 => true,
-          1 => true,
-          2 => true,
-          3 => true,
-          4 => true,
-          5 => true,
-          6 => true,
-          7 => true,
-          8 => true,
-          9 => true,
-          10 => true,
-          11 => true,
-          12 => true,
-          13 => true,
-          14 => true,
-          15 => true,
-          16 => true,
-          17 => true,
-          18 => true,
-          19 => true,
-          20 => true,
-          21 => true,
-          22 => true,
-          23 => true,
-        },
-        minutes: {
-          0 => true,
-          10 => true,
-          20 => true,
-          30 => true,
-          40 => true,
-          50 => true,
-        },
-      },
-      condition: {
-        'ticket.state_id' => { 'operator' => 'is' },
-        'ticket.created_at' => { 'operator' => 'before (relative)', 'value' => '2', 'range' => 'day' },
-      },
-      perform: {
-        'ticket.state_id' => { 'value' => Ticket::State.lookup(name: 'closed').id.to_s }
-      },
-      disable_notification: true,
-      last_run_at: nil,
-      updated_at: Time.zone.now - 15.minutes,
-      active: true,
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert(job1.executable?)
-    assert(job1.in_timeplan?)
-    Job.run
-
-    # verify changes on tickets
-    ticket1_later = Ticket.find(ticket1.id)
-    assert_equal('new', ticket1_later.state.name)
-    assert_equal(ticket1.updated_at.to_s, ticket1_later.updated_at.to_s)
-
-    ticket2_later = Ticket.find(ticket2.id)
-    assert_equal('new', ticket2_later.state.name)
-    assert_equal(ticket2.updated_at.to_s, ticket2_later.updated_at.to_s)
-
   end
 
   test 'case 3' do

test/unit/ticket_trigger_test.rb

@@ -2446,4 +2446,29 @@ class TicketTriggerTest < ActiveSupport::TestCase
 
   end
 
+  test '1 empty condition should not create errors' do
+    assert_raises(Exception) {
+      trigger_empty = Trigger.create_or_update(
+        name: 'aaa loop check',
+        condition: {
+          'ticket.number' => {
+            'operator' => 'contains',
+            'value'    => '',
+          },
+        },
+        perform: {
+          'notification.email' => {
+            'body' => 'some lala',
+            'recipient' => 'ticket_customer',
+            'subject' => 'Thanks for your inquiry - loop check (#{ticket.title})!',
+          },
+        },
+        disable_notification: true,
+        active: true,
+        created_by_id: 1,
+        updated_by_id: 1,
+      )
+    }
+  end
+
 end