Audit and harden profitable metrics

[rameerez]

Summary

This PR is the production-ready follow-up to the committed Fable review. It keeps the broad metric audit and adds another pass over real Pay schemas, load behavior, API boundaries, and edge cases that the standalone test doubles could miss.

Metric accuracy fixes from the Fable review

• Excludes canceled trials that never converted from subscriber/customer counts, new MRR, churned MRR, churn, and dashboard summaries.
• Fixes monthly-summary churn survivor bias by calculating each month’s denominator from subscriptions billable at that month’s start.
• Caps monthly/daily summary event windows at Time.current so future trial conversions do not count early.
• Normalizes SQLite JSON boolean comparisons so paid: false charges are excluded consistently across SQLite/PostgreSQL/MySQL.
• Handles processor status variants: expired churns at ends_at; pending is never billable.
• Returns Profitable::NumericResult from time_to_next_mrr_milestone, so the documented .to_readable works.
• Consolidates shared MRR, churn-event, and subscription-payload logic to reduce drift between public metrics.

Additional production-readiness fixes

• Makes charge JSON filtering schema-aware: Pay 7-9 apps do not have pay_charges.object, so SQL now only references columns that actually exist.
• Splits “ever crossed into a billable lifecycle” from “billable at this exact date,” so paused subscriptions without pause_starts_at do not count as current MRR but still count as historical customers/subscribers once they converted.
• Treats positive sub-dollar MRR as real MRR in milestone messaging instead of integer-dividing it down to zero.
• Makes monthly churn denominators inclusive at the period boundary, matching the canonical churn helper.
• Adds explicit load dependencies for delegate and actionview; declares actionview in the gemspec because the gem directly requires and uses Action View helpers.
• Removes the full top-level require "rails" added during hardening; Rails apps already load Rails before engine gems are required. The gem still directly requires pay, because pay is a runtime dependency and should be loaded with profitable.
• Adds Profitable.mrr_at(date) as the public historical MRR snapshot API, and keeps the raw calculate_mrr_at helper private so public methods still return NumericResult.
• Adds a memoized charge_payload_columns helper for the schema-aware charge JSON SQL path, plus an explicit dual-payload regression test.
• Fixes the README’s stale Profitable.valuation_estimate example and updates the 0.6.0 changelog date/context.

Behavior Notes

This release intentionally changes user-visible metrics where previous values were wrong. The largest expected shifts are:

• Trial-only cancellations no longer appear as paid subscribers or churn.
• expired subscriptions no longer inflate active/billable metrics forever.
• Pay 7-9 data-only schemas no longer crash revenue queries that check charge payload JSON.
• Paused-but-previously-billable subscriptions remain historical subscribers/customers, while still staying out of current MRR.

Amount-derived subscription metrics remain fully supported out of the box for Stripe. Braintree/Paddle amount parsing still requires applications to backfill processor subscription payloads locally; the README now states this explicitly.

Automated Review Response

Addressed from the automated reviews:

• Removed the full top-level require "rails" and kept the narrower direct dependency requires needed by this gem.
• Replaced the private-method send coupling with public Profitable.mrr_at(date), then moved raw calculate_mrr_at back behind the private boundary.
• Added charge_payload_columns so schema-aware charge payload column detection is centralized and memoized.
• Fixed the 0.6.0 changelog date to 2026-06-20.
• Added a comment documenting why the ends_at > became_billable_at guard is intentionally strict. Changing it to >= would re-admit canceled-trial equality cases.
• Added a clarifying churn fixture comment and an explicit dual-payload object/data charge regression test.

Reviewed but intentionally not changed:

• Kept require "pay" in lib/profitable.rb; pay is a runtime dependency and profitable is explicitly pay-powered, so loading it directly is the safest behavior for host apps.
• Did not add more travel_to wrappers to the canceled-trial regression tests; those tests use rolling windows with explicit relative offsets and are not month-bucket-sensitive. The monthly/daily summary cases already pin time.
• Did not rewrite the repeated paid_charges SQL expression into a subquery; the reviewer marked it non-blocking, and the current SQL is clear and covered.

Verification

• bundle exec rake test
  • 336 runs, 516 assertions, 0 failures, 0 errors, 0 skips
  • Line coverage: 97.75%
  • Branch coverage: 89.9%
