From c8e2c7473caa8f2abac7d382301be7d55a574d56 Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Thu, 19 Aug 2021 19:31:15 +0200 Subject: [PATCH] =?UTF-8?q?Fixes=20#3105=20-=20Merging=20tickets=20doesn?= =?UTF-8?q?=E2=80=99t=20trigger=20notification=20for=20target=20ticket?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../_ui_element/ticket_selector.coffee | 6 +- .../javascripts/app/models/ticket.coffee | 28 ++-- app/models/ticket.rb | 20 +++ app/models/transaction/notification.rb | 33 ++-- .../ticket_update_merged_into/en.html.erb | 11 ++ .../ticket_update_received_merge/en.html.erb | 11 ++ lib/notification_factory/mailer.rb | 3 + spec/factories/trigger.rb | 21 +++ spec/factories/user.rb | 20 +++ spec/models/ticket_spec.rb | 144 ++++++++++++++++++ spec/support/active_job.rb | 1 - spec/system/ticket/zoom_spec.rb | 56 +++++++ 12 files changed, 325 insertions(+), 29 deletions(-) create mode 100644 app/views/mailer/ticket_update_merged_into/en.html.erb create mode 100644 app/views/mailer/ticket_update_received_merge/en.html.erb diff --git a/app/assets/javascripts/app/controllers/_ui_element/ticket_selector.coffee b/app/assets/javascripts/app/controllers/_ui_element/ticket_selector.coffee index 0ba4484fa..6a68478a6 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/ticket_selector.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/ticket_selector.coffee @@ -73,8 +73,10 @@ class App.UiElement.ticket_selector null: false translate: true options: - create: 'created' - update: 'updated' + create: 'created' + update: 'updated' + 'update.merged_into': 'merged into' + 'update.received_merge': 'received merge' operator: ['is', 'is not'] for groupKey, groupMeta of groups diff --git a/app/assets/javascripts/app/models/ticket.coffee b/app/assets/javascripts/app/models/ticket.coffee index b33090ffe..0880e3552 100644 --- a/app/assets/javascripts/app/models/ticket.coffee +++ b/app/assets/javascripts/app/models/ticket.coffee @@ -103,17 +103,23 @@ class App.Ticket extends App.Model return if !item return if !item.created_by - if item.type is 'create' - return App.i18n.translateContent('%s created Ticket |%s|', item.created_by.displayName(), item.title) - else if item.type is 'update' - return App.i18n.translateContent('%s updated Ticket |%s|', item.created_by.displayName(), item.title) - else if item.type is 'reminder_reached' - return App.i18n.translateContent('Pending reminder reached for Ticket |%s|', item.title) - else if item.type is 'escalation' - return App.i18n.translateContent('Ticket |%s| is escalated!', item.title) - else if item.type is 'escalation_warning' - return App.i18n.translateContent('Ticket |%s| will escalate soon!', item.title) - return "Unknow action for (#{@objectDisplayName()}/#{item.type}), extend activityMessage() of model." + switch item.type + when 'create' + App.i18n.translateContent('%s created Ticket |%s|', item.created_by.displayName(), item.title) + when 'update' + App.i18n.translateContent('%s updated Ticket |%s|', item.created_by.displayName(), item.title) + when 'reminder_reached' + App.i18n.translateContent('Pending reminder reached for Ticket |%s|', item.title) + when 'escalation' + App.i18n.translateContent('Ticket |%s| is escalated!', item.title) + when 'escalation_warning' + App.i18n.translateContent('Ticket |%s| will escalate soon!', item.title) + when 'update.merged_into' + App.i18n.translateContent('Ticket |%s| was merged into another ticket', item.title) + when 'update.received_merge' + App.i18n.translateContent('Another ticket was merged into ticket |%s|', item.title) + else + "Unknow action for (#{@objectDisplayName()}/#{item.type}), extend activityMessage() of model." # apply macro @macro: (params) -> diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 71dc22eab..7b113938d 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -376,6 +376,26 @@ returns # touch new ticket (to broadcast change) target_ticket.touch # rubocop:disable Rails/SkipsModelValidations + + EventBuffer.add('transaction', { + object: target_ticket.class.name, + type: 'update.received_merge', + data: target_ticket, + changes: {}, + id: target_ticket.id, + user_id: UserInfo.current_user_id, + created_at: Time.zone.now, + }) + + EventBuffer.add('transaction', { + object: self.class.name, + type: 'update.merged_into', + data: self, + changes: {}, + id: id, + user_id: UserInfo.current_user_id, + created_at: Time.zone.now, + }) end true end diff --git a/app/models/transaction/notification.rb b/app/models/transaction/notification.rb index 53dd4ffef..dc0df9ac0 100644 --- a/app/models/transaction/notification.rb +++ b/app/models/transaction/notification.rb @@ -182,21 +182,24 @@ class Transaction::Notification # get user based notification template # if create, send create message / block update messages - template = nil - case @item[:type] - when 'create' - template = 'ticket_create' - when 'update' - template = 'ticket_update' - when 'reminder_reached' - template = 'ticket_reminder_reached' - when 'escalation' - template = 'ticket_escalation' - when 'escalation_warning' - template = 'ticket_escalation_warning' - else - raise "unknown type for notification #{@item[:type]}" - end + 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 diff --git a/app/views/mailer/ticket_update_merged_into/en.html.erb b/app/views/mailer/ticket_update_merged_into/en.html.erb new file mode 100644 index 000000000..805931245 --- /dev/null +++ b/app/views/mailer/ticket_update_merged_into/en.html.erb @@ -0,0 +1,11 @@ +Ticket (#{ticket.title}) was merged into another ticket + +
Hi #{recipient.firstname},
+
+
+Ticket (#{ticket.title}) was merged into another ticket by "#{current_user.longname}". +
+
+
+ #{t('View this in Zammad')} +
diff --git a/app/views/mailer/ticket_update_received_merge/en.html.erb b/app/views/mailer/ticket_update_received_merge/en.html.erb new file mode 100644 index 000000000..bea749bab --- /dev/null +++ b/app/views/mailer/ticket_update_received_merge/en.html.erb @@ -0,0 +1,11 @@ +Another ticket was merged into ticket (#{ticket.title}) + +
Hi #{recipient.firstname},
+
+
+Another ticket was merged into ticket (#{ticket.title}) by "#{current_user.longname}". +
+
+
+ #{t('View this in Zammad')} +
diff --git a/lib/notification_factory/mailer.rb b/lib/notification_factory/mailer.rb index a6db2c179..3d0ee633d 100644 --- a/lib/notification_factory/mailer.rb +++ b/lib/notification_factory/mailer.rb @@ -28,6 +28,9 @@ returns map = { 'escalation_warning' => 'escalation' } + + type = type.split('.').first # pick parent type of a subtype. Eg. update vs update.merged_into + if map[type] type = map[type] end diff --git a/spec/factories/trigger.rb b/spec/factories/trigger.rb index e697fe5c3..cdef28d02 100644 --- a/spec/factories/trigger.rb +++ b/spec/factories/trigger.rb @@ -9,4 +9,25 @@ FactoryBot.define do created_by_id { 1 } updated_by_id { 1 } end + + trait :conditionable do + transient do + condition_ticket_action { nil } + end + + condition { {} } + + callback(:after_stub, :before_create) do |object, context| + hash = object.condition + + hash['ticket.action'] = { 'operator' => 'is', 'value' => context.condition_ticket_action.to_s } if context.condition_ticket_action + + object.condition = hash + end + end + + # empty trigger to help to test atomically + trait :no_perform do + perform { { null: true } } + end end diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 34a3201fe..14edb9906 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -57,6 +57,26 @@ FactoryBot.define do password_plain = user.password user.define_singleton_method(:password_plain, -> { password_plain }) end + + trait :preferencable do + transient do + notification_group_ids { [] } + end + + preferences do + { + 'notification_config' => { + 'matrix' => { + 'create' => { 'criteria' => { 'owned_by_me' => true, 'owned_by_nobody' => true }, 'channel' => { 'email' => true, 'online' => true } }, + 'update' => { 'criteria' => { 'owned_by_me' => true, 'owned_by_nobody' => true }, 'channel' => { 'email' => true, 'online' => true } }, + 'reminder_reached' => { 'criteria' => { 'owned_by_me' => true, 'owned_by_nobody' => true }, 'channel' => { 'email' => true, 'online' => true } }, + 'escalation' => { 'criteria' => { 'owned_by_me' => true, 'owned_by_nobody' => true }, 'channel' => { 'email' => true, 'online' => true } }, + }, + 'group_ids' => notification_group_ids + } + } + end + end end sequence(:password_valid) do |n| diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index 61047fec8..d9df81f30 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -274,6 +274,150 @@ RSpec.describe Ticket, type: :model do end end end + + # https://github.com/zammad/zammad/issues/3105 + context 'when merge actions triggers exist', :performs_jobs do + before do + ticket && target_ticket + merged_into_trigger && received_merge_trigger && update_trigger + + allow_any_instance_of(described_class).to receive(:perform_changes) do |ticket, trigger| # rubocop:disable RSpec/AnyInstance + log << { ticket: ticket.id, trigger: trigger.id } + end + + perform_enqueued_jobs do + ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) + end + end + + let(:merged_into_trigger) { create(:trigger, :conditionable, condition_ticket_action: 'update.merged_into') } + let(:received_merge_trigger) { create(:trigger, :conditionable, condition_ticket_action: 'update.received_merge') } + let(:update_trigger) { create(:trigger, :conditionable, condition_ticket_action: 'update') } + + let(:log) { [] } + + it 'merge_into triggered with source ticket' do + expect(log).to include({ ticket: ticket.id, trigger: merged_into_trigger.id }) + end + + it 'received_merge not triggered with source ticket' do + expect(log).not_to include({ ticket: ticket.id, trigger: received_merge_trigger.id }) + end + + it 'update not triggered with source ticket' do + expect(log).not_to include({ ticket: ticket.id, trigger: update_trigger.id }) + end + + it 'merge_into not triggered with target ticket' do + expect(log).not_to include({ ticket: target_ticket.id, trigger: merged_into_trigger.id }) + end + + it 'received_merge triggered with target ticket' do + expect(log).to include({ ticket: target_ticket.id, trigger: received_merge_trigger.id }) + end + + it 'update not triggered with target ticket' do + expect(log).not_to include({ ticket: target_ticket.id, trigger: update_trigger.id }) + end + end + + # https://github.com/zammad/zammad/issues/3105 + context 'when user has notifications enabled', :performs_jobs do + before do + user + + allow(OnlineNotification).to receive(:add) do |**args| + next if args[:object] != 'Ticket' + + log << { type: :online, event: args[:type], ticket_id: args[:o_id], user_id: args[:user_id] } + end + + allow(NotificationFactory::Mailer).to receive(:notification) do |**args| + log << { type: :email, event: args[:template], ticket_id: args[:objects][:ticket].id, user_id: args[:user].id } + end + + perform_enqueued_jobs do + ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) + end + end + + let(:user) { create(:agent, :preferencable, notification_group_ids: [ticket, target_ticket].map(&:group_id), groups: [ticket, target_ticket].map(&:group)) } + let(:log) { [] } + + it 'merge_into notification sent with source ticket' do + expect(log).to include({ type: :online, event: 'update.merged_into', ticket_id: ticket.id, user_id: user.id }) + end + + it 'received_merge notification not sent with source ticket' do + expect(log).not_to include({ type: :online, event: 'update.received_merge', ticket_id: ticket.id, user_id: user.id }) + end + + it 'update notification not sent with source ticket' do + expect(log).not_to include({ type: :online, event: 'update', ticket_id: ticket.id, user_id: user.id }) + end + + it 'merge_into notification not sent with target ticket' do + expect(log).not_to include({ type: :online, event: 'update.merged_into', ticket_id: target_ticket.id, user_id: user.id }) + end + + it 'received_merge notification sent with target ticket' do + expect(log).to include({ type: :online, event: 'update.received_merge', ticket_id: target_ticket.id, user_id: user.id }) + end + + it 'update notification not sent with target ticket' do + expect(log).not_to include({ type: :online, event: 'update', ticket_id: target_ticket.id, user_id: user.id }) + end + + it 'merge_into email sent with source ticket' do + expect(log).to include({ type: :email, event: 'ticket_update_merged_into', ticket_id: ticket.id, user_id: user.id }) + end + + it 'received_merge email not sent with source ticket' do + expect(log).not_to include({ type: :email, event: 'ticket_update_received_merge', ticket_id: ticket.id, user_id: user.id }) + end + + it 'update email not sent with source ticket' do + expect(log).not_to include({ type: :email, event: 'ticket_update', ticket_id: ticket.id, user_id: user.id }) + end + + it 'merge_into email not sent with target ticket' do + expect(log).not_to include({ type: :email, event: 'ticket_update_merged_into', ticket_id: target_ticket.id, user_id: user.id }) + end + + it 'received_merge email sent with target ticket' do + expect(log).to include({ type: :email, event: 'ticket_update_received_merge', ticket_id: target_ticket.id, user_id: user.id }) + end + + it 'update email not sent with target ticket' do + expect(log).not_to include({ type: :email, event: 'ticket_update', ticket_id: target_ticket.id, user_id: user.id }) + end + end + + # https://github.com/zammad/zammad/issues/3105 + context 'when sending notification email correct template', :performs_jobs do + before do + user + + allow(NotificationFactory::Mailer).to receive(:send) do |**args| + log << args[:subject] + end + + perform_enqueued_jobs do + ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) + end + end + + let(:user) { create(:agent, :preferencable, notification_group_ids: [ticket, target_ticket].map(&:group_id), groups: [ticket, target_ticket].map(&:group)) } + let(:log) { [] } + + it 'is used for merged_into' do + expect(log).to include(start_with("Ticket (#{ticket.title}) was merged into another ticket")) + end + + it 'is used for received_merge' do + expect(log).to include(start_with("Another ticket was merged into ticket (#{target_ticket.title})")) + end + end end describe '#perform_changes' do diff --git a/spec/support/active_job.rb b/spec/support/active_job.rb index a1af672cf..9bec5362c 100644 --- a/spec/support/active_job.rb +++ b/spec/support/active_job.rb @@ -1,7 +1,6 @@ # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ module ZammadActiveJobHelper - delegate :enqueued_jobs, :performed_jobs, to: :queue_adapter def queue_adapter diff --git a/spec/system/ticket/zoom_spec.rb b/spec/system/ticket/zoom_spec.rb index b8a649843..7898d0c31 100644 --- a/spec/system/ticket/zoom_spec.rb +++ b/spec/system/ticket/zoom_spec.rb @@ -1706,4 +1706,60 @@ RSpec.describe 'Ticket zoom', type: :system do end end end + + describe 'merging', authenticated_as: :user do + before do + merged_into_trigger && received_merge_trigger && update_trigger + + visit "ticket/zoom/#{ticket.id}" + visit "ticket/zoom/#{target_ticket.id}" + + await_empty_ajax_queue + + ensure_websocket do + visit 'dashboard' + end + end + + let(:merged_into_trigger) { create(:trigger, :conditionable, condition_ticket_action: :merged_into) } + let(:received_merge_trigger) { create(:trigger, :conditionable, condition_ticket_action: :received_merge) } + let(:update_trigger) { create(:trigger, :conditionable, condition_ticket_action: :update) } + + let(:ticket) { create(:ticket) } + let(:target_ticket) { create(:ticket) } + + let(:user) { create(:agent, :preferencable, notification_group_ids: [ticket, target_ticket].map(&:group_id), groups: [ticket, target_ticket].map(&:group)) } + + context 'when merging ticket' do + before do + ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) + end + + it 'pulses source ticket' do + expect(page).to have_css("#navigation a.is-modified[data-key=\"Ticket-#{ticket.id}\"]") + end + + it 'pulses target ticket' do + expect(page).to have_css("#navigation a.is-modified[data-key=\"Ticket-#{target_ticket.id}\"]") + end + end + + context 'when merging and looking at online notifications', :performs_jobs do + before do + perform_enqueued_jobs do + ticket.merge_to(ticket_id: target_ticket.id, user_id: 1) + end + + find('.js-toggleNotifications').click + end + + it 'shows online notification for source ticket' do + expect(page).to have_text("Ticket #{ticket.title} was merged into another ticket") + end + + it 'shows online notification for target ticket' do + expect(page).to have_text("Another ticket was merged into ticket #{ticket.title}") + end + end + end end