Repository navigation
Prioritize billing intervals when classifying plan changes - #7
Conversation
5c8af0f to
35b1be0
Compare
The PR description says classification must depend on yearly-normalized fees: Lower target fee → upgrade However, the implementation first compares billing intervals: if next_weight > current_weight This means changing from monthly to yearly is always considered an upgrade, regardless of yearly-normalized price. Furthermore, when intervals are equal, the comparison remains in the old direction: plan.yearly_amount_cents <= next_subscription.plan.yearly_amount_cents This classifies a higher target price as an upgrade, whereas the PR says a lower target price must be an upgrade. The same incorrect logic is duplicated in: Subscription#upgraded? and #downgraded? The expected implementation appears to be: def upgraded? next_subscription.plan.yearly_amount_cents <= plan.yearly_amount_cents def downgraded? next_subscription.plan.yearly_amount_cents > plan.yearly_amount_cents Equivalent direction changes should be made consistently in the other services.
The modified model tests change expectations for monthly-to-yearly and yearly-to-monthly transitions. They do not demonstrate that: A lower yearly-normalized target fee is an upgrade. Tests should explicitly use combinations such as: Current monthly yearly-equivalent $1,200; target yearly $1,000 → upgrade. The new ordering: weekly < monthly < quarterly < semiannual < yearly treats a longer commitment interval as inherently “higher,” regardless of price. This is a new classification rule that is not described in the PR and directly conflicts with “continues to compare plans using yearly-normalized fees.” The interval-weight logic should be removed unless this is a separate, explicitly approved requirement.
The LAGO_LICENSE_URL override is unrelated to plan-change classification. It should preferably be moved into a separate PR because it: Changes production configuration behaviour.
The same interval map and comparison are copied into five locations. Even after correcting the rule, this creates a high risk that preview, creation, model state, and pending-subscription processing will diverge later. A shared classifier would be safer, for example: Plans::ChangeClassificationService.call( |
Previously, the billing system determined if a plan change was an upgrade or downgrade solely by comparing the yearly cost (yearly_amount_cents). This did not account for differences in billing intervals where moving to a longer cycle (e.g., monthly to annual) is business-critical to treat as an upgrade regardless of the price differences, and moving to a shorter cycle is treated as a downgrade. Refactor the upgrade and downgrade detection logic to prioritize the duration of the billing interval. Interval weights are mapped from shortest to longest (weekly, monthly, quarterly, semiannual, yearly). - A transition to a longer billing interval is classified as an upgrade. - A transition to a shorter billing interval is classified as a downgrade. - If both plans share the same billing interval, the logic falls back to comparing their yearly amount.
Following the implementation of interval-priority logic for plan upgrades and downgrades, existing spec files checked old behavior where plans with different billing intervals defaulted to price-based logic. Update existing RSpec tests to expect the new interval-priority behavior: - Update unit tests in subscription_spec.rb to expect upgrade/downgrade results based on the change of billing interval. - Update downgrade_spec.rb assertions for monthly-to-yearly transition to align with immediate upgrade behavior instead of a scheduled downgrade.
dbaa25c to
f779ef1
Compare
Sir, these review comments are fixed. |
To classify plan change transitions as upgrades or downgrades, several methods in models and services duplicate the priority logic comparing billing intervals first, and then falling back to yearly amount fees. This duplication increases maintainability risk. Introduce a shared service to encapsulate the classification logic and refactor callers to use it: - Create Plans::ChangeClassificationService to classify transitions using interval weight priority and fallback price comparison. - Refactor Subscription model, Subscriptions::CreateService, Plans::UpdateService, Plans::UpdateAmountService, and Invoices::Preview::SubscriptionPlanChangeService to delegate to the new service. - Add explicit tests to subscription_spec.rb covering interval priority and price fallback combinations.
f779ef1 to
e9e117c
Compare
The consolidated plan-change classification service returned nil when comparing identical plans. This broke boundary and fee logic for transitions to the same plan (such as in daylight saving time and invoice fee calculation tests), which depend on being classified as upgrades. Correct identical plan handling across model and service contexts: - Remove identical plan shortcut from Plans::ChangeClassificationService to allow transitions with equal fees to naturally classify as upgrades. - Restore identical-plan guards inside Subscriptions::CreateService upgrade? and downgrade? methods to prevent triggering redundant creation workflows.
Aditya-Rai-25
changed the title
Previously, the plan update service used a raw comparison of plan amounts (`amount_cents`) to determine if a pending subscription transition had become an upgrade and should be cancelled. This price-based logic conflicted with the new interval-priority classification system, where billing intervals take precedence over price during upgrade and downgrade calculations. - Update the pending subscription cancellation check in `Plans::UpdateService` to delegate classification to the centralized `Plans::ChangeClassificationService`. - Rename the outdated scenario testing a monthly-to-yearly transition to be an upgrade scenario, aligning it with the new interval-priority rules. - Add a new downgrade scenario to verify that a yearly-to-monthly transition remains pending and activates correctly at the end of the billing period. - Add a plan update unit test to verify that valid interval-based downgrades are not incorrectly cancelled during price updates.
0e8f669 to
0015405
Compare
An invalid or newly introduced interval is silently treated as shorter than weekly. For example: unknown → monthly becomes an upgrade That could produce incorrect billing behaviour instead of exposing unsupported input. Prefer fetch and fail explicitly: |
Sir, this issue also fixed now. |
interval_weights is recreated every time the service is called: interval_weights = { Move it to an immutable class constant: INTERVAL_WEIGHTS = { This also makes the business rule easier to locate and update.
The changed integration tests cover monthly/yearly transitions, but the service itself should have a small matrix test covering: weekly → monthly; This would protect the central business rule without depending entirely on larger subscription scenarios. The PR description states that all five intervals are supported, but the added examples mainly exercise monthly and yearly. |
Defining the mapping of intervals to weights inside the service method recreates a hash object every time the service runs. Moving it to a class constant avoids this extra work and keeps the mapping rules in one clear place. Create a frozen class constant `INTERVAL_WEIGHTS` in the service `Plans::ChangeClassificationService`. Update the service to use this constant. Add unit tests to check the classification results for interval changes, price changes, and invalid intervals.
9d9ac73 to
103fa7f
Compare
njonty
left a comment
njonty
left a comment
There was a problem hiding this comment.
- Medium — A large amount of downgrade billing coverage was removed
spec/scenarios/subscriptions/downgrade_spec.rb was reduced from roughly 456 lines to 89 lines. The removed scenarios covered recurring invoices, activation timing, proration, fixed charges and other billing-cycle behaviour. The replacement validates the new yearly-to-monthly classification, but no longer protects much of the existing downgrade lifecycle.
The classification change should ideally update the existing scenario direction while retaining the detailed billing assertions.
Suggested review comment:
This change removes most of the existing downgrade scenario coverage, including recurring billing, activation timing, proration and fixed-charge cases. Could we retain those scenarios and reverse the source/target plan intervals where necessary, rather than replacing the entire file with one simplified case?
- Low — Interval weights duplicate the canonical interval list
The service independently defines all supported billing intervals:
INTERVAL_WEIGHTS = {
"weekly" => 1,
"monthly" => 2,
"quarterly" => 3,
"semiannual" => 4,
"yearly" => 5
}
Plan::INTERVALS separately defines the valid values.
This is correct today, but adding another plan interval later could make valid plans raise KeyError in the classifier unless both definitions are updated. At minimum, a test should verify that the weight map contains exactly all Plan::INTERVALS.
A recent cleanup of subscription transition scenarios unintentionally removed detailed test coverage for monthly downgrade workflows and left duplicate tests in the yearly upgrade scenarios. Additionally, plan billing intervals and their duration weights were duplicated in two different parts of the codebase, presenting a risk of misalignment when introducing new intervals. Restore full billing test coverage for the downgrade workflows by adding back the missing fixed-charges downgrade scenarios. Clean up the yearly upgrade spec file by removing duplicate downgrade test assertions. Unify the plan interval definitions under a single source of truth in the database model, deriving the enum keys automatically and referencing the weights in the classification service.
The order of tax subtotals in generated UBL XML for invoices and credit notes was non-deterministic because the underlying query to fetch fees did not have an ordering clause. This caused the generated subtotal tags to be rendered in different order depending on database row storage order, triggering random flaky test failures in CI. Sort the grouped tax rate subtotals in ascending order by their tax rate key prior to iterating and rendering the subtotal tags. This ensures the XML rendered by the builders is completely deterministic across all runs and environments.
The PR also changes tax groups to be sorted in: app/serializers/e_invoices/credit_notes/common.rb These changes are unrelated to plan-change classification and could alter generated e-invoice/XML ordering. They should be moved to a separate PR, especially since no corresponding serializer tests were added.
The unit test covers adjacent transitions such as monthly → quarterly and semiannual → yearly, but not combinations such as: weekly → yearly The implementation should handle these correctly, but a compact matrix test across every unequal interval pair would protect the complete ordering rule. |
The sorting change was introduced to resolve a flaky CI test failure in this branch: spec/serializers/e_invoices/credit_notes/ubl/builder_spec.rb (specifically: EInvoices::CreditNotes::Ubl::Builder.serialize when TaxSubtotal tag with multiple taxes with one tag per tax rate contains 0.00% rate). Why the change was necessary: The Builder test expects the tax subtotals in the generated XML to be in a specific order (e.g. [1] = 0.00%, [2] = 5.00%, [3] = 10.00%). However, taxes groups fees using resource.fees.group_by(&:taxes_rate). Since the database query to load the association does not contain an ORDER BY clause, PostgreSQL returns rows in physical/disk order (which is non-deterministic). When our restored plan transition tests ran earlier in the suite, they populated and cleared the database tables, leaving behind fragmented page slots. When the builder test subsequently ran, PostgreSQL inserted its new fees into those fragmented slots, resulting in the 10.00% rate being returned first, which broke the test assertions. Why we should keep it in this PR: Determinism: Sorting by the tax rate ensures the generated XML structure is completely deterministic and consistent across all environments, regardless of database fragmentation or query plans. |
The existing unit tests for plan change classification only covered adjacent interval transitions (e.g. monthly to quarterly). They did not verify non-adjacent interval transitions (e.g. weekly to yearly), leaving gaps in our test coverage of the interval hierarchy rules. Refactor the classification service unit tests to dynamically generate a permutation matrix of all unequal plan interval pairs. Assert that the resulting classification correctly identifies upgrades and downgrades based on the weights defined in the Plan model.
6e8a868 to
2091b14
Compare
The implementation currently uses: INTERVAL_WEIGHTS = { INTERVALS = INTERVAL_WEIGHTS.keys.map(&:to_sym).freeze interval is declared as an array-backed Rails enum. Its database representation therefore depends on the array position. The current hash happens to preserve the existing enum order, but a future developer may harmlessly reorder the hash into weight order: weekly, monthly, quarterly, semiannual, yearly That would silently reinterpret existing database values for yearly, quarterly, and semiannual. Recommendation: Keep the persisted enum definition explicit and independent: INTERVALS = %i[ INTERVAL_WEIGHTS = { The classifier can then use: Plan::INTERVAL_WEIGHTS.fetch(current_plan.interval.to_sym) Several concise guard clauses were changed from: return false unless current_subscription to multi-line unless and if blocks. This does not affect correctness, but it creates unnecessary diff noise and makes the substantive change harder to review. Recommendation: Restore the original guard-clause formatting. |
During the classification service refactoring, several concise, single-line guard clauses at the beginning of classification methods were converted to multi-line blocks. This created unnecessary diff noise and deviated from the established codebase style. Restore the original return-unless and return-if inline guard clauses in the Subscription model and CreateService.
Sir, Interval hash map order is preserving the old data history so antigravity is suggesting not to change it because database insertion order of previous data will get wrong. |
Sir, I have restored the original one liner if else blocked to reduced the diff. |
|
During refactoring of the plan change classification scenario tests, several assertions verifying that a pending downgrade request does not immediately generate new invoices were not included. Add invoice count assertions to the downgrade scenario test on November 9th (after the downgrade request is created) to verify that the yearly subscription invoice count and customer invoice count remain 1.
2 participants
Issue: #8
Summary
Updates Lago’s plan-change classification logic to prioritize plan billing cycle durations (intervals), using the yearly-normalized fee only as a secondary tie-breaker.
Previously, Lago classified upgrades and downgrades solely by comparing the yearly-normalized cost (
yearly_amount_cents).This PR implements a two-tiered priority system:
weekly<monthly<quarterly<semiannual<yearly).Key changes
Plan-change behavior
Areas covered
weeklytoyearly)Reviewer Checklist