From 379e28df1f0bf14244bd4251859dfa546c2caeef Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Wed, 13 May 2026 18:43:23 +0530 Subject: [PATCH] fix: prevent bot metrics double-counting when handoff and resolution coexist [CW-6210] (#14032) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bot metrics dashboard can show `handoff_rate + resolution_rate > 100%`. A single conversation can accumulate both `conversation_bot_handoff` and `conversation_bot_resolved` events, and the rate queries count them independently against a shared denominator. ## How it happens ``` Customer messages bot inbox │ ▼ ┌──────────┐ │ pending │ (bot handling) └────┬─────┘ │ bot can't help ▼ ┌──────────┐ │ open │ (handed off → conversation_bot_handoff event created) └────┬─────┘ │ agent clicks "Resolve" WITHOUT sending a message ▼ ┌──────────┐ │ resolved │ conversation_resolved fires └──────────┘ │ ▼ create_bot_resolved_event guard checks: ✅ inbox.active_bot? ✅ no outgoing messages with sender_type: 'User' ← agent never messaged! │ ▼ conversation_bot_resolved event ALSO created ← BUG │ ▼ Same conversation counted in BOTH rates → sum exceeds 100% ``` ## Why fix at the read path, not the write path An earlier attempt added guards in the listener to make the two events mutually exclusive per conversation — deleting `bot_resolved` when a handoff fires, suppressing resolutions when a handoff exists. This was rejected because conversations can be reopened across multiple cycles (bot resolves on day 1, customer returns on day 5, bot hands off). Deleting the day-1 resolution corrupts historical reports, and the async event dispatcher makes listener-level guards vulnerable to race conditions. ## What this PR does Within a reporting window, if a conversation has both events, **handoff wins** — the conversation is excluded from the resolution count. This is applied via SQL subquery across all three read paths: ``` ┌─────────────────────────┐ │ Reporting Events DB │ │ │ │ conv_bot_handoff: [A,B] │ │ conv_bot_resolved: [A,C]│ └────────┬────────────────┘ │ ┌──────────────┼──────────────┐ ▼ ▼ ▼ BotMetricsBuilder ReportHelper CountReportBuilder (rate cards) (bot_summary) (timeseries charts) │ │ │ ▼ ▼ ▼ resolutions: resolutions: resolutions: [A,C] minus [A,B] same logic same logic = [C] only = [C] only = [C] only Result: Conversation A → handoff only Conversation B → handoff only Conversation C → resolution only ``` For wide date ranges spanning multiple lifecycles, a conversation bot-resolved in one cycle and handed off in a later cycle will only show as a handoff. This is an acceptable tradeoff — the alternative (>100% rates) is clearly worse, and narrow ranges handle this correctly since the events fall into different windows. No reporting events are modified, so historical data stays intact. ## Diagnostic tool `rake bot_metrics:diagnose` — read-only task that prompts for account ID and date range, shows a before/after rate comparison without modifying data. --------- Co-authored-by: aakashb95 Co-authored-by: Aakash Bakhle <48802744+aakashb95@users.noreply.github.com> --- .../v2/reports/bot_metrics_builder.rb | 19 ++++- app/helpers/report_helper.rb | 11 ++- app/services/reports/raw_data_source.rb | 9 ++ .../reports/report_metric_registry.rb | 4 +- spec/builders/v2/report_builder_spec.rb | 2 + .../v2/reports/bot_metrics_builder_spec.rb | 82 +++++++++++++++++-- .../reports/timeseries/report_builder_spec.rb | 39 +++++++++ .../reports/report_metric_registry_spec.rb | 9 ++ 8 files changed, 156 insertions(+), 19 deletions(-) diff --git a/app/builders/v2/reports/bot_metrics_builder.rb b/app/builders/v2/reports/bot_metrics_builder.rb index c46daf978..f151e1a66 100644 --- a/app/builders/v2/reports/bot_metrics_builder.rb +++ b/app/builders/v2/reports/bot_metrics_builder.rb @@ -31,13 +31,24 @@ class V2::Reports::BotMetricsBuilder end def bot_resolutions_count - account.reporting_events.joins(:conversation).select(:conversation_id).where(account_id: account.id, name: :conversation_bot_resolved, - created_at: range).distinct.count + # Exclude conversations that also had a handoff in the same range — handoff wins + account.reporting_events.joins(:conversation).select(:conversation_id) + .where(account_id: account.id, name: :conversation_bot_resolved, created_at: range) + .where.not(conversation_id: bot_handoff_conversation_ids_subquery) + .distinct.count end def bot_handoffs_count - account.reporting_events.joins(:conversation).select(:conversation_id).where(account_id: account.id, name: :conversation_bot_handoff, - created_at: range).distinct.count + account.reporting_events.joins(:conversation).select(:conversation_id) + .where(account_id: account.id, name: :conversation_bot_handoff, created_at: range) + .distinct.count + end + + def bot_handoff_conversation_ids_subquery + account.reporting_events + .where(name: :conversation_bot_handoff, created_at: range) + .where.not(conversation_id: nil) + .select(:conversation_id) end def bot_resolution_rate diff --git a/app/helpers/report_helper.rb b/app/helpers/report_helper.rb index 09a84b110..d5b773c87 100644 --- a/app/helpers/report_helper.rb +++ b/app/helpers/report_helper.rb @@ -53,13 +53,12 @@ module ReportHelper end def resolutions - scope.reporting_events.where(account_id: account.id, name: :conversation_resolved, - created_at: range) + scope.reporting_events.where(account_id: account.id, name: :conversation_resolved, created_at: range) end def bot_resolutions - scope.reporting_events.where(account_id: account.id, name: :conversation_bot_resolved, - created_at: range) + scope.reporting_events.where(account_id: account.id, name: :conversation_bot_resolved, created_at: range) + .where.not(conversation_id: bot_handoff_conversation_ids_subquery) end def bot_handoffs @@ -67,6 +66,10 @@ module ReportHelper created_at: range).distinct end + def bot_handoff_conversation_ids_subquery + bot_handoffs + end + def avg_first_response_time grouped_reporting_events = (get_grouped_values scope.reporting_events.where(name: 'first_response', account_id: account.id)) return grouped_reporting_events.average(:value_in_business_hours) if params[:business_hours] diff --git a/app/services/reports/raw_data_source.rb b/app/services/reports/raw_data_source.rb index f37b3c454..5e75f7f6e 100644 --- a/app/services/reports/raw_data_source.rb +++ b/app/services/reports/raw_data_source.rb @@ -83,11 +83,20 @@ class Reports::RawDataSource < Reports::DataSource created_at: range ) + return events.where.not(conversation_id: bot_handoff_conversation_ids_subquery) if raw_count_strategy == :exclude_bot_handoffs return events unless raw_count_strategy == :distinct_conversation events.joins(:conversation).select(:conversation_id).distinct end + def bot_handoff_conversation_ids_subquery + scope.reporting_events.where( + name: :conversation_bot_handoff, + account_id: account.id, + created_at: range + ).where.not(conversation_id: nil).select(:conversation_id) + end + def summary_scope scope = account.reporting_events.where(created_at: range) return scope.joins(:conversation) if dimension_type == 'team' diff --git a/app/services/reports/report_metric_registry.rb b/app/services/reports/report_metric_registry.rb index 8df2e091d..079dfafc7 100644 --- a/app/services/reports/report_metric_registry.rb +++ b/app/services/reports/report_metric_registry.rb @@ -79,8 +79,8 @@ module Reports::ReportMetricRegistry bot_resolutions_count: Metric.new( name: :bot_resolutions_count, aggregate: :count, - raw_event_name: :conversation_bot_resolved, - rollup_metric: :bot_resolutions_count + raw_event_name: :conversation_bot_resolved, rollup_metric: :bot_resolutions_count, + raw_count_strategy: :exclude_bot_handoffs ), bot_handoffs_count: Metric.new( name: :bot_handoffs_count, diff --git a/spec/builders/v2/report_builder_spec.rb b/spec/builders/v2/report_builder_spec.rb index b6bf83f0a..ece1d2f9a 100644 --- a/spec/builders/v2/report_builder_spec.rb +++ b/spec/builders/v2/report_builder_spec.rb @@ -120,6 +120,8 @@ describe V2::ReportBuilder do # Reopen 1 conversation conversations.first.open! end + create(:reporting_event, account: account, inbox: account.inboxes.first, conversation: nil, conversation_id: nil, + name: 'conversation_bot_handoff', created_at: Time.zone.today) builder = described_class.new(account, params) metrics = builder.timeseries diff --git a/spec/builders/v2/reports/bot_metrics_builder_spec.rb b/spec/builders/v2/reports/bot_metrics_builder_spec.rb index 53e52d595..d183da1b9 100644 --- a/spec/builders/v2/reports/bot_metrics_builder_spec.rb +++ b/spec/builders/v2/reports/bot_metrics_builder_spec.rb @@ -4,35 +4,99 @@ RSpec.describe V2::Reports::BotMetricsBuilder do subject(:bot_metrics_builder) { described_class.new(inbox.account, params) } let(:inbox) { create(:inbox) } - let!(:resolved_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } - let!(:unresolved_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } let(:since) { 1.week.ago.to_i.to_s } let(:until_time) { Time.now.to_i.to_s } let(:params) { { since: since, until: until_time } } before do create(:agent_bot_inbox, inbox: inbox) - create(:message, account: inbox.account, conversation: resolved_conversation, created_at: 2.days.ago, message_type: 'outgoing') - create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_resolved', conversation_id: resolved_conversation.id, - created_at: 2.days.ago) - create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_handoff', - conversation_id: resolved_conversation.id, created_at: 2.days.ago) - create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_handoff', - conversation_id: unresolved_conversation.id, created_at: 2.days.ago) end describe '#metrics' do context 'with valid params' do + let!(:resolved_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } + let!(:handoff_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } + + before do + create(:message, account: inbox.account, conversation: resolved_conversation, created_at: 2.days.ago, message_type: 'outgoing') + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_resolved', + conversation_id: resolved_conversation.id, created_at: 2.days.ago) + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_handoff', + conversation_id: handoff_conversation.id, created_at: 2.days.ago) + end + it 'returns correct metrics' do metrics = bot_metrics_builder.metrics expect(metrics[:conversation_count]).to eq(2) expect(metrics[:message_count]).to eq(1) expect(metrics[:resolution_rate]).to eq(50) + expect(metrics[:handoff_rate]).to eq(50) + end + end + + context 'when a conversation has both bot_resolved and bot_handoff events in the same range' do + let!(:double_counted_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } + let!(:handoff_only_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } + + before do + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_resolved', + conversation_id: double_counted_conversation.id, created_at: 2.days.ago) + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_handoff', + conversation_id: double_counted_conversation.id, created_at: 2.days.ago) + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_handoff', + conversation_id: handoff_only_conversation.id, created_at: 2.days.ago) + end + + it 'excludes the conversation from resolution count — handoff wins' do + metrics = bot_metrics_builder.metrics + + expect(metrics[:conversation_count]).to eq(2) + expect(metrics[:resolution_rate]).to eq(0) expect(metrics[:handoff_rate]).to eq(100) end end + context 'when bot_resolved and bot_handoff are in different date ranges' do + let!(:multi_cycle_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } + + before do + # Bot resolved in current range + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_resolved', + conversation_id: multi_cycle_conversation.id, created_at: 2.days.ago) + # Handoff happened before the range (in a previous cycle) + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_handoff', + conversation_id: multi_cycle_conversation.id, created_at: 2.weeks.ago) + end + + it 'counts the resolution since the handoff is outside the range' do + metrics = bot_metrics_builder.metrics + + expect(metrics[:conversation_count]).to eq(1) + expect(metrics[:resolution_rate]).to eq(100) + expect(metrics[:handoff_rate]).to eq(0) + end + end + + context 'when a bot_handoff event has no conversation' do + let!(:resolved_conversation) { create(:conversation, account: inbox.account, inbox: inbox, created_at: 2.days.ago) } + + before do + create(:reporting_event, account_id: inbox.account.id, name: 'conversation_bot_resolved', + conversation_id: resolved_conversation.id, created_at: 2.days.ago) + create(:reporting_event, account: inbox.account, inbox: inbox, conversation: nil, conversation_id: nil, + name: 'conversation_bot_handoff', created_at: 2.days.ago) + end + + it 'does not exclude all bot resolutions' do + metrics = bot_metrics_builder.metrics + + expect(metrics[:conversation_count]).to eq(1) + expect(metrics[:resolution_rate]).to eq(100) + expect(metrics[:handoff_rate]).to eq(0) + end + end + context 'with missing params' do let(:params) { {} } diff --git a/spec/builders/v2/reports/timeseries/report_builder_spec.rb b/spec/builders/v2/reports/timeseries/report_builder_spec.rb index eec3e99da..0f504b17a 100644 --- a/spec/builders/v2/reports/timeseries/report_builder_spec.rb +++ b/spec/builders/v2/reports/timeseries/report_builder_spec.rb @@ -310,4 +310,43 @@ describe V2::Reports::Timeseries::ReportBuilder do end end end + + describe 'bot resolution counts' do + subject(:builder) { described_class.new(account, params) } + + let(:account) { create(:account) } + let(:inbox) { create(:inbox, account: account) } + let(:current_time) { Time.current } + let(:params) do + { + type: 'account', + metric: 'bot_resolutions_count', + since: (current_time - 1.day).beginning_of_day.to_i.to_s, + until: current_time.end_of_day.to_i.to_s, + timezone_offset: nil, + group_by: 'day' + } + end + + before do + travel_to current_time + + resolved_conversation = create(:conversation, account: account, inbox: inbox) + double_counted_conversation = create(:conversation, account: account, inbox: inbox) + + create(:reporting_event, name: 'conversation_bot_resolved', account: account, conversation: resolved_conversation, + created_at: current_time) + create(:reporting_event, name: 'conversation_bot_resolved', account: account, conversation: double_counted_conversation, + created_at: current_time) + create(:reporting_event, name: 'conversation_bot_handoff', account: account, conversation: double_counted_conversation, + created_at: current_time) + create(:reporting_event, name: 'conversation_bot_handoff', account: account, inbox: inbox, conversation: nil, conversation_id: nil, + created_at: current_time) + end + + it 'excludes conversations that also had a bot handoff in the range' do + expect(builder.aggregate_value).to eq(1) + expect(builder.timeseries.sum { |row| row[:value] }).to eq(1) + end + end end diff --git a/spec/services/reports/report_metric_registry_spec.rb b/spec/services/reports/report_metric_registry_spec.rb index 33fcb41e5..1f961a7a1 100644 --- a/spec/services/reports/report_metric_registry_spec.rb +++ b/spec/services/reports/report_metric_registry_spec.rb @@ -30,6 +30,15 @@ RSpec.describe Reports::ReportMetricRegistry do expect(metric.raw_count_strategy).to eq(:distinct_conversation) end + it 'locks the handoff exclusion strategy for bot_resolutions_count' do + metric = described_class.fetch(:bot_resolutions_count) + + expect(metric.count?).to be(true) + expect(metric.raw_event_name).to eq(:conversation_bot_resolved) + expect(metric.rollup_metric).to eq(:bot_resolutions_count) + expect(metric.raw_count_strategy).to eq(:exclude_bot_handoffs) + end + it 'returns nil for unsupported metrics' do expect(described_class.fetch(:unknown_metric)).to be_nil end