feat: Improve Report API performance (#9476)

- Re-write the methods for clarity
- Remove the dependency on the ReportHelper class.
- Remove n+1 queries in the average metric time series data.

app/builders/v2/reports/conversations/base_report_builder.rb

@@ -0,0 +1,30 @@
+class V2::Reports::Conversations::BaseReportBuilder
+  pattr_initialize :account, :params
+
+  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
+  end
+
+  def log_invalid_metric
+    Rails.logger.error "ReportBuilder: Invalid metric - #{params[:metric]}"
+
+    {}
+  end
+end

app/builders/v2/reports/conversations/metric_builder.rb

@@ -0,0 +1,30 @@
+class V2::Reports::Conversations::MetricBuilder < V2::Reports::Conversations::BaseReportBuilder
+  def summary
+    {
+      conversations_count: count('conversations_count'),
+      incoming_messages_count: count('incoming_messages_count'),
+      outgoing_messages_count: count('outgoing_messages_count'),
+      avg_first_response_time: count('avg_first_response_time'),
+      avg_resolution_time: count('avg_resolution_time'),
+      resolutions_count: count('resolutions_count'),
+      reply_time: count('reply_time')
+    }
+  end
+
+  def bot_summary
+    {
+      bot_resolutions_count: count('bot_resolutions_count'),
+      bot_handoffs_count: count('bot_handoffs_count')
+    }
+  end
+
+  private
+
+  def count(metric)
+    builder_class(metric).new(account, builder_params(metric)).aggregate_value
+  end
+
+  def builder_params(metric)
+    params.merge({ metric: metric })
+  end
+end

app/builders/v2/reports/conversations/report_builder.rb

@@ -0,0 +1,21 @@
+class V2::Reports::Conversations::ReportBuilder < V2::Reports::Conversations::BaseReportBuilder
+  def timeseries
+    perform_action(:timeseries)
+  end
+
+  def aggregate_value
+    perform_action(:aggregate_value)
+  end
+
+  private
+
+  def perform_action(method_name)
+    return builder.new(account, params).public_send(method_name) if builder.present?
+
+    log_invalid_metric
+  end
+
+  def builder
+    builder_class(params[:metric])
+  end
+end

app/builders/v2/reports/timeseries/average_report_builder.rb

@@ -0,0 +1,48 @@
+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)
+  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

app/builders/v2/reports/timeseries/base_timeseries_builder.rb

@@ -0,0 +1,46 @@
+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
+    when :account
+      account
+    when :inbox
+      inbox
+    when :agent
+      user
+    when :label
+      label
+    when :team
+      team
+    end
+  end
+
+  def inbox
+    @inbox ||= account.inboxes.find(params[:id])
+  end
+
+  def user
+    @user ||= account.users.find(params[:id])
+  end
+
+  def label
+    @label ||= account.labels.find(params[:id])
+  end
+
+  def team
+    @team ||= account.teams.find(params[:id])
+  end
+
+  def group_by
+    @group_by ||= %w[day week month year hour].include?(params[:group_by]) ? params[:group_by] : DEFAULT_GROUP_BY
+  end
+
+  def timezone
+    @timezone ||= timezone_name_from_offset(params[:timezone_offset])
+  end
+end

app/builders/v2/reports/timeseries/count_report_builder.rb

@@ -0,0 +1,71 @@
+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.joins(:conversation).select(:conversation_id).where(
+      name: :conversation_resolved,
+      conversations: { status: :resolved }, created_at: range
+    ).distinct
+  end
+
+  def scope_for_bot_resolutions_count
+    scope.reporting_events.joins(:conversation).select(:conversation_id).where(
+      name: :conversation_bot_resolved,
+      conversations: { status: :resolved }, created_at: range
+    ).distinct
+  end
+
+  def scope_for_bot_handoffs_count
+    scope.reporting_events.joins(:conversation).select(:conversation_id).where(
+      name: :conversation_bot_handoff,
+      created_at: range
+    ).distinct
+  end
+
+  def grouped_count
+    @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

app/controllers/api/v2/accounts/reports_controller.rb

@@ -5,19 +5,17 @@ class Api::V2::Accounts::ReportsController < Api::V1::Accounts::BaseController
   before_action :check_authorization
 
   def index
-    builder = V2::ReportBuilder.new(Current.account, report_params)
-    data = builder.build
+    builder = V2::Reports::Conversations::ReportBuilder.new(Current.account, report_params)
+    data = builder.timeseries
     render json: data
   end
 
   def summary
-    render json: summary_metrics
+    render json: build_summary(:summary)
   end
 
   def bot_summary
-    summary = V2::ReportBuilder.new(Current.account, current_summary_params).bot_summary
-    summary[:previous] = V2::ReportBuilder.new(Current.account, previous_summary_params).bot_summary
-    render json: summary
+    render json: build_summary(:bot_summary)
   end
 
   def agents
@@ -126,10 +124,11 @@ class Api::V2::Accounts::ReportsController < Api::V1::Accounts::BaseController
     }
   end
 
-  def summary_metrics
-    summary = V2::ReportBuilder.new(Current.account, current_summary_params).summary
-    summary[:previous] = V2::ReportBuilder.new(Current.account, previous_summary_params).summary
-    summary
+  def build_summary(method)
+    builder = V2::Reports::Conversations::MetricBuilder
+    current_summary = builder.new(Current.account, current_summary_params).send(method)
+    previous_summary = builder.new(Current.account, previous_summary_params).send(method)
+    current_summary.merge(previous: previous_summary)
   end
 
   def conversation_metrics

