From a0477ccca838b68a5afde7a7b03ad3bc627380ad Mon Sep 17 00:00:00 2001 From: Ancor Cruz Date: Wed, 13 Nov 2024 16:30:20 +0000 Subject: [PATCH 1/5] update outdated code comment... do we really need this? --- app/services/plans/update_service.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/plans/update_service.rb b/app/services/plans/update_service.rb index 9d1fce0e0bd..56f7d55b101 100644 --- a/app/services/plans/update_service.rb +++ b/app/services/plans/update_service.rb @@ -18,8 +18,8 @@ def call plan.description = params[:description] if params.key?(:description) plan.amount_cents = params[:amount_cents] if params.key?(:amount_cents) - # NOTE: Only name and description are editable if plan - # is attached to subscriptions + # NOTE: If plan is attached to subscriptions the editable attributes are: + # name, invoice_display_name, description, amount_cents unless plan.attached_to_subscriptions? plan.code = params[:code] if params.key?(:code) plan.interval = params[:interval].to_sym if params.key?(:interval) From 4f1be20777521ea7c229365aaaa879b2b6d5c92e Mon Sep 17 00:00:00 2001 From: Ancor Cruz Date: Thu, 14 Nov 2024 10:24:12 +0000 Subject: [PATCH 2/5] Extract subscription plan upgrade to a service --- app/services/subscriptions/create_service.rb | 57 +----- .../subscriptions/plan_upgrade_service.rb | 113 +++++++++++ .../plan_upgrade_service_spec.rb | 189 ++++++++++++++++++ 3 files changed, 303 insertions(+), 56 deletions(-) create mode 100644 app/services/subscriptions/plan_upgrade_service.rb create mode 100644 spec/services/subscriptions/plan_upgrade_service_spec.rb diff --git a/app/services/subscriptions/create_service.rb b/app/services/subscriptions/create_service.rb index 118324e0c89..b303a12e651 100644 --- a/app/services/subscriptions/create_service.rb +++ b/app/services/subscriptions/create_service.rb @@ -136,48 +136,7 @@ def create_subscription end def upgrade_subscription - if current_subscription.starting_in_the_future? - update_pending_subscription - - return current_subscription - end - - new_subscription = Subscription.new( - customer:, - plan: params.key?(:plan_overrides) ? override_plan(plan) : plan, - name:, - external_id: current_subscription.external_id, - previous_subscription_id: current_subscription.id, - subscription_at: current_subscription.subscription_at, - billing_time: current_subscription.billing_time, - ending_at: params.key?(:ending_at) ? params[:ending_at] : current_subscription.ending_at - ) - - cancel_pending_subscription if pending_subscription? - - # Collection that groups all billable subscriptions for an invoice - billable_subscriptions = billable_subscriptions(new_subscription) - - # NOTE: When upgrading, the new subscription becomes active immediately - # The previous one must be terminated - Subscriptions::TerminateService.call(subscription: current_subscription, upgrade: true) - - new_subscription.mark_as_active! - after_commit { SendWebhookJob.perform_later('subscription.started', new_subscription) } - - # NOTE: If plan is in advance we should create only one invoice for termination fees and for new plan fees - if billable_subscriptions.any? - # NOTE: Since job is launched from inside a db transaction - # we must wait for it to be committed before processing the job - # We do not set offset anymore but instead retry jobs - after_commit do - billing_at = Time.current + 1.second - BillSubscriptionJob.perform_later(billable_subscriptions, billing_at.to_i, invoicing_reason: :upgrading) - BillNonInvoiceableFeesJob.perform_later(billable_subscriptions, billing_at) - end - end - - new_subscription + PlanUpgradeService.call(current_subscription:, plan:, params:).subscription end def downgrade_subscription @@ -266,19 +225,5 @@ def editable_subscriptions def override_plan(plan) Plans::OverrideService.call(plan:, params: params[:plan_overrides].to_h.with_indifferent_access).plan end - - def billable_subscriptions(new_subscription) - billable_subscriptions = if current_subscription.starting_in_the_future? - [] - elsif current_subscription.pending? - [] - elsif !current_subscription.terminated? - [current_subscription] - end.to_a - - billable_subscriptions << new_subscription if plan.pay_in_advance? && !new_subscription.in_trial_period? - - billable_subscriptions - end end end diff --git a/app/services/subscriptions/plan_upgrade_service.rb b/app/services/subscriptions/plan_upgrade_service.rb new file mode 100644 index 00000000000..9252b397ac1 --- /dev/null +++ b/app/services/subscriptions/plan_upgrade_service.rb @@ -0,0 +1,113 @@ +module Subscriptions + class PlanUpgradeService < BaseService + def initialize(current_subscription:, plan:, params:) + @current_subscription = current_subscription + @plan = plan + + @params = params + @name = params[:name].to_s.strip + super + end + + def call + if current_subscription.starting_in_the_future? + update_pending_subscription + + result.subscription = current_subscription + return result + end + + new_subscription = new_subcription_with_overrides + + ActiveRecord::Base.transaction do + cancel_pending_subscription if pending_subscription? + + # Group subscriptions for billing + billable_subscriptions = billable_subscriptions(new_subscription) + + # Terminate current subscription as part of the upgrade process + Subscriptions::TerminateService.call( + subscription: current_subscription, + upgrade: true + ) + + new_subscription.mark_as_active! + after_commit do + SendWebhookJob.perform_later("subscription.started", new_subscription) + end + + bill_subscriptions(billable_subscriptions) if billable_subscriptions.any? + end + + result.subscription = new_subscription + result + rescue ActiveRecord::RecordInvalid => e + result.record_validation_failure!(record: e.record) + rescue BaseService::FailedResult => e + result.fail_with_error!(e) + end + + private + + attr_reader :current_subscription, :plan, :params, :name + + def new_subcription_with_overrides + Subscription.new( + customer: current_subscription.customer, + plan: params.key?(:plan_overrides) ? override_plan : plan, + name:, + external_id: current_subscription.external_id, + previous_subscription_id: current_subscription.id, + subscription_at: current_subscription.subscription_at, + billing_time: current_subscription.billing_time, + ending_at: params.key?(:ending_at) ? params[:ending_at] : current_subscription.ending_at + ) + end + + def update_pending_subscription + current_subscription.plan = plan + current_subscription.name = name if name.present? + current_subscription.save! + + if current_subscription.should_sync_crm_subscription? + Integrations::Aggregator::Subscriptions::Crm::UpdateJob.perform_later(subscription: current_subscription) + end + end + + def override_plan + Plans::OverrideService.call(plan:, params: params[:plan_overrides].to_h.with_indifferent_access).plan + end + + def cancel_pending_subscription + current_subscription.next_subscription.mark_as_canceled! + end + + def pending_subscription? + return false unless current_subscription.next_subscription + + current_subscription.next_subscription.pending? + end + + def billable_subscriptions(new_subscription) + billable_subscriptions = if current_subscription.starting_in_the_future? + [] + elsif current_subscription.pending? + [] + elsif !current_subscription.terminated? + [current_subscription] + end.to_a + + billable_subscriptions << new_subscription if plan.pay_in_advance? && !new_subscription.in_trial_period? + + billable_subscriptions + end + + def bill_subscriptions(billable_subscriptions) + after_commit do + billing_at = Time.current + 1.second + BillSubscriptionJob.perform_later(billable_subscriptions, billing_at.to_i, invoicing_reason: :upgrading) + BillNonInvoiceableFeesJob.perform_later(billable_subscriptions, billing_at) + end + end + end +end diff --git a/spec/services/subscriptions/plan_upgrade_service_spec.rb b/spec/services/subscriptions/plan_upgrade_service_spec.rb new file mode 100644 index 00000000000..99f646a0382 --- /dev/null +++ b/spec/services/subscriptions/plan_upgrade_service_spec.rb @@ -0,0 +1,189 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Subscriptions::PlanUpgradeService, type: :service do + subject(:result) do + described_class.call(current_subscription: subscription, plan:, params:) + end + + let(:subscription) do + create( + :subscription, + customer:, + plan: old_plan, + status: :active, + subscription_at: Time.current, + started_at: Time.current, + external_id: SecureRandom.uuid, + ) + end + + let(:old_plan) { create(:plan, amount_cents: 100, organization:, amount_currency: currency) } + let(:customer) { create(:customer, :with_hubspot_integration, organization:, currency:) } + let(:organization) { create(:organization) } + let(:currency) { "EUR" } + let(:plan) { create(:plan, amount_cents: 100, organization:) } + let(:params) { {name: subscription_name} } + let(:subscription_name) { "new invoice display name" } + + describe "#call", :aggregate_failures do + before do + subscription.mark_as_active! + end + + it "terminates the existing subscription" do + expect { result } + .to change { subscription.reload.status }.from("active").to("terminated") + end + + it "moves the lifetime_usage to the new subscription" do + lifetime_usage = subscription.lifetime_usage + expect(result.subscription.lifetime_usage).to eq(lifetime_usage.reload) + expect(subscription.reload.lifetime_usage).to be_nil + end + + it "sends terminated and started subscription webhooks" do + result + expect(SendWebhookJob).to have_been_enqueued.with("subscription.terminated", subscription) + expect(SendWebhookJob).to have_been_enqueued.with("subscription.started", result.subscription) + end + + it "enqueues the CRM update job" do + # TODO: review this one, this one should fail because the code conditional + # is not meet by the test setup... + # The subscription does not start in the future + result + expect(Integrations::Aggregator::Subscriptions::Crm::UpdateJob).to have_been_enqueued.with(subscription:) + end + + it "creates a new subscription" do + expect(result).to be_success + expect(result.subscription.id).not_to eq(subscription.id) + expect(result.subscription).to be_active + expect(result.subscription.name).to eq(subscription_name) + expect(result.subscription.plan.id).to eq(plan.id) + expect(result.subscription.previous_subscription_id).to eq(subscription.id) + expect(result.subscription.subscription_at).to eq(subscription.subscription_at) + end + + context "when current subscription is pending" do + before { subscription.pending! } + + it "returns existing subscription with updated attributes" do + expect(result).to be_success + expect(result.subscription.id).to eq(subscription.id) + expect(result.subscription.plan_id).to eq(plan.id) + expect(result.subscription.name).to eq(subscription_name) + end + end + + context "when old subscription is payed in arrear" do + let(:old_plan) { create(:plan, amount_cents: 100, organization:, pay_in_advance: false) } + + it "enqueues a job to bill the existing subscription" do + expect { result }.to have_enqueued_job(BillSubscriptionJob) + end + end + + context "when old subscription was payed in advance" do + let(:creation_time) { Time.current.beginning_of_month - 1.month } + let(:date_service) do + Subscriptions::DatesService.new_instance( + subscription, + Time.current.beginning_of_month, + current_usage: false + ) + end + + let(:invoice_subscription) do + create( + :invoice_subscription, + invoice:, + subscription:, + recurring: true, + from_datetime: date_service.from_datetime, + to_datetime: date_service.to_datetime, + charges_from_datetime: date_service.charges_from_datetime, + charges_to_datetime: date_service.charges_to_datetime + ) + end + + let(:invoice) do + create( + :invoice, + customer:, + currency:, + sub_total_excluding_taxes_amount_cents: 100, + fees_amount_cents: 100, + taxes_amount_cents: 20, + total_amount_cents: 120 + ) + end + + let(:last_subscription_fee) do + create( + :fee, + subscription:, + invoice:, + amount_cents: 100, + taxes_amount_cents: 20, + invoiceable_type: "Subscription", + invoiceable_id: subscription.id, + taxes_rate: 20 + ) + end + + let(:subscription) do + create( + :subscription, + customer:, + plan: old_plan, + status: :active, + subscription_at: creation_time, + started_at: creation_time, + external_id: SecureRandom.uuid, + billing_time: "anniversary" + ) + end + + let(:old_plan) { create(:plan, amount_cents: 100, organization:, pay_in_advance: true) } + + before do + invoice_subscription + last_subscription_fee + end + + it "creates a credit note for the remaining days" do + expect { result }.to change(CreditNote, :count) + end + end + + context "when new subscription is payed in advance" do + let(:plan) { create(:plan, amount_cents: 200, organization:, pay_in_advance: true) } + + it "enqueues a job to bill the existing subscription" do + expect { result }.to have_enqueued_job(BillSubscriptionJob) + end + end + + context "with pending next subscription" do + let(:next_subscription) do + create( + :subscription, + status: :pending, + previous_subscription: subscription, + organization:, + customer: + ) + end + + before { next_subscription } + + it "canceled the next subscription" do + expect(result).to be_success + expect(next_subscription.reload).to be_canceled + end + end + end +end From 56c6259095e4cb1af80ed616e18033bbb9d2ca30 Mon Sep 17 00:00:00 2001 From: Ancor Cruz Date: Thu, 14 Nov 2024 12:00:27 +0000 Subject: [PATCH 3/5] Add error result management for PlanUpgrade on Subscription creation --- app/services/subscriptions/create_service.rb | 4 +++- .../subscriptions/create_service_spec.rb | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/services/subscriptions/create_service.rb b/app/services/subscriptions/create_service.rb index b303a12e651..bd117096d8b 100644 --- a/app/services/subscriptions/create_service.rb +++ b/app/services/subscriptions/create_service.rb @@ -136,7 +136,9 @@ def create_subscription end def upgrade_subscription - PlanUpgradeService.call(current_subscription:, plan:, params:).subscription + PlanUpgradeService.call(current_subscription:, plan:, params:).tap do |result| + result.raise_if_error! + end.subscription end def downgrade_subscription diff --git a/spec/services/subscriptions/create_service_spec.rb b/spec/services/subscriptions/create_service_spec.rb index 6598234fc75..fd195255e17 100644 --- a/spec/services/subscriptions/create_service_spec.rb +++ b/spec/services/subscriptions/create_service_spec.rb @@ -494,6 +494,28 @@ end end + context "when subscription upgrade fails" do + let(:result_failure) do + BaseService::Result.new.validation_failure!( + errors: {billing_time: ['value_is_invalid']} + ) + end + + before do + allow(Subscriptions::PlanUpgradeService) + .to receive(:call) + .and_return(result_failure) + end + + it "returns an error", :aggregate_failures do + result = create_service.call + + expect(result).not_to be_success + expect(result.error).to be_a(BaseService::ValidationFailure) + expect(result.error.messages).to eq({ billing_time: ["value_is_invalid"] }) + end + end + context 'when current subscription is pending' do before { subscription.pending! } From bb418461172a52c16abe7aae4aa6da3a232f3f51 Mon Sep 17 00:00:00 2001 From: Ancor Cruz Date: Thu, 14 Nov 2024 12:32:10 +0000 Subject: [PATCH 4/5] Upgrade subscription on plan update with amount changes --- app/services/plans/update_service.rb | 19 +++++--- spec/services/plans/update_service_spec.rb | 51 +++++++++++++++++++--- 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/app/services/plans/update_service.rb b/app/services/plans/update_service.rb index 56f7d55b101..c53a662b77d 100644 --- a/app/services/plans/update_service.rb +++ b/app/services/plans/update_service.rb @@ -268,12 +268,21 @@ def process_downgraded_subscriptions end end - # NOTE: We should remove pending subscriptions - # if plan has been downgraded but amount cents of pending plan became higher than original plan. - # This pending subscription is not relevant in this case and downgrade should be ignored + # NOTE: If new plan yearly amount is higher than its value before the update + # and there are pending subscriptions for the plan, + # this is a plan upgrade, old subscription must be terminated and billed + # new subscription with updated plan must be activated inmediately. def process_pending_subscriptions - Subscription.where(plan:, status: :pending).find_each do |sub| - sub.mark_as_canceled! if plan.amount_cents > sub.previous_subscription.plan.amount_cents + Subscription.where(plan:, status: :pending).find_each do |subscription| + next unless subscription.previous_subscription + + if plan.yearly_amount_cents >= subscription.previous_subscription.plan.yearly_amount_cents + Subscriptions::PlanUpgradeService.call( + current_subscription: subscription.previous_subscription, + plan: plan, + params: {name: subscription.name} + ).raise_if_error! + end end end end diff --git a/spec/services/plans/update_service_spec.rb b/spec/services/plans/update_service_spec.rb index b5686da4899..ead32f31de5 100644 --- a/spec/services/plans/update_service_spec.rb +++ b/spec/services/plans/update_service_spec.rb @@ -402,17 +402,54 @@ amount_currency: 'EUR' } end + let(:plan_upgrade_result) { BaseService::Result.new } - before { pending_subscription } + before do + allow(Subscriptions::PlanUpgradeService) + .to receive(:call) + .and_return(plan_upgrade_result) - it 'correctly cancels pending subscriptions' do + pending_subscription + end + + it "upgrades subscription plan" do + plans_service.call + + expect(Subscriptions::PlanUpgradeService).to have_received(:call) + end + + it 'updates the plan', :aggregate_failures do result = plans_service.call - updated_plan = result.plan - aggregate_failures do - expect(updated_plan.name).to eq('Updated plan name') - expect(updated_plan.amount_cents).to eq(200) - expect(Subscription.find_by(id: pending_subscription.id).status).to eq('canceled') + expect(result.plan.name).to eq('Updated plan name') + expect(result.plan.amount_cents).to eq(200) + end + + context "when pending subscription does not have a previous one" do + let(:pending_subscription) do + create(:subscription, plan:, status: :pending, previous_subscription_id: nil) + end + + it "does not upgrade it" do + plans_service.call + + expect(Subscriptions::PlanUpgradeService).not_to have_received(:call) + end + end + + context "when subscription upgrade fails" do + let(:plan_upgrade_result) do + BaseService::Result.new.validation_failure!( + errors: {billing_time: ['value_is_invalid']} + ) + end + + it "returns an error", :aggregate_failures do + result = plans_service.call + + expect(result).not_to be_success + expect(result.error).to be_a(BaseService::ValidationFailure) + expect(result.error.messages).to eq({ billing_time: ["value_is_invalid"] }) end end end From 93bf1f6e5d31c13cb61ada020718c9db42baeed1 Mon Sep 17 00:00:00 2001 From: Ancor Cruz Date: Fri, 15 Nov 2024 08:12:15 +0000 Subject: [PATCH 5/5] Fix rubocop offenses --- app/services/subscriptions/plan_upgrade_service.rb | 2 ++ spec/services/plans/update_service_spec.rb | 2 +- spec/services/subscriptions/create_service_spec.rb | 2 +- spec/services/subscriptions/plan_upgrade_service_spec.rb | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/services/subscriptions/plan_upgrade_service.rb b/app/services/subscriptions/plan_upgrade_service.rb index 9252b397ac1..5e086cf58f5 100644 --- a/app/services/subscriptions/plan_upgrade_service.rb +++ b/app/services/subscriptions/plan_upgrade_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Subscriptions class PlanUpgradeService < BaseService def initialize(current_subscription:, plan:, params:) diff --git a/spec/services/plans/update_service_spec.rb b/spec/services/plans/update_service_spec.rb index ead32f31de5..f5a92bd33bf 100644 --- a/spec/services/plans/update_service_spec.rb +++ b/spec/services/plans/update_service_spec.rb @@ -449,7 +449,7 @@ expect(result).not_to be_success expect(result.error).to be_a(BaseService::ValidationFailure) - expect(result.error.messages).to eq({ billing_time: ["value_is_invalid"] }) + expect(result.error.messages).to eq({billing_time: ["value_is_invalid"]}) end end end diff --git a/spec/services/subscriptions/create_service_spec.rb b/spec/services/subscriptions/create_service_spec.rb index fd195255e17..8d015f5dbaa 100644 --- a/spec/services/subscriptions/create_service_spec.rb +++ b/spec/services/subscriptions/create_service_spec.rb @@ -512,7 +512,7 @@ expect(result).not_to be_success expect(result.error).to be_a(BaseService::ValidationFailure) - expect(result.error.messages).to eq({ billing_time: ["value_is_invalid"] }) + expect(result.error.messages).to eq({billing_time: ["value_is_invalid"]}) end end diff --git a/spec/services/subscriptions/plan_upgrade_service_spec.rb b/spec/services/subscriptions/plan_upgrade_service_spec.rb index 99f646a0382..77f9ce134d1 100644 --- a/spec/services/subscriptions/plan_upgrade_service_spec.rb +++ b/spec/services/subscriptions/plan_upgrade_service_spec.rb @@ -15,7 +15,7 @@ status: :active, subscription_at: Time.current, started_at: Time.current, - external_id: SecureRandom.uuid, + external_id: SecureRandom.uuid ) end