fix (invoice-numbering): revert moving invoice numbering outside DB t…

…ransaction (#2684) ## Description This PR reverts #2621
getlago · Oct 14, 2024 · 265d8ad · 265d8ad
1 parent 096f5e3
commit 265d8ad
Show file tree

Hide file tree

Showing 16 changed files with 27 additions and 249 deletions.
diff --git a/app/models/concerns/sequenced.rb b/app/models/concerns/sequenced.rb
@@ -27,7 +27,7 @@ def generate_sequential_id
         transaction: true,
         timeout_seconds: 10.seconds
       ) do
-        sequential_id = sequence_scope.with_sequential_id.where.not(id:).order(sequential_id: :desc).limit(1).pick(:sequential_id)
+        sequential_id = sequence_scope.with_sequential_id.order(sequential_id: :desc).limit(1).pick(:sequential_id)
         sequential_id ||= 0
 
         loop do

diff --git a/app/models/invoice.rb b/app/models/invoice.rb
@@ -11,6 +11,9 @@ class Invoice < ApplicationRecord
   CREDIT_NOTES_MIN_VERSION = 2
   COUPON_BEFORE_VAT_VERSION = 3
 
+  before_save :ensure_organization_sequential_id, if: -> { organization.per_organization? }
+  before_save :ensure_number
+
   belongs_to :customer, -> { with_discarded }
   belongs_to :organization
 
@@ -333,23 +336,20 @@ def document_invoice_name
     I18n.t('invoice.document_name')
   end
 
-  def ensure_organization_sequential_id
-    return if organization_sequential_id.present? && organization_sequential_id.positive?
-    return unless finalized?
+  private
 
-    self.organization_sequential_id = generate_organization_sequential_id
+  def should_assign_sequential_id?
+    status_changed_to_finalized?
   end
 
-  def ensure_invoice_sequential_id
-    return if sequential_id.present?
-
-    self.sequential_id = generate_sequential_id
+  def void_invoice!
+    update!(ready_for_payment_processing: false)
   end
 
   def ensure_number
-    self.number = "#{organization.document_number_prefix}-DRAFT" if number.blank? && !finalized?
+    self.number = "#{organization.document_number_prefix}-DRAFT" if number.blank? && !status_changed_to_finalized?
 
-    return unless finalized?
+    return unless status_changed_to_finalized?
 
     if organization.per_customer?
       # NOTE: Example of expected customer slug format is ORG_PREFIX-005
@@ -365,15 +365,11 @@ def ensure_number
     end
   end
 
-  private
-
-  # For invoices we want to skip callback and set sequential_id from the dedicated service
-  def should_assign_sequential_id?
-    false
-  end
+  def ensure_organization_sequential_id
+    return if organization_sequential_id.present? && organization_sequential_id.positive?
+    return unless status_changed_to_finalized?
 
-  def void_invoice!
-    update!(ready_for_payment_processing: false)
+    self.organization_sequential_id = generate_organization_sequential_id
   end
 
   def generate_organization_sequential_id
@@ -391,12 +387,11 @@ def generate_organization_sequential_id
     ) do
       # If previous invoice had different numbering, base sequential id is the total number of invoices
       organization_sequential_id = if switched_from_customer_numbering?
-        organization.invoices.with_generated_number.where.not(id:).count
+        organization.invoices.with_generated_number.count
       else
         organization
           .invoices
           .where.not(organization_sequential_id: 0)
-          .where.not(id:)
           .order(organization_sequential_id: :desc)
           .limit(1)
           .pick(:organization_sequential_id) || 0
@@ -417,12 +412,19 @@ def generate_organization_sequential_id
   end
 
   def switched_from_customer_numbering?
-    last_invoice = organization.invoices.where.not(id:).order(created_at: :desc).with_generated_number.first
+    last_invoice = organization.invoices.order(created_at: :desc).with_generated_number.first
 
     return false unless last_invoice
 
     last_invoice&.organization_sequential_id&.zero?
   end
+
+  def status_changed_to_finalized?
+    status_changed?(from: 'draft', to: 'finalized') ||
+      status_changed?(from: 'generating', to: 'finalized') ||
+      status_changed?(from: 'open', to: 'finalized') ||
+      status_changed?(from: 'failed', to: 'finalized')
+  end
 end
 
 # == Schema Information

diff --git a/app/services/invoices/add_on_service.rb b/app/services/invoices/add_on_service.rb
@@ -32,7 +32,6 @@ def create
         result.invoice = invoice
       end
 
-      Invoices::NumberGenerationService.call(invoice: result.invoice)
       Utils::SegmentTrack.invoice_created(result.invoice)
       SendWebhookJob.perform_later('invoice.add_on_added', result.invoice)
       GeneratePdfAndNotifyJob.perform_later(invoice: result.invoice, email: should_deliver_email?)

