fix(subscriptions): prevent duplicate external_ids per organization (#5558)

Fixes BIL-44

## Context

Multiple active subscriptions with the same `external_id` could coexist
within the same organization for different customers.

The model validation only ran on `:create` missing status changes (like
`pending` => `active`).

There was also no database-level unique index.

## Description

In this PR:
- Subscription model validation `validate_external_id` is now set to run
on updates too
- Added a service-level cross-customer check in
`Subscriptions::CreateService`
- Simplified the "with multuple subscriptions" tests for
`SubscriptionsController`. They were by accident creating two active
subscriptions and validations now block that
- Did not add any unique index to the database. This should be discussed
further and handled separately.

Author: endenis

app/models/subscription.rb

@@ -48,7 +48,7 @@ class Subscription < ApplicationRecord
   delegate :amount_currency, to: :plan, prefix: true
 
   validates :external_id, :billing_time, presence: true
-  validate :validate_external_id, on: :create
+  validate :validate_external_id, on: [:create, :update], if: -> { status_changed? }
 
   STATUSES = [
     :pending,

app/services/subscriptions/create_service.rb

@@ -62,6 +62,12 @@ def call
           @current_subscription = editable_subscriptions
             .find_by("id = ? OR external_id = ?", params[:subscription_id], external_id)
 
+          if current_subscription.nil? &&
+              customer.organization.subscriptions.active.exists?(external_id:)
+            result.validation_failure!(errors: {external_id: ["value_already_exist"]})
+            result.raise_if_error!
+          end
+
           subscription = handle_subscription
 
           if params[:usage_thresholds].present?

spec/models/subscription_spec.rb

@@ -265,6 +265,46 @@
           expect(incomplete_sub).not_to be_valid
         end
       end
+
+      context "when a pending subscription transitions to active" do
+        let(:external_id) { SecureRandom.uuid }
+        let(:pending_subscription) do
+          create(
+            :subscription,
+            plan:,
+            status: :pending,
+            external_id:,
+            customer: create(:customer, organization:)
+          )
+        end
+
+        before { pending_subscription }
+
+        context "when another active subscription with the same external_id exists" do
+          before do
+            create(
+              :subscription,
+              plan:,
+              status: :active,
+              external_id:,
+              customer: create(:customer, organization:)
+            )
+          end
+
+          it "rejects the activation" do
+            pending_subscription.assign_attributes(status: :active)
+            expect(pending_subscription).not_to be_valid
+            expect(pending_subscription.errors[:external_id]).to include("value_already_exist")
+          end
+        end
+
+        context "when no other active subscription with the same external_id exists" do
+          it "allows the activation" do
+            pending_subscription.assign_attributes(status: :active)
+            expect(pending_subscription).to be_valid
+          end
+        end
+      end
     end
 
     describe "started_at validation" do

spec/requests/api/v1/subscriptions_controller_spec.rb

@@ -1723,39 +1723,33 @@ def test_termination(expected_on_termination_credit_note: nil, expected_on_termi
     end
 
     context "with multuple subscriptions" do
+      let(:update_params) do
+        {name: "subscription name new"}
+      end
       let(:active_plan) { create(:plan, organization:, amount_cents: 5000, description: "desc") }
-      let(:active_subscription) do
+      let!(:active_subscription) do
         create(:subscription, external_id: subscription.external_id, customer:, plan:)
       end
 
-      before { active_subscription }
-
       it "updates the active subscription" do
         subject
 
         expect(response).to have_http_status(:success)
         expect(json[:subscription][:lago_id]).to eq(active_subscription.id)
         expect(json[:subscription][:name]).to eq("subscription name new")
-
-        expect(json[:subscription][:plan]).to include(
-          name: "Override"
-        )
       end
 
       context "with pending params" do
         let(:params) { {subscription: update_params, status: "pending"} }
 
         it "updates the pending subscription" do
           subject
+          subscription.reload
 
           expect(response).to have_http_status(:success)
           expect(json[:subscription][:lago_id]).to eq(subscription.id)
           expect(json[:subscription][:name]).to eq("subscription name new")
-          expect(json[:subscription][:subscription_at].to_s).to eq("2022-09-05T12:23:12Z")
-
-          expect(json[:subscription][:plan]).to include(
-            name: "Override"
-          )
+          expect(subscription.status).to eq("pending")
         end
       end
     end

spec/services/subscriptions/create_service_spec.rb

@@ -1488,6 +1488,22 @@
       end
     end
 
+    context "when another customer in the same organization has an active subscription with the same external_id" do
+      let(:other_customer) { create(:customer, organization:) }
+
+      before do
+        create(:subscription, customer: other_customer, plan:, organization:, external_id:)
+      end
+
+      it "returns an external_id already exist error" do
+        result = create_service.call
+
+        expect(result).not_to be_success
+        expect(result.error).to be_a(BaseService::ValidationFailure)
+        expect(result.error.messages[:external_id]).to eq(["value_already_exist"])
+      end
+    end
+
     context "with activation_rules" do
       let(:customer) { create(:customer, organization:, payment_provider: "stripe") }