From fbf7c768447628609b4f128fad499771093f6f61 Mon Sep 17 00:00:00 2001 From: Lovro Colic Date: Fri, 18 Oct 2024 17:49:49 +0200 Subject: [PATCH 1/4] add logic for cascading charge removal --- app/jobs/charges/destroy_job.rb | 12 ++++ app/services/charges/destroy_service.rb | 33 ++++++++++ app/services/plans/update_service.rb | 23 ++++--- spec/jobs/charges/destroy_job_spec.rb | 17 ++++++ spec/services/charges/destroy_service_spec.rb | 54 +++++++++++++++++ spec/services/plans/update_service_spec.rb | 60 +++++++++++++++++++ 6 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 app/jobs/charges/destroy_job.rb create mode 100644 app/services/charges/destroy_service.rb create mode 100644 spec/jobs/charges/destroy_job_spec.rb create mode 100644 spec/services/charges/destroy_service_spec.rb diff --git a/app/jobs/charges/destroy_job.rb b/app/jobs/charges/destroy_job.rb new file mode 100644 index 00000000000..35491892e1f --- /dev/null +++ b/app/jobs/charges/destroy_job.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Charges + class DestroyJob < ApplicationJob + queue_as 'default' + + def perform(charge:) + destroy_result = Charges::DestroyService.call(charge:) + destroy_result.raise_if_error! + end + end +end diff --git a/app/services/charges/destroy_service.rb b/app/services/charges/destroy_service.rb new file mode 100644 index 00000000000..28d33192815 --- /dev/null +++ b/app/services/charges/destroy_service.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Charges + class DestroyService < BaseService + def initialize(charge:) + @charge = charge + + super + end + + def call + return result.not_found_failure!(resource: 'charge') unless charge + + ActiveRecord::Base.transaction do + charge.discard! + charge.filter_values.discard_all + charge.filters.discard_all + + result.charge = charge + end + + result + rescue ActiveRecord::RecordInvalid => e + result.record_validation_failure!(record: e.record) + rescue BaseService::FailedResult => e + e.result + end + + private + + attr_reader :charge + end +end diff --git a/app/services/plans/update_service.rb b/app/services/plans/update_service.rb index 6d4dc9e64c2..082e8edc63a 100644 --- a/app/services/plans/update_service.rb +++ b/app/services/plans/update_service.rb @@ -108,6 +108,17 @@ def cascade_charge_creation(payload_charge) end end + def cascade_charge_removal(charge) + return unless cascade? + return if plan.children.empty? + + plan.children.includes(:charges).each do |p| + child_charge = p.charges.find { |c| c.parent_id == charge.id } + + Charges::DestroyJob.perform_later(charge: child_charge) if child_charge + end + end + def cascade? ActiveModel::Type::Boolean.new.cast(params[:cascade_updates]) end @@ -258,7 +269,10 @@ def process_charges(plan, params_charges) def sanitize_charges(plan, args_charges, created_charges_ids) args_charges_ids = args_charges.map { |c| c[:id] }.compact charges_ids = plan.charges.pluck(:id) - args_charges_ids - created_charges_ids - plan.charges.where(id: charges_ids).find_each { |charge| discard_charge!(charge) } + plan.charges.where(id: charges_ids).find_each do |charge| + Charges::DestroyService.call(charge:) + cascade_charge_removal(charge) + end end def sanitize_thresholds(plan, args_thresholds, created_thresholds_ids) @@ -267,13 +281,6 @@ def sanitize_thresholds(plan, args_thresholds, created_thresholds_ids) plan.usage_thresholds.where(id: thresholds_ids).discard_all end - def discard_charge!(charge) - charge.discard! - - charge.filter_values.discard_all - charge.filters.discard_all - end - # NOTE: We should remove pending subscriptions # if plan has been downgraded but amount cents became less than downgraded value. This pending subscription # is not relevant in this case and downgrade should be ignored diff --git a/spec/jobs/charges/destroy_job_spec.rb b/spec/jobs/charges/destroy_job_spec.rb new file mode 100644 index 00000000000..6bead3f8fbb --- /dev/null +++ b/spec/jobs/charges/destroy_job_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Charges::DestroyJob, type: :job do + let(:charge) { create(:charge) } + + before do + allow(Charges::DestroyService).to receive(:call).with(charge:).and_return(BaseService::Result.new) + end + + it 'calls the service' do + described_class.perform_now(plan:, params:) + + expect(Charges::DestroyService).to have_received(:call) + end +end diff --git a/spec/services/charges/destroy_service_spec.rb b/spec/services/charges/destroy_service_spec.rb new file mode 100644 index 00000000000..64e878c79fc --- /dev/null +++ b/spec/services/charges/destroy_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Charges::DestroyService, type: :service do + subject(:destroy_service) { described_class.new(charge:) } + + let(:membership) { create(:membership) } + let(:organization) { membership.organization } + let(:billable_metric) { create(:billable_metric, organization:) } + let(:subscription) { create(:subscription) } + let(:charge) { create(:standard_charge, plan: subscription.plan, billable_metric:) } + + let(:filters) { create_list(:billable_metric_filter, 2, billable_metric:) } + let(:charge_filter) { create(:charge_filter, charge:) } + let(:filter_value) do + create(:charge_filter_value, charge_filter:, billable_metric_filter: filters.first) + end + + before do + charge + filter_value + end + + describe '#call' do + it 'soft deletes the charge' do + freeze_time do + expect { destroy_service.call }.to change(Charge, :count).by(-1) + .and change { charge.reload.deleted_at }.from(nil).to(Time.current) + end + end + + it 'soft deletes all related filters' do + freeze_time do + expect { destroy_service.call }.to change { charge_filter.reload.deleted_at }.from(nil).to(Time.current) + end + end + + it 'soft deletes all related filter values' do + freeze_time do + expect { destroy_service.call }.to change { filter_value.reload.deleted_at }.from(nil).to(Time.current) + end + end + + context 'when charge is not found' do + it 'returns an error' do + result = described_class.new(charge: nil).call + + expect(result).not_to be_success + expect(result.error.error_code).to eq('charge_not_found') + end + end + end +end diff --git a/spec/services/plans/update_service_spec.rb b/spec/services/plans/update_service_spec.rb index 8199360d338..df08b6693dd 100644 --- a/spec/services/plans/update_service_spec.rb +++ b/spec/services/plans/update_service_spec.rb @@ -932,6 +932,66 @@ .to change { charge.reload.deleted_at }.from(nil).to(Time.current) end end + + context 'with cascade option' do + let(:child_plan) { create(:plan, organization:, parent_id:) } + let(:parent_id) { plan.id } + let(:charge_parent_id) { charge.id } + let(:child_charge) do + create( + :standard_charge, + plan_id: child_plan.id, + parent_id: charge_parent_id, + billable_metric_id: billable_metric.id, + properties: {amount: '300'} + ) + end + + before do + child_charge + update_args[:cascade_updates] = true + end + + context 'when cascade is true and there is no children plans' do + let(:parent_id) { nil } + + it 'does not enqueue the job for removing charge' do + expect do + plans_service.call + end.not_to have_enqueued_job(Charges::DestroyJob) + end + end + + context 'when cascade is true and there are children plans' do + it 'enqueues the job for removing charge' do + expect do + plans_service.call + end.to have_enqueued_job(Charges::DestroyJob) + end + end + + context 'when cascade is true and there are children plans without link to parent charge' do + let(:charge_parent_id) { nil } + + it 'does not enqueue the job for removing charge' do + expect do + plans_service.call + end.not_to have_enqueued_job(Charges::DestroyJob) + end + end + + context 'when cascade is false with children plans' do + before do + update_args[:cascade_updates] = false + end + + it 'does not enqueue the job for removing charge' do + expect do + plans_service.call + end.not_to have_enqueued_job(Charges::DestroyJob) + end + end + end end context 'when attached to a subscription' do From 4ff72703389867e07c35635c209c76ef8face11a Mon Sep 17 00:00:00 2001 From: Lovro Colic Date: Fri, 18 Oct 2024 17:54:10 +0200 Subject: [PATCH 2/4] fix linter issue --- app/services/plans/update_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/plans/update_service.rb b/app/services/plans/update_service.rb index 082e8edc63a..b6e306f3b86 100644 --- a/app/services/plans/update_service.rb +++ b/app/services/plans/update_service.rb @@ -112,7 +112,7 @@ def cascade_charge_removal(charge) return unless cascade? return if plan.children.empty? - plan.children.includes(:charges).each do |p| + plan.children.includes(:charges).find_each do |p| child_charge = p.charges.find { |c| c.parent_id == charge.id } Charges::DestroyJob.perform_later(charge: child_charge) if child_charge From 99c94c7dedf2be2b239cbf59624d94bd526f2f86 Mon Sep 17 00:00:00 2001 From: Lovro Colic Date: Fri, 18 Oct 2024 18:08:55 +0200 Subject: [PATCH 3/4] fix spec --- spec/jobs/charges/destroy_job_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/jobs/charges/destroy_job_spec.rb b/spec/jobs/charges/destroy_job_spec.rb index 6bead3f8fbb..5eb196e2e5b 100644 --- a/spec/jobs/charges/destroy_job_spec.rb +++ b/spec/jobs/charges/destroy_job_spec.rb @@ -3,14 +3,14 @@ require 'rails_helper' RSpec.describe Charges::DestroyJob, type: :job do - let(:charge) { create(:charge) } + let(:charge) { create(:standard_charge) } before do allow(Charges::DestroyService).to receive(:call).with(charge:).and_return(BaseService::Result.new) end it 'calls the service' do - described_class.perform_now(plan:, params:) + described_class.perform_now(charge:) expect(Charges::DestroyService).to have_received(:call) end From 7878e5b32f37704552e0af28c8bd6fd11a528cf8 Mon Sep 17 00:00:00 2001 From: Lovro Colic Date: Mon, 21 Oct 2024 10:44:20 +0200 Subject: [PATCH 4/4] add small improvement --- app/jobs/charges/create_job.rb | 3 +-- app/jobs/charges/destroy_job.rb | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/jobs/charges/create_job.rb b/app/jobs/charges/create_job.rb index 9e3ebe52ffa..c28e3efc2fc 100644 --- a/app/jobs/charges/create_job.rb +++ b/app/jobs/charges/create_job.rb @@ -5,8 +5,7 @@ class CreateJob < ApplicationJob queue_as 'default' def perform(plan:, params:) - create_result = Charges::CreateService.call(plan:, params:) - create_result.raise_if_error! + Charges::CreateService.call(plan:, params:).raise_if_error! end end end diff --git a/app/jobs/charges/destroy_job.rb b/app/jobs/charges/destroy_job.rb index 35491892e1f..38b1642a506 100644 --- a/app/jobs/charges/destroy_job.rb +++ b/app/jobs/charges/destroy_job.rb @@ -5,8 +5,7 @@ class DestroyJob < ApplicationJob queue_as 'default' def perform(charge:) - destroy_result = Charges::DestroyService.call(charge:) - destroy_result.raise_if_error! + Charges::DestroyService.call(charge:).raise_if_error! end end end