From 4ba2744689f4da75b7ea6dafb7fa9de98371f9db Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 26 Jul 2017 20:46:31 +0200 Subject: [PATCH] Notify agent if mail delivery failed. Enhancement to issue #1198. --- app/models/channel.rb | 6 ------ .../communicate_email/background_job.rb | 15 +++++++++------ .../ticket/online_notification_seen.rb | 7 +++---- .../background_job.rb | 2 +- app/models/observer/transaction.rb | 4 ++++ app/models/ticket.rb | 2 +- app/models/transaction/notification.rb | 10 ++++++---- lib/sessions.rb | 6 +++--- test/integration/email_deliver_test.rb | 19 ++++++++++++++++++- 9 files changed, 45 insertions(+), 26 deletions(-) diff --git a/app/models/channel.rb b/app/models/channel.rb index 2decd642ef4e..3a13312a6898 100644 --- a/app/models/channel.rb +++ b/app/models/channel.rb @@ -223,12 +223,6 @@ def self.stream 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] diff --git a/app/models/observer/ticket/article/communicate_email/background_job.rb b/app/models/observer/ticket/article/communicate_email/background_job.rb index e3fdfb053ba0..6e3c51454198 100644 --- a/app/models/observer/ticket/article/communicate_email/background_job.rb +++ b/app/models/observer/ticket/article/communicate_email/background_job.rb @@ -124,7 +124,7 @@ def log_error(local_record, message, channel = nil) 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 @@ def log_error(local_record, message, channel = nil) 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 @@ def log_error(local_record, message, channel = nil) 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 @@ def max_attempts 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 diff --git a/app/models/observer/ticket/online_notification_seen.rb b/app/models/observer/ticket/online_notification_seen.rb index ec8883d027fe..dee1774ef367 100644 --- a/app/models/observer/ticket/online_notification_seen.rb +++ b/app/models/observer/ticket/online_notification_seen.rb @@ -16,12 +16,11 @@ def after_update(record) 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 diff --git a/app/models/observer/ticket/online_notification_seen/background_job.rb b/app/models/observer/ticket/online_notification_seen/background_job.rb index 90e931ff41d7..237c3adf942e 100644 --- a/app/models/observer/ticket/online_notification_seen/background_job.rb +++ b/app/models/observer/ticket/online_notification_seen/background_job.rb @@ -16,7 +16,7 @@ def perform next if seen == notification.seen notification.seen = true notification.updated_by_id = @user_id - notification.save + notification.save! } end end diff --git a/app/models/observer/transaction.rb b/app/models/observer/transaction.rb index e20e5965d662..97651de8f708 100644 --- a/app/models/observer/transaction.rb +++ b/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 diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 5f4fee0bb82f..039ee689a3ff 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -335,7 +335,7 @@ def online_notification_seen_state(user_id_check = nil) 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 diff --git a/app/models/transaction/notification.rb b/app/models/transaction/notification.rb index ba0267b9c6e5..dcc91531547e 100644 --- a/app/models/transaction/notification.rb +++ b/app/models/transaction/notification.rb @@ -38,8 +38,10 @@ def perform # 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 @@ def perform # 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 @@ def perform 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) diff --git a/lib/sessions.rb b/lib/sessions.rb index 929213bff186..c6e3d44358e5 100644 --- a/lib/sessions.rb +++ b/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 @@ def self.broadcast(data, recipient = 'authenticated', sender_user_id = nil) 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 diff --git a/test/integration/email_deliver_test.rb b/test/integration/email_deliver_test.rb index 440dc550b267..7fa40989709e 100644 --- a/test/integration/email_deliver_test.rb +++ b/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