fix(subscription upgrade): Upgrade subscriptions previously considere…

…d downgrade on amount increase (#2823)

## Context

Currently, when a user updates the subscription fee amount for a plan
associated with a pending downgrade (scheduled to activate at the end of
the billing period), the system does not treat the pending subscription
as an upgrade. As a result, the new subscription is not activated, and
the previous one is not terminated.

## Description

This change extracts the subscription upgrade logic into a dedicated
service, ensuring it can be consistently applied across flows that
trigger subscription upgrades. Currently, this includes plan updates and
initial subscription creation.

An additional edge case, involving plan subscription overrides, is not
addressed in this PR. Handling this scenario introduces extra technical
and product complexities that require further consideration.

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)
@@ -276,12 +276,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! }