Add support for Subscription Schedule #1491

remi-stripe · 2019-01-28T21:43:05Z

Add support for Subscription Schedule APIs

Add Subscription Schedule
Add Subscription Schedule Revisions
Add invoice_settings on the Customer

cc @stripe/api-libraries

remi-stripe · 2019-02-07T05:43:47Z

@ob-stripe Okay I think this is ready to review entirely. There is a lot of content here as this is a complex resource with multiple layers of nested hashes and there are "shared" properties where naming (in this PR) is not great.

Can I ask you to review thoroughly to ensure everything is fully correct? Please pay attention to all class names and feel free to ask me to rename everything.

Relatedly, there's a deserialization error because I try to expand phases.coupon but I don't get a coupon backand only getnull` which fails the test and shows either a failure in the fixture/openapi/something else.

remi-stripe · 2019-02-07T05:46:35Z

src/Stripe.net/Entities/SubscriptionsSchedules/SubscriptionSchedulePhasePlan.cs

+        /// new billing period.
+        /// </summary>
+        [JsonProperty("billing_thresholds")]
+        public SubscriptionSchedulePhasePlanBillingThresholds BillingThresholds { get; set; }


Flagging that this is the same hash as the one on Subscription Item documented here. I did not want to re-use that class name as it's SubscriptionItemBillingThresholds and it's confusing.

@alexander-stripe suggested we unify though and rename both to UsageBasedBillingThreshold for example. Renaming would be a breaking change but since have a major revision shipping tomorrow or Friday maybe it's worth it? This also impacts the Options class(es).

Note that there is also billing_thresholds at the top level of this object and this one matches the one on Subscription and so I shared SubscriptionBillingThresholds because it felt like it made sense.

My default position on this is that we should try to match the OpenAPI spec as much as possible. In this case, the spec uses subscription_item_billing_thresholds here so I think we should do the same. That way, if the structure changes in the future, we'll only have to update one class instead of two.

I have clear fud here because openapi is generated based on client-metadata in pay-server. This is a separate discussion though.
Also whatever is in openapi can change without being considered a breaking change by anyone but a few of us.

I am fine using the subscription_item_billing_thresholds if you want, but I think it's for the wrong reasons. We should rename openapi in that case really.

My point was that both resources use the same schema in OpenAPI, which means that in our backend code subscription_item_billing_thresholds is defined once and referenced in both resources. So if it ever changes, it would change for both resources at once and we'd need to reflect that in the bindings.

I agree the name is bad because it specifically mentions subscription_item, and should be changed.

remi-stripe · 2019-02-07T05:47:07Z

src/Stripe.net/Entities/SubscriptionsSchedules/SubscriptionSchedule.cs

+        [JsonProperty("current_phase")]
+        public SubscriptionScheduleCurrentPhase CurrentPhase { get; set; }
+
+        // TODO: figure out how we document those


Should I bother documenting since this is going away in your next major version?

The properties are not going away, so I'd say yes, we should document them.

@ob-stripe Sure but the entire "region" is going away though. So my todo/question holds: How should I document this? Can you show me an example with this region on where to put the comments so that your rebase is not too painful?

The region would stay there too, what would change is:

public string CustomerId { get; set; } --> public string CustomerId => InternalCustomer.Id;

public Customer Customer { get; set; } --> public Customer Customer => InternalCustomer.ExpandedObject;

internal object InternalCustomer { ... } --> internal ExpandableField<Customer> InternalCustomer;

Here's an example for documenting the public accessors for the ID and object: https://github.com/stripe/stripe-dotnet/blob/master/src/Stripe.net/Entities/Disputes/Evidence.cs#L16-L28. Not sure it's the best format, but it's something.

remi-stripe · 2019-02-07T05:53:33Z

src/Stripe.net/Services/SubscriptionSchedules/SubscriptionScheduleInvoiceSettingsOptions.cs

+    using System.Collections.Generic;
+    using Newtonsoft.Json;
+
+    public class SubscriptionScheduleInvoiceSettingsOptions : StripeEntity


Talking with @alexander-stripe There is a world in which all invoice_settings are merged (instead of being 2 separate hashes). This is not yet scheduled/planned though, just some thoughts. This means that when it happens it'd be a major/breaking change anyway.

Still flagging that I am not happy with all those classes names that are attached to the first object ever seeing them...

Yeah, I feel the same, but I don't have a great solution to offer. The OpenAPI spec isn't even helpful here.

Maybe the right way to handle this is via inner classes, but we don't have a precedent for this in Stripe.net and I'm really not sure this is how we want to handle these nested parameters.

I think last time we tried there was a stylecop rule against nested class.

Ah right, thanks. I'm not against changing that rule, but we'd have to be sure it's the right move. stripe-java does use inner classes, so it's worth experimenting with them at least.

remi-stripe · 2019-02-07T05:54:05Z

src/Stripe.net/Services/SubscriptionSchedules/SubscriptionScheduleListOptions.cs

+
+    public class SubscriptionScheduleListOptions : ListOptionsWithCreated
+    {
+        // TODO: Figure out how to document those


How do I document those? Do I ignore since you're fixing in the next major revision?

Yeah, probably fine to leave them undocumented for now. I'll add docstrings once #1495 lands.

remi-stripe · 2019-02-07T05:54:45Z

src/Stripe.net/Services/SubscriptionSchedules/SubscriptionScheduleRenewalIntervalOptions.cs

+        /// Interval at which to renew the subscription schedule for when it ends. Possible values
+        /// are <code>day</code>, <code>week</code>, <code>month</code>, or <code>year</code>.
+        /// </summary>
+        // TODO: Decide if using PlanInterval


Should I be using PlanInterval here? It's the same list, but also feels like a different meaning (and maybe we would like this to be renamed to BillingInterval?)

Yeah, it's annoying, and the OpenAPI spec is no help here either as the enums are not defined as a single schema that is referenced from different parameters, but rather duplicated for each parameter.

I'd leave as is for now, but it's something we should fix upstream.

ob-stripe

Some minor comments. LGTM in general, but I haven't reviewed every single resource attribute and request parameter. Someone from Billing should probably take a look too.

src/Stripe.net/Entities/SubscriptionsScheduleRevisions/SubscriptionScheduleRevision.cs

ob-stripe · 2019-02-07T09:47:34Z

src/Stripe.net/Entities/SubscriptionsSchedules/SubscriptionSchedule.cs

+        [JsonProperty("current_phase")]
+        public SubscriptionScheduleCurrentPhase CurrentPhase { get; set; }
+
+        // TODO: figure out how we document those


The properties are not going away, so I'd say yes, we should document them.

ob-stripe · 2019-02-07T09:54:03Z

src/Stripe.net/Entities/SubscriptionsSchedules/SubscriptionSchedulePhasePlan.cs

+        /// new billing period.
+        /// </summary>
+        [JsonProperty("billing_thresholds")]
+        public SubscriptionSchedulePhasePlanBillingThresholds BillingThresholds { get; set; }


My default position on this is that we should try to match the OpenAPI spec as much as possible. In this case, the spec uses subscription_item_billing_thresholds here so I think we should do the same. That way, if the structure changes in the future, we'll only have to update one class instead of two.

src/Stripe.net/Services/SubscriptionScheduleRevisions/SubscriptionScheduleRevisionService.cs

ob-stripe · 2019-02-07T11:30:35Z

src/Stripe.net/Services/SubscriptionSchedules/SubscriptionScheduleInvoiceSettingsOptions.cs

+    using System.Collections.Generic;
+    using Newtonsoft.Json;
+
+    public class SubscriptionScheduleInvoiceSettingsOptions : StripeEntity


Yeah, I feel the same, but I don't have a great solution to offer. The OpenAPI spec isn't even helpful here.

Maybe the right way to handle this is via inner classes, but we don't have a precedent for this in Stripe.net and I'm really not sure this is how we want to handle these nested parameters.

ob-stripe · 2019-02-07T11:31:33Z

src/Stripe.net/Services/SubscriptionSchedules/SubscriptionScheduleListOptions.cs

+
+    public class SubscriptionScheduleListOptions : ListOptionsWithCreated
+    {
+        // TODO: Figure out how to document those


Yeah, probably fine to leave them undocumented for now. I'll add docstrings once #1495 lands.

ob-stripe · 2019-02-07T12:40:39Z

src/Stripe.net/Services/SubscriptionSchedules/SubscriptionScheduleRenewalIntervalOptions.cs

+        /// Interval at which to renew the subscription schedule for when it ends. Possible values
+        /// are <code>day</code>, <code>week</code>, <code>month</code>, or <code>year</code>.
+        /// </summary>
+        // TODO: Decide if using PlanInterval


Yeah, it's annoying, and the OpenAPI spec is no help here either as the enums are not defined as a single schema that is referenced from different parameters, but rather duplicated for each parameter.

I'd leave as is for now, but it's something we should fix upstream.

src/StripeTests/Entities/SubscriptionScheduleRevisions/SubscriptionScheduleRevisionTest.cs

ob-stripe · 2019-02-07T17:01:32Z

Relatedly, there's a deserialization error because I try to expand phases.coupon but I don't get a coupon backand only getnull` which fails the test and shows either a failure in the fixture/openapi/something else.

