Merge branch 'develop' of git.znuny.com:zammad/zammad into develop

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')

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],

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

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

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

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