app/helpers/timezone_helper.rb

@@ -0,0 +1,19 @@
+module TimezoneHelper
+  # ActiveSupport TimeZone is not aware of the current time, so ActiveSupport::Timezone[offset]
+  # would return the timezone without considering day light savings. To get the correct timezone,
+  # this method uses zone.now.utc_offset for comparison as referenced in the issues below
+  #
+  # https://github.com/rails/rails/pull/22243
+  # https://github.com/rails/rails/issues/21501
+  # https://github.com/rails/rails/issues/7297
+  def timezone_name_from_offset(offset)
+    return 'UTC' if offset.blank?
+
+    offset_in_seconds = offset.to_f * 3600
+    matching_zone = ActiveSupport::TimeZone.all.find do |zone|
+      zone.now.utc_offset == offset_in_seconds
+    end
+
+    return matching_zone.name if matching_zone
+  end
+end

spec/builders/v2/reports/conversations/metric_builder_spec.rb

@@ -0,0 +1,50 @@
+require 'rails_helper'
+
+RSpec.describe V2::Reports::Conversations::MetricBuilder, type: :model do
+  subject { described_class.new(account, params) }
+
+  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) }
+
+  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)
+  end
+
+  describe '#summary' do
+    it 'returns the correct summary values' do
+      summary = subject.summary
+      expect(summary).to eq(
+        {
+          conversations_count: 42,
+          incoming_messages_count: 42,
+          outgoing_messages_count: 42,
+          avg_first_response_time: 42,
+          avg_resolution_time: 42,
+          resolutions_count: 42,
+          reply_time: 42
+        }
+      )
+    end
+
+    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'))
+    end
+  end
+
+  describe '#bot_summary' do
+    it 'returns a detailed summary of bot-specific conversation metrics' do
+      bot_summary = subject.bot_summary
+      expect(bot_summary).to eq(
+        {
+          bot_resolutions_count: 42,
+          bot_handoffs_count: 42
+        }
+      )
+    end
+  end
+end

spec/builders/v2/reports/conversations/report_builder_spec.rb

@@ -0,0 +1,44 @@
+require 'rails_helper'
+
+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 }
+
+  shared_examples 'valid metric handler' do |metric, method, builder|
+    context 'when a valid metric is given' do
+      let(:params) { { metric: metric } }
+
+      it "calls the correct #{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)
+
+        builder_instance.public_send(method)
+        expect(builder_instance).to have_received(method)
+      end
+    end
+  end
+
+  context 'when invalid metric is given' do
+    let(:metric) { 'invalid_metric' }
+    let(:params) { { metric: metric } }
+
+    it 'logs the error and returns empty value' do
+      expect(Rails.logger).to receive(:error).with("ReportBuilder: Invalid metric - #{metric}")
+      expect(subject.timeseries).to eq({})
+    end
+  end
+
+  describe '#timeseries' do
+    include_examples 'valid metric handler', 'avg_first_response_time', :timeseries, V2::Reports::Timeseries::AverageReportBuilder
+    include_examples 'valid metric handler', 'conversations_count', :timeseries, V2::Reports::Timeseries::CountReportBuilder
+  end
+
+  describe '#aggregate_value' do
+    include_examples 'valid metric handler', 'avg_first_response_time', :aggregate_value, V2::Reports::Timeseries::AverageReportBuilder
+    include_examples 'valid metric handler', 'conversations_count', :aggregate_value, V2::Reports::Timeseries::CountReportBuilder
+  end
+end

spec/builders/v2/reports/timeseries/average_report_builder_spec.rb

@@ -0,0 +1,174 @@
+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

spec/controllers/api/v2/accounts/report_controller_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe 'Reports API', type: :request do
   let!(:user) { create(:user, account: account) }
   let!(:inbox) { create(:inbox, account: account) }
   let(:inbox_member) { create(:inbox_member, user: user, inbox: inbox) }
-  let(:default_timezone) { ActiveSupport::TimeZone[0]&.name }
+  let(:default_timezone) { 'UTC' }
   let(:start_of_today) { Time.current.in_time_zone(default_timezone).beginning_of_day.to_i }
   let(:end_of_today) { Time.current.in_time_zone(default_timezone).end_of_day.to_i }
   let(:params) { { timezone_offset: Time.zone.utc_offset } }
@@ -18,7 +18,7 @@ RSpec.describe 'Reports API', type: :request do
                                    assignee: user, created_at: Time.current.in_time_zone(default_timezone).to_date)
   end
 
-  describe 'GET /api/v2/accounts/:account_id/reports/account' do
+  describe 'GET /api/v2/accounts/:account_id/reports' do
     context 'when it is an unauthenticated user' do
       it 'returns unauthorized' do
         get "/api/v2/accounts/#{account.id}/reports"

spec/factories/reporting_events.rb

@@ -2,6 +2,7 @@ FactoryBot.define do
   factory :reporting_event do
     name { 'MyString' }
     value { 1.5 }
+    value_in_business_hours { 1 }
     account_id { 1 }
     inbox_id { 1 }
     user_id { 1 }