• Appraisal matrix:
  • pay-7.3: 336 runs, 516 assertions, 0 failures
  • pay-8.3: 336 runs, 516 assertions, 0 failures
  • pay-9.0: 336 runs, 516 assertions, 0 failures
  • pay-10.0: 336 runs, 516 assertions, 0 failures
  • pay-11.0: 336 runs, 516 assertions, 0 failures
  • rails-7.2: 336 runs, 516 assertions, 0 failures
  • rails-8.1: 336 runs, 516 assertions, 0 failures
• Real Pay smoke tests:
  • Pay 11.4.3 with actual Pay migrations/models: passed (all_time_revenue: 4200, total_customers: 1, mrr: 9900, arr: 118800)
  • Pay 7.3.0 with actual data-only schema and no object columns: passed with the same expected metrics
• Load/build checks:
  • bundle exec ruby -e 'require "rails"; require "active_record"; require "action_controller"; require "profitable"; ...': passed and confirmed mrr_at is public while calculate_mrr_at is private
  • bundle exec rake build: passed (pkg/profitable-0.6.0.gem)
• Static checks:
  • ruby -c on changed Ruby files: OK
  • git diff --check: OK

[claude]

Code Review: Audit and harden profitable metrics (v0.6.0)

This is a substantial correctness release — nine real bugs fixed, all with regression tests, tested across Pay 7.3–11.x and Rails 7.2–8.1. The core design choices (splitting "ever billable" from "billable right now", the churned_subscription_events_in_period helper, the schema-aware charge_json_value_sql) are all sound. Below are the findings worth resolving before merge.

---

Issues

1. require "rails" at gem load time — non-standard and risky

lib/profitable.rb now opens with:

require "rails"
require "pay"
require "active_support"
...

For a Rails engine gem this is unusual. The Engine already hooks into the host app's initialization pipeline — that's the correct place to guarantee load order. A bare require "rails" at gem load time:

• Triggers Rails' own boot sequence early and out of context. This can interfere with apps that control initialization order (API-only apps, custom initializers, jemalloc/YJIT setup hooks).
• The stated motivation is that NumericResult includes ActionView::Helpers::NumberHelper "at definition time." But include is evaluated when the class body runs, not when the method is called — and require "action_view" in numeric_result.rb already handles that. The individual require "delegate" / require "action_view" at the top of that file are the right fix.
• require "pay" at the top of the library file means loading profitable always loads pay, even in test harnesses or scripts that set up their own test doubles.

Suggestion: Remove the require "rails" / require "pay" / require "active_support" / require "active_support/time" / require "active_support/core_ext/string/filters" lines from lib/profitable.rb. The engine's railtie already guarantees these are available when the host app boots. The require "action_view" in numeric_result.rb and the individual ActiveSupport requires can stay where they are.

---

2. Profitable.send(:calculate_mrr_at, ...) in MrrCalculator — private-method coupling

# lib/profitable/mrr_calculator.rb
def self.calculate
  Profitable.send(:calculate_mrr_at, Time.current)
  ...
end

Using send to call a private method across module boundaries is a red flag. It means MrrCalculator has an undocumented dependency on a private implementation detail of Profitable::Metrics. If calculate_mrr_at is renamed or moved the compiler won't catch it; send will silently dispatch to the wrong method or raise at runtime.

Suggestion: Either make calculate_mrr_at a public private method (module-function) so it can be called without send, or move the shared logic to a third module that both MrrCalculator and Metrics call directly.

---

3. charge_json_value_sql calls Pay::Charge.column_names at query build time

def charge_json_value_sql(key)
  columns = %w[object data].select { |column| Pay::Charge.column_names.include?(column) }
  ...
end

column_names is cached by ActiveRecord per process, so this is fast after the first call. However, it's called inside paid_charges, which is called inside every revenue query. The check is O(1) after caching, so it's not a performance problem, but the result is stable for the lifetime of the process — it could be memoized:

def charge_payload_columns
  @charge_payload_columns ||= %w[object data].select { |c| Pay::Charge.column_names.include?(c) }
end

Minor, but worth doing for clarity.

---

4. Regression tests for Bug #11 (canceled trials) don't use travel_to

The test_bug11_* tests assert over in_the_last: 30.days windows using subscriptions created with explicit ends_at: 10.days.ago / created_at: 20.days.ago. These don't depend on month bucketing, so they're safe. However, the create_stripe_style_canceled_trial / create_paddle_style_canceled_trial helpers set created_at: 20.days.ago and ends_at: 10.days.ago, meaning both dates fall inside a 30-day window. This is fine today, but if the lookback window in those helpers is ever tightened or the fixed offsets drift, there's no date pin to catch it. The pattern used elsewhere (travel_to Time.current.beginning_of_month + 14.days) is a good model to follow for new helpers.

