From 6cbddbdb67a560d6d71a54002afd578167d6d3c9 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Mon, 20 Apr 2026 11:15:48 +0530 Subject: [PATCH 1/2] feat(rollup): report builder abstraction [2/3] (#13798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 Co-authored-by: Tanmay Deep Sharma Co-authored-by: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> --- app/builders/v2/report_builder.rb | 1 + .../v2/reports/agent_summary_builder.rb | 4 - .../v2/reports/base_summary_builder.rb | 59 ++-- .../conversations/base_report_builder.rb | 19 +- .../v2/reports/inbox_summary_builder.rb | 13 - .../v2/reports/team_summary_builder.rb | 8 - .../timeseries/average_report_builder.rb | 48 --- .../timeseries/base_timeseries_builder.rb | 23 +- .../timeseries/count_report_builder.rb | 78 ----- .../v2/reports/timeseries/report_builder.rb | 9 + .../v2/accounts/summary_reports_controller.rb | 11 +- app/services/reports/data_source.rb | 64 ++++ app/services/reports/raw_data_source.rb | 156 +++++++++ .../reports/report_metric_registry.rb | 120 +++++++ .../conversations/metric_builder_spec.rb | 10 +- .../conversations/report_builder_spec.rb | 20 +- .../timeseries/average_report_builder_spec.rb | 174 ---------- .../timeseries/count_report_builder_spec.rb | 113 ------- .../reports/timeseries/report_builder_spec.rb | 313 ++++++++++++++++++ .../summary_reports_controller_spec.rb | 15 +- spec/finders/email_channel_finder_spec.rb | 2 +- .../reports/report_metric_registry_spec.rb | 74 +++++ 22 files changed, 822 insertions(+), 512 deletions(-) delete mode 100644 app/builders/v2/reports/timeseries/average_report_builder.rb delete mode 100644 app/builders/v2/reports/timeseries/count_report_builder.rb create mode 100644 app/builders/v2/reports/timeseries/report_builder.rb create mode 100644 app/services/reports/data_source.rb create mode 100644 app/services/reports/raw_data_source.rb create mode 100644 app/services/reports/report_metric_registry.rb delete mode 100644 spec/builders/v2/reports/timeseries/average_report_builder_spec.rb delete mode 100644 spec/builders/v2/reports/timeseries/count_report_builder_spec.rb create mode 100644 spec/builders/v2/reports/timeseries/report_builder_spec.rb create mode 100644 spec/services/reports/report_metric_registry_spec.rb 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/agent_summary_builder.rb b/app/builders/v2/reports/agent_summary_builder.rb index 9b4541aaa..58689aaad 100644 --- a/app/builders/v2/reports/agent_summary_builder.rb +++ b/app/builders/v2/reports/agent_summary_builder.rb @@ -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) diff --git a/app/builders/v2/reports/base_summary_builder.rb b/app/builders/v2/reports/base_summary_builder.rb index d4a9e7c0b..bd8ed5f56 100644 --- a/app/builders/v2/reports/base_summary_builder.rb +++ b/app/builders/v2/reports/base_summary_builder.rb @@ -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 diff --git a/app/builders/v2/reports/conversations/base_report_builder.rb b/app/builders/v2/reports/conversations/base_report_builder.rb index a7961b0d6..ee0155150 100644 --- a/app/builders/v2/reports/conversations/base_report_builder.rb +++ b/app/builders/v2/reports/conversations/base_report_builder.rb @@ -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 diff --git a/app/builders/v2/reports/inbox_summary_builder.rb b/app/builders/v2/reports/inbox_summary_builder.rb index 935afeb82..fcfabc599 100644 --- a/app/builders/v2/reports/inbox_summary_builder.rb +++ b/app/builders/v2/reports/inbox_summary_builder.rb @@ -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 diff --git a/app/builders/v2/reports/team_summary_builder.rb b/app/builders/v2/reports/team_summary_builder.rb index b98151bc6..3bcf51816 100644 --- a/app/builders/v2/reports/team_summary_builder.rb +++ b/app/builders/v2/reports/team_summary_builder.rb @@ -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) diff --git a/app/builders/v2/reports/timeseries/average_report_builder.rb b/app/builders/v2/reports/timeseries/average_report_builder.rb deleted file mode 100644 index 5df718b6a..000000000 --- a/app/builders/v2/reports/timeseries/average_report_builder.rb +++ /dev/null @@ -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 diff --git a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb index 50699417d..86fddba07 100644 --- a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb +++ b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb @@ -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 diff --git a/app/builders/v2/reports/timeseries/count_report_builder.rb b/app/builders/v2/reports/timeseries/count_report_builder.rb deleted file mode 100644 index bb3b1250c..000000000 --- a/app/builders/v2/reports/timeseries/count_report_builder.rb +++ /dev/null @@ -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 diff --git a/app/builders/v2/reports/timeseries/report_builder.rb b/app/builders/v2/reports/timeseries/report_builder.rb new file mode 100644 index 000000000..a3e83c78a --- /dev/null +++ b/app/builders/v2/reports/timeseries/report_builder.rb @@ -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 diff --git a/app/controllers/api/v2/accounts/summary_reports_controller.rb b/app/controllers/api/v2/accounts/summary_reports_controller.rb index 98b3f05d7..40d5947b9 100644 --- a/app/controllers/api/v2/accounts/summary_reports_controller.rb +++ b/app/controllers/api/v2/accounts/summary_reports_controller.rb @@ -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 diff --git a/app/services/reports/data_source.rb b/app/services/reports/data_source.rb new file mode 100644 index 000000000..dff392c12 --- /dev/null +++ b/app/services/reports/data_source.rb @@ -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 diff --git a/app/services/reports/raw_data_source.rb b/app/services/reports/raw_data_source.rb new file mode 100644 index 000000000..f37b3c454 --- /dev/null +++ b/app/services/reports/raw_data_source.rb @@ -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 diff --git a/app/services/reports/report_metric_registry.rb b/app/services/reports/report_metric_registry.rb new file mode 100644 index 000000000..8df2e091d --- /dev/null +++ b/app/services/reports/report_metric_registry.rb @@ -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 diff --git a/spec/builders/v2/reports/conversations/metric_builder_spec.rb b/spec/builders/v2/reports/conversations/metric_builder_spec.rb index 1b0ed7a38..018bd567b 100644 --- a/spec/builders/v2/reports/conversations/metric_builder_spec.rb +++ b/spec/builders/v2/reports/conversations/metric_builder_spec.rb @@ -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 diff --git a/spec/builders/v2/reports/conversations/report_builder_spec.rb b/spec/builders/v2/reports/conversations/report_builder_spec.rb index db7a0ac45..a2b293461 100644 --- a/spec/builders/v2/reports/conversations/report_builder_spec.rb +++ b/spec/builders/v2/reports/conversations/report_builder_spec.rb @@ -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 diff --git a/spec/builders/v2/reports/timeseries/average_report_builder_spec.rb b/spec/builders/v2/reports/timeseries/average_report_builder_spec.rb deleted file mode 100644 index 4f6036f07..000000000 --- a/spec/builders/v2/reports/timeseries/average_report_builder_spec.rb +++ /dev/null @@ -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 diff --git a/spec/builders/v2/reports/timeseries/count_report_builder_spec.rb b/spec/builders/v2/reports/timeseries/count_report_builder_spec.rb deleted file mode 100644 index 038bd61c2..000000000 --- a/spec/builders/v2/reports/timeseries/count_report_builder_spec.rb +++ /dev/null @@ -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 diff --git a/spec/builders/v2/reports/timeseries/report_builder_spec.rb b/spec/builders/v2/reports/timeseries/report_builder_spec.rb new file mode 100644 index 000000000..eec3e99da --- /dev/null +++ b/spec/builders/v2/reports/timeseries/report_builder_spec.rb @@ -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 diff --git a/spec/controllers/api/v2/accounts/summary_reports_controller_spec.rb b/spec/controllers/api/v2/accounts/summary_reports_controller_spec.rb index 38c66eeb0..696e1f53e 100644 --- a/spec/controllers/api/v2/accounts/summary_reports_controller_spec.rb +++ b/spec/controllers/api/v2/accounts/summary_reports_controller_spec.rb @@ -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) diff --git a/spec/finders/email_channel_finder_spec.rb b/spec/finders/email_channel_finder_spec.rb index d56d97008..06fe5c968 100644 --- a/spec/finders/email_channel_finder_spec.rb +++ b/spec/finders/email_channel_finder_spec.rb @@ -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(',')) diff --git a/spec/services/reports/report_metric_registry_spec.rb b/spec/services/reports/report_metric_registry_spec.rb new file mode 100644 index 000000000..33fcb41e5 --- /dev/null +++ b/spec/services/reports/report_metric_registry_spec.rb @@ -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 From 787fcc4a9593a1b9ac7012eae568b13752c3a6ad Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Mon, 20 Apr 2026 13:08:19 +0530 Subject: [PATCH 2/2] chore: update conversation sidebar interactions (#13988) --- .../Contacts/ContactsDetailsLayout.vue | 3 +- .../Contacts/ContactsHeader/ContactHeader.vue | 4 +- .../NewConversation/ComposeConversation.vue | 110 ++------ .../components/ComposeNewConversationForm.vue | 2 +- .../components-next/popover/Popover.vue | 121 +++++++++ .../components-next/sidebar/Sidebar.vue | 24 +- .../conversation/ConversationSidebar.vue | 11 +- .../spec/useDropdownPosition.spec.js | 239 ++++++++++++++++++ .../composables/useDropdownPosition.js | 128 ++++++++++ .../modules/contact/ContactDeleteModal.vue | 94 +++++++ .../modules/contact/ContactMergeModal.vue | 69 +++-- .../conversation/contact/ContactInfo.vue | 136 +++------- .../conversation/contact/EditContact.vue | 112 ++++---- 13 files changed, 747 insertions(+), 306 deletions(-) create mode 100644 app/javascript/dashboard/components-next/popover/Popover.vue create mode 100644 app/javascript/dashboard/composables/spec/useDropdownPosition.spec.js create mode 100644 app/javascript/dashboard/composables/useDropdownPosition.js create mode 100644 app/javascript/dashboard/modules/contact/ContactDeleteModal.vue diff --git a/app/javascript/dashboard/components-next/Contacts/ContactsDetailsLayout.vue b/app/javascript/dashboard/components-next/Contacts/ContactsDetailsLayout.vue index 0bc1e376c..351cc7071 100644 --- a/app/javascript/dashboard/components-next/Contacts/ContactsDetailsLayout.vue +++ b/app/javascript/dashboard/components-next/Contacts/ContactsDetailsLayout.vue @@ -107,11 +107,10 @@ const closeMobileSidebar = () => { size="sm" /> -