Expose pricing plan resolution and provenance helpers

[rameerez]

Summary

• add PricingPlans::PlanResolution as the canonical result of plan resolution
• expose current_pricing_plan_resolution and current_pricing_plan_source on plan owners
• preserve underlying Stripe subscription context even when a manual assignment wins
• refactor PlanResolver to derive effective plan selection from a single resolution path
• document and test that effective plan access is separate from billing subscription state

Testing

• bundle exec rake test

[claude]

PR Review: Add pricing plan resolution provenance helpers

Overall this is a clean, well-motivated addition. The PlanResolution struct is a good abstraction, the refactoring is readable, and test coverage is solid. A few things worth looking at:

---

Bug risk: subscription_active_for? check removed

The old resolve_plan_from_pay guarded against inactive/canceled subscriptions:

# OLD is_active = PaySupport.subscription_active_for?(plan_owner) if is_active subscription = PaySupport.current_subscription_for(plan_owner) ... end

The new current_subscription_for calls PaySupport.current_subscription_for directly without the active-check:

# NEW def current_subscription_for(plan_owner) return nil unless plan_owner && pay_available? PaySupport.current_subscription_for(plan_owner) end

If PaySupport.current_subscription_for can return a canceled/expired subscription, the resolution logic will now map it to a subscription-backed plan when it shouldn't:

if subscription processor_plan = subscription.processor_plan if processor_plan && (plan = plan_from_processor_plan(processor_plan)) return PlanResolution.new(plan: plan, source: :subscription, ...) # wrong if subscription is inactive end end

