Fix UsageRecords so it passes through the rest of the arguments

[jlomas-stripe]

Makes sure that all of the elements of the original arguments array - with the first one being modified to extract and remove the SubscriptionItem ID - are passed through to the stripeMethod() call.

Fixes #451.

r? @brandur-stripe

[brandur-stripe]

@jlomas-stripe Thanks for taking a crack at this!

Just to make sure that I understand this correctly: what exactly is unusual about this function that means we need this custom code here, but not most other places?

[fred-stripe]

cc @alexander-stripe

[alexander-stripe]

Ah, this is needed to make sure the callback is passed through! Thanks for taking this.

I'm also a bit worried that the test-suite didn't catch this - i.E. the timestamp is marked as required in our API docs ( https://stripe.com/docs/api#usage_record_create ) but I had a typo in timestamp and the tests still passed :-(

[jlomas-stripe]

@brandur-stripe I think that because the subscription_item ID is inside the object there, rather than its own (first) argument, it needs to be extracted in a non-standard way as there's currently no way to extract anything from the parameters object for use in the path.

If we were to switch it to:

stripe.usageRecords.create('si_123', {
  quantity: 123,
  timestamp: 123321,
  action: 'increment'
});

Then it could use the regular stripeMethod approach.

I have those changes sitting in my local copy; happy to add them to this PR if that makes more sense, though that would require docs changes, and might mean a breaking change / major version bump at this point...?

[brandur-stripe]

@jlomas-stripe Sorry, I totally lost track of this one!

Looking over this again ... I'm thinking that I might have been a little hasty merging the original PR. This should be modeled closely after application fee refunds, which have a similar setup in that their a subresource of another resource. Its implementation is quite simple:

module.exports = StripeResource.extend({
  path: 'application_fees/{feeId}/refunds',
  includeBasic: ['create', 'list', 'retrieve', 'update'],
});

I'm actually not 100% sure that the create there works as expected because there's no test for it, but the method for creating a new refund that's recommended on our docs is this:

stripe.applicationFees.createRefund(
  "fee_1B73DOKbnvuxQXGuhY8Aw0TN",
  function(err, applicationFee) {
    // asynchronously called
  }
);

Which also has a very simple implementation:

  createRefund: stripeMethod({
    method: 'POST',
    path: '/{feeId}/refunds',
    urlParams: ['feeId'],
  }),

I'm sort of thinking that we should change usage records over to use that system instead of what we have now ... what do you think?

[jlomas-stripe]

@brandur-stripe Agreed. I actually had those changes queued up in my local repo already.

This just adds the create (similar to createRefund), since that's all we can currently do anyways.

[brandur-stripe]

Ah, nice. Thanks @jlomas-stripe! (And my mistake — you said this before, but I didn't quite get it.)

Alright, I just looked at stripe-node usage on this endpoint, and unfortunately there is some already, so I think we're going to have to roll this as a major version bump given the breaking API change. Still, it just be a very easy migration.

[remi-stripe]

could we support both and deprecate the old one?

[brandur-stripe]

could we support both and deprecate the old one?

Yeah probably, but because both methods would have the same name (create), it might require some more involved code that'll be harder to maintain.

IMO, given that this is a relatively new product and there can't be that many users on it yet, we should try to just roll forward on this one. Thoughts?

[remi-stripe]

I'm assuming you meant keeping the move forward as a major version? If so I agree, it's just that major versions should also be used to remove/deprecate things we've wanted remove for a while.

[brandur-stripe]

I'm assuming you meant keeping the move forward as a major version? If so I agree, it's just that major versions should also be used to remove/deprecate things we've wanted remove for a while.

Yeah, agreed in principle — did you have anything in mind?

If there's nothing that we want to do opportunistically though, I'd rather not block too long on this (the longer we wait, the more likely people are to start using the older/current API). If we want to change something again later, we can always bump the major version again — users upgrading to compensate for 5 breaking changes across two major version numbers is the same amount of work as compensating for 5 breaking changes across one major version number.

[remi-stripe]

Okay, I'm fine to leave as is then and we can always prioritize a v6 soon! Sorry for the chunder

[brandur-stripe]

Okay, I'm fine to leave as is then and we can always prioritize a v6 soon! Sorry for the chunder

Just to be clear, I'm suggesting that we release V6 now (and if there are any immediate backwards-incompatible changes I can bundle them in), then release V7 later if we want to bundle up any larger backwards-incompatible changes. Are you still okay with that?

[remi-stripe]

yes, sorry, I got my major versions number mixed up. Please go ahead and ignore most of what I said earlier.

[brandur-stripe]

Okay, great. Thanks @jlomas-stripe @remi-stripe! Pulling this in.

[brandur-stripe]

(By the way, I'll take updating the docs.)

[brandur-stripe]

Released as 6.0.0.