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

Add support for billing thresholds #655

Merged
merged 1 commit into from
Jan 17, 2019
Merged

Conversation

remi-stripe
Copy link
Contributor

@remi-stripe remi-stripe commented Jan 17, 2019

r? @mickjermsurawong-stripe @ob-stripe
cc @stripe/api-libraries

cc @clintonb-stripe Could you double check that I added the right set of properties too and I'm not missing any? (Java only lists properties, not parameters).

Docs: https://stripe.com/docs/billing/subscriptions/metered-billing/thresholds
Internal PR with gate removal: #126799

Copy link

@clintonb-stripe clintonb-stripe left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! 🎉

@remi-stripe
Copy link
Contributor Author

leaving approval to mick/ob since it's our process. Thanks a lot for the quick review @clintonb-stripe! Other two PRs incoming in a bit

Copy link
Contributor

@mickjermsurawong-stripe mickjermsurawong-stripe left a comment

Choose a reason for hiding this comment

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

Thank you @remi-stripe!
There's one incorrect typing, and one is my proposal on shortening class nesting
ptal @remi-stripe

@@ -59,6 +60,7 @@
Long subtotal;
Long tax;
BigDecimal taxPercent;
String thresholdReason;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be typed as ThresholdReason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickjermsurawong-stripe I don't think so no. We don't do "list of constants" in java AFAIK. @ob-stripe do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what Mick meant. The threshold_reason attribute is an object, not a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh I misunderstood. That reason naming is throwing me off. Nice catch mick, fixing!

@EqualsAndHashCode(callSuper = false)
public static class ItemReason extends StripeObject {
List<String> lineItemIds;
Long usageGte;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the innermost class, when referred outside this class (in normal usage) will be
Invoice.ThresholdItemReason instead of the 3-level nesting as Invoice.ThresholdReason.ItemReason here? I think the former reads better.
So we can implement the following:

public static class ThresholdReason extends StripeObject {
  List<ThresholdItemReason> itemReasons
}
// the inner-most class is now at nested in Invoice instead
public static class ThresholdItemReason extends StripeObject {
}

Actually, in the autogen SDK, we currently enforce nesting at most two levels to flatten classes. It help to flatten long resource dependency we have in pay-server annotation too.
The Charge.Level3.LineItem will be in autogen (hypothetically) as Charge.Level3LineItem (although we decided on unwinding it to top level resource)

public class Charge {
  public static class Level3 extends StripeObject {
    List<Level3LineItem> lineItems;
    ....
  }
  public static class Level3LineItem extends StripeObject {
     ....
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@remi-stripe
Copy link
Contributor Author

@mickjermsurawong-stripe Fixed both! Can you take another look?

@mickjermsurawong-stripe
Copy link
Contributor

yay! thank you @remi-stripe and @ob-stripe!
LGTM

@remi-stripe
Copy link
Contributor Author

Had to rebase as it conflicted with #653

@remi-stripe
Copy link
Contributor Author

tests passed, re-approving

@remi-stripe remi-stripe merged commit 0905d4d into master Jan 17, 2019
@remi-stripe remi-stripe deleted the remi-add-billing-thresholds branch January 17, 2019 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants