Improved replacement of notification tags (removed eval), added more tests.

app/models/observer/ticket/notification.rb

@@ -33,13 +33,12 @@ class Observer::Ticket::Notification < ActiveRecord::Observer
             :subject   => 'New Ticket (#{ticket.title})',
             :body      => 'Hi #{recipient.firstname},
 
-            a new Ticket (#{ticket.title}) via i18n(#{article.type.name}).
+            a new Ticket (#{ticket.title}) via i18n(#{article.type.name}) by #{ticket.updated_by.fullname}.
 
             Group: #{ticket.group.name}
             Owner: #{ticket.owner.firstname} #{ticket.owner.lastname}
             State: i18n(#{ticket.state.name})
 
-            From: #{article.from}
             <snip>
             #{article.body}
             </snip>
@@ -68,12 +67,11 @@ class Observer::Ticket::Notification < ActiveRecord::Observer
             :subject   => 'Updated (#{ticket.title})',
             :body      => 'Hi #{recipient.firstname},
 
-            updated (#{ticket.title}) via i18n(#{article.type.name}).
+            updated (#{ticket.title}) via i18n(#{article.type.name}) by #{ticket.updated_by.fullname}.
 
             Changes:
             ' + changes + '
 
-            From: #{article.from}
             <snip>
             #{article.body}
             </snip>

app/models/ticket.rb

@@ -53,6 +53,7 @@ class Ticket < ApplicationModel
   belongs_to    :owner,                 :class_name => 'User'
   belongs_to    :customer,              :class_name => 'User'
   belongs_to    :created_by,            :class_name => 'User'
+  belongs_to    :updated_by,            :class_name => 'User'
   belongs_to    :create_article_type,   :class_name => 'Ticket::Article::Type'
   belongs_to    :create_article_sender, :class_name => 'Ticket::Article::Sender'
 

app/models/ticket/article.rb

@@ -12,6 +12,7 @@ class Ticket::Article < ApplicationModel
   belongs_to    :type,        :class_name => 'Ticket::Article::Type'
   belongs_to    :sender,      :class_name => 'Ticket::Article::Sender'
   belongs_to    :created_by,  :class_name => 'User'
+  belongs_to    :updated_by,  :class_name => 'User'
   after_create  :notify_clients_after_create
   after_update  :notify_clients_after_update
   after_destroy :notify_clients_after_destroy

lib/notification_factory.rb

@@ -15,61 +15,70 @@ module NotificationFactory
 
   def self.build(data)
 
-    data[:string].gsub!( / \#\{ \s* ( .+? ) \s* \} /x ) { |placeholder|
+    data[:string].gsub!( / \#\{ \s* ( .+? ) \s* \} /xm ) { |placeholder|
 
       # store possible callback to work with
       # and check if it's valid for execution
-      original_string = $&
+      original_string      = $&
-      callback        = $1
+      callback             = $1
-      callback_valid  = nil
 
-      # use config params
+      object_name   = nil
-      callback.gsub!( /\A config\.( [\w\.]+ ) \z/x ) { |item|
+      object_method = nil
-        callback_valid = true
-        name           = $1
-        item           = "Setting.get('#{name}')"
-      }
-
-      # use object params
-      callback.gsub!( /\A ( [\w]+ )( \.[\w\.]+ ) \z/x ) { |item|
 
+      if callback =~ /\A ( [\w]+ )\.( [\w\.]+ ) \z/x
         object_name   = $1
         object_method = $2
-
+      end
-        if data[:objects][object_name.to_sym]
-          callback_valid = true
-          item           = "data[:objects]['#{object_name}'.to_sym]#{object_method}"
-        else
-          item = item
-        end
-      }
-
-      # use quoted text
-      callback.gsub!( /\A ( data\[:objects\]\['article'\.to_sym\] ) \.body \z/x ) { |item|
-        callback_valid = true
-        if data[:objects][:article].content_type == 'text/html'
-          item           = item + '.html2text.message_quote.chomp'
-        else
-          item           = item + '.word_wrap( :line_width => 82 ).message_quote.chomp'
-        end
-      }
 
       # do validaton, ignore some methodes
-      callback.gsub!( /\.(save|destroy|delete|remove|drop|update|create\(|new|all|where|find)/ix ) { |item|
+      if callback =~ /(`|\.(|\s*)(save|destroy|delete|remove|drop|update\(|update_att|create\(|new|all|where|find))/i
-        callback_valid = false
+        placeholder = "#{original_string} (not allowed)"
-      }
 
-      if callback_valid
+      # get value based on object_name and object_method
-        # replace value
+      elsif object_name && object_method
-        begin
+
-          placeholder = eval callback
+        # use config params
-        rescue Exception => e
+        if object_name == 'config'
-          Rails.logger.error "Evaluation error caused by callback '#{callback}'"
+          placeholder = Setting.get(object_method)
-          Rails.logger.error e.inspect
+
+        # if object_name dosn't exist
+        elsif !data[:objects][object_name.to_sym]
+          placeholder = "\#{#{object_name} / no such object}"
+        else
+          value            = nil
+          object_refs      = data[:objects][object_name.to_sym]
+          object_methods   = object_method.split('.')
+          object_methods_s = ''
+          object_methods.each {|method|
+            if object_methods_s != ''
+              object_methods_s += '.'
+            end
+            object_methods_s += method
+
+            # if method exists
+            if !object_refs.respond_to?( method.to_sym )
+              value = "\#{#{object_name}.#{object_methods_s} / no such method}"
+              break
+            end
+            object_refs = object_refs.send( method.to_sym )
+
+            # add body quote
+            if object_name == 'article' && method == 'body'
+              if data[:objects][:article].content_type == 'text/html'
+                object_refs = object_refs.html2text.message_quote.chomp
+              else
+                object_refs = object_refs.word_wrap( :line_width => 82 ).message_quote.chomp
+              end
+            end
+          }
+          if !value
+            placeholder = object_refs
+          else
+            placeholder = value
+          end
         end
-      else
-        placeholder = original_string
       end
+      placeholder
     }
 
     # translate

test/unit/notification_factory_test.rb

@@ -9,8 +9,8 @@ class NotificationFactoryTest < ActiveSupport::TestCase
       :customer_id   => 2,
       :state         => Ticket::State.lookup( :name => 'new' ),
       :priority      => Ticket::Priority.lookup( :name => '2 normal' ),
-      :updated_by_id => 1,
+      :updated_by_id => 2,
-      :created_by_id => 1,
+      :created_by_id => 2,
     )
     article_plain = Ticket::Article.create(
       :ticket_id     => ticket.id,
@@ -53,6 +53,31 @@ class NotificationFactoryTest < ActiveSupport::TestCase
         :string => '\'i18n(#{ticket.state.name})\' ticket state',
         :result => '\'neu\' ticket state',
       },
+      {
+        :locale => 'de',
+        :string => 'a #{not_existing_object.test}',
+        :result => 'a #{not_existing_object / no such object}',
+      },
+      {
+        :locale => 'de',
+        :string => 'a #{ticket.level1}',
+        :result => 'a #{ticket.level1 / no such method}',
+      },
+      {
+        :locale => 'de',
+        :string => 'a #{ticket.level1.level2}',
+        :result => 'a #{ticket.level1 / no such method}',
+      },
+      {
+        :locale => 'de',
+        :string => 'a #{ticket.title.level2}',
+        :result => 'a #{ticket.title.level2 / no such method}',
+      },
+      {
+        :locale => 'de',
+        :string => 'by #{ticket.updated_by.fullname}',
+        :result => 'by Nicole Braun',
+      },
       {
         :locale => 'de',
         :string => 'Subject #{article.from}, Group: #{ticket.group.name}',
@@ -66,7 +91,7 @@ class NotificationFactoryTest < ActiveSupport::TestCase
       {
         :locale => 'de',
         :string => '\#{puts `ls`}',
-        :result => '\#{puts `ls`}',
+        :result => '\#{puts `ls`} (not allowed)',
       },
     ]
     tests.each { |test|
@@ -165,44 +190,44 @@ class NotificationFactoryTest < ActiveSupport::TestCase
       {
         :locale => 'de',
         :string => '\#{puts `ls`}',
-        :result => '\#{puts `ls`}',
+        :result => '\#{puts `ls`} (not allowed)',
       },
       {
         :locale => 'de',
         :string => 'attack#1 #{article.destroy}',
-        :result => 'attack#1 #{article.destroy}',
+        :result => 'attack#1 #{article.destroy} (not allowed)',
       },
       {
         :locale => 'de',
         :string => 'attack#2 #{Article.where}',
-        :result => 'attack#2 #{Article.where}',
+        :result => 'attack#2 #{Article.where} (not allowed)',
       },
       {
         :locale => 'de',
         :string => 'attack#1 #{article.
         destroy}',
         :result => 'attack#1 #{article.
-        destroy}',
+        destroy} (not allowed)',
       },
       {
         :locale => 'de',
         :string => 'attack#1 #{article.find}',
-        :result => 'attack#1 #{article.find}',
+        :result => 'attack#1 #{article.find} (not allowed)',
       },
       {
         :locale => 'de',
-        :string => 'attack#1 #{article.update}',
+        :string => 'attack#1 #{article.update(:name => "test")}',
-        :result => 'attack#1 #{article.update}',
+        :result => 'attack#1 #{article.update(:name => "test")} (not allowed)',
       },
       {
         :locale => 'de',
         :string => 'attack#1 #{article.all}',
-        :result => 'attack#1 #{article.all}',
+        :result => 'attack#1 #{article.all} (not allowed)',
       },
       {
         :locale => 'de',
         :string => 'attack#1 #{article.delete}',
-        :result => 'attack#1 #{article.delete}',
+        :result => 'attack#1 #{article.delete} (not allowed)',
       },
     ]
     tests.each { |test|