diff --git a/app/services/invoices/advance_charges_service.rb b/app/services/invoices/advance_charges_service.rb
@@ -20,8 +20,6 @@ def call
 
       invoice = create_group_invoice
 
-      Invoices::NumberGenerationService.call(invoice:) if invoice
-
       if invoice && !invoice.closed?
         SendWebhookJob.perform_later('invoice.created', invoice)
         Invoices::GeneratePdfAndNotifyJob.perform_later(invoice:, email: false)

diff --git a/app/services/invoices/create_one_off_service.rb b/app/services/invoices/create_one_off_service.rb
@@ -40,8 +40,6 @@ def call
         invoice.save!
       end
 
-      Invoices::NumberGenerationService.call(invoice:)
-
       unless invoice.closed?
         Utils::SegmentTrack.invoice_created(invoice)
         SendWebhookJob.perform_later('invoice.one_off_created', invoice)

diff --git a/app/services/invoices/create_pay_in_advance_charge_service.rb b/app/services/invoices/create_pay_in_advance_charge_service.rb
@@ -47,8 +47,6 @@ def call
         invoice.save!
       end
 
-      Invoices::NumberGenerationService.call(invoice:)
-
       unless invoice.closed?
         Utils::SegmentTrack.invoice_created(invoice)
         deliver_webhooks

diff --git a/app/services/invoices/finalize_open_credit_service.rb b/app/services/invoices/finalize_open_credit_service.rb
@@ -20,8 +20,6 @@ def call
         invoice.save!
       end
 
-      Invoices::NumberGenerationService.call(invoice:)
-
       SendWebhookJob.perform_later('invoice.paid_credit_added', result.invoice)
       GeneratePdfAndNotifyJob.perform_later(invoice:, email: should_deliver_email?)
       Integrations::Aggregator::Invoices::CreateJob.perform_later(invoice:) if invoice.should_sync_invoice?

diff --git a/app/services/invoices/number_generation_service.rb b/app/services/invoices/number_generation_service.rb
diff --git a/app/services/invoices/paid_credit_service.rb b/app/services/invoices/paid_credit_service.rb
@@ -30,8 +30,6 @@ def call
         end
       end
 
-      Invoices::NumberGenerationService.call(invoice:)
-
       if invoice.finalized?
         Utils::SegmentTrack.invoice_created(result.invoice)
         SendWebhookJob.perform_later('invoice.paid_credit_added', result.invoice)

diff --git a/app/services/invoices/progressive_billing_service.rb b/app/services/invoices/progressive_billing_service.rb
@@ -45,7 +45,6 @@ def call
 
       # TODO: deduct previous progressive billing invoices
 
-      Invoices::NumberGenerationService.call(invoice:)
       Utils::SegmentTrack.invoice_created(invoice)
       SendWebhookJob.perform_later('invoice.created', invoice)
       Invoices::GeneratePdfAndNotifyJob.perform_later(invoice:, email: should_deliver_email?)

diff --git a/app/services/invoices/refresh_draft_and_finalize_service.rb b/app/services/invoices/refresh_draft_and_finalize_service.rb
@@ -29,8 +29,6 @@ def call
       end
 
       result.invoice = invoice.reload
-      Invoices::NumberGenerationService.call(invoice:)
-
       unless invoice.closed?
         SendWebhookJob.perform_later('invoice.created', invoice)
         GeneratePdfAndNotifyJob.perform_later(invoice:, email: should_deliver_email?)

diff --git a/app/services/invoices/retry_service.rb b/app/services/invoices/retry_service.rb
@@ -40,7 +40,6 @@ def call
         result.invoice = invoice
       end
 
-      Invoices::NumberGenerationService.call(invoice:)
       SendWebhookJob.perform_later('invoice.created', invoice)
       GeneratePdfAndNotifyJob.perform_later(invoice:, email: should_deliver_email?)
       Integrations::Aggregator::Invoices::CreateJob.perform_later(invoice:) if invoice.should_sync_invoice?

diff --git a/app/services/invoices/subscription_service.rb b/app/services/invoices/subscription_service.rb
@@ -46,9 +46,6 @@ def call
         flag_lifetime_usage_for_refresh
         fee_result
       end
-
-      Invoices::NumberGenerationService.call(invoice:)
-
       result.non_invoiceable_fees = fee_result.non_invoiceable_fees
 
       # non-invoiceable fees are created the first time, regardless of grace period.

diff --git a/spec/models/invoice_spec.rb b/spec/models/invoice_spec.rb
@@ -54,7 +54,6 @@
     it 'assigns a sequential id and organization sequential id to a new invoice' do
       invoice.save!
       invoice.finalized!
-      invoice.ensure_invoice_sequential_id
 
       aggregate_failures do
         expect(invoice).to be_valid
