-
Notifications
You must be signed in to change notification settings - Fork 721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create price #758
Create price #758
Conversation
I'd love some feedback on testing - I was not able to get clean tests running locally with some exceptions that appear unrelated and a few attempts to run some focused tests failed. Which means that currently this PR has tests but do they work? 🤷 |
@@ -110,9 +110,41 @@ def validate_create_plan_params(params) | |||
|
|||
end | |||
|
|||
def validate_create_price_params(params) | |||
price_id = params[:id].to_s | |||
product_id = params[:product] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something worth noting, stripe-rails gem asks for the product_id
instead of product
when setting up the initializer.
references;
https://github.com/tansengming/stripe-rails#configuring-your-plans-and-coupons (see prices.rb readme)
https://stripe.com/docs/api/prices/create#create_price-product
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, stripe-rails requires product_id
and the Stripe API uses product
. I think that difference is because stripe-rails will optionally create a product and it has abstracted some of the Stripe API so that it makes the right request without the user having to set product_data
in stripe-rails.
For this gem where it mocks the Stripe API I think product
is the correct param to use.
@gilbert What will it take to get this merged in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. Regarding tests, it looks like the suite needs some attention in general. It currently fails locally.
We will need to dedicate some attention to getting tests to a good spot before this can be an official release, I think.
But adding this makes the library relevant again, even if test coverage/bug coverage isn't perfect, so I'm going to merge it in for a pre-release.
@csalvato can you check this, please 🙏? |
@asdolo To what are you trying to draw my attention? What problem does this solve within the gem? |
This comes out of #730 - it doesn't look like that PR is moving but I would like to see the functionality in stripe-ruby-mock
I've also added support for listing prices by lookup key and made an attempt at adding tests