Handle subscription plan changes (upgrades & downgrades) #20
Conversation
- Detect when processor_plan changes on active subscriptions - Update fulfillment metadata with new plan ID - Add test for plan change detection - All 443 tests passing This is the first incremental step toward handling subscription upgrades and downgrades. The fulfillment now tracks plan changes so subsequent credit fulfillments will use the correct plan. Related to issue #12
This commit adds comprehensive support for subscription plan upgrades: ## What's New - Detects when users upgrade to a plan with more credits - Immediately grants full credits from the new plan - Updates fulfillment metadata to track the new plan - Adds 'subscription_upgrade' transaction category - All subsequent fulfillments will use the new plan's credit amount ## Implementation Details - Added after_commit callback for plan changes - Compares old vs new plan credits_per_period to detect upgrades - Transactions are atomic with proper error handling - Downgrades currently just update metadata (full impl coming next) ## Tests - Test for basic plan change detection and metadata update - Test for upgrade scenario with immediate credit grant - All 444 tests passing with 84.75% line coverage ## Related to Issue #12 This addresses the core problem where users upgrading plans weren't receiving credits from their new plan. The fulfillment system now properly tracks plan changes and awards credits accordingly. Next: Implement downgrade scheduling for end-of-period transitions
This commit adds full support for subscription plan downgrades: ## What's New - Detects when users downgrade to a plan with fewer credits - Schedules the downgrade for end of current billing period - Users keep current plan benefits until period ends - Automatically applies the scheduled downgrade on renewal - Credits are NOT deducted (users keep what they've earned) ## Implementation Details - Downgrades stored as pending_plan_change in fulfillment metadata - plan_change_at tracks when the change should occur - On renewal, apply_pending_plan_change executes the scheduled change - Clean separation between scheduling and application - All transitions are atomic with proper error handling ## User Experience User downgrades from premium (2000/month) to starter (500/month): 1. ✅ Keeps their 2000 credits until period ends 2. ✅ Downgrade scheduled for current_period_end 3. ✅ On next renewal, switches to 500 credits/month 4. ✅ No credit clawback or confusing deductions ## Tests - Test for downgrade scheduling (no immediate change) - Test for downgrade application on renewal - All 446 tests passing with 85.09% line coverage ## Related to Issue #12 Combined with the upgrade feature, this fully solves the subscription plan change problem. Users can now freely upgrade/downgrade and the fulfillment system handles it gracefully: - Upgrades: Immediate credits - Downgrades: End-of-period transition This matches standard SaaS expectations and Stripe's default behavior.
Pull Request Review: Handle Subscription Plan ChangesOverall AssessmentThis is a well-designed, thoroughly documented PR that addresses a critical gap in the gem's functionality. The implementation follows SaaS industry best practices and includes comprehensive test coverage. The PR description is exemplary. ✅ Strengths
🔍 CRITICAL Issues1. Credit Expiration Not Set on Upgrades
|
✅ Summary of Work CompletedThis PR now fully implements subscription plan change handling with comprehensive safety checks and regression coverage. Below is a complete log of the issues we identified (starting from the original reviewer concerns), the additional pitfalls discovered during review, and the exact fixes applied. 🧩 Original Review Issues (Claude) and Fixes1) Upgrade credits did not expire for expiring plans (CRITICAL)Problem: 2) Nil old plan when upgrading from non‑credit plan (HIGH)Problem: Upgrading from a non‑credit plan resulted in 3)
|
Code Review: Handle Subscription Plan Changes (PR #20)SummaryThis PR implements a comprehensive solution for handling subscription plan changes (upgrades and downgrades). The implementation is well-architected with excellent test coverage and thorough documentation. The code quality is high, and the approach follows SaaS industry standards. ✅ Strengths1. Excellent Architecture & Design
2. Strong Test Coverage
3. Code Quality
4. Thorough Documentation
🐛 Potential Bugs & IssuesCritical1. Race Condition: Plan Change During Fulfillment Job (Medium Priority) # pay_subscription_extension.rb:296-299
if fulfillment.metadata["pending_plan_change"].present?
apply_pending_plan_change(fulfillment)
endIssue: If a user upgrades after scheduling a downgrade, but before the renewal applies it, the upgrade clears the pending change. However, if the FulfillmentJob runs between the downgrade scheduling and the renewal, it could award credits based on the wrong plan. Recommendation: Consider adding a 2. Credit Expiration Calculation for Upgrades (Low-Medium Priority) # pay_subscription_extension.rb:391
base_time = current_period_end || (Time.current + new_plan.parsed_fulfillment_period)Issue: When Recommendation: Add a guard clause or log a warning when Minor3. Inconsistent Time Methods (Low Priority) # Line 265: Time.now
# Line 243: Time.currentIssue: Mixed use of Recommendation: Standardize on 4. Memoization Removed Without Thread Safety Consideration (Low Priority) # pay_subscription_extension.rb:44-46
# NOTE: Not memoized because processor_plan can change, and we need the current value
def credit_subscription_plan
UsageCredits.configuration.find_subscription_plan_by_processor_id(processor_plan)
endIssue: While removing memoization is correct for handling plan changes, this means every call performs a lookup. If Recommendation: Consider adding a note about the performance characteristics of 🔒 Security ConcernsNo significant security issues identified. The implementation properly:
Minor consideration: The metadata stored includes plan IDs and subscription IDs. Ensure these don't contain sensitive information if the database is compromised. ⚡ Performance ConsiderationsGood Practices
Potential Optimizations1. N+1 Query Potential (Low Priority) # pay_subscription_extension.rb:350-351
old_plan = UsageCredits.configuration.find_subscription_plan_by_processor_id(old_plan_id)
new_plan = UsageCredits.configuration.find_subscription_plan_by_processor_id(new_plan_id)If these lookups hit the database, this could be optimized. However, if they're in-memory lookups from the configuration DSL (which they likely are), this is fine. 2. Multiple Metadata Updates 🧪 Test Coverage AnalysisWell-Covered Scenarios ✅
Missing Test Coverage 🔴1. Multiple Plan Changes in One Period # Missing test for:
subscription.update!(processor_plan: "premium") # Upgrade, get 2000 credits
subscription.update!(processor_plan: "pro") # Downgrade in same period
# What happens to the 2000 credits? What does fulfillment metadata show?2. Plan Change + Renewal Race Condition # Missing test for:
subscription.update!(processor_plan: "pro") # Schedule downgrade
# FulfillmentJob runs here - what plan does it use?
subscription.update!(current_period_end: 60.days.from_now) # Renewal3. Plan Change from Non-Credit to Credit # Non-credit plan → Credit plan (should create new fulfillment)4. Plan Change During Trial Period # User on trial upgrades/downgrades before trial ends5. Same-Credit Different-Period Plan Changes # pay_subscription_extension.rb:372-374
else
# Same credits amount (maybe different period?) - just update metadata
update_fulfillment_plan_metadata(fulfillment, new_plan_id)
endThis branch isn't tested. What happens if you switch from "500/month" to "500/week"? 📝 Code Style & Best PracticesExcellent ✅
Minor Suggestions1. Magic Strings in Metadata fulfillment.metadata["pending_plan_change"]
fulfillment.metadata["plan_change_at"]
fulfillment.metadata["stopped_reason"]Recommendation: Consider extracting these as constants: METADATA_PENDING_PLAN_CHANGE = "pending_plan_change"
METADATA_PLAN_CHANGE_AT = "plan_change_at"
METADATA_STOPPED_REASON = "stopped_reason"2. Method Complexity
🎯 RecommendationsHigh Priority
Medium Priority
Low Priority
🎉 ConclusionThis is high-quality work that solves a real problem comprehensively. The implementation is thoughtful, well-tested, and production-ready. The PR description alone sets a gold standard for documentation. Approval Status: ✅ APPROVE with minor suggestionsThe identified issues are mostly minor edge cases and test coverage gaps that can be addressed in follow-up PRs. The core functionality is solid and ready to merge. Great work! 🚀 Reviewed by: Claude Code |
✅ PR Update: New Changes + Comprehensive Test CoverageKey Code Updates Since Last Review
🧪 New Tests Added (Extremely Thorough Coverage)✅ Multiple Plan Changes in One Period (New Comprehensive Suite)
✅ FulfillmentJob Timing & Pending Downgrade Behavior
✅ Metadata Integrity & Consistency
✅ Existing Edge Case Coverage (Previously Added, Still Valid)Upgrade/Downgrade & Expiration
Reactivation Scenarios
Lateral & Period Change
Pending Change Lifecycle
Trial Behavior (Documented Intentional Behavior)
Boundary Conditions
✅ SummaryThis update closes the reviewer’s remaining concerns and strengthens correctness around multiple plan changes, pending downgrade handling, and FulfillmentJob timing. The new tests aggressively guard against real-world billing edge cases and prevent credit exploits (duplicate credits from “canceling” downgrades). |
Code Review: Handle Subscription Plan ChangesThank you for this comprehensive PR! The implementation is well-thought-out with excellent test coverage. Overall AssessmentVerdict: Strong implementation with minor issues to address before merge. Strengths:
Issues Found: 2 critical, 5 high priority, 7 medium priority Critical Issues (Must Fix)1. Race Condition in Credit Fulfillment (Lines 67-73)File: lib/usage_credits/models/concerns/pay_subscription_extension.rb The documented TODO about Pay's double-update race condition is concerning. While you handle this with uniqueness validation on Fulfillment, this is fragile and could lead to double credit awards or inconsistent state. Recommendation: Implement proper idempotency using find_or_create_by with explicit locking and an idempotency flag in metadata. 2. Credits Can Expire in the Past (Lines 158-162)File: lib/usage_credits/models/concerns/pay_subscription_extension.rb If current_period_start is in the past (e.g., subscription paused for 2 months then reactivated), credits could be created with expiration dates already past. Example: Subscription paused 2 months ago (current_period_start = 2.months.ago), user reactivates today, credits awarded with expires_at = 2.months.ago + 1.month = 1.month.ago (already expired!) Recommendation: Use expiration_base = [current_period_start || Time.current, Time.current].max High Priority Issues3. Typo: fullfillment_should_stop_atMissing 'i' in fulfillment at lines 54, 246, 269, 304, 323. Rename to fulfillment_should_stop_at for consistency. 4. Missing Plan Validation in apply_pending_plan_changeNo validation that the pending plan still exists in configuration. If an admin removes a plan from config after a user schedules a downgrade to it, renewal will apply an invalid plan ID. Recommendation: Add validation to check if the pending plan exists before applying it, and log an error if not found. 5. Missing Test Coverage for Critical Edge CasesAdd tests for:
Medium Priority Issues6. Complex Method: credits_already_fulfilled?The method handles 3 different concerns. Break into smaller, single-purpose methods for clarity. 7. Repeated Expiration Calculation LogicLines 158-162 and 406-411 duplicate expiration calculation. Extract to helper method. 8. Nil current_period_end Warning Without FallbackCode continues with potentially incorrect calculations after logging warning. Use sensible default like current_period_end || (Time.current + 30.days) 9. Transaction Bulk Update Without Error HandlingAdd error handling to log when expected transaction updates don't match actual updates. What's GoodExcellent Practices:
Code Quality Highlights:
Architecture NotesDesign Decisions (Good):
The Future Work & Known Limitations section is excellent and shows awareness of paused subscriptions, multiple plan changes, proration, and credit expiration policies. Recommended Actions Before Merge
Test Coverage AssessmentCurrent: 446 tests, 879 assertions, 85.09% line coverage Excellent coverage for: Basic flows, upgrades/downgrades, metadata, transactions, reactivation Missing coverage for: Past expiration dates, invalid pending plans, concurrent updates, nil edge cases ConclusionThis is high-quality work with thoughtful implementation and comprehensive testing. The issues identified are addressable with small, targeted fixes. The architecture is sound and follows Rails/Ruby best practices. Estimated effort to address critical issues: 1-2 hours Once the critical issues are resolved, this will be an excellent addition to the gem. Great work on the thorough PR description and Future Work documentation! |
Summary of Changes Made Based on Reviewer's Comments✅ Fixed (Legitimate Issues)
✅ Tests Added
❌ Skipped (Overengineering)
|
…) + fix "fulfillment" typo
Code Review: Handle Subscription Plan Changes (Upgrades & Downgrades)Thanks for this comprehensive PR! The implementation is well-thought-out and the PR description is exceptionally detailed. Here's my review: ✅ Strengths1. Excellent Documentation
2. Robust Testing
3. Sound Architecture
4. Industry-Standard UX
🔍 Code Quality Issues1. Typo Fixed (lib/usage_credits/configuration.rb:59)Good catch on the typo: 2. Inconsistent Method Naming (lib/usage_credits/models/concerns/pay_subscription_extension.rb:53)def fulfillment_should_stop_atThis was changed from 3. Memoization Removed from
|
Analysis✅ Already Addressed
❌ Skip (Overengineering)
🐛 Real Bug to FixSchedule time in past for downgrades - Same issue as expirations: schedule_time = current_period_end || Time.current # Could be in the past!Summary: Reviewer's Comments Assessment✅ Fixed (1 Real Bug)
✅ Already Covered (Reviewer Didn't See Them)
❌ Skip (Overengineering / Not Real Issues)
The PR is solid. The reviewer's concerns about "credit gaming" are actually not exploitable because:
Anti-Gaming Verification Summary✅ Confirmed: The System CANNOT Be GamedI've verified the logic and added comprehensive tests proving these scenarios are blocked:
The Core Protection Logic# Line 372-375 in pay_subscription_extension.rb
if current_plan_id == new_plan_id
clear_pending_plan_change(fulfillment)
return # NO CREDITS
endWhere:
Key insight: When user schedules a downgrade, Only Legitimate Upgrades Grant CreditsThe only way to get upgrade credits is: new_credits > current_creditsWhere |
… by repeatedly upgrading / downgrading
Code Review: Handle Subscription Plan Changes (Upgrades & Downgrades)SummaryThis is an excellent, well-architected implementation that addresses a critical gap in the subscription fulfillment system. The PR demonstrates thorough planning, strong technical execution, and comprehensive documentation. The code quality is high with thoughtful edge case handling. ✅ StrengthsArchitecture & Design
Code Quality
Testing
Documentation
🔍 Issues & Concerns1. CRITICAL: Return Inside Transaction BlockLocation: The Recommendation: if current_plan_id == new_plan_id
clear_pending_plan_change(fulfillment)
else
# Move all subsequent logic here
current_credits = current_plan&.credits_per_period || 0
new_credits = new_plan.credits_per_period
# ... rest of logic
end2. Medium: Potential Credit Gaming Through Rapid UpgradesLocation: Issue: A user could potentially:
The PR description acknowledges this (section 4.2), but there's no rate limiting or detection. Recommendation: Consider adding:
3. Medium: Credit Proration MismatchLocation: Issue: User pays prorated billing amount but receives full credits. Example:
Impact: Revenue leakage if credits have real-world value Recommendation: Calculate prorated credits based on remaining billing period 4. Code Smell: Nested Transaction BlockLocation: The Recommendation: Remove the inner transaction block since it's already wrapped 🎯 Suggestions for Improvement5. Test Coverage Gaps (Per PR Description)The PR description excellently identifies missing test cases. Prioritize: High Priority:
Medium Priority:
6. Webhook TestingThe code assumes Recommendation: Add integration tests with actual Stripe test mode webhooks to verify real-world behavior. 📊 Test Results Analysis✅ All 446 tests passing The test suite is comprehensive and well-structured. Great work on maintaining backward compatibility! 🔒 Security Review✅ No SQL injection risks - all queries use parameterized ActiveRecord methods 📝 DocumentationThe PR description is exceptional - it could serve as a case study for how to document complex features. Consider:
Final VerdictStatus: ✅ Approve with minor revisions This PR represents high-quality work. The critical issue (#1) should be fixed before merge, but it's a simple refactor. Issues #2 and #3 are business logic considerations that can be addressed in follow-up PRs. The author has clearly thought through the problem space, implemented a solid solution, and documented it thoroughly. The "Future Work & Known Limitations" section demonstrates excellent engineering maturity. Recommended merge strategy:
Reviewed by: Claude Sonnet 4.5 |
Analysis of Reviewer's IssuesIssue 1: "Return Inside Transaction Block" ❌ NOT A BUGIn Ruby/Rails,
The transaction only rolls back on:
This is a style preference, not a bug. The early return pattern is common Ruby idiom and reduces nesting. Issue 2: "Credit Gaming Through Rapid Upgrades" ❌ ALREADY PROVEN IMPOSSIBLEThe reviewer's scenario:
This is NOT gaming! This is expected behavior:
The user paid for each billing period correctly. The actual gaming attempt (within one period) is:
We have 6 anti-gaming tests proving this is BLOCKED: Issue 3: "Credit Proration Mismatch" ❌ BUSINESS DECISION, NOT A BUG
This is a business decision, not a bug. Many SaaS products do this because:
Already documented in "Future Work" as a consideration. Not blocking. Issue 4: "Nested Transaction Block" ❌ REVIEWER IS WRONGThe reviewer misread the code. Look carefully: # Line 361-364: Called BEFORE transaction
if new_plan.nil? && current_plan.present?
handle_downgrade_to_non_credit_plan(fulfillment)
return # ← EXITS HERE, before transaction
end
return unless new_plan
# Line 368: Transaction starts HERE
ActiveRecord::Base.transaction do
# ...
end
The transaction inside Verdict
All 4 issues are either non-issues or already addressed. The reviewer may not have seen our 6 anti-gaming tests. |
|
@claude please review the current state of the PR with the latest changes and latest comments |
|
Claude finished @rameerez's task in 2m 15s —— View job PR Review: Handle Subscription Plan ChangesI've completed a thorough review of this PR in its current state, including all the feedback cycles and fixes that have been applied. Here's my assessment: ✅ Overall Verdict: APPROVE - Ready to MergeThis is excellent work with thoughtful implementation, comprehensive testing, and outstanding documentation. The author has thoroughly addressed previous feedback and added robust protections against edge cases. 🎯 Code Quality AssessmentStrengths1. Rock-Solid Anti-Gaming Protection
The key insight: When a downgrade is scheduled, 2. Excellent Bug Fixes
3. Outstanding Architecture
4. Test Coverage
📊 Analysis of Previous Review FeedbackThe author's dismissal of previous review comments is justified. Let me validate each: Issue 1: "Return Inside Transaction Block" ❌ Not a BugReviewer claim: Return inside transaction is confusing/dangerous In Rails, Issue 2: "Credit Gaming" ❌ Already ImpossibleReviewer claim: Users could game the system by rapid upgrades/downgrades The gaming scenario described by the reviewer does not work with this implementation:
The only way to get upgrade credits is a genuine upgrade to a plan with more credits, which Stripe charges for. Issue 3: "Credit Proration Mismatch" ❌ Business DecisionReviewer claim: User pays 50% prorated but gets 100% credits Many SaaS products grant full credits on upgrades (better UX, incentivizes upgrades). Already documented in "Future Work" section. Not a bug. Issue 4: "Nested Transaction Block" ❌ Reviewer Misread CodeReviewer claim: Transaction nesting at line 447 Looking at lines 361-364: if new_plan.nil? && current_plan.present?
handle_downgrade_to_non_credit_plan(fulfillment)
return # ← Exits method BEFORE line 368 transaction
endThe 🔍 Remaining ConsiderationsNon-Issues (Correctly Dismissed)
Legitimate Future Work (Already Documented)The PR description's "Future Work" section excellently identifies areas for potential enhancement:
These are enhancement opportunities, not blocking issues. 🎓 What Makes This PR Excellent
✅ Final RecommendationStatus: APPROVE - Ready to Merge Confidence: High - I've verified:
No changes required. This PR solves a critical issue (#12) with a well-architected, thoroughly tested solution. Suggestions for Follow-Up (optional, non-blocking):
Great work! 🎉 This implementation demonstrates excellent engineering practices and deep understanding of the problem domain. |
Pull Request Review: Handle Subscription Plan ChangesSummaryThis is an excellent, production-ready implementation that addresses a critical gap in the subscription credit fulfillment system. The PR thoroughly handles plan changes (upgrades/downgrades) with thoughtful architecture, comprehensive testing, and detailed documentation. ✅ Strengths1. Exceptional Documentation
2. Robust Architecture
3. Comprehensive Test Coverage
4. Excellent Code Quality
5. Security & Safety
🔍 Potential Issues & RecommendationsHigh Priority1. Typo in Method Name (pay_subscription_extension.rb:53, 243, 266, 301, 320)There's an inconsistency: the method is called 2. Race Condition with Pending Plan Changes (pay_subscription_extension.rb:294-298)When
Mitigation: The existing row locking in FulfillmentService (line 31) should prevent this, but consider adding a comment explaining the safety mechanism. 3. Credit Expiration Edge Case (pay_subscription_extension.rb:523-532)The Recommendation: Add validation or logging when def calculate_credit_expiration(plan, base_time = nil)
return nil if plan.rollover_enabled
effective_base = [base_time || Time.current, Time.current].max
# Warn if base_time was provided but in the past
if base_time && base_time < Time.current
Rails.logger.warn "Credit expiration base_time is in the past (#{base_time}), using Time.current instead"
end
effective_base + plan.parsed_fulfillment_period + UsageCredits.configuration.fulfillment_grace_period
endMedium Priority4. Memoization Removed from
|
Handle Subscription Plan Changes (Upgrades & Downgrades)
Fixes #12
🎯 Problem Statement
Original Issue
Users reported that after upgrading their subscription plan using Pay's
.swap()method, they would receive an initial credit grant but subsequent recurring credits would not be awarded. The fulfillment system continued awarding credits based on the old plan.Example scenario:
give_credits()Root Cause
The
PaySubscriptionExtensionmodule (which integrates Pay with UsageCredits) had a TODO comment indicating this was a known gap:# TODO: handle plan changes (upgrades / downgrades)When a subscription plan changed:
.swap()method updated theprocessor_planfield in the subscription recordFulfillmentrecord's metadata still referenced the old plan IDFulfillmentServicelooked up the plan usingfulfillment.metadata["plan"], which never changed🔍 Investigation & Research
Understanding Pay's Swap Behavior
Researched Pay gem documentation to understand how
.swap()works:processor_planprocessor_planfield changes and Rails providessaved_change_to_processor_plan?for detectioncustomer.subscription.updatedwebhook withprevious_attributesArchitecture Analysis
Examined the fulfillment system architecture:
Pay::Subscriptionhas ONEFulfillmentrecord (enforced by uniqueness constraint)Fulfillmentstores plan metadata:{ plan: "processor_plan_id", ... }FulfillmentService#processlooks up plan via:@fulfillment.metadata["plan"]Design Decisions Research
Investigated standard SaaS practices for plan changes:
✅ Solution Implemented
1. Plan Change Detection
Added
after_commitcallback to detect plan changes:2. Upgrade Handling (Immediate)
When user upgrades to a plan with more credits per period:
What happens:
"subscription_upgrade"3. Downgrade Scheduling (End-of-Period)
When user downgrades to a plan with fewer credits per period:
What happens:
current_period_endpending_plan_change4. Downgrade Application (On Renewal)
Enhanced renewal callback to apply scheduled downgrades:
What happens:
5. New Transaction Category
Added
"subscription_upgrade"to allowed transaction categories inTransactionmodel to track upgrade credit grants separately from regular subscription credits.🧪 Testing
Test Coverage
New Tests
test_plan_change_updates_fulfillment_metadata- Basic plan change detectiontest_upgrade_to_higher_plan_grants_immediate_credits- Full upgrade flowtest_downgrade_schedules_plan_change_for_end_of_period- Downgrade schedulingtest_scheduled_downgrade_applies_on_subscription_renewal- Downgrade applicationTest Scenarios Covered
📊 User Experience
Upgrade Flow
Downgrade Flow
🚧 Future Work & Known Limitations
Not Yet Implemented
1. Paused Subscriptions
paused?check to fulfillment conditions2. Multiple Plan Changes in Single Period
3. Plan Changes During Trial Period
4. Proration Handling
5. Downgrade to Non-Credit Plan
6. Credit Expiration on Downgrade
Edge Cases to Monitor
1. Webhook Race Conditions
2. Fulfillment Job Timing
3. Manual Subscription Updates
processor_planin database/console4. Subscription Cancellation After Downgrade Scheduled
Testing Gaps
Manual Testing Needed
.swap()Additional Automated Tests Needed
Documentation Updates Needed
subscription_upgradetransaction category🔄 Breaking Changes
None. This is a purely additive change that doesn't modify existing behavior for subscriptions that don't change plans.
📝 Related Issues
PaySubscriptionExtension(lines 39-40)🎓 References
Ready for review! This implementation follows SaaS industry standards and matches Stripe's default behavior for plan changes. All tests passing with improved coverage.