Membership invoicing, better handling canceling of overcharged invoices

When a membership configuration changed and that existing invoices are
over-charging, we need to cancel the overcharged invoices and create new
ones.

The canceling of overcharged invoices used to be done directly after
any change to the membership configuration, which was not ideal for
multiple step baskets changes that would end up with the same membership
price as before. As this would cancel invoice for nothing.

This patch moves the canceling of overcharged invoices to the recurring
invoice generation, which is a better place for it.

Close #111

app/models/basket.rb

@@ -18,7 +18,6 @@ class Basket < ApplicationRecord
   before_create :add_complements
   before_validation :set_prices
   before_save :set_calculated_price_extra
-  after_commit { membership.cancel_outdated_invoice! }
 
   scope :current_year, -> { joins(:delivery).merge(Delivery.current_year) }
   scope :during_year, ->(year) { joins(:delivery).merge(Delivery.during_year(year)) }

app/models/baskets_basket_complement.rb

@@ -11,8 +11,6 @@ class BasketsBasketComplement < ApplicationRecord
   validates :quantity, numericality: { greater_than_or_equal_to: 0 }, presence: true
   validate :basket_delivery_must_be_in_complement_deliveries
 
-  after_commit { basket.membership.cancel_outdated_invoice! }
-
   before_validation do
     self.price ||= basket_complement&.delivery_price
   end

app/models/billing/invoicer.rb

@@ -58,6 +58,7 @@ def next_date
     def invoice(**attrs)
       return unless billable?
 
+      membership&.cancel_overcharged_invoice!
       I18n.with_locale(member.language) do
         invoice = build_invoice(**attrs)
         invoice.save

app/models/membership.rb

@@ -77,7 +77,7 @@ class Membership < ApplicationRecord
   after_destroy :update_renewal_of_previous_membership_after_deletion, :destroy_or_cancel_invoices!
   after_commit :update_renewal_of_previous_membership_after_creation, on: :create
   after_commit :update_price_and_invoices_amount!, on: %i[create update]
-  after_commit :update_member_and_baskets!, :update_activity_participations_demanded!, :cancel_outdated_invoice!
+  after_commit :update_member_and_baskets!, :update_activity_participations_demanded!
 
   scope :started, -> { where('started_on < ?', Time.current) }
   scope :past, -> { where('ended_on < ?', Time.current) }
@@ -129,7 +129,8 @@ def basket_description(public_name: false)
   end
 
   def billable?
-    fy_year >= Current.fy_year && missing_invoices_amount.positive?
+    fy_year >= Current.fy_year &&
+      (missing_invoices_amount.positive? || overcharged_invoices_amount?)
   end
 
   def trial?
@@ -438,18 +439,21 @@ def update_absent_baskets!
         baskets.between(absence.period).update_all(absent: true)
       end
       update_price_and_invoices_amount!
-      cancel_outdated_invoice!
     end
   end
 
-  def cancel_outdated_invoice!
+  def overcharged_invoices_amount?
+    invoices.not_canceled.any? && invoices_amount > price
+  end
+
+  def cancel_overcharged_invoice!
     return if destroyed?
     return unless current_year?
+    return unless overcharged_invoices_amount?
 
-    if invoices_amount > price && invoices.not_canceled.any?
-      invoices.not_canceled.order(:date).last.destroy_or_cancel!
-      update_price_and_invoices_amount!
-    end
+    invoices.not_canceled.order(:date).last.destroy_or_cancel!
+    update_price_and_invoices_amount!
+    cancel_overcharged_invoice!
   end
 
   private

spec/models/billing/invoicer_spec.rb

@@ -194,6 +194,24 @@ def create_invoice(member, **attrs)
       expect(invoice.memberships_amount_description).to be_present
       expect(invoice.memberships_amount).to eq 2 * 2
     end
+
+    specify 'when already billed, but with a membership change (overcharged)', freeze: '2022-01-01' do
+      create_deliveries(2)
+      overcharged_invoice = create_invoice(member, send_email: true)
+      membership.update!(basket_price: 20)
+
+      invoice = nil
+      expect {
+        invoice = create_invoice(member)
+      }.to change { overcharged_invoice.reload.state }.to('canceled')
+
+      expect(invoice.entity).to eq membership
+      expect(invoice.entity.baskets_count).to eq 2
+      expect(invoice.annual_fee).to eq 30
+      expect(invoice.paid_memberships_amount).to eq 0
+      expect(invoice.memberships_amount_description).to be_present
+      expect(invoice.memberships_amount).to eq 2 * 20
+    end
   end
 
   context 'when billed quarterly' do

