379e28df1f
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 <aakashbakhle@gmail.com>
Co-authored-by: Aakash Bakhle <48802744+aakashb95@users.noreply.github.com>
485 lines
17 KiB
Ruby
485 lines
17 KiB
Ruby
require 'rails_helper'
|
|
|
|
describe V2::ReportBuilder do
|
|
include ActiveJob::TestHelper
|
|
let_it_be(:account) { create(:account) }
|
|
let_it_be(:label_1) { create(:label, title: 'Label_1', account: account) }
|
|
let_it_be(:label_2) { create(:label, title: 'Label_2', account: account) }
|
|
|
|
describe '#timeseries' do
|
|
# Use before_all to share expensive setup across all tests in this describe block
|
|
# This runs once instead of 21 times, dramatically speeding up the suite
|
|
before_all do
|
|
travel_to(Time.zone.today) do
|
|
user = create(:user, account: account)
|
|
inbox = create(:inbox, account: account)
|
|
create(:inbox_member, user: user, inbox: inbox)
|
|
|
|
gravatar_url = 'https://www.gravatar.com'
|
|
favicon_url = 'https://www.google.com/s2/favicons'
|
|
stub_request(:get, /#{gravatar_url}.*/).to_return(status: 404)
|
|
stub_request(:get, /#{Regexp.escape(favicon_url)}.*/).to_return(status: 404)
|
|
|
|
perform_enqueued_jobs do
|
|
10.times do
|
|
conversation = create(:conversation, account: account,
|
|
inbox: inbox, assignee: user,
|
|
created_at: Time.zone.today)
|
|
create_list(:message, 5, message_type: 'outgoing',
|
|
account: account, inbox: inbox,
|
|
conversation: conversation, created_at: Time.zone.today + 2.hours)
|
|
create_list(:message, 2, message_type: 'incoming',
|
|
account: account, inbox: inbox,
|
|
conversation: conversation,
|
|
created_at: Time.zone.today + 3.hours)
|
|
conversation.update_labels('label_1')
|
|
conversation.label_list
|
|
conversation.save!
|
|
end
|
|
|
|
5.times do
|
|
conversation = create(:conversation, account: account,
|
|
inbox: inbox, assignee: user,
|
|
created_at: (Time.zone.today - 2.days))
|
|
create_list(:message, 3, message_type: 'outgoing',
|
|
account: account, inbox: inbox,
|
|
conversation: conversation,
|
|
created_at: (Time.zone.today - 2.days))
|
|
create_list(:message, 1, message_type: 'incoming',
|
|
account: account, inbox: inbox,
|
|
conversation: conversation,
|
|
created_at: (Time.zone.today - 2.days))
|
|
conversation.update_labels('label_2')
|
|
conversation.label_list
|
|
conversation.save!
|
|
end
|
|
end
|
|
end
|
|
end
|
|
|
|
context 'when report type is account' do
|
|
it 'return conversations count' do
|
|
params = {
|
|
metric: 'conversations_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today]).to be 10
|
|
expect(metrics[Time.zone.today - 2.days]).to be 5
|
|
end
|
|
|
|
it 'return incoming messages count' do
|
|
params = {
|
|
metric: 'incoming_messages_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today]).to be 20
|
|
expect(metrics[Time.zone.today - 2.days]).to be 5
|
|
end
|
|
|
|
it 'return outgoing messages count' do
|
|
params = {
|
|
metric: 'outgoing_messages_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today]).to be 50
|
|
expect(metrics[Time.zone.today - 2.days]).to be 15
|
|
end
|
|
|
|
it 'return resolutions count' do
|
|
travel_to(Time.zone.today) do
|
|
params = {
|
|
metric: 'resolutions_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
conversations = account.conversations.where('created_at < ?', 1.day.ago)
|
|
perform_enqueued_jobs do
|
|
# Resolve all 5 conversations
|
|
conversations.each(&:resolved!)
|
|
|
|
# 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
|
|
|
|
# 5 resolution events occurred (even though 1 was later reopened)
|
|
expect(metrics[Time.zone.today]).to be 5
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
end
|
|
end
|
|
|
|
it 'return resolutions count with multiple resolutions of same conversation' do
|
|
travel_to(Time.zone.today) do
|
|
params = {
|
|
metric: 'resolutions_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
conversations = account.conversations.where('created_at < ?', 1.day.ago)
|
|
perform_enqueued_jobs do
|
|
# Resolve all 5 conversations (first round)
|
|
conversations.each(&:resolved!)
|
|
|
|
# Reopen 2 conversations and resolve them again
|
|
conversations.first(2).each do |conversation|
|
|
conversation.open!
|
|
conversation.resolved!
|
|
end
|
|
end
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
# 7 total resolution events: 5 initial + 2 re-resolutions
|
|
expect(metrics[Time.zone.today]).to be 7
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
end
|
|
end
|
|
|
|
it 'returns bot_resolutions count' do
|
|
travel_to(Time.zone.today) do
|
|
params = {
|
|
metric: 'bot_resolutions_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
create(:agent_bot_inbox, inbox: account.inboxes.first)
|
|
conversations = account.conversations.where('created_at < ?', 1.day.ago)
|
|
conversations.each do |conversation|
|
|
conversation.messages.outgoing.all.update(sender: nil)
|
|
end
|
|
|
|
perform_enqueued_jobs do
|
|
# Resolve all 5 conversations
|
|
conversations.each(&:resolved!)
|
|
|
|
# Reopen 1 conversation
|
|
conversations.first.open!
|
|
end
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
summary = builder.bot_summary
|
|
|
|
# 5 bot resolution events occurred (even though 1 was later reopened)
|
|
expect(metrics[Time.zone.today]).to be 5
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
expect(summary[:bot_resolutions_count]).to be 5
|
|
end
|
|
end
|
|
|
|
it 'return bot_handoff count' do
|
|
travel_to(Time.zone.today) do
|
|
params = {
|
|
metric: 'bot_handoffs_count',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
create(:agent_bot_inbox, inbox: account.inboxes.first)
|
|
conversations = account.conversations.where('created_at < ?', 1.day.ago)
|
|
conversations.each do |conversation|
|
|
conversation.pending!
|
|
conversation.messages.outgoing.all.update(sender: nil)
|
|
end
|
|
|
|
perform_enqueued_jobs do
|
|
# Resolve all 5 conversations
|
|
conversations.each(&:bot_handoff!)
|
|
|
|
# Reopen 1 conversation
|
|
conversations.first.open!
|
|
end
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
summary = builder.bot_summary
|
|
|
|
# 4 conversations are resolved
|
|
expect(metrics[Time.zone.today]).to be 5
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
expect(summary[:bot_handoffs_count]).to be 5
|
|
end
|
|
end
|
|
|
|
it 'returns average first response time' do
|
|
params = {
|
|
metric: 'avg_first_response_time',
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today].to_f).to be 0.48e4
|
|
end
|
|
|
|
it 'returns summary' do
|
|
params = {
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.summary
|
|
|
|
expect(metrics[:conversations_count]).to be 15
|
|
expect(metrics[:incoming_messages_count]).to be 25
|
|
expect(metrics[:outgoing_messages_count]).to be 65
|
|
expect(metrics[:avg_resolution_time]).to be 0
|
|
expect(metrics[:resolutions_count]).to be 0
|
|
end
|
|
|
|
it 'returns argument error for incorrect group by' do
|
|
params = {
|
|
type: :account,
|
|
metric: 'avg_first_response_time',
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s,
|
|
group_by: 'test'.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
expect { builder.timeseries }.to raise_error(ArgumentError)
|
|
end
|
|
|
|
it 'logs error when metric is nil' do
|
|
params = {
|
|
metric: nil, # Set metric to nil to test this case
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
|
|
expect(Rails.logger).to receive(:error).with('ReportBuilder: Invalid metric - ')
|
|
builder.timeseries
|
|
end
|
|
|
|
it 'calls the appropriate metric method for a valid metric' do
|
|
params = {
|
|
metric: 'not_conversation_count', # Provide a invalid metric
|
|
type: :account,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
expect(Rails.logger).to receive(:error).with('ReportBuilder: Invalid metric - not_conversation_count')
|
|
|
|
builder.timeseries
|
|
end
|
|
end
|
|
|
|
context 'when report type is label' do
|
|
it 'return conversations count' do
|
|
params = {
|
|
metric: 'conversations_count',
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today - 2.days]).to be 5
|
|
end
|
|
|
|
it 'return incoming messages count' do
|
|
params = {
|
|
metric: 'incoming_messages_count',
|
|
type: :label,
|
|
id: label_1.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: (Time.zone.today + 1.day).to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today]).to be 20
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
end
|
|
|
|
it 'return outgoing messages count' do
|
|
params = {
|
|
metric: 'outgoing_messages_count',
|
|
type: :label,
|
|
id: label_1.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: (Time.zone.today + 1.day).to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
expect(metrics[Time.zone.today]).to be 50
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
end
|
|
|
|
it 'return resolutions count' do
|
|
travel_to(Time.zone.today) do
|
|
params = {
|
|
metric: 'resolutions_count',
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: (Time.zone.today + 1.day).to_time.to_i.to_s
|
|
}
|
|
|
|
conversations = account.conversations.where('created_at < ?', 1.day.ago)
|
|
|
|
perform_enqueued_jobs do
|
|
# ensure 5 reporting events are created
|
|
conversations.each(&:resolved!)
|
|
|
|
# open one of the conversations to check if it is not counted
|
|
conversations.last.open!
|
|
end
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
# this should count all 5 resolution events (even though 1 was later reopened)
|
|
expect(metrics[Time.zone.today]).to be 5
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
end
|
|
end
|
|
|
|
it 'return resolutions count with multiple resolutions of same conversation' do
|
|
travel_to(Time.zone.today) do
|
|
params = {
|
|
metric: 'resolutions_count',
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: (Time.zone.today + 1.day).to_time.to_i.to_s
|
|
}
|
|
|
|
conversations = account.conversations.where('created_at < ?', 1.day.ago)
|
|
|
|
perform_enqueued_jobs do
|
|
# Resolve all 5 conversations (first round)
|
|
conversations.each(&:resolved!)
|
|
|
|
# Reopen 3 conversations and resolve them again
|
|
conversations.first(3).each do |conversation|
|
|
conversation.open!
|
|
conversation.resolved!
|
|
end
|
|
end
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
|
|
# 8 total resolution events: 5 initial + 3 re-resolutions
|
|
expect(metrics[Time.zone.today]).to be 8
|
|
expect(metrics[Time.zone.today - 2.days]).to be 0
|
|
end
|
|
end
|
|
|
|
it 'returns average first response time' do
|
|
label_2.reporting_events.update(value: 1.5)
|
|
|
|
params = {
|
|
metric: 'avg_first_response_time',
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.timeseries
|
|
expect(metrics[Time.zone.today].to_f).to be 0.15e1
|
|
end
|
|
|
|
it 'returns summary' do
|
|
params = {
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.summary
|
|
|
|
expect(metrics[:conversations_count]).to be 5
|
|
expect(metrics[:incoming_messages_count]).to be 5
|
|
expect(metrics[:outgoing_messages_count]).to be 15
|
|
expect(metrics[:avg_resolution_time]).to be 0
|
|
expect(metrics[:resolutions_count]).to be 0
|
|
end
|
|
|
|
it 'returns summary for correct group by' do
|
|
params = {
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s,
|
|
group_by: 'week'.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
metrics = builder.summary
|
|
|
|
expect(metrics[:conversations_count]).to be 5
|
|
expect(metrics[:incoming_messages_count]).to be 5
|
|
expect(metrics[:outgoing_messages_count]).to be 15
|
|
expect(metrics[:avg_resolution_time]).to be 0
|
|
expect(metrics[:resolutions_count]).to be 0
|
|
end
|
|
|
|
it 'returns argument error for incorrect group by' do
|
|
params = {
|
|
metric: 'avg_first_response_time',
|
|
type: :label,
|
|
id: label_2.id,
|
|
since: (Time.zone.today - 3.days).to_time.to_i.to_s,
|
|
until: Time.zone.today.end_of_day.to_time.to_i.to_s,
|
|
group_by: 'test'.to_s
|
|
}
|
|
|
|
builder = described_class.new(account, params)
|
|
expect { builder.timeseries }.to raise_error(ArgumentError)
|
|
end
|
|
end
|
|
end
|
|
end
|