diff --git a/spec/jobs/ticket_article_communicate_email_job_spec.rb b/spec/jobs/ticket_article_communicate_email_job_spec.rb new file mode 100644 index 000000000..7723971d8 --- /dev/null +++ b/spec/jobs/ticket_article_communicate_email_job_spec.rb @@ -0,0 +1,28 @@ +require 'rails_helper' + +RSpec.describe TicketArticleCommunicateEmailJob, type: :job do + describe '#perform' do + context 'for an email article' do + let(:article) { create(:ticket_article, type_name: 'email') } + let(:recipient_list) { [article.to, article.cc].reject(&:blank?).join(',') } + + before { allow(Rails.logger).to receive(:info) } + + # What we _really_ want is to expect an email to be sent. + # So why are we testing log messages instead? + # + # Because so far, our attempts to test email dispatch have either + # a) been closely tied to implementation, with lots of ugly mock objects; or + # b) had to test faraway classes like Channel::Driver::Imap. + # + # In other words, this test is NOT set in stone, and very open to improvement. + it 'records outgoing email dispatch to Rails log' do + described_class.perform_now(article.id) + + expect(Rails.logger) + .to have_received(:info) + .with("Send email to: '#{recipient_list}' (from #{article.from})") + end + end + end +end diff --git a/spec/lib/application_handle_info_spec.rb b/spec/lib/application_handle_info_spec.rb index 972fef046..6f95328f5 100644 --- a/spec/lib/application_handle_info_spec.rb +++ b/spec/lib/application_handle_info_spec.rb @@ -8,7 +8,21 @@ RSpec.describe ApplicationHandleInfo do end context 'for a given starting ApplicationHandleInfo' do - before { described_class.current = 'foo' } + # This `around` block is identical to ApplicationHandleInfo.use. + # + # Q: So why don't we just use it here to DRY things up? + # A: Because that's the method we're trying to test, dummy! + # + # Q: Why can't we do `before { ApplicationHandleInfo.current = 'foo' }` instead? + # A: Because that would change `ApplicationHandleInfo.current` for all subsequent specs. + # (RSpec uses database transactions to keep test environments clean, + # but `ApplicationHandleInfo.current` lives outside of the database.) + around do |example| + original = described_class.current + described_class.current = 'foo' + example.run + described_class.current = original + end it 'runs the block using the given ApplicationHandleInfo' do described_class.use('bar') do @@ -26,7 +40,7 @@ RSpec.describe ApplicationHandleInfo do it 'does not rescue the error, and still resets ApplicationHandleInfo' do expect { described_class.use('bar') { raise } } .to raise_error(StandardError) - .and not_change { described_class.current } + .and not_change(described_class, :current) end end end diff --git a/spec/models/channel/email_parser_spec.rb b/spec/models/channel/email_parser_spec.rb index 4202812ba..aae355be2 100644 --- a/spec/models/channel/email_parser_spec.rb +++ b/spec/models/channel/email_parser_spec.rb @@ -978,5 +978,23 @@ RSpec.describe Channel::EmailParser, type: :model do end end end + + describe 'suppressing normal Ticket::Article callbacks' do + context 'from sender: "Agent"' do + let(:agent) { create(:agent_user) } + + it 'does not dispatch an email on article creation' do + expect(TicketArticleCommunicateEmailJob).not_to receive(:perform_later) + + described_class.new.process({}, <<~RAW.chomp) + From: #{agent.email} + To: customer@example.com + Subject: some subject + + Some Text + RAW + end + end + end end end diff --git a/spec/models/ticket/article_spec.rb b/spec/models/ticket/article_spec.rb index a7020931f..89030e3c6 100644 --- a/spec/models/ticket/article_spec.rb +++ b/spec/models/ticket/article_spec.rb @@ -403,6 +403,64 @@ RSpec.describe Ticket::Article, type: :model do end end end + + describe 'Sending of outgoing emails', performs_jobs: true do + subject(:article) { create(:ticket_article, type_name: type, sender_name: sender) } + + shared_examples 'sends email' do + it 'dispatches an email on creation (via TicketArticleCommunicateEmailJob)' do + expect { article } + .to have_enqueued_job(TicketArticleCommunicateEmailJob) + end + end + + shared_examples 'does not send email' do + it 'does not dispatch an email' do + expect { article } + .not_to have_enqueued_job(TicketArticleCommunicateEmailJob) + end + end + + context 'with "application_server" application handle', application_handle: 'application_server' do + context 'for type: "email"' do + let(:type) { 'email' } + + context 'from sender: "Agent"' do + let(:sender) { 'Agent' } + + include_examples 'sends email' + end + + context 'from sender: "Customer"' do + let(:sender) { 'Customer' } + + include_examples 'does not send email' + end + end + + context 'for any other type' do + let(:type) { 'sms' } + + context 'from any sender' do + let(:sender) { 'Agent' } + + include_examples 'does not send email' + end + end + end + + context 'with "*.postmaster" application handle', application_handle: 'scheduler.postmaster' do + context 'for any type' do + let(:type) { 'email' } + + context 'from any sender' do + let(:sender) { 'Agent' } + + include_examples 'does not send email' + end + end + end + end end describe 'clone attachments' do diff --git a/test/test_helper.rb b/test/test_helper.rb index b5031eea5..c67e3b1f9 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -74,23 +74,4 @@ class ActiveSupport::TestCase count end - def email_count(recipient) - - # read config file and count & recipients - file = Rails.root.join('log', "#{Rails.env}.log") - lines = [] - IO.foreach(file) do |line| - lines.push line - end - count = 0 - lines.reverse_each do |line| - break if line.match?(/\+\+\+\+NEW\+\+\+\+TEST\+\+\+\+/) - next if line !~ /Send email to:/ - next if line !~ /to:\s'#{recipient}'/ - - count += 1 - end - count - end - end diff --git a/test/unit/ticket_article_communicate_test.rb b/test/unit/ticket_article_communicate_test.rb deleted file mode 100644 index cd4a78e3e..000000000 --- a/test/unit/ticket_article_communicate_test.rb +++ /dev/null @@ -1,158 +0,0 @@ -require 'test_helper' - -class TicketArticleCommunicateTest < ActiveSupport::TestCase - - test 'via config' do - - # via application server - ApplicationHandleInfo.current = 'application_server' - ticket1 = Ticket.create( - title: 'com test 1', - group: Group.lookup(name: 'Users'), - customer_id: 2, - state: Ticket::State.lookup(name: 'new'), - priority: Ticket::Priority.lookup(name: '2 normal'), - updated_by_id: 1, - created_by_id: 1, - ) - assert(ticket1, 'ticket created') - - article_email1_1 = Ticket::Article.create( - ticket_id: ticket1.id, - from: 'some_customer_com-1@example.com', - to: 'some_zammad_com-1@example.com', - subject: 'com test 1', - message_id: 'some@id_com_1', - body: 'some message 123', - internal: false, - sender: Ticket::Article::Sender.find_by(name: 'Customer'), - type: Ticket::Article::Type.find_by(name: 'email'), - updated_by_id: 1, - created_by_id: 1, - ) - assert_equal('some_customer_com-1@example.com', article_email1_1.from) - assert_equal('some_zammad_com-1@example.com', article_email1_1.to) - assert_equal(0, email_count('some_customer_com-1@example.com')) - assert_equal(0, email_count('some_zammad_com-1@example.com')) - Scheduler.worker(true) - assert_equal('some_customer_com-1@example.com', article_email1_1.from) - assert_equal('some_zammad_com-1@example.com', article_email1_1.to) - assert_equal(0, email_count('some_customer_com-1@example.com')) - assert_equal(0, email_count('some_zammad_com-1@example.com')) - - article_email1_2 = Ticket::Article.create( - ticket_id: ticket1.id, - from: 'some_zammad_com-1@example.com', - to: 'some_customer_com-1@example.com', - subject: 'com test 1', - message_id: 'some@id_com_2', - body: 'some message 123', - internal: false, - sender: Ticket::Article::Sender.find_by(name: 'Agent'), - type: Ticket::Article::Type.find_by(name: 'email'), - updated_by_id: 1, - created_by_id: 1, - ) - assert_equal('Zammad ', article_email1_2.from) - assert_equal('some_customer_com-1@example.com', article_email1_2.to) - assert_equal(0, email_count('some_customer_com-1@example.com')) - assert_equal(0, email_count('some_zammad_com-1@example.com')) - Scheduler.worker(true) - assert_equal('Zammad ', article_email1_2.from) - assert_equal('some_customer_com-1@example.com', article_email1_2.to) - assert_equal(1, email_count('some_customer_com-1@example.com')) - assert_equal(0, email_count('some_zammad_com-1@example.com')) - - # via scheduler (e. g. postmaster) - ApplicationHandleInfo.current = 'scheduler.postmaster' - ticket2 = Ticket.create( - title: 'com test 2', - group: Group.lookup(name: 'Users'), - customer_id: 2, - state: Ticket::State.lookup(name: 'new'), - priority: Ticket::Priority.lookup(name: '2 normal'), - updated_by_id: 1, - created_by_id: 1, - ) - assert(ticket2, 'ticket created') - - article_email2_1 = Ticket::Article.create( - ticket_id: ticket2.id, - from: 'some_customer_com-2@example.com', - to: 'some_zammad_com-2@example.com', - subject: 'com test 2', - message_id: 'some@id_com_1', - body: 'some message 123', - internal: false, - sender: Ticket::Article::Sender.find_by(name: 'Customer'), - type: Ticket::Article::Type.find_by(name: 'email'), - updated_by_id: 1, - created_by_id: 1, - ) - assert_equal('some_customer_com-2@example.com', article_email2_1.from) - assert_equal('some_zammad_com-2@example.com', article_email2_1.to) - assert_equal(0, email_count('some_customer_com-2@example.com')) - assert_equal(0, email_count('some_zammad_com-2@example.com')) - Scheduler.worker(true) - assert_equal('some_customer_com-2@example.com', article_email2_1.from) - assert_equal('some_zammad_com-2@example.com', article_email2_1.to) - assert_equal(0, email_count('some_customer_com-2@example.com')) - assert_equal(0, email_count('some_zammad_com-2@example.com')) - - ApplicationHandleInfo.current = 'scheduler.postmaster' - article_email2_2 = Ticket::Article.create( - ticket_id: ticket2.id, - from: 'some_zammad_com-2@example.com', - to: 'some_customer_com-2@example.com', - subject: 'com test 2', - message_id: 'some@id_com_2', - body: 'some message 123', - internal: false, - sender: Ticket::Article::Sender.find_by(name: 'Agent'), - type: Ticket::Article::Type.find_by(name: 'email'), - updated_by_id: 1, - created_by_id: 1, - ) - assert_equal(0, email_count('some_customer_com-2@example.com')) - assert_equal(0, email_count('some_zammad_com-2@example.com')) - assert_equal('some_zammad_com-2@example.com', article_email2_2.from) - assert_equal('some_customer_com-2@example.com', article_email2_2.to) - Scheduler.worker(true) - assert_equal(0, email_count('some_customer_com-2@example.com')) - assert_equal(0, email_count('some_zammad_com-2@example.com')) - assert_equal('some_zammad_com-2@example.com', article_email2_2.from) - assert_equal('some_customer_com-2@example.com', article_email2_2.to) - end - - test 'postmaster process - do not change from - verify article sender' do - email_raw_string = "From: my_zammad@example.com -To: customer@example.com -Subject: some subject -X-Zammad-Article-Sender: Agent -X-Loop: yes - -Some Text" - - _ticket_p, article_p, _user_p, _mail = Channel::EmailParser.new.process({ trusted: true }, email_raw_string) - article_email = Ticket::Article.find(article_p.id) - - assert_equal(0, email_count('my_zammad@example.com')) - assert_equal(0, email_count('customer@example.com')) - assert_equal('my_zammad@example.com', article_email.from) - assert_equal('customer@example.com', article_email.to) - assert_equal('Agent', article_email.sender.name) - assert_equal('email', article_email.type.name) - assert_equal(1, article_email.ticket.articles.count) - - Scheduler.worker(true) - article_email = Ticket::Article.find(article_p.id) - assert_equal(0, email_count('my_zammad@example.com')) - assert_equal(0, email_count('customer@example.com')) - assert_equal('my_zammad@example.com', article_email.from) - assert_equal('customer@example.com', article_email.to) - assert_equal('Agent', article_email.sender.name) - assert_equal('email', article_email.type.name) - assert_equal(1, article_email.ticket.articles.count) - end - -end