Pantera plan and product updates #1099

pantera-stripe · 2018-02-08T21:49:48Z

Summary
Replacement for @jhoon-stripe's PR: #1068
This PR adds support for an upcoming Stripe API version, released on 2018-02-05. Extends the original PR by adding nickname to StripePlan and type to StripeProduct. Also adds BillingCycleAnchor to StripeSubscription which was also released on 2018-02-05

r? @ob-stripe @brandur-stripe (brandur reviewed original PR)
cc @stripe/api-libraries

remi-stripe · 2018-02-12T13:30:42Z

@pantera-stripe wanted to flag #1101 to make sure it could be done in the same PR. Let me know if you need help!

pantera-stripe · 2018-02-12T16:30:10Z

@remi-stripe You're right, we're missing that variable. I was about to cargo-cult the code from the create options, but I just realized... it's buggy. (If I'm reading this correctly)

        [JsonProperty("billing_cycle_anchor")]
        internal string BillingCycleAnchorInternal
        {
            get
            {
                if (BillingCycleAnchorNow)
                    return "now";

                if (BillingCycleAnchorUnchanged)
                    return "unchanged";

                return BillingCycleAnchor?.ConvertDateTimeToEpoch().ToString();
            }
        }

If BillingCycleAnchorNow is true it'll send now. If you set both BillingCycleAnchorNow and BillingCycleAnchorUnchanged to true it'll also send now! Yay!

Basically, setting BillingCycleAnchorNow will override either of the other values. I'm fine replicating this not-so-amazing behavior in this PR. But I'd like to push a larger fix off to another PR. I really want to get this reviewed and merged.

remi-stripe · 2018-02-12T16:33:20Z

@pantera-stripe I'm fine with reproducing the current behaviour. It's not perfect but the alternative would be to either send both and rely on the API error or do an assert or similar to leave early. FWIW we have some logic like this in stripe-go where we make a decision on which one to send first and I think it's okay.

pantera-stripe · 2018-02-12T17:17:34Z

r? @brandur-stripe @remi-stripe

remi-stripe · 2018-02-12T17:31:00Z

src/Stripe.Tests.XUnit/plans/_fixture.cs

