fix: Address three PR review bugs in rollup system
Fix metric_covered? NoMethodError when metric param is nil - summary builders don't pass a metric param, so add nil guard before calling to_sym. Fix business_hours not respected in rollup paths - rollup queries were always using sum_value instead of switching to sum_value_business_hours when business_hours param is true. Add rollup_value_column helper to RollupConditions and use it across all rollup read paths. Fix date range timezone mismatch in rollup queries - rollup date column stores account-local dates but queries filtered with UTC DateTime boundaries from DateRangeHelper. Add rollup_date_range helper that converts UTC range to account-local dates and use it across all rollup query paths.
This commit is contained in:
@@ -48,21 +48,22 @@ class V2::Reports::BaseSummaryBuilder
|
||||
def load_rollup_data
|
||||
group_by_key.to_s.split('.').last
|
||||
dimension_type = dimension_type_to_rollup
|
||||
value_col = rollup_value_column
|
||||
|
||||
# Fetch rollup data grouped by dimension_id
|
||||
rollup_rows = ReportingEventsRollup.where(
|
||||
account_id: account.id,
|
||||
dimension_type: dimension_type,
|
||||
date: range.first..range.last
|
||||
date: rollup_date_range
|
||||
).group(:dimension_id).select(
|
||||
'dimension_id',
|
||||
'SUM(CASE WHEN metric = \'resolutions_count\' THEN count ELSE 0 END) as resolved_count',
|
||||
'SUM(CASE WHEN metric = \'resolution_time\' THEN count ELSE 0 END) as resolution_count',
|
||||
'SUM(CASE WHEN metric = \'resolution_time\' THEN sum_value ELSE 0 END) as resolution_sum_value',
|
||||
'SUM(CASE WHEN metric = \'first_response\' THEN count ELSE 0 END) as first_response_count',
|
||||
'SUM(CASE WHEN metric = \'first_response\' THEN sum_value ELSE 0 END) as first_response_sum_value',
|
||||
'SUM(CASE WHEN metric = \'reply_time\' THEN count ELSE 0 END) as reply_count',
|
||||
'SUM(CASE WHEN metric = \'reply_time\' THEN sum_value ELSE 0 END) as reply_sum_value'
|
||||
"SUM(CASE WHEN metric = 'resolution_time' THEN count ELSE 0 END) as resolution_count",
|
||||
"SUM(CASE WHEN metric = 'resolution_time' THEN #{value_col} ELSE 0 END) as resolution_sum_value",
|
||||
"SUM(CASE WHEN metric = 'first_response' THEN count ELSE 0 END) as first_response_count",
|
||||
"SUM(CASE WHEN metric = 'first_response' THEN #{value_col} ELSE 0 END) as first_response_sum_value",
|
||||
"SUM(CASE WHEN metric = 'reply_time' THEN count ELSE 0 END) as reply_count",
|
||||
"SUM(CASE WHEN metric = 'reply_time' THEN #{value_col} ELSE 0 END) as reply_sum_value"
|
||||
)
|
||||
|
||||
# Convert to the expected format
|
||||
|
||||
@@ -33,6 +33,8 @@ module V2::Reports::Concerns::RollupConditions
|
||||
private
|
||||
|
||||
def metric_covered?
|
||||
return false if params[:metric].blank?
|
||||
|
||||
COVERED_METRICS.key?(params[:metric].to_sym)
|
||||
end
|
||||
|
||||
@@ -64,6 +66,17 @@ module V2::Reports::Concerns::RollupConditions
|
||||
COVERED_METRICS[metric.to_sym]
|
||||
end
|
||||
|
||||
def rollup_value_column
|
||||
ActiveModel::Type::Boolean.new.cast(params[:business_hours]).present? ? :sum_value_business_hours : :sum_value
|
||||
end
|
||||
|
||||
def rollup_date_range
|
||||
tz = ActiveSupport::TimeZone[account.reporting_timezone]
|
||||
start_date = range.first.in_time_zone(tz).to_date
|
||||
end_date = range.last.in_time_zone(tz).to_date
|
||||
start_date..end_date
|
||||
end
|
||||
|
||||
def dimension_type_to_rollup
|
||||
case params[:type].to_s
|
||||
when 'account'
|
||||
|
||||
@@ -32,7 +32,7 @@ class V2::Reports::Timeseries::AverageReportBuilder < V2::Reports::Timeseries::B
|
||||
metric: metric,
|
||||
dimension_type: dimension_type,
|
||||
dimension_id: dimension_id,
|
||||
date: range.first..range.last
|
||||
date: rollup_date_range
|
||||
)
|
||||
|
||||
group_and_aggregate_rollup(rollup_rows)
|
||||
@@ -47,14 +47,15 @@ class V2::Reports::Timeseries::AverageReportBuilder < V2::Reports::Timeseries::B
|
||||
dimension_id = dimension_id_for_rollup
|
||||
metric = metric_to_rollup_metric(params[:metric])
|
||||
|
||||
value_col = rollup_value_column
|
||||
|
||||
result = ReportingEventsRollup.where(
|
||||
account_id: account.id,
|
||||
metric: metric,
|
||||
dimension_type: dimension_type,
|
||||
dimension_id: dimension_id,
|
||||
date: range.first..range.last
|
||||
).pluck(Arel.sql('SUM(count), SUM(sum_value)'))
|
||||
.first
|
||||
date: rollup_date_range
|
||||
).pick(Arel.sql("SUM(count), SUM(#{value_col})"))
|
||||
|
||||
return nil if result.blank? || result[0].to_i.zero?
|
||||
|
||||
@@ -76,6 +77,7 @@ class V2::Reports::Timeseries::AverageReportBuilder < V2::Reports::Timeseries::B
|
||||
|
||||
def group_and_aggregate_rollup(rollup_rows)
|
||||
grouped_data = {}
|
||||
value_col = rollup_value_column
|
||||
|
||||
rollup_rows.each do |row|
|
||||
date_key = case group_by
|
||||
@@ -93,7 +95,7 @@ class V2::Reports::Timeseries::AverageReportBuilder < V2::Reports::Timeseries::B
|
||||
|
||||
grouped_data[date_key] ||= { count: 0, sum_value: 0.0 }
|
||||
grouped_data[date_key][:count] += row.count
|
||||
grouped_data[date_key][:sum_value] += row.sum_value
|
||||
grouped_data[date_key][:sum_value] += row.public_send(value_col)
|
||||
end
|
||||
|
||||
grouped_data.each_with_object([]) do |(date_key, data), arr|
|
||||
|
||||
@@ -33,7 +33,7 @@ class V2::Reports::Timeseries::CountReportBuilder < V2::Reports::Timeseries::Bas
|
||||
metric: metric,
|
||||
dimension_type: dimension_type,
|
||||
dimension_id: dimension_id,
|
||||
date: range.first..range.last
|
||||
date: rollup_date_range
|
||||
)
|
||||
|
||||
group_and_aggregate_rollup_counts(rollup_rows)
|
||||
@@ -55,7 +55,7 @@ class V2::Reports::Timeseries::CountReportBuilder < V2::Reports::Timeseries::Bas
|
||||
metric: metric,
|
||||
dimension_type: dimension_type,
|
||||
dimension_id: dimension_id,
|
||||
date: range.first..range.last
|
||||
date: rollup_date_range
|
||||
).sum(:count).to_i
|
||||
end
|
||||
|
||||
|
||||
Reference in New Issue
Block a user