---

Observations / Minor Notes

• DEFAULT_PERIOD is 30.days, so the revenue_run_rate and estimated_revenue_run_rate_valuation default-parameter changes are no-ops. Good — no silent breaking change.

• subscription_was_billable_before_ending_sql is correct and elegant. The predicate ends_at IS NULL OR ends_at > COALESCE(trial_ends_at, created_at) cleanly handles the Stripe "both fields set to the same timestamp" case and the Paddle "stale future trial_ends_at" case without branching on processor type.

• churned_subscription_events_in_period consolidation is the right call. Before this PR, churn events were written inline in three different places. A single helper means a future status-list change (another expired-style addition) propagates everywhere at once.

• Monthly summary survivor bias fix is correct. Switching from billable_at < month_start to billable_at <= month_start in the Ruby denominator filter matches the churn helper's inclusive period-start semantics and correctly includes subscriptions that started exactly at midnight on the first of the month.

• travel_to + 14.days in summary tests. The pattern of pinning time to beginning_of_month + 14.days before exercising monthly-bucketed logic is solid — 14 days of slack prevents the test from straddling a month boundary regardless of when in the month CI runs it. Worth keeping.

• SQLite CAST + NOT IN ('false', '0') fix is correct. SQLite json_extract returns JSON booleans as integers (0/1), so both forms must be excluded. The TEXT cast makes all three adapters return comparable string types.

• calculate_arr changed from (mrr.to_f * 12).round to mrr.to_i * 12. This is a minor precision change: a non-integer MRR value (e.g. from floating-point arithmetic) would previously be rounded to the nearest dollar; now it's truncated. Given that MRR is stored in cents (integers), mrr.to_i and mrr.to_f will agree, so this is effectively a no-op for the current implementation, but worth noting if MRR ever gains fractional-cent support.

---

Summary

Two items warrant action before merge:

1. Remove require "rails" / require "pay" from lib/profitable.rb — the engine handles this.
2. Eliminate the Profitable.send(:calculate_mrr_at, ...) call by making that method accessible without send.

Everything else — the split billability predicates, the churn-event consolidation, the schema-aware charge query, the SQLite CAST fix, and the comprehensive regression suite — is solid work.

[rameerez]

Follow-up pushed in 57db5db after reviewing the automated feedback.

Acted on:

• Removed the full top-level require "rails". Rails apps load Rails before engine gems, and the narrower dependency requires are enough here.
• Replaced Profitable.send(:calculate_mrr_at, Time.current) with a direct Profitable.calculate_mrr_at(Time.current) call by exposing the shared historical MRR snapshot method.
• Added charge_payload_columns so charge payload column detection is centralized and memoized.

Reviewed but left unchanged:

• Kept require "pay". pay is a runtime dependency and profitable is explicitly pay-powered; relying on the host app to separately require a transitive dependency is more fragile than loading it directly.
• Did not add extra travel_to wrappers to the rolling-window canceled-trial tests. The month/day summary tests already pin time; these assertions are not bucket-boundary-sensitive.

Re-verified after the changes:

• bundle exec rake test: 333 runs, 512 assertions, 0 failures
• Full appraisal matrix green: Pay 7.3, 8.3, 9.0, 10.0, 11.0 and Rails 7.2, 8.1
• Real Pay 11.4.3 migration/model smoke passed
• Real Pay 7.3.0 data-only schema smoke passed
• ruby -c and git diff --check passed

[claude]

Code Review: Audit and Harden Profitable Metrics (0.6.0)

This is a large, high-quality accuracy release. The scope is significant — 16 changed files, 9 numbered bugs fixed, ~98% test coverage — and the overall approach is sound. Below is a thorough review.

---

Overview

The PR hardens metric accuracy across the board: canceled trials, survivor-biased churn denominators, future-event leakage in summaries, SQLite/PostgreSQL JSON-boolean divergence, unrecognized processor statuses, and the time_to_next_mrr_milestone API contract. It also consolidates duplicated query logic into shared helpers and fixes load ordering in the entrypoint.

---

Strengths

