From 06ffaa90fc1b18b0943a499ae23417ab8660d963 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Sat, 25 Feb 2023 09:48:48 +0530 Subject: [PATCH] fix: bots included in time to response metrics (#6409) * feat: ignore bots in avg_first_response_time * feat: ignore bots in avg_first_response count * feat: add bot handoff event * feat: add handoff event listener and reporting event * fix: ignore agent bot in first response * refactor: calculate first_response with last handoff * refactor: method defn order * test: new reporting events * feat: Revert "feat: ignore bots in avg_first_response count" This reverts commit de1977c219a2e7a9180dd02272244fe3b3f7ce89. * feat: Revert "feat: ignore bots in avg_first_response_time" This reverts commit bb9171945d5e3b2f6015f4f96dd1b76b3efb6987. * fix: business hour calculation for first_reply * fix: event_start_time for first_response * feat: add migration to recompute first_responses * refactor: separate mute helpers for conversation * refactor: rename migration * refactor: migration script * fix: migration typo * fix: typo in query * feat: update schema.rb * Revert "feat: update schema.rb" This reverts commit 353ef355f2d956dd219907bb66982dc90ca5d896. * feat: update schema * refactor: update events as a batch job * fix: ignore the event if value is negative * feat: don't create a new hand-off if it's already present * refactor: break the action into smaller chunks * refactor: update reporting listener spec Handle the case to ensure extra bot handoffs are not created for a give conversation * fix: import error --------- Co-authored-by: Vishnu Narayanan --- app/helpers/reporting_event_helper.rb | 9 +++ ...t_response_time_in_reporting_events_job.rb | 61 +++++++++++++++++++ app/listeners/reporting_event_listener.rb | 30 ++++++++- .../concerns/conversation_mute_helpers.rb | 28 +++++++++ app/models/conversation.rb | 25 ++------ app/models/message.rb | 9 ++- ...g_events_with_incorrect_first_responses.rb | 10 +++ db/schema.rb | 2 +- lib/events/types.rb | 1 + lib/integrations/bot_processor_service.rb | 2 +- .../reporting_event_listener_spec.rb | 51 ++++++++++++++++ 11 files changed, 200 insertions(+), 28 deletions(-) create mode 100644 app/jobs/migration/update_first_response_time_in_reporting_events_job.rb create mode 100644 app/models/concerns/conversation_mute_helpers.rb create mode 100644 db/migrate/20230214025901_update_reporting_events_with_incorrect_first_responses.rb diff --git a/app/helpers/reporting_event_helper.rb b/app/helpers/reporting_event_helper.rb index eee1af283..e08b5691c 100644 --- a/app/helpers/reporting_event_helper.rb +++ b/app/helpers/reporting_event_helper.rb @@ -17,6 +17,15 @@ module ReportingEventHelper from_in_inbox_timezone.working_time_until(to_in_inbox_timezone) end + def last_non_human_activity(conversation) + # check if a handoff event already exists + handoff_event = ReportingEvent.where(conversation_id: conversation.id, name: 'conversation_bot_handoff').last + + # if a handoff exists, last non human activity is when the handoff ended, + # otherwise it's when the conversation was created + handoff_event&.event_end_time || conversation.created_at + end + private def configure_working_hours(working_hours) diff --git a/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb b/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb new file mode 100644 index 000000000..2e658708d --- /dev/null +++ b/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb @@ -0,0 +1,61 @@ +# Delete migration and spec after 2 consecutive releases. +class Migration::UpdateFirstResponseTimeInReportingEventsJob < ApplicationJob + include ReportingEventHelper + + queue_as :scheduled_jobs + + def perform(account) + account.reporting_events.where(name: 'first_response', user_id: nil).each do |event| + conversation = event.conversation + next if conversation.nil? + + update_event_data(event, conversation) + end + end + + def update_event_data(event, conversation) + last_bot_reply = conversation.messages.where(sender_type: 'AgentBot').order(created_at: :asc).last + first_human_reply = conversation.messages.where(sender_type: 'User').order(created_at: :asc).first + + # accomodate for campaign if required + # new_value = difference between the first_human_reply and the first_bot_reply if it exists or first_human_reply and created at + # + # conversation bot conversation + # start handoff resolved + # | | | + # |____|___|_________|____|_______|_____|________| + # bot reply ^ ^ human reply + # | | + # | | + # last_bot_reply first_human_reply + # + # + # bot handoff happens at the last_bot_reply created time + # the response time is the time between last bot reply created and the first human reply created + + return if last_bot_reply.blank? || first_human_reply.blank? + return if last_bot_reply.created_at.to_i >= first_human_reply.created_at.to_i + + # this means a bot replied existed, so we need to update the event_start_time + update_event_details(event, last_bot_reply, first_human_reply, conversation.inbox) + end + + def update_event_details(event, last_bot_reply, first_human_reply, inbox) + # rubocop:disable Rails/SkipsModelValidations + event.update_columns(event_start_time: last_bot_reply.created_at, + event_end_time: first_human_reply.created_at, + value: calculate_event_value(last_bot_reply, first_human_reply), + value_in_business_hours: calculate_event_value_in_business_hours(inbox, last_bot_reply, + first_human_reply), + user_id: first_human_reply.sender_id) + # rubocop:enable Rails/SkipsModelValidations + end + + def calculate_event_value(last_bot_reply, first_human_reply) + first_human_reply.created_at.to_i - last_bot_reply.created_at.to_i + end + + def calculate_event_value_in_business_hours(inbox, last_bot_reply, first_human_reply) + business_hours(inbox, last_bot_reply.created_at, first_human_reply.created_at) + end +end diff --git a/app/listeners/reporting_event_listener.rb b/app/listeners/reporting_event_listener.rb index e84d778bd..c0c54cc26 100644 --- a/app/listeners/reporting_event_listener.rb +++ b/app/listeners/reporting_event_listener.rb @@ -1,5 +1,6 @@ class ReportingEventListener < BaseListener include ReportingEventHelper + def conversation_resolved(event) conversation = extract_conversation_and_account(event)[0] time_to_resolve = conversation.updated_at.to_i - conversation.created_at.to_i @@ -22,18 +23,18 @@ class ReportingEventListener < BaseListener def first_reply_created(event) message = extract_message_and_account(event)[0] conversation = message.conversation - first_response_time = message.created_at.to_i - conversation.created_at.to_i + first_response_time = message.created_at.to_i - last_non_human_activity(conversation).to_i reporting_event = ReportingEvent.new( name: 'first_response', value: first_response_time, - value_in_business_hours: business_hours(conversation.inbox, conversation.created_at, + value_in_business_hours: business_hours(conversation.inbox, last_non_human_activity(conversation), message.created_at), account_id: conversation.account_id, inbox_id: conversation.inbox_id, user_id: conversation.assignee_id, conversation_id: conversation.id, - event_start_time: conversation.created_at, + event_start_time: last_non_human_activity(conversation), event_end_time: message.created_at ) @@ -41,4 +42,27 @@ class ReportingEventListener < BaseListener reporting_event.save! end + + def conversation_bot_handoff(event) + conversation = extract_conversation_and_account(event)[0] + + # check if a conversation_bot_handoff event exists for this conversation + bot_handoff_event = ReportingEvent.find_by(conversation_id: conversation.id, name: 'conversation_bot_handoff') + return if bot_handoff_event.present? + + time_to_handoff = conversation.updated_at.to_i - conversation.created_at.to_i + + reporting_event = ReportingEvent.new( + name: 'conversation_bot_handoff', + value: time_to_handoff, + value_in_business_hours: business_hours(conversation.inbox, conversation.created_at, conversation.updated_at), + account_id: conversation.account_id, + inbox_id: conversation.inbox_id, + user_id: conversation.assignee_id, + conversation_id: conversation.id, + event_start_time: conversation.created_at, + event_end_time: conversation.updated_at + ) + reporting_event.save! + end end diff --git a/app/models/concerns/conversation_mute_helpers.rb b/app/models/concerns/conversation_mute_helpers.rb new file mode 100644 index 000000000..525346869 --- /dev/null +++ b/app/models/concerns/conversation_mute_helpers.rb @@ -0,0 +1,28 @@ +module ConversationMuteHelpers + extend ActiveSupport::Concern + + def mute! + resolved! + Redis::Alfred.setex(mute_key, 1, mute_period) + create_muted_message + end + + def unmute! + Redis::Alfred.delete(mute_key) + create_unmuted_message + end + + def muted? + Redis::Alfred.get(mute_key).present? + end + + private + + def mute_key + format(Redis::RedisKeys::CONVERSATION_MUTE_KEY, id: id) + end + + def mute_period + 6.hours + end +end diff --git a/app/models/conversation.rb b/app/models/conversation.rb index 5d40ab513..119df82e9 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -48,6 +48,7 @@ class Conversation < ApplicationRecord include ActivityMessageHandler include UrlHelper include SortHandler + include ConversationMuteHelpers validates :account_id, presence: true validates :inbox_id, presence: true @@ -142,19 +143,9 @@ class Conversation < ApplicationRecord save end - def mute! - resolved! - Redis::Alfred.setex(mute_key, 1, mute_period) - create_muted_message - end - - def unmute! - Redis::Alfred.delete(mute_key) - create_unmuted_message - end - - def muted? - Redis::Alfred.get(mute_key).present? + def bot_handoff! + open! + dispatcher_dispatch(CONVERSATION_BOT_HANDOFF) end def unread_messages @@ -269,14 +260,6 @@ class Conversation < ApplicationRecord create_label_removed(user_name, previous_labels - current_labels) end - def mute_key - format(Redis::RedisKeys::CONVERSATION_MUTE_KEY, id: id) - end - - def mute_period - 6.hours - end - def validate_referer_url return unless additional_attributes['referer'] diff --git a/app/models/message.rb b/app/models/message.rb index 6f11d2690..40f4af757 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -188,10 +188,15 @@ class Message < ApplicationRecord sender.update(last_activity_at: DateTime.now) if sender.is_a?(Contact) end + def first_human_response? + conversation.messages.outgoing + .where.not(sender_type: 'AgentBot') + .where("(additional_attributes->'campaign_id') is null").count == 1 + end + def dispatch_create_events Rails.configuration.dispatcher.dispatch(MESSAGE_CREATED, Time.zone.now, message: self, performed_by: Current.executed_by) - - if outgoing? && conversation.messages.outgoing.where("(additional_attributes->'campaign_id') is null").count == 1 + if outgoing? && first_human_response? Rails.configuration.dispatcher.dispatch(FIRST_REPLY_CREATED, Time.zone.now, message: self, performed_by: Current.executed_by) end end diff --git a/db/migrate/20230214025901_update_reporting_events_with_incorrect_first_responses.rb b/db/migrate/20230214025901_update_reporting_events_with_incorrect_first_responses.rb new file mode 100644 index 000000000..437d1f5ba --- /dev/null +++ b/db/migrate/20230214025901_update_reporting_events_with_incorrect_first_responses.rb @@ -0,0 +1,10 @@ +class UpdateReportingEventsWithIncorrectFirstResponses < ActiveRecord::Migration[6.1] + def change + ::Account.find_in_batches do |account_batch| + Rails.logger.info "Updated reporting events till #{account_batch.first.id}\n" + account_batch.each do |account| + Migration::UpdateFirstResponseTimeInReportingEventsJob.perform_later(account) + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 68284421c..bd4e95b2a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2023_02_09_033203) do +ActiveRecord::Schema.define(version: 2023_02_14_025901) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" diff --git a/lib/events/types.rb b/lib/events/types.rb index b015cd3c5..85a77a6d3 100644 --- a/lib/events/types.rb +++ b/lib/events/types.rb @@ -16,6 +16,7 @@ module Events::Types CONVERSATION_CREATED = 'conversation.created' CONVERSATION_UPDATED = 'conversation.updated' CONVERSATION_READ = 'conversation.read' + CONVERSATION_BOT_HANDOFF = 'conversation.bot_handoff' # FIXME: deprecate the opened and resolved events in future in favor of status changed event. CONVERSATION_OPENED = 'conversation.opened' CONVERSATION_RESOLVED = 'conversation.resolved' diff --git a/lib/integrations/bot_processor_service.rb b/lib/integrations/bot_processor_service.rb index 42a416fdf..a76c3706d 100644 --- a/lib/integrations/bot_processor_service.rb +++ b/lib/integrations/bot_processor_service.rb @@ -55,7 +55,7 @@ class Integrations::BotProcessorService def process_action(message, action) case action when 'handoff' - message.conversation.open! + message.conversation.bot_handoff! when 'resolve' message.conversation.resolved! end diff --git a/spec/listeners/reporting_event_listener_spec.rb b/spec/listeners/reporting_event_listener_spec.rb index d3b6bd35d..2f0fc24bd 100644 --- a/spec/listeners/reporting_event_listener_spec.rb +++ b/spec/listeners/reporting_event_listener_spec.rb @@ -60,5 +60,56 @@ describe ReportingEventListener do expect(account.reporting_events.where(name: 'first_response')[0]['value_in_business_hours']).to be 144_000.0 end end + + # this ensures last_non_human_activity method accurately accounts for handoff events + context 'when last handoff event exists' do + let(:conversation_updated_at) { 20.seconds.from_now } + let(:human_message_created_at) { 62.seconds.from_now } + let(:new_conversation) { create(:conversation, account: account, inbox: inbox, assignee: user, updated_at: conversation_updated_at) } + let(:new_message) do + create(:message, message_type: 'outgoing', created_at: human_message_created_at, account: account, inbox: inbox, + conversation: new_conversation) + end + + it 'creates first_response event with handoff value' do + # this will create a handoff event + event = Events::Base.new('conversation.bot_handoff', conversation_updated_at, conversation: new_conversation) + listener.conversation_bot_handoff(event) + + # create the first reply event + event = Events::Base.new('first.reply.created', human_message_created_at, message: new_message) + listener.first_reply_created(event) + expect(account.reporting_events.where(name: 'first_response')[0]['value']).to be 42.0 + end + end + end + + describe '#conversation_bot_handoff' do + it 'creates conversation_bot_handoff event only once' do + expect(account.reporting_events.where(name: 'conversation_bot_handoff').count).to be 0 + event = Events::Base.new('conversation.bot_handoff', Time.zone.now, conversation: conversation) + listener.conversation_bot_handoff(event) + expect(account.reporting_events.where(name: 'conversation_bot_handoff').count).to be 1 + + # add extra handoff event for the same and ensure it's not created + event = Events::Base.new('conversation.bot_handoff', Time.zone.now, conversation: conversation) + listener.conversation_bot_handoff(event) + expect(account.reporting_events.where(name: 'conversation_bot_handoff').count).to be 1 + end + + context 'when business hours enabled for inbox' do + let(:created_at) { Time.zone.parse('March 20, 2022 00:00') } + let(:updated_at) { Time.zone.parse('March 26, 2022 23:59') } + let!(:new_inbox) { create(:inbox, working_hours_enabled: true, account: account) } + let!(:new_conversation) do + create(:conversation, created_at: created_at, updated_at: updated_at, account: account, inbox: new_inbox, assignee: user) + end + + it 'creates conversation_bot_handoff event with business hour value' do + event = Events::Base.new('conversation.bot_handoff', Time.zone.now, conversation: new_conversation) + listener.conversation_bot_handoff(event) + expect(account.reporting_events.where(name: 'conversation_bot_handoff')[0]['value_in_business_hours']).to be 144_000.0 + end + end end end