Worth checking whether PaySupport.current_subscription_for already filters to active-only. If not, the subscription_active_for? guard needs to be restored. The new behavior for the provenance use-case (preserving the subscription object even when it's not active) could be achieved by separating "fetch for entitlement check" from "fetch for billing context."

---

Performance: subscription always fetched even when assignment wins

Previously, resolution_for returned early on a manual assignment before ever touching Pay:

# OLD return Registry.plan(assignment.plan_key) if assignment # no Pay call

Now both assignment and subscription are fetched unconditionally at the top:

# NEW assignment = assignment_for(plan_owner) subscription = current_subscription_for(plan_owner) # always called

This is intentional (to preserve billing context in the resolution), but it adds an extra DB/API round-trip for every resolution when there's a manual assignment. For apps with high-traffic entitlement checks this could add up. Consider documenting this trade-off, or making the subscription fetch lazy (e.g., only fetch subscription inside the assignment branch when building the resolution object).

---

PlanResolution#to_h shadows Struct#to_h

Struct already defines to_h. The override here adds computed fields (plan_key, assignment_source), which is fine, but it's non-obvious. A comment would help:

# Extends Struct#to_h to include computed fields for serialization def to_h ... end

---

Missing test: inactive subscription doesn't resolve as subscription-backed

Given the behavior change above, there should be a test confirming that a canceled/inactive subscription does not resolve with source: :subscription. Something like:

def test_inactive_subscription_falls_back_to_default org = create_organization( pay_subscription: { active: false, processor_plan: "price_pro_123" } ) resolution = PricingPlans::PlanResolver.resolution_for(org) assert_equal :default, resolution.source end

---

Positives

• PlanResolution as a Struct with keyword_init: true and source validation is a clean design.
• Preserving the subscription object in the resolution even when assignment wins is a smart design decision — the entitlement/billing separation comment in the docs is exactly the right context for users.
• The refactoring into assignment_for / current_subscription_for helper methods makes resolution_for much easier to follow than the old resolve_plan_from_pay.
• Test coverage for the new provenance paths is thorough, including the test_resolution_for_unknown_subscription_plan_falls_back_to_default_but_keeps_subscription edge case.
• The pay_support.rb fix for plan_owner.id with respond_to?(:id) guard is a nice defensive improvement worth keeping regardless.

---

The active-subscription check regression is the main thing to verify before merging. Everything else is minor.

[rameerez]

Addressed the substantive inactive-subscription concern in 2bb8df0:

• added an explicit direct-pay regression test proving an inactive subscription falls back to the default resolution and does not resolve as source: :subscription
• added the same regression test for the payment_processor path
• added a short comment on PlanResolution#to_h since it intentionally extends Struct#to_h with derived fields

The code path itself was already filtering to active / trial / grace subscriptions inside PaySupport.current_subscription_for, so there was no entitlement regression, but the new tests make that contract explicit.

On the remaining review notes:

• the eager subscription fetch in resolution_for is intentional, because preserving the underlying subscription context even when an assignment wins is the point of the new provenance API; that mixed state is what the app layer needs to render plan access and Stripe billing separately
• I did not rework that into a lazier API because it would complicate the seam without improving correctness for the intended use case
• the Struct#to_h note was low-cost clarity, so I addressed it with the comment above

Re-ran bundle exec rake test after the change.

[claude]

PR Review: Expose pricing plan resolution and provenance helpers

This is a solid refactor with good test coverage and a clean API. The single-resolution-path design is an improvement over the old multi-path approach. A few things worth addressing before merging:

---

Bugs / Correctness

PlanOwner#current_pricing_plan_source triggers a double resolution

Any caller that uses both current_pricing_plan and current_pricing_plan_source will hit the database twice — two Assignment.find_by calls plus two subscription fetches. The PR adds these as separate helpers, which encourages that pattern, and the docs example shows them on separate lines without warning. Consider either caching the resolution on the instance, or at minimum adding a note in the docs that callers needing both should use current_pricing_plan_resolution directly and derive plan/source from it.

Unaddressed parallel bug in PaySupport#subscription_active_for? (line 18)

The PR correctly fixes .id in current_subscription_for using respond_to?(:id), but the same unsafe call on line 18 of subscription_active_for? was left untouched. Should be fixed for consistency.

---

Design

PlanResolution is a mutable Struct

Struct exposes writers by default, so a caller could do resolution.source = :assignment on a resolution that came from the default path, silently corrupting state. Since this is meant to be a read-only value object, consider calling .freeze at the end of initialize, or using Data.define (Ruby 3.2+) if the minimum Ruby version allows it.

to_h is fragile and will miss new fields

The custom to_h manually reconstructs every field rather than building on super. When a new field is added to the Struct it will not appear in to_h until someone remembers to add it here. super.merge(plan_key: plan_key, assignment_source: assignment_source) would keep it in sync automatically.

PlanResolver.plan_key_for is now redundant

The old plan_key_for was a thin convenience over effective_plan_for&.key. The new version calls resolution_for(plan_owner).plan_key — same cost as effective_plan_for. It is not surfaced on PlanOwner and does not appear to be called anywhere in the gem. If it is not part of the public API contract, it could be removed.

---

Minor

Residual debug log

The defined?(Pay) log line in PlanResolver#current_subscription_for was carried over from the old code and is now redundant since the pay_available? result is already logged on the line above.

---

What is good

• The single-resolution-path refactor is the right call; the old code duplicated the find-subscription logic across two separate query paths.
• Preserving the subscription on the resolution even when a manual assignment wins is a well-thought-out design — billing and entitlements are genuinely separate concerns.
• The validation in PlanResolution#initialize (raising on invalid source) is good defensive programming.
• Test coverage is comprehensive, including the edge case where a subscription exists but the processor_plan maps to no known plan (falls back to default but preserves the subscription object).
• The PaySupport#current_subscription_for safe-id fix is a good, minimal improvement.

[rameerez]

Follow-up for the latest review in 1989f84:

Addressed:

• fixed the remaining unsafe debug log in PaySupport.subscription_active_for? by guarding plan_owner.id the same way as current_subscription_for
• made PlanResolution immutable by freezing it at the end of initialization
• changed PlanResolution#to_h to build on super.merge(...) so new struct fields stay in sync automatically
• removed the redundant defined?(Pay) debug log line from PlanResolver#current_subscription_for
• added a docs note recommending current_pricing_plan_resolution when callers need both the effective plan and its provenance
• added focused tests for PlanResolution immutability / serialization and for PaySupport handling objects without id

Intentionally not changed:

• I did not add instance-level memoization for current_pricing_plan_resolution on PlanOwner; that would reduce duplicate lookups, but it would also introduce stale-state behavior on long-lived model instances. For this gem, I’d rather keep the helper fresh and document the "call resolution once" pattern when both plan and source are needed.
• I kept PlanResolver.plan_key_for as-is. It is small, already tested, and keeping it avoids unnecessary API churn in a provenance-focused PR.

Re-ran bundle exec rake test after the change.

[claude]

PR Review: Expose pricing plan resolution and provenance helpers

Overall this is a clean, well-motivated PR. The PlanResolution value object is a solid abstraction and the refactor to derive effective_plan_for from resolution_for is the right move. A few things worth addressing:

---

Bug / Behavioral Change

current_subscription_for no longer checks subscription_active_for? first

The old resolve_plan_from_pay called PaySupport.subscription_active_for? before fetching the subscription. The new PlanResolver#current_subscription_for skips that gate and calls PaySupport.current_subscription_for unconditionally (when Pay is available).

For users with Pay configured but no active subscription, every resolution_for call now makes an extra database/API round-trip that previously short-circuited. In write-heavy paths this could add up. And there's a subtle semantic shift: if PaySupport.current_subscription_for ever returns an inactive subscription record, the resolution result changes silently.

The tests currently pass because the test doubles return nil for inactive subscriptions from current_subscription_for, but the contract isn't made explicit. Worth adding a guard or a comment explaining the assumption.

---

Performance: No memoization on instance helpers

def current_pricing_plan_source current_pricing_plan_resolution.source # calls resolution_for → 1 DB query + Pay call end

A user who calls both current_pricing_plan and current_pricing_plan_source on the same object in a single request triggers two full resolution paths (two Assignment queries + two PaySupport calls). The docs note to prefer current_pricing_plan_resolution, but that's easy to miss.

Consider memoizing at the instance level:

def current_pricing_plan_resolution @current_pricing_plan_resolution ||= PlanResolver.resolution_for(self) end

The resolution is frozen and the sources it reads (assignment row, subscription) don't change mid-request in practice, so memoization is safe. You'd just need to clear it when assign_pricing_plan! / remove_pricing_plan! is called (which is already where you'd want a cache bust).

---

to_h includes live AR objects

def to_h super.merge(plan_key: plan_key, assignment_source: assignment_source) end

super here includes plan:, assignment:, and subscription: as raw Ruby objects. Any caller that pipes resolution.to_h into JSON serialization (logs, API responses, audit trails) will get whatever to_s/inspect or serializer those AR objects produce — which may be verbose or fail. Since the method's own comment says "commonly useful in serialization", it's worth either documenting this explicitly, or stripping the AR objects and keeping only scalar/serializable fields (plan_key, source, assignment_source, and maybe subscription.processor_plan).

---

Minor: OpenStruct in tests

plan_resolution_test.rb and a few others use OpenStruct.new. OpenStruct is soft-deprecated in modern Ruby (allocation is expensive, and it's being de-emphasized). Struct.new(:key) or Data.define(:key) (Ruby 3.2+) would be more idiomatic. Not urgent, but worth a note.

---

What's working well

• The single-path refactor in resolution_for is significantly cleaner than the old branching approach.
• PlanResolution being frozen is a great property for a value object — eliminates a class of accidental mutation bugs.
• The documented distinction between effective plan (entitlements) and billing state (subscription) is valuable and was missing before.
• Test coverage is thorough — the test_resolution_for_manual_assignment_keeps_underlying_subscription case in particular covers the tricky "assignment wins but billing context is preserved" scenario cleanly.
• pay_support.rb defensive respond_to?(:id) fix is a good hardening change.

---

Summary: The core design is solid. The two things I'd prioritize before merging are (1) clarifying/fixing the subscription_active_for? gate removal and its performance implications, and (2) documenting or adjusting to_h so it doesn't surprise callers expecting serializable output.

[rameerez]

Follow-up for the latest review in 88c69cc:

Addressed:

• made the current_subscription_for contract explicit in PlanResolver: it intentionally relies on PaySupport.current_subscription_for to return only active / trial / grace subscriptions, so the single resolution path can preserve billing context without changing entitlement semantics
• tightened the PlanResolution#to_h docs/comment so it no longer implies JSON-safe serialization; it now explicitly documents that the raw plan, assignment, and subscription objects are preserved
• added the same clarification to the model helpers docs, with guidance to build a scalar payload explicitly when callers need JSON-safe output
• replaced the new OpenStruct usages in plan_resolution_test.rb with small Struct stubs

Intentionally not changed:

• I still did not add instance-level memoization for current_pricing_plan_resolution. It would save duplicate lookups in some call patterns, but it also introduces cache invalidation concerns on mutable model instances. For this gem, I still prefer the explicit current_pricing_plan_resolution seam plus docs telling callers to reuse that object when they need both plan and provenance.

Re-ran bundle exec rake test after the change.

[claude]

Review — v0.3.3 plan resolution provenance

Thanks for the thorough follow-up across the prior rounds. The core struct is in good shape: frozen PlanResolution, to_h via super.merge, respond_to?(:id) guards, and Struct stubs in tests are all clean. A few observations that haven't been fully resolved:

---

1. effective_plan_for / current_pricing_plan now always incur 2 DB queries

File: lib/pricing_plans/plan_resolver.rb:9

def effective_plan_for(plan_owner) resolution_for(plan_owner).plan # fetches BOTH assignment + subscription end

The old implementation short-circuited:

# Old: for users with a manual assignment → 1 DB query, subscription never fetched if assignment return Registry.plan(assignment.plan_key) end

Now resolution_for always calls current_subscription_for before checking assignment. For apps where most users have manual assignments (admin overrides, demos, trials), current_pricing_plan now silently doubles the DB work. Since effective_plan_for is the most-called method in this gem (feature checks, limit checks, every request), this is worth benchmarking. If the extra query is acceptable, it's worth a note in the CHANGELOG.

---

2. current_pricing_plan_source causes double resolution (still open)

File: lib/pricing_plans/plan_owner.rb:202–205

def current_pricing_plan_source current_pricing_plan_resolution.source # full resolution, uncached end

The docs now say "prefer calling current_pricing_plan_resolution once" — which is good guidance. But if a caller does user.current_pricing_plan and then user.current_pricing_plan_source, that's 4 DB queries where 2 would suffice. The decision not to memoize is defensible, but the docs should be more explicit that calling both is expensive, not just redundant.

---

3. Semantic oddity: source: :default with non-nil subscription

File: lib/pricing_plans/plan_resolver.rb:47–52 and test/services/plan_resolver_test.rb:61

# When subscription has an unrecognized processor_plan: resolution.source # => :default resolution.subscription # => <subscription with processor_plan: "price_unknown_999">

The test test_resolution_for_unknown_subscription_plan_falls_back_to_default_but_keeps_subscription covers this, but the docs don't. A caller reading source == :default naturally assumes no subscription is present. Consider adding a note to the PlanResolution class comment or docs/03-model-helpers.md clarifying that subscription can be non-nil even when source == :default (e.g., when the subscription's Stripe price ID isn't mapped to a configured plan).

---

Minor: plan_key_for public API regression

PlanResolver.plan_key_for now calls resolution_for (2 queries) where it previously called effective_plan_for (1 query for assigned users). Any external callers relying on this being lightweight will silently regress. Low priority since this is a public gem method — worth a CHANGELOG note if the double-query behavior is intentional.

---

Overall

The design goal is clear and well-tested. The PlanResolution value object is a good abstraction. The main thing to resolve before merging is deciding whether the silent query-cost increase on effective_plan_for / current_pricing_plan is acceptable or needs mitigation (lazy subscription fetch, or a note in the CHANGELOG at minimum).

[rameerez]

Compatibility note on the 0.3.3 change set:

I did a final backward-compatibility pass against origin/main before this branch. My conclusion is:

• high confidence this is backward-compatible for normal installs using the documented/public API
• not something I would describe as a blanket guarantee for every possible installation, because that would include apps reaching into private internals or monkey-patching gem internals

Why I consider the upgrade safe for normal consumers:

• no Ruby version or runtime dependency change in the gemspec
• no public API removals or signature changes in the existing PlanOwner helpers
• existing plan resolution precedence is preserved: manual assignment -> active/trial/grace subscription -> default plan
• the old/public helpers still exist and keep their roles: current_pricing_plan, on_free_plan?, assign_pricing_plan!, remove_pricing_plan!, plan_allows?, PlanResolver.effective_plan_for, PlanResolver.plan_key_for
• the provenance work is additive: current_pricing_plan_resolution, current_pricing_plan_source, PlanResolution, and PlanResolver.resolution_for
• the test suite passes after the refactor

Two caveats worth being explicit about:

• if a host app was reaching into private internals like PlanResolver#resolve_plan_from_pay, that private seam changed and was not treated as compatibility surface
• when a manual assignment exists, resolution_for now still fetches the active subscription so the resolution can preserve billing context; that should not change entitlement outcomes, but it is a small performance/behavioral difference at the internal level

So the compatibility claim I am comfortable making is: safe additive release for public API consumers, with the usual caveat that private/internal hooks were never part of the contract.

[claude]

PR Review: Expose pricing plan resolution and provenance helpers

Overall this is a well-designed, well-tested PR. The PlanResolution value object is clean, the refactor to a single resolution code path eliminates real duplication, and preserving the underlying subscription even when an assignment wins is a thoughtful API decision. A few things worth discussing:

---

Performance regression in effective_plan_for / current_pricing_plan

plan_resolver.rb:21-22 — resolution_for now unconditionally runs both the assignment query and the subscription query before branching:

assignment = assignment_for(plan_owner) subscription = current_subscription_for(plan_owner) # always runs

Before this PR, a manual assignment caused an early return without ever touching Pay/subscriptions. Now every call to current_pricing_plan costs 2 DB queries regardless. This is intentional (to populate subscription in the assignment-backed resolution), but it's a non-obvious trade-off that changes the cost of effective_plan_for for everyone, not just callers who need provenance.

Suggestion: Either add a comment on resolution_for explicitly noting this trade-off (so the next person doesn't try to "fix" it), or consider lazy-fetching subscription only when needed (though this complicates the struct construction).

---

Inconsistent subscription preservation: inactive vs. unknown-plan subscriptions

The PR preserves subscription on the resolution when an active subscription's processor_plan doesn't match any registry plan (falls back to default), but resolution.subscription is nil when the subscription is inactive — because PaySupport.current_subscription_for filters out inactive records before they even reach resolution_for.

This means:

• Active sub, unknown Stripe price → resolution.subscription is non-nil, source: :default
• Cancelled/inactive sub → resolution.subscription is nil, source: :default

The test test_inactive_payment_processor_subscription_falls_back_to_default documents the second case (assert_nil resolution.subscription), but a caller reading the API might expect that "source: :default with a subscription" always means "active sub, unrecognized plan." That's currently true, but it's a subtle invariant not stated anywhere.

Suggestion: Add a note in the PlanResolution class docs (or the edge-case section already in docs/03-model-helpers.md) clarifying that subscription is only non-nil for active subscriptions, and nil for inactive/cancelled ones.

---

current_pricing_plan_source is a convenience footgun

plan_owner.rb:204-206

def current_pricing_plan_source current_pricing_plan_resolution.source end

The performance docs do warn about calling these separately, which is good. But current_pricing_plan_source creates a full resolution (2 queries) just to extract .source. Anyone doing:

if user.current_pricing_plan_source == :assignment plan = user.current_pricing_plan # 2 more queries end

…pays 4 queries for what should be 1. Given that current_pricing_plan_resolution is the canonical API, consider whether this convenience method is worth the footgun risk, or whether the performance warning in the docs should be more prominent (e.g., a # NOTE: comment directly on the method).

---

Shallow freeze semantics worth noting

plan_resolution.rb:14 — freeze makes the struct's member pointers immutable, but plan, assignment, and subscription remain mutable objects. This is standard Ruby, and the to_h comment already flags it. Just noting it for completeness in case someone adds equality/hash caching later and relies on deep immutability.

---

Minor: to_h is inconsistent about object vs scalar fields

to_h returns a mix: raw Plan, Assignment, and Pay::Subscription objects alongside scalar plan_key and assignment_source. The docs note "build one explicitly from the scalar fields you care about" for JSON, which is the right guidance. Could add # :nodoc: or a # For debugging/tests only comment on to_h to make the intent unambiguous.

---

What's good

• Single resolution code path — effective_plan_for and plan_key_for both delegate cleanly to resolution_for. No more duplicated branching logic.
• Input validation on source at construction time with a clear error message.
• freeze in the constructor is the right call for a value object.
• Test coverage is thorough: all three resolution paths, the unknown-plan edge case, the assignment-wins-but-keeps-subscription case, and the frozen/mutation-protection test.
• Docs clearly distinguish "effective plan" from "billing state" — this is a genuinely useful conceptual clarification.
• The pay_support.rb defensive respond_to?(:id) fix is a good robustness improvement.

Looks good to merge with the performance trade-off at minimum documented in the code.