From 9a1c54a82d55b2eab3db1f1685cdaf56e0b42ee9 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Thu, 28 Mar 2024 13:14:16 +0530 Subject: [PATCH 1/3] feat: disable automation rules if condition fails multiple times (#9017) * feat: add email for disabling automation rule * feat: disable automation rules and notify admin * feat: reset error count after update * feat: trigger invalid_condition_error if rule is invalid * feat: setup error trackable concern * refactor: use ErrorTrackable in Reauthorizable * fix: optional argument * feat: separate reauthorization_required_key * test: update case to use ERROR_TRACKABLE_COUNT * Revert "test: update case to use ERROR_TRACKABLE_COUNT" This reverts commit f439847147556a02759a7597a7fcf1d66091cafc. * Revert "feat: separate reauthorization_required_key" This reverts commit f4514fce217b0a2f2c2bf701a15de0a8b47acbc4. * Revert "fix: optional argument" This reverts commit 93b4194ec3f10f67e2402388c966c071c4d3b4fd. * Revert "refactor: use ErrorTrackable in Reauthorizable" This reverts commit 513c2a522bc782e73ea4b0f5ae34ce01e70e042c. * Revert "feat: setup error trackable concern" This reverts commit 278683060cf422f60af5d5c77100aa5272141141. * feat: use reauthorizable for automation rule * feat: remove redis key * test: fix method names * chore: refactor --------- Co-authored-by: Vishnu Narayanan Co-authored-by: Sojan --- .../channel_notifications_mailer.rb | 12 +++++++ app/models/automation_rule.rb | 3 ++ app/models/concerns/reauthorizable.rb | 3 ++ .../conditions_filter_service.rb | 2 +- .../automation_rule_disabled.liquid | 8 +++++ spec/models/automation_rule_spec.rb | 36 +++++++++++++++++++ spec/models/concerns/reauthorizable_shared.rb | 9 +++++ 7 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 app/views/mailers/administrator_notifications/channel_notifications_mailer/automation_rule_disabled.liquid diff --git a/app/mailers/administrator_notifications/channel_notifications_mailer.rb b/app/mailers/administrator_notifications/channel_notifications_mailer.rb index 6c0f7cee2..dc4e6d7fe 100644 --- a/app/mailers/administrator_notifications/channel_notifications_mailer.rb +++ b/app/mailers/administrator_notifications/channel_notifications_mailer.rb @@ -69,6 +69,18 @@ class AdministratorNotifications::ChannelNotificationsMailer < ApplicationMailer send_mail_with_liquid(to: email_to, subject: subject) and return end + def automation_rule_disabled(rule) + return unless smtp_config_set_or_development? + + @action_url ||= "#{ENV.fetch('FRONTEND_URL', nil)}/app/accounts/#{Current.account.id}/settings/automation/list" + + subject = 'Automation rule disabled due to validation errors.'.freeze + @meta = {} + @meta['rule_name'] = rule.name + + send_mail_with_liquid(to: admin_emails, subject: subject) and return + end + private def admin_emails diff --git a/app/models/automation_rule.rb b/app/models/automation_rule.rb index 955df7497..9f7da28a0 100644 --- a/app/models/automation_rule.rb +++ b/app/models/automation_rule.rb @@ -19,6 +19,7 @@ # class AutomationRule < ApplicationRecord include Rails.application.routes.url_helpers + include Reauthorizable belongs_to :account has_many_attached :files @@ -28,6 +29,8 @@ class AutomationRule < ApplicationRecord validate :query_operator_presence validates :account_id, presence: true + after_update_commit :reauthorized!, if: -> { saved_change_to_conditions? } + scope :active, -> { where(active: true) } def conditions_attributes diff --git a/app/models/concerns/reauthorizable.rb b/app/models/concerns/reauthorizable.rb index fa46d0060..0fc234ada 100644 --- a/app/models/concerns/reauthorizable.rb +++ b/app/models/concerns/reauthorizable.rb @@ -50,6 +50,9 @@ module Reauthorizable mailer.whatsapp_disconnect(inbox).deliver_later when 'Channel::Email' mailer.email_disconnect(inbox).deliver_later + when 'AutomationRule' + update!(active: false) + mailer.automation_rule_disabled(self).deliver_later end end diff --git a/app/services/automation_rules/conditions_filter_service.rb b/app/services/automation_rules/conditions_filter_service.rb index daf9cb7e2..23873371d 100644 --- a/app/services/automation_rules/conditions_filter_service.rb +++ b/app/services/automation_rules/conditions_filter_service.rb @@ -45,8 +45,8 @@ class AutomationRules::ConditionsFilterService < FilterService def rule_valid? is_valid = AutomationRules::ConditionValidationService.new(@rule).perform - Rails.logger.info "Automation rule condition validation failed for rule id: #{@rule.id}" unless is_valid + @rule.authorization_error! unless is_valid is_valid end diff --git a/app/views/mailers/administrator_notifications/channel_notifications_mailer/automation_rule_disabled.liquid b/app/views/mailers/administrator_notifications/channel_notifications_mailer/automation_rule_disabled.liquid new file mode 100644 index 000000000..56aefd6b7 --- /dev/null +++ b/app/views/mailers/administrator_notifications/channel_notifications_mailer/automation_rule_disabled.liquid @@ -0,0 +1,8 @@ +

Hello there,

+ +

The automation rule {{meta['rule_name']}} has been disabled becuase it has invalid conditions.

+

This typically happens when you delete any custom attributes which are still being used in automation rules.

+ +

+Click here to update the conditions. +

diff --git a/spec/models/automation_rule_spec.rb b/spec/models/automation_rule_spec.rb index a20d3d71d..53ebfa0c7 100644 --- a/spec/models/automation_rule_spec.rb +++ b/spec/models/automation_rule_spec.rb @@ -1,6 +1,11 @@ require 'rails_helper' +require Rails.root.join 'spec/models/concerns/reauthorizable_shared.rb' RSpec.describe AutomationRule do + describe 'concerns' do + it_behaves_like 'reauthorizable' + end + describe 'associations' do let(:account) { create(:account) } let(:params) do @@ -56,4 +61,35 @@ RSpec.describe AutomationRule do expect(rule.errors.messages[:conditions]).to eq(['Automation conditions should have query operator.']) end end + + describe 'reauthorizable' do + context 'when prompt_reauthorization!' do + it 'marks the rule inactive' do + rule = create(:automation_rule) + expect(rule.active).to be true + rule.prompt_reauthorization! + expect(rule.active).to be false + end + end + + context 'when reauthorization_required?' do + it 'unsets the error count if conditions are updated' do + rule = create(:automation_rule) + rule.prompt_reauthorization! + expect(rule.reauthorization_required?).to be true + + rule.update!(conditions: [{ attribute_key: 'browser_language', filter_operator: 'equal_to', values: ['en'], query_operator: 'AND' }]) + expect(rule.reauthorization_required?).to be false + end + + it 'will not unset the error count if conditions are not updated' do + rule = create(:automation_rule) + rule.prompt_reauthorization! + expect(rule.reauthorization_required?).to be true + + rule.update!(name: 'Updated name') + expect(rule.reauthorization_required?).to be true + end + end + end end diff --git a/spec/models/concerns/reauthorizable_shared.rb b/spec/models/concerns/reauthorizable_shared.rb index 0bfa112c4..9efe232e8 100644 --- a/spec/models/concerns/reauthorizable_shared.rb +++ b/spec/models/concerns/reauthorizable_shared.rb @@ -25,10 +25,19 @@ shared_examples_for 'reauthorizable' do it 'prompt_reauthorization!' do obj = FactoryBot.create(model.to_s.underscore.tr('/', '_').to_sym) + mailer = double + mailer_method = double + allow(AdministratorNotifications::ChannelNotificationsMailer).to receive(:with).and_return(mailer) + # allow mailer to receive any methods and return mailer + allow(mailer).to receive(:method_missing).and_return(mailer_method) + allow(mailer_method).to receive(:deliver_later) + expect(obj.reauthorization_required?).to be false obj.prompt_reauthorization! expect(obj.reauthorization_required?).to be true + expect(AdministratorNotifications::ChannelNotificationsMailer).to have_received(:with).with(account: obj.account) + expect(mailer_method).to have_received(:deliver_later) end it 'reauthorized!' do From 6956436a76c7fe6f7e32044cc8b216f9c4780b47 Mon Sep 17 00:00:00 2001 From: Vishnu Narayanan Date: Fri, 29 Mar 2024 02:01:43 +1100 Subject: [PATCH 2/3] 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 Co-authored-by: Sojan Jose --- .../sla/process_account_applied_slas_job.rb | 2 +- enterprise/app/models/applied_sla.rb | 2 +- enterprise/app/models/sla_event.rb | 26 ++++ .../sla/evaluate_applied_sla_service.rb | 77 ++++++------ .../process_account_applied_slas_job_spec.rb | 6 +- spec/enterprise/models/sla_event_spec.rb | 33 +++++ .../sla/evaluate_applied_sla_service_spec.rb | 118 ++++++++++++------ 7 files changed, 184 insertions(+), 80 deletions(-) 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 From 16282f6a66741dd62dfa56fa4e50173272032c36 Mon Sep 17 00:00:00 2001 From: Vishnu Narayanan Date: Fri, 29 Mar 2024 20:27:21 +1100 Subject: [PATCH 3/3] feat: Add push/email notification support for SLA (#9140) * feat: update SLA evaluation logic * Update enterprise/app/services/sla/evaluate_applied_sla_service.rb Co-authored-by: Muhsin Keloth * 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 * feat: add SLA push/email notification support * chore: refactor sla_status to include active_with_misses * chore: add support for sla push/email notifications * chore: refactor * chore: add liquid templates * chore: add spec for liquid templates * chore: add spec for sla email notifications * chore: add spec for SlaPolicyDrop * chore: refactor to ee namespace * chore: set enterprise test type to mailer * feat: enable sla notification settings only if SLA enabled * chore: refactor * chore: fix spec --------- Co-authored-by: Muhsin Keloth --- .../dashboard/i18n/locale/en/settings.json | 10 +- .../settings/profile/NotificationSettings.vue | 105 ++++++++++++++++++ .../conversation_notifications_mailer.rb | 5 +- app/models/application_record.rb | 8 +- app/models/notification.rb | 4 +- .../sla_missed_first_response.liquid | 10 ++ .../sla_missed_next_response.liquid | 10 ++ .../sla_missed_resolution.liquid | 10 ++ enterprise/app/drops/sla_policy_drop.rb | 9 ++ .../conversation_notifications_mailer.rb | 32 ++++++ .../models/enterprise/application_record.rb | 5 + spec/enterprise/drops/sla_policy_drop_spec.rb | 15 +++ .../conversation_notifications_mailer_spec.rb | 54 +++++++++ 13 files changed, 270 insertions(+), 7 deletions(-) create mode 100644 app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_first_response.liquid create mode 100644 app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_next_response.liquid create mode 100644 app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_resolution.liquid create mode 100644 enterprise/app/drops/sla_policy_drop.rb create mode 100644 enterprise/app/mailers/enterprise/agent_notifications/conversation_notifications_mailer.rb create mode 100644 enterprise/app/models/enterprise/application_record.rb create mode 100644 spec/enterprise/drops/sla_policy_drop_spec.rb create mode 100644 spec/enterprise/mailers/enterprise/agent_notifications/conversation_notifications_mailer_spec.rb diff --git a/app/javascript/dashboard/i18n/locale/en/settings.json b/app/javascript/dashboard/i18n/locale/en/settings.json index 9a4bde2c8..d9834c545 100644 --- a/app/javascript/dashboard/i18n/locale/en/settings.json +++ b/app/javascript/dashboard/i18n/locale/en/settings.json @@ -83,7 +83,10 @@ "CONVERSATION_CREATION": "Send email notifications when a new conversation is created", "CONVERSATION_MENTION": "Send email notifications when you are mentioned in a conversation", "ASSIGNED_CONVERSATION_NEW_MESSAGE": "Send email notifications when a new message is created in an assigned conversation", - "PARTICIPATING_CONVERSATION_NEW_MESSAGE": "Send email notifications when a new message is created in a participating conversation" + "PARTICIPATING_CONVERSATION_NEW_MESSAGE": "Send email notifications when a new message is created in a participating conversation", + "SLA_MISSED_FIRST_RESPONSE": "Send email notifications when a conversation misses first response SLA", + "SLA_MISSED_NEXT_RESPONSE": "Send email notifications when a conversation misses next response SLA", + "SLA_MISSED_RESOLUTION": "Send email notifications when a conversation misses resolution SLA" }, "API": { "UPDATE_SUCCESS": "Your notification preferences are updated successfully", @@ -98,7 +101,10 @@ "ASSIGNED_CONVERSATION_NEW_MESSAGE": "Send push notifications when a new message is created in an assigned conversation", "PARTICIPATING_CONVERSATION_NEW_MESSAGE": "Send push notifications when a new message is created in a participating conversation", "HAS_ENABLED_PUSH": "You have enabled push for this browser.", - "REQUEST_PUSH": "Enable push notifications" + "REQUEST_PUSH": "Enable push notifications", + "SLA_MISSED_FIRST_RESPONSE": "Send push notifications when a conversation misses first response SLA", + "SLA_MISSED_NEXT_RESPONSE": "Send push notifications when a conversation misses next response SLA", + "SLA_MISSED_RESOLUTION": "Send push notifications when a conversation misses resolution SLA" }, "PROFILE_IMAGE": { "LABEL": "Profile Image" diff --git a/app/javascript/dashboard/routes/dashboard/settings/profile/NotificationSettings.vue b/app/javascript/dashboard/routes/dashboard/settings/profile/NotificationSettings.vue index 7214d0deb..cdbc40ec4 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/profile/NotificationSettings.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/profile/NotificationSettings.vue @@ -236,6 +236,54 @@ }} +
+ + +
+
+ + +
+
+ + +
+ +
+ + +
+ +
+ + +
+ +
+ + +
@@ -367,6 +466,7 @@ import { requestPushPermissions, verifyServiceWorkerExistence, } from '../../../../helper/pushHelper'; +import { FEATURE_FLAGS } from 'dashboard/featureFlags'; export default { mixins: [alertMixin, configMixin, uiSettingsMixin], @@ -393,13 +493,18 @@ export default { }, computed: { ...mapGetters({ + accountId: 'getCurrentAccountId', emailFlags: 'userNotificationSettings/getSelectedEmailFlags', pushFlags: 'userNotificationSettings/getSelectedPushFlags', uiSettings: 'getUISettings', + isFeatureEnabledonAccount: 'accounts/isFeatureEnabledonAccount', }), hasPushAPISupport() { return !!('Notification' in window); }, + isSLAEnabled() { + return this.isFeatureEnabledonAccount(this.accountId, FEATURE_FLAGS.SLA); + }, }, watch: { emailFlags(value) { diff --git a/app/mailers/agent_notifications/conversation_notifications_mailer.rb b/app/mailers/agent_notifications/conversation_notifications_mailer.rb index 874449adf..bc498d43b 100644 --- a/app/mailers/agent_notifications/conversation_notifications_mailer.rb +++ b/app/mailers/agent_notifications/conversation_notifications_mailer.rb @@ -61,7 +61,10 @@ class AgentNotifications::ConversationNotificationsMailer < ApplicationMailer user: @agent, conversation: @conversation, inbox: @conversation.inbox, - message: @message + message: @message, + sla_policy: @sla_policy }) end end + +AgentNotifications::ConversationNotificationsMailer.include_mod_with('AgentNotifications::ConversationNotificationsMailer') diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 117950e35..64fc8cebf 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -5,11 +5,13 @@ class ApplicationRecord < ActiveRecord::Base before_validation :validates_column_content_length # the models that exposed in email templates through liquid - DROPPABLES = %w[Account Channel Conversation Inbox User Message].freeze + def droppables + %w[Account Channel Conversation Inbox User Message] + end # ModelDrop class should exist in app/drops def to_drop - return unless DROPPABLES.include?(self.class.name) + return unless droppables.include?(self.class.name) "#{self.class.name}Drop".constantize.new(self) end @@ -47,3 +49,5 @@ class ApplicationRecord < ActiveRecord::Base end end end + +ApplicationRecord.include_mod_with('Enterprise::ApplicationRecord') diff --git a/app/models/notification.rb b/app/models/notification.rb index c71bdbb62..b5834225a 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -118,11 +118,11 @@ class Notification < ApplicationRecord def push_message_body case notification_type - when 'conversation_creation' + when 'conversation_creation', 'sla_missed_first_response' message_body(conversation.messages.first) when 'assigned_conversation_new_message', 'participating_conversation_new_message', 'conversation_mention' message_body(secondary_actor) - when 'conversation_assignment' + when 'conversation_assignment', 'sla_missed_next_response', 'sla_missed_resolution' message_body(conversation.messages.incoming.last) else '' diff --git a/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_first_response.liquid b/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_first_response.liquid new file mode 100644 index 000000000..d7988ad5f --- /dev/null +++ b/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_first_response.liquid @@ -0,0 +1,10 @@ +

Hi {{user.available_name}},

+ +

+ Conversation #{{conversation.display_id}} in {{ inbox.name }} + has missed the SLA for first response under policy {{ sla_policy.name }}. +

+ +

+Please address immediately. +

diff --git a/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_next_response.liquid b/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_next_response.liquid new file mode 100644 index 000000000..d7bf8d445 --- /dev/null +++ b/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_next_response.liquid @@ -0,0 +1,10 @@ +

Hi {{user.available_name}},

+ +

+ Conversation #{{conversation.display_id}} in {{ inbox.name }} + has missed the SLA for next response under policy {{ sla_policy.name }}.. +

+ +

+Please address immediately. +

diff --git a/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_resolution.liquid b/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_resolution.liquid new file mode 100644 index 000000000..efd24913e --- /dev/null +++ b/app/views/mailers/agent_notifications/conversation_notifications_mailer/sla_missed_resolution.liquid @@ -0,0 +1,10 @@ +

Hi {{user.available_name}},

+ +

+ Conversation #{{conversation.display_id}} in {{ inbox.name }} + has missed the SLA for resolution time under policy {{ sla_policy.name }}. +

+ +

+Please address immediately. +

diff --git a/enterprise/app/drops/sla_policy_drop.rb b/enterprise/app/drops/sla_policy_drop.rb new file mode 100644 index 000000000..ea9fbe34d --- /dev/null +++ b/enterprise/app/drops/sla_policy_drop.rb @@ -0,0 +1,9 @@ +class SlaPolicyDrop < BaseDrop + def name + @obj.try(:name) + end + + def description + @obj.try(:description) + end +end diff --git a/enterprise/app/mailers/enterprise/agent_notifications/conversation_notifications_mailer.rb b/enterprise/app/mailers/enterprise/agent_notifications/conversation_notifications_mailer.rb new file mode 100644 index 000000000..df71beb10 --- /dev/null +++ b/enterprise/app/mailers/enterprise/agent_notifications/conversation_notifications_mailer.rb @@ -0,0 +1,32 @@ +module Enterprise::AgentNotifications::ConversationNotificationsMailer + def sla_missed_first_response(conversation, agent, sla_policy) + return unless smtp_config_set_or_development? + + @agent = agent + @conversation = conversation + @sla_policy = sla_policy + subject = "Conversation [ID - #{@conversation.display_id}] missed SLA for first response" + @action_url = app_account_conversation_url(account_id: @conversation.account_id, id: @conversation.display_id) + send_mail_with_liquid(to: @agent.email, subject: subject) and return + end + + def sla_missed_next_response(conversation, agent, sla_policy) + return unless smtp_config_set_or_development? + + @agent = agent + @conversation = conversation + @sla_policy = sla_policy + @action_url = app_account_conversation_url(account_id: @conversation.account_id, id: @conversation.display_id) + send_mail_with_liquid(to: @agent.email, subject: "Conversation [ID - #{@conversation.display_id}] missed SLA for next response") and return + end + + def sla_missed_resolution(conversation, agent, sla_policy) + return unless smtp_config_set_or_development? + + @agent = agent + @conversation = conversation + @sla_policy = sla_policy + @action_url = app_account_conversation_url(account_id: @conversation.account_id, id: @conversation.display_id) + send_mail_with_liquid(to: @agent.email, subject: "Conversation [ID - #{@conversation.display_id}] missed SLA for resolution time") and return + end +end diff --git a/enterprise/app/models/enterprise/application_record.rb b/enterprise/app/models/enterprise/application_record.rb new file mode 100644 index 000000000..a05f60767 --- /dev/null +++ b/enterprise/app/models/enterprise/application_record.rb @@ -0,0 +1,5 @@ +module Enterprise::ApplicationRecord + def droppables + super + %w[SlaPolicy] + end +end diff --git a/spec/enterprise/drops/sla_policy_drop_spec.rb b/spec/enterprise/drops/sla_policy_drop_spec.rb new file mode 100644 index 000000000..c1be13c70 --- /dev/null +++ b/spec/enterprise/drops/sla_policy_drop_spec.rb @@ -0,0 +1,15 @@ +require 'rails_helper' + +describe SlaPolicyDrop do + subject(:sla_policy_drop) { described_class.new(sla_policy) } + + let!(:sla_policy) { create(:sla_policy) } + + it 'returns name' do + expect(sla_policy_drop.name).to eq sla_policy.name + end + + it 'returns description' do + expect(sla_policy_drop.description).to eq sla_policy.description + end +end diff --git a/spec/enterprise/mailers/enterprise/agent_notifications/conversation_notifications_mailer_spec.rb b/spec/enterprise/mailers/enterprise/agent_notifications/conversation_notifications_mailer_spec.rb new file mode 100644 index 000000000..e5e2b14da --- /dev/null +++ b/spec/enterprise/mailers/enterprise/agent_notifications/conversation_notifications_mailer_spec.rb @@ -0,0 +1,54 @@ +require 'rails_helper' + +# rails helper is using infer filetype to detect rspec type +# so we need to include type: :mailer to make this test work in enterprise namespace +RSpec.describe AgentNotifications::ConversationNotificationsMailer, type: :mailer do + let(:class_instance) { described_class.new } + let!(:account) { create(:account) } + let(:agent) { create(:user, email: 'agent1@example.com', account: account) } + let(:conversation) { create(:conversation, assignee: agent, account: account) } + + before do + allow(described_class).to receive(:new).and_return(class_instance) + allow(class_instance).to receive(:smtp_config_set_or_development?).and_return(true) + end + + describe 'sla_missed_first_response' do + let(:sla_policy) { create(:sla_policy, account: account) } + let(:mail) { described_class.with(account: account).sla_missed_first_response(conversation, agent, sla_policy).deliver_now } + + it 'renders the subject' do + expect(mail.subject).to eq("Conversation [ID - #{conversation.display_id}] missed SLA for first response") + end + + it 'renders the receiver email' do + expect(mail.to).to eq([agent.email]) + end + end + + describe 'sla_missed_next_response' do + let(:sla_policy) { create(:sla_policy, account: account) } + let(:mail) { described_class.with(account: account).sla_missed_next_response(conversation, agent, sla_policy).deliver_now } + + it 'renders the subject' do + expect(mail.subject).to eq("Conversation [ID - #{conversation.display_id}] missed SLA for next response") + end + + it 'renders the receiver email' do + expect(mail.to).to eq([agent.email]) + end + end + + describe 'sla_missed_resolution' do + let(:sla_policy) { create(:sla_policy, account: account) } + let(:mail) { described_class.with(account: account).sla_missed_resolution(conversation, agent, sla_policy).deliver_now } + + it 'renders the subject' do + expect(mail.subject).to eq("Conversation [ID - #{conversation.display_id}] missed SLA for resolution time") + end + + it 'renders the receiver email' do + expect(mail.to).to eq([agent.email]) + end + end +end