feat(rollup): report builder abstraction [2/3] (#13798)

## PR2: Report builder refactor — DataSource abstraction

The existing report builders (timeseries + summary) had their SQL
queries inlined — each builder constructed its own scopes, groupings,
and aggregations directly. This made it hard to swap the underlying data
source without duplicating builder logic.

This PR extracts all raw-event querying into a `Reports::RawDataSource`
behind a `Reports::DataSource` factory. Builders now call
`data_source.timeseries`, `.aggregate`, or `.summary` instead of
constructing queries themselves. Behavior is identical —
`DataSource.for(...)` returns `RawDataSource` in all cases today.

The timeseries path had two separate builders (`CountReportBuilder`,
`AverageReportBuilder`) that were selected via a metric-name case
statement in `Conversations::BaseReportBuilder`. These are replaced by a
single `ReportBuilder` that delegates to the data source. The metric
type (count vs average) is now decided inside the data source, not the
builder.

Summary builders similarly moved their inline SQL into
`RawDataSource#summary`, which returns a unified hash keyed by dimension
ID.
 the rollup read path.

## Flow

### Before

```
ReportsController ──▶ case metric ──▶ AverageReportBuilder ──▶ inline SQL ──▶ DB
                                  └──▶ CountReportBuilder   ──▶ inline SQL ──▶ DB

SummaryController ──▶ AgentSummaryBuilder ──▶ inline SQL ──▶ DB
                  └──▶ InboxSummaryBuilder ──▶ inline SQL ──▶ DB
                  └──▶ TeamSummaryBuilder  ──▶ inline SQL ──▶ DB
```

### After

```
ReportsController ──▶ ReportBuilder  ──┐
                                       ├──▶ DataSource.for ──▶ RawDataSource ──▶ DB
SummaryController ──▶ SummaryBuilder ──┘
```


### Expected (after rollup read path)

```
ReportsController ──▶ ReportBuilder  ──┐
                                       ├──▶ DataSource.for ──▶ RawDataSource    ──▶ reporting_events
SummaryController ──▶ SummaryBuilder ──┘                   └──▶ RollupDataSource ──▶ reporting_events_rollups
```

### What changed

- `Reports::DataSource` factory + `Reports::RawDataSource`
- `TimezoneHelper#timezone_name_from_params` — prefers IANA name, falls
back to offset
- Unified `Timeseries::ReportBuilder` replaces `CountReportBuilder` +
`AverageReportBuilder`
- Summary builders delegate to `DataSource` instead of querying directly

### How to test

This is a pure refactor — all existing report pages (Overview, Agent,
Inbox, Label, Team) should produce identical numbers. No feature flag or
new config needed.

---------

Co-authored-by: Muhsin Keloth <muhsinkeramam@gmail.com>
Co-authored-by: Tanmay Deep Sharma <tanmaydeepsharma21@gmail.com>
Co-authored-by: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com>
This commit is contained in:
Shivam Mishra
2026-04-20 11:15:48 +05:30
committed by GitHub
co-authored by Muhsin Keloth Tanmay Deep Sharma Tanmay Deep Sharma
parent 5e82f24be5
commit 6cbddbdb67
22 changed files with 822 additions and 512 deletions
+1
View File
@@ -1,6 +1,7 @@
class V2::ReportBuilder
include DateRangeHelper
include ReportHelper
attr_reader :account, :params
DEFAULT_GROUP_BY = 'day'.freeze
@@ -11,10 +11,6 @@ class V2::Reports::AgentSummaryBuilder < V2::Reports::BaseSummaryBuilder
attr_reader :conversations_count, :resolved_count,
:avg_resolution_time, :avg_first_response_time, :avg_reply_time
def fetch_conversations_count
account.conversations.where(created_at: range).group('assignee_id').count
end
def prepare_report
account.account_users.map do |account_user|
build_agent_stats(account_user)
+27 -32
View File
@@ -9,37 +9,13 @@ class V2::Reports::BaseSummaryBuilder
private
def load_data
@conversations_count = fetch_conversations_count
load_reporting_events_data
end
results = data_source.summary
def load_reporting_events_data
# Extract the column name for indexing (e.g., 'conversations.team_id' -> 'team_id')
index_key = group_by_key.to_s.split('.').last
results = reporting_events
.select(
"#{group_by_key} as #{index_key}",
"COUNT(CASE WHEN name = 'conversation_resolved' THEN 1 END) as resolved_count",
"AVG(CASE WHEN name = 'conversation_resolved' THEN #{average_value_key} END) as avg_resolution_time",
"AVG(CASE WHEN name = 'first_response' THEN #{average_value_key} END) as avg_first_response_time",
"AVG(CASE WHEN name = 'reply_time' THEN #{average_value_key} END) as avg_reply_time"
)
.group(group_by_key)
.index_by { |record| record.public_send(index_key) }
@resolved_count = results.transform_values(&:resolved_count)
@avg_resolution_time = results.transform_values(&:avg_resolution_time)
@avg_first_response_time = results.transform_values(&:avg_first_response_time)
@avg_reply_time = results.transform_values(&:avg_reply_time)
end
def reporting_events
@reporting_events ||= account.reporting_events.where(created_at: range)
end
def fetch_conversations_count
# Override this method
@conversations_count = results.transform_values { |data| data[:conversations_count] }
@resolved_count = results.transform_values { |data| data[:resolved_conversations_count] }
@avg_resolution_time = results.transform_values { |data| data[:avg_resolution_time] }
@avg_first_response_time = results.transform_values { |data| data[:avg_first_response_time] }
@avg_reply_time = results.transform_values { |data| data[:avg_reply_time] }
end
def group_by_key
@@ -50,7 +26,26 @@ class V2::Reports::BaseSummaryBuilder
# Override this method
end
def average_value_key
ActiveModel::Type::Boolean.new.cast(params[:business_hours]).present? ? :value_in_business_hours : :value
def data_source
@data_source ||= Reports::DataSource.for(
account: account,
metric: nil,
dimension_type: summary_dimension_type,
dimension_id: nil,
scope: nil,
range: range,
group_by: 'day',
timezone_offset: params[:timezone_offset],
business_hours: params[:business_hours]
)
end
def summary_dimension_type
{
'account_id' => 'account',
'user_id' => 'agent',
'inbox_id' => 'inbox',
'conversations.team_id' => 'team'
}.fetch(group_by_key.to_s)
end
end
@@ -3,23 +3,10 @@ class V2::Reports::Conversations::BaseReportBuilder
private
AVG_METRICS = %w[avg_first_response_time avg_resolution_time reply_time].freeze
COUNT_METRICS = %w[
conversations_count
incoming_messages_count
outgoing_messages_count
resolutions_count
bot_resolutions_count
bot_handoffs_count
].freeze
def builder_class(metric)
case metric
when *AVG_METRICS
V2::Reports::Timeseries::AverageReportBuilder
when *COUNT_METRICS
V2::Reports::Timeseries::CountReportBuilder
end
return unless Reports::ReportMetricRegistry.supported?(metric)
V2::Reports::Timeseries::ReportBuilder
end
def log_invalid_metric
@@ -11,15 +11,6 @@ class V2::Reports::InboxSummaryBuilder < V2::Reports::BaseSummaryBuilder
attr_reader :conversations_count, :resolved_count,
:avg_resolution_time, :avg_first_response_time, :avg_reply_time
def load_data
@conversations_count = fetch_conversations_count
load_reporting_events_data
end
def fetch_conversations_count
account.conversations.where(created_at: range).group(group_by_key).count
end
def prepare_report
account.inboxes.map do |inbox|
build_inbox_stats(inbox)
@@ -40,8 +31,4 @@ class V2::Reports::InboxSummaryBuilder < V2::Reports::BaseSummaryBuilder
def group_by_key
:inbox_id
end
def average_value_key
ActiveModel::Type::Boolean.new.cast(params[:business_hours]) ? :value_in_business_hours : :value
end
end
@@ -6,14 +6,6 @@ class V2::Reports::TeamSummaryBuilder < V2::Reports::BaseSummaryBuilder
attr_reader :conversations_count, :resolved_count,
:avg_resolution_time, :avg_first_response_time, :avg_reply_time
def fetch_conversations_count
account.conversations.where(created_at: range).group(:team_id).count
end
def reporting_events
@reporting_events ||= account.reporting_events.where(created_at: range).joins(:conversation)
end
def prepare_report
account.teams.map do |team|
build_team_stats(team)
@@ -1,48 +0,0 @@
class V2::Reports::Timeseries::AverageReportBuilder < V2::Reports::Timeseries::BaseTimeseriesBuilder
def timeseries
grouped_average_time = reporting_events.average(average_value_key)
grouped_event_count = reporting_events.count
grouped_average_time.each_with_object([]) do |element, arr|
event_date, average_time = element
arr << {
value: average_time,
timestamp: event_date.in_time_zone(timezone).to_i,
count: grouped_event_count[event_date]
}
end
end
def aggregate_value
object_scope.average(average_value_key)
end
private
def event_name
metric_to_event_name = {
avg_first_response_time: :first_response,
avg_resolution_time: :conversation_resolved,
reply_time: :reply_time
}
metric_to_event_name[params[:metric].to_sym]
end
def object_scope
scope.reporting_events.where(name: event_name, created_at: range, account_id: account.id)
end
def reporting_events
@grouped_values = object_scope.group_by_period(
group_by,
:created_at,
default_value: 0,
range: range,
permit: %w[day week month year hour],
time_zone: timezone
)
end
def average_value_key
@average_value_key ||= params[:business_hours].present? ? :value_in_business_hours : :value
end
end
@@ -1,12 +1,13 @@
class V2::Reports::Timeseries::BaseTimeseriesBuilder
include TimezoneHelper
include DateRangeHelper
DEFAULT_GROUP_BY = 'day'.freeze
pattr_initialize :account, :params
def scope
case params[:type].to_sym
case dimension_type.to_sym
when :account
account
when :inbox
@@ -20,6 +21,20 @@ class V2::Reports::Timeseries::BaseTimeseriesBuilder
end
end
def data_source
@data_source ||= Reports::DataSource.for(
account: account,
metric: params[:metric],
dimension_type: dimension_type,
dimension_id: params[:id],
scope: scope,
range: range,
group_by: group_by,
timezone_offset: params[:timezone_offset],
business_hours: params[:business_hours]
)
end
def inbox
@inbox ||= account.inboxes.find(params[:id])
end
@@ -43,4 +58,10 @@ class V2::Reports::Timeseries::BaseTimeseriesBuilder
def timezone
@timezone ||= timezone_name_from_offset(params[:timezone_offset])
end
private
def dimension_type
(params[:type].presence || 'account').to_s
end
end
@@ -1,78 +0,0 @@
class V2::Reports::Timeseries::CountReportBuilder < V2::Reports::Timeseries::BaseTimeseriesBuilder
def timeseries
grouped_count.each_with_object([]) do |element, arr|
event_date, event_count = element
# The `event_date` is in Date format (without time), such as "Wed, 15 May 2024".
# We need a timestamp for the start of the day. However, we can't use `event_date.to_time.to_i`
# because it converts the date to 12:00 AM server timezone.
# The desired output should be 12:00 AM in the specified timezone.
arr << { value: event_count, timestamp: event_date.in_time_zone(timezone).to_i }
end
end
def aggregate_value
object_scope.count
end
private
def metric
@metric ||= params[:metric]
end
def object_scope
send("scope_for_#{metric}")
end
def scope_for_conversations_count
scope.conversations.where(account_id: account.id, created_at: range)
end
def scope_for_incoming_messages_count
scope.messages.where(account_id: account.id, created_at: range).incoming.unscope(:order)
end
def scope_for_outgoing_messages_count
scope.messages.where(account_id: account.id, created_at: range).outgoing.unscope(:order)
end
def scope_for_resolutions_count
scope.reporting_events.where(
name: :conversation_resolved,
account_id: account.id,
created_at: range
)
end
def scope_for_bot_resolutions_count
scope.reporting_events.where(
name: :conversation_bot_resolved,
account_id: account.id,
created_at: range
)
end
def scope_for_bot_handoffs_count
scope.reporting_events.joins(:conversation).select(:conversation_id).where(
name: :conversation_bot_handoff,
account_id: account.id,
created_at: range
).distinct
end
def grouped_count
# IMPORTANT: time_zone parameter affects both data grouping AND output timestamps
# It converts timestamps to the target timezone before grouping, which means
# the same event can fall into different day buckets depending on timezone
# Example: 2024-01-15 00:00 UTC becomes 2024-01-14 16:00 PST (falls on different day)
@grouped_values = object_scope.group_by_period(
group_by,
:created_at,
default_value: 0,
range: range,
permit: %w[day week month year hour],
time_zone: timezone
).count
end
end
@@ -0,0 +1,9 @@
class V2::Reports::Timeseries::ReportBuilder < V2::Reports::Timeseries::BaseTimeseriesBuilder
def timeseries
data_source.timeseries
end
def aggregate_value
data_source.aggregate
end
end
@@ -3,15 +3,15 @@ class Api::V2::Accounts::SummaryReportsController < Api::V1::Accounts::BaseContr
before_action :prepare_builder_params, only: [:agent, :team, :inbox, :label, :channel]
def agent
render_report_with(V2::Reports::AgentSummaryBuilder)
render_report_with(V2::Reports::AgentSummaryBuilder, type: :agent)
end
def team
render_report_with(V2::Reports::TeamSummaryBuilder)
render_report_with(V2::Reports::TeamSummaryBuilder, type: :team)
end
def inbox
render_report_with(V2::Reports::InboxSummaryBuilder)
render_report_with(V2::Reports::InboxSummaryBuilder, type: :inbox)
end
def label
@@ -38,8 +38,9 @@ class Api::V2::Accounts::SummaryReportsController < Api::V1::Accounts::BaseContr
}
end
def render_report_with(builder_class)
builder = builder_class.new(account: Current.account, params: @builder_params)
def render_report_with(builder_class, type: nil)
builder_params = type.present? ? @builder_params.merge(type: type) : @builder_params
builder = builder_class.new(account: Current.account, params: builder_params)
render json: builder.build
end
+64
View File
@@ -0,0 +1,64 @@
class Reports::DataSource
include TimezoneHelper
attr_reader :account, :metric, :dimension_type, :dimension_id,
:scope, :range, :group_by, :timezone_offset,
:business_hours
class << self
def for(**context)
# TODO: Route to Reports::RollupDataSource when rollup reads are implemented
Reports::RawDataSource.new(**context)
end
end
def initialize(**context)
@account = context[:account]
@metric = context[:metric]
@dimension_type = (context[:dimension_type].presence || 'account').to_s
@dimension_id = context[:dimension_id]
@scope = context[:scope]
@range = context[:range]
@group_by = context[:group_by].to_s.presence || 'day'
@timezone_offset = context[:timezone_offset]
@business_hours = context[:business_hours]
end
private
def report_metric
@report_metric ||= Reports::ReportMetricRegistry.fetch(metric)
end
def average_metric?
report_metric&.average?
end
def count_metric?
!average_metric?
end
def rollup_metric
report_metric&.rollup_metric
end
def raw_event_name
report_metric&.raw_event_name
end
def raw_count_strategy
report_metric&.raw_count_strategy
end
def summary_metrics
@summary_metrics ||= Reports::ReportMetricRegistry.summary_metrics
end
def timezone
@timezone ||= timezone_name_from_offset(timezone_offset)
end
def use_business_hours?
ActiveModel::Type::Boolean.new.cast(business_hours)
end
end
+156
View File
@@ -0,0 +1,156 @@
class Reports::RawDataSource < Reports::DataSource
def timeseries
average_metric? ? average_timeseries : count_timeseries
end
def aggregate
average_metric? ? average_scope.average(average_value_key) : count_scope.count
end
def summary
metric_results = summary_scope
.select(*summary_select_fields)
.group(summary_group_by_key)
.index_by { |record| record.public_send(summary_index_key) }
merge_summary_results(metric_results, summary_conversation_counts)
end
private
def count_timeseries
grouped_count.map do |event_date, event_count|
{ value: event_count, timestamp: event_date.in_time_zone(timezone).to_i }
end
end
def average_timeseries
grouped_average_time = grouped_average_scope.average(average_value_key)
grouped_event_count = grouped_average_scope.count
grouped_average_time.each_with_object([]) do |(event_date, average_time), results|
results << {
value: average_time,
timestamp: event_date.in_time_zone(timezone).to_i,
count: grouped_event_count[event_date]
}
end
end
def grouped_average_scope
average_scope.group_by_period(
group_by,
:created_at,
default_value: 0,
range: range,
permit: %w[day week month year hour],
time_zone: timezone
)
end
def grouped_count
count_scope.group_by_period(
group_by,
:created_at,
default_value: 0,
range: range,
permit: %w[day week month year hour],
time_zone: timezone
).count
end
def average_scope
scope.reporting_events.where(name: raw_event_name, created_at: range, account_id: account.id)
end
def count_scope
case metric.to_s
when 'conversations_count'
scope.conversations.where(account_id: account.id, created_at: range)
when 'incoming_messages_count'
scope.messages.where(account_id: account.id, created_at: range).incoming.unscope(:order)
when 'outgoing_messages_count'
scope.messages.where(account_id: account.id, created_at: range).outgoing.unscope(:order)
else
reporting_event_count_scope
end
end
def reporting_event_count_scope
events = scope.reporting_events.where(
name: raw_event_name,
account_id: account.id,
created_at: range
)
return events unless raw_count_strategy == :distinct_conversation
events.joins(:conversation).select(:conversation_id).distinct
end
def summary_scope
scope = account.reporting_events.where(created_at: range)
return scope.joins(:conversation) if dimension_type == 'team'
scope
end
def summary_conversation_counts
account.conversations
.where(created_at: range)
.group(summary_conversation_group_by_key)
.count
end
def merge_summary_results(metric_results, conversation_counts)
(metric_results.keys | conversation_counts.keys).each_with_object({}) do |dimension_id, results|
record = metric_results[dimension_id]
results[dimension_id] = summary_attributes_for(record, conversation_counts[dimension_id])
end
end
def summary_select_fields
["#{summary_group_by_key} as #{summary_index_key}"] + summary_metrics.map { |definition| summary_select_field(definition) }
end
def summary_select_field(definition)
if definition.count?
"COUNT(CASE WHEN name = '#{definition.raw_event_name}' THEN 1 END) as #{definition.summary_key}"
else
"AVG(CASE WHEN name = '#{definition.raw_event_name}' THEN #{average_value_key} END) as #{definition.summary_key}"
end
end
def summary_attributes_for(record, conversations_count = 0)
summary_metrics.each_with_object({ conversations_count: conversations_count.to_i }) do |definition, attributes|
value = record&.public_send(definition.summary_key)
attributes[definition.summary_key] = definition.count? ? value.to_i : value
end
end
def summary_group_by_key
{
'account' => :account_id,
'agent' => :user_id,
'inbox' => :inbox_id,
'team' => 'conversations.team_id'
}[dimension_type]
end
def summary_conversation_group_by_key
{
'account' => :account_id,
'agent' => :assignee_id,
'inbox' => :inbox_id,
'team' => :team_id
}[dimension_type]
end
def summary_index_key
summary_group_by_key.to_s.split('.').last
end
def average_value_key
use_business_hours? ? :value_in_business_hours : :value
end
end
@@ -0,0 +1,120 @@
module Reports::ReportMetricRegistry
# Describes one public report metric.
# name: API-facing metric name requested by reports.
# aggregate: whether the metric is a count or average.
# raw_event_name: source reporting_events name for raw queries.
# rollup_metric: source reporting_events_rollups metric for rollup queries.
# summary_key: key used when this metric appears in grouped summary responses.
# raw_count_strategy: optional raw-query counting rule, such as distinct conversations.
Metric = Data.define(
:name,
:aggregate,
:raw_event_name,
:rollup_metric,
:summary_key,
:raw_count_strategy
) do
def initialize(name:, aggregate:, raw_event_name: nil, rollup_metric: nil, summary_key: nil, raw_count_strategy: nil) # rubocop:disable Metrics/ParameterLists
super
end
def average?
aggregate == :average
end
def count?
aggregate == :count
end
def rollup_supported?
rollup_metric.present?
end
def summary?
summary_key.present?
end
end
METRICS = {
conversations_count: Metric.new(
name: :conversations_count,
aggregate: :count
),
incoming_messages_count: Metric.new(
name: :incoming_messages_count,
aggregate: :count
),
outgoing_messages_count: Metric.new(
name: :outgoing_messages_count,
aggregate: :count
),
avg_first_response_time: Metric.new(
name: :avg_first_response_time,
aggregate: :average,
raw_event_name: :first_response,
rollup_metric: :first_response,
summary_key: :avg_first_response_time
),
avg_resolution_time: Metric.new(
name: :avg_resolution_time,
aggregate: :average,
raw_event_name: :conversation_resolved,
rollup_metric: :resolution_time,
summary_key: :avg_resolution_time
),
reply_time: Metric.new(
name: :reply_time,
aggregate: :average,
raw_event_name: :reply_time,
rollup_metric: :reply_time,
summary_key: :avg_reply_time
),
resolutions_count: Metric.new(
name: :resolutions_count,
aggregate: :count,
raw_event_name: :conversation_resolved,
rollup_metric: :resolutions_count,
summary_key: :resolved_conversations_count
),
bot_resolutions_count: Metric.new(
name: :bot_resolutions_count,
aggregate: :count,
raw_event_name: :conversation_bot_resolved,
rollup_metric: :bot_resolutions_count
),
bot_handoffs_count: Metric.new(
name: :bot_handoffs_count,
aggregate: :count,
raw_event_name: :conversation_bot_handoff,
rollup_metric: :bot_handoffs_count,
raw_count_strategy: :distinct_conversation
)
}.freeze
SUMMARY_METRIC_NAMES = %i[
resolutions_count
avg_resolution_time
avg_first_response_time
reply_time
].freeze
module_function
def fetch(name)
return if name.blank?
METRICS[name.to_sym]
end
def supported?(name)
fetch(name).present?
end
def rollup_supported?(name)
fetch(name)&.rollup_supported? || false
end
def summary_metrics
SUMMARY_METRIC_NAMES.map { |metric_name| METRICS.fetch(metric_name) }
end
end
@@ -5,12 +5,10 @@ RSpec.describe V2::Reports::Conversations::MetricBuilder, type: :model do
let(:account) { create(:account) }
let(:params) { { since: '2023-01-01', until: '2024-01-01' } }
let(:count_builder_instance) { instance_double(V2::Reports::Timeseries::CountReportBuilder, aggregate_value: 42) }
let(:avg_builder_instance) { instance_double(V2::Reports::Timeseries::AverageReportBuilder, aggregate_value: 42) }
let(:builder_instance) { instance_double(V2::Reports::Timeseries::ReportBuilder, aggregate_value: 42) }
before do
allow(V2::Reports::Timeseries::CountReportBuilder).to receive(:new).and_return(count_builder_instance)
allow(V2::Reports::Timeseries::AverageReportBuilder).to receive(:new).and_return(avg_builder_instance)
allow(V2::Reports::Timeseries::ReportBuilder).to receive(:new).and_return(builder_instance)
end
describe '#summary' do
@@ -31,8 +29,8 @@ RSpec.describe V2::Reports::Conversations::MetricBuilder, type: :model do
it 'creates builders with proper params' do
subject.summary
expect(V2::Reports::Timeseries::CountReportBuilder).to have_received(:new).with(account, params.merge(metric: 'conversations_count'))
expect(V2::Reports::Timeseries::AverageReportBuilder).to have_received(:new).with(account, params.merge(metric: 'avg_first_response_time'))
expect(V2::Reports::Timeseries::ReportBuilder).to have_received(:new).with(account, params.merge(metric: 'conversations_count'))
expect(V2::Reports::Timeseries::ReportBuilder).to have_received(:new).with(account, params.merge(metric: 'avg_first_response_time'))
end
end
@@ -4,19 +4,19 @@ describe V2::Reports::Conversations::ReportBuilder do
subject { described_class.new(account, params) }
let(:account) { create(:account) }
let(:average_builder) { V2::Reports::Timeseries::AverageReportBuilder }
let(:count_builder) { V2::Reports::Timeseries::CountReportBuilder }
let(:builder) { V2::Reports::Timeseries::ReportBuilder }
shared_examples 'valid metric handler' do |metric, method, builder|
shared_examples 'valid metric handler' do |metric, method|
context 'when a valid metric is given' do
let(:params) { { metric: metric } }
it "calls the correct #{method} builder for #{metric}" do
it "calls the shared #{method} builder for #{metric}" do
builder_instance = instance_double(builder)
allow(builder).to receive(:new).and_return(builder_instance)
allow(builder_instance).to receive(method)
allow(builder_instance).to receive(method).and_return(:result)
builder_instance.public_send(method)
expect(subject.public_send(method)).to eq(:result)
expect(builder).to have_received(:new).with(account, params)
expect(builder_instance).to have_received(method)
end
end
@@ -33,12 +33,12 @@ describe V2::Reports::Conversations::ReportBuilder do
end
describe '#timeseries' do
it_behaves_like 'valid metric handler', 'avg_first_response_time', :timeseries, V2::Reports::Timeseries::AverageReportBuilder
it_behaves_like 'valid metric handler', 'conversations_count', :timeseries, V2::Reports::Timeseries::CountReportBuilder
it_behaves_like 'valid metric handler', 'avg_first_response_time', :timeseries
it_behaves_like 'valid metric handler', 'conversations_count', :timeseries
end
describe '#aggregate_value' do
it_behaves_like 'valid metric handler', 'avg_first_response_time', :aggregate_value, V2::Reports::Timeseries::AverageReportBuilder
it_behaves_like 'valid metric handler', 'conversations_count', :aggregate_value, V2::Reports::Timeseries::CountReportBuilder
it_behaves_like 'valid metric handler', 'avg_first_response_time', :aggregate_value
it_behaves_like 'valid metric handler', 'conversations_count', :aggregate_value
end
end
@@ -1,174 +0,0 @@
require 'rails_helper'
describe V2::Reports::Timeseries::AverageReportBuilder do
subject { described_class.new(account, params) }
let(:account) { create(:account) }
let(:team) { create(:team, account: account) }
let(:inbox) { create(:inbox, account: account) }
let(:label) { create(:label, title: 'spec-billing', account: account) }
let!(:conversation) { create(:conversation, account: account, inbox: inbox, team: team) }
let(:current_time) { '26.10.2020 10:00'.to_datetime }
let(:params) do
{
type: filter_type,
business_hours: business_hours,
timezone_offset: timezone_offset,
group_by: group_by,
metric: metric,
since: (current_time - 1.week).beginning_of_day.to_i.to_s,
until: current_time.end_of_day.to_i.to_s,
id: filter_id
}
end
let(:timezone_offset) { nil }
let(:group_by) { 'day' }
let(:metric) { 'avg_first_response_time' }
let(:business_hours) { false }
let(:filter_type) { :account }
let(:filter_id) { '' }
before do
travel_to current_time
conversation.label_list.add(label.title)
conversation.save!
create(:reporting_event, name: 'first_response', value: 80, value_in_business_hours: 10, account: account, created_at: Time.zone.now,
conversation: conversation, inbox: inbox)
create(:reporting_event, name: 'first_response', value: 100, value_in_business_hours: 20, account: account, created_at: 1.hour.ago)
create(:reporting_event, name: 'first_response', value: 93, value_in_business_hours: 30, account: account, created_at: 1.week.ago)
end
describe '#timeseries' do
context 'when there is no filter applied' do
it 'returns the correct values' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: 1_603_065_600, value: 93.0 },
{ count: 0, timestamp: 1_603_152_000, value: 0 },
{ count: 0, timestamp: 1_603_238_400, value: 0 },
{ count: 0, timestamp: 1_603_324_800, value: 0 },
{ count: 0, timestamp: 1_603_411_200, value: 0 },
{ count: 0, timestamp: 1_603_497_600, value: 0 },
{ count: 0, timestamp: 1_603_584_000, value: 0 },
{ count: 2, timestamp: 1_603_670_400, value: 90.0 }
]
)
end
context 'when business hours is provided' do
let(:business_hours) { true }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: 1_603_065_600, value: 30.0 },
{ count: 0, timestamp: 1_603_152_000, value: 0 },
{ count: 0, timestamp: 1_603_238_400, value: 0 },
{ count: 0, timestamp: 1_603_324_800, value: 0 },
{ count: 0, timestamp: 1_603_411_200, value: 0 },
{ count: 0, timestamp: 1_603_497_600, value: 0 },
{ count: 0, timestamp: 1_603_584_000, value: 0 },
{ count: 2, timestamp: 1_603_670_400, value: 15.0 }
]
)
end
end
context 'when group_by is provided' do
let(:group_by) { 'week' }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: (current_time - 1.week).beginning_of_week(:sunday).to_i, value: 93.0 },
{ count: 2, timestamp: current_time.beginning_of_week(:sunday).to_i, value: 90.0 }
]
)
end
end
context 'when timezone offset is provided' do
let(:timezone_offset) { '5.5' }
let(:group_by) { 'week' }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: (current_time - 1.week).in_time_zone('Chennai').beginning_of_week(:sunday).to_i, value: 93.0 },
{ count: 2, timestamp: current_time.in_time_zone('Chennai').beginning_of_week(:sunday).to_i, value: 90.0 }
]
)
end
end
end
context 'when the label filter is applied' do
let(:group_by) { 'week' }
let(:filter_type) { 'label' }
let(:filter_id) { label.id }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
start_of_the_week = current_time.beginning_of_week(:sunday).to_i
last_week_start_of_the_week = (current_time - 1.week).beginning_of_week(:sunday).to_i
expect(timeseries_values).to eq(
[
{ count: 0, timestamp: last_week_start_of_the_week, value: 0 },
{ count: 1, timestamp: start_of_the_week, value: 80.0 }
]
)
end
end
context 'when the inbox filter is applied' do
let(:group_by) { 'week' }
let(:filter_type) { 'inbox' }
let(:filter_id) { inbox.id }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
start_of_the_week = current_time.beginning_of_week(:sunday).to_i
last_week_start_of_the_week = (current_time - 1.week).beginning_of_week(:sunday).to_i
expect(timeseries_values).to eq(
[
{ count: 0, timestamp: last_week_start_of_the_week, value: 0 },
{ count: 1, timestamp: start_of_the_week, value: 80.0 }
]
)
end
end
context 'when the team filter is applied' do
let(:group_by) { 'week' }
let(:filter_type) { 'team' }
let(:filter_id) { team.id }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
start_of_the_week = current_time.beginning_of_week(:sunday).to_i
last_week_start_of_the_week = (current_time - 1.week).beginning_of_week(:sunday).to_i
expect(timeseries_values).to eq(
[
{ count: 0, timestamp: last_week_start_of_the_week, value: 0 },
{ count: 1, timestamp: start_of_the_week, value: 80.0 }
]
)
end
end
end
describe '#aggregate_value' do
context 'when there is no filter applied' do
it 'returns the correct average value' do
expect(subject.aggregate_value).to eq 91.0
end
end
end
end
@@ -1,113 +0,0 @@
require 'rails_helper'
describe V2::Reports::Timeseries::CountReportBuilder do
subject { described_class.new(account, params) }
let(:account) { create(:account) }
let(:account2) { create(:account) }
let(:user) { create(:user, email: 'agent1@example.com') }
let(:inbox) { create(:inbox, account: account) }
let(:inbox2) { create(:inbox, account: account2) }
let(:current_time) { Time.current }
let(:params) do
{
type: 'agent',
metric: 'resolutions_count',
since: (current_time - 1.day).beginning_of_day.to_i.to_s,
until: current_time.end_of_day.to_i.to_s,
id: user.id.to_s
}
end
before do
travel_to current_time
# Add the same user to both accounts
create(:account_user, account: account, user: user)
create(:account_user, account: account2, user: user)
# Create conversations in account1
conversation1 = create(:conversation, account: account, inbox: inbox, assignee: user)
conversation2 = create(:conversation, account: account, inbox: inbox, assignee: user)
# Create conversations in account2
conversation3 = create(:conversation, account: account2, inbox: inbox2, assignee: user)
conversation4 = create(:conversation, account: account2, inbox: inbox2, assignee: user)
# User resolves 2 conversations in account1
create(:reporting_event,
name: 'conversation_resolved',
account: account,
user: user,
conversation: conversation1,
created_at: current_time - 12.hours)
create(:reporting_event,
name: 'conversation_resolved',
account: account,
user: user,
conversation: conversation2,
created_at: current_time - 6.hours)
# Same user resolves 3 conversations in account2 - these should NOT be counted for account1
create(:reporting_event,
name: 'conversation_resolved',
account: account2,
user: user,
conversation: conversation3,
created_at: current_time - 8.hours)
create(:reporting_event,
name: 'conversation_resolved',
account: account2,
user: user,
conversation: conversation4,
created_at: current_time - 4.hours)
# Create another conversation in account2 for testing
conversation5 = create(:conversation, account: account2, inbox: inbox2, assignee: user)
create(:reporting_event,
name: 'conversation_resolved',
account: account2,
user: user,
conversation: conversation5,
created_at: current_time - 2.hours)
end
describe '#aggregate_value' do
it 'returns only resolutions performed by the user in the specified account' do
# User should have 2 resolutions in account1, not 5 (total across both accounts)
expect(subject.aggregate_value).to eq(2)
end
context 'when querying account2' do
subject { described_class.new(account2, params) }
it 'returns only resolutions for account2' do
# User should have 3 resolutions in account2
expect(subject.aggregate_value).to eq(3)
end
end
end
describe '#timeseries' do
it 'filters resolutions by account' do
result = subject.timeseries
# Should only count the 2 resolutions from account1
total_count = result.sum { |r| r[:value] }
expect(total_count).to eq(2)
end
end
describe 'account isolation' do
it 'does not leak data between accounts' do
# If account isolation works correctly, the counts should be different
account1_count = described_class.new(account, params).aggregate_value
account2_count = described_class.new(account2, params).aggregate_value
expect(account1_count).to eq(2)
expect(account2_count).to eq(3)
end
end
end
@@ -0,0 +1,313 @@
require 'rails_helper'
describe V2::Reports::Timeseries::ReportBuilder do
describe 'average metrics' do
subject { described_class.new(account, params) }
let(:account) { create(:account) }
let(:team) { create(:team, account: account) }
let(:inbox) { create(:inbox, account: account) }
let(:label) { create(:label, title: 'spec-billing', account: account) }
let!(:conversation) { create(:conversation, account: account, inbox: inbox, team: team) }
let(:current_time) { '26.10.2020 10:00'.to_datetime }
let(:params) do
{
type: filter_type,
business_hours: business_hours,
timezone_offset: timezone_offset,
group_by: group_by,
metric: metric,
since: (current_time - 1.week).beginning_of_day.to_i.to_s,
until: current_time.end_of_day.to_i.to_s,
id: filter_id
}
end
let(:timezone_offset) { nil }
let(:group_by) { 'day' }
let(:metric) { 'avg_first_response_time' }
let(:business_hours) { false }
let(:filter_type) { :account }
let(:filter_id) { '' }
before do
travel_to current_time
conversation.label_list.add(label.title)
conversation.save!
create(:reporting_event, name: 'first_response', value: 80, value_in_business_hours: 10, account: account, created_at: Time.zone.now,
conversation: conversation, inbox: inbox)
create(:reporting_event, name: 'first_response', value: 100, value_in_business_hours: 20, account: account, created_at: 1.hour.ago)
create(:reporting_event, name: 'first_response', value: 93, value_in_business_hours: 30, account: account, created_at: 1.week.ago)
end
describe '#timeseries' do
it 'returns the correct values' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: 1_603_065_600, value: 93.0 },
{ count: 0, timestamp: 1_603_152_000, value: 0 },
{ count: 0, timestamp: 1_603_238_400, value: 0 },
{ count: 0, timestamp: 1_603_324_800, value: 0 },
{ count: 0, timestamp: 1_603_411_200, value: 0 },
{ count: 0, timestamp: 1_603_497_600, value: 0 },
{ count: 0, timestamp: 1_603_584_000, value: 0 },
{ count: 2, timestamp: 1_603_670_400, value: 90.0 }
]
)
end
context 'when business hours is provided' do
let(:business_hours) { true }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: 1_603_065_600, value: 30.0 },
{ count: 0, timestamp: 1_603_152_000, value: 0 },
{ count: 0, timestamp: 1_603_238_400, value: 0 },
{ count: 0, timestamp: 1_603_324_800, value: 0 },
{ count: 0, timestamp: 1_603_411_200, value: 0 },
{ count: 0, timestamp: 1_603_497_600, value: 0 },
{ count: 0, timestamp: 1_603_584_000, value: 0 },
{ count: 2, timestamp: 1_603_670_400, value: 15.0 }
]
)
end
end
context 'when group_by is provided' do
let(:group_by) { 'week' }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: (current_time - 1.week).beginning_of_week(:sunday).to_i, value: 93.0 },
{ count: 2, timestamp: current_time.beginning_of_week(:sunday).to_i, value: 90.0 }
]
)
end
end
context 'when timezone offset is provided' do
let(:timezone_offset) { '5.5' }
let(:group_by) { 'week' }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
expect(timeseries_values).to eq(
[
{ count: 1, timestamp: (current_time - 1.week).in_time_zone('Chennai').beginning_of_week(:sunday).to_i, value: 93.0 },
{ count: 2, timestamp: current_time.in_time_zone('Chennai').beginning_of_week(:sunday).to_i, value: 90.0 }
]
)
end
end
context 'when the label filter is applied' do
let(:group_by) { 'week' }
let(:filter_type) { 'label' }
let(:filter_id) { label.id }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
start_of_the_week = current_time.beginning_of_week(:sunday).to_i
last_week_start_of_the_week = (current_time - 1.week).beginning_of_week(:sunday).to_i
expect(timeseries_values).to eq(
[
{ count: 0, timestamp: last_week_start_of_the_week, value: 0 },
{ count: 1, timestamp: start_of_the_week, value: 80.0 }
]
)
end
end
context 'when the inbox filter is applied' do
let(:group_by) { 'week' }
let(:filter_type) { 'inbox' }
let(:filter_id) { inbox.id }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
start_of_the_week = current_time.beginning_of_week(:sunday).to_i
last_week_start_of_the_week = (current_time - 1.week).beginning_of_week(:sunday).to_i
expect(timeseries_values).to eq(
[
{ count: 0, timestamp: last_week_start_of_the_week, value: 0 },
{ count: 1, timestamp: start_of_the_week, value: 80.0 }
]
)
end
end
context 'when the team filter is applied' do
let(:group_by) { 'week' }
let(:filter_type) { 'team' }
let(:filter_id) { team.id }
it 'returns correct timeseries' do
timeseries_values = subject.timeseries
start_of_the_week = current_time.beginning_of_week(:sunday).to_i
last_week_start_of_the_week = (current_time - 1.week).beginning_of_week(:sunday).to_i
expect(timeseries_values).to eq(
[
{ count: 0, timestamp: last_week_start_of_the_week, value: 0 },
{ count: 1, timestamp: start_of_the_week, value: 80.0 }
]
)
end
end
end
describe '#aggregate_value' do
context 'when there is no filter applied' do
it 'returns the correct average value' do
expect(subject.aggregate_value).to eq 91.0
end
end
context 'when rollups are enabled and the agent does not exist' do
let(:filter_type) { :agent }
let(:filter_id) { '999999' }
let(:timezone_offset) { '0' }
before do
account.update!(reporting_timezone: 'Etc/UTC')
allow(account).to receive(:feature_enabled?).with(:report_rollup).and_return(true)
end
it 'raises record not found to preserve raw path behavior' do
expect { subject.aggregate_value }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
end
describe 'count metrics' do
subject { described_class.new(account, params) }
let(:account) { create(:account) }
let(:account2) { create(:account) }
let(:user) { create(:user, email: 'agent1@example.com') }
let(:inbox) { create(:inbox, account: account) }
let(:inbox2) { create(:inbox, account: account2) }
let(:current_time) { Time.current }
let(:params) do
{
type: 'agent',
metric: 'resolutions_count',
since: since_time.beginning_of_day.to_i.to_s,
until: current_time.end_of_day.to_i.to_s,
timezone_offset: timezone_offset,
group_by: group_by,
id: user.id.to_s
}
end
let(:group_by) { 'day' }
let(:since_time) { current_time - 1.day }
let(:timezone_offset) { nil }
before do
travel_to current_time
create(:account_user, account: account, user: user)
create(:account_user, account: account2, user: user)
conversation1 = create(:conversation, account: account, inbox: inbox, assignee: user)
conversation2 = create(:conversation, account: account, inbox: inbox, assignee: user)
conversation3 = create(:conversation, account: account2, inbox: inbox2, assignee: user)
conversation4 = create(:conversation, account: account2, inbox: inbox2, assignee: user)
create(:reporting_event,
name: 'conversation_resolved',
account: account,
user: user,
conversation: conversation1,
created_at: current_time - 12.hours)
create(:reporting_event,
name: 'conversation_resolved',
account: account,
user: user,
conversation: conversation2,
created_at: current_time - 6.hours)
create(:reporting_event,
name: 'conversation_resolved',
account: account2,
user: user,
conversation: conversation3,
created_at: current_time - 8.hours)
create(:reporting_event,
name: 'conversation_resolved',
account: account2,
user: user,
conversation: conversation4,
created_at: current_time - 4.hours)
conversation5 = create(:conversation, account: account2, inbox: inbox2, assignee: user)
create(:reporting_event,
name: 'conversation_resolved',
account: account2,
user: user,
conversation: conversation5,
created_at: current_time - 2.hours)
end
describe '#aggregate_value' do
it 'returns only resolutions performed by the user in the specified account' do
expect(subject.aggregate_value).to eq(2)
end
context 'when rollups are enabled and the agent does not exist' do
let(:timezone_offset) { '0' }
let(:params) do
super().merge(id: '999999')
end
before do
account.update!(reporting_timezone: 'Etc/UTC')
allow(account).to receive(:feature_enabled?).with(:report_rollup).and_return(true)
end
it 'raises record not found to preserve raw path behavior' do
expect { subject.aggregate_value }.to raise_error(ActiveRecord::RecordNotFound)
end
end
context 'when querying account2' do
subject { described_class.new(account2, params) }
it 'returns only resolutions for account2' do
expect(subject.aggregate_value).to eq(3)
end
end
end
describe '#timeseries' do
it 'filters resolutions by account' do
result = subject.timeseries
total_count = result.sum { |row| row[:value] }
expect(total_count).to eq(2)
end
end
describe 'account isolation' do
it 'does not leak data between accounts' do
account1_count = described_class.new(account, params).aggregate_value
account2_count = described_class.new(account2, params).aggregate_value
expect(account1_count).to eq(2)
expect(account2_count).to eq(3)
end
end
end
end
@@ -45,7 +45,10 @@ RSpec.describe 'Summary Reports API', type: :request do
headers: admin.create_new_auth_token,
as: :json
expect(V2::Reports::AgentSummaryBuilder).to have_received(:new).with(account: account, params: params)
expect(V2::Reports::AgentSummaryBuilder).to have_received(:new).with(
account: account,
params: params.merge(type: :agent)
)
expect(agent_summary_builder).to have_received(:build)
expect(response).to have_http_status(:success)
@@ -96,7 +99,10 @@ RSpec.describe 'Summary Reports API', type: :request do
headers: admin.create_new_auth_token,
as: :json
expect(V2::Reports::InboxSummaryBuilder).to have_received(:new).with(account: account, params: params)
expect(V2::Reports::InboxSummaryBuilder).to have_received(:new).with(
account: account,
params: params.merge(type: :inbox)
)
expect(inbox_summary_builder).to have_received(:build)
expect(response).to have_http_status(:success)
@@ -147,7 +153,10 @@ RSpec.describe 'Summary Reports API', type: :request do
headers: admin.create_new_auth_token,
as: :json
expect(V2::Reports::TeamSummaryBuilder).to have_received(:new).with(account: account, params: params)
expect(V2::Reports::TeamSummaryBuilder).to have_received(:new).with(
account: account,
params: params.merge(type: :team)
)
expect(team_summary_builder).to have_received(:build)
expect(response).to have_http_status(:success)
+1 -1
View File
@@ -83,7 +83,7 @@ describe EmailChannelFinder do
reply_mail.mail['bcc'] = 'test@example.com'
# Configure other account IDs but not this one
other_account_ids = [123, 456, 789]
other_account_ids = [channel_email.account_id + 1, channel_email.account_id + 2, channel_email.account_id + 3]
allow(GlobalConfigService).to receive(:load)
.with('SKIP_INCOMING_BCC_PROCESSING', '')
.and_return(other_account_ids.join(','))
@@ -0,0 +1,74 @@
require 'rails_helper'
RSpec.describe Reports::ReportMetricRegistry do
describe '.fetch' do
it 'returns the definition for raw-only count metrics' do
metric = described_class.fetch(:conversations_count)
expect(metric.name).to eq(:conversations_count)
expect(metric.count?).to be(true)
expect(metric.rollup_supported?).to be(false)
expect(metric.raw_event_name).to be_nil
end
it 'returns the definition for avg_resolution_time' do
metric = described_class.fetch(:avg_resolution_time)
expect(metric.name).to eq(:avg_resolution_time)
expect(metric.average?).to be(true)
expect(metric.raw_event_name).to eq(:conversation_resolved)
expect(metric.rollup_metric).to eq(:resolution_time)
expect(metric.summary_key).to eq(:avg_resolution_time)
end
it 'locks the distinct conversation strategy for bot_handoffs_count' do
metric = described_class.fetch(:bot_handoffs_count)
expect(metric.count?).to be(true)
expect(metric.raw_event_name).to eq(:conversation_bot_handoff)
expect(metric.rollup_metric).to eq(:bot_handoffs_count)
expect(metric.raw_count_strategy).to eq(:distinct_conversation)
end
it 'returns nil for unsupported metrics' do
expect(described_class.fetch(:unknown_metric)).to be_nil
end
end
describe '.supported?' do
it 'returns true for supported raw-only metrics' do
expect(described_class.supported?(:conversations_count)).to be(true)
end
it 'returns false for unsupported metrics' do
expect(described_class.supported?(:unknown_metric)).to be(false)
end
end
describe '.rollup_supported?' do
it 'returns true for rollup-backed metrics' do
expect(described_class.rollup_supported?(:reply_time)).to be(true)
end
it 'returns false for raw-only metrics' do
expect(described_class.rollup_supported?(:conversations_count)).to be(false)
end
end
describe '.summary_metrics' do
it 'returns the summary metric definitions in registry order' do
expect(
described_class.summary_metrics.map do |metric|
[metric.name, metric.summary_key, metric.aggregate, metric.raw_event_name, metric.rollup_metric]
end
).to eq(
[
[:resolutions_count, :resolved_conversations_count, :count, :conversation_resolved, :resolutions_count],
[:avg_resolution_time, :avg_resolution_time, :average, :conversation_resolved, :resolution_time],
[:avg_first_response_time, :avg_first_response_time, :average, :first_response, :first_response],
[:reply_time, :avg_reply_time, :average, :reply_time, :reply_time]
]
)
end
end
end