From 340e9e6b41cef6a09b05136e8205d99a26d90a5e Mon Sep 17 00:00:00 2001 From: Julien Bourdeau Date: Thu, 4 Jul 2024 17:59:30 +0200 Subject: [PATCH] feat(charges): Add `regroup_paid_fees` to Charge (#2232) ## Context When your charge is pay in advance but not invoiceable, the fees are created but not attached to any invoice. This adds an options so at the end of the period, you get a new invoice showing all PAID fees. unpaid fees are ignored. See also https://github.com/getlago/lago-api/pull/2171 ## Description We're dropping the idea of `invoice_strategy` because it turned out to be a bad idea. Instead we're adding `regroup_paid_fees` attribute that takes only one value today `invoice`. --- app/controllers/api/v1/plans_controller.rb | 1 + app/graphql/types/charges/object.rb | 1 + .../types/charges/regroup_paid_fees_enum.rb | 11 ++ app/models/charge.rb | 12 ++ app/serializers/v1/charge_serializer.rb | 1 + .../invoices/advance_charges_service.rb | 8 +- app/services/plans/create_service.rb | 1 + app/services/plans/update_service.rb | 1 + config/locales/en.yml | 1 + ...update_properties_on_percentage_charges.rb | 1 - ...1_add_rate_to_percentage_amount_details.rb | 6 +- ...add_custom_aggregation_to_organizations.rb | 4 + ...081109_add_regroup_paid_fees_to_charges.rb | 5 + db/schema.rb | 3 +- lib/tasks/charges.rake | 14 --- schema.graphql | 5 + schema.json | 31 +++++ spec/factories/charges.rb | 6 + spec/graphql/types/charges/object_spec.rb | 1 + spec/models/charge_spec.rb | 30 +++++ spec/requests/api/v1/plans_controller_spec.rb | 108 ++++++++++++++++-- .../invoices/advance_charges_service_spec.rb | 4 +- spec/services/plans/create_service_spec.rb | 2 + 23 files changed, 226 insertions(+), 31 deletions(-) create mode 100644 app/graphql/types/charges/regroup_paid_fees_enum.rb create mode 100644 db/migrate/20240702081109_add_regroup_paid_fees_to_charges.rb diff --git a/app/controllers/api/v1/plans_controller.rb b/app/controllers/api/v1/plans_controller.rb index 48c0b01f840..8027bc28800 100644 --- a/app/controllers/api/v1/plans_controller.rb +++ b/app/controllers/api/v1/plans_controller.rb @@ -105,6 +105,7 @@ def input_params :pay_in_advance, :prorated, :invoiceable, + :regroup_paid_fees, :min_amount_cents, { properties: {} diff --git a/app/graphql/types/charges/object.rb b/app/graphql/types/charges/object.rb index 2ae902c3ec9..64bb25a142e 100644 --- a/app/graphql/types/charges/object.rb +++ b/app/graphql/types/charges/object.rb @@ -15,6 +15,7 @@ class Object < Types::BaseObject field :pay_in_advance, Boolean, null: false field :properties, Types::Charges::Properties, null: true field :prorated, Boolean, null: false + field :regroup_paid_fees, Types::Charges::RegroupPaidFeesEnum, null: true field :filters, [Types::ChargeFilters::Object], null: true diff --git a/app/graphql/types/charges/regroup_paid_fees_enum.rb b/app/graphql/types/charges/regroup_paid_fees_enum.rb new file mode 100644 index 00000000000..2f444bb50a9 --- /dev/null +++ b/app/graphql/types/charges/regroup_paid_fees_enum.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Types + module Charges + class RegroupPaidFeesEnum < Types::BaseEnum + Charge::REGROUPING_PAID_FEES_OPTIONS.each do |type| + value type + end + end + end +end diff --git a/app/models/charge.rb b/app/models/charge.rb index 3a728c202b6..65fc163ae39 100644 --- a/app/models/charge.rb +++ b/app/models/charge.rb @@ -27,7 +27,10 @@ class Charge < ApplicationRecord custom ].freeze + REGROUPING_PAID_FEES_OPTIONS = %i[invoice].freeze + enum charge_model: CHARGE_MODELS + enum regroup_paid_fees: REGROUPING_PAID_FEES_OPTIONS validate :validate_amount, if: -> { standard? && group_properties.empty? } validate :validate_graduated, if: -> { graduated? && group_properties.empty? } @@ -40,6 +43,7 @@ class Charge < ApplicationRecord validates :charge_model, presence: true validate :validate_pay_in_advance + validate :validate_regroup_paid_fees validate :validate_prorated validate :validate_min_amount_cents validate :validate_uniqueness_group_properties @@ -98,6 +102,14 @@ def validate_pay_in_advance end end + # NOTE: regroup_paid_fees only works with pay_in_advance and non-invoiceable charges + def validate_regroup_paid_fees + return if regroup_paid_fees.nil? + return if pay_in_advance? && !invoiceable? + + errors.add(:regroup_paid_fees, :only_compatible_with_pay_in_advance_and_non_invoiceable) + end + def validate_min_amount_cents return unless pay_in_advance? && min_amount_cents.positive? diff --git a/app/serializers/v1/charge_serializer.rb b/app/serializers/v1/charge_serializer.rb index 46e78b03d19..4322e188299 100644 --- a/app/serializers/v1/charge_serializer.rb +++ b/app/serializers/v1/charge_serializer.rb @@ -11,6 +11,7 @@ def serialize created_at: model.created_at.iso8601, charge_model: model.charge_model, invoiceable: model.invoiceable, + regroup_paid_fees: model.regroup_paid_fees, pay_in_advance: model.pay_in_advance, prorated: model.prorated, min_amount_cents: model.min_amount_cents, diff --git a/app/services/invoices/advance_charges_service.rb b/app/services/invoices/advance_charges_service.rb index b15c1bd38cf..ddadefb527c 100644 --- a/app/services/invoices/advance_charges_service.rb +++ b/app/services/invoices/advance_charges_service.rb @@ -14,8 +14,7 @@ def initialize(subscriptions:, billing_at:) end def call - # TODO: check if related charges have correct `invoicing_strategy` - # return result unless has_charges_to_invoice? + return result unless has_charges_with_statement? return result if subscriptions.empty? @@ -40,6 +39,11 @@ def call attr_accessor :subscriptions, :billing_at, :customer, :organization, :currency + def has_charges_with_statement? + plan_ids = subscriptions.pluck(:plan_id) + Charge.where(plan_id: plan_ids, pay_in_advance: true, invoiceable: false, regroup_paid_fees: :invoice).any? + end + def create_group_invoice invoice = nil diff --git a/app/services/plans/create_service.rb b/app/services/plans/create_service.rb index bd1bee3cde5..723e1f756da 100644 --- a/app/services/plans/create_service.rb +++ b/app/services/plans/create_service.rb @@ -103,6 +103,7 @@ def create_charge(plan, args) if License.premium? charge.invoiceable = args[:invoiceable] unless args[:invoiceable].nil? + charge.regroup_paid_fees = args[:regroup_paid_fees] if args.has_key?(:regroup_paid_fees) charge.min_amount_cents = args[:min_amount_cents] || 0 end diff --git a/app/services/plans/update_service.rb b/app/services/plans/update_service.rb index 95a64014aea..ecf7234bb7d 100644 --- a/app/services/plans/update_service.rb +++ b/app/services/plans/update_service.rb @@ -99,6 +99,7 @@ def create_charge(plan, params) if License.premium? charge.invoiceable = params[:invoiceable] unless params[:invoiceable].nil? + charge.regroup_paid_fees = params[:regroup_paid_fees] if params.has_key?(:regroup_paid_fees) charge.min_amount_cents = params[:min_amount_cents] || 0 end diff --git a/config/locales/en.yml b/config/locales/en.yml index c8dbecd2c72..8572ca12d46 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -33,6 +33,7 @@ en: missing_volume_ranges: missing_volume_ranges not_compatible_with_aggregation_type: not_compatible_with_aggregation_type not_compatible_with_pay_in_advance: not_compatible_with_pay_in_advance + only_compatible_with_pay_in_advance_and_non_invoiceable: only_compatible_with_pay_in_advance_and_non_invoiceable required: relation_must_exist taken: value_already_exist too_long: value_is_too_long diff --git a/db/migrate/20220816120137_update_properties_on_percentage_charges.rb b/db/migrate/20220816120137_update_properties_on_percentage_charges.rb index 386a8d10fb2..084837454f1 100644 --- a/db/migrate/20220816120137_update_properties_on_percentage_charges.rb +++ b/db/migrate/20220816120137_update_properties_on_percentage_charges.rb @@ -3,6 +3,5 @@ class UpdatePropertiesOnPercentageCharges < ActiveRecord::Migration[7.0] def change LagoApi::Application.load_tasks - Rake::Task['charges:update_properties_for_free_units'].invoke end end diff --git a/db/migrate/20231218170631_add_rate_to_percentage_amount_details.rb b/db/migrate/20231218170631_add_rate_to_percentage_amount_details.rb index a6521228402..5c0f7ab693e 100644 --- a/db/migrate/20231218170631_add_rate_to_percentage_amount_details.rb +++ b/db/migrate/20231218170631_add_rate_to_percentage_amount_details.rb @@ -2,7 +2,11 @@ class AddRateToPercentageAmountDetails < ActiveRecord::Migration[7.0] class Fee < ApplicationRecord - belongs_to :charge, -> { with_discarded }, optional: true + belongs_to :charge, optional: true + end + + class Charge < ApplicationRecord + enum charge_model: %i[percentage graduated_percentage].freeze end def up diff --git a/db/migrate/20240429141108_add_custom_aggregation_to_organizations.rb b/db/migrate/20240429141108_add_custom_aggregation_to_organizations.rb index 5b83a02adb4..32056c0e6f0 100644 --- a/db/migrate/20240429141108_add_custom_aggregation_to_organizations.rb +++ b/db/migrate/20240429141108_add_custom_aggregation_to_organizations.rb @@ -1,5 +1,9 @@ # frozen_string_literal: true +class Charge + attribute :regroup_paid_fees, :integer, default: nil +end + class AddCustomAggregationToOrganizations < ActiveRecord::Migration[7.0] def change add_column :organizations, :custom_aggregation, :boolean, default: false diff --git a/db/migrate/20240702081109_add_regroup_paid_fees_to_charges.rb b/db/migrate/20240702081109_add_regroup_paid_fees_to_charges.rb new file mode 100644 index 00000000000..d860a51ae2f --- /dev/null +++ b/db/migrate/20240702081109_add_regroup_paid_fees_to_charges.rb @@ -0,0 +1,5 @@ +class AddRegroupPaidFeesToCharges < ActiveRecord::Migration[7.1] + def change + add_column :charges, :regroup_paid_fees, :integer, default: nil + end +end diff --git a/db/schema.rb b/db/schema.rb index 212a083a66c..5a93abe270d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_07_01_083355) do +ActiveRecord::Schema[7.1].define(version: 2024_07_02_081109) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -225,6 +225,7 @@ t.boolean "invoiceable", default: true, null: false t.boolean "prorated", default: false, null: false t.string "invoice_display_name" + t.integer "regroup_paid_fees" t.index ["billable_metric_id"], name: "index_charges_on_billable_metric_id" t.index ["deleted_at"], name: "index_charges_on_deleted_at" t.index ["plan_id"], name: "index_charges_on_plan_id" diff --git a/lib/tasks/charges.rake b/lib/tasks/charges.rake index 1a56916ea88..9c2738c179c 100644 --- a/lib/tasks/charges.rake +++ b/lib/tasks/charges.rake @@ -1,20 +1,6 @@ # frozen_string_literal: true namespace :charges do - desc 'Update Properties for Fixed Fee and Free Units' - task update_properties_for_free_units: :environment do - # Notes: We consider that we don’t have any clients with a percentage charge - # created containing a fixed_amount. All existing charges have fixed_amount - # and fixed_amount_target with a null value. - - Charge.unscoped.percentage.where("properties -> 'fixed_amount_target' IS NOT NULL").find_each do |charge| - charge.properties.delete('fixed_amount_target') - charge.properties['free_units_per_events'] = nil - charge.properties['free_units_per_total_aggregation'] = nil - charge.save! - end - end - desc 'Set graduated properties to hash and rename volume ranges' task update_graduated_properties_to_hash: :environment do # Rename existing volume ranges from `ranges: []` to `volume_ranges: []` diff --git a/schema.graphql b/schema.graphql index 92bb3d277ca..747555956ee 100644 --- a/schema.graphql +++ b/schema.graphql @@ -257,6 +257,7 @@ type Charge { payInAdvance: Boolean! properties: Properties prorated: Boolean! + regroupPaidFees: RegroupPaidFeesEnum taxes: [Tax!] updatedAt: ISO8601DateTime! } @@ -5808,6 +5809,10 @@ input RegisterUserInput { password: String! } +enum RegroupPaidFeesEnum { + invoice +} + """ ResetPassword type """ diff --git a/schema.json b/schema.json index dbe6f6db06a..1aa12b327f3 100644 --- a/schema.json +++ b/schema.json @@ -2587,6 +2587,20 @@ ] }, + { + "name": "regroupPaidFees", + "description": null, + "type": { + "kind": "ENUM", + "name": "RegroupPaidFeesEnum", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null, + "args": [ + + ] + }, { "name": "taxes", "description": null, @@ -30030,6 +30044,23 @@ ], "enumValues": null }, + { + "kind": "ENUM", + "name": "RegroupPaidFeesEnum", + "description": null, + "interfaces": null, + "possibleTypes": null, + "fields": null, + "inputFields": null, + "enumValues": [ + { + "name": "invoice", + "description": null, + "isDeprecated": false, + "deprecationReason": null + } + ] + }, { "kind": "OBJECT", "name": "ResetPassword", diff --git a/spec/factories/charges.rb b/spec/factories/charges.rb index d1564ef6dcc..3de969a53a6 100644 --- a/spec/factories/charges.rb +++ b/spec/factories/charges.rb @@ -85,5 +85,11 @@ trait :pay_in_advance do pay_in_advance { true } end + + trait :regroup_paid_fees do + pay_in_advance { true } + invoiceable { false } + regroup_paid_fees { 'invoice' } + end end end diff --git a/spec/graphql/types/charges/object_spec.rb b/spec/graphql/types/charges/object_spec.rb index a3730e8e727..5f3dab271f3 100644 --- a/spec/graphql/types/charges/object_spec.rb +++ b/spec/graphql/types/charges/object_spec.rb @@ -9,6 +9,7 @@ it { is_expected.to have_field(:invoice_display_name).of_type('String') } it { is_expected.to have_field(:billable_metric).of_type('BillableMetric!') } it { is_expected.to have_field(:charge_model).of_type('ChargeModelEnum!') } + it { is_expected.to have_field(:regroup_paid_fees).of_type('RegroupPaidFeesEnum') } it { is_expected.to have_field(:invoiceable).of_type('Boolean!') } it { is_expected.to have_field(:min_amount_cents).of_type('BigInt!') } it { is_expected.to have_field(:pay_in_advance).of_type('Boolean!') } diff --git a/spec/models/charge_spec.rb b/spec/models/charge_spec.rb index aae4956c706..7a59e7a1d09 100644 --- a/spec/models/charge_spec.rb +++ b/spec/models/charge_spec.rb @@ -424,6 +424,36 @@ end end + describe '#validate_regroup_paid_fees' do + context 'when regroup_paid_fees is nil' do + it 'does not return an error when' do + expect(build(:standard_charge, pay_in_advance: true, invoiceable: true, regroup_paid_fees: nil)).to be_valid + expect(build(:standard_charge, pay_in_advance: true, invoiceable: false, regroup_paid_fees: nil)).to be_valid + expect(build(:standard_charge, pay_in_advance: false, invoiceable: true, regroup_paid_fees: nil)).to be_valid + expect(build(:standard_charge, pay_in_advance: false, invoiceable: false, regroup_paid_fees: nil)).to be_valid + end + end + + context 'when regroup_paid_fees is `invoice`' do + it 'requires charge to be pay_in_advance and non invoiceable' do + expect(build(:standard_charge, pay_in_advance: true, invoiceable: false, regroup_paid_fees: 'invoice')).to be_valid + + [ + {pay_in_advance: true, invoiceable: true}, + {pay_in_advance: false, invoiceable: true}, + {pay_in_advance: false, invoiceable: false} + ].each do |params| + charge = build(:standard_charge, regroup_paid_fees: 'invoice', **params) + + aggregate_failures do + expect(charge).not_to be_valid + expect(charge.errors.messages[:regroup_paid_fees]).to include('only_compatible_with_pay_in_advance_and_non_invoiceable') + end + end + end + end + end + describe '#validate_min_amount_cents' do it 'does not return an error' do expect(build(:standard_charge)).to be_valid diff --git a/spec/requests/api/v1/plans_controller_spec.rb b/spec/requests/api/v1/plans_controller_spec.rb index b34928ec632..aab6df55968 100644 --- a/spec/requests/api/v1/plans_controller_spec.rb +++ b/spec/requests/api/v1/plans_controller_spec.rb @@ -28,6 +28,9 @@ { billable_metric_id: billable_metric.id, charge_model: 'standard', + pay_in_advance: true, + invoiceable: false, + regroup_paid_fees: 'invoice', properties: { amount: '0.22' }, @@ -51,6 +54,30 @@ expect(json[:plan][:charges].first[:lago_id]).to be_present end + context 'when license is not premium' do + it 'ignores premium fields' do + post_with_token(organization, '/api/v1/plans', {plan: create_params}) + + expect(response).to have_http_status(:success) + charge = json[:plan][:charges].first + expect(charge[:invoiceable]).to be true + expect(charge[:regroup_paid_fees]).to be_nil + end + end + + context 'when license is premium' do + around { |test| lago_premium!(&test) } + + it 'updates premium fields' do + post_with_token(organization, '/api/v1/plans', {plan: create_params}) + + expect(response).to have_http_status(:success) + charge = json[:plan][:charges].first + expect(charge[:invoiceable]).to be false + expect(charge[:regroup_paid_fees]).to eq 'invoice' + end + end + context 'with minimum commitment' do context 'when license is premium' do around { |test| lago_premium!(&test) } @@ -242,18 +269,21 @@ amount_currency: 'EUR', trial_period: 1, pay_in_advance: false, - charges: [ - { - billable_metric_id: billable_metric.id, - charge_model: 'standard', - properties: { - amount: '0.22' - }, - tax_codes: - } - ] + charges: charges_params } end + let(:charges_params) do + [ + { + billable_metric_id: billable_metric.id, + charge_model: 'standard', + properties: { + amount: '0.22' + }, + tax_codes: + } + ] + end let(:minimum_commitment_params) do { @@ -301,6 +331,64 @@ end end + context 'when license is not premium' do + let(:charges_params) do + [ + { + billable_metric_id: billable_metric.id, + charge_model: 'standard', + properties: { + amount: '0.22' + }, + tax_codes:, + pay_in_advance: true, + invoiceable: false, + regroup_paid_fees: 'invoice' + } + ] + end + + it 'ignores premium fields' do + post_with_token(organization, '/api/v1/plans', {plan: update_params}) + + expect(response).to have_http_status(:success) + charge = json[:plan][:charges].first + expect(charge[:pay_in_advance]).to be true + expect(charge[:invoiceable]).to be true + expect(charge[:regroup_paid_fees]).to be_nil + end + end + + context 'when license is premium' do + let(:charges_params) do + [ + { + billable_metric_id: billable_metric.id, + charge_model: 'standard', + properties: { + amount: '0.22' + }, + tax_codes:, + pay_in_advance: true, + invoiceable: false, + regroup_paid_fees: 'invoice' + } + ] + end + + around { |test| lago_premium!(&test) } + + it 'updates premium fields' do + post_with_token(organization, '/api/v1/plans', {plan: update_params}) + + expect(response).to have_http_status(:success) + charge = json[:plan][:charges].first + expect(charge[:pay_in_advance]).to be true + expect(charge[:invoiceable]).to be false + expect(charge[:regroup_paid_fees]).to eq 'invoice' + end + end + context 'when plan has no minimum commitment' do context 'when request contains minimum commitment params' do before { update_params.merge!(minimum_commitment_params) } diff --git a/spec/services/invoices/advance_charges_service_spec.rb b/spec/services/invoices/advance_charges_service_spec.rb index ae26c67c8a4..4f20be819f2 100644 --- a/spec/services/invoices/advance_charges_service_spec.rb +++ b/spec/services/invoices/advance_charges_service_spec.rb @@ -48,7 +48,7 @@ def fee_boundaries context 'with existing standalone fees' do before do tax - charge = create(:standard_charge, plan: subscription.plan, charge_model: 'standard') + charge = create(:standard_charge, :regroup_paid_fees, plan: subscription.plan) create_list(:charge_fee, 3, :succeeded, invoice_id: nil, subscription:, charge:, amount_cents: 100, properties: fee_boundaries) create_list(:charge_fee, 2, :failed, invoice_id: nil, subscription:, charge:, amount_cents: 100, properties: fee_boundaries) end @@ -97,7 +97,7 @@ def fee_boundaries context 'with integration requiring sync' do before do tax - charge = create(:standard_charge, plan: subscription.plan, charge_model: 'standard') + charge = create(:standard_charge, :regroup_paid_fees, plan: subscription.plan) create(:charge_fee, :succeeded, invoice_id: nil, subscription:, charge:, amount_cents: 100, properties: fee_boundaries) allow_any_instance_of(Invoice).to receive(:should_sync_invoice?).and_return(true) # rubocop:disable RSpec/AnyInstance diff --git a/spec/services/plans/create_service_spec.rb b/spec/services/plans/create_service_spec.rb index 7fe0f7b7152..60f60231724 100644 --- a/spec/services/plans/create_service_spec.rb +++ b/spec/services/plans/create_service_spec.rb @@ -196,6 +196,7 @@ charge_model: 'graduated_percentage', pay_in_advance: true, invoiceable: false, + regroup_paid_fees: 'invoice', properties: { graduated_percentage_ranges: [ { @@ -238,6 +239,7 @@ { pay_in_advance: true, invoiceable: false, + regroup_paid_fees: 'invoice', charge_model: 'graduated_percentage' } )