From 45ef14b09adac2d6f8a6e3c90506370ed6965a8c Mon Sep 17 00:00:00 2001 From: Toon Willems Date: Fri, 20 Sep 2024 13:55:49 +0200 Subject: [PATCH] Add dynamic charge model & validator Add Charge model + extend properties service to support new charge model --- app/models/charge.rb | 6 + app/models/clickhouse/events_raw.rb | 15 +-- app/services/base_service.rb | 4 + .../aggregations/base_service.rb | 8 ++ .../aggregations/sum_service.rb | 6 +- .../prorated_aggregations/sum_service.rb | 4 +- .../unique_count_service.rb | 5 +- .../build_default_properties_service.rb | 5 + app/services/charges/charge_model_factory.rb | 2 + .../charges/charge_models/dynamic_service.rb | 24 ++++ .../charges/validators/dynamic_service.rb | 22 ++++ .../events/stores/clickhouse_store.rb | 19 +++- app/services/events/stores/postgres_store.rb | 4 + ...dd_precise_total_amount_cents_to_events.rb | 7 ++ schema.graphql | 1 + schema.json | 6 + spec/factories/charges.rb | 8 ++ spec/models/charge_spec.rb | 52 +++++++++ .../aggregations/sum_service_spec.rb | 105 ++++++++++++++++++ .../build_default_properties_service_spec.rb | 8 ++ .../charges/charge_model_factory_spec.rb | 6 + .../charge_models/dynamic_service_spec.rb | 39 +++++++ .../validators/dynamic_service_spec.rb | 36 ++++++ .../events/stores/clickhouse_store_spec.rb | 9 +- .../events/stores/postgres_store_spec.rb | 9 +- 25 files changed, 395 insertions(+), 15 deletions(-) create mode 100644 app/services/charges/charge_models/dynamic_service.rb create mode 100644 app/services/charges/validators/dynamic_service.rb create mode 100644 db/clickhouse_migrate/20240923124501_add_precise_total_amount_cents_to_events.rb create mode 100644 spec/services/charges/charge_models/dynamic_service_spec.rb create mode 100644 spec/services/charges/validators/dynamic_service_spec.rb diff --git a/app/models/charge.rb b/app/models/charge.rb index fd6d35e92d2f..7019fb631e52 100644 --- a/app/models/charge.rb +++ b/app/models/charge.rb @@ -24,6 +24,7 @@ class Charge < ApplicationRecord volume graduated_percentage custom + dynamic ].freeze REGROUPING_PAID_FEES_OPTIONS = %i[invoice].freeze @@ -37,6 +38,7 @@ class Charge < ApplicationRecord validate :validate_percentage, if: -> { percentage? } validate :validate_volume, if: -> { volume? } validate :validate_graduated_percentage, if: -> { graduated_percentage? } + validate :validate_dynamic, if: -> { dynamic? } validates :min_amount_cents, numericality: {greater_than_or_equal_to: 0}, allow_nil: true validates :charge_model, presence: true @@ -79,6 +81,10 @@ def validate_graduated_percentage validate_charge_model(Charges::Validators::GraduatedPercentageService) end + def validate_dynamic + validate_charge_model(Charges::Validators::DynamicService) + end + def validate_charge_model(validator) instance = validator.new(charge: self) return if instance.valid? diff --git a/app/models/clickhouse/events_raw.rb b/app/models/clickhouse/events_raw.rb index a9fe060ee80d..6243bb0a2b54 100644 --- a/app/models/clickhouse/events_raw.rb +++ b/app/models/clickhouse/events_raw.rb @@ -10,11 +10,12 @@ class EventsRaw < BaseRecord # # Table name: events_raw # -# code :string not null -# properties :string not null -# timestamp :datetime not null -# external_customer_id :string not null -# external_subscription_id :string not null -# organization_id :string not null -# transaction_id :string not null +# code :string not null +# precise_total_amount_cents :decimal(40, 15) +# properties :string not null +# timestamp :datetime not null +# external_customer_id :string not null +# external_subscription_id :string not null +# organization_id :string not null +# transaction_id :string not null # diff --git a/app/services/base_service.rb b/app/services/base_service.rb index d0be05288262..0d7d0e095448 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -170,6 +170,10 @@ def call_async(**args, &block) raise NotImplementedError end + protected + + attr_writer :result + private attr_reader :result, :source diff --git a/app/services/billable_metrics/aggregations/base_service.rb b/app/services/billable_metrics/aggregations/base_service.rb index 52db785e8cfd..d84698e29811 100644 --- a/app/services/billable_metrics/aggregations/base_service.rb +++ b/app/services/billable_metrics/aggregations/base_service.rb @@ -26,6 +26,10 @@ def aggregate(options: {}) else compute_aggregation(options:) end + if charge.dynamic? + compute_precise_total_amount_cents(options:) + end + result end def compute_aggregation(options: {}) @@ -36,6 +40,10 @@ def compute_grouped_by_aggregation(options: {}) raise NotImplementedError end + def compute_precise_total_amount_cents(options: {}) + raise NotImplementedError + end + def per_event_aggregation(exclude_event: false) Result.new.tap do |result| result.event_aggregation = compute_per_event_aggregation(exclude_event:) diff --git a/app/services/billable_metrics/aggregations/sum_service.rb b/app/services/billable_metrics/aggregations/sum_service.rb index a884866137c4..313b75afc4ad 100644 --- a/app/services/billable_metrics/aggregations/sum_service.rb +++ b/app/services/billable_metrics/aggregations/sum_service.rb @@ -4,7 +4,7 @@ module BillableMetrics module Aggregations class SumService < BillableMetrics::Aggregations::BaseService def initialize(...) - super(...) + super event_store.numeric_property = true event_store.aggregation_property = billable_metric.field_name @@ -64,6 +64,10 @@ def compute_grouped_by_aggregation(options: {}) result.service_failure!(code: 'aggregation_failure', message: e.message) end + def compute_precise_total_amount_cents(options: {}) + result.precise_total_amount_cents = event_store.sum_precise_total_amount_cents + end + # NOTE: Return cumulative sum of field_name based on the number of free units # (per_events or per_total_aggregation). def running_total(options) diff --git a/app/services/billable_metrics/prorated_aggregations/sum_service.rb b/app/services/billable_metrics/prorated_aggregations/sum_service.rb index 611da64afd97..92a32cf75224 100644 --- a/app/services/billable_metrics/prorated_aggregations/sum_service.rb +++ b/app/services/billable_metrics/prorated_aggregations/sum_service.rb @@ -4,9 +4,9 @@ module BillableMetrics module ProratedAggregations class SumService < BillableMetrics::ProratedAggregations::BaseService def initialize(**args) + super @base_aggregator = BillableMetrics::Aggregations::SumService.new(**args) - - super(**args) + @base_aggregator.result = result event_store.numeric_property = true event_store.aggregation_property = billable_metric.field_name diff --git a/app/services/billable_metrics/prorated_aggregations/unique_count_service.rb b/app/services/billable_metrics/prorated_aggregations/unique_count_service.rb index 4e87ac7af6e3..e00008406fc8 100644 --- a/app/services/billable_metrics/prorated_aggregations/unique_count_service.rb +++ b/app/services/billable_metrics/prorated_aggregations/unique_count_service.rb @@ -4,9 +4,10 @@ module BillableMetrics module ProratedAggregations class UniqueCountService < BillableMetrics::ProratedAggregations::BaseService def initialize(**args) - @base_aggregator = BillableMetrics::Aggregations::UniqueCountService.new(**args) + super - super(**args) + @base_aggregator = BillableMetrics::Aggregations::UniqueCountService.new(**args) + @base_aggregator.result = result event_store.aggregation_property = billable_metric.field_name event_store.use_from_boundary = !billable_metric.recurring diff --git a/app/services/charges/build_default_properties_service.rb b/app/services/charges/build_default_properties_service.rb index 718a94d8b5c2..7e51953939e4 100644 --- a/app/services/charges/build_default_properties_service.rb +++ b/app/services/charges/build_default_properties_service.rb @@ -15,6 +15,7 @@ def call when :percentage then default_percentage_properties when :volume then default_volume_properties when :graduated_percentage then default_graduated_percentage_properties + when :dynamic then default_dynamic_properties end end @@ -77,5 +78,9 @@ def default_graduated_percentage_properties ] } end + + def default_dynamic_properties + {} + end end end diff --git a/app/services/charges/charge_model_factory.rb b/app/services/charges/charge_model_factory.rb index 82ce3ba6978b..696426342d84 100644 --- a/app/services/charges/charge_model_factory.rb +++ b/app/services/charges/charge_model_factory.rb @@ -30,6 +30,8 @@ def self.charge_model_class(charge:, aggregation_result:, properties:) Charges::ChargeModels::VolumeService when :custom Charges::ChargeModels::CustomService + when :dynamic + Charges::ChargeModels::DynamicService else raise(NotImplementedError) end diff --git a/app/services/charges/charge_models/dynamic_service.rb b/app/services/charges/charge_models/dynamic_service.rb new file mode 100644 index 000000000000..eff2a26946fb --- /dev/null +++ b/app/services/charges/charge_models/dynamic_service.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Charges + module ChargeModels + class DynamicService < Charges::ChargeModels::BaseService + protected + + def compute_amount + total_units = aggregation_result.full_units_number || units + return 0 if total_units.zero? + + aggregation_result.precise_total_amount_cents + end + + def unit_amount + # eventhough `full_units_number` is not set by the SumService, we still keep this code as is, to be future proof + total_units = aggregation_result.full_units_number || units + return 0 if total_units.zero? + + compute_amount / total_units + end + end + end +end diff --git a/app/services/charges/validators/dynamic_service.rb b/app/services/charges/validators/dynamic_service.rb new file mode 100644 index 000000000000..8af739be2900 --- /dev/null +++ b/app/services/charges/validators/dynamic_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Charges + module Validators + class DynamicService < Charges::Validators::BaseService + def valid? + validate_billable_metric + + super + end + + private + + def validate_billable_metric + # Only sum aggregation is compatible with Dynamic Pricing for now + return if charge.billable_metric.sum_agg? + + add_error(field: :billable_metric, error_code: 'invalid_billable_metric_value') + end + end + end +end diff --git a/app/services/events/stores/clickhouse_store.rb b/app/services/events/stores/clickhouse_store.rb index 2da230c4c3d2..3dab1d36aa1b 100644 --- a/app/services/events/stores/clickhouse_store.rb +++ b/app/services/events/stores/clickhouse_store.rb @@ -4,7 +4,9 @@ module Events module Stores class ClickhouseStore < BaseStore DECIMAL_SCALE = 26 - DEDUPLICATION_GROUP = "events_raw.transaction_id, events_raw.properties, events_raw.timestamp" + + DEDUPLICATION_GROUP = 'events_raw.transaction_id, events_raw.properties, events_raw.timestamp' + PRECISE_TOTAL_AMOUNT_DEDUPLICATION_GROUP = 'events_raw.transaction_id, events_raw.precise_total_amount_cents, events_raw.timestamp' # NOTE: keeps in mind that events could contains duplicated transaction_id # and should be deduplicated depending on the aggregation logic @@ -264,6 +266,21 @@ def grouped_last prepare_grouped_result(::Clickhouse::EventsRaw.connection.select_all(sql).rows) end + def sum_precise_total_amount_cents + cte_sql = events.group(PRECISE_TOTAL_AMOUNT_DEDUPLICATION_GROUP) + .select(Arel.sql("precise_total_amount_cents as property")) + .to_sql + + sql = <<-SQL + with events as (#{cte_sql}) + + select sum(events.property) + from events + SQL + + ::Clickhouse::EventsRaw.connection.select_value(sql) + end + def sum cte_sql = events.group(DEDUPLICATION_GROUP) .select(Arel.sql("#{sanitized_numeric_property} AS property")) diff --git a/app/services/events/stores/postgres_store.rb b/app/services/events/stores/postgres_store.rb index e0328d7ac023..33578d4b25fd 100644 --- a/app/services/events/stores/postgres_store.rb +++ b/app/services/events/stores/postgres_store.rb @@ -188,6 +188,10 @@ def grouped_last prepare_grouped_result(Event.connection.select_all(sql).rows) end + def sum_precise_total_amount_cents + events.sum(:precise_total_amount_cents) + end + def sum events.sum("(#{sanitized_property_name})::numeric") end diff --git a/db/clickhouse_migrate/20240923124501_add_precise_total_amount_cents_to_events.rb b/db/clickhouse_migrate/20240923124501_add_precise_total_amount_cents_to_events.rb new file mode 100644 index 000000000000..cb683bbf7ad1 --- /dev/null +++ b/db/clickhouse_migrate/20240923124501_add_precise_total_amount_cents_to_events.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddPreciseTotalAmountCentsToEvents < ActiveRecord::Migration[7.1] + def change + add_column :events_raw, :precise_total_amount_cents, :decimal, precision: 40, scale: 15 + end +end diff --git a/schema.graphql b/schema.graphql index 97f37630c98f..e8122a7bebdd 100644 --- a/schema.graphql +++ b/schema.graphql @@ -351,6 +351,7 @@ input ChargeInput { enum ChargeModelEnum { custom + dynamic graduated graduated_percentage package diff --git a/schema.json b/schema.json index 65803ce93a0f..50f91b76ba45 100644 --- a/schema.json +++ b/schema.json @@ -3268,6 +3268,12 @@ "description": null, "isDeprecated": false, "deprecationReason": null + }, + { + "name": "dynamic", + "description": null, + "isDeprecated": false, + "deprecationReason": null } ] }, diff --git a/spec/factories/charges.rb b/spec/factories/charges.rb index 3de969a53a61..52a02512d4ed 100644 --- a/spec/factories/charges.rb +++ b/spec/factories/charges.rb @@ -53,6 +53,14 @@ end end + factory :dynamic_charge do + charge_model { 'dynamic' } + billable_metric { create(:sum_billable_metric) } + properties do + {} + end + end + factory :graduated_percentage_charge do charge_model { 'graduated_percentage' } properties do diff --git a/spec/models/charge_spec.rb b/spec/models/charge_spec.rb index 2c0691a666d0..bae9da8f5061 100644 --- a/spec/models/charge_spec.rb +++ b/spec/models/charge_spec.rb @@ -297,6 +297,58 @@ end end + describe '#validate_dynamic' do + subject(:charge) { build(:dynamic_charge) } + + let(:validation_service) { instance_double(Charges::Validators::DynamicService) } + + let(:service_response) do + BaseService::Result.new.validation_failure!( + errors: { + billable_metric: ['invalid_billable_metric_value'] + } + ) + end + + it 'delegates to a validation service' do + allow(Charges::Validators::DynamicService).to receive(:new) + .and_return(validation_service) + allow(validation_service).to receive(:valid?) + .and_return(false) + allow(validation_service).to receive(:result) + .and_return(service_response) + + aggregate_failures do + expect(charge).not_to be_valid + expect(charge.errors.messages.keys).to include(:properties) + expect(charge.errors.messages[:properties]).to include('invalid_billable_metric_value') + + expect(Charges::Validators::DynamicService).to have_received(:new).with(charge:) + expect(validation_service).to have_received(:valid?) + expect(validation_service).to have_received(:result) + end + end + + context 'when charge model is not dynamic' do + subject(:charge) { build(:standard_charge) } + + it 'does not apply the validation' do + allow(Charges::Validators::DynamicService).to receive(:new) + .and_return(validation_service) + allow(validation_service).to receive(:valid?) + .and_return(false) + allow(validation_service).to receive(:result) + .and_return(service_response) + + charge.valid? + + expect(Charges::Validators::DynamicService).not_to have_received(:new) + expect(validation_service).not_to have_received(:valid?) + expect(validation_service).not_to have_received(:result) + end + end + end + describe '#validate_graduated_percentage' do subject(:charge) do build(:graduated_percentage_charge, properties: charge_properties) diff --git a/spec/services/billable_metrics/aggregations/sum_service_spec.rb b/spec/services/billable_metrics/aggregations/sum_service_spec.rb index 5997c5320155..2cbdc57e6993 100644 --- a/spec/services/billable_metrics/aggregations/sum_service_spec.rb +++ b/spec/services/billable_metrics/aggregations/sum_service_spec.rb @@ -150,6 +150,111 @@ end end + context "when charge is dynamic" do + let(:charge) { create(:dynamic_charge, billable_metric:) } + + it "computes the precise_total_amount_cents" do + result = sum_service.aggregate(options:) + expect(result.precise_total_amount_cents).to be_zero + end + + context 'with events that specify a precise_total_amount_cents' do + let(:old_events) do + create_list( + :event, + 2, + organization_id: organization.id, + code: billable_metric.code, + customer:, + subscription:, + timestamp: subscription.started_at + 3.months, + properties: { + total_count: 2.5 + }, + precise_total_amount_cents: 12 + ) + end + let(:latest_events) do + create_list( + :event, + 4, + organization_id: organization.id, + code: billable_metric.code, + customer:, + subscription:, + timestamp: to_datetime - 1.day, + properties: { + total_count: 12 + }, + precise_total_amount_cents: 10 + ) + end + + it "computes the precise_total_amount_cents" do + result = sum_service.aggregate(options:) + expect(result.aggregation).to eq(4 * 12) + expect(result.precise_total_amount_cents).to eq(4 * 10) + end + end + + context 'when filters are given' do + let(:matching_filters) { {region: ['europe']} } + + before do + create( + :event, + organization_id: organization.id, + code: billable_metric.code, + customer:, + subscription:, + timestamp: to_datetime - 1.day, + properties: { + total_count: 12, + region: 'europe' + }, + precise_total_amount_cents: 5 + ) + + create( + :event, + organization_id: organization.id, + code: billable_metric.code, + customer:, + subscription:, + timestamp: to_datetime - 1.day, + properties: { + total_count: 8, + region: 'europe' + }, + precise_total_amount_cents: 7 + ) + + create( + :event, + organization_id: organization.id, + code: billable_metric.code, + customer:, + subscription:, + timestamp: to_datetime - 1.day, + properties: { + total_count: 12, + region: 'africa' + }, + precise_total_amount_cents: 9 + ) + end + + it 'aggregates the events matching the filter' do + result = sum_service.aggregate(options:) + + expect(result.aggregation).to eq(20) + expect(result.count).to eq(2) + expect(result.precise_total_amount_cents).to eq(12) + expect(result.options).to eq({running_total: [12, 20]}) + end + end + end + context 'when events are out of bounds' do let(:latest_events) do create_list( diff --git a/spec/services/charges/build_default_properties_service_spec.rb b/spec/services/charges/build_default_properties_service_spec.rb index 13118c2f2390..f53456347e47 100644 --- a/spec/services/charges/build_default_properties_service_spec.rb +++ b/spec/services/charges/build_default_properties_service_spec.rb @@ -93,5 +93,13 @@ ) end end + + context 'when dynamic charge model' do + let(:charge_model) { :dynamic } + + it 'returns dynamic default properties' do + expect(service.call).to eq({}) + end + end end end diff --git a/spec/services/charges/charge_model_factory_spec.rb b/spec/services/charges/charge_model_factory_spec.rb index b59d6d2b36da..1710e0feeb9b 100644 --- a/spec/services/charges/charge_model_factory_spec.rb +++ b/spec/services/charges/charge_model_factory_spec.rb @@ -59,6 +59,12 @@ it { expect(result).to be_a(Charges::ChargeModels::VolumeService) } end + context 'with dynamic charge model' do + let(:charge) { build(:dynamic_charge) } + + it { expect(result).to be_a(Charges::ChargeModels::DynamicService) } + end + context 'with custom charge model' do let(:charge) { build(:custom_charge) } diff --git a/spec/services/charges/charge_models/dynamic_service_spec.rb b/spec/services/charges/charge_models/dynamic_service_spec.rb new file mode 100644 index 000000000000..1b734066091d --- /dev/null +++ b/spec/services/charges/charge_models/dynamic_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Charges::ChargeModels::DynamicService, type: :service do + subject(:apply_dynamic_service) do + described_class.apply( + charge:, + aggregation_result:, + properties: charge.properties + ) + end + + before do + aggregation_result.aggregation = aggregation + aggregation_result.precise_total_amount_cents = precise_total_amount_cents + end + + let(:aggregation_result) { BaseService::Result.new } + + let(:charge) { create(:dynamic_charge) } + + let(:aggregation) { 20 } + let(:precise_total_amount_cents) { 40.2.to_d } + + it 'applies the model to the values' do + expect(apply_dynamic_service.amount).to eq(40.2) + expect(apply_dynamic_service.unit_amount).to eq(2.01) + end + + context 'when aggregation is zero' do + let(:aggregation) { 0 } + + it 'applies the model to the values' do + expect(apply_dynamic_service.amount).to eq(0) + expect(apply_dynamic_service.unit_amount).to eq(0) + end + end +end diff --git a/spec/services/charges/validators/dynamic_service_spec.rb b/spec/services/charges/validators/dynamic_service_spec.rb new file mode 100644 index 000000000000..c075da9e19df --- /dev/null +++ b/spec/services/charges/validators/dynamic_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Charges::Validators::DynamicService, type: :service do + subject(:dynamic_service) { described_class.new(charge:) } + + let(:charge) { build(:dynamic_charge, billable_metric:) } + + context "with sum aggregation metric" do + let(:billable_metric) { create(:sum_billable_metric) } + + describe '.validate' do + it 'is valiid' do + aggregate_failures do + expect(dynamic_service).to be_valid + end + end + end + end + + context "with other aggregation metric" do + let(:billable_metric) { create(:latest_billable_metric) } + + describe '.validate' do + it 'is invalid' do + aggregate_failures do + expect(dynamic_service).not_to be_valid + expect(dynamic_service.result.error).to be_a(BaseService::ValidationFailure) + expect(dynamic_service.result.error.messages.keys).to include(:billable_metric) + expect(dynamic_service.result.error.messages[:billable_metric]).to include('invalid_billable_metric_value') + end + end + end + end +end diff --git a/spec/services/events/stores/clickhouse_store_spec.rb b/spec/services/events/stores/clickhouse_store_spec.rb index 1bdd5cb45fdb..9378f1d60f13 100644 --- a/spec/services/events/stores/clickhouse_store_spec.rb +++ b/spec/services/events/stores/clickhouse_store_spec.rb @@ -66,7 +66,8 @@ external_customer_id: customer.external_id, code:, timestamp: boundaries[:from_datetime] + (i + 1).days, - properties: + properties:, + precise_total_amount_cents: i + 1 ) end @@ -168,6 +169,12 @@ end end + describe '#sum_precise_total_amount_cents' do + it 'returns the sum of precise_total_amount_cent values' do + expect(event_store.sum_precise_total_amount_cents).to eq(15) + end + end + describe '#active_unique_property?' do before { event_store.aggregation_property = billable_metric.field_name } diff --git a/spec/services/events/stores/postgres_store_spec.rb b/spec/services/events/stores/postgres_store_spec.rb index 7b1363575890..334fc4d2d392 100644 --- a/spec/services/events/stores/postgres_store_spec.rb +++ b/spec/services/events/stores/postgres_store_spec.rb @@ -52,7 +52,8 @@ timestamp: boundaries[:from_datetime] + (i + 1).days, properties: { billable_metric.field_name => i + 1 - } + }, + precise_total_amount_cents: i + 1 ) if i.even? @@ -747,6 +748,12 @@ end end + describe '#sum_precise_total_amount_cents' do + it 'returns the sum of precise_total_amount_cent values' do + expect(event_store.sum_precise_total_amount_cents).to eq(15) + end + end + describe '#sum' do it 'returns the sum of event values' do event_store.aggregation_property = billable_metric.field_name