Refactor amounts calculation (#1188)

#  Here be dragons 💀

Time to clean the way we calculate amounts, mere mortal

- [x] Kill PfmpAmountCalculator concern (-> `PfmpManager`) 🎯 
- [x] Add new API for updating day_count 🎯 
- [x] Kill after_save callback for Amount recalculation 🎯 
- [x] Add validation on amount not being negative (😮)
- [x] Remove state transition on after save too
- [x] Add lock on Amounts calculation (prevent possible race)
- [x] Encapsulate calculation methods as private methods on the Manager
object

## About the removal of after_save callback check

Why remove it? Because it sucks, amounts ceilings can be exceeded by
spamming requests, rebalancing could happily trigger other rebalancing
(the fact that it doesnt currently is pure fluke), highway to **Callback
Hell** 👿 and also just to simplify the mental model around this
unnecessarily complex process.

Replaced by what? A new method on the `PfmpManager` called `update` ->
`PfmpManager.new(pfmp).update(day_count: X)` with no side effects other
than the one we want which is rebalancing of other pfmps for that same
mef.

### Retrocompatibility issues
What are the entry points of those callbacks? -> AFAIK:
- Pfmp controller update -> replace with new API
- Specs trying to check something -> replace with new API
- In case of Pfmp destruction -> maybe change condition

### Risks
- Not enough tests?
- Potential side effects with callbacks of callbacks that might go
unnoticed?
- Potential negative amounts Pfmps: I reset the ones in the db to 0 (77
records)

app/controllers/classes_controller.rb

@@ -54,7 +54,7 @@ def bulk_pfmp_completion
   def update_bulk_pfmp
     @pfmps = bulk_pfmp_completion_params[:pfmps].map do |pfmp_params|
       @classe.pfmps.find(pfmp_params[:id]).tap do |pfmp|
-        pfmp.day_count = pfmp_params[:day_count]
+        PfmpManager.new(pfmp).update(day_count: pfmp_params[:day_count])
       end
     end
 

app/controllers/pfmps_controller.rb

@@ -20,9 +20,9 @@ def new
   def edit; end
 
   def create
-    @pfmp = Pfmp.new(pfmp_params.merge(schooling: @schooling))
+    @pfmp = Pfmp.new
 
-    if @pfmp.save
+    if PfmpManager.new(@pfmp).update(pfmp_params.merge(schooling: @schooling))
       redirect_to student_path(@schooling.student),
                   notice: t("pfmps.new.success")
     else
@@ -31,7 +31,7 @@ def create
   end
 
   def update
-    if @pfmp.update(pfmp_params)
+    if PfmpManager.new(@pfmp).update(pfmp_params)
       redirect_to school_year_class_schooling_pfmp_path(selected_school_year, @classe, @schooling, @pfmp),
                   notice: t("pfmps.edit.success")
     else

app/models/pfmp.rb

@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 class Pfmp < ApplicationRecord # rubocop:disable Metrics/ClassLength
-  include PfmpAmountCalculator
-
   TRANSITION_CLASS = PfmpTransition
   STATE_MACHINE_CLASS = PfmpStateMachine
   TRANSITION_RELATION_NAME = :transitions
@@ -50,6 +48,8 @@ class Pfmp < ApplicationRecord # rubocop:disable Metrics/ClassLength
             comparison: { greater_than_or_equal_to: :start_date },
             if: -> { start_date && end_date }
 
+  validates :amount, numericality: { only_integer: true, allow_nil: true, greater_than_or_equal_to: 0 }
+
   validates :day_count,
             numericality: {
               only_integer: true,
@@ -66,25 +66,6 @@ class Pfmp < ApplicationRecord # rubocop:disable Metrics/ClassLength
 
   before_destroy :ensure_destroyable?, prepend: true
 
-  after_save do
-    if day_count.present?
-      transition_to!(:completed) if in_state?(:pending)
-    elsif in_state?(:completed, :validated)
-      transition_to!(:pending)
-    end
-  end
-
-  after_save :recalculate_amounts_if_needed
-
-  # Recalculate amounts for the current PFMP and all follow up PFMPs that are still modifiable
-  def recalculate_amounts_if_needed
-    changed_day_count = day_count_before_last_save != day_count
-
-    return if !changed_day_count
-
-    PfmpManager.new(self).recalculate_amounts!
-  end
-
   def validate!
     transition_to!(:validated)
   end
@@ -156,15 +137,24 @@ def can_retrigger_payment?
     latest_payment_request.failed?
   end
 
+  def all_pfmps_for_mef
+    student.pfmps
+           .joins(schooling: :classe)
+           .where("classes.mef_id": mef.id, "classes.school_year_id": school_year.id)
+  end
+
   private
 
   def amounts_yearly_cap
     return unless mef
 
+    pfmps = all_pfmps_for_mef
     cap = mef.wage.yearly_cap
-    total = all_pfmps_for_mef.sum(:amount)
+    total = pfmps.sum(:amount)
     return unless total > cap
 
-    errors.add(:amount, "Yearly cap of #{cap} not respected for Mef code: #{mef.code}")
+    errors.add(:amount,
+               "Yearly cap of #{cap} not respected for Mef code: #{mef.code} \\
+               -> #{total}/#{cap} with #{pfmps.count} PFMPs")
   end
 end

app/models/pfmp_state_machine.rb

@@ -28,15 +28,10 @@ class PfmpStateMachine
   end
 
   after_transition(to: :rectified) do |pfmp|
-    PfmpManager.new(pfmp).recalculate_amounts!
     new_payment_request = PfmpManager.new(pfmp).create_new_payment_request!
     new_payment_request.mark_ready!
   end
 
-  after_transition(to: :completed) do |pfmp|
-    PfmpManager.new(pfmp).recalculate_amounts!
-  end
-
   after_transition(to: :validated) do |pfmp|
     PfmpManager.new(pfmp).create_new_payment_request!
   end

app/services/pfmp_manager.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 # Simple Ruby service to Handle complex actions on PFMPs
-# TODO: refactor PfmpAmountCalculator to be part of this class (requires a lot of spec changes)
 
 class PfmpManager
   class PfmpManagerError < StandardError; end
@@ -15,12 +14,20 @@ def initialize(pfmp)
     @pfmp = pfmp
   end
 
-  def recalculate_amounts!
-    raise PfmpNotModifiableError unless pfmp.can_be_modified?
+  def update(params)
+    update!(params)
+    true
+  rescue ActiveRecord::RecordInvalid
+    false
+  end
 
-    ApplicationRecord.transaction do
-      pfmp.update!(amount: pfmp.calculate_amount)
-      rebalance_following_pfmps!
+  def update!(params)
+    params = params.to_h.with_indifferent_access
+
+    Pfmp.transaction do
+      pfmp.update!(params)
+      recalculate_amounts! if params[:day_count].present?
+      transition!
     end
   end
 
@@ -41,13 +48,20 @@ def retry_incomplete_payment_request!
   end
 
   def rectify_and_update_attributes!(confirmed_pfmp_params, confirmed_address_params)
-    ApplicationRecord.transaction do
-      @pfmp.update!(confirmed_pfmp_params)
-      @pfmp.student.update!(confirmed_address_params)
-      @pfmp.rectify!
+    Pfmp.transaction do
+      update!(confirmed_pfmp_params)
+      pfmp.student.update!(confirmed_address_params)
+      pfmp.rectify!
     end
   end
 
+  def previously_locked_amount(pfmp)
+    other_priced_pfmps(pfmp)
+      .map(&:amount)
+      .compact
+      .sum
+  end
+
   def retry_payment_request!(reasons)
     return unless @pfmp.latest_payment_request&.eligible_for_rejected_or_unpaid_auto_retry?(reasons)
 
@@ -57,9 +71,48 @@ def retry_payment_request!(reasons)
 
   private
 
-  def rebalance_following_pfmps!
-    pfmp.rebalancable_pfmps.each do |pfmp|
-      pfmp.update!(amount: pfmp.calculate_amount)
+  def calculate_amount(target_pfmp)
+    return 0 if target_pfmp.day_count.nil?
+
+    [
+      target_pfmp.day_count * target_pfmp.wage.daily_rate,
+      target_pfmp.wage.yearly_cap - previously_locked_amount(target_pfmp)
+    ].min
+  end
+
+  def recalculate_amounts!
+    raise PfmpNotModifiableError unless pfmp.can_be_modified?
+
+    pfmp.all_pfmps_for_mef.lock!
+    pfmp.update!(amount: calculate_amount(pfmp))
+    rebalance_other_pfmps!
+  end
+
+  def other_pfmps_for_mef(excluded_pfmp)
+    pfmp.all_pfmps_for_mef.excluding(excluded_pfmp)
+  end
+
+  def other_priced_pfmps(pfmp)
+    other_pfmps_for_mef(pfmp)
+      .where.not(amount: nil)
+  end
+
+  def rebalancable_pfmps
+    @rebalancable_pfmps ||= other_pfmps_for_mef(pfmp)
+                            .select(&:can_be_rebalanced?)
+  end
+
+  def rebalance_other_pfmps!
+    rebalancable_pfmps.each do |rebalancable_pfmp|
+      rebalancable_pfmp.update!(amount: calculate_amount(rebalancable_pfmp))
+    end
+  end
+
+  def transition!
+    if pfmp.day_count.present?
+      pfmp.transition_to!(:completed) if pfmp.in_state?(:pending)
+    elsif pfmp.in_state?(:completed, :validated)
+      pfmp.transition_to!(:pending)
     end
   end
 end

app/views/pfmps/_payment_panel.html.haml

@@ -13,5 +13,5 @@
     = number_to_currency pfmp.wage.yearly_cap
     de plafond maximum par an
   %div.fr-mb-1w
-    = number_to_currency pfmp.previously_locked_amount
+    = number_to_currency PfmpManager.new(pfmp).previously_locked_amount(pfmp)
     déjà engagés par d'autres PFMPs sur cette scolarité.

spec/factories/pfmps.rb

@@ -11,7 +11,12 @@
     end
 
     trait :completed do
-      day_count { rand(1..6) } # lovely roll dice
+      transient do
+        day_count { 3 }
+      end
+      after(:create) do |pfmp, eval|
+        PfmpManager.new(pfmp).update!(day_count: eval.day_count)
+      end
     end
 
     trait :can_be_validated do
@@ -44,5 +49,9 @@
         pfmp.reload.rectify!
       end
     end
+
+    after(:build) do |pfmp, _|
+      PfmpManager.new(pfmp).update!(day_count: pfmp.day_count) if pfmp.day_count.present?
+    end
   end
 end