diff --git a/app/builders/v2/reports/base_summary_builder.rb b/app/builders/v2/reports/base_summary_builder.rb index ec98a7a63..603263ee9 100644 --- a/app/builders/v2/reports/base_summary_builder.rb +++ b/app/builders/v2/reports/base_summary_builder.rb @@ -36,7 +36,7 @@ class V2::Reports::BaseSummaryBuilder scope: nil, range: range, group_by: 'day', - timezone: timezone_name_from_offset(params[:timezone_offset]), + timezone: timezone_name_from_params(params[:timezone], params[:timezone_offset]), timezone_offset: params[:timezone_offset], business_hours: params[:business_hours] ) diff --git a/app/builders/v2/reports/label_summary_builder.rb b/app/builders/v2/reports/label_summary_builder.rb index 8b7c21e8e..013cc3a0d 100644 --- a/app/builders/v2/reports/label_summary_builder.rb +++ b/app/builders/v2/reports/label_summary_builder.rb @@ -7,8 +7,7 @@ class V2::Reports::LabelSummaryBuilder < V2::Reports::BaseSummaryBuilder @account = account @params = params - timezone_offset = (params[:timezone_offset] || 0).to_f - @timezone = ActiveSupport::TimeZone[timezone_offset]&.name + @timezone = timezone_name_from_params(params[:timezone], params[:timezone_offset]) end # rubocop:enable Lint/MissingSuper diff --git a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb index e1c668e44..0eaab7e52 100644 --- a/app/builders/v2/reports/timeseries/base_timeseries_builder.rb +++ b/app/builders/v2/reports/timeseries/base_timeseries_builder.rb @@ -57,7 +57,7 @@ class V2::Reports::Timeseries::BaseTimeseriesBuilder end def timezone - @timezone ||= timezone_name_from_offset(params[:timezone_offset]) + @timezone ||= timezone_name_from_params(params[:timezone], params[:timezone_offset]) end private diff --git a/app/controllers/api/v2/accounts/reports_controller.rb b/app/controllers/api/v2/accounts/reports_controller.rb index 192b3619c..cb93cbef6 100644 --- a/app/controllers/api/v2/accounts/reports_controller.rb +++ b/app/controllers/api/v2/accounts/reports_controller.rb @@ -112,6 +112,7 @@ class Api::V2::Accounts::ReportsController < Api::V1::Accounts::BaseController common_params.merge({ since: range[:current][:since], until: range[:current][:until], + timezone: params[:timezone], timezone_offset: params[:timezone_offset] }) end @@ -120,6 +121,7 @@ class Api::V2::Accounts::ReportsController < Api::V1::Accounts::BaseController common_params.merge({ since: range[:previous][:since], until: range[:previous][:until], + timezone: params[:timezone], timezone_offset: params[:timezone_offset] }) end @@ -129,6 +131,7 @@ class Api::V2::Accounts::ReportsController < Api::V1::Accounts::BaseController metric: params[:metric], since: params[:since], until: params[:until], + timezone: params[:timezone], timezone_offset: params[:timezone_offset] }) end diff --git a/app/controllers/api/v2/accounts/summary_reports_controller.rb b/app/controllers/api/v2/accounts/summary_reports_controller.rb index 31cd6eece..a504d894a 100644 --- a/app/controllers/api/v2/accounts/summary_reports_controller.rb +++ b/app/controllers/api/v2/accounts/summary_reports_controller.rb @@ -36,6 +36,7 @@ class Api::V2::Accounts::SummaryReportsController < Api::V1::Accounts::BaseContr until: permitted_params[:until], business_hours: ActiveModel::Type::Boolean.new.cast(permitted_params[:business_hours]) } + @builder_params[:timezone] = permitted_params[:timezone] if permitted_params[:timezone].present? @builder_params[:timezone_offset] = permitted_params[:timezone_offset] if permitted_params[:timezone_offset].present? end @@ -46,7 +47,7 @@ class Api::V2::Accounts::SummaryReportsController < Api::V1::Accounts::BaseContr end def permitted_params - params.permit(:since, :until, :business_hours, :timezone_offset) + params.permit(:since, :until, :business_hours, :timezone, :timezone_offset) end def date_range_too_long? diff --git a/app/helpers/timezone_helper.rb b/app/helpers/timezone_helper.rb index b016cc9d9..d4a05f054 100644 --- a/app/helpers/timezone_helper.rb +++ b/app/helpers/timezone_helper.rb @@ -1,4 +1,10 @@ module TimezoneHelper + def timezone_name_from_params(timezone, offset) + return timezone if timezone.present? && ActiveSupport::TimeZone[timezone].present? + + timezone_name_from_offset(offset) + end + # 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 diff --git a/app/javascript/dashboard/api/reports.js b/app/javascript/dashboard/api/reports.js index 00f040f8e..7093ec0d0 100644 --- a/app/javascript/dashboard/api/reports.js +++ b/app/javascript/dashboard/api/reports.js @@ -2,6 +2,7 @@ import ApiClient from './ApiClient'; const getTimeOffset = () => -new Date().getTimezoneOffset() / 60; +const getTimeZone = () => Intl.DateTimeFormat().resolvedOptions().timeZone; class ReportsAPI extends ApiClient { constructor() { @@ -26,6 +27,7 @@ class ReportsAPI extends ApiClient { id, group_by: groupBy, business_hours: businessHours, + timezone: getTimeZone(), timezone_offset: getTimeOffset(), }, }); @@ -41,6 +43,7 @@ class ReportsAPI extends ApiClient { id, group_by: groupBy, business_hours: businessHours, + timezone: getTimeZone(), timezone_offset: getTimeOffset(), }, }); @@ -105,6 +108,8 @@ class ReportsAPI extends ApiClient { type: 'account', group_by: groupBy, business_hours: businessHours, + timezone: getTimeZone(), + timezone_offset: getTimeOffset(), }, }); } diff --git a/app/javascript/dashboard/api/specs/reports.spec.js b/app/javascript/dashboard/api/specs/reports.spec.js index e458633d0..de81729b9 100644 --- a/app/javascript/dashboard/api/specs/reports.spec.js +++ b/app/javascript/dashboard/api/specs/reports.spec.js @@ -2,6 +2,8 @@ import reportsAPI from '../reports'; import ApiClient from '../ApiClient'; describe('#Reports API', () => { + const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone; + it('creates correct instance', () => { expect(reportsAPI).toBeInstanceOf(ApiClient); expect(reportsAPI.apiVersion).toBe('/api/v2'); @@ -46,6 +48,7 @@ describe('#Reports API', () => { since: 1621103400, until: 1621621800, type: 'account', + timezone, timezone_offset: -0, }, }); @@ -59,6 +62,7 @@ describe('#Reports API', () => { group_by: undefined, id: undefined, since: 1621103400, + timezone, timezone_offset: -0, type: 'account', until: 1621621800, @@ -140,6 +144,8 @@ describe('#Reports API', () => { type: 'account', group_by: 'date', business_hours: true, + timezone, + timezone_offset: -0, }, } ); diff --git a/app/javascript/dashboard/api/specs/summaryReports.spec.js b/app/javascript/dashboard/api/specs/summaryReports.spec.js new file mode 100644 index 000000000..27bc56fbc --- /dev/null +++ b/app/javascript/dashboard/api/specs/summaryReports.spec.js @@ -0,0 +1,47 @@ +import summaryReportsAPI from '../summaryReports'; +import ApiClient from '../ApiClient'; + +describe('#Summary Reports API', () => { + const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone; + + it('creates correct instance', () => { + expect(summaryReportsAPI).toBeInstanceOf(ApiClient); + expect(summaryReportsAPI.apiVersion).toBe('/api/v2'); + }); + + describe('API calls', () => { + const originalAxios = window.axios; + const axiosMock = { + get: vi.fn(() => Promise.resolve()), + }; + + beforeEach(() => { + window.axios = axiosMock; + }); + + afterEach(() => { + window.axios = originalAxios; + }); + + it('includes timezone data in summary report requests', () => { + summaryReportsAPI.getAgentReports({ + since: 1621103400, + until: 1621621800, + businessHours: true, + }); + + expect(axiosMock.get).toHaveBeenCalledWith( + '/api/v2/summary_reports/agent', + { + params: { + since: 1621103400, + until: 1621621800, + business_hours: true, + timezone, + timezone_offset: -0, + }, + } + ); + }); + }); +}); diff --git a/app/javascript/dashboard/api/summaryReports.js b/app/javascript/dashboard/api/summaryReports.js index 7c774c773..1773afbe5 100644 --- a/app/javascript/dashboard/api/summaryReports.js +++ b/app/javascript/dashboard/api/summaryReports.js @@ -2,6 +2,7 @@ import ApiClient from './ApiClient'; const getTimeOffset = () => -new Date().getTimezoneOffset() / 60; +const getTimeZone = () => Intl.DateTimeFormat().resolvedOptions().timeZone; class SummaryReportsAPI extends ApiClient { constructor() { @@ -14,6 +15,7 @@ class SummaryReportsAPI extends ApiClient { since, until, business_hours: businessHours, + timezone: getTimeZone(), timezone_offset: getTimeOffset(), }, }); @@ -25,6 +27,7 @@ class SummaryReportsAPI extends ApiClient { since, until, business_hours: businessHours, + timezone: getTimeZone(), timezone_offset: getTimeOffset(), }, }); @@ -36,6 +39,7 @@ class SummaryReportsAPI extends ApiClient { since, until, business_hours: businessHours, + timezone: getTimeZone(), timezone_offset: getTimeOffset(), }, }); @@ -47,6 +51,7 @@ class SummaryReportsAPI extends ApiClient { since, until, business_hours: businessHours, + timezone: getTimeZone(), timezone_offset: getTimeOffset(), }, }); diff --git a/app/services/reports/data_source.rb b/app/services/reports/data_source.rb index 61b4c6101..7c013295d 100644 --- a/app/services/reports/data_source.rb +++ b/app/services/reports/data_source.rb @@ -16,11 +16,13 @@ class Reports::DataSource rollup_enabled_for_account?(account) && !hourly_grouping?(context[:group_by]) && supported_dimension?(context[:dimension_type]) && - timezone_matches_account?(account, context[:timezone_offset]) && + timezone_matches_account?(account, context[:timezone], context[:timezone_offset]) && supported_metric?(context[:metric]) end - def timezone_matches_account?(account, timezone_offset) + def timezone_matches_account?(account, timezone, timezone_offset) + return normalized_timezone_identifier(timezone) == normalized_timezone_identifier(account.reporting_timezone) if timezone.present? + return false if timezone_offset.blank? offset_in_seconds = timezone_offset.to_f * 3600 @@ -49,6 +51,10 @@ class Reports::DataSource def supported_metric?(metric) metric.blank? || ReportingEvents::MetricRegistry.rollup_supported_metric?(metric) end + + def normalized_timezone_identifier(timezone) + ActiveSupport::TimeZone[timezone]&.tzinfo&.name + end end def initialize(**context) diff --git a/spec/builders/v2/reports/label_summary_builder_spec.rb b/spec/builders/v2/reports/label_summary_builder_spec.rb index 750f82241..645cc9761 100644 --- a/spec/builders/v2/reports/label_summary_builder_spec.rb +++ b/spec/builders/v2/reports/label_summary_builder_spec.rb @@ -33,7 +33,13 @@ RSpec.describe V2::Reports::LabelSummaryBuilder do it 'sets timezone from timezone_offset' do builder_with_offset = described_class.new(account: account, params: { timezone_offset: -8 }) - expect(builder_with_offset.instance_variable_get(:@timezone)).to eq('Pacific Time (US & Canada)') + expected_timezone = ActiveSupport::TimeZone.all.find { |zone| zone.now.utc_offset == -8.hours }.name + expect(builder_with_offset.instance_variable_get(:@timezone)).to eq(expected_timezone) + end + + it 'prefers timezone when it is provided' do + builder_with_timezone = described_class.new(account: account, params: { timezone: 'Asia/Kolkata', timezone_offset: -8 }) + expect(builder_with_timezone.instance_variable_get(:@timezone)).to eq('Asia/Kolkata') end it 'defaults timezone when timezone_offset is not provided' do diff --git a/spec/builders/v2/reports/timeseries/report_builder_spec.rb b/spec/builders/v2/reports/timeseries/report_builder_spec.rb index 4ecb2432e..29540a924 100644 --- a/spec/builders/v2/reports/timeseries/report_builder_spec.rb +++ b/spec/builders/v2/reports/timeseries/report_builder_spec.rb @@ -15,6 +15,7 @@ describe V2::Reports::Timeseries::ReportBuilder do { type: filter_type, business_hours: business_hours, + timezone: timezone, timezone_offset: timezone_offset, group_by: group_by, metric: metric, @@ -23,6 +24,7 @@ describe V2::Reports::Timeseries::ReportBuilder do id: filter_id } end + let(:timezone) { nil } let(:timezone_offset) { nil } let(:group_by) { 'day' } let(:metric) { 'avg_first_response_time' } @@ -166,6 +168,21 @@ describe V2::Reports::Timeseries::ReportBuilder do end end + context 'when timezone is provided' do + let(:timezone) { 'Asia/Kolkata' } + let(:timezone_offset) { '0' } + let(:group_by) { 'week' } + + it 'uses the timezone name instead of the offset for timestamps' do + expect(subject.timeseries).to eq( + [ + { count: 1, timestamp: (current_time - 1.week).in_time_zone('Asia/Kolkata').beginning_of_week(:sunday).to_i, value: 93.0 }, + { count: 2, timestamp: current_time.in_time_zone('Asia/Kolkata').beginning_of_week(:sunday).to_i, value: 90.0 } + ] + ) + end + end + context 'when weekly rollups are enabled' do let(:group_by) { 'week' } let(:timezone_offset) { '5.5' } @@ -300,6 +317,7 @@ describe V2::Reports::Timeseries::ReportBuilder do metric: 'resolutions_count', since: since_time.beginning_of_day.to_i.to_s, until: current_time.end_of_day.to_i.to_s, + timezone: timezone, timezone_offset: timezone_offset, group_by: group_by, id: user.id.to_s @@ -307,6 +325,7 @@ describe V2::Reports::Timeseries::ReportBuilder do end let(:group_by) { 'day' } let(:since_time) { current_time - 1.day } + let(:timezone) { nil } let(:timezone_offset) { nil } before do diff --git a/spec/services/reports/data_source_spec.rb b/spec/services/reports/data_source_spec.rb index 3f6e0a545..851bcd7a2 100644 --- a/spec/services/reports/data_source_spec.rb +++ b/spec/services/reports/data_source_spec.rb @@ -39,8 +39,24 @@ RSpec.describe Reports::DataSource do expect(described_class.for(**params)).to be_a(Reports::RawDataSource) end + it 'matches rollups using the requested timezone identifier' do + account.update!(reporting_timezone: 'Chennai') + + expect( + described_class.for(**params, timezone: 'Asia/Kolkata', timezone_offset: 0) + ).to be_a(Reports::RollupDataSource) + end + + it 'falls back to the raw adapter when the requested timezone differs from the account' do + account.update!(reporting_timezone: 'Chennai') + + expect( + described_class.for(**params, timezone: 'Asia/Colombo', timezone_offset: 5.5) + ).to be_a(Reports::RawDataSource) + end + it 'falls back to the raw adapter when the timezone offset does not match the account' do - expect(described_class.for(**params, timezone_offset: 0)).to be_a(Reports::RawDataSource) + expect(described_class.for(**params, timezone: nil, timezone_offset: 0)).to be_a(Reports::RawDataSource) end it 'falls back to the raw adapter for hourly groupings' do