Always return the result of APIResource#refresh in APIResource.retrieve

[AnotherJoSmith]

This is somewhat of a bug report masked as a PR, since I'm not sure what I suggested is the desired solution based on the API design of ApiResource.refresh. If we think this is the best solution for now, I'm happy to add tests as well.

With the refactor of v13, there are now cases where self is not mutated in the call to refresh and instead a new object is returned. This change ensures that the new object is always returned by returning the result of refresh instead.

This case happens when the else branch of this method is triggered.

https://github.com/stripe/stripe-ruby/blob/a2e2881c7cad5ebe5ad03c013521d01384ab77d9/lib/stripe/api_requestor.rb#L236-L244

This happens if you defined a custom object that extends APIResource. For example, at Shopify we have an object like so:

module Stripe
  class ReservePlan < APIResource

  class << self
    def retrieve(id, opts = {})
      super
    end
  end
end

The retrieve method no longer works with the method for v13, since it doesn't return the result of the API call, since instance is not mutated in this case.

However even after the change in this PR, there is still a regression since calling super now returns a generic StripeObject. To be able to return an instance of our custom class, I had to change the code of retrieve to something like this:

def retrieve(id, opts = {})
  result = super
  construct_from(result.to_h, opts, result.last_response)
end

This is obviously not as clean as it was before. It's also a bit unintuitive to have a method called #refresh which returns a new StripeObject instance instead of mutating the existing instance, which seems like the intent of that method.

[cla-assistant[bot]]

All committers have signed the CLA.

[cla-assistant[bot]]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[AnotherJoSmith]

CC @helenweng-stripe @xavdid-stripe

[helenye-stripe]

Hi @AnotherJoSmith, sorry for the friction here! We changed this due to the change in behavior for StripeClient, but we did not realize this regression would occur. Do you mind adding tests and we will get this merged. I've also made a ticket to track this internally, and will track details for

there is still a regression since calling super now returns a generic StripeObject

this regression as well.

In the future I would recommend opening an issue and linking the PR to the issue -- we track user GH issues much more closely than user PRs!

[AnotherJoSmith]

@helenye-stripe Added a test (which I verified fails without my change).

I also updated the README which had outdated instructions on installing stripe-mock.

[AnotherJoSmith]

@helenye-stripe Any chance you can release a new patch version? That will unblock our ability to upgrade this gem at Shopify.

[helenye-stripe]

We're releasing soon - @jar-stripe will be taking care of this.

[helenye-stripe]

@AnotherJoSmith This should be out in v13.0.2!