diff --git a/enterprise/app/jobs/sla/process_account_applied_slas_job.rb b/enterprise/app/jobs/sla/process_account_applied_slas_job.rb index 153749267..d8786565c 100644 --- a/enterprise/app/jobs/sla/process_account_applied_slas_job.rb +++ b/enterprise/app/jobs/sla/process_account_applied_slas_job.rb @@ -2,7 +2,7 @@ class Sla::ProcessAccountAppliedSlasJob < ApplicationJob queue_as :medium def perform(account) - account.applied_slas.where(sla_status: 'active').each do |applied_sla| + account.applied_slas.where(sla_status: %w[active active_with_misses]).each do |applied_sla| Sla::ProcessAppliedSlaJob.perform_later(applied_sla) end end diff --git a/enterprise/app/models/applied_sla.rb b/enterprise/app/models/applied_sla.rb index 4558d5d23..111b78e84 100644 --- a/enterprise/app/models/applied_sla.rb +++ b/enterprise/app/models/applied_sla.rb @@ -27,7 +27,7 @@ class AppliedSla < ApplicationRecord validates :account_id, uniqueness: { scope: %i[sla_policy_id conversation_id] } before_validation :ensure_account_id - enum sla_status: { active: 0, hit: 1, missed: 2 } + enum sla_status: { active: 0, hit: 1, missed: 2, active_with_misses: 3 } scope :filter_by_date_range, ->(range) { where(created_at: range) if range.present? } scope :filter_by_inbox_id, ->(inbox_id) { where(inbox_id: inbox_id) if inbox_id.present? } diff --git a/enterprise/app/models/sla_event.rb b/enterprise/app/models/sla_event.rb index 28f709346..59068684e 100644 --- a/enterprise/app/models/sla_event.rb +++ b/enterprise/app/models/sla_event.rb @@ -32,6 +32,8 @@ class SlaEvent < ApplicationRecord before_validation :ensure_applied_sla_id, :ensure_account_id, :ensure_inbox_id, :ensure_sla_policy_id + after_create_commit :create_notifications + private def ensure_applied_sla_id @@ -49,4 +51,28 @@ class SlaEvent < ApplicationRecord def ensure_sla_policy_id self.sla_policy_id ||= applied_sla&.sla_policy_id end + + def create_notifications + notify_users = conversation.conversation_participants.map(&:user) + # Add all admins from the account to notify list + notify_users += account.administrators + # Ensure conversation assignee is notified + notify_users += [conversation.assignee] if conversation.assignee.present? + + notification_type = { + 'frt' => 'sla_missed_first_response', + 'nrt' => 'sla_missed_next_response', + 'rt' => 'sla_missed_resolution' + }[event_type] + + notify_users.uniq.each do |user| + NotificationBuilder.new( + notification_type: notification_type, + user: user, + account: account, + primary_actor: conversation, + secondary_actor: sla_policy + ).perform + end + end end diff --git a/enterprise/app/services/sla/evaluate_applied_sla_service.rb b/enterprise/app/services/sla/evaluate_applied_sla_service.rb index d6eb9839e..4cc953899 100644 --- a/enterprise/app/services/sla/evaluate_applied_sla_service.rb +++ b/enterprise/app/services/sla/evaluate_applied_sla_service.rb @@ -7,8 +7,8 @@ class Sla::EvaluateAppliedSlaService # We will calculate again in the next iteration return unless applied_sla.conversation.resolved? - # No SLA missed, so marking as hit as conversation is resolved - handle_hit_sla(applied_sla) if applied_sla.active? + # after conversation is resolved, we will check if the SLA was hit or missed + handle_hit_sla(applied_sla) end private @@ -49,6 +49,14 @@ class Sla::EvaluateAppliedSlaService handle_missed_sla(applied_sla, 'nrt') end + def get_last_message_id(conversation) + conversation.messages.where(message_type: :incoming).last&.id + end + + def already_missed?(applied_sla, type, meta = {}) + SlaEvent.exists?(applied_sla: applied_sla, event_type: type, meta: meta) + end + def check_resolution_time_threshold(applied_sla, conversation, sla_policy) return if conversation.resolved? @@ -58,48 +66,41 @@ class Sla::EvaluateAppliedSlaService handle_missed_sla(applied_sla, 'rt') end - def handle_missed_sla(applied_sla, type) - return unless applied_sla.active? + def handle_missed_sla(applied_sla, type, meta = {}) + meta = { message_id: get_last_message_id(applied_sla.conversation) } if type == 'nrt' + return if already_missed?(applied_sla, type, meta) - applied_sla.update!(sla_status: 'missed') - generate_notifications_for_sla(applied_sla, type) - Rails.logger.warn "SLA missed for conversation #{applied_sla.conversation.id} " \ + create_sla_event(applied_sla, type, meta) + Rails.logger.warn "SLA #{type} missed for conversation #{applied_sla.conversation.id} " \ "in account #{applied_sla.account_id} " \ "for sla_policy #{applied_sla.sla_policy.id}" + + applied_sla.update!(sla_status: 'active_with_misses') if applied_sla.sla_status != 'active_with_misses' end def handle_hit_sla(applied_sla) - return unless applied_sla.active? - - applied_sla.update!(sla_status: 'hit') - Rails.logger.info "SLA hit for conversation #{applied_sla.conversation.id} " \ - "in account #{applied_sla.account_id} " \ - "for sla_policy #{applied_sla.sla_policy.id}" - end - - def generate_notifications_for_sla(applied_sla, type) - notify_users = applied_sla.conversation.conversation_participants.map(&:user) - # add all admins from the account to notify list - notify_users += applied_sla.account.administrators - # ensure conversation assignee is notified - notify_users += [applied_sla.conversation.assignee] if applied_sla.conversation.assignee.present? - - notification_type = if type == 'frt' - 'sla_missed_first_response' - elsif type == 'nrt' - 'sla_missed_next_response' - else - 'sla_missed_resolution' - end - - notify_users.uniq.each do |user| - NotificationBuilder.new( - notification_type: notification_type, - user: user, - account: applied_sla.account, - primary_actor: applied_sla.conversation, - secondary_actor: applied_sla.sla_policy - ).perform + if applied_sla.active? + applied_sla.update!(sla_status: 'hit') + Rails.logger.info "SLA hit for conversation #{applied_sla.conversation.id} " \ + "in account #{applied_sla.account_id} " \ + "for sla_policy #{applied_sla.sla_policy.id}" + else + applied_sla.update!(sla_status: 'missed') + Rails.logger.info "SLA missed for conversation #{applied_sla.conversation.id} " \ + "in account #{applied_sla.account_id} " \ + "for sla_policy #{applied_sla.sla_policy.id}" end end + + def create_sla_event(applied_sla, event_type, meta = {}) + SlaEvent.create!( + applied_sla: applied_sla, + conversation: applied_sla.conversation, + event_type: event_type, + meta: meta, + account: applied_sla.account, + inbox: applied_sla.conversation.inbox, + sla_policy: applied_sla.sla_policy + ) + end end diff --git a/spec/enterprise/jobs/sla/process_account_applied_slas_job_spec.rb b/spec/enterprise/jobs/sla/process_account_applied_slas_job_spec.rb index beae967db..5d628f71e 100644 --- a/spec/enterprise/jobs/sla/process_account_applied_slas_job_spec.rb +++ b/spec/enterprise/jobs/sla/process_account_applied_slas_job_spec.rb @@ -7,18 +7,20 @@ RSpec.describe Sla::ProcessAccountAppliedSlasJob do let!(:applied_sla) { create(:applied_sla, account: account, sla_policy: sla_policy, sla_status: 'active') } let!(:hit_applied_sla) { create(:applied_sla, account: account, sla_policy: sla_policy, sla_status: 'hit') } let!(:miss_applied_sla) { create(:applied_sla, account: account, sla_policy: sla_policy, sla_status: 'missed') } + let!(:active_with_misses_applied_sla) { create(:applied_sla, account: account, sla_policy: sla_policy, sla_status: 'active_with_misses') } it 'enqueues the job' do expect { described_class.perform_later }.to have_enqueued_job(described_class) .on_queue('medium') end - it 'calls the ProcessAppliedSlaJob' do + it 'calls the ProcessAppliedSlaJob for both active and active_with_misses' do + expect(Sla::ProcessAppliedSlaJob).to receive(:perform_later).with(active_with_misses_applied_sla).and_call_original expect(Sla::ProcessAppliedSlaJob).to receive(:perform_later).with(applied_sla).and_call_original described_class.perform_now(account) end - it 'does not call the ProcessAppliedSlaJob for not active applied slas' do + it 'does not call the ProcessAppliedSlaJob for applied slas that are hit or miss' do expect(Sla::ProcessAppliedSlaJob).not_to receive(:perform_later).with(hit_applied_sla) expect(Sla::ProcessAppliedSlaJob).not_to receive(:perform_later).with(miss_applied_sla) described_class.perform_now(account) diff --git a/spec/enterprise/models/sla_event_spec.rb b/spec/enterprise/models/sla_event_spec.rb index 862b22e6c..3c44d3961 100644 --- a/spec/enterprise/models/sla_event_spec.rb +++ b/spec/enterprise/models/sla_event_spec.rb @@ -25,4 +25,37 @@ RSpec.describe SlaEvent, type: :model do expect(sla_event.sla_policy_id).to eq sla_event.applied_sla.sla_policy_id end end + + describe 'create notifications' do + # create account, user and inbox + let!(:account) { create(:account) } + let!(:assignee) { create(:user, account: account) } + let!(:participant) { create(:user, account: account) } + let!(:admin) { create(:user, account: account, role: :administrator) } + let!(:inbox) { create(:inbox, account: account) } + let(:conversation) { create(:conversation, inbox: inbox, assignee: assignee, account: account) } + let(:sla_policy) { create(:sla_policy, account: conversation.account) } + let(:sla_event) { create(:sla_event, event_type: 'frt', conversation: conversation, sla_policy: sla_policy) } + + before do + # to ensure notifications are not sent to other users + create(:user, account: account) + create(:inbox_member, inbox: inbox, user: participant) + create(:conversation_participant, conversation: conversation, user: participant) + end + + it 'creates notifications for conversation participants, admins, and assignee' do + sla_event + + expect(Notification.count).to eq(3) + # check if notification type is sla_missed_first_response + expect(Notification.where(notification_type: 'sla_missed_first_response').count).to eq(3) + # Check if notification is created for the assignee + expect(Notification.where(user_id: assignee.id).count).to eq(1) + # Check if notification is created for the account admin + expect(Notification.where(user_id: admin.id).count).to eq(1) + # Check if notification is created for participant + expect(Notification.where(user_id: participant.id).count).to eq(1) + end + end end diff --git a/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb b/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb index 12cb59d35..f6cd657be 100644 --- a/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb +++ b/spec/enterprise/services/sla/evaluate_applied_sla_service_spec.rb @@ -3,8 +3,6 @@ require 'rails_helper' RSpec.describe Sla::EvaluateAppliedSlaService do let!(:account) { create(:account) } let!(:user_1) { create(:user, account: account) } - let!(:user_2) { create(:user, account: account) } - let!(:admin) { create(:user, account: account, role: :administrator) } let!(:sla_policy) do create(:sla_policy, @@ -28,19 +26,17 @@ RSpec.describe Sla::EvaluateAppliedSlaService do it 'updates the SLA status to missed and logs a warning' do allow(Rails.logger).to receive(:warn) described_class.new(applied_sla: applied_sla).perform - expect(Rails.logger).to have_received(:warn).with("SLA missed for conversation #{conversation.id} in account " \ + expect(Rails.logger).to have_received(:warn).with("SLA frt missed for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}") - expect(applied_sla.reload.sla_status).to eq('missed') + expect(applied_sla.reload.sla_status).to eq('active_with_misses') + end - expect(Notification.count).to eq(2) - # check if notification type is sla_missed_first_response - expect(Notification.where(notification_type: 'sla_missed_first_response').count).to eq(2) - # Check if notification is created for the assignee - expect(Notification.where(user_id: user_1.id).count).to eq(1) - # Check if notification is created for the account admin - expect(Notification.where(user_id: admin.id).count).to eq(1) - # Check if no notification is created for other user - expect(Notification.where(user_id: user_2.id).count).to eq(0) + it 'creates SlaEvent only for frt miss' do + described_class.new(applied_sla: applied_sla).perform + + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(1) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'nrt').count).to eq(0) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'rt').count).to eq(0) end end @@ -53,19 +49,17 @@ RSpec.describe Sla::EvaluateAppliedSlaService do it 'updates the SLA status to missed and logs a warning' do allow(Rails.logger).to receive(:warn) described_class.new(applied_sla: applied_sla).perform - expect(Rails.logger).to have_received(:warn).with("SLA missed for conversation #{conversation.id} in account " \ + expect(Rails.logger).to have_received(:warn).with("SLA nrt missed for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}") - expect(applied_sla.reload.sla_status).to eq('missed') + expect(applied_sla.reload.sla_status).to eq('active_with_misses') + end - expect(Notification.count).to eq(2) - # check if notification type is sla_missed_first_response - expect(Notification.where(notification_type: 'sla_missed_next_response').count).to eq(2) - # Check if notification is created for the assignee - expect(Notification.where(user_id: user_1.id).count).to eq(1) - # Check if notification is created for the account admin - expect(Notification.where(user_id: admin.id).count).to eq(1) - # Check if no notification is created for other user - expect(Notification.where(user_id: user_2.id).count).to eq(0) + it 'creates SlaEvent only for nrt miss' do + described_class.new(applied_sla: applied_sla).perform + + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(0) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'nrt').count).to eq(1) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'rt').count).to eq(0) end end @@ -75,18 +69,18 @@ RSpec.describe Sla::EvaluateAppliedSlaService do it 'updates the SLA status to missed and logs a warning' do allow(Rails.logger).to receive(:warn) described_class.new(applied_sla: applied_sla).perform - expect(Rails.logger).to have_received(:warn).with("SLA missed for conversation #{conversation.id} in account " \ + expect(Rails.logger).to have_received(:warn).with("SLA rt missed for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}") - expect(applied_sla.reload.sla_status).to eq('missed') - expect(Notification.count).to eq(2) - expect(Notification.where(notification_type: 'sla_missed_resolution').count).to eq(2) - # Check if notification is created for the assignee - expect(Notification.where(user_id: user_1.id).count).to eq(1) - # Check if notification is created for the account admin - expect(Notification.where(user_id: admin.id).count).to eq(1) - # Check if no notification is created for other user - expect(Notification.where(user_id: user_2.id).count).to eq(0) + expect(applied_sla.reload.sla_status).to eq('active_with_misses') + end + + it 'creates SlaEvent only for rt miss' do + described_class.new(applied_sla: applied_sla).perform + + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(0) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'nrt').count).to eq(0) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'rt').count).to eq(1) end end @@ -110,13 +104,14 @@ RSpec.describe Sla::EvaluateAppliedSlaService do conversation.update(first_reply_created_at: 5.hours.ago, waiting_since: 5.hours.ago) end - it 'updates the SLA status to missed and logs a warning' do + it 'updates the SLA status to missed and logs multiple warnings' do allow(Rails.logger).to receive(:warn) described_class.new(applied_sla: applied_sla).perform - expect(Rails.logger).to have_received(:warn).with("SLA missed for conversation #{conversation.id} in account " \ + expect(Rails.logger).to have_received(:warn).with("SLA rt missed for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}").exactly(1).time - expect(applied_sla.reload.sla_status).to eq('missed') - expect(Notification.count).to eq(2) + expect(Rails.logger).to have_received(:warn).with("SLA nrt missed for conversation #{conversation.id} in account " \ + "#{applied_sla.account_id} for sla_policy #{sla_policy.id}").exactly(1).time + expect(applied_sla.reload.sla_status).to eq('active_with_misses') end end end @@ -140,6 +135,7 @@ RSpec.describe Sla::EvaluateAppliedSlaService do expect(Rails.logger).to have_received(:info).with("SLA hit for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}") expect(applied_sla.reload.sla_status).to eq('hit') + expect(SlaEvent.count).to eq(0) expect(Notification.count).to eq(0) end end @@ -162,6 +158,7 @@ RSpec.describe Sla::EvaluateAppliedSlaService do expect(Rails.logger).to have_received(:info).with("SLA hit for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}") expect(applied_sla.reload.sla_status).to eq('hit') + expect(SlaEvent.count).to eq(0) end end @@ -177,7 +174,52 @@ RSpec.describe Sla::EvaluateAppliedSlaService do expect(Rails.logger).to have_received(:info).with("SLA hit for conversation #{conversation.id} in account " \ "#{applied_sla.account_id} for sla_policy #{sla_policy.id}") expect(applied_sla.reload.sla_status).to eq('hit') + expect(SlaEvent.count).to eq(0) end end end + + describe 'SLA evaluation with frt hit, multiple nrt misses and rt miss' do + before do + # Setup SLA Policy thresholds + sla_policy.update( + first_response_time_threshold: 2.hours, # Hit frt + next_response_time_threshold: 1.hour, # Miss nrt multiple times + resolution_time_threshold: 4.hours # Miss rt + ) + + # Simulate conversation timeline + # Hit frt + # incoming message from customer + create(:message, conversation: conversation, created_at: 6.hours.ago, message_type: :incoming) + # outgoing message from agent within frt + create(:message, conversation: conversation, created_at: 5.hours.ago, message_type: :outgoing) + + # Miss nrt first time + create(:message, conversation: conversation, created_at: 4.hours.ago, message_type: :incoming) + described_class.new(applied_sla: applied_sla).perform + + # Miss nrt second time + create(:message, conversation: conversation, created_at: 3.hours.ago, message_type: :incoming) + described_class.new(applied_sla: applied_sla).perform + + # Conversation is resolved missing rt + conversation.update(status: 'resolved') + + # this will not create a new notification for rt miss as conversation is resolved + # but we would have already created an rt miss notification during previous evaluation + described_class.new(applied_sla: applied_sla).perform + end + + it 'updates the SLA status to missed' do + # the status would be missed as the conversation is resolved + expect(applied_sla.reload.sla_status).to eq('missed') + end + + it 'creates necessary sla events' do + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(0) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'nrt').count).to eq(2) + expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'rt').count).to eq(1) + end + end end