Closed jamesprior closed 3 years ago
Thanks for all the work you've put into this @jamesprior ! Give me a few more days to get through it, reviewing this is going take more time than usual =)
Absolutely! I'm never completely confident and just happy to have some extra double checks on things.
It looks like you've extracted the options for the recurring
hash into attr_accessor
so that instead of
Stripe.price :lite do |price|
price.name = 'Acme as a service LITE'
price.unit_amount = 699
price.currency = 'usd'
price.recurring = {
interval = 'month',
interval_count = 3
}
end
folks will do this instead:
Stripe.price :lite do |price|
price.name = 'Acme as a service LITE'
price.unit_amount = 699
price.currency = 'usd'
price.interval = 'month'
price.interval_count = 3
end
I think it's more obvious to pass a hash to recurring (like the first example) to match the docs at https://stripe.com/docs/api/prices/create
Is there a reason why you chose to flatten it out?
Sidenote: it also looks like attr_accessor :recurring
does nothing the way it is written right now since the recurring
that is passed to stripe is really recurring_options
I think the thought there was to make it function more like the product_data
than the transform_quantity
, and it kept the price interface pretty similar to the older plan interface. There's a lot of validations around those options too.
The latest version moves those to a hash, it's a bit hamfisted in how it keeps the validations going so all ears on feedback - good direction to continue in? What are the expected validations?
The latest version moves those to a hash, it's a bit hamfisted in how it keeps the validations going so all ears on feedback - good direction to continue in?
I think it's great! Thanks for the work so far. I'm happy for you to finish it up.
What are the expected validations?
I think it's fair to just inherit the ones from products, like what you're doing here. Other folks can keep adding more validations later if they'd like.
Thanks for the review! I added in support for interval_count
and tests for it. I think that's everything missing but I'm happy for any review and feedback you've got.
It looked to me like Stripe recommended using Prices instead of Plans (although they are backwards compatible). To help me follow along with the stripe documentation and match things up easier I added a Price configuration object to stripe-rails.