From a88fef2e1d273086f9cc9d6b6d7c525e4eb5b065 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Wed, 13 Aug 2025 16:39:43 +0530 Subject: [PATCH] fix: incorrect first response time for reopened conversations (#12058) Co-authored-by: Muhsin Keloth --- app/helpers/reporting_event_helper.rb | 23 ++- app/listeners/reporting_event_listener.rb | 39 ++++ spec/helpers/reporting_event_helper_spec.rb | 177 ++++++++++++++++++ .../reporting_event_listener_spec.rb | 173 +++++++++++++++++ 4 files changed, 407 insertions(+), 5 deletions(-) create mode 100644 spec/helpers/reporting_event_helper_spec.rb diff --git a/app/helpers/reporting_event_helper.rb b/app/helpers/reporting_event_helper.rb index e08b5691c..f0a419cc8 100644 --- a/app/helpers/reporting_event_helper.rb +++ b/app/helpers/reporting_event_helper.rb @@ -18,12 +18,25 @@ module ReportingEventHelper 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 + # Try to get either a handoff or reopened event first + # These will always take precedence over any other activity + # Also, any of these events can happen at any time in the course of a conversation lifecycle. + # So we pick the latest event + event = ReportingEvent.where( + conversation_id: conversation.id, + name: %w[conversation_bot_handoff conversation_opened] + ).order(event_end_time: :desc).first - # 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 + return event.event_end_time if event&.event_end_time + + # Fallback to bot resolved event + # Because this will be closest to the most accurate activity instead of conversation.created_at + bot_event = ReportingEvent.where(conversation_id: conversation.id, name: 'conversation_bot_resolved').last + + return bot_event.event_end_time if bot_event&.event_end_time + + # If no events found, return conversation creation time + conversation.created_at end private diff --git a/app/listeners/reporting_event_listener.rb b/app/listeners/reporting_event_listener.rb index 9f22fe8de..9f683a97f 100644 --- a/app/listeners/reporting_event_listener.rb +++ b/app/listeners/reporting_event_listener.rb @@ -90,8 +90,47 @@ class ReportingEventListener < BaseListener reporting_event.save! end + def conversation_opened(event) + conversation = extract_conversation_and_account(event)[0] + + # Find the most recent resolved event for this conversation + last_resolved_event = ReportingEvent.where( + conversation_id: conversation.id, + name: 'conversation_resolved' + ).order(event_end_time: :desc).first + + # For first-time openings, value is 0 + # For reopenings, calculate time since resolution + if last_resolved_event + time_since_resolved = conversation.updated_at.to_i - last_resolved_event.event_end_time.to_i + business_hours_value = business_hours(conversation.inbox, last_resolved_event.event_end_time, conversation.updated_at) + start_time = last_resolved_event.event_end_time + else + time_since_resolved = 0 + business_hours_value = 0 + start_time = conversation.created_at + end + + create_conversation_opened_event(conversation, time_since_resolved, business_hours_value, start_time) + end + private + def create_conversation_opened_event(conversation, time_since_resolved, business_hours_value, start_time) + reporting_event = ReportingEvent.new( + name: 'conversation_opened', + value: time_since_resolved, + value_in_business_hours: business_hours_value, + account_id: conversation.account_id, + inbox_id: conversation.inbox_id, + user_id: conversation.assignee_id, + conversation_id: conversation.id, + event_start_time: start_time, + event_end_time: conversation.updated_at + ) + reporting_event.save! + end + def create_bot_resolved_event(conversation, reporting_event) return unless conversation.inbox.active_bot? # We don't want to create a bot_resolved event if there is user interaction on the conversation diff --git a/spec/helpers/reporting_event_helper_spec.rb b/spec/helpers/reporting_event_helper_spec.rb new file mode 100644 index 000000000..e4a3c0255 --- /dev/null +++ b/spec/helpers/reporting_event_helper_spec.rb @@ -0,0 +1,177 @@ +require 'rails_helper' + +RSpec.describe ReportingEventHelper, type: :helper do + describe '#last_non_human_activity' do + let(:account) { create(:account) } + let(:inbox) { create(:inbox, account: account) } + let(:user) { create(:user, account: account) } + let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user) } + + context 'when conversation has no events' do + it 'returns conversation created_at' do + expect(helper.last_non_human_activity(conversation)).to eq(conversation.created_at) + end + end + + context 'when conversation has bot handoff event' do + let!(:handoff_event) do + create(:reporting_event, + name: 'conversation_bot_handoff', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_end_time: 2.hours.ago) + end + + it 'returns handoff event end time' do + expect(helper.last_non_human_activity(conversation).to_i).to eq(handoff_event.event_end_time.to_i) + end + end + + context 'when conversation has bot resolved event' do + let!(:bot_resolved_event) do + create(:reporting_event, + name: 'conversation_bot_resolved', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_end_time: 3.hours.ago) + end + + it 'returns bot resolved event end time' do + expect(helper.last_non_human_activity(conversation).to_i).to eq(bot_resolved_event.event_end_time.to_i) + end + end + + context 'when conversation is reopened after bot resolution' do + let(:creation_time) { 5.days.ago } + let(:bot_resolution_time) { 5.days.ago + 5.minutes } + let(:reopening_time) { 1.hour.ago } + + let!(:conversation) do + create(:conversation, + account: account, + inbox: inbox, + assignee: user, + created_at: creation_time) + end + + before do + # First opened event + create(:reporting_event, + name: 'conversation_opened', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + value: 0, + event_start_time: creation_time, + event_end_time: creation_time) + + # Bot resolved event + create(:reporting_event, + name: 'conversation_bot_resolved', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_start_time: creation_time, + event_end_time: bot_resolution_time) + + # Resolved event + create(:reporting_event, + name: 'conversation_resolved', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_start_time: creation_time, + event_end_time: bot_resolution_time) + + # Reopened event + create(:reporting_event, + name: 'conversation_opened', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + value: (reopening_time - bot_resolution_time).to_i, + event_start_time: bot_resolution_time, + event_end_time: reopening_time) + end + + it 'returns the reopening event time, not the creation time' do + # This is the key test: last_non_human_activity should return the reopening time + # so that first response time is calculated from when the conversation was reopened, + # not from when it was originally created + expect(helper.last_non_human_activity(conversation).to_i).to eq(reopening_time.to_i) + + # Verify it's not returning the creation time or bot resolution time + expect(helper.last_non_human_activity(conversation).to_i).not_to eq(creation_time.to_i) + expect(helper.last_non_human_activity(conversation).to_i).not_to eq(bot_resolution_time.to_i) + end + end + + context 'when conversation has multiple types of events' do + let(:opened_event_time) { 1.hour.ago } + + before do + create(:reporting_event, + name: 'conversation_bot_resolved', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_end_time: 4.hours.ago) + + create(:reporting_event, + name: 'conversation_bot_handoff', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_end_time: 3.hours.ago) + + create(:reporting_event, + name: 'conversation_opened', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + event_end_time: opened_event_time) + end + + it 'returns the most recent handoff or opened event' do + # opened_event is more recent than handoff_event + expect(helper.last_non_human_activity(conversation).to_i).to eq(opened_event_time.to_i) + end + end + + context 'when conversation has multiple reopenings' do + let(:third_opened_time) { 30.minutes.ago } + + before do + create(:reporting_event, + name: 'conversation_opened', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + value: 0, + event_end_time: 5.days.ago) + + create(:reporting_event, + name: 'conversation_opened', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + value: 3600, + event_end_time: 2.days.ago) + + create(:reporting_event, + name: 'conversation_opened', + conversation_id: conversation.id, + account_id: account.id, + inbox_id: inbox.id, + value: 7200, + event_end_time: third_opened_time) + end + + it 'returns the most recent opened event' do + expect(helper.last_non_human_activity(conversation).to_i).to eq(third_opened_time.to_i) + end + end + end +end diff --git a/spec/listeners/reporting_event_listener_spec.rb b/spec/listeners/reporting_event_listener_spec.rb index 0edf79556..8004349bf 100644 --- a/spec/listeners/reporting_event_listener_spec.rb +++ b/spec/listeners/reporting_event_listener_spec.rb @@ -267,4 +267,177 @@ describe ReportingEventListener do end end end + + describe '#conversation_opened' do + context 'when conversation is opened for the first time' do + let(:new_conversation) { create(:conversation, account: account, inbox: inbox, assignee: user) } + + it 'creates conversation_opened event with value 0' do + expect(account.reporting_events.where(name: 'conversation_opened').count).to be 0 + event = Events::Base.new('conversation.opened', Time.zone.now, conversation: new_conversation) + listener.conversation_opened(event) + expect(account.reporting_events.where(name: 'conversation_opened').count).to be 1 + + opened_event = account.reporting_events.where(name: 'conversation_opened').first + expect(opened_event.value).to eq 0 + expect(opened_event.value_in_business_hours).to eq 0 + expect(opened_event.event_start_time).to be_within(1.second).of(new_conversation.created_at) + expect(opened_event.event_end_time).to be_within(1.second).of(new_conversation.updated_at) + end + end + + context 'when conversation is reopened after being resolved' do + let(:resolved_time) { 2.hours.ago } + let(:reopened_time) { 1.hour.ago } + let(:reopened_conversation) do + create(:conversation, account: account, inbox: inbox, assignee: user, updated_at: reopened_time) + end + + before do + # Create a resolved event first + create(:reporting_event, + name: 'conversation_resolved', + account_id: account.id, + inbox_id: inbox.id, + conversation_id: reopened_conversation.id, + user_id: user.id, + value: 3600, + event_start_time: reopened_conversation.created_at, + event_end_time: resolved_time) + end + + it 'creates conversation_opened event' do + expect(account.reporting_events.where(name: 'conversation_opened').count).to be 0 + event = Events::Base.new('conversation.opened', reopened_time, conversation: reopened_conversation) + listener.conversation_opened(event) + expect(account.reporting_events.where(name: 'conversation_opened').count).to be 1 + end + + it 'calculates correct time since resolution' do + event = Events::Base.new('conversation.opened', reopened_time, conversation: reopened_conversation) + listener.conversation_opened(event) + + reopened_event = account.reporting_events.where(name: 'conversation_opened').first + expect(reopened_event.value).to be_within(1).of(3600) # 1 hour = 3600 seconds + expect(reopened_event.event_start_time).to be_within(1.second).of(resolved_time) + expect(reopened_event.event_end_time).to be_within(1.second).of(reopened_time) + end + + it 'sets correct attributes for conversation_opened event' do + event = Events::Base.new('conversation.opened', reopened_time, conversation: reopened_conversation) + listener.conversation_opened(event) + + reopened_event = account.reporting_events.where(name: 'conversation_opened').first + expect(reopened_event.account_id).to eq(account.id) + expect(reopened_event.inbox_id).to eq(inbox.id) + expect(reopened_event.conversation_id).to eq(reopened_conversation.id) + expect(reopened_event.user_id).to eq(user.id) + end + + context 'when business hours enabled for inbox' do + let(:resolved_time) { Time.zone.parse('March 20, 2022 12:00') } + let(:reopened_time) { Time.zone.parse('March 21, 2022 14:00') } + let!(:business_hours_inbox) { create(:inbox, working_hours_enabled: true, account: account) } + let!(:business_hours_conversation) do + create(:conversation, account: account, inbox: business_hours_inbox, assignee: user, updated_at: reopened_time) + end + + before do + create(:reporting_event, + name: 'conversation_resolved', + account_id: account.id, + inbox_id: business_hours_inbox.id, + conversation_id: business_hours_conversation.id, + user_id: user.id, + value: 3600, + event_start_time: business_hours_conversation.created_at, + event_end_time: resolved_time) + end + + it 'creates conversation_opened event with business hour value' do + event = Events::Base.new('conversation.opened', reopened_time, conversation: business_hours_conversation) + listener.conversation_opened(event) + + reopened_event = account.reporting_events.where(name: 'conversation_opened').first + expect(reopened_event.value_in_business_hours).to be 18_000.0 # 5 business hours (26 hours total - 21 non-business hours) + end + end + end + + context 'when conversation has multiple resolutions' do + let(:first_resolved_time) { 3.hours.ago } + let(:second_resolved_time) { 1.hour.ago } + let(:reopened_time) { 30.minutes.ago } + let(:multiple_resolution_conversation) do + create(:conversation, account: account, inbox: inbox, assignee: user, updated_at: reopened_time) + end + + before do + # Create first resolved event + create(:reporting_event, + name: 'conversation_resolved', + account_id: account.id, + inbox_id: inbox.id, + conversation_id: multiple_resolution_conversation.id, + user_id: user.id, + value: 3600, + event_start_time: multiple_resolution_conversation.created_at, + event_end_time: first_resolved_time) + + # Create second resolved event (more recent) + create(:reporting_event, + name: 'conversation_resolved', + account_id: account.id, + inbox_id: inbox.id, + conversation_id: multiple_resolution_conversation.id, + user_id: user.id, + value: 1800, + event_start_time: first_resolved_time, + event_end_time: second_resolved_time) + end + + it 'uses the most recent resolved event for calculation' do + event = Events::Base.new('conversation.opened', reopened_time, conversation: multiple_resolution_conversation) + listener.conversation_opened(event) + + reopened_event = account.reporting_events.where(name: 'conversation_opened').first + expect(reopened_event.value).to be_within(1).of(1800) # 30 minutes from second resolution + expect(reopened_event.event_start_time).to be_within(1.second).of(second_resolved_time) + end + end + + context 'when agent bot resolves and conversation is reopened' do + # This implicitly tests that the first_response time is correctly calculated + # By checking that a conversation reopened event is created with the correct values + let(:agent_bot) { create(:agent_bot, account: account) } + let(:agent_bot_inbox) { create(:inbox, account: account) } + let(:bot_resolved_time) { 2.hours.ago } + let(:reopened_time) { 1.hour.ago } + let(:bot_conversation) do + create(:conversation, account: account, inbox: agent_bot_inbox, assignee: user, updated_at: reopened_time) + end + + before do + create(:agent_bot_inbox, agent_bot: agent_bot, inbox: agent_bot_inbox) + + create(:reporting_event, + name: 'conversation_resolved', + account_id: account.id, + inbox_id: agent_bot_inbox.id, + conversation_id: bot_conversation.id, + user_id: user.id, + event_end_time: bot_resolved_time) + end + + it 'creates conversation_opened event for agent bot reopening' do + event = Events::Base.new('conversation.opened', reopened_time, conversation: bot_conversation) + listener.conversation_opened(event) + + reopened_event = account.reporting_events.where(name: 'conversation_opened').first + expect(reopened_event.value).to be_within(1).of(3600) # 1 hour since resolution + expect(reopened_event.event_start_time).to be_within(1.second).of(bot_resolved_time) + expect(reopened_event.event_end_time).to be_within(1.second).of(reopened_time) + end + end + end end