Skip to content
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

Fix return value of Customer#delete_discount #964

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Feb 8, 2021

Customer#delete_discount has been broken for some time in that it
tries to re-initialize self (which is a customer) with a received
discount response. This is incorrect and leads to various problems.

Here, we redefine the return value of delete_discount as a discount,
and have it no longer mutate the object on which is was called. We add a
comment as well just to help flag some of the behavior which could
potentially be confusing.

Fixes #963.

r? @richardm-stripe

@richardm-stripe @remi-stripe I know there are some concerns around
backwards compatibility here, but given that anyone current trying to
use the return value of this method is pretty broken, and has been for a
while, I was thinking about just releasing it as a patch version. Any
objections?

@richardm-stripe
Copy link
Contributor

No objection here.

`Customer#delete_discount` has been broken for some time in that it
tries to re-initialize `self` (which is a customer) with a received
discount response. This is incorrect and leads to various problems.

Here, we redefine the return value of `delete_discount` as a discount,
and have it no longer mutate the object on which is was called. We add a
comment as well just to help flag some of the behavior which could
potentially be confusing.

Fixes #963.
@stripe-ci stripe-ci assigned brandur and unassigned richardm-stripe Feb 8, 2021
@richardm-stripe
Copy link
Contributor

This looks good. Since this is special/non-standard I wouldn't say no to adding a test

@remi-stripe
Copy link
Contributor

Flagging my concern publicly for traceability: Before 5.X calling delete_discount returned a mutated Customer with discount nulled out. With this change in 5.X the same method will "suddenly" return a Discount instead of a Customer. This is a subtle behaviour change that we need to clearly call out in the v5 migration guide if we move forward with this change.

@brandur-stripe
Copy link
Contributor

This looks good. Since this is special/non-standard I wouldn't say no to adding a test

@richardm-stripe I looked into this, and I don't think we can do too much better than the existing test (now change for discount) unfortunately. stripe-mock just doesn't return that much useful data to assert on other than the object type:

From: /Users/brandur/stripe/stripe-ruby/test/stripe/customer_test.rb:62 :

    57:       should "delete a discount" do
    58:         customer = Stripe::Customer.retrieve("cus_123")
    59:         discount = customer.delete_discount
    60:         assert_requested :delete, "#{Stripe.api_base}/v1/customers/cus_123/discount"
    61:         require "pry" ; binding.pry
 => 62:         assert discount.is_a?(Stripe::Discount)
    63:       end
    64:     end
    65:
    66:     context ".delete_discount" do
    67:       should "delete a discount" do

[1] pry(#<Stripe::CustomerTest>)> discount
=> #<Stripe::Discount:0x8e8 id=_G0mwyqMLBk3vJTA> JSON: {
  "id": "_G0mwyqMLBk3vJTA",
  "coupon": {"id":"_G0mwyqMLBk3vJTA","amount_off":null,"applies_to":{"products":[]},"created":0,"currency":null,"duration":"forever","duration_in_months":null,"livemode":false,"max_redemptions":null,"metadata":null,"name":null,"object":"coupon","percent_off":null,"redeem_by":null,"times_redeemed":0,"valid":false},
  "deleted": true,
  "object": "discount",
  "start": 1234567890
}

@brandur-stripe
Copy link
Contributor

Flagging my concern publicly for traceability: Before 5.X calling delete_discount returned a mutated Customer with discount nulled out. With this change in 5.X the same method will "suddenly" return a Discount instead of a Customer. This is a subtle behaviour change that we need to clearly call out in the v5 migration guide if we move forward with this change.

@remi-stripe Fair enough. Just to confirm: are you okay with the change as long as I update the V5 migration guide as I bring it in?

@remi-stripe
Copy link
Contributor

Yes I am okay with the change and it will mirror what we do in subscription.rb too. I just wanted this discussion to be summarized publicly for any developers who ends up on that PR in the future!

@brandur-stripe
Copy link
Contributor

Ah, great! Okay, going to pull this in then. I've updated the V5 migration guide.

@brandur-stripe brandur-stripe merged commit c28ee66 into master Feb 9, 2021
@brandur-stripe brandur-stripe deleted the brandur-fix-delete-discount branch February 9, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stripe::Customer.delete_discount corrupts Stripe::Customer object
4 participants