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.

Sign up for GitHub

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

Jump to bottom

Flexible/Metered Billing API support #634

Merged
merged 1 commit into from
Apr 11, 2018
Merged

Flexible/Metered Billing API support #634

merged 1 commit into from
Apr 11, 2018

Conversation

alexander-stripe
Copy link
Contributor

Implement support for flexible billing options on plans and creating usage records.

@alexander-stripe
Copy link
Contributor Author

Regarding the failing tests: I think this might be a bug in the newest stripe-mock? Looks like the server is not returning IDs for deleted things.

@brandur-stripe
Copy link
Contributor

@alexander-stripe Thanks! LGTM.

Unfortunately, OB noticed this morning that there seems to be a slight problem with the stripe-mock Ruby upgrade involving deleted resources no longer returning a valid id, and this is breaking the test suite. stripe/stripe-mock#56 is filed and I'll take a look in a little while.

@alexander-stripe
Copy link
Contributor Author

cc @brandur-stripe

@brandur-stripe
Copy link
Contributor

Regarding the failing tests: I think this might be a bug in the newest stripe-mock? Looks like the server is not returning IDs for deleted things.

Oops. Yep, that's it.

@alexander-stripe alexander-stripe mentioned this pull request Apr 4, 2018
Merged
@alexander-stripe
Copy link
Contributor Author

After updating to the newest stripe mock there still seems to be a problem with the IDs - all deleted IDs are ch_1CDJmuCLWM6HLS3KC956JkAm?

@ob-stripe
Copy link
Contributor

After updating to the newest stripe mock there still seems to be a problem with the IDs - all deleted IDs are ch_1CDJmuCLWM6HLS3KC956JkAm?

@alexander-stripe Yes, @brandur ran into that issue in #635. Looks like another stripe-mock update will be needed.

@brandur-stripe
Copy link
Contributor

Yeah, sorry about the trouble, but you'll have to hold out on this one for a bit longer. I think I know what I can do to fix this, but it requires a somewhat non-trivial feature addition to stripe-mock which I'll tackle today.

@brandur-stripe
Copy link
Contributor

Alright, this ended up taking way too long, but I finally got the test suite running again with this change to stripe-mock: stripe/stripe-mock#59

I'm waiting for review, but we should have everything going again shortly.

@brandur-stripe
Copy link
Contributor

@alexander-stripe Good news! We finally got stripe-mock upgraded in master. Would you mind rebasing this branch? After that's done, I think we're good to bring it in. Thanks!

@brandur-stripe
Copy link
Contributor

(Oh, and to avoid conflict problems, it might be best just to remove anything files that you touched here that are unrelated to metered billing.)

@alexander-stripe alexander-stripe force-pushed the alexander/flexible-billing branch from f782a2a to bb9dfc8 Compare April 10, 2018 21:26
@alexander-stripe
Copy link
Contributor Author

alexander-stripe commented Apr 10, 2018 •
edited
Loading

Nice! Thanks you :-)

lib/stripe/util.rb:4:3: C: Metrics/ModuleLength: Module has too many lines. [306/305]
  module Util ...
  ^^^^^^^^^^^
lib/stripe/util.rb:40:5: C: Metrics/MethodLength: Method has too many lines. [46/45]
    def self.object_classes ...
    ^^^^^^^^^^^^^^^^^^^^^^^

🙈

Will fix...

@brandur-stripe
Copy link
Contributor

@alexander-stripe Ugh. The right thing to do for now is just to pop open .rubocop_todo.yml and increase the relevant numbers there by one.

@stripe-ci
Copy link

Gerald Rule: Rubocop Core File Change

cc @pt-stripe

@alexander-stripe
Copy link
Contributor Author

Ah, I would have disabled then locally, but this works too :)

@alexander-stripe
Copy link
Contributor Author

Thanks!

@alexander-stripe
Copy link
Contributor Author

Alright, now ready for final review + merging :)

brandur-stripe
brandur-stripe reviewed Apr 10, 2018
View reviewed changes
lib/stripe/usage_record.rb Outdated
module Stripe
class UsageRecord < APIResource
def self.create(params = {}, opts = {})
req_params = params.clone.delete_if { |key, _value| key == :subscription_item }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a brand new function, could we throw an ArgumentError if subscription_item is missing when we enter this method?

The trouble is that currently if this parameter is forgotten, it'll fall through and produce a confusing 404 as it interpolations a nil into the URL below.

Copy link
Contributor

@ob-stripe ob-stripe Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 If we want to be consistent with other libraries, we should throw an InvalidRequestError here. That said, ArgumentError sounds reasonable too and is probably more idiomatic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, ArgumentError probably is more idiomatic. That said though, I can see some value in throwing exceptions that descending from a common Stripe tree, so I could sway either way.

Thanks for weighing in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to throw an ArgumentError 👍

@brandur-stripe
Copy link
Contributor

Thanks Alexander!

@alexander-stripe alexander-stripe force-pushed the alexander/flexible-billing branch from 8eee2e5 to c066c9c Compare April 11, 2018 20:29
@brandur-stripe brandur-stripe merged commit 9594875 into master Apr 11, 2018
@brandur-stripe brandur-stripe deleted the alexander/flexible-billing branch April 11, 2018 20:50
@brandur-stripe
Copy link
Contributor

Released as 3.13.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ob-stripe ob-stripe ob-stripe left review comments

@brandur-stripe brandur-stripe brandur-stripe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexander-stripe @brandur-stripe @ob-stripe @stripe-ci