Notify agent if mail delivery failed. Enhancement to issue #1198.

app/models/channel.rb

@@ -223,12 +223,6 @@ send via account
 
   def deliver(mail_params, notification = false)
 
-    # ignore notifications in developer mode
-    if notification == true && Setting.get('developer_mode') == true
-      logger.info "Do not send notification #{mail_params.inspect} because of enabled developer_mode"
-      return
-    end
-
     adapter         = options[:adapter]
     adapter_options = options
     if options[:outbound] && options[:outbound][:adapter]

app/models/observer/ticket/article/communicate_email/background_job.rb

@@ -124,7 +124,7 @@ class Observer::Ticket::Article::CommunicateEmail::BackgroundJob
     local_record.preferences['delivery_status'] = 'fail'
     local_record.preferences['delivery_status_message'] = message
     local_record.preferences['delivery_status_date'] = Time.zone.now
-    local_record.save
+    local_record.save!
     Rails.logger.error message
 
     if local_record.preferences['delivery_retry'] > 3
@@ -141,7 +141,10 @@ class Observer::Ticket::Article::CommunicateEmail::BackgroundJob
         recipient_list += local_record[key]
       }
 
-      Ticket::Article.create(
+      # reopen ticket and notify agent
+      Observer::Transaction.reset
+      UserInfo.current_user_id = 1
+      Ticket::Article.create!(
         ticket_id: local_record.ticket_id,
         content_type: 'text/plain',
         body: "Unable to send email to '#{recipient_list}': #{message}",
@@ -151,14 +154,14 @@ class Observer::Ticket::Article::CommunicateEmail::BackgroundJob
         preferences: {
           delivery_article_id_related: local_record.id,
           delivery_message: true,
+          notification: true,
         },
-        updated_by_id: 1,
-        created_by_id: 1,
       )
-
       ticket       = Ticket.find(local_record.ticket_id)
       ticket.state = Ticket::State.find_by(default_follow_up: true)
       ticket.save!
+      Observer::Transaction.commit
+      UserInfo.current_user_id = nil
     end
 
     raise message
@@ -170,7 +173,7 @@ class Observer::Ticket::Article::CommunicateEmail::BackgroundJob
 
   def reschedule_at(current_time, attempts)
     if Rails.env.production?
-      return current_time + attempts * 20.seconds
+      return current_time + attempts * 25.seconds
     end
     current_time + 5.seconds
   end

app/models/observer/ticket/online_notification_seen.rb

@@ -16,12 +16,11 @@ class Observer::Ticket::OnlineNotificationSeen < ActiveRecord::Observer
   def _check(record)
 
     # return if we run import mode
-    return if Setting.get('import_mode')
+    return false if Setting.get('import_mode')
 
     # set seen only if state has changes
-    return if !record.changes
-    return if record.changes.empty?
-    return if !record.changes['state_id']
+    return false if record.changes.blank?
+    return false if record.changes['state_id'].blank?
 
     # check if existing online notifications for this ticket should be set to seen
     return true if !record.online_notification_seen_state

app/models/observer/ticket/online_notification_seen/background_job.rb

@@ -16,7 +16,7 @@ class Observer::Ticket::OnlineNotificationSeen::BackgroundJob
         next if seen == notification.seen
         notification.seen = true
         notification.updated_by_id = @user_id
-        notification.save
+        notification.save!
       }
     end
   end

app/models/observer/transaction.rb

@@ -3,6 +3,10 @@
 class Observer::Transaction < ActiveRecord::Observer
   observe :ticket, 'ticket::_article', :user, :organization, :tag
 
+  def self.reset
+    EventBuffer.reset('transaction')
+  end
+
   def self.commit(params = {})
 
     # add attribute of interface handle (e. g. to send (no) notifications if a agent

app/models/ticket.rb

@@ -335,7 +335,7 @@ returns
     state      = Ticket::State.lookup(id: state_id)
     state_type = Ticket::StateType.lookup(id: state.state_type_id)
 
-    # always to set unseen for ticket owner
+    # always to set unseen for ticket owner and users which did not the update
     if state_type.name != 'merged'
       if user_id_check
         return false if user_id_check == owner_id && user_id_check != updated_by_id

app/models/transaction/notification.rb

@@ -38,8 +38,10 @@ class Transaction::Notification
       # ignore notifications
       sender = Ticket::Article::Sender.lookup(id: article.sender_id)
       if sender && sender.name == 'System'
-        return if @item[:changes].empty?
-        article = nil
+        return if @item[:changes].blank? && article.preferences[:notification] != true
+        if article.preferences[:notification] != true
+          article = nil
+        end
       end
     end
 
@@ -78,7 +80,7 @@ class Transaction::Notification
 
       # ignore if no changes has been done
       changes = human_changes(user, ticket)
