diff --git a/app/assets/javascripts/app/controllers/cti.coffee b/app/assets/javascripts/app/controllers/cti.coffee index dddd07602..0fa50580d 100644 --- a/app/assets/javascripts/app/controllers/cti.coffee +++ b/app/assets/javascripts/app/controllers/cti.coffee @@ -10,6 +10,7 @@ class App.CTI extends App.Controller active: false counter: 0 state: {} + backendEnabled: false constructor: -> super @@ -29,9 +30,9 @@ class App.CTI extends App.Controller @meta.counter += 1 @updateNavMenu() if data.state is 'answer' || data.state is 'hangup' - return if !@meta.state[data.id] - delete @meta.state[data.id] - @meta.counter -= 1 + if @meta.state[data.id] + delete @meta.state[data.id] + @meta.counter -= 1 @updateNavMenu() 'cti_event' ) @@ -82,6 +83,17 @@ class App.CTI extends App.Controller App.Collection.loadAssets(data.assets) if data.backends @backends = data.backends + + # check if configured backends are changed + backendEnabled = false + for backend in @backends + if backend.enabled + backendEnabled = true + if backendEnabled isnt @backendEnabled + @renderDone = false + @backendEnabled = backendEnabled + + # render new caller list if data.list @list = data.list if @renderDone @@ -109,11 +121,7 @@ class App.CTI extends App.Controller return # check if min one backend is enabled - backendEnabled = false - for backend in @backends - if backend.enabled - backendEnabled = true - if !backendEnabled + if !@backendEnabled @html App.view('cti/not_configured')( backends: @backends isAdmin: @permissionCheck('admin.integration') diff --git a/app/models/ticket.rb b/app/models/ticket.rb index f71bc5300..2dd8db5d9 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -806,6 +806,12 @@ perform changes on ticket def perform_changes(perform, perform_origin, item = nil, current_user_id = nil) logger.debug { "Perform #{perform_origin} #{perform.inspect} on Ticket.find(#{id})" } + article = begin + Ticket::Article.lookup(id: item.try(:dig, :article_id)) + rescue ArgumentError + nil + end + # if the configuration contains the deletion of the ticket then # we skip all other ticket changes because they does not matter if perform['ticket.action'].present? && perform['ticket.action']['value'] == 'delete' @@ -835,8 +841,7 @@ perform changes on ticket recipients_raw = [] value_recipient.each do |recipient| if recipient == 'article_last_sender' - if item && item[:article_id] - article = Ticket::Article.lookup(id: item[:article_id]) + if article.present? if article.reply_to.present? recipients_raw.push(article.reply_to) elsif article.from.present? @@ -914,12 +919,9 @@ perform changes on ticket end # check if notification should be send because of customer emails - if item && item[:article_id] - article = Ticket::Article.lookup(id: item[:article_id]) - if article&.preferences&.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i - logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" - next - end + if article.present? && article.preferences.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i + logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email" + next end # loop protection / check if maximal count of trigger mail has reached @@ -984,9 +986,12 @@ perform changes on ticket next end + # articles.last breaks (returns the wrong article) + # if another email notification trigger preceded this one + # (see https://github.com/zammad/zammad/issues/1543) objects = { ticket: self, - article: articles.last, + article: article || articles.last } # get subject @@ -1007,7 +1012,7 @@ perform changes on ticket (body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id) - article = Ticket::Article.create( + message = Ticket::Article.create( ticket_id: id, to: recipient_string, subject: subject, @@ -1026,7 +1031,7 @@ perform changes on ticket attachments_inline.each do |attachment| Store.add( object: 'Ticket::Article', - o_id: article.id, + o_id: message.id, data: attachment[:data], filename: attachment[:filename], preferences: attachment[:preferences], diff --git a/script/build/test_slice_tests.sh b/script/build/test_slice_tests.sh index 08c5ff28e..daec7e3d8 100755 --- a/script/build/test_slice_tests.sh +++ b/script/build/test_slice_tests.sh @@ -65,6 +65,7 @@ if [ "$LEVEL" == '1' ]; then # test/browser/maintenance_session_message_test.rb # test/browser/manage_test.rb # test/browser/monitoring_test.rb + rm test/browser/phone_notify_not_clearing_on_leftside_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -137,6 +138,7 @@ elif [ "$LEVEL" == '2' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb + rm test/browser/phone_notify_not_clearing_on_leftside_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -209,6 +211,7 @@ elif [ "$LEVEL" == '3' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb + rm test/browser/phone_notify_not_clearing_on_leftside_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -281,6 +284,7 @@ elif [ "$LEVEL" == '4' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb + rm test/browser/phone_notify_not_clearing_on_leftside_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -352,6 +356,7 @@ elif [ "$LEVEL" == '5' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb + rm test/browser/phone_notify_not_clearing_on_leftside_test.rb rm test/browser/preferences_language_test.rb rm test/browser/preferences_permission_check_test.rb rm test/browser/preferences_token_access_test.rb @@ -426,6 +431,7 @@ elif [ "$LEVEL" == '6' ]; then rm test/browser/maintenance_session_message_test.rb rm test/browser/manage_test.rb rm test/browser/monitoring_test.rb + # rm test/browser/phone_notify_not_clearing_on_leftside_test.rb # test/browser/preferences_language_test.rb # test/browser/preferences_permission_check_test.rb # test/browser/preferences_token_access_test.rb diff --git a/spec/factories/email_address.rb b/spec/factories/email_address.rb new file mode 100644 index 000000000..951917a68 --- /dev/null +++ b/spec/factories/email_address.rb @@ -0,0 +1,9 @@ +FactoryBot.define do + factory :email_address do + email 'zammad@localhost' + realname 'zammad' + channel_id 1 + created_by_id 1 + updated_by_id 1 + end +end diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index bdb317e4f..057c7118a 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -218,6 +218,45 @@ RSpec.describe Ticket do .and not_change { trigger.perform['notification.email'][:subject] } end + # Regression test for https://github.com/zammad/zammad/issues/1543 + # + # If a new article fires an email notification trigger, + # and then another article is added to the same ticket + # before that trigger is performed, + # the email template's 'article' var should refer to the originating article, + # not the newest one. + # + # (This occurs whenever one action fires multiple email notification triggers.) + it 'passes the correct article to NotificationFactory::Mailer' do + # required by Ticket#perform_changes for email notifications + Group.first.update(email_address: create(:email_address)) + + ticket = Ticket.first + orig_article = Ticket::Article.where(ticket_id: ticket.id).first + newer_article = create(:ticket_article, ticket_id: ticket.id) + trigger = Trigger.new( + perform: { + 'notification.email' => { + body: '', + recipient: 'ticket_customer', + subject: '' + } + } + ) + + allow(NotificationFactory::Mailer).to receive(:template).and_return('') + + ticket.perform_changes(trigger.perform, 'trigger', { article_id: orig_article.id }, 1) + + expect(NotificationFactory::Mailer) + .to have_received(:template) + .with(hash_including(objects: { ticket: ticket, article: orig_article })) + .at_least(:once) + + expect(NotificationFactory::Mailer) + .not_to have_received(:template) + .with(hash_including(objects: { ticket: ticket, article: newer_article })) + end end describe '#selectors' do diff --git a/test/browser/phone_notify_not_clearing_on_leftside_test.rb b/test/browser/phone_notify_not_clearing_on_leftside_test.rb new file mode 100644 index 000000000..df9d674da --- /dev/null +++ b/test/browser/phone_notify_not_clearing_on_leftside_test.rb @@ -0,0 +1,51 @@ +require 'browser_test_helper' + +# Regression test for #2017 + +class PhoneNotifyNotClearingOnLeftsideTest < TestCase + def test_notify_badge + id = rand(99_999_999) + + @browser = browser_instance + login( + username: 'master@example.com', + password: 'test', + url: browser_url, + ) + + click(css: 'a[href="#manage"]') + click(css: 'a[href="#system/integration"]') + click(css: 'a[href="#system/integration/sipgate"]') + + if !@browser.find_element(css: 'input[name=sipgate_integration]').property('checked') + switch( + css: '.active .js-switch', + type: 'on' + ) + end + + watch_for( + css: 'a[href="#cti"]' + ) + + click(css: 'a[href="#cti"]') + + # simulate sipgate callbacks + + url = URI.join(browser_url, 'api/v1/sipgate/in') + params = { direction: 'in', from: '491715000002', to: '4930600000000', callId: "4991155921769858278-#{id}", cause: 'busy' } + Net::HTTP.post_form(url, params.merge(event: 'newCall')) + Net::HTTP.post_form(url, params.merge(event: 'hangup')) + + watch_for( + css: '.js-phoneMenuItem .counter', + value: '1' + ) + + @browser.find_element(css: '.table-checkbox label').click + + watch_for_disappear( + css: '.js-phoneMenuItem .counter' + ) + end +end