fix(subscription upgrade): Upgrade subscriptions previously considered downgrade on amount increase

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)
@@ -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

app/services/subscriptions/create_service.rb

@@ -136,48 +136,9 @@ 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:).tap do |result|
+        result.raise_if_error!
+      end.subscription
     end
 
     def downgrade_subscription
@@ -266,19 +227,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

app/services/subscriptions/plan_upgrade_service.rb

@@ -0,0 +1,115 @@
+# frozen_string_literal: true
+
+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

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

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! }