Skip to content

Commit

Permalink
Fixes zammad#2133 - Email notifications sending will be aborted mid-l…
Browse files Browse the repository at this point in the history 
…ist if SMTP error code is received
  • Loading branch information
@mantas
mantas committed Jul 9, 2024
1 parent 3d54e1b commit c240012
Show file tree
Hide file tree
Showing 3 changed files with 193 additions and 124 deletions.
272 changes: 152 additions & 120 deletions app/models/transaction/notification.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@
class Transaction::Notification
include ChecksHumanChanges

# Following SMTP error codes will be handled gracefully.
# They will be logged at info level only and the code will not propagate up the error.
# Other SMTP error codes will stop processing and exit with logging it at error level.
#
# 4xx - temporary issues.
# 52x - permanent receiving server errors.
# 55x - permanent receiving mailbox errors.
SILENCABLE_SMTP_ERROR_CODES = [400..499, 520..529, 550..559].freeze

=begin
{
object: 'Ticket',
Expand Down Expand Up @@ -47,6 +56,15 @@ def perform
end
end

(recipients_and_channels, recipients_reason) = recipients_and_reasons(ticket)

# send notifications
recipients_and_channels.each do |item|
send_to_single_recipient(item, article, ticket, recipients_reason)
end
end

def recipients_and_reasons(ticket)
# find recipients
recipients_and_channels = []
recipients_reason = {}
Expand Down Expand Up @@ -102,147 +120,161 @@ def perform
recipients_reason[user.id] = __('You are receiving this because you are a member of the group of this ticket.')
end

# send notifications
recipients_and_channels.each do |item|
user = item[:user]
channels = item[:channels]
[recipients_and_channels, recipients_reason]
end

# ignore user who changed it by him self via web
if @params[:interface_handle] == 'application_server'
next if article&.updated_by_id == user.id
next if !article && @item[:user_id] == user.id
end
def send_to_single_recipient(item, article, ticket, recipients_reason)
user = item[:user]
channels = item[:channels]

# ignore inactive users
next if !user.active?
# ignore user who changed it by him self via web
if @params[:interface_handle] == 'application_server'
return if article&.updated_by_id == user.id
return if !article && @item[:user_id] == user.id
end

# ignore if no changes has been done
changes = human_changes(@item[:changes], ticket, user)
next if @item[:type] == 'update' && !article && changes.blank?
# ignore inactive users
return if !user.active?

# check if today already notified
if @item[:type] == 'reminder_reached' || @item[:type] == 'escalation' || @item[:type] == 'escalation_warning'
identifier = user.email
if !identifier || identifier == ''
identifier = user.login
end
# ignore if no changes has been done
changes = human_changes(@item[:changes], ticket, user)
return if @item[:type] == 'update' && !article && changes.blank?

already_notified_cutoff = Time.use_zone(Setting.get('timezone_default')) { Time.current.beginning_of_day }
# check if today already notified
if @item[:type] == 'reminder_reached' || @item[:type] == 'escalation' || @item[:type] == 'escalation_warning'
identifier = user.email
if !identifier || identifier == ''
identifier = user.login
end

already_notified = History.where(
history_type_id: History.type_lookup('notification').id,
history_object_id: History.object_lookup('Ticket').id,
o_id: ticket.id
).where('created_at > ?', already_notified_cutoff).exists?(['value_to LIKE ?', "%#{SqlHelper.quote_like(identifier)}(#{SqlHelper.quote_like(@item[:type])}:%"])
already_notified_cutoff = Time.use_zone(Setting.get('timezone_default')) { Time.current.beginning_of_day }

next if already_notified
end
already_notified = History.where(
history_type_id: History.type_lookup('notification').id,
history_object_id: History.object_lookup('Ticket').id,
o_id: ticket.id
).where('created_at > ?', already_notified_cutoff).exists?(['value_to LIKE ?', "%#{SqlHelper.quote_like(identifier)}(#{SqlHelper.quote_like(@item[:type])}:%"])

# create online notification
used_channels = []
if channels['online']
used_channels.push 'online'

created_by_id = @item[:user_id] || 1

# delete old notifications
if @item[:type] == 'reminder_reached'
seen = false
created_by_id = 1
OnlineNotification.remove_by_type('Ticket', ticket.id, @item[:type], user)

elsif @item[:type] == 'escalation' || @item[:type] == 'escalation_warning'
seen = false
created_by_id = 1
OnlineNotification.remove_by_type('Ticket', ticket.id, 'escalation', user)
OnlineNotification.remove_by_type('Ticket', ticket.id, 'escalation_warning', user)

# on updates without state changes create unseen messages
elsif @item[:type] != 'create' && (@item[:changes].blank? || @item[:changes]['state_id'].blank?)
seen = false
else
seen = OnlineNotification.seen_state?(ticket, user.id)
end
return if already_notified
end

OnlineNotification.add(
type: @item[:type],
object: 'Ticket',
o_id: ticket.id,
seen: seen,
created_by_id: created_by_id,
user_id: user.id,
)
Rails.logger.debug { "sent ticket online notifiaction to agent (#{@item[:type]}/#{ticket.id}/#{user.email})" }
# create online notification
used_channels = []
if channels['online']
used_channels.push 'online'

created_by_id = @item[:user_id] || 1

# delete old notifications
if @item[:type] == 'reminder_reached'
seen = false
created_by_id = 1
OnlineNotification.remove_by_type('Ticket', ticket.id, @item[:type], user)

elsif @item[:type] == 'escalation' || @item[:type] == 'escalation_warning'
seen = false
created_by_id = 1
OnlineNotification.remove_by_type('Ticket', ticket.id, 'escalation', user)
OnlineNotification.remove_by_type('Ticket', ticket.id, 'escalation_warning', user)

# on updates without state changes create unseen messages
elsif @item[:type] != 'create' && (@item[:changes].blank? || @item[:changes]['state_id'].blank?)
seen = false
else
seen = OnlineNotification.seen_state?(ticket, user.id)
end

# ignore email channel notification and empty emails
if !channels['email'] || user.email.blank?
add_recipient_list(ticket, user, used_channels, @item[:type])
next
end
OnlineNotification.add(
type: @item[:type],
object: 'Ticket',
o_id: ticket.id,
seen: seen,
created_by_id: created_by_id,
user_id: user.id,
)
Rails.logger.debug { "sent ticket online notification to agent (#{@item[:type]}/#{ticket.id}/#{user.email})" }
end

used_channels.push 'email'
add_recipient_list(ticket, user, used_channels, @item[:type])

# get user based notification template
# if create, send create message / block update messages
template = case @item[:type]
when 'create'
'ticket_create'
when 'update'
'ticket_update'
when 'reminder_reached'
'ticket_reminder_reached'
when 'escalation'
'ticket_escalation'
when 'escalation_warning'
'ticket_escalation_warning'
when 'update.merged_into'
'ticket_update_merged_into'
when 'update.received_merge'
'ticket_update_received_merge'
else
raise "unknown type for notification #{@item[:type]}"
end

current_user = User.lookup(id: @item[:user_id])
if !current_user
current_user = User.lookup(id: 1)
end
# ignore email channel notification and empty emails
if !channels['email'] || user.email.blank?
add_recipient_list_to_history(ticket, user, used_channels, @item[:type])
return
end

used_channels.push 'email'
add_recipient_list_to_history(ticket, user, used_channels, @item[:type])

# get user based notification template
# if create, send create message / block update messages
template = case @item[:type]
when 'create'
'ticket_create'
when 'update'
'ticket_update'
when 'reminder_reached'
'ticket_reminder_reached'
when 'escalation'
'ticket_escalation'
when 'escalation_warning'
'ticket_escalation_warning'
when 'update.merged_into'
'ticket_update_merged_into'
when 'update.received_merge'
'ticket_update_received_merge'
else
raise "unknown type for notification #{@item[:type]}"
end

current_user = User.lookup(id: @item[:user_id])
if !current_user
current_user = User.lookup(id: 1)
end

attachments = []
if article
attachments = article.attachments_inline
end
NotificationFactory::Mailer.notification(
template: template,
user: user,
objects: {
ticket: ticket,
article: article,
recipient: user,
current_user: current_user,
changes: changes,
reason: recipients_reason[user.id],
},
message_id: "<notification.#{DateTime.current.to_fs(:number)}.#{ticket.id}.#{user.id}.#{SecureRandom.uuid}@#{Setting.get('fqdn')}>",
references: ticket.get_references,
main_object: ticket,
attachments: attachments,
)
Rails.logger.debug { "sent ticket email notification to agent (#{@item[:type]}/#{ticket.id}/#{user.email})" }
rescue Channel::DeliveryError => e
status_code = begin
e.original_error.response.status.to_i
rescue
raise e
end

attachments = []
if article
attachments = article.attachments_inline
if SILENCABLE_SMTP_ERROR_CODES.any? { |elem| elem.include? status_code }
Rails.logger.info do
"could not send ticket email notification to agent (#{@item[:type]}/#{ticket.id}/#{user.email}) #{e.original_error}"
end
NotificationFactory::Mailer.notification(
template: template,
user: user,
objects: {
ticket: ticket,
article: article,
recipient: user,
current_user: current_user,
changes: changes,
reason: recipients_reason[user.id],
},
message_id: "<notification.#{DateTime.current.to_fs(:number)}.#{ticket.id}.#{user.id}.#{SecureRandom.uuid}@#{Setting.get('fqdn')}>",
references: ticket.get_references,
main_object: ticket,
attachments: attachments,
)
Rails.logger.debug { "sent ticket email notifiaction to agent (#{@item[:type]}/#{ticket.id}/#{user.email})" }

return
end

raise e
end

def add_recipient_list(ticket, user, channels, type)
def add_recipient_list_to_history(ticket, user, channels, type)
return if channels.blank?

identifier = user.email
if !identifier || identifier == ''
identifier = user.login
end
identifier = user.email.presence || user.login
recipient_list = "#{identifier}(#{type}:#{channels.join(',')})"

History.add(
o_id: ticket.id,
history_type: 'notification',
Expand Down
8 changes: 4 additions & 4 deletions i18n/zammad.pot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16537,19 +16537,19 @@ msgstr ""
msgid "You are on waiting list position <strong>%s</strong>."
msgstr ""

#: app/models/transaction/notification.rb:81
#: app/models/transaction/notification.rb:98
msgid "You are receiving this because you are a member of the group of this ticket."
msgstr ""

#: app/models/transaction/notification.rb:235
#: app/models/transaction/notification.rb:267
msgid "You are receiving this because you are out-of-office replacement for a participant of this ticket."
msgstr ""

#: app/models/transaction/notification.rb:53
#: app/models/transaction/notification.rb:70
msgid "You are receiving this because you are the owner of this ticket."
msgstr ""

#: app/models/transaction/notification.rb:47
#: app/models/transaction/notification.rb:64
msgid "You are receiving this because you were mentioned in this ticket."
msgstr ""

Expand Down
37 changes: 37 additions & 0 deletions spec/models/transaction/notification_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,43 @@
end
end

describe 'SMTP errors' do
let(:group) { create(:group) }
let(:user) { create(:agent, groups: [group]) }
let(:ticket) { create(:ticket, owner: user, state_name: 'open', pending_time: Time.current) }
let(:response) { Net::SMTP::Response.new(response_status_code, 'mocked SMTP response') }
let(:error) { Net::SMTPFatalError.new(response) }

before do
allow_any_instance_of(Net::SMTP).to receive(:start).and_raise(error)

Service::System::SetEmailNotificationConfiguration
.new(
adapter: 'smtp',
new_configuration: {}
).execute
end

context 'when there is a problem with the sending SMTP server' do
let(:response_status_code) { 535 }

it 'raises an eroror' do
expect { run(ticket, user, 'reminder_reached') }
.to raise_error(Channel::DeliveryError)
end
end

context 'when there is a problem with the receiving SMTP server' do
let(:response_status_code) { 550 }

it 'logs the information about failed email delivery' do
allow(Rails.logger).to receive(:info)
run(ticket, user, 'reminder_reached')
expect(Rails.logger).to have_received(:info)
end
end
end

it_behaves_like 'ChecksHumanChanges'

def run(ticket, user, type)
Expand Down

0 comments on commit c240012

Please sign in to comment.