feat: refactor SLA evaluation logic (#9133)

* feat: update SLA evaluation logic

* chore: handle nrt

* chore: handle applied_sla status

* chore: refactor spec to bring down expecations in a single block

* chore: fix process_account_applied_sla spec

* chore: add spec to test multiple nrt misses

* feat: persist sla notifications

* feat: revert persist sla notifications

* chore: refactor sla_status to include active_with_misses

* chore: refactor spec

* Update evaluate_applied_sla_service.rb

* minor refactors

* clean up

* move notification related spec

* chore: refactor notifications spec to sla_event model

---------

Co-authored-by: Muhsin Keloth <muhsinkeramam@gmail.com>
Co-authored-by: Sojan Jose <sojan@pepalo.com>

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

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? }

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

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
+    # after conversation is resolved, we will check if the SLA was hit or missed
-    handle_hit_sla(applied_sla) if applied_sla.active?
+    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)
+  def handle_missed_sla(applied_sla, type, meta = {})
-    return unless applied_sla.active?
+    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')
+    create_sla_event(applied_sla, type, meta)
-    generate_notifications_for_sla(applied_sla, type)
+    Rails.logger.warn "SLA #{type} missed for conversation #{applied_sla.conversation.id} " \
-    Rails.logger.warn "SLA 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?
+    if applied_sla.active?
-
+      applied_sla.update!(sla_status: 'hit')
-    applied_sla.update!(sla_status: 'hit')
+      Rails.logger.info "SLA hit for conversation #{applied_sla.conversation.id} " \
-    Rails.logger.info "SLA hit for conversation #{applied_sla.conversation.id} " \
+                        "in account #{applied_sla.account_id} " \
-                      "in account #{applied_sla.account_id} " \
+                        "for sla_policy #{applied_sla.sla_policy.id}"
-                      "for sla_policy #{applied_sla.sla_policy.id}"
+    else
-  end
+      applied_sla.update!(sla_status: 'missed')
-
+      Rails.logger.info "SLA missed for conversation #{applied_sla.conversation.id} " \
-  def generate_notifications_for_sla(applied_sla, type)
+                        "in account #{applied_sla.account_id} " \
-    notify_users = applied_sla.conversation.conversation_participants.map(&:user)
+                        "for sla_policy #{applied_sla.sla_policy.id}"
-    # 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
     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

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)

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

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)
+      it 'creates SlaEvent only for frt miss' do
-        # check if notification type is sla_missed_first_response
+        described_class.new(applied_sla: applied_sla).perform
-        expect(Notification.where(notification_type: 'sla_missed_first_response').count).to eq(2)
+
-        # Check if notification is created for the assignee
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(1)
-        expect(Notification.where(user_id: user_1.id).count).to eq(1)
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'nrt').count).to eq(0)
-        # Check if notification is created for the account admin
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'rt').count).to eq(0)
-        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)
       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)
+      it 'creates SlaEvent only for nrt miss' do
-        # check if notification type is sla_missed_first_response
+        described_class.new(applied_sla: applied_sla).perform
-        expect(Notification.where(notification_type: 'sla_missed_next_response').count).to eq(2)
+
-        # Check if notification is created for the assignee
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(0)
-        expect(Notification.where(user_id: user_1.id).count).to eq(1)
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'nrt').count).to eq(1)
-        # Check if notification is created for the account admin
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'rt').count).to eq(0)
-        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)
       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(applied_sla.reload.sla_status).to eq('active_with_misses')
-        expect(Notification.where(notification_type: 'sla_missed_resolution').count).to eq(2)
+      end
-        # Check if notification is created for the assignee
+
-        expect(Notification.where(user_id: user_1.id).count).to eq(1)
+      it 'creates SlaEvent only for rt miss' do
-        # Check if notification is created for the account admin
+        described_class.new(applied_sla: applied_sla).perform
-        expect(Notification.where(user_id: admin.id).count).to eq(1)
+
-        # Check if no notification is created for other user
+        expect(SlaEvent.where(applied_sla: applied_sla, event_type: 'frt').count).to eq(0)
-        expect(Notification.where(user_id: user_2.id).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(Rails.logger).to have_received(:warn).with("SLA nrt missed for conversation #{conversation.id} in account " \
-        expect(Notification.count).to eq(2)
+                                                          "#{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