• Regression tests for every bug. Each BUG #N block in regression_test.rb documents the original behavior, the root cause, and the fix, then covers it with multiple test cases including control paths. This is exactly the right pattern for a correctness-focused release.
• DRY consolidation is real. churned_subscription_events_in_period, mrr_sum, charge_json_value_sql, and Processors::Base.subscription_data all eliminate genuine duplication that was causing drift (e.g., monthly summary and churned_mrr used to have separate churned-subscription queries that could diverge).
• The subscription_has_billable_lifecycle_by / subscription_is_billable_by split is the right design. Separating "has this subscription ever crossed into billing?" from "is it billable right now?" is the correct semantic distinction.
• travel_to wrapping for month-boundary safety. Pinning tests to beginning_of_month + 14.days prevents flaky failures when tests run near midnight on the 1st.
• find_each is used consistently in mrr_sum, keeping memory flat on large datasets.

---

Issues and Suggestions

1. calculate_mrr_at naming signals private but it's now public

# lib/profitable/metrics.rb
def calculate_mrr_at(date)          # <-- public
  mrr_sum(billable_subscription_scope_at(date))
end

private

# ...
def calculate_churn_rate_for_period ...

The calculate_* convention throughout this codebase means "private implementation helper." Making calculate_mrr_at public so MrrCalculator can call it without send is pragmatic, but it leaks an implementation-detail name into the gem's public surface. Consider renaming it mrr_at(date) to match the public metric naming pattern (mrr, arr, ttm, etc.), and update the regression test accordingly. The MrrCalculator delegation then reads as Profitable.mrr_at(Time.current) which is self-documenting.

2. CHANGELOG release date is off by one day

## [0.6.0] - 2026-06-21

Today is 2026-06-20. The date appears to be set for tomorrow — likely copy-paste from a planned publish date. Update to 2026-06-20 if releasing today, or note this before merging.

3. subscription_was_billable_before_ending_sql edge case: zero-duration trials

The predicate is:

ends_at IS NULL OR ends_at > COALESCE(trial_ends_at, created_at)

For a subscription that converts instantly (trial starts and ends at the same second as created_at, or trial_ends_at == created_at), ends_at > COALESCE(trial_ends_at, created_at) requires strict >, meaning a subscription that was billed for exactly one instant before immediate cancellation (same-second ends_at == trial_ends_at) is treated as a canceled trial. In practice this edge case is rare, but if it matters, consider >= instead of >. The existing tests use ends_at: trial_ended_at with trial_ends_at: trial_ended_at (same value), which confirms the current behavior filters these out — just make sure that's intentional.

4. paid_charges SQL now double-evaluates charge_json_value_sql result

paid = charge_json_value_sql('paid')
status = charge_json_value_sql('status')

