From 5dd2a4235e2cf23207cfee82dbba9c0ca75252df Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 21 Jun 2026 00:58:06 +0200 Subject: [PATCH 1/5] Fable codebase review --- .simplecov | 8 +- CHANGELOG.md | 15 ++ README.md | 17 +- lib/profitable.rb | 17 +- lib/profitable/json_helpers.rb | 11 +- lib/profitable/metrics.rb | 202 +++++++++-------- lib/profitable/mrr_calculator.rb | 48 +--- lib/profitable/processors/base.rb | 10 +- lib/profitable/version.rb | 2 +- test/json_helpers_test.rb | 5 +- test/mrr_calculator_test.rb | 25 +++ test/profitable_test.rb | 123 ++++++----- test/regression_test.rb | 351 ++++++++++++++++++++++++++++++ test/test_helper.rb | 6 + 14 files changed, 624 insertions(+), 216 deletions(-) diff --git a/.simplecov b/.simplecov index 9529c64..1e2d178 100644 --- a/.simplecov +++ b/.simplecov @@ -15,10 +15,10 @@ SimpleCov.start do add_filter "/lib/profitable/engine.rb" add_filter "/app/" - # Exclude the main profitable.rb entry point - it loads the engine and - # defines the Profitable module. Our unit tests use a test-specific - # Profitable module (defined in test_helper.rb) to avoid Rails dependencies. - # The core logic is tested via the individual lib/profitable/*.rb files. + # Exclude the main profitable.rb entry point - it only requires the engine + # and the gem's components. The test harness loads those components directly + # (test_helper.rb) because requiring the engine needs a full Rails app, so + # all core logic in lib/profitable/*.rb runs as real production code paths. add_filter "/lib/profitable.rb" # Track the lib directory (core gem logic) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fe0cb5..b14a342 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # `profitable` +## [0.6.0] - 2026-06-11 + +Accuracy-focused release: every metric was re-audited line by line against the actual data `pay` (7.x–11.x) stores. + +- **Fix canceled trials polluting nearly every flow metric**: a trial that was canceled without ever converting no longer counts in `total_subscribers`, `total_customers`, `new_customers`, `new_subscribers`, `new_mrr`, `churned_customers`, `churned_mrr`, `churn`, or the monthly/daily summaries. A subscription now only counts as ever-billable if it ended *after* billing started +- **Fix survivor bias in `monthly_summary` churn rates**: the churn denominator now includes subscribers who churned later inside the window, so early months no longer overstate churn +- **Fix summaries counting future trial conversions**: a trial scheduled to convert later this month/day no longer appears as a new subscriber (and new MRR) before it actually converts +- **Fix `paid: false` charges counting as revenue on SQLite**: JSON booleans surface as integers on SQLite (vs. text on PostgreSQL/MySQL); JSON extraction is now text-cast so all adapters filter identically (Rails 8 runs SQLite in production) +- **Handle Braintree/Lemon Squeezy statuses**: `expired` now counts as churn (at `ends_at`) instead of billable-forever, and `pending` (future start date, never billed) is excluded everywhere +- **Fix `time_to_next_mrr_milestone` returning a bare String**: it now returns a `NumericResult` like every other metric, so the documented `.to_readable` works (plain string comparisons still behave the same) +- **Correct the processor coverage docs**: `pay` only stores subscription price payloads locally for Stripe, so Braintree/Paddle subscriptions contribute 0 to MRR-style metrics unless the payload is backfilled — the README previously overstated this; charge-based and lifecycle-based metrics remain fully portable across all processors +- DRY consolidation: MRR "billable right now" and "billable at date" are now literally the same query (`MrrCalculator.calculate` delegates to the shared snapshot), churn-event queries are defined once and reused by all metrics and summaries, and `subscription_data` has a single source of truth +- `MrrCalculator.calculate` streams subscriptions in batches (`find_each`) everywhere, keeping memory flat on large datasets +- Test coverage increased to ~98% line / ~91% branch, including regression tests for every bug above, exercised against Pay 7.3–11.x and Rails 7.2–8.1 + ## [0.5.0] - 2026-03-19 - Add `Profitable.ttm_revenue` for trailing twelve-month revenue - Add `Profitable.ttm` as a founder-friendly alias for `ttm_revenue` diff --git a/README.md b/README.md index 2ed55c6..3758164 100644 --- a/README.md +++ b/README.md @@ -28,23 +28,18 @@ Then run `bundle install`. Provided you have a valid [`pay`](https://github.com/pay-rails/pay) installation (`Pay::Customer`, `Pay::Subscription`, `Pay::Charge`, etc.) everything is already set up and you can just start using [`Profitable` methods](#main-profitable-methods) right away. -Current MRR processor coverage is verified for `stripe`, `braintree`, `paddle_billing`, and `paddle_classic`. - For Stripe, metered subscription items are intentionally excluded from fixed run-rate metrics like `mrr`, `arr`, `new_mrr`, and `churned_mrr`. > [!IMPORTANT] -> `profitable` does **not** yet normalize MRR for every processor that `pay` supports. -> If a subscription comes from an unsupported processor such as `lemon_squeezy`, it will currently contribute `0` to processor-adapter-dependent metrics until an adapter is added. +> MRR-style metrics need each subscription's price payload, and `pay` only stores that payload locally for **Stripe** (in the `object` column on Pay v10+, and in `data['subscription_items']` on Pay 7–9). So: > -> Verified processor-adapter coverage today: -> - `stripe` -> - `braintree` -> - `paddle_billing` -> - `paddle_classic` +> - **Subscription amount parsing (full support): `stripe`.** +> - `braintree`, `paddle_billing`, and `paddle_classic` subscriptions contribute `0` to amount-derived metrics out of the box, because `pay` does not persist their price payloads locally. `profitable` ships parsing adapters for them, so they are supported if your app backfills `pay_subscriptions.object` with the processor's subscription payload — otherwise they are counted in subscriber/churn metrics but add `0` MRR (never wrong amounts). +> - Subscriptions from any other processor (e.g. `lemon_squeezy`) also contribute `0` to amount-derived metrics until an adapter is added. > > Metrics that depend on processor-specific subscription amount parsing include `mrr`, `arr`, `new_mrr`, `churned_mrr`, `mrr_growth`, `mrr_growth_rate`, `lifetime_value`, `time_to_next_mrr_milestone`, and MRR-derived fields in summaries. > -> Metrics based primarily on `Pay::Charge` and generic subscription lifecycle fields are much more portable across processors, including `all_time_revenue`, `revenue_in_period`, `ttm_revenue`, `revenue_run_rate`, customer counts, subscriber counts, and churn calculations. +> Metrics based on `Pay::Charge` and generic subscription lifecycle fields are portable across **all** processors, including `all_time_revenue`, `revenue_in_period`, `ttm_revenue`, `revenue_run_rate`, customer counts, subscriber counts, and churn calculations — `pay` stores real amounts and lifecycle dates locally for every processor. ## Mount the `/profitable` dashboard @@ -170,6 +165,8 @@ Revenue methods are net of refunds when `amount_refunded` is present on `pay_cha ### Notes on specific metrics +- **Canceled trials never count.** A subscription only enters subscriber counts, new/churned MRR, churn rates, and the dashboard summaries if it actually started billing before it ended. A free trial that gets canceled (before or at conversion) adds nothing to any metric — it was never revenue, so it is neither a "new subscriber" nor "churn". +- `churn` and `churned_customers`: a churn event happens when billing actually stops (`ends_at`), not when the cancellation is requested. A subscription canceled with a grace period keeps counting in MRR and `active_subscribers` until the grace period runs out. - `mrr_growth_rate`: This calculation compares the MRR at the start and end of the specified period. It assumes a linear growth rate over the period, which may not reflect short-term fluctuations. For more accurate results, consider using shorter periods or implementing a more sophisticated growth calculation method if needed. - `time_to_next_mrr_milestone`: This estimation is based on the current MRR and the recent growth rate. It assumes a constant growth rate, which may not reflect real-world conditions. The calculation may be inaccurate for very new businesses or those with irregular growth patterns. diff --git a/lib/profitable.rb b/lib/profitable.rb index 95f3d1c..cc9d98a 100644 --- a/lib/profitable.rb +++ b/lib/profitable.rb @@ -1,15 +1,20 @@ # frozen_string_literal: true +# Third-party dependencies must load first: NumericResult and the metrics +# module mix in ActionView helpers and use ActiveSupport durations at +# definition time, so the gem cannot rely on the host app (for example an +# API-only Rails app) having loaded these frameworks already. +require "pay" +require "active_support" +require "active_support/time" +require "active_support/core_ext/numeric/conversions" +require "active_support/core_ext/string/filters" +require "action_view" + require_relative "profitable/version" require_relative "profitable/error" require_relative "profitable/engine" - require_relative "profitable/mrr_calculator" require_relative "profitable/numeric_result" require_relative "profitable/json_helpers" - -require "pay" -require "active_support/core_ext/numeric/conversions" -require "action_view" - require_relative "profitable/metrics" diff --git a/lib/profitable/json_helpers.rb b/lib/profitable/json_helpers.rb index 12abbe3..09e913c 100644 --- a/lib/profitable/json_helpers.rb +++ b/lib/profitable/json_helpers.rb @@ -7,8 +7,9 @@ module JsonHelpers VALID_TABLE_COLUMN_PATTERN = /\A[a-zA-Z_][a-zA-Z0-9_.]*\z/ VALID_JSON_KEY_PATTERN = /\A[a-zA-Z_][a-zA-Z0-9_]*\z/ - # Returns the appropriate JSON extraction syntax for the current database adapter - # Supports PostgreSQL, MySQL (5.7.9+), and SQLite + # Returns the appropriate JSON extraction syntax for the current database adapter, + # always yielding a TEXT-typed value so comparisons behave identically everywhere. + # Supports PostgreSQL, MySQL (5.7.9+), and SQLite. # # @param table_column [String] The table and column name (e.g., 'pay_charges.object') # @param json_key [String] The JSON key to extract (e.g., 'paid', 'status') @@ -25,7 +26,7 @@ module JsonHelpers # # @example SQLite # json_extract('pay_charges.object', 'paid') - # # => "json_extract(pay_charges.object, '$.paid')" + # # => "CAST(json_extract(pay_charges.object, '$.paid') AS TEXT)" def json_extract(table_column, json_key) # Validate inputs to prevent SQL injection validate_table_column!(table_column) @@ -41,7 +42,9 @@ def json_extract(table_column, json_key) # We use JSON_UNQUOTE(JSON_EXTRACT()) for maximum compatibility "JSON_UNQUOTE(JSON_EXTRACT(#{table_column}, '$.#{json_key}'))" when /sqlite/ - "json_extract(#{table_column}, '$.#{json_key}')" + # SQLite returns JSON booleans as integers (0/1), unlike ->> on + # PostgreSQL/MySQL which return text. CAST keeps the adapters in sync. + "CAST(json_extract(#{table_column}, '$.#{json_key}') AS TEXT)" else # Fallback to PostgreSQL syntax for unknown adapters Rails.logger.warn("Unknown database adapter '#{adapter}' for JSON extraction. Falling back to PostgreSQL syntax.") diff --git a/lib/profitable/metrics.rb b/lib/profitable/metrics.rb index a8eb879..af3454b 100644 --- a/lib/profitable/metrics.rb +++ b/lib/profitable/metrics.rb @@ -4,9 +4,16 @@ module Profitable # Pay exposes some processor-specific status variants beyond the core generic list. # We normalize them into business-meaningful groups so current-state metrics, # historical event metrics, and churn denominators all behave consistently. + # + # - Trial statuses bill nothing until the trial actually ends (Stripe: trialing, + # Lemon Squeezy: on_trial). + # - Churned statuses stop billing at ends_at (Stripe: canceled, Paddle Classic: + # deleted, Braintree/Lemon Squeezy: expired, legacy Pay: ended/cancelled). + # - Never-billable statuses either failed to start billing (Stripe: incomplete, + # incomplete_expired, unpaid) or have not started billing yet (Braintree: pending). TRIAL_SUBSCRIPTION_STATUSES = ['trialing', 'on_trial'].freeze - CHURNED_STATUSES = ['canceled', 'cancelled', 'ended', 'deleted'].freeze - NEVER_BILLABLE_SUBSCRIPTION_STATUSES = ['incomplete', 'incomplete_expired', 'unpaid'].freeze + CHURNED_STATUSES = ['canceled', 'cancelled', 'ended', 'deleted', 'expired'].freeze + NEVER_BILLABLE_SUBSCRIPTION_STATUSES = ['incomplete', 'incomplete_expired', 'unpaid', 'pending'].freeze class << self include ActionView::Helpers::NumberHelper @@ -63,7 +70,7 @@ def recurring_revenue_percentage(in_the_last: DEFAULT_PERIOD) NumericResult.new(calculate_recurring_revenue_percentage(in_the_last), :percentage) end - def revenue_run_rate(in_the_last: 30.days) + def revenue_run_rate(in_the_last: DEFAULT_PERIOD) NumericResult.new(calculate_revenue_run_rate(in_the_last)) end @@ -83,7 +90,7 @@ def estimated_ttm_revenue_valuation(multiplier = nil, at: nil, multiple: nil) NumericResult.new(calculate_estimated_valuation_from(ttm_revenue.to_i, actual_multiplier)) end - def estimated_revenue_run_rate_valuation(multiplier = nil, at: nil, multiple: nil, in_the_last: 30.days) + def estimated_revenue_run_rate_valuation(multiplier = nil, at: nil, multiple: nil, in_the_last: DEFAULT_PERIOD) actual_multiplier = multiplier || at || multiple || 3 NumericResult.new(calculate_estimated_valuation_from(revenue_run_rate(in_the_last:).to_i, actual_multiplier)) end @@ -148,25 +155,7 @@ def mrr_growth_rate(in_the_last: DEFAULT_PERIOD) end def time_to_next_mrr_milestone - current_mrr = (mrr.to_i) / 100 # Convert cents to dollars - return "Unable to calculate. No MRR yet." if current_mrr <= 0 - - next_milestone = MRR_MILESTONES.find { |milestone| milestone > current_mrr } - return "Congratulations! You've reached the highest milestone." unless next_milestone - - monthly_growth_rate = calculate_mrr_growth_rate / 100 - return "Unable to calculate. Need more data or positive growth." if monthly_growth_rate <= 0 - - # Convert monthly growth rate to daily growth rate - daily_growth_rate = (1 + monthly_growth_rate) ** (1.0 / 30) - 1 - return "Unable to calculate. Growth rate too small." if daily_growth_rate <= 0 - - # Calculate the number of days to reach the next milestone - days_to_milestone = (Math.log(next_milestone.to_f / current_mrr) / Math.log(1 + daily_growth_rate)).ceil - - target_date = Time.current + days_to_milestone.days - - "#{days_to_milestone} days left to $#{number_with_delimiter(next_milestone)} MRR (#{target_date.strftime('%b %d, %Y')})" + NumericResult.new(calculate_time_to_next_mrr_milestone, :string) end def monthly_summary(months: 12) @@ -198,6 +187,16 @@ def subscription_became_billable_at_sql 'COALESCE(pay_subscriptions.trial_ends_at, pay_subscriptions.created_at)' end + # A subscription only ever counts as billable if it ended AFTER billing started. + # Canceled trials never satisfy this: Pay normalizes trial_ends_at down to the + # end date on ended Stripe subscriptions (ends_at == trial_ends_at), and Paddle + # leaves a stale future trial_ends_at (ends_at < trial_ends_at). This single + # predicate keeps never-converted trials out of subscriber counts, new/churned + # MRR, churn rates, and the dashboard summaries. + def subscription_was_billable_before_ending_sql + "(pay_subscriptions.ends_at IS NULL OR pay_subscriptions.ends_at > #{subscription_became_billable_at_sql})" + end + # We intentionally do not reuse Pay::Subscription.active here. # Pay's active scope is access-oriented and can include free-trial access, # while profitable needs billable subscription semantics for metrics. @@ -217,6 +216,7 @@ def subscription_is_billable_by(date, scope = Pay::Subscription.all) "(pay_subscriptions.status != ? OR pay_subscriptions.pause_starts_at IS NOT NULL)", 'paused' ) + .where(subscription_was_billable_before_ending_sql) end # Any subscription that has ever crossed into a paid/billable state, @@ -248,40 +248,58 @@ def billable_subscription_events_in_period(period_start, period_end, scope = Pay .where("#{subscription_became_billable_at_sql} BETWEEN ? AND ?", period_start, period_end) end + # Churn events: subscriptions whose billing actually stopped inside the window. + # Pay stores the moment access/billing ends on ends_at. The billable-before-ending + # guard keeps canceled trials (which never paid) from showing up as churn. + def churned_subscription_events_in_period(period_start, period_end, scope = Pay::Subscription.all) + scope + .where(status: CHURNED_STATUSES) + .where(ends_at: period_start..period_end) + .where(subscription_was_billable_before_ending_sql) + end + def subscription_became_billable_at(subscription) subscription.trial_ends_at || subscription.created_at end + # Full fixed monthly value of every subscription in the scope. + # find_each keeps memory flat on large datasets. + def mrr_sum(scope) + subscriptions_with_processor(scope).find_each.sum do |subscription| + MrrCalculator.process_subscription(subscription) + end + end + def paid_charges - # Pay gem v10+ stores charge data in `object` column, older versions used `data` - # We check both columns for backwards compatibility using database-agnostic JSON extraction + # Stripe charges (the only processor Pay stores raw payloads for) carry + # `paid` and `status` keys we filter on. Charges from other processors have + # neither key, so they pass through the IS NULL branches — Pay only syncs + # their successful transactions in the first place. # # Performance note: The COALESCE pattern may prevent index usage on some databases. # This is an acceptable tradeoff for backwards compatibility with Pay < 10. # For high-volume scenarios, consider adding a composite index or upgrading to Pay 10+ # where only the `object` column is used. + paid = charge_json_value_sql('paid') + status = charge_json_value_sql('status') - # Build JSON extraction SQL for both object and data columns - paid_object = json_extract('pay_charges.object', 'paid') - paid_data = json_extract('pay_charges.data', 'paid') - status_object = json_extract('pay_charges.object', 'status') - status_data = json_extract('pay_charges.data', 'status') - + # JSON booleans surface as 'false'/'true' on PostgreSQL/MySQL but as + # text-cast integers '0'/'1' on SQLite, so we exclude both spellings. Pay::Charge .where("pay_charges.amount > 0") - .where(<<~SQL.squish, 'false', 'succeeded') - ( - (COALESCE(#{paid_object}, #{paid_data}) IS NULL - OR COALESCE(#{paid_object}, #{paid_data}) != ?) - ) + .where(<<~SQL.squish, 'false', '0', 'succeeded') + (#{paid} IS NULL OR #{paid} NOT IN (?, ?)) AND - ( - COALESCE(#{status_object}, #{status_data}) = ? - OR COALESCE(#{status_object}, #{status_data}) IS NULL - ) + (#{status} = ? OR #{status} IS NULL) SQL end + # Pay gem v10+ stores charge payloads in the `object` column, older versions + # used `data`. We check both for backwards compatibility. + def charge_json_value_sql(key) + "COALESCE(#{json_extract('pay_charges.object', key)}, #{json_extract('pay_charges.data', key)})" + end + # Revenue metrics should reflect net cash collected, not gross billed amounts. # When Pay stores refunded cents on the charge, subtract them from revenue. def net_revenue(scope) @@ -297,11 +315,7 @@ def calculate_all_time_revenue end def calculate_arr - (mrr.to_f * 12).round - end - - def calculate_estimated_valuation(multiplier = 3) - calculate_estimated_valuation_from(calculate_arr, multiplier) + mrr.to_i * 12 end def calculate_estimated_valuation_from(base_amount, multiplier = 3) @@ -356,7 +370,7 @@ def calculate_revenue_run_rate(period) def calculate_recurring_revenue_in_period(period) net_revenue( paid_charges - .joins('INNER JOIN pay_subscriptions ON pay_charges.subscription_id = pay_subscriptions.id') + .joins(:subscription) .where(created_at: period.ago..Time.current) ) end @@ -452,17 +466,35 @@ def calculate_mrr_growth_rate(period = DEFAULT_PERIOD) ((end_mrr.to_f - start_mrr) / start_mrr * 100).round(2) end + def calculate_time_to_next_mrr_milestone + current_mrr = (mrr.to_i) / 100 # Convert cents to dollars + return "Unable to calculate. No MRR yet." if current_mrr <= 0 + + next_milestone = MRR_MILESTONES.find { |milestone| milestone > current_mrr } + return "Congratulations! You've reached the highest milestone." unless next_milestone + + monthly_growth_rate = calculate_mrr_growth_rate / 100 + return "Unable to calculate. Need more data or positive growth." if monthly_growth_rate <= 0 + + # Convert monthly growth rate to daily growth rate + daily_growth_rate = (1 + monthly_growth_rate) ** (1.0 / 30) - 1 + return "Unable to calculate. Growth rate too small." if daily_growth_rate <= 0 + + # Calculate the number of days to reach the next milestone + days_to_milestone = (Math.log(next_milestone.to_f / current_mrr) / Math.log(1 + daily_growth_rate)).ceil + + target_date = Time.current + days_to_milestone.days + + "#{days_to_milestone} days left to $#{number_with_delimiter(next_milestone)} MRR (#{target_date.strftime('%b %d, %Y')})" + end + def calculate_mrr_at(date) # Find subscriptions that were active AT the given date: # - Started billing before or on that date # - Not ended before that date (ends_at is nil OR ends_at > date) # - Not paused at that date # - Not still in a free trial at that date - subscriptions_with_processor( - billable_subscription_scope_at(date) - ).sum do |subscription| - MrrCalculator.process_subscription(subscription) - end + mrr_sum(billable_subscription_scope_at(date)) end def calculate_period_data(period) @@ -480,7 +512,7 @@ def calculate_period_data(period) # Churn rate (reuses churned_count) total_at_start = billable_subscription_scope_at(period_start) .distinct - .count('customer_id') + .count(:customer_id) churn_rate = total_at_start > 0 ? (churned_count.to_f / total_at_start * 100).round(1) : 0 { @@ -497,18 +529,17 @@ def calculate_period_data(period) # Batched: loads all data in 5 queries then groups by month in Ruby def calculate_monthly_summary(months_count) overall_start = (months_count - 1).months.ago.beginning_of_month - overall_end = Time.current.end_of_month + # Events cannot exist in the future: a trial scheduled to convert later + # this month is not a new subscriber yet, so the window is capped at now. + overall_end = Time.current # Bulk load all data for the full range, then group in Ruby. # This keeps the dashboard query count low while preserving the same # billable-date semantics used by the single-metric helpers. - new_sub_records = Pay::Subscription - .merge(billable_subscription_events_in_period(overall_start, overall_end)) + new_sub_records = billable_subscription_events_in_period(overall_start, overall_end) .pluck(:customer_id, Arel.sql(subscription_became_billable_at_sql)) - churned_sub_records = Pay::Subscription - .where(status: CHURNED_STATUSES) - .where(ends_at: overall_start..overall_end) + churned_sub_records = churned_subscription_events_in_period(overall_start, overall_end) .pluck(:customer_id, :ends_at) new_mrr_subs = subscriptions_with_processor( @@ -516,14 +547,16 @@ def calculate_monthly_summary(months_count) ).to_a churned_mrr_subs = subscriptions_with_processor( - Pay::Subscription - .where(status: CHURNED_STATUSES) - .where(ends_at: overall_start..overall_end) + churned_subscription_events_in_period(overall_start, overall_end) ).to_a - churn_base_records = billable_subscription_scope_at(overall_end, Pay::Subscription) + # The churn denominator needs every subscription that was billable at each + # month's start — including ones that churned later inside the window — so + # the end-date and pause cutoffs are evaluated per month in Ruby, not in SQL. + churn_base_records = subscription_is_billable_by(overall_end) + .where("#{subscription_became_billable_at_sql} <= ?", overall_end) .where('pay_subscriptions.ends_at IS NULL OR pay_subscriptions.ends_at > ?', overall_start) - .pluck(:customer_id, Arel.sql(subscription_became_billable_at_sql), :ends_at) + .pluck(:customer_id, Arel.sql(subscription_became_billable_at_sql), :ends_at, :pause_starts_at) # Group by month in Ruby using billable-at and ends_at as the event dates, # rather than raw subscription created_at. @@ -533,7 +566,7 @@ def calculate_monthly_summary(months_count) month_end = month_start.end_of_month new_count = new_sub_records - .select { |_, created_at| created_at >= month_start && created_at <= month_end } + .select { |_, became_billable_at| became_billable_at >= month_start && became_billable_at <= month_end } .map(&:first).uniq.count churned_count = churned_sub_records @@ -549,7 +582,11 @@ def calculate_monthly_summary(months_count) .sum { |s| MrrCalculator.process_subscription(s) } total_at_start = churn_base_records - .select { |_, billable_at, ends_at| billable_at < month_start && (ends_at.nil? || ends_at > month_start) } + .select do |_, billable_at, ends_at, pause_starts_at| + billable_at < month_start && + (ends_at.nil? || ends_at > month_start) && + (pause_starts_at.nil? || pause_starts_at > month_start) + end .map(&:first).uniq.count churn_rate = total_at_start > 0 ? (churned_count.to_f / total_at_start * 100).round(1) : 0 @@ -573,17 +610,15 @@ def calculate_monthly_summary(months_count) # Batched: loads all data in 2 queries then groups by day in Ruby def calculate_daily_summary(days_count) overall_start = (days_count - 1).days.ago.beginning_of_day - overall_end = Time.current.end_of_day + # Capped at now so trials scheduled to convert later today do not count yet. + overall_end = Time.current # Daily summary intentionally uses the same "became billable" event date as # new_subscribers/new_mrr, so trial starts do not appear as paid conversions. - new_sub_records = Pay::Subscription - .merge(billable_subscription_events_in_period(overall_start, overall_end)) + new_sub_records = billable_subscription_events_in_period(overall_start, overall_end) .pluck(:customer_id, Arel.sql(subscription_became_billable_at_sql)) - churned_sub_records = Pay::Subscription - .where(status: CHURNED_STATUSES) - .where(ends_at: overall_start..overall_end) + churned_sub_records = churned_subscription_events_in_period(overall_start, overall_end) .pluck(:customer_id, :ends_at) summary = [] @@ -592,7 +627,7 @@ def calculate_daily_summary(days_count) day_end = day_start.end_of_day new_count = new_sub_records - .select { |_, created_at| created_at >= day_start && created_at <= day_end } + .select { |_, became_billable_at| became_billable_at >= day_start && became_billable_at <= day_end } .map(&:first).uniq.count churned_count = churned_sub_records @@ -617,34 +652,21 @@ def calculate_new_subscribers_in_period(period_start, period_end) end def calculate_churned_subscribers_in_period(period_start, period_end) - # Churn happens when access/billing actually ends, which Pay stores on ends_at. - Pay::Subscription - .where(status: CHURNED_STATUSES) - .where(ends_at: period_start..period_end) + churned_subscription_events_in_period(period_start, period_end) .distinct - .count('customer_id') + .count(:customer_id) end def calculate_new_mrr_in_period(period_start, period_end) # New MRR is the full fixed monthly value of subscriptions whose billing # started in the window. It is not prorated, and it still counts if the # subscription churns later in the same period. - subscriptions_with_processor( - billable_subscription_events_in_period(period_start, period_end) - ).sum do |subscription| - MrrCalculator.process_subscription(subscription) - end + mrr_sum(billable_subscription_events_in_period(period_start, period_end)) end def calculate_churned_mrr_in_period(period_start, period_end) # Churned MRR is the full fixed monthly value being lost at churn time. - subscriptions_with_processor( - Pay::Subscription - .where(status: CHURNED_STATUSES) - .where(ends_at: period_start..period_end) - ).sum do |subscription| - MrrCalculator.process_subscription(subscription) - end + mrr_sum(churned_subscription_events_in_period(period_start, period_end)) end def calculate_churn_rate_for_period(period_start, period_end) @@ -652,7 +674,7 @@ def calculate_churn_rate_for_period(period_start, period_end) # This keeps free trials and not-yet-paying subscriptions out of the denominator. total_subscribers_start = billable_subscription_scope_at(period_start) .distinct - .count('customer_id') + .count(:customer_id) churned = calculate_churned_subscribers_in_period(period_start, period_end) return 0 if total_subscribers_start == 0 diff --git a/lib/profitable/mrr_calculator.rb b/lib/profitable/mrr_calculator.rb index af161bf..d194965 100644 --- a/lib/profitable/mrr_calculator.rb +++ b/lib/profitable/mrr_calculator.rb @@ -6,40 +6,11 @@ module Profitable class MrrCalculator + # Current MRR is just the historical MRR snapshot taken right now. + # The billable-subscription query lives in Profitable's metrics module so + # current MRR, MRR-at-date, and growth rates can never drift apart. def self.calculate - total_mrr = 0 - - # Do not use Pay::Subscription.active here. - # Pay's active scope is designed for entitlement/access checks and can include - # free-trial access. MRR needs subscriptions that are billable right now. - subscriptions = Pay::Subscription - .where.not(status: Profitable::NEVER_BILLABLE_SUBSCRIPTION_STATUSES) - .where( - "(pay_subscriptions.status NOT IN (?) OR (pay_subscriptions.trial_ends_at IS NOT NULL AND pay_subscriptions.trial_ends_at <= ?))", - Profitable::TRIAL_SUBSCRIPTION_STATUSES, - Time.current - ) - .where( - "(pay_subscriptions.status NOT IN (?) OR pay_subscriptions.ends_at IS NOT NULL)", - Profitable::CHURNED_STATUSES - ) - .where( - "(pay_subscriptions.status != ? OR pay_subscriptions.pause_starts_at IS NOT NULL)", - 'paused' - ) - .where('COALESCE(pay_subscriptions.trial_ends_at, pay_subscriptions.created_at) <= ?', Time.current) - .where('pay_subscriptions.pause_starts_at IS NULL OR pay_subscriptions.pause_starts_at > ?', Time.current) - .where('pay_subscriptions.ends_at IS NULL OR pay_subscriptions.ends_at > ?', Time.current) - .includes(:customer) - .select('pay_subscriptions.*, pay_customers.processor as customer_processor') - .joins(:customer) - - subscriptions.find_each do |subscription| - mrr = process_subscription(subscription) - total_mrr += mrr if mrr.is_a?(Numeric) && mrr > 0 - end - - total_mrr + Profitable.send(:calculate_mrr_at, Time.current) rescue => e Rails.logger.error("Error calculating total MRR: #{e.message}") raise Profitable::Error, "Failed to calculate MRR: #{e.message}" @@ -62,15 +33,16 @@ def self.process_subscription(subscription) 0 end - # Pay gem v10+ stores Stripe objects in the `object` column, - # while older versions used `data`. This method provides backwards compatibility. def self.subscription_data(subscription) - subscription.try(:object) || subscription.try(:data) + Processors::Base.subscription_data(subscription) end def self.processor_for(processor_name) - # MRR parsing is only implemented for processors with explicit adapters below. - # Unknown processors safely fall back to Base and contribute zero until supported. + # MRR parsing needs the processor's price payload stored locally, which Pay + # only does for Stripe (in `object` on Pay v10+, `data` before that). + # The remaining adapters only apply when that payload has been backfilled by + # the application; otherwise unknown or payload-less subscriptions safely + # contribute zero instead of guessing. case processor_name when 'stripe' Processors::StripeProcessor diff --git a/lib/profitable/processors/base.rb b/lib/profitable/processors/base.rb index 2920c9f..83ef26b 100644 --- a/lib/profitable/processors/base.rb +++ b/lib/profitable/processors/base.rb @@ -3,6 +3,12 @@ module Processors class Base attr_reader :subscription + # Pay gem v10+ stores processor payloads in the `object` column, + # while older versions used `data`. Single source of truth for that fallback. + def self.subscription_data(subscription) + subscription.try(:object) || subscription.try(:data) + end + def initialize(subscription) @subscription = subscription end @@ -13,10 +19,8 @@ def calculate_mrr protected - # Pay gem v10+ stores Stripe objects in the `object` column, - # while older versions used `data`. This method provides backwards compatibility. def subscription_data - subscription.try(:object) || subscription.try(:data) + self.class.subscription_data(subscription) end # Converts a billing amount to its monthly equivalent rate. diff --git a/lib/profitable/version.rb b/lib/profitable/version.rb index 21c95be..b533751 100644 --- a/lib/profitable/version.rb +++ b/lib/profitable/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Profitable - VERSION = "0.5.0" + VERSION = "0.6.0" end diff --git a/test/json_helpers_test.rb b/test/json_helpers_test.rb index ae30c09..641d675 100644 --- a/test/json_helpers_test.rb +++ b/test/json_helpers_test.rb @@ -152,10 +152,11 @@ def test_rejects_json_key_with_special_characters # ============================================================================ def test_generates_sqlite_syntax - # Our test environment uses SQLite + # Our test environment uses SQLite. The CAST keeps JSON booleans (which + # SQLite returns as integers) comparable as text, like PostgreSQL/MySQL. result = @helper.json_extract('pay_charges.object', 'paid') - assert_equal "json_extract(pay_charges.object, '$.paid')", result + assert_equal "CAST(json_extract(pay_charges.object, '$.paid') AS TEXT)", result end # ============================================================================ diff --git a/test/mrr_calculator_test.rb b/test/mrr_calculator_test.rb index b1ca51a..dd63892 100644 --- a/test/mrr_calculator_test.rb +++ b/test/mrr_calculator_test.rb @@ -409,6 +409,31 @@ def test_process_subscription_handles_exceptions_gracefully assert_equal 0, Profitable::MrrCalculator.process_subscription(subscription) end + def test_calculate_wraps_unexpected_errors_in_profitable_error + Profitable.stubs(:calculate_mrr_at).raises(StandardError.new("database exploded")) + + error = assert_raises(Profitable::Error) { Profitable::MrrCalculator.calculate } + + assert_includes error.message, "database exploded" + end + + def test_calculate_matches_the_mrr_at_snapshot_for_now + create_stripe_subscription_v10( + customer: @stripe_customer, + unit_amount: 9900, + interval: "month" + ) + create_braintree_subscription( + customer: @braintree_customer, + price: 3000, + interval: "month" + ) + + # Current MRR and the MRR-at-date snapshot must be the same query; + # if these ever diverge, growth rates stop being trustworthy. + assert_equal Profitable.send(:calculate_mrr_at, Time.current), Profitable::MrrCalculator.calculate + end + # ============================================================================ # SUBSCRIPTION_DATA: BACKWARDS COMPATIBILITY # ============================================================================ diff --git a/test/profitable_test.rb b/test/profitable_test.rb index b38b870..e55f6a1 100644 --- a/test/profitable_test.rb +++ b/test/profitable_test.rb @@ -815,7 +815,7 @@ def test_churned_customers_counts_ended_subscriptions interval: "month", status: "canceled" ) - churned_sub.update!(ends_at: 10.days.ago) + churned_sub.update!(created_at: 45.days.ago, ends_at: 10.days.ago) assert_equal 1, Profitable.churned_customers(in_the_last: 30.days).to_i end @@ -1218,69 +1218,76 @@ def test_monthly_summary_captures_new_subscribers end def test_monthly_summary_uses_trial_conversion_month_for_new_subscribers_and_new_mrr - trial_subscription = create_stripe_subscription_v10( - customer: @customer, - unit_amount: 9900, - interval: "month", - status: "active" - ) - trial_subscription.update!(created_at: 40.days.ago, trial_ends_at: 5.days.ago) - - result = Profitable.monthly_summary(months: 2) - current_month = result.last - previous_month = result.first - - assert_equal 1, current_month[:new_subscribers] - assert_equal 9900, current_month[:new_mrr] - assert_equal 0, previous_month[:new_subscribers] - assert_equal 0, previous_month[:new_mrr] + # Mid-month so the day-based offsets always land in the intended months + travel_to Time.current.beginning_of_month + 14.days do + trial_subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "active" + ) + trial_subscription.update!(created_at: 40.days.ago, trial_ends_at: 5.days.ago) + + result = Profitable.monthly_summary(months: 2) + current_month = result.last + previous_month = result.first + + assert_equal 1, current_month[:new_subscribers] + assert_equal 9900, current_month[:new_mrr] + assert_equal 0, previous_month[:new_subscribers] + assert_equal 0, previous_month[:new_mrr] + end end def test_monthly_summary_captures_churned_subscribers - churned_sub = create_stripe_subscription_v10( - customer: @customer, - unit_amount: 5000, - interval: "month", - status: "canceled" - ) - churned_sub.update!( - created_at: 60.days.ago, - ends_at: 5.days.ago - ) - - result = Profitable.monthly_summary(months: 1) - current_month = result.first - - assert_equal 1, current_month[:churned_subscribers] - assert_equal 5000, current_month[:churned_mrr] + travel_to Time.current.beginning_of_month + 14.days do + churned_sub = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 5000, + interval: "month", + status: "canceled" + ) + churned_sub.update!( + created_at: 60.days.ago, + ends_at: 5.days.ago + ) + + result = Profitable.monthly_summary(months: 1) + current_month = result.first + + assert_equal 1, current_month[:churned_subscribers] + assert_equal 5000, current_month[:churned_mrr] + end end def test_monthly_summary_calculates_net_correctly - # New subscriber this month - create_stripe_subscription_v10( - customer: @customer, - unit_amount: 9900, - interval: "month" - ) - - # Churned subscriber this month - churned_customer = create_customer(processor: "stripe") - churned_sub = create_stripe_subscription_v10( - customer: churned_customer, - unit_amount: 5000, - interval: "month", - status: "canceled" - ) - churned_sub.update!( - created_at: 60.days.ago, - ends_at: 5.days.ago - ) - - result = Profitable.monthly_summary(months: 1) - current_month = result.first - - assert_equal 0, current_month[:net_subscribers] # 1 new - 1 churned - assert_equal 4900, current_month[:net_mrr] # 9900 - 5000 + travel_to Time.current.beginning_of_month + 14.days do + # New subscriber this month + create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month" + ) + + # Churned subscriber this month + churned_customer = create_customer(processor: "stripe") + churned_sub = create_stripe_subscription_v10( + customer: churned_customer, + unit_amount: 5000, + interval: "month", + status: "canceled" + ) + churned_sub.update!( + created_at: 60.days.ago, + ends_at: 5.days.ago + ) + + result = Profitable.monthly_summary(months: 1) + current_month = result.first + + assert_equal 0, current_month[:net_subscribers] # 1 new - 1 churned + assert_equal 4900, current_month[:net_mrr] # 9900 - 5000 + end end def test_monthly_summary_ordered_oldest_first diff --git a/test/regression_test.rb b/test/regression_test.rb index 55e00f7..ddefe50 100644 --- a/test/regression_test.rb +++ b/test/regression_test.rb @@ -510,4 +510,355 @@ def test_bug10_milestone_handles_zero_growth assert_includes message, "Unable to calculate", "BUG #10 REGRESSION: Should handle zero/negative growth gracefully" end + + # =========================================================================== + # BUG #11: Canceled trials counted as subscribers, new MRR, and churn + # =========================================================================== + # + # ORIGINAL BUG: A trial that was canceled before ever converting still ended + # up with `status: canceled` and an `ends_at`. Once its (normalized) trial end + # date passed, every "became billable" check treated it as a paying + # subscription: it inflated total_subscribers, total_customers, new_customers, + # new_subscribers, new_mrr, churned_customers, churned_mrr, churn rate, and + # both summaries — even though the customer never paid a cent. + # + # Pay normalizes `trial_ends_at` down to `ended_at` on ended Stripe + # subscriptions (so canceled trials have ends_at == trial_ends_at), while + # Paddle leaves a stale future trial_ends_at (so ends_at < trial_ends_at). + # + # FIX: A subscription only counts as ever-billable if it actually ended + # AFTER billing started: `ends_at IS NULL OR ends_at > became_billable_at`. + # =========================================================================== + + # Stripe-style canceled trial: Pay normalizes both fields from the same + # processor timestamp, so trial_ends_at == ends_at exactly + def create_stripe_style_canceled_trial(unit_amount: 9900) + trial_ended_at = 10.days.ago + subscription = create_stripe_subscription_v10( + customer: create_customer(processor: "stripe"), + unit_amount: unit_amount, + interval: "month", + status: "canceled" + ) + subscription.update!(created_at: 20.days.ago, trial_ends_at: trial_ended_at, ends_at: trial_ended_at) + subscription + end + + # Paddle-style canceled trial: stale future trial_ends_at, past ends_at + def create_paddle_style_canceled_trial(unit_amount: 9900) + subscription = create_stripe_subscription_v10( + customer: create_customer(processor: "stripe"), + unit_amount: unit_amount, + interval: "month", + status: "canceled" + ) + subscription.update!(created_at: 20.days.ago, trial_ends_at: 5.days.from_now, ends_at: 10.days.ago) + subscription + end + + def test_bug11_canceled_trial_is_not_a_total_subscriber_or_customer + create_stripe_style_canceled_trial + create_paddle_style_canceled_trial + + assert_equal 0, Profitable.total_subscribers.to_i, + "BUG #11 REGRESSION: canceled trials never paid and are not subscribers" + assert_equal 0, Profitable.total_customers.to_i, + "BUG #11 REGRESSION: canceled trials never paid and are not customers" + end + + def test_bug11_canceled_trial_is_not_a_new_subscriber_or_new_customer + create_stripe_style_canceled_trial + + assert_equal 0, Profitable.new_subscribers(in_the_last: 30.days).to_i, + "BUG #11 REGRESSION: canceled trials are not new subscribers" + assert_equal 0, Profitable.new_customers(in_the_last: 30.days).to_i, + "BUG #11 REGRESSION: canceled trials are not new customers" + end + + def test_bug11_canceled_trial_generates_no_new_mrr + create_stripe_style_canceled_trial + + assert_equal 0, Profitable.new_mrr(in_the_last: 30.days).to_i, + "BUG #11 REGRESSION: canceled trials never billed, so they add no new MRR" + end + + def test_bug11_canceled_trial_is_not_churn + create_stripe_style_canceled_trial + create_paddle_style_canceled_trial + + assert_equal 0, Profitable.churned_customers(in_the_last: 30.days).to_i, + "BUG #11 REGRESSION: canceled trials are not churned customers" + assert_equal 0, Profitable.churned_mrr(in_the_last: 30.days).to_i, + "BUG #11 REGRESSION: canceled trials never contributed MRR, so they cannot churn it" + assert_equal 0, Profitable.churn(in_the_last: 30.days).to_f, + "BUG #11 REGRESSION: canceled trials do not belong in the churn numerator" + end + + def test_bug11_canceled_trial_excluded_from_monthly_and_daily_summaries + travel_to Time.current.beginning_of_month + 14.days do + create_stripe_style_canceled_trial + + monthly = Profitable.monthly_summary(months: 1).first + assert_equal 0, monthly[:new_subscribers], "BUG #11 REGRESSION: monthly new subscribers" + assert_equal 0, monthly[:churned_subscribers], "BUG #11 REGRESSION: monthly churned subscribers" + assert_equal 0, monthly[:new_mrr], "BUG #11 REGRESSION: monthly new MRR" + assert_equal 0, monthly[:churned_mrr], "BUG #11 REGRESSION: monthly churned MRR" + + daily = Profitable.daily_summary(days: 30) + assert_equal 0, daily.sum { |d| d[:new_subscribers] }, "BUG #11 REGRESSION: daily new subscribers" + assert_equal 0, daily.sum { |d| d[:churned_subscribers] }, "BUG #11 REGRESSION: daily churned subscribers" + end + end + + def test_bug11_customer_with_paid_charge_and_canceled_trial_is_still_a_customer + # The canceled trial must not count, but the paid one-off charge does. + customer = create_customer(processor: "stripe") + create_successful_charge(customer: customer, amount: 5000) + trial_ended_at = 10.days.ago + subscription = create_stripe_subscription_v10( + customer: customer, + unit_amount: 9900, + interval: "month", + status: "canceled" + ) + subscription.update!(created_at: 20.days.ago, trial_ends_at: trial_ended_at, ends_at: trial_ended_at) + + assert_equal 1, Profitable.total_customers.to_i + assert_equal 0, Profitable.total_subscribers.to_i + end + + def test_bug11_converted_then_canceled_subscription_still_counts_everywhere + # Control case: a trial that converted, paid, and churned later IS a real + # subscriber and real churn. The fix must not swallow genuine churn. + subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "canceled" + ) + subscription.update!(created_at: 60.days.ago, trial_ends_at: 50.days.ago, ends_at: 10.days.ago) + + assert_equal 1, Profitable.total_subscribers.to_i + assert_equal 1, Profitable.churned_customers(in_the_last: 30.days).to_i + assert_equal 9900, Profitable.churned_mrr(in_the_last: 30.days).to_i + end + + # =========================================================================== + # BUG #12: Monthly summary churn rate had survivor bias + # =========================================================================== + # + # ORIGINAL BUG: calculate_monthly_summary built its churn-rate denominator + # from subscriptions billable at the END of the whole window, which excluded + # every subscription that churned inside the window. Early months lost those + # subscribers from their denominators, overstating their churn rates. + # + # FIX: Load billability facts once and evaluate ends_at/pause per month. + # =========================================================================== + + def test_bug12_monthly_churn_rate_includes_mid_window_churns_in_denominator + travel_to Time.current.beginning_of_month + 14.days do + month_zero = Time.current.beginning_of_month + + # Two subscribers since before the window + survivor = create_stripe_subscription_v10(customer: @customer, unit_amount: 9900, interval: "month") + survivor.update!(created_at: month_zero - 4.months) + + churned = create_stripe_subscription_v10( + customer: create_customer(processor: "stripe"), + unit_amount: 9900, + interval: "month", + status: "canceled" + ) + # Churned in the middle of the month two months ago (inside the window) + churned.update!(created_at: month_zero - 4.months, ends_at: month_zero - 2.months + 14.days) + + summary = Profitable.monthly_summary(months: 4) + churn_month = summary.find { |m| m[:month_date] == month_zero - 2.months } + + assert_equal 1, churn_month[:churned_subscribers] + # 1 churned out of 2 billable at month start = 50%, not 100% + assert_in_delta 50.0, churn_month[:churn_rate], 0.01, + "BUG #12 REGRESSION: churn base must include subscribers who churned later in the window" + end + end + + # =========================================================================== + # BUG #13: Summaries counted future trial conversions as already-new + # =========================================================================== + # + # ORIGINAL BUG: monthly_summary/daily_summary queried events up to the end of + # the current month/day (a future timestamp). A subscription still on trial + # whose trial is scheduled to end later this month was already counted as a + # new subscriber and new MRR — even though it may never convert. + # + # FIX: Event windows are capped at Time.current. + # =========================================================================== + + def test_bug13_trial_converting_later_this_month_is_not_a_new_subscriber_yet + travel_to Time.current.beginning_of_month + 10.days do + subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "trialing" + ) + # Trial scheduled to convert in 5 days (still inside the current month) + subscription.update!(created_at: 10.days.ago, trial_ends_at: 5.days.from_now) + + monthly = Profitable.monthly_summary(months: 1).first + assert_equal 0, monthly[:new_subscribers], + "BUG #13 REGRESSION: a future trial conversion is not a new subscriber yet" + assert_equal 0, monthly[:new_mrr], + "BUG #13 REGRESSION: a future trial conversion is not new MRR yet" + + daily = Profitable.daily_summary(days: 7) + assert_equal 0, daily.sum { |d| d[:new_subscribers] }, + "BUG #13 REGRESSION: future conversions must not appear in the daily summary" + end + end + + # =========================================================================== + # BUG #14: time_to_next_mrr_milestone returned a bare String + # =========================================================================== + # + # ORIGINAL BUG: Every other metric returns a Profitable::NumericResult, and + # the README documents `Profitable.time_to_next_mrr_milestone.to_readable`. + # The method returned a plain String, so following the README raised + # NoMethodError. (NumericResult's :string type existed for this but was + # never used.) + # + # FIX: Wrap all return paths in NumericResult.new(message, :string). + # =========================================================================== + + def test_bug14_milestone_returns_numeric_result_with_to_readable + result = Profitable.time_to_next_mrr_milestone + + assert_kind_of Profitable::NumericResult, result, + "BUG #14 REGRESSION: milestone should return a NumericResult like every other metric" + assert_equal "Unable to calculate. No MRR yet.", result.to_readable + end + + def test_bug14_milestone_to_readable_works_on_the_happy_path + old_subscription = create_stripe_subscription_v10(customer: @customer, unit_amount: 10000, interval: "month") + old_subscription.update!(created_at: 60.days.ago) + + create_stripe_subscription_v10( + customer: create_customer(processor: "stripe"), + unit_amount: 10000, + interval: "month" + ) + + result = Profitable.time_to_next_mrr_milestone + + assert_kind_of Profitable::NumericResult, result + assert_includes result.to_readable, "days left to $" + # Still behaves like a string for backwards compatibility + assert_includes result, "MRR" + end + + # =========================================================================== + # BUG #15: paid: false charges counted as revenue on SQLite + # =========================================================================== + # + # ORIGINAL BUG: SQLite's json_extract returns JSON booleans as integers + # (0/1), and `0 != 'false'` is true in SQLite, so a charge with + # `paid: false` and no `status` key passed the paid_charges filter on + # SQLite (but not on PostgreSQL/MySQL, where ->> returns 'false'). + # Rails 8 apps run SQLite in production, so adapters must agree. + # + # FIX: json_extract casts to TEXT on SQLite and paid_charges excludes + # both 'false' and '0'. + # =========================================================================== + + def test_bug15_unpaid_charge_without_status_is_not_revenue + Pay::Charge.create!( + customer: @customer, + processor_id: "ch_unpaid_no_status", + amount: 7000, + currency: "usd", + object: { "paid" => false } + ) + + assert_equal 0, Profitable.all_time_revenue.to_i, + "BUG #15 REGRESSION: paid: false charges are not revenue, regardless of database adapter" + assert_equal 0, Profitable.total_customers.to_i + end + + def test_bug15_paid_charge_without_status_still_counts + Pay::Charge.create!( + customer: @customer, + processor_id: "ch_paid_no_status", + amount: 7000, + currency: "usd", + object: { "paid" => true } + ) + + assert_equal 7000, Profitable.all_time_revenue.to_i + end + + def test_bug15_legacy_data_column_unpaid_charge_is_not_revenue + Pay::Charge.create!( + customer: @customer, + processor_id: "ch_unpaid_legacy", + amount: 7000, + currency: "usd", + data: { "paid" => false } + ) + + assert_equal 0, Profitable.all_time_revenue.to_i + end + + def test_bug15_zero_amount_charges_are_not_revenue_or_customers + create_successful_charge(customer: @customer, amount: 0) + + assert_equal 0, Profitable.all_time_revenue.to_i + assert_equal 0, Profitable.total_customers.to_i + end + + # =========================================================================== + # BUG #16: Braintree/Lemon Squeezy statuses leaked into billable metrics + # =========================================================================== + # + # ORIGINAL BUG: Braintree emits 'expired' (subscription completed its billing + # cycles) and 'pending' (future start date, never billed) statuses; Lemon + # Squeezy also emits 'expired'. Neither was in the status constants, so both + # fell through every guard and counted as billable: expired/pending + # subscriptions inflated MRR-style scopes, active_subscribers, and + # total_subscribers, and expirations never showed up as churn. + # + # FIX: 'expired' joined CHURNED_STATUSES (it ends at ends_at like any other + # churn) and 'pending' joined NEVER_BILLABLE_SUBSCRIPTION_STATUSES. + # =========================================================================== + + def test_bug16_expired_subscription_is_not_billable_and_counts_as_churn + expired = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "expired" + ) + expired.update!(created_at: 60.days.ago, ends_at: 10.days.ago) + + assert_equal 0, Profitable.mrr.to_i, "BUG #16 REGRESSION: expired subscriptions have no MRR" + assert_equal 0, Profitable.active_subscribers.to_i + assert_equal 1, Profitable.total_subscribers.to_i, "expired subscriptions were once billable" + assert_equal 1, Profitable.churned_customers(in_the_last: 30.days).to_i, + "BUG #16 REGRESSION: an expiration is a churn event" + assert_equal 9900, Profitable.churned_mrr(in_the_last: 30.days).to_i + end + + def test_bug16_pending_subscription_is_not_billable_anywhere + create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "pending" + ) + + assert_equal 0, Profitable.mrr.to_i, "BUG #16 REGRESSION: pending subscriptions have not billed yet" + assert_equal 0, Profitable.active_subscribers.to_i + assert_equal 0, Profitable.total_subscribers.to_i + assert_equal 0, Profitable.new_subscribers(in_the_last: 30.days).to_i + assert_equal 0, Profitable.new_mrr(in_the_last: 30.days).to_i + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 11898f7..0ab8c71 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -9,6 +9,7 @@ require "bundler/setup" require "active_record" require "active_support/all" +require "active_support/testing/time_helpers" require "action_view" require "minitest/autorun" require "minitest/mock" @@ -346,6 +347,7 @@ def create_failed_charge(customer:, amount:) class Minitest::Test include ProfitableTestHelpers + include ActiveSupport::Testing::TimeHelpers def setup # Clean up database before each test @@ -353,4 +355,8 @@ def setup Pay::Subscription.delete_all Pay::Customer.delete_all end + + def teardown + travel_back + end end From b4d60817be44ad9d9b8e85ce08444cfa2a42a62a Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 21 Jun 2026 01:09:58 +0200 Subject: [PATCH 2/5] Prepare metric audit fixes for production --- CHANGELOG.md | 7 +- README.md | 2 +- lib/profitable.rb | 1 + lib/profitable/metrics.rb | 31 ++++++-- lib/profitable/numeric_result.rb | 3 + profitable.gemspec | 1 + test/regression_test.rb | 131 +++++++++++++++++++++++++++++++ 7 files changed, 165 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b14a342..826bcda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # `profitable` -## [0.6.0] - 2026-06-11 +## [0.6.0] - 2026-06-21 Accuracy-focused release: every metric was re-audited line by line against the actual data `pay` (7.x–11.x) stores. @@ -10,10 +10,13 @@ Accuracy-focused release: every metric was re-audited line by line against the a - **Fix `paid: false` charges counting as revenue on SQLite**: JSON booleans surface as integers on SQLite (vs. text on PostgreSQL/MySQL); JSON extraction is now text-cast so all adapters filter identically (Rails 8 runs SQLite in production) - **Handle Braintree/Lemon Squeezy statuses**: `expired` now counts as churn (at `ends_at`) instead of billable-forever, and `pending` (future start date, never billed) is excluded everywhere - **Fix `time_to_next_mrr_milestone` returning a bare String**: it now returns a `NumericResult` like every other metric, so the documented `.to_readable` works (plain string comparisons still behave the same) +- **Fix Pay 7-9 schema compatibility for charge filtering**: revenue queries now only reference `pay_charges.object` when that column exists, so legacy `data`-only schemas do not crash +- **Fix paused subscriptions disappearing from lifecycle metrics**: paused subscriptions without a local pause start date remain excluded from current MRR, but still count as customers/subscribers if they had already become billable +- **Fix two small edge cases**: sub-dollar positive MRR no longer reports as "No MRR yet" for milestone messaging, and monthly churn denominators now match the inclusive period-start semantics used by `churn` - **Correct the processor coverage docs**: `pay` only stores subscription price payloads locally for Stripe, so Braintree/Paddle subscriptions contribute 0 to MRR-style metrics unless the payload is backfilled — the README previously overstated this; charge-based and lifecycle-based metrics remain fully portable across all processors - DRY consolidation: MRR "billable right now" and "billable at date" are now literally the same query (`MrrCalculator.calculate` delegates to the shared snapshot), churn-event queries are defined once and reused by all metrics and summaries, and `subscription_data` has a single source of truth - `MrrCalculator.calculate` streams subscriptions in batches (`find_each`) everywhere, keeping memory flat on large datasets -- Test coverage increased to ~98% line / ~91% branch, including regression tests for every bug above, exercised against Pay 7.3–11.x and Rails 7.2–8.1 +- Test coverage increased to ~98% line / ~90% branch, including regression tests for every bug above, exercised against Pay 7.3–11.x and Rails 7.2–8.1 ## [0.5.0] - 2026-03-19 - Add `Profitable.ttm_revenue` for trailing twelve-month revenue diff --git a/README.md b/README.md index 3758164..40d6f78 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ Usually, you would look into your Stripe Dashboard or query the Stripe API to know your MRR / ARR / churn – but when you're using `pay`, you already have that data available and auto synced to your own database. So we can leverage it to make handy, composable ActiveRecord queries that you can reuse in any part of your Rails app (dashboards, internal pages, reports, status messages, etc.) -Think doing something like: `"Current MRR: $#{Profitable.mrr}"` or `"Your app is worth $#{Profitable.valuation_estimate("3x")} at a 3x valuation"` +Think doing something like: `"Current MRR: $#{Profitable.mrr}"` or `"Your app is worth $#{Profitable.estimated_valuation("3x")} at a 3x valuation"` ## Installation diff --git a/lib/profitable.rb b/lib/profitable.rb index cc9d98a..afad448 100644 --- a/lib/profitable.rb +++ b/lib/profitable.rb @@ -4,6 +4,7 @@ # module mix in ActionView helpers and use ActiveSupport durations at # definition time, so the gem cannot rely on the host app (for example an # API-only Rails app) having loaded these frameworks already. +require "rails" require "pay" require "active_support" require "active_support/time" diff --git a/lib/profitable/metrics.rb b/lib/profitable/metrics.rb index af3454b..0e5d384 100644 --- a/lib/profitable/metrics.rb +++ b/lib/profitable/metrics.rb @@ -200,7 +200,7 @@ def subscription_was_billable_before_ending_sql # We intentionally do not reuse Pay::Subscription.active here. # Pay's active scope is access-oriented and can include free-trial access, # while profitable needs billable subscription semantics for metrics. - def subscription_is_billable_by(date, scope = Pay::Subscription.all) + def subscription_has_billable_lifecycle_by(date, scope = Pay::Subscription.all) scope .where.not(status: NEVER_BILLABLE_SUBSCRIPTION_STATUSES) .where( @@ -212,17 +212,21 @@ def subscription_is_billable_by(date, scope = Pay::Subscription.all) "(pay_subscriptions.status NOT IN (?) OR pay_subscriptions.ends_at IS NOT NULL)", CHURNED_STATUSES ) + .where(subscription_was_billable_before_ending_sql) + end + + def subscription_is_billable_by(date, scope = Pay::Subscription.all) + subscription_has_billable_lifecycle_by(date, scope) .where( "(pay_subscriptions.status != ? OR pay_subscriptions.pause_starts_at IS NOT NULL)", 'paused' ) - .where(subscription_was_billable_before_ending_sql) end # Any subscription that has ever crossed into a paid/billable state, # even if it later churned. This is used for "ever" style counts. def ever_billable_subscription_scope(scope = Pay::Subscription.all) - subscription_is_billable_by(Time.current, scope) + subscription_has_billable_lifecycle_by(Time.current, scope) .where("#{subscription_became_billable_at_sql} <= ?", Time.current) end @@ -244,7 +248,7 @@ def current_billable_subscription_scope(scope = Pay::Subscription.all) # Historical "new subscriber" / "new MRR" event window. # The event date is when billing starts, not when the subscription record is created. def billable_subscription_events_in_period(period_start, period_end, scope = Pay::Subscription.all) - subscription_is_billable_by(period_end, scope) + subscription_has_billable_lifecycle_by(period_end, scope) .where("#{subscription_became_billable_at_sql} BETWEEN ? AND ?", period_start, period_end) end @@ -295,9 +299,20 @@ def paid_charges end # Pay gem v10+ stores charge payloads in the `object` column, older versions - # used `data`. We check both for backwards compatibility. + # used `data`. Real Pay 7-9 schemas do not have an `object` column, so only + # reference columns that are actually present in the host app. def charge_json_value_sql(key) - "COALESCE(#{json_extract('pay_charges.object', key)}, #{json_extract('pay_charges.data', key)})" + columns = %w[object data].select { |column| Pay::Charge.column_names.include?(column) } + extractions = columns.map { |column| json_extract("pay_charges.#{column}", key) } + + case extractions.length + when 0 + 'NULL' + when 1 + extractions.first + else + "COALESCE(#{extractions.join(', ')})" + end end # Revenue metrics should reflect net cash collected, not gross billed amounts. @@ -467,7 +482,7 @@ def calculate_mrr_growth_rate(period = DEFAULT_PERIOD) end def calculate_time_to_next_mrr_milestone - current_mrr = (mrr.to_i) / 100 # Convert cents to dollars + current_mrr = mrr.to_f / 100 # Convert cents to dollars return "Unable to calculate. No MRR yet." if current_mrr <= 0 next_milestone = MRR_MILESTONES.find { |milestone| milestone > current_mrr } @@ -583,7 +598,7 @@ def calculate_monthly_summary(months_count) total_at_start = churn_base_records .select do |_, billable_at, ends_at, pause_starts_at| - billable_at < month_start && + billable_at <= month_start && (ends_at.nil? || ends_at > month_start) && (pause_starts_at.nil? || pause_starts_at > month_start) end diff --git a/lib/profitable/numeric_result.rb b/lib/profitable/numeric_result.rb index 99c0f90..4b21498 100644 --- a/lib/profitable/numeric_result.rb +++ b/lib/profitable/numeric_result.rb @@ -1,3 +1,6 @@ +require "delegate" +require "action_view" + module Profitable class NumericResult < SimpleDelegator include ActionView::Helpers::NumberHelper diff --git a/profitable.gemspec b/profitable.gemspec index e4ab64b..a1cb6c1 100644 --- a/profitable.gemspec +++ b/profitable.gemspec @@ -35,6 +35,7 @@ Gem::Specification.new do |spec| spec.add_dependency "pay", ">= 7.0.0" spec.add_dependency "activesupport", ">= 5.2" + spec.add_dependency "actionview", ">= 5.2" # Uncomment to register a new dependency of your gem # spec.add_dependency "example-gem", "~> 1.0" diff --git a/test/regression_test.rb b/test/regression_test.rb index ddefe50..568c777 100644 --- a/test/regression_test.rb +++ b/test/regression_test.rb @@ -808,6 +808,32 @@ def test_bug15_legacy_data_column_unpaid_charge_is_not_revenue assert_equal 0, Profitable.all_time_revenue.to_i end + def test_bug15_legacy_pay_schema_without_object_column_still_counts_data_charges + create_successful_charge_legacy(customer: @customer, amount: 4200) + + legacy_column_names = Pay::Charge.column_names - ["object"] + Pay::Charge.stubs(:column_names).returns(legacy_column_names) + + assert_equal 4200, Profitable.all_time_revenue.to_i, + "BUG #15 REGRESSION: Pay 7-9 schemas do not have pay_charges.object" + end + + def test_bug15_legacy_pay_schema_without_object_column_still_excludes_unpaid_data_charges + Pay::Charge.create!( + customer: @customer, + processor_id: "ch_unpaid_legacy_schema", + amount: 7000, + currency: "usd", + data: { "paid" => false } + ) + + legacy_column_names = Pay::Charge.column_names - ["object"] + Pay::Charge.stubs(:column_names).returns(legacy_column_names) + + assert_equal 0, Profitable.all_time_revenue.to_i, + "BUG #15 REGRESSION: Pay 7-9 charge filtering must not reference object" + end + def test_bug15_zero_amount_charges_are_not_revenue_or_customers create_successful_charge(customer: @customer, amount: 0) @@ -861,4 +887,109 @@ def test_bug16_pending_subscription_is_not_billable_anywhere assert_equal 0, Profitable.new_subscribers(in_the_last: 30.days).to_i assert_equal 0, Profitable.new_mrr(in_the_last: 30.days).to_i end + + # =========================================================================== + # BUG #17: Paused subscriptions disappeared from ever/new subscriber metrics + # =========================================================================== + # + # ORIGINAL BUG: The shared "billable" predicate excluded `status: paused` + # subscriptions unless `pause_starts_at` was present. That is correct for + # current MRR, but too strict for lifecycle metrics: Lemon Squeezy can store + # paused subscriptions without a local pause start date, and those customers + # still became subscribers before pausing. + # + # FIX: Split "has this subscription ever crossed into a billable lifecycle?" + # from "is this subscription billable at this exact date?" + # =========================================================================== + + def test_bug17_paused_subscription_without_pause_date_still_counts_as_ever_subscriber + subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "paused" + ) + subscription.update!(created_at: 10.days.ago, pause_starts_at: nil) + + assert_equal 0, Profitable.mrr.to_i, + "BUG #17 REGRESSION: paused subscriptions are not current MRR" + assert_equal 0, Profitable.active_subscribers.to_i, + "BUG #17 REGRESSION: paused subscriptions are not active subscribers" + assert_equal 1, Profitable.total_subscribers.to_i, + "BUG #17 REGRESSION: paused subscriptions still became subscribers" + assert_equal 1, Profitable.total_customers.to_i, + "BUG #17 REGRESSION: paused subscribers are monetized customers" + end + + def test_bug17_paused_subscription_without_pause_date_still_counts_as_new_subscriber + subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "paused" + ) + subscription.update!(created_at: 10.days.ago, pause_starts_at: nil) + + assert_equal 1, Profitable.new_subscribers(in_the_last: 30.days).to_i, + "BUG #17 REGRESSION: pause state must not erase the original conversion" + assert_equal 9900, Profitable.new_mrr(in_the_last: 30.days).to_i, + "BUG #17 REGRESSION: new MRR is the subscription's full conversion MRR" + end + + # =========================================================================== + # BUG #18: Sub-dollar MRR was treated as no MRR in milestone messaging + # =========================================================================== + # + # ORIGINAL BUG: `time_to_next_mrr_milestone` converted cents to dollars with + # integer division. Positive MRR below $1 became 0, so the method returned + # "No MRR yet" even though there was real recurring revenue. + # + # FIX: Convert cents to a float dollar amount before choosing the next + # milestone and checking for zero MRR. + # =========================================================================== + + def test_bug18_sub_dollar_mrr_is_not_reported_as_no_mrr + create_stripe_subscription_v10( + customer: @customer, + unit_amount: 50, + interval: "month" + ) + + result = Profitable.time_to_next_mrr_milestone.to_readable + + refute_equal "Unable to calculate. No MRR yet.", result, + "BUG #18 REGRESSION: positive MRR below $1 is still MRR" + assert_equal "Unable to calculate. Need more data or positive growth.", result + end + + # =========================================================================== + # BUG #19: Monthly churn base excluded subscriptions billable at month start + # =========================================================================== + # + # ORIGINAL BUG: `monthly_summary` used `billable_at < month_start` for churn + # denominators, while the canonical churn helper uses inclusive period starts. + # A subscriber whose paid access started exactly at midnight on the first day + # of the month could churn later that same month but be absent from the base. + # + # FIX: Monthly churn denominators use `billable_at <= month_start`. + # =========================================================================== + + def test_bug19_monthly_churn_base_includes_subscribers_billable_exactly_at_month_start + travel_to Time.current.beginning_of_month + 14.days do + month_start = Time.current.beginning_of_month + subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "canceled" + ) + subscription.update!(created_at: month_start, ends_at: month_start + 7.days) + + month = Profitable.monthly_summary(months: 1).first + + assert_equal 1, month[:churned_subscribers] + assert_in_delta 100.0, month[:churn_rate], 0.01, + "BUG #19 REGRESSION: month-start subscribers belong in the starting base" + end + end end From 57db5db18eaad83875782a39a2b6b515c548d48d Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 21 Jun 2026 01:22:49 +0200 Subject: [PATCH 3/5] Address automated metric audit review --- lib/profitable.rb | 9 +++------ lib/profitable/metrics.rb | 28 +++++++++++++++++----------- lib/profitable/mrr_calculator.rb | 2 +- test/mrr_calculator_test.rb | 2 +- test/regression_test.rb | 8 +++----- 5 files changed, 25 insertions(+), 24 deletions(-) diff --git a/lib/profitable.rb b/lib/profitable.rb index afad448..d064b41 100644 --- a/lib/profitable.rb +++ b/lib/profitable.rb @@ -1,12 +1,9 @@ # frozen_string_literal: true -# Third-party dependencies must load first: NumericResult and the metrics -# module mix in ActionView helpers and use ActiveSupport durations at -# definition time, so the gem cannot rely on the host app (for example an -# API-only Rails app) having loaded these frameworks already. -require "rails" +# Third-party dependencies must load before the metrics module: NumericResult +# mixes in ActionView helpers and metrics use ActiveSupport durations at +# definition time. require "pay" -require "active_support" require "active_support/time" require "active_support/core_ext/numeric/conversions" require "active_support/core_ext/string/filters" diff --git a/lib/profitable/metrics.rb b/lib/profitable/metrics.rb index 0e5d384..007ad2c 100644 --- a/lib/profitable/metrics.rb +++ b/lib/profitable/metrics.rb @@ -170,6 +170,18 @@ def period_data(in_the_last: DEFAULT_PERIOD) calculate_period_data(in_the_last) end + # Historical MRR snapshot in cents. + # Exposed for MrrCalculator so current MRR and historical MRR use exactly + # the same query without reaching through private methods with `send`. + def calculate_mrr_at(date) + # Find subscriptions that were active AT the given date: + # - Started billing before or on that date + # - Not ended before that date (ends_at is nil OR ends_at > date) + # - Not paused at that date + # - Not still in a free trial at that date + mrr_sum(billable_subscription_scope_at(date)) + end + private # Helper to load subscriptions with processor info from customer @@ -302,8 +314,7 @@ def paid_charges # used `data`. Real Pay 7-9 schemas do not have an `object` column, so only # reference columns that are actually present in the host app. def charge_json_value_sql(key) - columns = %w[object data].select { |column| Pay::Charge.column_names.include?(column) } - extractions = columns.map { |column| json_extract("pay_charges.#{column}", key) } + extractions = charge_payload_columns.map { |column| json_extract("pay_charges.#{column}", key) } case extractions.length when 0 @@ -315,6 +326,10 @@ def charge_json_value_sql(key) end end + def charge_payload_columns + @charge_payload_columns ||= %w[object data].select { |column| Pay::Charge.column_names.include?(column) } + end + # Revenue metrics should reflect net cash collected, not gross billed amounts. # When Pay stores refunded cents on the charge, subtract them from revenue. def net_revenue(scope) @@ -503,15 +518,6 @@ def calculate_time_to_next_mrr_milestone "#{days_to_milestone} days left to $#{number_with_delimiter(next_milestone)} MRR (#{target_date.strftime('%b %d, %Y')})" end - def calculate_mrr_at(date) - # Find subscriptions that were active AT the given date: - # - Started billing before or on that date - # - Not ended before that date (ends_at is nil OR ends_at > date) - # - Not paused at that date - # - Not still in a free trial at that date - mrr_sum(billable_subscription_scope_at(date)) - end - def calculate_period_data(period) period_start = period.ago period_end = Time.current diff --git a/lib/profitable/mrr_calculator.rb b/lib/profitable/mrr_calculator.rb index d194965..75e0146 100644 --- a/lib/profitable/mrr_calculator.rb +++ b/lib/profitable/mrr_calculator.rb @@ -10,7 +10,7 @@ class MrrCalculator # The billable-subscription query lives in Profitable's metrics module so # current MRR, MRR-at-date, and growth rates can never drift apart. def self.calculate - Profitable.send(:calculate_mrr_at, Time.current) + Profitable.calculate_mrr_at(Time.current) rescue => e Rails.logger.error("Error calculating total MRR: #{e.message}") raise Profitable::Error, "Failed to calculate MRR: #{e.message}" diff --git a/test/mrr_calculator_test.rb b/test/mrr_calculator_test.rb index dd63892..419b9fc 100644 --- a/test/mrr_calculator_test.rb +++ b/test/mrr_calculator_test.rb @@ -431,7 +431,7 @@ def test_calculate_matches_the_mrr_at_snapshot_for_now # Current MRR and the MRR-at-date snapshot must be the same query; # if these ever diverge, growth rates stop being trustworthy. - assert_equal Profitable.send(:calculate_mrr_at, Time.current), Profitable::MrrCalculator.calculate + assert_equal Profitable.calculate_mrr_at(Time.current), Profitable::MrrCalculator.calculate end # ============================================================================ diff --git a/test/regression_test.rb b/test/regression_test.rb index 568c777..69e3328 100644 --- a/test/regression_test.rb +++ b/test/regression_test.rb @@ -249,7 +249,7 @@ def test_bug5_mrr_at_calculates_historical_state new_sub.update!(created_at: 15.days.ago) # MRR at 45 days ago should include churned_sub but NOT new_sub - historical_mrr = Profitable.send(:calculate_mrr_at, 45.days.ago) + historical_mrr = Profitable.calculate_mrr_at(45.days.ago) assert_equal 5000, historical_mrr, "BUG #5 REGRESSION: Should calculate MRR at historical date" end @@ -811,8 +811,7 @@ def test_bug15_legacy_data_column_unpaid_charge_is_not_revenue def test_bug15_legacy_pay_schema_without_object_column_still_counts_data_charges create_successful_charge_legacy(customer: @customer, amount: 4200) - legacy_column_names = Pay::Charge.column_names - ["object"] - Pay::Charge.stubs(:column_names).returns(legacy_column_names) + Profitable.stubs(:charge_payload_columns).returns(["data"]) assert_equal 4200, Profitable.all_time_revenue.to_i, "BUG #15 REGRESSION: Pay 7-9 schemas do not have pay_charges.object" @@ -827,8 +826,7 @@ def test_bug15_legacy_pay_schema_without_object_column_still_excludes_unpaid_dat data: { "paid" => false } ) - legacy_column_names = Pay::Charge.column_names - ["object"] - Pay::Charge.stubs(:column_names).returns(legacy_column_names) + Profitable.stubs(:charge_payload_columns).returns(["data"]) assert_equal 0, Profitable.all_time_revenue.to_i, "BUG #15 REGRESSION: Pay 7-9 charge filtering must not reference object" From 2819ccb852e7d908c583ef070fb1b6223562f6c3 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 21 Jun 2026 01:33:09 +0200 Subject: [PATCH 4/5] Address follow-up automated review --- CHANGELOG.md | 3 ++- README.md | 4 ++++ lib/profitable/metrics.rb | 34 ++++++++++++++++++-------------- lib/profitable/mrr_calculator.rb | 2 +- test/mrr_calculator_test.rb | 4 ++-- test/profitable_test.rb | 26 ++++++++++++++++++++++++ test/regression_test.rb | 16 ++++++++++++++- 7 files changed, 69 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 826bcda..f9c6776 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # `profitable` -## [0.6.0] - 2026-06-21 +## [0.6.0] - 2026-06-20 Accuracy-focused release: every metric was re-audited line by line against the actual data `pay` (7.x–11.x) stores. @@ -13,6 +13,7 @@ Accuracy-focused release: every metric was re-audited line by line against the a - **Fix Pay 7-9 schema compatibility for charge filtering**: revenue queries now only reference `pay_charges.object` when that column exists, so legacy `data`-only schemas do not crash - **Fix paused subscriptions disappearing from lifecycle metrics**: paused subscriptions without a local pause start date remain excluded from current MRR, but still count as customers/subscribers if they had already become billable - **Fix two small edge cases**: sub-dollar positive MRR no longer reports as "No MRR yet" for milestone messaging, and monthly churn denominators now match the inclusive period-start semantics used by `churn` +- Add `Profitable.mrr_at(date)` as the public historical MRR snapshot API, keeping the raw `calculate_mrr_at` helper private - **Correct the processor coverage docs**: `pay` only stores subscription price payloads locally for Stripe, so Braintree/Paddle subscriptions contribute 0 to MRR-style metrics unless the payload is backfilled — the README previously overstated this; charge-based and lifecycle-based metrics remain fully portable across all processors - DRY consolidation: MRR "billable right now" and "billable at date" are now literally the same query (`MrrCalculator.calculate` delegates to the shared snapshot), churn-event queries are defined once and reused by all metrics and summaries, and `subscription_data` has a single source of truth - `MrrCalculator.calculate` streams subscriptions in batches (`find_each`) everywhere, keeping memory flat on large datasets diff --git a/README.md b/README.md index 40d6f78..d9adbc0 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,7 @@ All methods return numbers that can be converted to a nicely-formatted, human-re ### Revenue metrics - `Profitable.mrr`: Monthly Recurring Revenue (MRR) from subscriptions that are billable right now +- `Profitable.mrr_at(date)`: Historical MRR snapshot from subscriptions that were billable at the given date - `Profitable.arr`: Annual Recurring Revenue (ARR), calculated as current `mrr * 12`, not trailing revenue - `Profitable.ttm`: Founder-friendly shorthand alias for `ttm_revenue` - `Profitable.ttm_revenue`: Trailing twelve-month revenue, net of refunds when `amount_refunded` is present @@ -113,6 +114,9 @@ All methods return numbers that can be converted to a nicely-formatted, human-re # Get the current MRR Profitable.mrr.to_readable # => "$1,234" +# Get the MRR snapshot for a historical date +Profitable.mrr_at(30.days.ago).to_readable # => "$987" + # Get the number of new customers in the last 60 days Profitable.new_customers(in_the_last: 60.days).to_readable # => "42" diff --git a/lib/profitable/metrics.rb b/lib/profitable/metrics.rb index 007ad2c..1261864 100644 --- a/lib/profitable/metrics.rb +++ b/lib/profitable/metrics.rb @@ -29,6 +29,11 @@ def mrr NumericResult.new(MrrCalculator.calculate) end + # Historical MRR snapshot from subscriptions that were billable at the given date. + def mrr_at(date) + NumericResult.new(calculate_mrr_at(date)) + end + # Annual Recurring Revenue (ARR) based on the current recurring base. # This is today's MRR annualized, not historical 12-month revenue. def arr @@ -170,18 +175,6 @@ def period_data(in_the_last: DEFAULT_PERIOD) calculate_period_data(in_the_last) end - # Historical MRR snapshot in cents. - # Exposed for MrrCalculator so current MRR and historical MRR use exactly - # the same query without reaching through private methods with `send`. - def calculate_mrr_at(date) - # Find subscriptions that were active AT the given date: - # - Started billing before or on that date - # - Not ended before that date (ends_at is nil OR ends_at > date) - # - Not paused at that date - # - Not still in a free trial at that date - mrr_sum(billable_subscription_scope_at(date)) - end - private # Helper to load subscriptions with processor info from customer @@ -202,9 +195,11 @@ def subscription_became_billable_at_sql # A subscription only ever counts as billable if it ended AFTER billing started. # Canceled trials never satisfy this: Pay normalizes trial_ends_at down to the # end date on ended Stripe subscriptions (ends_at == trial_ends_at), and Paddle - # leaves a stale future trial_ends_at (ends_at < trial_ends_at). This single - # predicate keeps never-converted trials out of subscriber counts, new/churned - # MRR, churn rates, and the dashboard summaries. + # leaves a stale future trial_ends_at (ends_at < trial_ends_at). The strict + # `>` is intentional: equality means the subscription never had a billable + # interval for metric purposes, even if those timestamps share a second. + # This single predicate keeps never-converted trials out of subscriber counts, + # new/churned MRR, churn rates, and the dashboard summaries. def subscription_was_billable_before_ending_sql "(pay_subscriptions.ends_at IS NULL OR pay_subscriptions.ends_at > #{subscription_became_billable_at_sql})" end @@ -485,6 +480,15 @@ def calculate_mrr_growth(period = DEFAULT_PERIOD) new_mrr - churned_mrr end + def calculate_mrr_at(date) + # Find subscriptions that were active AT the given date: + # - Started billing before or on that date + # - Not ended before that date (ends_at is nil OR ends_at > date) + # - Not paused at that date + # - Not still in a free trial at that date + mrr_sum(billable_subscription_scope_at(date)) + end + def calculate_mrr_growth_rate(period = DEFAULT_PERIOD) end_date = Time.current start_date = end_date - period diff --git a/lib/profitable/mrr_calculator.rb b/lib/profitable/mrr_calculator.rb index 75e0146..16c8114 100644 --- a/lib/profitable/mrr_calculator.rb +++ b/lib/profitable/mrr_calculator.rb @@ -10,7 +10,7 @@ class MrrCalculator # The billable-subscription query lives in Profitable's metrics module so # current MRR, MRR-at-date, and growth rates can never drift apart. def self.calculate - Profitable.calculate_mrr_at(Time.current) + Profitable.mrr_at(Time.current).to_i rescue => e Rails.logger.error("Error calculating total MRR: #{e.message}") raise Profitable::Error, "Failed to calculate MRR: #{e.message}" diff --git a/test/mrr_calculator_test.rb b/test/mrr_calculator_test.rb index 419b9fc..a188b56 100644 --- a/test/mrr_calculator_test.rb +++ b/test/mrr_calculator_test.rb @@ -410,7 +410,7 @@ def test_process_subscription_handles_exceptions_gracefully end def test_calculate_wraps_unexpected_errors_in_profitable_error - Profitable.stubs(:calculate_mrr_at).raises(StandardError.new("database exploded")) + Profitable.stubs(:mrr_at).raises(StandardError.new("database exploded")) error = assert_raises(Profitable::Error) { Profitable::MrrCalculator.calculate } @@ -431,7 +431,7 @@ def test_calculate_matches_the_mrr_at_snapshot_for_now # Current MRR and the MRR-at-date snapshot must be the same query; # if these ever diverge, growth rates stop being trustworthy. - assert_equal Profitable.calculate_mrr_at(Time.current), Profitable::MrrCalculator.calculate + assert_equal Profitable.mrr_at(Time.current).to_i, Profitable::MrrCalculator.calculate end # ============================================================================ diff --git a/test/profitable_test.rb b/test/profitable_test.rb index e55f6a1..90a738f 100644 --- a/test/profitable_test.rb +++ b/test/profitable_test.rb @@ -34,6 +34,30 @@ def test_mrr_calculates_correctly assert_equal 9900, Profitable.mrr.to_i end + def test_mrr_at_returns_numeric_result + assert_kind_of Profitable::NumericResult, Profitable.mrr_at(Time.current) + end + + def test_mrr_at_calculates_historical_snapshot + churned_subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month", + status: "canceled" + ) + churned_subscription.update!(created_at: 60.days.ago, ends_at: 10.days.ago) + + current_subscription = create_stripe_subscription_v10( + customer: create_customer(processor: "stripe"), + unit_amount: 4900, + interval: "month" + ) + current_subscription.update!(created_at: 5.days.ago) + + assert_equal 9900, Profitable.mrr_at(30.days.ago).to_i + assert_equal 4900, Profitable.mrr_at(Time.current).to_i + end + def test_mrr_excludes_subscriptions_still_on_trial_even_if_status_is_active subscription = create_stripe_subscription_v10( customer: @customer, @@ -815,6 +839,8 @@ def test_churned_customers_counts_ended_subscriptions interval: "month", status: "canceled" ) + # Churn fixtures must have existed before ends_at to satisfy the + # billable-before-ending guard. churned_sub.update!(created_at: 45.days.ago, ends_at: 10.days.ago) assert_equal 1, Profitable.churned_customers(in_the_last: 30.days).to_i diff --git a/test/regression_test.rb b/test/regression_test.rb index 69e3328..19119ab 100644 --- a/test/regression_test.rb +++ b/test/regression_test.rb @@ -249,7 +249,7 @@ def test_bug5_mrr_at_calculates_historical_state new_sub.update!(created_at: 15.days.ago) # MRR at 45 days ago should include churned_sub but NOT new_sub - historical_mrr = Profitable.calculate_mrr_at(45.days.ago) + historical_mrr = Profitable.mrr_at(45.days.ago).to_i assert_equal 5000, historical_mrr, "BUG #5 REGRESSION: Should calculate MRR at historical date" end @@ -808,6 +808,20 @@ def test_bug15_legacy_data_column_unpaid_charge_is_not_revenue assert_equal 0, Profitable.all_time_revenue.to_i end + def test_bug15_dual_payload_columns_prefer_object_payload + Pay::Charge.create!( + customer: @customer, + processor_id: "ch_dual_payload", + amount: 7000, + currency: "usd", + object: { "paid" => false }, + data: { "paid" => true, "status" => "succeeded" } + ) + + assert_equal 0, Profitable.all_time_revenue.to_i, + "BUG #15 REGRESSION: object payloads must win when both charge payload columns exist" + end + def test_bug15_legacy_pay_schema_without_object_column_still_counts_data_charges create_successful_charge_legacy(customer: @customer, amount: 4200) From cbd33fccfdda114b37c1a87fad0031b21afffa36 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Tue, 23 Jun 2026 04:30:29 +0200 Subject: [PATCH 5/5] Finalize metric audit release prep --- CHANGELOG.md | 3 ++- lib/profitable/metrics.rb | 3 ++- lib/profitable/mrr_calculator.rb | 2 ++ test/mrr_calculator_test.rb | 8 ++++++++ test/profitable_test.rb | 12 ++++++++++++ 5 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9c6776..c9e8f10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # `profitable` -## [0.6.0] - 2026-06-20 +## [0.6.0] - 2026-06-23 Accuracy-focused release: every metric was re-audited line by line against the actual data `pay` (7.x–11.x) stores. @@ -14,6 +14,7 @@ Accuracy-focused release: every metric was re-audited line by line against the a - **Fix paused subscriptions disappearing from lifecycle metrics**: paused subscriptions without a local pause start date remain excluded from current MRR, but still count as customers/subscribers if they had already become billable - **Fix two small edge cases**: sub-dollar positive MRR no longer reports as "No MRR yet" for milestone messaging, and monthly churn denominators now match the inclusive period-start semantics used by `churn` - Add `Profitable.mrr_at(date)` as the public historical MRR snapshot API, keeping the raw `calculate_mrr_at` helper private +- Preserve existing `Profitable::Error` messages in MRR snapshot delegation instead of double-wrapping them - **Correct the processor coverage docs**: `pay` only stores subscription price payloads locally for Stripe, so Braintree/Paddle subscriptions contribute 0 to MRR-style metrics unless the payload is backfilled — the README previously overstated this; charge-based and lifecycle-based metrics remain fully portable across all processors - DRY consolidation: MRR "billable right now" and "billable at date" are now literally the same query (`MrrCalculator.calculate` delegates to the shared snapshot), churn-event queries are defined once and reused by all metrics and summaries, and `subscription_data` has a single source of truth - `MrrCalculator.calculate` streams subscriptions in batches (`find_each`) everywhere, keeping memory flat on large datasets diff --git a/lib/profitable/metrics.rb b/lib/profitable/metrics.rb index 1261864..5ed60df 100644 --- a/lib/profitable/metrics.rb +++ b/lib/profitable/metrics.rb @@ -197,7 +197,8 @@ def subscription_became_billable_at_sql # end date on ended Stripe subscriptions (ends_at == trial_ends_at), and Paddle # leaves a stale future trial_ends_at (ends_at < trial_ends_at). The strict # `>` is intentional: equality means the subscription never had a billable - # interval for metric purposes, even if those timestamps share a second. + # interval for metric purposes, even if those timestamps share a second + # (for example, ends_at == trial_ends_at on an immediately canceled trial). # This single predicate keeps never-converted trials out of subscriber counts, # new/churned MRR, churn rates, and the dashboard summaries. def subscription_was_billable_before_ending_sql diff --git a/lib/profitable/mrr_calculator.rb b/lib/profitable/mrr_calculator.rb index 16c8114..089f737 100644 --- a/lib/profitable/mrr_calculator.rb +++ b/lib/profitable/mrr_calculator.rb @@ -11,6 +11,8 @@ class MrrCalculator # current MRR, MRR-at-date, and growth rates can never drift apart. def self.calculate Profitable.mrr_at(Time.current).to_i + rescue Profitable::Error + raise rescue => e Rails.logger.error("Error calculating total MRR: #{e.message}") raise Profitable::Error, "Failed to calculate MRR: #{e.message}" diff --git a/test/mrr_calculator_test.rb b/test/mrr_calculator_test.rb index a188b56..65aa2ac 100644 --- a/test/mrr_calculator_test.rb +++ b/test/mrr_calculator_test.rb @@ -417,6 +417,14 @@ def test_calculate_wraps_unexpected_errors_in_profitable_error assert_includes error.message, "database exploded" end + def test_calculate_reraises_profitable_errors_without_double_wrapping + Profitable.stubs(:mrr_at).raises(Profitable::Error.new("already wrapped")) + + error = assert_raises(Profitable::Error) { Profitable::MrrCalculator.calculate } + + assert_equal "already wrapped", error.message + end + def test_calculate_matches_the_mrr_at_snapshot_for_now create_stripe_subscription_v10( customer: @stripe_customer, diff --git a/test/profitable_test.rb b/test/profitable_test.rb index 90a738f..fa327de 100644 --- a/test/profitable_test.rb +++ b/test/profitable_test.rb @@ -58,6 +58,18 @@ def test_mrr_at_calculates_historical_snapshot assert_equal 4900, Profitable.mrr_at(Time.current).to_i end + def test_mrr_at_respects_pause_state_at_snapshot_date + subscription = create_stripe_subscription_v10( + customer: @customer, + unit_amount: 9900, + interval: "month" + ) + subscription.update!(created_at: 60.days.ago, pause_starts_at: 30.days.ago) + + assert_equal 9900, Profitable.mrr_at(45.days.ago).to_i + assert_equal 0, Profitable.mrr_at(15.days.ago).to_i + end + def test_mrr_excludes_subscriptions_still_on_trial_even_if_status_is_active subscription = create_stripe_subscription_v10( customer: @customer,