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 @@ def ensure_inbox_id
   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 @@ def perform
     # 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 @@ def check_next_response_time_threshold(applied_sla, conversation, sla_policy)
     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 @@ def check_resolution_time_threshold(applied_sla, conversation, sla_policy)
     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}"
+    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 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
-    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 @@
     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 @@
       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