-      next if @item[:type] == 'update' && !article && (!changes || changes.empty?)
+      next if @item[:type] == 'update' && !article && changes.blank?
 
       # check if today already notified
       if @item[:type] == 'reminder_reached' || @item[:type] == 'escalation' || @item[:type] == 'escalation_warning'
@@ -117,7 +119,7 @@ class Transaction::Notification
           OnlineNotification.remove_by_type('Ticket', ticket.id, 'escalation_warning', user)
 
         # on updates without state changes create unseen messages
-        elsif @item[:type] != 'create' && (!@item[:changes] || @item[:changes].empty? || !@item[:changes]['state_id'])
+        elsif @item[:type] != 'create' && (@item[:changes].blank? || @item[:changes]['state_id'].blank?)
           seen = false
         else
           seen = ticket.online_notification_seen_state(user.id)

lib/sessions.rb

@@ -5,7 +5,7 @@ module Sessions
 
   # get application root directory
   @root = Dir.pwd.to_s
-  if !@root || @root.empty? || @root == '/'
+  if @root.blank? || @root == '/'
     @root = Rails.root
   end
 
@@ -379,8 +379,8 @@ broadcase also not to sender
       next if !session
 
       if recipient != 'public'
-        next if !session[:user]
-        next if !session[:user]['id']
+        next if session[:user].blank?
+        next if session[:user]['id'].blank?
       end
 
       if sender_user_id

test/integration/email_deliver_test.rb

@@ -202,9 +202,13 @@ class EmailDeliverTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
 
+    ticket1.state = Ticket::State.find_by(name: 'closed')
+    ticket1.save
+
     assert_raises(RuntimeError) {
       Scheduler.worker(true)
     }
+    ticket1.reload
 
     article2_lookup = Ticket::Article.find(article2.id)
     assert_equal(2, ticket1.articles.count)
@@ -214,6 +218,7 @@ class EmailDeliverTest < ActiveSupport::TestCase
     assert(article2_lookup.preferences['delivery_status_message'])
 
     Scheduler.worker(true)
+    ticket1.reload
 
     article2_lookup = Ticket::Article.find(article2.id)
     assert_equal(2, ticket1.articles.count)
@@ -221,11 +226,13 @@ class EmailDeliverTest < ActiveSupport::TestCase
     assert_equal('fail', article2_lookup.preferences['delivery_status'])
     assert(article2_lookup.preferences['delivery_status_date'])
     assert(article2_lookup.preferences['delivery_status_message'])
+    assert_equal('closed', ticket1.state.name)
 
     sleep 6
     assert_raises(RuntimeError) {
       Scheduler.worker(true)
     }
+    ticket1.reload
 
     article2_lookup = Ticket::Article.find(article2.id)
     assert_equal(2, ticket1.articles.count)
@@ -233,30 +240,39 @@ class EmailDeliverTest < ActiveSupport::TestCase
     assert_equal('fail', article2_lookup.preferences['delivery_status'])
     assert(article2_lookup.preferences['delivery_status_date'])
     assert(article2_lookup.preferences['delivery_status_message'])
+    assert_equal('closed', ticket1.state.name)
 
     Scheduler.worker(true)
+    ticket1.reload
+
     article2_lookup = Ticket::Article.find(article2.id)
     assert_equal(2, ticket1.articles.count)
     assert_equal(2, article2_lookup.preferences['delivery_retry'])
     assert_equal('fail', article2_lookup.preferences['delivery_status'])
     assert(article2_lookup.preferences['delivery_status_date'])
     assert(article2_lookup.preferences['delivery_status_message'])
+    assert_equal('closed', ticket1.state.name)
 
     sleep 11
     assert_raises(RuntimeError) {
       Scheduler.worker(true)
     }
+    ticket1.reload
+
     article2_lookup = Ticket::Article.find(article2.id)
     assert_equal(2, ticket1.articles.count)
     assert_equal(3, article2_lookup.preferences['delivery_retry'])
     assert_equal('fail', article2_lookup.preferences['delivery_status'])
     assert(article2_lookup.preferences['delivery_status_date'])
     assert(article2_lookup.preferences['delivery_status_message'])
+    assert_equal('closed', ticket1.state.name)
 
     sleep 16
     assert_raises(RuntimeError) {
       Scheduler.worker(true)
     }
+    ticket1.reload
+
     article2_lookup = Ticket::Article.find(article2.id)
     article_delivery_system = ticket1.articles.last
     assert_equal(3, ticket1.articles.count)
@@ -267,7 +283,8 @@ class EmailDeliverTest < ActiveSupport::TestCase
     assert_equal('System', article_delivery_system.sender.name)
     assert_equal(true, article_delivery_system.preferences['delivery_message'])
     assert_equal(article2.id, article_delivery_system.preferences['delivery_article_id_related'])
-
+    assert_equal(true, article_delivery_system.preferences['notification'])
+    assert_equal(Ticket::State.find_by(default_follow_up: true).name, ticket1.state.name)
   end
 
 end