Pay::Charge
  .where(<<~SQL.squish, 'false', '0', 'succeeded')
    (#{paid} IS NULL OR #{paid} NOT IN (?, ?))
    AND
    (#{status} = ? OR #{status} IS NULL)
  SQL

#{paid} is expanded twice in the WHERE clause. For the COALESCE(...) path (Pay 7-9 apps with both columns), this means the full COALESCE(json_extract(...), json_extract(...)) expression appears twice in the SQL string. Most query planners will deduplicate it, but it's slightly wasteful and harder to read. Consider aliasing in a subquery or using Arel if this becomes a maintenance concern. Not a blocking issue.

5. @charge_payload_columns memoization on the module — correct but note the constraint

def charge_payload_columns
  @charge_payload_columns ||= %w[object data].select { |column| Pay::Charge.column_names.include?(column) }
end

This caches the result on the Profitable module object. It is correct for production (schema doesn't change at runtime), but it is why the test stubs charge_payload_columns directly rather than testing the column detection. If someone runs tests that alter the schema mid-suite, they'd need to clear @charge_payload_columns manually. Not a bug, just worth noting for future contributors.

6. The calculate_arr simplification is correct but loses a comment

-  def calculate_arr
-    (mrr.to_f * 12).round
-  end
+  def calculate_arr
+    mrr.to_i * 12
+  end

mrr returns a NumericResult wrapping integer cents, so mrr.to_i * 12 is exact. The old to_f * 12).round was harmless but noisy. The new version is correct. No issue.

7. test_churned_customers_counts_ended_subscriptions fix should have a comment

-    churned_sub.update!(ends_at: 10.days.ago)
+    churned_sub.update!(created_at: 45.days.ago, ends_at: 10.days.ago)

The created_at: 45.days.ago is needed because of the new subscription_was_billable_before_ending_sql predicate: without it ends_at (10.days.ago) > COALESCE(trial_ends_at, created_at (now)) is false, so the churned subscription would be filtered out as if it were a canceled trial. A one-line comment on the test explaining why created_at must precede ends_at would help future readers.

8. billable_subscription_scope_at is referenced but not changed — confirm it includes the new guard

calculate_mrr_at delegates to billable_subscription_scope_at(date) which eventually calls subscription_is_billable_by(date). Since subscription_is_billable_by now includes .where(subscription_was_billable_before_ending_sql) (via subscription_has_billable_lifecycle_by), the new guard is included. This is correct, but it's worth verifying that the MrrCalculator smoke test (mrr: 9900) was confirmed against a subscription that clearly satisfies the new predicate, which the PR description indicates.

---

Minor / Nits

• count('customer_id') → count(:customer_id) is a correct Rails idiom improvement. No issue.
• The .joins(:subscription) for calculate_recurring_revenue_in_period is cleaner than raw SQL. Good change.
• Requiring delegate in numeric_result.rb explicitly is correct — SimpleDelegator is in delegate, and relying on it being already loaded by Rails is fragile outside Rails contexts.
• revenue_run_rate and estimated_revenue_run_rate_valuation defaulting to DEFAULT_PERIOD instead of hardcoded 30.days is a good consistency fix assuming DEFAULT_PERIOD is 30.days (which it appears to be from the README).

---

Test Coverage Assessment

333 runs / 512 assertions across Pay 7.3–11.x and Rails 7.2–8.1 with deterministic seeds at 97.73% line / 89.9% branch is excellent for a gem of this scope. The regression tests for bugs 11–19 are thorough. The control case in test_bug11_converted_then_canceled_subscription_still_counts_everywhere is particularly important — confirming the fix doesn't swallow real churn.

One gap: there are no tests for the charge_payload_columns auto-detection path where both object and data exist (Pay 10 schema has both during a migration). The stubs simulate Pay 7-9 (["data"] only), but the COALESCE path for dual-column schemas isn't explicitly exercised. Low risk since the COALESCE path was already tested before this PR, but worth noting.

---

Summary

This PR is ready to merge with minor fixes. The core bug fixes are all correct, the DRY consolidation is well-executed, and the regression test suite is comprehensive. The two most important things to address before merging:

1. Rename calculate_mrr_at → mrr_at (or keep private and explain the tradeoff) to avoid leaking an impl-detail name into the public API.
2. Fix the CHANGELOG date (2026-06-21 → 2026-06-20).

The zero-duration trial edge case (#3 above) is worth a comment even if the current behavior is intentional.

Great work on the audit — the canceled-trial and survivor-bias bugs in particular were subtle and could have significantly distorted metrics for apps with meaningful trial volume.

[rameerez]

Follow-up pushed in 2819ccb after reviewing the second automated comment.

Acted on:

• Added public Profitable.mrr_at(date) and moved raw calculate_mrr_at back to private, preserving the gem convention that public metrics return NumericResult.
• Updated MrrCalculator.calculate to use Profitable.mrr_at(Time.current).to_i instead of exposing/calling an implementation helper.
• Fixed the 0.6.0 changelog date to 2026-06-20.
• Documented why the ends_at > became_billable_at guard is intentionally strict; changing this to >= would re-admit canceled trials where Pay stores equality.
• Added the requested churn fixture comment.
• Added explicit dual-payload object/data charge coverage for the schema-aware COALESCE path.
• Documented mrr_at(date) in the README and changelog.

Reviewed but left unchanged:

• The duplicated paid_charges SQL expression remains as-is because it is clear, covered, and the review marked it non-blocking.
• The module-level charge_payload_columns memoization remains as-is; production schemas are stable per process, and tests that need legacy schema behavior stub the helper directly.

Re-verified after this follow-up:

• bundle exec rake test: 336 runs, 516 assertions, 0 failures
• Full appraisal matrix green: Pay 7.3, 8.3, 9.0, 10.0, 11.0 and Rails 7.2, 8.1
• Load smoke confirmed mrr_at public and calculate_mrr_at private
• bundle exec rake build: passed
• ruby -c and git diff --check: passed

[claude]

Code Review: Audit and harden profitable metrics

This is a thorough, well-engineered release. The bug descriptions are precise, the fixes are targeted, and the regression test suite is exemplary. Below is a full pass.

---

Architecture: Scope Hierarchy

The new three-level scope hierarchy is a clean design:

Method	What it answers
subscription_has_billable_lifecycle_by(date)	Has this sub cleared lifecycle gates by date? (no current-state date constraints)
subscription_is_billable_by(date)	Same + not paused at date (still no ends_at > date constraint)
billable_subscription_scope_at(date)	Is this sub billable at exactly date? (adds billable_at <= date, ends_at > date, pause_starts_at > date)

This is the correct split for the monthly-summary churn denominator: using subscription_is_billable_by(overall_end) (no ends_at date cut) + .where(ends_at > overall_start) correctly fetches all subscriptions active at any point in the window, and per-month Ruby filtering handles the rest. Good call, and well-executed.

---

Bugs / Potential Issues

1. MrrCalculator.calculate double-wraps Profitable::Error

def self.calculate
  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}"
end

rescue => e catches StandardError and all subclasses — including Profitable::Error itself. If mrr_at raises a Profitable::Error, the message becomes "Failed to calculate MRR: Failed to calculate MRR: <original>". This was present in the old code too so it's not a regression, but the delegation inversion (MrrCalculator now calling Profitable) makes this path newly reachable. Consider narrowing the rescue or re-raising the original if it's already a Profitable::Error:

rescue Profitable::Error
  raise
rescue => e
  Rails.logger.error("Error calculating total MRR: #{e.message}")
  raise Profitable::Error, "Failed to calculate MRR: #{e.message}"
end

2. charge_payload_columns memoization is process-scoped

def charge_payload_columns
  @charge_payload_columns ||= %w[object data].select { |column| Pay::Charge.column_names.include?(column) }
end

@charge_payload_columns is a module-level instance variable, memoized for the lifetime of the process. This is fine in production. In test suites that run appraisals against different Pay schemas in the same process, a previously cached value could bleed into a later appraisal run. The stubs in tests (Profitable.stubs(:charge_payload_columns).returns(...)) bypass the variable correctly, so the existing tests are clean. Just worth keeping in mind if the appraisal matrix ever adds a test that needs live schema detection rather than a stub.

3. subscription_was_billable_before_ending_sql strict > edge case

The comment explains the choice well, but it's worth documenting one implication: a subscription created and immediately cancelled in the same second (e.g. a test/staging fixture or a Stripe webhook race) will have ends_at == became_billable_at and be silently excluded. This is almost certainly correct behavior for production metrics, but if anyone investigates why a specific subscription doesn't appear, this predicate is the non-obvious place to look. The existing comment is good; a brief # E.g. ends_at == trial_ends_at on an immediately-cancelled Stripe trial in the comment block would help future readers.

---

Nits

calculate_arr changes from float to int multiplication

# Before
(mrr.to_f * 12).round
# After
mrr.to_i * 12

Since mrr wraps an integer (cents), these are equivalent. The new form is cleaner. ✓

DEFAULT_PERIOD change for revenue_run_rate and estimated_revenue_run_rate_valuation

Both previously defaulted to 30.days (a literal). DEFAULT_PERIOD = 30.days, so this is a no-op behaviour change. ✓

count(:customer_id) over count('customer_id')

Better Rails idiom, avoids string quoting inconsistencies across adapters. ✓

.joins(:subscription) over .joins('INNER JOIN pay_subscriptions ...')

Cleaner and lets ActiveRecord handle the join key. ✓

---

Test Quality

The regression suite additions are excellent:

• Every bug gets its own named test block with failure messages that cite the bug number.
• Both Stripe-style (ends_at == trial_ends_at) and Paddle-style (ends_at < trial_ends_at) cancelled-trial fixtures are covered separately — good, since the > predicate behaves differently in each case.
• travel_to wrappers on month-boundary-sensitive tests prevent day-of-month flakiness.
• teardown { travel_back } ensures time travel never leaks between tests.
• The test_bug15_dual_payload_columns_prefer_object_payload test verifies COALESCE column ordering, which is easy to get wrong silently.

One minor coverage gap: there's no test for mrr_at on a subscription that was paused at the historical date but active now (or vice versa). billable_subscription_scope_at handles this via pause_starts_at > date, but a test would lock in that the historical snapshot excludes a subscription paused before the snapshot date.

---

Summary

The code is correct, well-documented, and thoroughly tested. The three actionable items above are: the double-error-wrap in MrrCalculator.calculate, a clarifying note on the strict > predicate, and the charge_payload_columns test-suite footgun. The rest is nitpick-level.

97.7% line / 89.9% branch coverage across a 5-version Pay appraisal matrix is a strong signal this is ready to ship.

[claude]

Code Review

This is a thorough, well-structured release that fixes a meaningful collection of real metric accuracy bugs — canceled trials, survivor bias in churn denominators, SQLite boolean normalization, Pay 7-9 schema compatibility, and several status constants gaps. The test coverage is excellent (regression tests for every named bug, travel_to pins for month-bucket sensitivity, and dual-payload regression tests). The behavioral change documentation in the PR body is clear and honest about expected metric shifts.

Four findings worth considering before merge:

---

1. mrr_sum "memory flat" claim doesn't hold with includes(:customer) — lib/profitable/metrics.rb ~line 280

# 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

subscriptions_with_processor chains .includes(:customer).joins(:customer). Rails resolves the includes via the JOIN strategy when .joins is also present, so each find_each batch loads 1 000 subscription rows together with their full customer rows via a single JOIN query. The batching is real and the code is correct — but memory per batch is 1_000 * (subscription_size + customer_size), not just subscriptions. The comment says "flat" in a way that implies a pure streaming cursor. Worth clarifying if the batch size is ever tuned.

Not a correctness bug, but the comment creates a false expectation.

---

2. @charge_payload_columns module-level memoization has no reset path — lib/profitable/metrics.rb ~line 326

def charge_payload_columns
  @charge_payload_columns ||= %w[object data].select { |column| Pay::Charge.column_names.include?(column) }
end

This ivar lives on the Profitable singleton class for the process lifetime. The test stubs work correctly (Mocha replaces the method, not the ivar), and a production server restart after a migration clears it. The one scenario it doesn't handle cleanly: Rails development mode with config.cache_classes = false and config.eager_load = false — on each request the module is reloaded but the ivar resets from scratch anyway, so it's fine there too.

The real gap is that there's no .reset_charge_payload_columns! escape hatch, so any app that programmatically alters the Pay schema at runtime (rare, but used in multi-tenant setups with dynamic schemas) cannot invalidate the cache. A def reload_charge_payload_columns! = @charge_payload_columns = nil one-liner would cover this and is zero cost to add.

---

3. Positional bind params in paid_charges are fragile — lib/profitable/metrics.rb ~line 295–308

paid   = charge_json_value_sql('paid')
status = charge_json_value_sql('status')

Pay::Charge
  .where("pay_charges.amount > 0")
  .where(<<~SQL.squish, 'false', '0', 'succeeded')
    (#{paid} IS NULL OR #{paid} NOT IN (?, ?))
    AND
    (#{status} = ? OR #{status} IS NULL)
  SQL

The ordering is correct: NOT IN (?, ?) consumes 'false' and '0', then = ? consumes 'succeeded'. But paid and status are runtime SQL fragments interpolated into the heredoc, and any future reordering of the SQL clauses would silently swap the bind values. Named binds aren't available here (raw SQL), but at minimum a comment above the .where call naming each positional slot would make the intent explicit and prevent a silent logic flip if the clause order is ever changed.

---

4. Two inline MrrCalculator.process_subscription calls in calculate_monthly_summary bypass the new mrr_sum abstraction — lib/profitable/metrics.rb ~lines 604 and 608

new_mrr_amount = new_mrr_subs
  .select { |s| subscription_became_billable_at(s).between?(month_start, month_end) }
  .sum { |s| MrrCalculator.process_subscription(s) }

churned_mrr_amount = churned_mrr_subs
  .select { |s| s.ends_at >= month_start && s.ends_at <= month_end }
  .sum { |s| MrrCalculator.process_subscription(s) }

These operate on pre-loaded Ruby arrays (not AR scopes), so mrr_sum (which calls find_each on a scope) can't be used directly. But if mrr_sum ever gains normalization logic (e.g., a currency conversion, a floor to zero check), these two paths would silently diverge. A shared mrr_sum_from_array(subscriptions) helper wrapping the raw process_subscription loop would keep things in sync — or at minimum a comment noting why the inline form is necessary.

---

Summary: No correctness bugs survived verification. All four fixes above are low-severity — the PR is solid and the test suite is thorough. The critical behavioral fixes (canceled-trial exclusion, survivor-bias denominator, SQLite boolean normalization, Pay schema awareness) are all correct and well-tested.