@@ -72,7 +71,6 @@
       it 'does not replace the sequential_id and organization_sequential_id' do
         invoice.save!
         invoice.finalized!
-        invoice.ensure_invoice_sequential_id
 
         aggregate_failures do
           expect(invoice).to be_valid
@@ -91,7 +89,6 @@
       it 'takes the next available id' do
         invoice.save!
         invoice.finalized!
-        invoice.ensure_invoice_sequential_id
 
         aggregate_failures do
           expect(invoice).to be_valid
@@ -109,7 +106,6 @@
       it 'scopes the sequence to the organization' do
         invoice.save!
         invoice.finalized!
-        invoice.ensure_invoice_sequential_id
 
         aggregate_failures do
           expect(invoice).to be_valid
@@ -131,8 +127,6 @@
       it 'scopes the organization_sequential_id to the organization and month' do
         invoice.save!
         invoice.finalized!
-        invoice.ensure_invoice_sequential_id
-        invoice.ensure_organization_sequential_id
 
         aggregate_failures do
           expect(invoice).to be_valid
@@ -319,7 +313,7 @@
     end
   end
 
-  describe 'ensure_number' do
+  describe 'number' do
     let(:organization) { create(:organization, name: 'LAGO') }
     let(:customer) { create(:customer, organization:) }
     let(:subscription) { create(:subscription, organization:, customer:) }
@@ -328,8 +322,6 @@
     it 'generates the invoice number' do
       invoice.save!
       invoice.finalized!
-      invoice.ensure_invoice_sequential_id
-      invoice.ensure_number
       organization_id_substring = organization.id.last(4).upcase
 
       expect(invoice.number).to eq("LAG-#{organization_id_substring}-001-001")
@@ -341,9 +333,6 @@
       it 'scopes the organization_sequential_id to the organization and month' do
         invoice.save!
         invoice.finalized!
-        invoice.ensure_invoice_sequential_id
-        invoice.ensure_organization_sequential_id
-        invoice.ensure_number
         organization_id_substring = organization.id.last(4).upcase
 
         expect(invoice.number).to eq("LAG-#{organization_id_substring}-#{Time.now.utc.strftime("%Y%m")}-001")
@@ -360,9 +349,6 @@
         it 'scopes the organization_sequential_id to the organization and month' do
           invoice.save!
           invoice.finalized!
-          invoice.ensure_invoice_sequential_id
-          invoice.ensure_organization_sequential_id
-          invoice.ensure_number
 
           organization_id_substring = organization.id.last(4).upcase
 
@@ -381,9 +367,6 @@
         it 'scopes the organization_sequential_id to the organization and month' do
           invoice.save!
           invoice.finalized!
-          invoice.ensure_invoice_sequential_id
-          invoice.ensure_organization_sequential_id
-          invoice.ensure_number
 
           organization_id_substring = organization.id.last(4).upcase
 
@@ -426,9 +409,6 @@
         it 'scopes the organization_sequential_id to the organization and month' do
           invoice.save!
           invoice.finalized!
-          invoice.ensure_invoice_sequential_id
-          invoice.ensure_organization_sequential_id
-          invoice.ensure_number
 
           organization_id_substring = organization.id.last(4).upcase
 
@@ -440,8 +420,7 @@
           expect(invoice1.reload.number).to eq("LAG-#{organization_id_substring}-#{Time.now.utc.strftime("%Y%m")}-014")
           expect(invoice2.reload.number).to eq("LAG-#{organization_id_substring}-#{Time.now.utc.strftime("%Y%m")}-015")
 
-          invoice.finalized!
-          Invoices::RefreshDraftAndFinalizeService.call(invoice: invoice1)
+          invoice1.finalized!
 
           expect(invoice1.reload.number).to eq("LAG-#{organization_id_substring}-#{Time.now.utc.strftime("%Y%m")}-014")
         end
@@ -482,9 +461,6 @@
         it 'scopes the organization_sequential_id to the organization and month' do
           invoice.save!
           invoice.finalized!
-          invoice.ensure_invoice_sequential_id
-          invoice.ensure_organization_sequential_id
-          invoice.ensure_number
 
           organization_id_substring = organization.id.last(4).upcase
 
@@ -496,8 +472,7 @@
           expect(invoice1.reload.number).to eq("LAG-#{organization_id_substring}-DRAFT")
           expect(invoice2.reload.number).to eq("LAG-#{organization_id_substring}-#{Time.now.utc.strftime("%Y%m")}-014")
 
-          invoice.finalized!
-          Invoices::RefreshDraftAndFinalizeService.call(invoice: invoice1)
+          invoice1.finalized!
 
           expect(invoice1.reload.number).to eq("LAG-#{organization_id_substring}-#{Time.now.utc.strftime("%Y%m")}-016")
         end