diff --git a/app/builders/v2/reports/conversations/base_report_builder.rb b/app/builders/v2/reports/conversations/base_report_builder.rb new file mode 100644 index 000000000..a7961b0d6 --- /dev/null +++ b/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 diff --git a/app/builders/v2/reports/conversations/metric_builder.rb b/app/builders/v2/reports/conversations/metric_builder.rb new file mode 100644 index 000000000..6635fb186 --- /dev/null +++ b/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 diff --git a/app/builders/v2/reports/conversations/report_builder.rb b/app/builders/v2/reports/conversations/report_builder.rb new file mode 100644 index 000000000..8f992d4c6 --- /dev/null +++ b/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 diff --git a/app/builders/v2/reports/timeseries/average_report_builder.rb b/app/builders/v2/reports/timeseries/average_report_builder.rb new file mode 100644 index 000000000..3e30557e1 --- /dev/null +++ b/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 diff --git a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb new file mode 100644 index 000000000..50699417d --- /dev/null +++ b/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 diff --git a/app/builders/v2/reports/timeseries/count_report_builder.rb b/app/builders/v2/reports/timeseries/count_report_builder.rb new file mode 100644 index 000000000..03a87a6fa --- /dev/null +++ b/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 diff --git a/app/controllers/api/v2/accounts/reports_controller.rb b/app/controllers/api/v2/accounts/reports_controller.rb index c67b74a43..ed5be5518 100644 --- a/app/controllers/api/v2/accounts/reports_controller.rb +++ b/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 diff --git a/app/helpers/timezone_helper.rb b/app/helpers/timezone_helper.rb new file mode 100644 index 000000000..b016cc9d9 --- /dev/null +++ b/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 diff --git a/spec/builders/v2/reports/conversations/metric_builder_spec.rb b/spec/builders/v2/reports/conversations/metric_builder_spec.rb new file mode 100644 index 000000000..1b0ed7a38 --- /dev/null +++ b/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 diff --git a/spec/builders/v2/reports/conversations/report_builder_spec.rb b/spec/builders/v2/reports/conversations/report_builder_spec.rb new file mode 100644 index 000000000..d3cde98e6 --- /dev/null +++ b/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 diff --git a/spec/builders/v2/reports/timeseries/average_report_builder_spec.rb b/spec/builders/v2/reports/timeseries/average_report_builder_spec.rb new file mode 100644 index 000000000..4f6036f07 --- /dev/null +++ b/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 diff --git a/spec/controllers/api/v2/accounts/report_controller_spec.rb b/spec/controllers/api/v2/accounts/report_controller_spec.rb index fc0228c86..6202946a1 100644 --- a/spec/controllers/api/v2/accounts/report_controller_spec.rb +++ b/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" diff --git a/spec/factories/reporting_events.rb b/spec/factories/reporting_events.rb index c779c01f6..1db6a87df 100644 --- a/spec/factories/reporting_events.rb +++ b/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 }