Fixes #3110 - ServiceNow mails from other service providers are not detected.

app/models/channel/filter/service_now_check.rb

@@ -4,23 +4,25 @@ module Channel::Filter::ServiceNowCheck
 
   # This filter will run pre and post
   def self.run(_channel, mail, ticket = nil, _article = nil, _session_user = nil)
-    source_id = self.source_id(from: mail[:from], subject: mail[:subject])
+    return if mail['x-servicenow-generated'].blank?
+
+    source_id = self.source_id(subject: mail[:subject])
     return if source_id.blank?
 
+    source_name = self.source_name(from: mail[:from])
+    return if source_name.blank?
+
     # check if we can followup by existing service now relation
     if ticket.blank?
-      sync_entry = ExternalSync.find_by(
+      from_sync_entry(
-        source:    'ServiceNow',
+        mail:        mail,
-        source_id: source_id,
+        source_name: source_name,
-        object:    'Ticket',
+        source_id:   source_id,
       )
-      return if sync_entry.blank?
-
-      mail[ 'x-zammad-ticket-id'.to_sym ] = sync_entry.o_id
       return
     end
 
-    ExternalSync.create_with(source_id: source_id).find_or_create_by(source: 'ServiceNow', object: 'Ticket', o_id: ticket.id)
+    ExternalSync.create_with(source_id: source_id).find_or_create_by(source: source_name, object: 'Ticket', o_id: ticket.id)
   end
 
 =begin
@@ -28,7 +30,7 @@ module Channel::Filter::ServiceNowCheck
 This function returns the source id of the service now email if given.
 
   source_id = Channel::Filter::ServiceNowCheck.source_id(
-    from:    'test@servicnow.com',
+    from:    'test@service-now.com',
     subject: 'Incident INC12345 --- test',
   )
 
@@ -38,16 +40,7 @@ returns:
 
 =end
 
-  def self.source_id(from: '', subject: '')
+  def self.source_id(subject: '')
-
-    # check if data is sent by service now
-    begin
-      return if Mail::AddressList.new(from).addresses.none? do |line|
-        line.address.end_with?('@service-now.com')
-      end
-    rescue
-      Rails.logger.info "Unable to parse email address in '#{from}'"
-    end
 
     # check if we can find the service now relation
     source_id = nil
@@ -57,4 +50,43 @@ returns:
 
     source_id
   end
+
+=begin
+
+This function returns the sync id of the service now email if given.
+
+  source_name = Channel::Filter::ServiceNowCheck.source_name(
+    from:    'test@service-now.com',
+  )
+
+returns:
+
+  source_name = 'ServiceNow-test@service-now.com'
+
+=end
+
+  def self.source_name(from:)
+    result = nil
+    begin
+      Mail::AddressList.new(from).addresses.each do |line|
+        result = "ServiceNow-#{line.address}"
+        break
+      end
+    rescue
+      Rails.logger.info "Unable to parse email address in '#{from}'"
+    end
+
+    result
+  end
+
+  def self.from_sync_entry(mail:, source_name:, source_id:)
+    sync_entry = ExternalSync.find_by(
+      source:    source_name,
+      source_id: source_id,
+      object:    'Ticket',
+    )
+    return if sync_entry.blank?
+
+    mail[ 'x-zammad-ticket-id'.to_sym ] = sync_entry.o_id
+  end
 end

db/migrate/20200709094556_issue_3110_service_now_provider.rb

@@ -0,0 +1,13 @@
+class Issue3110ServiceNowProvider < ActiveRecord::Migration[5.2]
+  def change
+
+    # return if it's a new setup
+    return if !Setting.find_by(name: 'system_init_done')
+
+    ExternalSync.where(source: 'ServiceNow').find_each do |row|
+      article = Ticket.find(row.o_id).articles.first
+      source_name = Channel::Filter::ServiceNowCheck.source_name(from: article.from)
+      row.update(source: source_name)
+    end
+  end
+end

spec/db/migrate/issue3110_service_now_provider_spec.rb

@@ -0,0 +1,27 @@
+require 'rails_helper'
+
+RSpec.describe Issue3110ServiceNowProvider, type: :db_migration do
+
+  let(:ticket) { create(:ticket) }
+  let(:external_sync) do
+    create(:external_sync,
+           source:    'ServiceNow',
+           source_id: 'INC678439',
+           object:    'Ticket',
+           o_id:      ticket.id)
+  end
+
+  before do
+    create(:ticket_article,
+           ticket:  ticket,
+           subject: 'Incident INC678439 -- zugewiesen an EXT-XXXINIS',
+           from:    'zam@mad-service-now.com')
+  end
+
+  it 'does migrates obsolete ServiceNow ExternalSync references' do
+    expect { migrate }
+      .to change { external_sync.reload.source }
+      .from('ServiceNow')
+      .to('ServiceNow-zam@mad-service-now.com')
+  end
+end

spec/factories/external_sync.rb

@@ -0,0 +1,4 @@
+FactoryBot.define do
+  factory :external_sync do
+  end
+end

spec/models/channel/email_parser_spec.rb

@@ -993,29 +993,36 @@ RSpec.describe Channel::EmailParser, type: :model do
     end
 
     describe 'ServiceNow handling' do
-      context 'when emails with service now reference are sent' do
+
+      context 'new Ticket' do
         let(:mail_file) { Rails.root.join('test/data/mail/mail089.box') }
-        let(:mail_file_answer) { Rails.root.join('test/data/mail/mail090.box') }
-        let(:raw_mail_answer)  { File.read(mail_file_answer) }
 
-        it 'does create a ticket with external sync reference' do
+        it 'creates an ExternalSync reference' do
-          expect { described_class.new.process({}, raw_mail) }
+          described_class.new.process({}, raw_mail)
-            .to change(Ticket, :count).by(1)
-            .and change(Ticket::Article, :count).by(1)
-            .and change(ExternalSync, :count).by(1)
 
-          expect(ExternalSync.last.source).to eq('ServiceNow')
+          expect(ExternalSync.last).to have_attributes(
-          expect(ExternalSync.last.source_id).to eq('INC678439')
+            source:    'ServiceNow-example@service-now.com',
-          expect(ExternalSync.last.object).to eq('Ticket')
+            source_id: 'INC678439',
-          expect(ExternalSync.last.o_id).to eq(Ticket.last.id)
+            object:    'Ticket',
-          expect(Ticket.last.articles.last.subject).to eq('Incident INC678439 -- zugewiesen an EXT-XXXINIS')
+            o_id:      Ticket.last.id,
+          )
+        end
+      end
 
-          expect { described_class.new.process({}, raw_mail_answer) }
+      context 'follow up' do
-            .to change(Ticket, :count).by(0)
-            .and change(Ticket::Article, :count).by(1)
-            .and change(ExternalSync, :count).by(0)
 
-          expect(Ticket.last.articles.last.subject).to eq('Incident INC678439 -- Arbeitsnotizen beigefügt')
+        let(:mail_file) { Rails.root.join('test/data/mail/mail090.box') }
+        let(:ticket) { create(:ticket) }
+        let!(:external_sync) do
+          create(:external_sync,
+                 source:    'ServiceNow-example@service-now.com',
+                 source_id: 'INC678439',
+                 object:    'Ticket',
+                 o_id:      ticket.id,)
+        end
+
+        it 'adds Article to existing Ticket' do
+          expect { described_class.new.process({}, raw_mail) }.to change { ticket.reload.articles.count }
         end
       end
     end