diff --git a/app/builders/v2/report_builder.rb b/app/builders/v2/report_builder.rb index fb986d335..b935bd520 100644 --- a/app/builders/v2/report_builder.rb +++ b/app/builders/v2/report_builder.rb @@ -1,6 +1,7 @@ class V2::ReportBuilder include DateRangeHelper include ReportHelper + attr_reader :account, :params DEFAULT_GROUP_BY = 'day'.freeze diff --git a/app/builders/v2/reports/concerns/rollup_conditions.rb b/app/builders/v2/reports/concerns/rollup_conditions.rb index a908b028f..50781b4d2 100644 --- a/app/builders/v2/reports/concerns/rollup_conditions.rb +++ b/app/builders/v2/reports/concerns/rollup_conditions.rb @@ -21,7 +21,7 @@ module V2::Reports::Concerns::RollupConditions return false unless metric_covered? # Condition 3: Not hourly granularity - return false if group_by == 'hour' + return false if params[:group_by] == 'hour' # Condition 4: Dimension must be supported return false unless dimension_supported? diff --git a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb index d562b396c..e8e5245ab 100644 --- a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb +++ b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb @@ -2,6 +2,7 @@ class V2::Reports::Timeseries::BaseTimeseriesBuilder include TimezoneHelper include DateRangeHelper include V2::Reports::Concerns::RollupConditions + DEFAULT_GROUP_BY = 'day'.freeze pattr_initialize :account, :params diff --git a/app/builders/v2/reports/timeseries/count_report_builder.rb b/app/builders/v2/reports/timeseries/count_report_builder.rb index 29aa1308e..5d9e8aa4f 100644 --- a/app/builders/v2/reports/timeseries/count_report_builder.rb +++ b/app/builders/v2/reports/timeseries/count_report_builder.rb @@ -73,34 +73,51 @@ class V2::Reports::Timeseries::CountReportBuilder < V2::Reports::Timeseries::Bas end def group_and_aggregate_rollup_counts(rollup_rows) - grouped_data = {} + # Pre-fill all periods with zero to match raw path's default_value: 0 behavior + grouped_data = all_periods_in_range.index_with { 0 } rollup_rows.each do |row| - date_key = case group_by - when 'day' - row.date - when 'week' - row.date.beginning_of_week(:monday) - when 'month' - row.date.beginning_of_month - when 'year' - row.date.beginning_of_year - else - row.date - end - + date_key = date_key_for_period(row.date) grouped_data[date_key] ||= 0 grouped_data[date_key] += row.count end - grouped_data.each_with_object([]) do |(date_key, count), arr| - arr << { - value: count, - timestamp: date_key.in_time_zone(timezone).to_i - } + grouped_data.map do |date_key, count| + { value: count, timestamp: date_key.in_time_zone(timezone).to_i } end.sort_by { |h| h[:timestamp] } end + def date_key_for_period(date) + case group_by + when 'day' then date + when 'week' then date.beginning_of_week(:monday) + when 'month' then date.beginning_of_month + when 'year' then date.beginning_of_year + else date + end + end + + def all_periods_in_range + date_range = rollup_date_range + dates = [] + current = date_key_for_period(date_range.first) + while current <= date_range.last + dates << current + current = advance_period(current) + end + dates + end + + def advance_period(date) + case group_by + when 'day' then date + 1.day + when 'week' then date + 1.week + when 'month' then date + 1.month + when 'year' then date + 1.year + else date + 1.day + end + end + def metric @metric ||= params[:metric] end diff --git a/spec/builders/v2/reports/concerns/rollup_conditions_spec.rb b/spec/builders/v2/reports/concerns/rollup_conditions_spec.rb index dcc6cd20b..a95bd44d2 100644 --- a/spec/builders/v2/reports/concerns/rollup_conditions_spec.rb +++ b/spec/builders/v2/reports/concerns/rollup_conditions_spec.rb @@ -4,11 +4,8 @@ describe V2::Reports::Concerns::RollupConditions do let(:dummy_class) do Class.new do include V2::Reports::Concerns::RollupConditions - attr_accessor :account, :params, :_group_by - def group_by - @_group_by || 'day' - end + attr_accessor :account, :params # Make private methods public for testing public :metric_to_rollup_metric, :dimension_type_to_rollup @@ -36,7 +33,6 @@ describe V2::Reports::Concerns::RollupConditions do context 'when all conditions pass' do it 'returns true' do builder.params = valid_params - builder._group_by = 'day' expect(builder.use_rollup?).to be true end end @@ -45,7 +41,6 @@ describe V2::Reports::Concerns::RollupConditions do it 'returns false' do account.update!(reporting_timezone: nil) builder.params = valid_params - builder._group_by = 'day' expect(builder.use_rollup?).to be false end end @@ -54,7 +49,6 @@ describe V2::Reports::Concerns::RollupConditions do it 'returns false' do allow(account).to receive(:feature_enabled?).with('reporting_events_rollup').and_return(false) builder.params = valid_params - builder._group_by = 'day' expect(builder.use_rollup?).to be false end end @@ -62,19 +56,19 @@ describe V2::Reports::Concerns::RollupConditions do context 'Condition 2: metric is not covered' do it 'returns false for conversations_count' do builder.params = valid_params.merge(metric: 'conversations_count') - builder._group_by = 'day' + expect(builder.use_rollup?).to be false end it 'returns false for incoming_messages_count' do builder.params = valid_params.merge(metric: 'incoming_messages_count') - builder._group_by = 'day' + expect(builder.use_rollup?).to be false end it 'returns false for outgoing_messages_count' do builder.params = valid_params.merge(metric: 'outgoing_messages_count') - builder._group_by = 'day' + expect(builder.use_rollup?).to be false end end @@ -82,69 +76,63 @@ describe V2::Reports::Concerns::RollupConditions do context 'Condition 2: metric is covered' do it 'returns true for avg_first_response_time' do builder.params = valid_params.merge(metric: 'avg_first_response_time') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for avg_resolution_time' do builder.params = valid_params - builder._group_by = 'day' expect(builder.use_rollup?).to be true end it 'returns true for reply_time' do builder.params = valid_params.merge(metric: 'reply_time') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for resolutions_count' do builder.params = valid_params.merge(metric: 'resolutions_count') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for bot_resolutions_count' do builder.params = valid_params.merge(metric: 'bot_resolutions_count') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for bot_handoffs_count' do builder.params = valid_params.merge(metric: 'bot_handoffs_count') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end end context 'Condition 3: group_by is hourly' do it 'returns false when group_by is hour' do - builder.params = valid_params - builder._group_by = 'hour' + builder.params = valid_params.merge(group_by: 'hour') expect(builder.use_rollup?).to be false end it 'returns true for day granularity' do builder.params = valid_params - builder._group_by = 'day' expect(builder.use_rollup?).to be true end it 'returns true for week granularity' do - builder.params = valid_params - builder._group_by = 'week' + builder.params = valid_params.merge(group_by: 'week') expect(builder.use_rollup?).to be true end it 'returns true for month granularity' do - builder.params = valid_params - builder._group_by = 'month' + builder.params = valid_params.merge(group_by: 'month') expect(builder.use_rollup?).to be true end it 'returns true for year granularity' do - builder.params = valid_params - builder._group_by = 'year' + builder.params = valid_params.merge(group_by: 'year') expect(builder.use_rollup?).to be true end end @@ -152,31 +140,31 @@ describe V2::Reports::Concerns::RollupConditions do context 'Condition 4: dimension is not supported' do it 'returns false for label dimension' do builder.params = valid_params.merge(type: 'label') - builder._group_by = 'day' + expect(builder.use_rollup?).to be false end it 'returns true for account dimension' do builder.params = valid_params.merge(type: 'account') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for agent dimension' do builder.params = valid_params.merge(type: 'agent') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for inbox dimension' do builder.params = valid_params.merge(type: 'inbox') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true for team dimension' do builder.params = valid_params.merge(type: 'team') - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end end @@ -184,25 +172,25 @@ describe V2::Reports::Concerns::RollupConditions do context 'Condition 5: timezone_offset does not match' do it 'returns false when timezone_offset is UTC instead of EST' do builder.params = valid_params.merge(timezone_offset: 0) - builder._group_by = 'day' + expect(builder.use_rollup?).to be false end it 'returns false when timezone_offset is UTC+5:30 (different from EST)' do builder.params = valid_params.merge(timezone_offset: 5.5) - builder._group_by = 'day' + expect(builder.use_rollup?).to be false end it 'returns true when timezone_offset matches account timezone' do builder.params = valid_params.merge(timezone_offset: -5) - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end it 'returns true when timezone_offset is blank (defaults to account timezone)' do builder.params = valid_params.merge(timezone_offset: nil) - builder._group_by = 'day' + expect(builder.use_rollup?).to be true end end @@ -210,7 +198,7 @@ describe V2::Reports::Concerns::RollupConditions do context 'when used from BaseSummaryBuilder (no params[:metric])' do it 'returns true because summary builders override metric_covered?' do builder.params = { type: 'agent', timezone_offset: -5 } - builder._group_by = 'day' + # Without the override, this would return false since params[:metric] is blank expect(builder.use_rollup?).to be false @@ -223,8 +211,7 @@ describe V2::Reports::Concerns::RollupConditions do summary_builder = summary_builder_class.new summary_builder.account = account - summary_builder.params = { type: 'agent', timezone_offset: -5 } - summary_builder._group_by = 'day' + summary_builder.params = { type: 'agent', timezone_offset: -5, group_by: 'day' } expect(summary_builder.use_rollup?).to be true end end