Skip to content

Commit

Permalink
fix(subscription): Make sure terminated invoice always comes with the…
Browse files Browse the repository at this point in the history
… right boundaries (#2561)

## Context

Some termination invoices failed to refresh or are generated with the
wrong boundaries.
This is related to a small mismatch with the time comparison when
looking if the subscription was terminated before the invoice timestamp.

## Description

This PR fixes the `subscription#terminated_at?` comparison so that it
always compare time with the second precision, to avoid side effects
related to rounding when persisting `ActiveRecord` objects.

A lot of `DateTime` objects are also converted into proper `Time.zone`
to fix potential issues around time management as DateTime should not be
used in Lago's context (see
https://docs.datadoghq.com/code_analysis/static_analysis_rules/ruby-best-practices/no-datetime/)
  • Loading branch information
vincent-pochet authored Sep 10, 2024
1 parent 30b31d4 commit 5cf42f1
Show file tree
Hide file tree
Showing 11 changed files with 314 additions and 310 deletions.
4 changes: 2 additions & 2 deletions app/models/invoice.rb
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,8 @@ def recurring_breakdown(fee)
charge: fee.charge,
subscription: fee.subscription,
boundaries: {
from_datetime: DateTime.parse(fee.properties['charges_from_datetime']),
to_datetime: DateTime.parse(fee.properties['charges_to_datetime']),
from_datetime: Time.zone.parse(fee.properties['charges_from_datetime']),
to_datetime: Time.zone.parse(fee.properties['charges_to_datetime']),
charges_duration: fee.properties['charges_duration']
},
filters:
Expand Down
6 changes: 5 additions & 1 deletion app/models/subscription.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,11 @@ def terminated_at?(timestamp)
return false unless terminated?
return false if terminated_at.nil? || timestamp.nil?

terminated_at <= timestamp
# TODO: should be cleaued up to only use Time
timestamp = timestamp.to_time if [Date, DateTime, String].include?(timestamp.class)
timestamp = Time.zone.at(timestamp) if timestamp.is_a?(Integer)

terminated_at.round <= timestamp.round
end
end

Expand Down
2 changes: 1 addition & 1 deletion app/services/fees/create_true_up_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def date_service

@date_service ||= Subscriptions::DatesService.new_instance(
subscription,
boundaries.timestamp || Time.current,
boundaries.timestamp ? Time.zone.parse(boundaries.timestamp) : Time.current,
current_usage: subscription.terminated? && subscription.upgraded?
)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def invoicing_reason_for_subscription(subscription)
# NOTE: upgrading is used as a not persisted reason as it means
# one subscription starting and a second one terminating
return invoicing_reason if invoicing_reason.to_sym != :upgrading
return :subscription_terminating if subscription.terminated? && subscription.terminated_at <= timestamp
return :subscription_terminating if subscription.terminated_at?(timestamp)

:subscription_starting
end
Expand Down
Loading

0 comments on commit 5cf42f1

Please sign in to comment.