diff --git a/app/jobs/charges/update_job.rb b/app/jobs/charges/update_job.rb index 14158b3b96d..bbb3863f6ac 100644 --- a/app/jobs/charges/update_job.rb +++ b/app/jobs/charges/update_job.rb @@ -4,8 +4,8 @@ module Charges class UpdateJob < ApplicationJob queue_as 'default' - def perform(charge:, params:, cascade:) - Charges::UpdateService.call(charge:, params:, cascade:).raise_if_error! + def perform(charge:, params:, cascade_options:) + Charges::UpdateService.call(charge:, params:, cascade_options:).raise_if_error! end end end diff --git a/app/models/charge_filter.rb b/app/models/charge_filter.rb index 82a73ddcefe..a965d005fc2 100644 --- a/app/models/charge_filter.rb +++ b/app/models/charge_filter.rb @@ -30,6 +30,12 @@ def to_h end end + def to_h_with_discarded + @to_h_with_discarded ||= values.with_discarded.each_with_object({}) do |filter_value, result| + result[filter_value.billable_metric_filter.key] = filter_value.values + end + end + def to_h_with_all_values @to_h_with_all_values ||= values.each_with_object({}) do |filter_value, result| values = filter_value.values diff --git a/app/services/charge_filters/create_or_update_batch_service.rb b/app/services/charge_filters/create_or_update_batch_service.rb index 821f292e57b..8f2b4abe0f2 100644 --- a/app/services/charge_filters/create_or_update_batch_service.rb +++ b/app/services/charge_filters/create_or_update_batch_service.rb @@ -2,9 +2,16 @@ module ChargeFilters class CreateOrUpdateBatchService < BaseService - def initialize(charge:, filters_params:) + def initialize(charge:, filters_params:, cascade_options: {}) @charge = charge @filters_params = filters_params + @cascade_updates = cascade_options[:cascade] + @parent_filters_attributes = cascade_options[:parent_filters] || [] + @parent_filters = if parent_filters_attributes.blank? + ChargeFilter.none + else + ChargeFilter.with_discarded.where(id: parent_filters_attributes.map { |f| f['id'] }) + end super end @@ -29,6 +36,20 @@ def call f.to_h.sort == filter_param[:values].sort end + # Skip cascade update if properties are already touched + if cascade_updates && filter && parent_filters + parent_filter = parent_filters.find do |pf| + pf.to_h.sort == filter.to_h.sort + end + + if parent_filter.blank? || parent_filter_properties(parent_filter) != filter.properties + filter.touch # rubocop:disable Rails/SkipsModelValidations + result.filters << filter + + next + end + end + filter ||= charge.filters.new filter.invoice_display_name = filter_param[:invoice_display_name] @@ -57,7 +78,9 @@ def call end # NOTE: remove old filters that were not created or updated - charge.filters.where.not(id: result.filters.map(&:id)).find_each do + remove_query = charge.filters + remove_query = remove_query.where(id: inherited_filter_ids) if cascade_updates && parent_filters + remove_query.where.not(id: result.filters.map(&:id)).find_each do remove_filter(_1) end end @@ -67,15 +90,27 @@ def call private - attr_reader :charge, :filters_params + attr_reader :charge, :filters_params, :cascade_updates, :parent_filters, :parent_filters_attributes def filters @filters ||= charge.filters.includes(values: :billable_metric_filter) end + def parent_filter_properties(parent_filter) + match = parent_filters_attributes.find do |f| + f['id'] == parent_filter.id + end + + match['properties'] + end + def remove_all ActiveRecord::Base.transaction do - charge.filters.each { remove_filter(_1) } + if cascade_updates + charge.filters.where(id: inherited_filter_ids).find_each { remove_filter(_1) } + else + charge.filters.each { remove_filter(_1) } + end end end @@ -83,5 +118,25 @@ def remove_filter(filter) filter.values.each(&:discard!) filter.discard! end + + def inherited_filter_ids + return @inherited_filter_ids if defined? @inherited_filter_ids + + @inherited_filter_ids = [] + + return @inherited_filter_ids if parent_filters.blank? || !cascade_updates + + parent_filters.find_each do |pf| + value = pf.to_h_with_discarded.sort + + match = filters.find do |f| + value == f.to_h.sort + end + + @inherited_filter_ids << match.id if match + end + + @inherited_filter_ids + end end end diff --git a/app/services/charges/update_service.rb b/app/services/charges/update_service.rb index 07e7a401d42..385f6bca369 100644 --- a/app/services/charges/update_service.rb +++ b/app/services/charges/update_service.rb @@ -2,10 +2,11 @@ module Charges class UpdateService < BaseService - def initialize(charge:, params:, cascade: false) + def initialize(charge:, params:, cascade_options: {}) @charge = charge @params = params - @cascade = cascade + @cascade_options = cascade_options + @cascade = cascade_options[:cascade] super end @@ -18,30 +19,29 @@ def call charge.charge_model = params[:charge_model] unless plan.attached_to_subscriptions? charge.invoice_display_name = params[:invoice_display_name] unless cascade - properties = params.delete(:properties).presence || Charges::BuildDefaultPropertiesService.call( - params[:charge_model] - ) - - charge.update!( - properties: Charges::FilterChargeModelPropertiesService.call( - charge:, - properties: - ).properties - ) - - result.charge = charge + if !cascade || cascade_options[:equal_properties] + properties = params.delete(:properties).presence || Charges::BuildDefaultPropertiesService.call( + params[:charge_model] + ) + charge.properties = Charges::FilterChargeModelPropertiesService.call(charge:, properties:).properties + end - # In cascade mode it is allowed only to change properties - return result if cascade + charge.save! filters = params.delete(:filters) unless filters.nil? ChargeFilters::CreateOrUpdateBatchService.call( charge:, - filters_params: filters.map(&:with_indifferent_access) + filters_params: filters.map(&:with_indifferent_access), + cascade_options: ).raise_if_error! end + result.charge = charge + + # In cascade mode it is allowed only to change properties + return result if cascade + tax_codes = params.delete(:tax_codes) if tax_codes taxes_result = Charges::ApplyTaxesService.call(charge:, tax_codes:) @@ -69,7 +69,7 @@ def call private - attr_reader :charge, :params, :cascade + attr_reader :charge, :params, :cascade_options, :cascade delegate :plan, to: :charge end diff --git a/app/services/plans/update_service.rb b/app/services/plans/update_service.rb index 9d1fce0e0bd..db33af0dabe 100644 --- a/app/services/plans/update_service.rb +++ b/app/services/plans/update_service.rb @@ -126,8 +126,16 @@ def cascade_charge_update(charge, payload_charge) plan.children.includes(:charges).find_each do |p| child_charge = p.charges.find { |c| c.parent_id == charge.id } - if child_charge && charge.equal_properties?(child_charge) - Charges::UpdateJob.perform_later(charge: child_charge, params: payload_charge, cascade: true) + if child_charge + Charges::UpdateJob.perform_later( + charge: child_charge, + params: payload_charge, + cascade_options: { + cascade: true, + parent_filters: charge.filters.map(&:attributes), + equal_properties: charge.equal_properties?(child_charge) + } + ) end end end diff --git a/spec/jobs/charges/update_job_spec.rb b/spec/jobs/charges/update_job_spec.rb index e02d2541b3a..c69559bb581 100644 --- a/spec/jobs/charges/update_job_spec.rb +++ b/spec/jobs/charges/update_job_spec.rb @@ -6,7 +6,7 @@ let(:organization) { create(:organization) } let(:plan) { create(:plan, organization:) } let(:charge) { create(:standard_charge, plan:) } - let(:cascade) { true } + let(:cascade_options) { {} } let(:params) do { properties: {} @@ -14,11 +14,11 @@ end before do - allow(Charges::UpdateService).to receive(:call).with(charge:, params:, cascade:).and_return(BaseService::Result.new) + allow(Charges::UpdateService).to receive(:call).with(charge:, params:, cascade_options:).and_return(BaseService::Result.new) end it 'calls the service' do - described_class.perform_now(charge:, params:, cascade:) + described_class.perform_now(charge:, params:, cascade_options:) expect(Charges::UpdateService).to have_received(:call) end diff --git a/spec/models/charge_filter_spec.rb b/spec/models/charge_filter_spec.rb index 4ec56db86b7..44b0c90bdcb 100644 --- a/spec/models/charge_filter_spec.rb +++ b/spec/models/charge_filter_spec.rb @@ -375,6 +375,30 @@ end end + describe '#to_h_with_discarded' do + subject(:charge_filter) { create(:charge_filter) } + + let(:card) { create(:billable_metric_filter, key: 'card', values: %w[credit debit]) } + let(:scheme) { create(:billable_metric_filter, key: 'scheme', values: %w[visa mastercard]) } + let(:values) do + [ + create(:charge_filter_value, charge_filter:, values: ['credit'], billable_metric_filter: card).discard, + create(:charge_filter_value, charge_filter:, values: ['visa'], billable_metric_filter: scheme).discard + ] + end + + before { values } + + it 'returns the values as a hash' do + expect(charge_filter.to_h_with_discarded).to eq( + { + 'card' => ['credit'], + 'scheme' => ['visa'] + } + ) + end + end + describe '#to_h_with_all_values' do subject(:charge_filter) { build(:charge_filter, values:) } diff --git a/spec/services/charge_filters/create_or_update_batch_service_spec.rb b/spec/services/charge_filters/create_or_update_batch_service_spec.rb index 5261dfd0fa7..a8285b0da1e 100644 --- a/spec/services/charge_filters/create_or_update_batch_service_spec.rb +++ b/spec/services/charge_filters/create_or_update_batch_service_spec.rb @@ -3,10 +3,11 @@ require 'rails_helper' RSpec.describe ChargeFilters::CreateOrUpdateBatchService do - subject(:service) { described_class.call(charge:, filters_params:) } + subject(:service) { described_class.call(charge:, filters_params:, cascade_options:) } let(:charge) { create(:standard_charge) } let(:filters_params) { {} } + let(:cascade_options) { {} } let(:card_location_filter) do create( @@ -58,6 +59,51 @@ expect { service }.to change { filter.reload.discarded? }.to(true) .and change { filter_value.reload.discarded? }.to(true) end + + context 'with cascade_updates set to true and existing filters' do + let(:charge_parent) { create(:standard_charge) } + let(:filter_extra) { create(:charge_filter, charge:) } + let(:filter_parent) { create(:charge_filter, charge: charge_parent) } + let(:filter_value_extra) do + create( + :charge_filter_value, + charge_filter: filter_extra, + billable_metric_filter: card_location_filter, + values: [card_location_filter.values.second] + ) + end + let(:filter_value_parent) do + create( + :charge_filter_value, + charge_filter: filter_parent, + billable_metric_filter: card_location_filter, + values: [card_location_filter.values.first] + ) + end + let(:cascade_options) do + { + cascade: true, + parent_filters: charge_parent.filters.map(&:attributes) + } + end + + before do + filter_value_extra + filter_value_parent + end + + it 'discards all filters and the related values that are inherited from parent' do + expect { service }.to change { filter.reload.discarded? }.to(true) + .and change { filter_value.reload.discarded? }.to(true) + end + + it 'does not discard filters and the related values that are defined on child' do + service + + expect(filter_extra.reload.discarded?).to eq(false) + expect(filter_value_extra.reload.discarded?).to eq(false) + end + end end end @@ -200,5 +246,127 @@ expect(new_filter.values.pluck(:values).flatten).to match_array(%w[domestic visa mastercard]) end end + + context 'with cascading option' do + let(:charge_parent) { create(:standard_charge) } + let(:filter_extra) { create(:charge_filter, charge:) } + let(:filter_parent) { create(:charge_filter, properties: filter.properties, charge: charge_parent) } + let(:filter_value_extra) do + create( + :charge_filter_value, + charge_filter: filter_extra, + billable_metric_filter: card_location_filter, + values: [card_location_filter.values.second] + ) + end + let(:filter_values_parent) do + [ + create( + :charge_filter_value, + charge_filter: filter_parent, + billable_metric_filter: card_location_filter, + values: ['domestic'] + ), + create( + :charge_filter_value, + charge_filter: filter_parent, + billable_metric_filter: scheme_filter, + values: ['visa'] + ) + ] + end + let(:cascade_options) do + { + cascade: true, + parent_filters: charge_parent.filters.map(&:attributes) + } + end + + before do + filter_values_parent + filter_value_extra + end + + it 'updates the filter if child and parent properties are the same' do + expect { service }.not_to change(ChargeFilter, :count) + + expect(filter.reload).to have_attributes( + invoice_display_name: 'New display name', + properties: {'amount' => '20'} + ) + expect(filter.values.count).to eq(2) + expect(filter.values.pluck(:values).flatten).to match_array(%w[domestic visa]) + end + + context 'when properties are already overridden' do + let(:properties) { {amount: '755'} } + let(:filter_parent) { create(:charge_filter, properties:, charge: charge_parent) } + + it 'does not update the filter' do + expect { service }.not_to change(ChargeFilter, :count) + + expect(filter.reload).to have_attributes( + invoice_display_name: nil, + properties: charge.properties + ) + expect(filter.values.count).to eq(2) + expect(filter.values.pluck(:values).flatten).to match_array(%w[domestic visa]) + end + end + + context 'when changing filter values' do + let(:filters_params) do + [ + { + values: { + card_location_filter.key => ['international'], + scheme_filter.key => ['mastercard'] + }, + invoice_display_name: 'New display name', + properties: {amount: '20'} + } + ] + end + + it 'creates a new filter and removes only the one that matches with parent' do + result = service + + expect(result.filters.count).to eq(1) + expect(filter.reload).to be_discarded + expect(filter_value_extra.reload).not_to be_discarded + + new_filter = result.filters.first + expect(new_filter.values.count).to eq(2) + expect(new_filter.values.pluck(:values).flatten).to match_array(%w[international mastercard]) + end + end + + context 'when adding a value into filter values' do + let(:filters_params) do + [ + { + values: { + card_location_filter.key => ['domestic'], + scheme_filter.key => %w[visa mastercard] + }, + invoice_display_name: 'New display name', + properties: {amount: '20'} + } + ] + end + + it 'creates a new filter and removes only the one that matches with parent' do + result = service + + expect(result.filters.count).to eq(1) + expect(filter.reload).to be_discarded + expect(filter_value_extra.reload).not_to be_discarded + + new_filter = result.filters.first + expect(new_filter.values.count).to eq(2) + expect(new_filter.values.pluck(:values).flatten).to match_array(%w[domestic visa mastercard]) + end + end + end end end diff --git a/spec/services/charges/update_service_spec.rb b/spec/services/charges/update_service_spec.rb index 8322660ef59..23673bc0546 100644 --- a/spec/services/charges/update_service_spec.rb +++ b/spec/services/charges/update_service_spec.rb @@ -3,12 +3,16 @@ require 'rails_helper' RSpec.describe Charges::UpdateService, type: :service do - subject(:update_service) { described_class.new(charge:, params:, cascade:) } + subject(:update_service) { described_class.new(charge:, params:, cascade_options:) } let(:membership) { create(:membership) } let(:organization) { membership.organization } let(:plan) { create(:plan, organization:) } - let(:cascade) { false } + let(:cascade_options) do + { + cascade: false + } + end describe '#call' do let(:sum_billable_metric) { create(:sum_billable_metric, organization:, recurring: true) } @@ -99,16 +103,60 @@ end context 'when cascade is true' do - let(:cascade) { true } + let(:cascade_options) do + { + cascade: true, + parent_filters: [], + equal_properties: true + } + end - it 'updates only charge properties' do + it 'updates charge properties and filters' do update_service.call expect(charge.reload).to have_attributes( properties: {'amount' => '0'} ) - expect(charge.filters.count).to eq(0) + expect(charge.filters.first).to have_attributes( + invoice_display_name: 'Card filter', + properties: {'amount' => '90'} + ) + expect(charge.filters.first.values.first).to have_attributes( + billable_metric_filter_id: billable_metric_filter.id, + values: ['card'] + ) + end + + context 'with charge properties already overridden' do + let(:cascade_options) do + { + cascade: true, + parent_filters: [], + equal_properties: false + } + end + let(:params) do + { + id: charge&.id, + billable_metric_id: sum_billable_metric.id, + charge_model: 'standard', + pay_in_advance: true, + prorated: true, + invoiceable: false, + properties: { + amount: '400' + } + } + end + + it 'does not update charge properties' do + update_service.call + + expect(charge.reload).to have_attributes( + properties: {'amount' => '300'} + ) + end end end end