(bug) invoice rounding error using coupons

2025-02-20 14:54:15 +01:00 · 2022-10-26 17:35:02 +02:00 · 2022-10-26 17:35:02 +02:00 · a3190115ab
commit a3190115ab
parent fc2b52c2ca
10 changed files with 396 additions and 24 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -5,6 +5,9 @@
 - Fix a bug: portuguese time formatting (#405)
 - Fix a bug: admin users groups being overriden by SSO group_id (#404)
 - Fix a bug: no statistics on trainings and spaces reservations
+- Fix a bug: invalid ventilation for amount coupons
+- Fix a bug: invalid VAT for invoices using amount coupons
+- Fix a bug: invalid 1 cent rounding for invoices using coupons
 - Fix a security issue: updated nokogiri to 1.13.9 to fix [GHSA-2qc6-mcvw-92cw](https://github.com/advisories/GHSA-2qc6-mcvw-92cw)
 - [TODO DEPLOY] `rails fablab:maintenance:regenerate_statistics[2021,6]`
 - [TODO DEPLOY] `rails fablab:setup:set_admins_group`
--- a/app/models/cart_item/machine_reservation.rb
+++ b/app/models/cart_item/machine_reservation.rb
@ -47,6 +47,6 @@ class CartItem::MachineReservation < CartItem::Reservation
    return 0 if @plan.nil?

    machine_credit = @plan.machine_credits.find { |credit| credit.creditable_id == @reservable.id }
-    credits_hours(machine_credit, @new_subscription)
+    credits_hours(machine_credit, new_plan_being_bought: @new_subscription)
  end
 end
--- a/app/models/cart_item/reservation.rb
+++ b/app/models/cart_item/reservation.rb
@ -23,7 +23,7 @@ class CartItem::Reservation < CartItem::BaseItem
    amount = 0

    hours_available = credits
-    grouped_slots.values.each do |slots|
+    grouped_slots.each_value do |slots|
      prices = applicable_prices(slots)
      slots.each_with_index do |slot, index|
        amount += get_slot_price_from_prices(prices, slot, is_privileged,
@ -100,7 +100,7 @@ class CartItem::Reservation < CartItem::BaseItem
    slot_minutes = (slot[:slot_attributes][:end_at].to_time - slot[:slot_attributes][:start_at].to_time) / SECONDS_PER_MINUTE
    price = prices[:prices].find { |p| p[:duration] <= slot_minutes && p[:duration].positive? }
    price = prices[:prices].first if price.nil?
-    hourly_rate = (price[:price].amount.to_f / price[:price].duration) * MINUTES_PER_HOUR
+    hourly_rate = ((Rational(price[:price].amount.to_f) / Rational(price[:price].duration)) * Rational(MINUTES_PER_HOUR)).to_f

    # apply the base price to the real slot duration
    real_price = get_slot_price(hourly_rate, slot, is_privileged, options)
@ -130,7 +130,7 @@ class CartItem::Reservation < CartItem::BaseItem
    slot_minutes = (slot[:slot_attributes][:end_at].to_time - slot[:slot_attributes][:start_at].to_time) / SECONDS_PER_MINUTE
    # apply the base price to the real slot duration
    real_price = if options[:is_division]
-                   (slot_rate / MINUTES_PER_HOUR) * slot_minutes
+                   ((Rational(slot_rate) / Rational(MINUTES_PER_HOUR)) * Rational(slot_minutes)).to_f
                 else
                   slot_rate
                 end
@ -138,7 +138,7 @@ class CartItem::Reservation < CartItem::BaseItem
    if real_price.positive? && options[:prepaid][:minutes]&.positive?
      consumed = slot_minutes
      consumed = options[:prepaid][:minutes] if slot_minutes > options[:prepaid][:minutes]
-      real_price = (slot_minutes - consumed) * (slot_rate / MINUTES_PER_HOUR)
+      real_price = (Rational(slot_minutes - consumed) * (Rational(slot_rate) / Rational(MINUTES_PER_HOUR))).to_f
      options[:prepaid][:minutes] -= consumed
    end

@ -158,7 +158,9 @@ class CartItem::Reservation < CartItem::BaseItem
  # and the base price (1 hours), we use the 7 hours price, then 3 hours price, and finally the base price twice (7+3+1+1 = 12).
  # All these prices are returned to be applied to the reservation.
  def applicable_prices(slots)
-    total_duration = slots.map { |slot| (slot[:slot_attributes][:end_at].to_time - slot[:slot_attributes][:start_at].to_time) / SECONDS_PER_MINUTE }.reduce(:+)
+    total_duration = slots.map do |slot|
+      (slot[:slot_attributes][:end_at].to_time - slot[:slot_attributes][:start_at].to_time) / SECONDS_PER_MINUTE
+    end.reduce(:+)
    rates = { prices: [] }

    remaining_duration = total_duration
@ -182,7 +184,7 @@ class CartItem::Reservation < CartItem::BaseItem
  ##
  # Compute the number of remaining hours in the users current credits (for machine or space)
  ##
-  def credits_hours(credits, new_plan_being_bought = false)
+  def credits_hours(credits, new_plan_being_bought: false)
    return 0 unless credits

    hours_available = credits.hours
--- a/app/models/cart_item/space_reservation.rb
+++ b/app/models/cart_item/space_reservation.rb
@ -41,6 +41,6 @@ class CartItem::SpaceReservation < CartItem::Reservation
    return 0 if @plan.nil?

    space_credit = @plan.space_credits.find { |credit| credit.creditable_id == @reservable.id }
-    credits_hours(space_credit, @new_subscription)
+    credits_hours(space_credit, new_plan_being_bought: @new_subscription)
  end
 end
--- a/app/models/invoice_item.rb
+++ b/app/models/invoice_item.rb
@ -4,8 +4,8 @@
 class InvoiceItem < Footprintable
  belongs_to :invoice

-  has_one :invoice_item # associates invoice_items of an invoice to invoice_items of an Avoir
-  has_one :payment_gateway_object, as: :item
+  has_one :invoice_item, dependent: :destroy # associates invoice_items of an invoice to invoice_items of an Avoir
+  has_one :payment_gateway_object, as: :item, dependent: :destroy

  belongs_to :object, polymorphic: true

@ -15,7 +15,8 @@ class InvoiceItem < Footprintable
  def amount_after_coupon
    # deduct coupon discount
    coupon_service = CouponService.new
-    coupon_service.ventilate(invoice.total, amount, invoice.coupon)
+    total_without_coupon = coupon_service.invoice_total_no_coupon(invoice)
+    coupon_service.ventilate(total_without_coupon, amount, invoice.coupon)
  end

  # return the item amount, coupon discount deducted, if any, and VAT excluded, if applicable
--- a/app/services/coupon_service.rb
+++ b/app/services/coupon_service.rb
@ -26,11 +26,14 @@ class CouponService
    return price if coupon_object.nil?

    if coupon_object.status(user_id, total) == 'active'
-      if coupon_object.type == 'percent_off'
-        price -= (price * coupon_object.percent_off / 100.00).truncate
-      elsif coupon_object.type == 'amount_off'
+      case coupon_object.type
+      when 'percent_off'
+        price -= (Rational(price * coupon_object.percent_off) / Rational(100.0)).to_f.ceil
+      when 'amount_off'
        # do not apply cash coupon unless it has a lower amount that the total price
        price -= coupon_object.amount_off if coupon_object.amount_off <= price
+      else
+        raise InvalidCouponError("unsupported coupon type #{coupon_object.type}")
      end
    end

@ -61,14 +64,15 @@ class CouponService
  def ventilate(total, amount, coupon)
    price = amount
    if !coupon.nil? && total != 0
-      if coupon.type == 'percent_off'
-        price = amount - (amount * coupon.percent_off / 100.00)
-      elsif coupon.type == 'amount_off'
-        ratio = (coupon.amount_off / 100.00) / total
-        discount = (amount * ratio.abs) * 100
-        price = amount - discount
+      case coupon.type
+      when 'percent_off'
+        price = amount - (Rational(amount * coupon.percent_off) / Rational(100.00)).to_f.round
+      when 'amount_off'
+        ratio = Rational(amount) / Rational(total)
+        discount = (coupon.amount_off * ratio.abs)
+        price = (amount - discount).to_f.round
      else
-        raise InvalidCouponError
+        raise InvalidCouponError("unsupported coupon type #{coupon.type}")
      end
    end
    price
--- a/app/services/vat_history_service.rb
+++ b/app/services/vat_history_service.rb
@ -37,7 +37,9 @@ class VatHistoryService

  private

-  def vat_history(vat_rate_type)
+  # This method is really complex and cannot be simplified using the current data model
+  # As a futur improvement, we should save the VAT rate for each invoice_item in the DB
+  def vat_history(vat_rate_type) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
    chronology = []
    end_date = DateTime.current
    Setting.find_by(name: 'invoice_VAT-active').history_values.order(created_at: 'DESC').each do |v|
@ -92,7 +94,7 @@ class VatHistoryService
        # when the VAT rate was enabled, set the date it was enabled and the rate
        range = chronology.find { |p| rate.created_at.to_i.between?(p[:start].to_i, p[:end].to_i) }
        date = range[:enabled] ? rate.created_at : range[:end]
-        date_rates.push(date: date, rate: rate.value.to_i) unless date_rates.find { |d| d[:date] == date }
+        date_rates.push(date: date, rate: rate.value.to_f) unless date_rates.find { |d| d[:date] == date }
      end
      chronology.reverse_each do |period|
        # when the VAT rate was disabled, set the date it was disabled and rate=0
--- a/test/fixtures/plans.yml
+++ b/test/fixtures/plans.yml
@ -51,7 +51,7 @@ plan_3:
  type: Plan
  base_name: Mensuel tarif réduit
  ui_weight: 0
-  interval_count: 1*
+  interval_count: 1
  slug: mensuel-tarif-reduit

 plan_schedulable:
@ -72,3 +72,21 @@ plan_schedulable:
  interval_count: 1
  monthly_payment: true
  slug: abonnement-mensualisable
+
+plan_5:
+  id: 5
+  name: Abonnement crazy fabbers
+  amount: 1498
+  interval: month
+  group_id: 2
+  stp_plan_id: mensuel-tarif-reduit-student-month-20160404171827
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+  training_credit_nb: 1
+  is_rolling:
+  description:
+  type: Plan
+  base_name: Crazy fabbers
+  ui_weight: 1
+  interval_count: 2
+  slug: crazy-fabbers
--- a/test/fixtures/prices.yml
+++ b/test/fixtures/prices.yml
@ -460,3 +460,80 @@ price_54:
  duration: 60
  created_at: 2016-04-04 15:18:28.860220000 Z
  updated_at: 2016-04-04 15:18:50.517702000 Z
+
+price_55:
+  id: 55
+  group_id: 2
+  plan_id: 5
+  priceable_id: 1
+  priceable_type: Machine
+  amount: 121
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+
+price_56:
+  id: 56
+  group_id: 2
+  plan_id: 5
+  priceable_id: 2
+  priceable_type: Machine
+  amount: 1116
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+
+price_57:
+  id: 57
+  group_id: 2
+  plan_id: 5
+  priceable_id: 3
+  priceable_type: Machine
+  amount: 1423
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+
+price_58:
+  id: 58
+  group_id: 2
+  plan_id: 5
+  priceable_id: 4
+  priceable_type: Machine
+  amount: 1238
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+
+price_59:
+  id: 59
+  group_id: 2
+  plan_id: 5
+  priceable_id: 5
+  priceable_type: Machine
+  amount: 724
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+
+price_60:
+  id: 60
+  group_id: 2
+  plan_id: 5
+  priceable_id: 6
+  priceable_type: Machine
+  amount: 666
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
+
+price_61:
+  id: 61
+  group_id: 2
+  plan_id: 5
+  priceable_id: 1
+  priceable_type: Space
+  amount: 1373
+  duration: 60
+  created_at: 2022-10-26 17:08:21.681615000 Z
+  updated_at: 2022-10-26 17:08:21.681615000 Z
--- a/test/integration/invoices/round_test.rb
+++ b/test/integration/invoices/round_test.rb
@ -0,0 +1,265 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+module Invoices; end
+
+class Invoices::RoundTest < ActionDispatch::IntegrationTest
+  def setup
+    @vlonchamp = User.find_by(username: 'vlonchamp')
+    @admin = User.find_by(username: 'admin')
+    login_as(@admin, scope: :user)
+  end
+
+  test 'invoice using percent coupon rounded up' do
+    machine = Machine.first
+    availability = machine.availabilities.first
+    plan = Plan.find(5)
+
+    # enable the VAT
+    Setting.set('invoice_VAT-active', true)
+    Setting.set('invoice_VAT-rate', 20)
+
+    post '/api/local_payment/confirm_payment', params: {
+      customer_id: @vlonchamp.id,
+      coupon_code: 'REDUC20',
+      items: [
+        {
+          reservation: {
+            reservable_id: machine.id,
+            reservable_type: machine.class.name,
+            slots_reservations_attributes: [
+              {
+                slot_id: availability.slots.first.id
+              }
+            ]
+          }
+        },
+        {
+          subscription: {
+            plan_id: plan.id
+          }
+        }
+      ]
+    }.to_json, headers: default_headers
+
+    # Check response format & status
+    assert_equal 201, response.status, response.body
+    assert_equal Mime[:json], response.content_type
+
+    # in the invoice, we should have:
+    # - machine reservation = 121    (97, coupon applied)
+    # - subscription = 1498          (1198, coupon applied)
+    ### intermediate total = 1619
+    # - coupon (20%) = -323.8 => round to 324
+    ### total incl. taxes = 1295
+    # - vat = 216
+    # - total exct. taxes = 1079
+    ### amount paid = 1295
+
+    invoice = Invoice.last
+    assert_equal 121, invoice.invoice_items.first.amount
+    assert_equal 1498, invoice.invoice_items.last.amount
+    assert_equal 1295, invoice.total
+
+    coupon_service = CouponService.new
+    total_without_coupon = coupon_service.invoice_total_no_coupon(invoice)
+    assert_equal 97, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.first.amount, invoice.coupon)
+    assert_equal 1198, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.last.amount, invoice.coupon)
+    assert_equal 324, total_without_coupon - invoice.total
+
+    vat_service = VatHistoryService.new
+    vat_rate_groups = vat_service.invoice_vat(invoice)
+    assert_equal 216, vat_rate_groups.values.pluck(:total_vat).reduce(:+)
+    assert_equal 1079, invoice.invoice_items.map(&:net_amount).reduce(:+)
+  end
+
+  test 'invoice using percent coupon rounded down' do
+    machine = Machine.find(3)
+    availability = machine.availabilities.first
+    plan = Plan.find(5)
+
+    # enable the VAT
+    Setting.set('invoice_VAT-active', true)
+    Setting.set('invoice_VAT-rate', 20)
+
+    post '/api/local_payment/confirm_payment', params: {
+      customer_id: @vlonchamp.id,
+      coupon_code: 'REDUC20',
+      items: [
+        {
+          reservation: {
+            reservable_id: machine.id,
+            reservable_type: machine.class.name,
+            slots_reservations_attributes: [
+              {
+                slot_id: availability.slots.first.id
+              }
+            ]
+          }
+        },
+        {
+          subscription: {
+            plan_id: plan.id
+          }
+        }
+      ]
+    }.to_json, headers: default_headers
+
+    # Check response format & status
+    assert_equal 201, response.status, response.body
+    assert_equal Mime[:json], response.content_type
+
+    # in the invoice, we should have:
+    # - machine reservation = 1423    (1138, coupon applied)
+    # - subscription = 1498          (1198, coupon applied)
+    ### intermediate total = 2921
+    # - coupon (20%) = -584.2 => round to 585
+    ### total incl. taxes = 2336
+    # - vat = 390
+    # - total exct. taxes = 1946
+    ### amount paid = 2336
+
+    invoice = Invoice.last
+    assert_equal 1423, invoice.invoice_items.first.amount
+    assert_equal 1498, invoice.invoice_items.last.amount
+    assert_equal 2336, invoice.total
+
+    coupon_service = CouponService.new
+    total_without_coupon = coupon_service.invoice_total_no_coupon(invoice)
+    assert_equal 1138, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.first.amount, invoice.coupon)
+    assert_equal 1198, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.last.amount, invoice.coupon)
+    assert_equal 585, total_without_coupon - invoice.total
+
+    vat_service = VatHistoryService.new
+    vat_rate_groups = vat_service.invoice_vat(invoice)
+    assert_equal 390, vat_rate_groups.values.pluck(:total_vat).reduce(:+)
+    assert_equal 1946, invoice.invoice_items.map(&:net_amount).reduce(:+)
+  end
+
+  test 'invoice using amount coupon rounded up' do
+    machine = Machine.first
+    availability = machine.availabilities.first
+    plan = Plan.find(5)
+
+    # enable the VAT
+    Setting.set('invoice_VAT-active', true)
+    Setting.set('invoice_VAT-rate', 19.6)
+
+    post '/api/local_payment/confirm_payment', params: {
+      customer_id: @vlonchamp.id,
+      coupon_code: 'GIME3EUR',
+      items: [
+        {
+          reservation: {
+            reservable_id: machine.id,
+            reservable_type: machine.class.name,
+            slots_reservations_attributes: [
+              {
+                slot_id: availability.slots.first.id
+              }
+            ]
+          }
+        },
+        {
+          subscription: {
+            plan_id: plan.id
+          }
+        }
+      ]
+    }.to_json, headers: default_headers
+
+    # Check response format & status
+    assert_equal 201, response.status, response.body
+    assert_equal Mime[:json], response.content_type
+
+    # in the invoice, we should have:
+    # - machine reservation = 121    (99, coupon applied)
+    # - subscription = 1498          (1220, coupon applied)
+    ### intermediate total = 1619
+    # - coupon (20%) = -300
+    ### total incl. taxes = 1319
+    # - vat = 216
+    # - total exct. taxes = 1103
+    ### amount paid = 1319
+
+    invoice = Invoice.last
+    assert_equal 121, invoice.invoice_items.first.amount
+    assert_equal 1498, invoice.invoice_items.last.amount
+    assert_equal 1319, invoice.total
+
+    coupon_service = CouponService.new
+    total_without_coupon = coupon_service.invoice_total_no_coupon(invoice)
+    assert_equal 99, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.first.amount, invoice.coupon)
+    assert_equal 1220, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.last.amount, invoice.coupon)
+    assert_equal 300, total_without_coupon - invoice.total
+
+    vat_service = VatHistoryService.new
+    vat_rate_groups = vat_service.invoice_vat(invoice)
+    assert_equal 216, vat_rate_groups.values.pluck(:total_vat).reduce(:+)
+    assert_equal 1103, invoice.invoice_items.map(&:net_amount).reduce(:+)
+  end
+
+  test 'invoice using amount coupon rounded down' do
+    machine = Machine.find(3)
+    availability = machine.availabilities.first
+    plan = Plan.find(5)
+
+    # enable the VAT
+    Setting.set('invoice_VAT-active', true)
+    Setting.set('invoice_VAT-rate', 20)
+
+    post '/api/local_payment/confirm_payment', params: {
+      customer_id: @vlonchamp.id,
+      coupon_code: 'GIME3EUR',
+      items: [
+        {
+          reservation: {
+            reservable_id: machine.id,
+            reservable_type: machine.class.name,
+            slots_reservations_attributes: [
+              {
+                slot_id: availability.slots.first.id
+              }
+            ]
+          }
+        },
+        {
+          subscription: {
+            plan_id: plan.id
+          }
+        }
+      ]
+    }.to_json, headers: default_headers
+
+    # Check response format & status
+    assert_equal 201, response.status, response.body
+    assert_equal Mime[:json], response.content_type
+
+    # in the invoice, we should have:
+    # - machine reservation = 1423    (1277, coupon applied)
+    # - subscription = 1498          (1344, coupon applied)
+    ### intermediate total = 2921
+    # - coupon (20%) = -300
+    ### total incl. taxes = 2621
+    # - vat = 430
+    # - total exct. taxes = 2191
+    ### amount paid = 2621
+
+    invoice = Invoice.last
+    assert_equal 1423, invoice.invoice_items.first.amount
+    assert_equal 1498, invoice.invoice_items.last.amount
+    assert_equal 2621, invoice.total
+
+    coupon_service = CouponService.new
+    total_without_coupon = coupon_service.invoice_total_no_coupon(invoice)
+    assert_equal 1277, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.first.amount, invoice.coupon)
+    assert_equal 1344, coupon_service.ventilate(total_without_coupon, invoice.invoice_items.last.amount, invoice.coupon)
+    assert_equal 300, total_without_coupon - invoice.total
+
+    vat_service = VatHistoryService.new
+    vat_rate_groups = vat_service.invoice_vat(invoice)
+    assert_equal 437, vat_rate_groups.values.pluck(:total_vat).reduce(:+)
+    assert_equal 2184, invoice.invoice_items.map(&:net_amount).reduce(:+)
+  end
+end