misc: Put external id as mandatory on subscriptions from API

app/models/subscription.rb

@@ -74,7 +74,8 @@ def already_billed?
   def validate_external_id
     return unless active?
 
-    used_ids = customer&.active_subscriptions&.pluck(:external_id)
+    # NOTE: We want unique external id per organization.
+    used_ids = organization.subscriptions.pluck(:external_id)
     errors.add(:external_id, :value_already_exists) if used_ids&.include?(external_id)
   end
 end

app/services/subscriptions/create_service.rb

@@ -20,7 +20,7 @@ def create_from_api(organization:, params:)
       @name = params[:name]&.strip
       @external_id = params[:external_id]&.strip
       @billing_time = params[:billing_time]
-      @current_subscription = find_current_subscription
+      @current_subscription = active_subscriptions&.find_by(external_id: external_id)
 
       process_create
     rescue ActiveRecord::RecordInvalid => e
@@ -42,7 +42,8 @@ def create(**args)
       @name = args[:name]&.strip
       @external_id = SecureRandom.uuid
       @billing_time = args[:billing_time]
-      @current_subscription = find_current_subscription(subscription_id: args[:subscription_id])
+      @current_subscription = active_subscriptions&.find_by(id: args[:subscription_id])
+
       process_create
     end
 
@@ -96,7 +97,7 @@ def create_subscription
         plan_id: current_plan.id,
         subscription_date: Time.zone.now.to_date,
         name: name,
-        external_id: external_id || current_customer.external_id,
+        external_id: external_id,
         billing_time: billing_time || :calendar,
       )
       new_subscription.mark_as_active!
@@ -211,12 +212,5 @@ def currency_missmatch?(old_plan, new_plan)
     def active_subscriptions
       @active_subscriptions ||= current_customer&.active_subscriptions
     end
-
-    def find_current_subscription(subscription_id: nil)
-      return active_subscriptions&.find_by(id: subscription_id) if subscription_id
-      return active_subscriptions&.find_by(external_id: external_id) if external_id
-
-      active_subscriptions&.find_by(external_id: current_customer.external_id)
-    end
   end
 end

spec/models/subscription_spec.rb

@@ -120,8 +120,22 @@
     let(:customer) { create(:customer, organization: organization) }
     let(:plan) { create(:plan) }
     let(:external_id) { SecureRandom.uuid }
-    let(:subscription) { create(:active_subscription, plan: plan, customer: customer) }
-    let(:new_subscription) { build(:active_subscription, plan: plan, external_id: external_id, customer: customer) }
+    let(:subscription) do
+      create(
+        :active_subscription,
+        plan: plan,
+        customer: create(:customer, organization: organization)
+      )
+    end
+
+    let(:new_subscription) do
+      build(
+        :active_subscription,
+        plan: plan,
+        external_id: external_id,
+        customer: create(:customer, organization: organization)
+      )
+    end
 
     before { subscription }
 

spec/requests/api/v1/subscriptions_spec.rb

@@ -26,6 +26,7 @@
       result = JSON.parse(response.body, symbolize_names: true)[:subscription]
 
       expect(result[:lago_id]).to be_present
+      expect(result[:external_id]).to be_present
       expect(result[:external_customer_id]).to eq(customer.external_id)
       expect(result[:lago_customer_id]).to eq(customer.id)
       expect(result[:plan_code]).to eq(plan.code)

spec/services/subscriptions/create_service_spec.rb

@@ -43,7 +43,7 @@
         expect(subscription.subscription_date).to be_present
         expect(subscription.name).to eq('invoice display name')
         expect(subscription).to be_active
-        expect(subscription.external_id).to be_present
+        expect(subscription.external_id).to eq(external_id)
         expect(subscription).to be_anniversary
       end
     end
@@ -72,13 +72,14 @@
     context 'when external_id is not given' do
       let(:external_id) { nil }
 
-      it 'sets customer_id as external_id' do
+      it 'returns an error' do
         result = create_service.create_from_api(
           organization: organization,
           params: params,
         )
 
-        expect(result.subscription.external_id).to eq(customer.external_id)
+        expect(result).not_to be_success
+        expect(result.error_details[:external_id]).to eq(['value_is_mandatory'])
       end
     end
 
@@ -258,28 +259,6 @@
         end
       end
 
-      context 'when external_id is not given' do
-        let(:params) do
-          {
-            external_customer_id: customer.external_id,
-            plan_code: plan.code,
-            name: 'invoice display name',
-          }
-        end
-
-        it 'returns existing subscription' do
-          subscription.update!(external_id: customer.external_id)
-
-          result = create_service.create_from_api(
-            organization: organization,
-            params: params,
-          )
-
-          expect(result).to be_success
-          expect(result.subscription.id).to eq(subscription.id)
-        end
-      end
-
       context 'when new plan has different currency than the old plan' do
         let(:new_plan) { create(:plan, amount_cents: 200, organization: organization, amount_currency: 'USD') }
         let(:params) do