Yes, phases.coupon is null in the fixtures file for both the schedule and schedule revision objects. Mind commenting out that assertion in the test for now?

remi-stripe · 2019-02-07T18:21:35Z

@alexander-stripe Assigning to you for the final review to ensure all the resources are described properly and that I did not miss anything.

The main "meta" question left is whether we want to "share" BillingThresholds classes between the one on a subscription item and the one on the phases[][plans][]. If you don't feel strongly we will go with what the PR did (separate classes) for now.

alexander-stripe · 2019-02-07T18:46:32Z

I glanced over this, it looks like it has the right shape to me. Thanks for this :-) I don't feel strongly to share the classes, although I can't see them diverge in the future so it may be worth unifying. Other than that

LGTM

remi-stripe · 2019-02-09T19:04:02Z

After spending a while on the stripe-java version (manual and autogenerated) I decided that I agreed and that Phase and PhaseItem were better names and that I should re-use the SubscriptionItemBillingThresholds* classes so I did just that.

* Add Subscription Schedule * Add Subscription Schedule Revisions * Add `invoice_settings` on the Customer

remi-stripe · 2019-02-12T06:18:54Z

@ob-stripe now that stripe-mock has been released, we should be able to merge. The feature is stable (but gated for a bit)

ob-stripe

LGTM