+            PlanCreateOptions = new StripePlanCreateOptions() {
+                // Add a space at the end to ensure the ID is properly URL encoded
+                // when passed in the URL for other methods
+                Id = "test-plan-" + Guid.NewGuid().ToString() + " ",


We likely can skip enforcing the Id here since it's now optional

remi-stripe · 2018-02-12T17:32:02Z

src/Stripe.Tests.XUnit/plans/when_creating_and_updating_plans.cs

@@ -11,14 +11,13 @@ public class creating_and_updating_plans : IClassFixture<plans_fixture>

        public creating_and_updating_plans(plans_fixture fixture)
        {
-            this.fixture = fixture;
+            this.fixture = fixture; 


remove extra space

remi-stripe · 2018-02-12T17:32:20Z

src/Stripe.Tests.XUnit/plans/when_creating_and_updating_plans.cs

        }

        [Fact]
        public void created_has_the_right_details()
        {
            fixture.Plan.Id.Should().Be(fixture.PlanCreateOptions.Id);
-            fixture.Plan.Name.Should().Be(fixture.PlanCreateOptions.Name);


Shouldn't we test the Nickname property here?

remi-stripe · 2018-02-12T17:32:40Z

src/Stripe.Tests.XUnit/plans/when_creating_and_updating_plans.cs

@@ -28,7 +27,6 @@ public void created_has_the_right_details()
        public void updated_has_the_right_details()
        {
            fixture.PlanUpdated.Id.Should().Be(fixture.Plan.Id);
-            fixture.PlanUpdated.Name.Should().BeEquivalentTo(fixture.PlanUpdateOptions.Name);


same as above, we should look at Nickname

remi-stripe · 2018-02-12T17:34:25Z

src/Stripe.net/Entities/StripePlan.cs

@@ -35,10 +35,16 @@ public class StripePlan : StripeEntityWithId
        [JsonProperty("name")]
        public string Name { get; set; }


Shouldn't we remove Name completely?

Wouldn't that prevent existing API integrations from upgrading their bindings?

I don't entirely follow. Anyone who gets that new version of the code will already have to change their integration since it's a new API version and we've added/moved/removed multiple properties.

Name does not exist on the new API version, only Nickname does so Name should go.

This will all be behind a major version

Right, I'm talking the other way. Do merchants ever update their bindings independent of their API version? If not, then I can remove it. But if they do, then updating bindings to this version would eliminate their ability to access Name

@pantera-stripe stripe-dotnet enforces an API version for the entire library. There is no way to "choose" an API version. Every user of stripe-dotnet is locked to the API version that the version of the library they use has been built for.

In another part of your PR you are now enforcing 2018-02-06 so every developer who upgrades their integration to that new version of stripe-dotnet will use that version on every API request. So Name will never exist or be set.

Yeah. Totally overlooked that. Will remove.

remi-stripe · 2018-02-12T17:35:46Z

src/Stripe.net/Entities/StripeProduct.cs


        /// <summary>
        /// A sublist of active SKUs associated with this product.
        /// </summary>
        [JsonProperty("skus")]
        public StripeList<StripeSku> Skus { get; set; }

+        /// <summary>
+        /// What appears on a customers credit card statement. Max of 22 characters.


customer's
I'd also remove the 22 chars mention, better to keep this in the official docs.

remi-stripe · 2018-02-12T17:36:33Z

src/Stripe.net/Entities/StripeProduct.cs

+        public string StatementDescriptor { get; set; }
+
+        /// <summary>
+        /// The type of the Product. Either 'good' or 'service'


missing trailing .

remi-stripe · 2018-02-12T17:38:13Z

src/Stripe.net/Services/Plans/StripePlanCreateOptions.cs

        [JsonProperty("metadata")]
        public Dictionary<string, string> Metadata { get; set; }
+
+        [JsonProperty("nickname")]
+        public string Nickname { get; set; }


should be in alphabetical order if we can

remi-stripe · 2018-02-12T17:39:49Z

src/Stripe.net/Services/Products/StripeProducNestedCreateOptions.cs

+
+namespace Stripe
+{
+    public class StripeProductNestedCreateOptions : INestedOptions


Since it's nested under the plan options I think we might want to name it StripePlanProductCreatOptions to indicate the relation.

Not a huge deal though as naming is at best inconsistent in the library today already.

My thought was there is nothing specific about the nesting to plans -- though you're right, that's the only place it's used and probably the only place it'll ever be used.

remi-stripe · 2018-02-12T17:41:19Z

src/Stripe.net/Services/Products/StripeProducNestedCreateOptions.cs

+
+        [JsonProperty("product[statement_descriptor]")]
+        public string StatementDescriptor { get; set; }
+    }


We should support all options here and not just name and statement_descriptor. The API docs indicate we support id and metadata too.

Good catch.

pantera-stripe · 2018-02-13T04:08:05Z

Applied the feedback.

r? @remi-stripe

remi-stripe · 2018-02-13T13:40:17Z

Looks good to me. I don't like giving the only +1 on PRs though.

r? @ob-stripe

ob-stripe

LGTM overall, just one comment to address.

ob-stripe · 2018-02-13T17:55:16Z

src/Stripe.net/Entities/StripePlan.cs

+        public string Nickname { get; set; }
+
+        [JsonProperty("product")]
+        public string ProductId { get; set; }

        [JsonProperty("statement_descriptor")]
        public string StatementDescriptor { get; set; }


Shouldn't we remove this property?

Yup. Will remove.

ob-stripe · 2018-02-13T19:50:57Z

@pantera-stripe LGTM. Mind squashing the commits before I merge?

… update

pantera-stripe · 2018-02-13T20:56:29Z

@ob-stripe squashed

pantera-stripe changed the title ~~Pantera plan and product updates~~ WIP Pantera plan and product updates Feb 8, 2018

pantera-stripe force-pushed the pantera-plan-and-product-updates branch 2 times, most recently from 8ef0147 to 4ab177b Compare February 11, 2018 02:23

remi-stripe mentioned this pull request Feb 12, 2018

Missed BillingCycleAnchorNow in the StripeSubscriptionUpdateOptions #1101

Closed

pantera-stripe changed the title ~~WIP Pantera plan and product updates~~ Pantera plan and product updates Feb 12, 2018

remi-stripe reviewed Feb 12, 2018

View reviewed changes

remi-stripe assigned ob-stripe Feb 13, 2018

ob-stripe suggested changes Feb 13, 2018

View reviewed changes

ob-stripe approved these changes Feb 13, 2018

View reviewed changes

Bump API version to 2018-02-06 and add support for Product & Plan API…

02af378

… update

pantera-stripe force-pushed the pantera-plan-and-product-updates branch from 2ee813c to 02af378 Compare February 13, 2018 20:55

ob-stripe added approved breaking-api-change labels Feb 13, 2018

ob-stripe merged commit 8b35fa0 into stripe:master Feb 13, 2018

pantera-stripe deleted the pantera-plan-and-product-updates branch February 13, 2018 21:54

pantera-stripe mentioned this pull request Feb 15, 2018

[WIP] updates for upcoming Stripe API version bump #1068

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pantera plan and product updates #1099

Pantera plan and product updates #1099

pantera-stripe commented Feb 8, 2018 •

edited

Loading

remi-stripe commented Feb 12, 2018

pantera-stripe commented Feb 12, 2018 •

edited

Loading

remi-stripe commented Feb 12, 2018

pantera-stripe commented Feb 12, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

pantera-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

pantera-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

pantera-stripe Feb 13, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

remi-stripe Feb 12, 2018

pantera-stripe Feb 13, 2018

remi-stripe Feb 12, 2018

pantera-stripe Feb 13, 2018

pantera-stripe commented Feb 13, 2018

remi-stripe commented Feb 13, 2018

ob-stripe left a comment

ob-stripe Feb 13, 2018

pantera-stripe Feb 13, 2018

ob-stripe commented Feb 13, 2018

pantera-stripe commented Feb 13, 2018

		@@ -35,10 +35,16 @@ public class StripePlan : StripeEntityWithId
		[JsonProperty("name")]
		public string Name { get; set; }

Pantera plan and product updates #1099

Pantera plan and product updates #1099

Conversation

pantera-stripe commented Feb 8, 2018 • edited Loading

remi-stripe commented Feb 12, 2018

pantera-stripe commented Feb 12, 2018 • edited Loading

remi-stripe commented Feb 12, 2018

pantera-stripe commented Feb 12, 2018

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

Choose a reason for hiding this comment

pantera-stripe commented Feb 13, 2018

remi-stripe commented Feb 13, 2018

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

ob-stripe commented Feb 13, 2018

pantera-stripe commented Feb 13, 2018

pantera-stripe commented Feb 8, 2018 •

edited

Loading

pantera-stripe commented Feb 12, 2018 •

edited

Loading