Fix credit pack fulfillment not working with Pay 10+ (Stripe data in object vs data in Pay::Charge)

[rameerez]

In Pay 10+, the Stripe charge object is stored in the object column, not data. The succeeded? method is reading from data["status"], which returns nil in Pay 10+, causing credits to never be fulfilled.

I had already done some work to migrate usage_credits to Pay 10+ (#19) but looks like I missed this.

We add a charge_object_data helper that checks object first (Pay 10+), then falls back to data (legacy Pay).

---

Summary

Fixes #1 - Credits not being fulfilled after successful credit pack purchase with Pay 10+.

This PR addresses a critical bug where credit packs were not being fulfilled when using Pay gem version 10.0 or later, even though charges were succeeding in Stripe.

Root Cause Analysis

The Problem

User @onurozer identified the issue in Issue #1:

charge = Pay::Charge.last 
charge.succeeded? # returns false - fulfillment doesn't happen
charge.data["status"] # returns nil
charge.object["status"] # returns "succeeded"

Pay Gem Architecture Change in Version 10.0

Starting with Pay 10.0 (released late 2024), the gem introduced a new object column to store the complete Stripe response object. This is documented in Pay's UPGRADE.md:

Pay 9.0 to Pay 10.0: Pay has introduced an object column on pay_customers pay_charges and pay_subscriptions to save a full copy of the Stripe objects to make future changes easier.

Where Data Lives in Each Version

Field	Pay < 10.0	Pay 10.0+
status	data["status"]	object["status"]
amount_captured	data["amount_captured"]	object["amount_captured"]
Payment method info	data[...]	data[...] (via store_accessor)

Relevant Pay Gem Code

From pay/app/models/pay/stripe/charge.rb line 32:

attrs = {
  object: object.to_hash,  # <-- Full Stripe charge object goes here
  amount: object.amount,
  # ...
}

The data column now only stores store_accessor fields like stripe_invoice and stripe_receipt_url (lines 8-9), NOT the raw Stripe response.

Our Code Before This Fix

From lib/usage_credits/models/concerns/pay_charge_extension.rb:

def succeeded?
  case type
  when "Pay::Stripe::Charge"
    status = data["status"]  # <-- Returns nil in Pay 10+!
    # ...
  end
end

The Fix

Added a new helper method charge_object_data that checks object first (Pay 10+), then falls back to data (legacy Pay):

def charge_object_data
  # Pay 10+ stores full Stripe object in `object` column
  if respond_to?(:object) && object.is_a?(Hash) && object.any?
    object.with_indifferent_access
  # Older Pay versions stored charge details in `data` column
  elsif data.is_a?(Hash) && data.any?
    data.with_indifferent_access
  else
    {}.with_indifferent_access
  end
end

Files Changed

File	Change
lib/usage_credits/models/concerns/pay_charge_extension.rb	Added charge_object_data helper, updated succeeded? to use it
test/models/concerns/pay_charge_extension_test.rb	Added 9 new tests covering Pay 10+ scenarios
test/dummy/db/migrate/20250416000000_add_object_to_pay_models.rb	Migration for test database
test/dummy/db/schema.rb	Updated schema

Tests Added

Pay 10+ Object Column Tests

• ✅ succeeded? returns true when status is in object column
• ✅ succeeded? returns false for status failed in object
• ✅ succeeded? returns false for status pending in object
• ✅ succeeded? returns false for status canceled in object
• ✅ object takes precedence over data when both have values

Legacy Pay Data Column Tests

• ✅ succeeded? falls back to data column when object is empty
• ✅ charge_object_data returns data when object is nil

Edge Cases

• ✅ Fallback to amount_captured comparison when status is nil
• ✅ Partial capture scenarios

Integration Test (Exact Issue #1 Scenario)

test "Pay 10+ integration: credits ARE fulfilled when status is in object column (Issue #1 fix)" do
  # This is the EXACT scenario from Issue #1:
  # - charge.data["status"] returns nil
  # - charge.object["status"] returns "succeeded"
  
  # ... creates charge with object["status"] = "succeeded", data["status"] = nil
  
  assert_nil charge.data["status"], "data['status'] should be nil (as reported in Issue #1)"
  assert_equal "succeeded", charge.object["status"]
  assert charge.succeeded?
  assert_equal 1000, wallet.reload.credits, "Credits should be fulfilled"
end

Backward Compatibility

This fix maintains full backward compatibility with all supported Pay versions:

Pay Version	Behavior
Pay 8.3.x	Uses data column (no object column exists) ✅
Pay 9.0.x	Uses data column ✅
Pay 10.x	Uses object column ✅
Pay 11.x	Uses object column ✅

Test Results

673 runs, 1516 assertions, 0 failures, 0 errors, 0 skips

Related PRs

This issue was not caught by previous Pay upgrade PRs because they focused on:

• Update Pay gem dependency to support version 8.3 to < 10.0 #10: Initial Pay dependency update
• Update Pay gem dependency to support versions 8.3 to 9.x #16: Pay 10.0 testing
• Add Pay version matrix testing with Appraisal #17, Upgrade Pay dependency to support version 10.x #18: Intermediate fixes
• Upgrade Pay dependency to support version 11.x #19: Pay 11.x association renames

None of these addressed the data vs object column data structure change.

---

🤖 Generated with Claude Code

[cursor]

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 6.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

[claude]

Pull Request Review: Fix credit pack fulfillment not working with Pay 10+ (Issue #1)

Overall Assessment

⭐ Excellent work! This PR addresses a critical bug with a clean, backward-compatible solution and comprehensive test coverage.

Summary:

• ✅ Correctly identifies and fixes the root cause
• ✅ Maintains full backward compatibility
• ✅ Comprehensive test coverage (9 new tests)
• ✅ Clear documentation in PR description
• ✅ No security concerns
• ⚠️ Minor suggestions for code quality improvements

---

Code Quality & Best Practices

✅ Strengths

1. Clean abstraction: The charge_object_data helper method is well-designed and follows the Single Responsibility Principle.

2. Defensive coding: Proper handling of edge cases (nil, empty hash, non-hash values).

3. Clear comments: The code includes helpful inline comments explaining the Pay version differences.

4. Idempotent migration: The migration correctly checks for existing columns before adding them.

🔍 Suggestions for Improvement

1. Minor: Optimize conditional logic in charge_object_data

Current code:

def charge_object_data
  if respond_to?(:object) && object.is_a?(Hash) && object.any?
    object.with_indifferent_access
  elsif data.is_a?(Hash) && data.any?
    data.with_indifferent_access
  else
    {}.with_indifferent_access
  end
end

Suggestion: Consider simplifying by extracting the source selection:

def charge_object_data
  source = if respond_to?(:object) && object.is_a?(Hash) && object.present?
    object
  elsif data.is_a?(Hash) && data.present?
    data
  end
  
  (source || {}).with_indifferent_access
end

This is a minor improvement for readability, but your current implementation is perfectly fine.

2. Consider memoization for performance

If charge_object_data is called multiple times during a request, consider memoizing it:

def charge_object_data
  @charge_object_data ||= begin
    if respond_to?(:object) && object.is_a?(Hash) && object.any?
      object.with_indifferent_access
    elsif data.is_a?(Hash) && data.any?
      data.with_indifferent_access
    else
      {}.with_indifferent_access
    end
  end
end

However, this is likely premature optimization since the method is typically called once per charge validation.

---

Test Coverage

✅ Excellent Coverage

Your test suite is comprehensive and well-organized:

1. Pay 10+ scenarios (5 tests):

   • ✅ Status in object column
   • ✅ Failed/pending/canceled statuses
   • ✅ Object precedence over data

2. Legacy Pay scenarios (2 tests):

   • ✅ Fallback to data column
   • ✅ Legacy succeeded logic

3. Edge cases (2 tests):

   • ✅ Nil status with amount_captured fallback
   • ✅ Partial capture scenarios

4. Integration test:

   • ✅ Exact Issue Credit balance not fulfilled after non-recurring credit pack purchase #1 scenario verification

---

Potential Bugs & Issues

✅ No Critical Issues Found

The code correctly handles:

• ✅ Nil vs empty hash
• ✅ Symbol vs string keys (via with_indifferent_access)
• ✅ Missing columns in older Pay versions
• ✅ All Stripe charge statuses (succeeded, failed, pending, canceled)

⚠️ Minor: Consider logging version detection

For debugging purposes in production, consider adding a one-time log during initialization to help users understand which Pay version behavior is being used:

# In an initializer or during app boot
if Pay::Charge.column_names.include?('object')
  Rails.logger.info "[UsageCredits] Detected Pay 10+ (using object column for charge data)"
else
  Rails.logger.info "[UsageCredits] Detected Pay <10 (using data column for charge data)"
end

This would help with support requests, but it's optional.

---

Performance Considerations

✅ No Performance Concerns

1. Database changes: Adding JSON columns to Pay models is appropriate and won't impact performance significantly.

2. Query performance: No new queries introduced; the fix operates on already-loaded objects.

3. Memory usage: Minimal overhead from with_indifferent_access.

---

Security Concerns

✅ No Security Issues

1. No SQL injection risks: All data access is through ActiveRecord methods.

2. No XSS risks: No user input is being rendered.

3. No authorization issues: The code operates on already-authorized charge objects.

4. Data integrity: The charge_object_data method defensively handles all edge cases.

---

Backward Compatibility

✅ Excellent Backward Compatibility

Your compatibility matrix is accurate:

Pay Version	Behavior	Status
Pay 8.3.x	Uses data column	✅
Pay 9.0.x	Uses data column	✅
Pay 10.x	Uses object column	✅
Pay 11.x	Uses object column	✅

The migration is idempotent and safe to run multiple times.

---

Documentation

✅ Outstanding PR Description

Your PR description is exemplary:

• Clear root cause analysis
• Links to upstream Pay gem changes
• Visual tables for version comparison
• Integration test description
• Backward compatibility matrix

📝 Suggestion: Update README

Consider adding a note in the README about Pay version compatibility. For example, in the "Requirements" section:

**Requirements**

- An ActiveJob backend (Sidekiq, `solid_queue`, etc.) for subscription credit fulfillment
- [`pay`](https://github.com/pay-rails/pay) gem for Stripe/PayPal/Lemon Squeezy integration
  - **Note:** `usage_credits` is compatible with Pay 8.3+, including Pay 10+ which introduced the `object` column for storing full Stripe response objects.

---

Additional Suggestions

1. Consider adding a changelog entry

If you maintain a CHANGELOG.md, this fix deserves an entry:

## [Unreleased]
### Fixed
- Credit pack fulfillment not working with Pay 10+ due to data structure changes (Issue #1)
  - Pay 10+ stores charge status in `object` column instead of `data` column
  - Maintains full backward compatibility with Pay 8.3+

2. Follow-up: Document Pay.sync for immediate fulfillment

Based on the issue comments, consider documenting the Pay.sync pattern for immediate fulfillment after checkout redirect:

# In your checkout success controller
def success
  session = Stripe::Checkout::Session.retrieve(params[:session_id])
  Pay.sync(session.charge) # Immediately sync charge and trigger fulfillment
  redirect_to credits_path, notice: "Credits added!"
end

This could prevent future issues like the one reported by @crobbo.

---

Final Verdict

✅ APPROVE - Ready to Merge

This PR demonstrates excellent engineering practices:

• Thorough root cause analysis
• Clean, maintainable solution
• Comprehensive test coverage
• Full backward compatibility
• Outstanding documentation

The suggestions above are all optional improvements. The code is production-ready as-is.

Test Results Confirm Quality:

673 runs, 1516 assertions, 0 failures, 0 errors, 0 skips

Great work on identifying and fixing this critical issue! 🎉

---

Review conducted by: Claude Code (Sonnet 4.5)
Repository guidelines: Reviewed against project standards