spec/models/delivery_spec.rb

@@ -78,11 +78,8 @@
 
     expect { delivery.update!(basket_complement_ids: [1]) }
       .to change { membership_1.reload.price }.by(-4.5)
-      .and change { invoice_1.reload.state }.to('canceled')
       .and change { membership_2.reload.price }.by(-4.5)
 
-    expect(invoice_2.reload.state).to eq('open')
-
     basket1 = delivery.baskets.find_by(membership: membership_1)
     expect(basket1.complement_ids).to match_array [1]
     expect(basket1.complements_price).to eq 3.2

spec/models/membership_spec.rb

@@ -834,7 +834,7 @@
     end
   end
 
-  describe '#cancel_outdated_invoice!' do
+  describe '#cancel_overcharged_invoice!' do
     before do
       Current.acp.update!(
         trial_basket_count: 0,
@@ -852,7 +852,8 @@
       end
       travel_to '2022-05-01' do
         invoice = Billing::Invoicer.force_invoice!(member, send_email: true)
-        expect { membership.update!(ended_on: '2022-05-01') }
+        membership.update!(ended_on: '2022-05-01')
+        expect { membership.cancel_overcharged_invoice! }
           .to change { invoice.reload.state }.from('open').to('canceled')
           .and change { membership.reload.invoices_amount }.to(0)
       end
@@ -871,8 +872,9 @@
       end
       travel_to '2022-09-01' do
         invoice_3 = Billing::Invoicer.force_invoice!(member, send_email: true)
+        membership.update!(baskets_annual_price_change: -11)
         expect {
-          expect { membership.update!(baskets_annual_price_change: -11) }
+          expect { membership.cancel_overcharged_invoice! }
             .to change { invoice_3.reload.state }.from('open').to('canceled')
             .and change { invoice_2.reload.state }.from('open').to('canceled')
             .and change { membership.reload.invoices_amount }.to(10)
@@ -887,7 +889,8 @@
       end
       travel_to '2022-02-01' do
         invoice = Billing::Invoicer.force_invoice!(member, send_email: true)
-        expect { membership.baskets.first.update!(basket_price: 5) }
+        membership.baskets.first.update!(basket_price: 5)
+        expect { membership.cancel_overcharged_invoice! }
           .to change { invoice.reload.state }.from('open').to('canceled')
           .and change { membership.reload.invoices_amount }.to(0)
       end
@@ -904,12 +907,11 @@
       end
       travel_to '2022-02-01' do
         invoice = Billing::Invoicer.force_invoice!(member, send_email: true)
-        expect {
-          create(:absence,
-            member: member,
-            started_on: '2022-05-01',
-            ended_on: '2022-07-01')
-        }
+        create(:absence,
+          member: member,
+          started_on: '2022-05-01',
+          ended_on: '2022-07-01')
+        expect { membership.cancel_overcharged_invoice! }
           .to change { invoice.reload.state }.from('open').to('canceled')
           .and change { membership.reload.invoices_amount }.to(0)
       end
@@ -920,7 +922,8 @@
       membership = create(:membership, member: member)
       invoice = Billing::Invoicer.force_invoice!(member, send_email: true)
       travel_to(Date.new(Current.fy_year + 1, 12, 15)) do
-        expect { membership.baskets.first.update!(basket_price: 5) }
+        membership.baskets.first.update!(basket_price: 5)
+        expect { membership.cancel_overcharged_invoice! }
           .not_to change { membership.reload.invoices_amount }
         expect(invoice.reload.state).to eq('open')
       end
@@ -940,11 +943,11 @@
       travel_to '2022-05-01' do
         invoice = Billing::Invoicer.force_invoice!(member, send_email: true)
         membership.reload
-        expect {
-          membership.update!(memberships_basket_complements_attributes: {
-            '0' => { basket_complement_id: 2, quantity: 1 }
-          })
-        }.not_to change { invoice.reload.state }.from('open')
+        membership.update!(memberships_basket_complements_attributes: {
+          '0' => { basket_complement_id: 2, quantity: 1 }
+        })
+        expect { membership.cancel_overcharged_invoice! }
+          .not_to change { invoice.reload.state }.from('open')
       end
     end
   end