Closed matthewbjones closed 3 years ago
I believe the issue stems from: https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/resources/customer.rb#L33
Here's a monkey patch that fixes the broken code above:
module Stripe
class Customer < APIResource
def delete_discount
resp, opts = execute_resource_request(:delete, resource_url + "/discount")
Stripe::Discount.construct_from(resp.data, opts)
end
end
end
Since resp.data
is a JSON payload about a Discount
, we do not want to call initialize_from
within Stripe::Customer
, but rather return a Discount
from the delete_discount
method.
I don't know if the Stripe::Customer
is stale and needs to refresh or not.
@matthewbjones Thank you so much for the detailed reproduction steps and finding the line of code that is causing issues. I think the bug was introduced by this change here. We used to remove the discount on the Customer
explicitly and then would return the Customer
but incorrectly switched to overriding the Customer with a Discount instead.
I could confirm it quickly by modifying the test for it to this
context "#delete_discount" do
should "delete a discount" do
customer = Stripe::Customer.retrieve("cus_123")
customer = customer.delete_discount
assert_requested :delete, "#{Stripe.api_base}/v1/customers/cus_123/discount"
assert customer.is_a?(Stripe::Customer)
assert customer.object == "customer"
end
end
it fails because object
is now discount
event though the class' instance is still a Customer.
In a way it's a simple fix, but the bug has been here for long enough that it's always problematic to make subtle changes like this in a patch release and it might need a major release for it.
I'll discuss this with the other maintainers next week to decide what is the best plan of action. We might also need to add that test logic to our test suite to ensure that you can't get a an instance of class A but object
matching class B.
In the meantime, I would recommend moving away from .save
entirely. While the library supports this (and we definitely shouldn't have broken this!) we consider it mostly deprecated and all our docs have been updated to move to the update
method approach instead a few years ago. So instead of saving the customer that way, we recommend always being explicit with what you want to change or mutate and use Stripe::Customer.update('cus_123', options)
approach instead.
@remi-stripe Thanks for the speedy and detailed reply. For context, we were previously using version 2.12.0
prior to this upgrade with an API version of 2013-02-13
. We've been on this stack for years and years throughout all of Stripe's updates and improvements and the Ruby gem + API held strong without issue. We only just got around to updating the gem to the latest version because we're finally looking to implement some of the "newer" Stripe features (Apple Pay, etc.) and we had to finally update the SDKs.
We wanted to make as little changes to our codebase as possible, given the 7+ years of stability and were a bit fearful of introducing a new bug considering just how long it's been and how much of the API and Ruby code changed over the years. The migration was more straightforward than feared so that's good, mostly replacing .all
with .list
on the various objects and then dealing with delete_subscription
no longer being a method on Stripe::Customer
, which was a rather easy fix given that our business only allows one subscription at a time (which I believe was the functionality way back in 2013). The only break in functionality we've seen in production so far is this delete_discount
glitch.
Thanks for the information on what the Stripe team recommends in terms of how to best use the Ruby gem these days. We have a whole test suite around billing + Stripe with VCR cassettes, etc. that instills a great level of confidence that our Stripe integration works as intended the way it was written years ago. This is something we will slowly migrate to over time to make sure we don't have any breaks in functionality.
Kudos to the Stripe team for allowing us to use 2013 API versions with a 2017 gem for all this time!
Thanks for reporting and bearing with us! We've released a fix in 5.29.1.
Introduced in version
5.0.0
, worked in prior releasesBug conditions
delete_discount
methodsave
, aStripe::InvalidRequestError ((Status 404) (Request req_0Hk4gAhb1KNnl3) No such customer: 'di_xxx')
will occurExpected To succeed, because deleting of a discount shouldn't prevent future operations on the Stripe::Customer object. The discount does actually get deleted, it's just future calls on the
Stripe::Customer
object, likesave
, will error with an incorrect customer ID.Gem version Broke in
5.0.0
(August 20, 2019), previous release4.24.0
(August 13, 2019) works and earlier versions