remi-stripe force-pushed the remi-add-subscription-schedules branch from 7ec3fb0 to dd7c76f Compare January 29, 2019 05:46

remi-stripe force-pushed the remi-add-subscription-schedules branch from dd7c76f to b98952e Compare February 7, 2019 05:38

remi-stripe commented Feb 7, 2019

View reviewed changes

remi-stripe assigned ob-stripe Feb 7, 2019

ob-stripe reviewed Feb 7, 2019

View reviewed changes

src/StripeTests/Entities/SubscriptionScheduleRevisions/SubscriptionScheduleRevisionTest.cs Outdated Show resolved Hide resolved

remi-stripe force-pushed the remi-add-subscription-schedules branch from b98952e to 33f1c6d Compare February 7, 2019 16:30

remi-stripe force-pushed the remi-add-subscription-schedules branch 2 times, most recently from 21795c8 to 7caa820 Compare February 7, 2019 18:02

remi-stripe assigned alexander-stripe and unassigned ob-stripe Feb 7, 2019

remi-stripe force-pushed the remi-add-subscription-schedules branch from 7caa820 to 48b1b7a Compare February 9, 2019 19:02

Add support for Subscription Schedule APIs

58fa431

* Add Subscription Schedule * Add Subscription Schedule Revisions * Add `invoice_settings` on the Customer

remi-stripe force-pushed the remi-add-subscription-schedules branch from 48b1b7a to 58fa431 Compare February 12, 2019 06:17

remi-stripe changed the title ~~[WIP] Add support for Subscription Schedule~~ Add support for Subscription Schedule Feb 12, 2019

remi-stripe assigned ob-stripe and unassigned alexander-stripe Feb 12, 2019

ob-stripe approved these changes Feb 12, 2019

View reviewed changes

ob-stripe added the approved label Feb 12, 2019

ob-stripe assigned remi-stripe and unassigned ob-stripe Feb 12, 2019

remi-stripe merged commit 3da6eea into master Feb 12, 2019

remi-stripe deleted the remi-add-subscription-schedules branch February 12, 2019 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Subscription Schedule #1491

Add support for Subscription Schedule #1491

remi-stripe commented Jan 28, 2019 •

edited

Loading

remi-stripe commented Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

remi-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

ob-stripe left a comment

ob-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

ob-stripe Feb 7, 2019

ob-stripe commented Feb 7, 2019

remi-stripe commented Feb 7, 2019

alexander-stripe commented Feb 7, 2019

remi-stripe commented Feb 9, 2019

remi-stripe commented Feb 12, 2019

ob-stripe left a comment

Add support for Subscription Schedule #1491

Add support for Subscription Schedule #1491

Conversation

remi-stripe commented Jan 28, 2019 • edited Loading

remi-stripe commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Feb 7, 2019

remi-stripe commented Feb 7, 2019

alexander-stripe commented Feb 7, 2019

remi-stripe commented Feb 9, 2019

remi-stripe commented Feb 12, 2019

ob-stripe left a comment

Choose a reason for hiding this comment

remi-stripe commented Jan 28, 2019 •

edited

Loading