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

Feature Request: Add models/entities for working with shipping+tax callbacks #1515

Closed
WillyHonez opened this issue Feb 14, 2019 · 6 comments · Fixed by #1482
Closed

Feature Request: Add models/entities for working with shipping+tax callbacks #1515

WillyHonez opened this issue Feb 14, 2019 · 6 comments · Fixed by #1482
Assignees
Labels

Comments

@WillyHonez
Copy link

Looking at this: https://stripe.com/docs/orders/dynamic-shipping-taxes

I thought - OK cool neat I can just take the order request posted to my server and that'll deserialize right into the Order object from the library. It doesn't : /

It would be helpful if an 'order' from the above mentioned doc was the same as an order in the dotnet lib.

Alt: let us pass in shipping options + taxes on order creation instead of requiring the callback.

@remi-stripe
Copy link
Contributor

@HopesOblivion What part specifically doesn't work with stripe-dotnet exactly? Would you be able to provide a sample JSON that breaks your integration? As far as I know we would send the normal Order payload to your endpoint.

@remi-stripe remi-stripe self-assigned this Feb 14, 2019
@WillyHonez
Copy link
Author

@remi-stripe wow that was fast. The problem is that the order payload sent doesn't deserialize into a stripe-dotnet Order.

You can find a sample json here: https://stripe.com/docs/orders/dynamic-shipping-taxes

Ideally I'd like to just setup an API Controller with a method signature like public IActionResult(Order orderThatYoullPostMeInTheCallback) { // do stuff; return Ok(new CallBackResponseYouWant{ ...} );}

Specific Issues:

  • The payload is actually { order: {} } so you have to make a dummy class with a single order property or use JObject or dynamic (not a big deal really, just kind of weak)
  • Stripe.OrderItem.Parent is a string, in the order payload the Parent is a fully expanded Sku
  • The response that the callback is looking for isn't modeled in stripe-dotnet anywhere

So my feature request is - add Entities that accurately represent the Request and Response for the callback shipping & tax calculations so we can just plug them into our api.

I hope that's clear.

@remi-stripe
Copy link
Contributor

remi-stripe commented Feb 14, 2019

The payload is actually { order: {} } so you have to make a dummy class with a single order property or use JObject or dynamic (not a big deal really, just kind of weak)

Gotcha. Could you just parse the JSON and the deserialize what is inside order onto our Order object instead?

Stripe.OrderItem.Parent is a string, in the order payload the Parent is a fully expanded Sku

This seems like something we should support in stripe-dotnet since parent is expandable. It isa bit of an exception in our API though as it expands only if it relates to a SKU. I started a PR to get this discussed asI: #1516

The response that the callback is looking for isn't modeled in stripe-dotnet anywhere

That one is a bit trickier to implement. We'd basically need a separate model for this. Today stripe-dotnet focuses on the public API in a way and what you're using is quite separate. Here you're more the one having the API and we contact you asking for an answer. In a lot of cases, this callback is built by third-party platforms such as Shippo for example.

I do see how it could be useful for sure. Realistically though, I'm not convinced it's something we would actively maintain in stripe-dotnet unfortunately, at least not in the short term.

let us pass in shipping options + taxes on order creation instead of requiring the callback.

This is definitely something we've thought about before. The original intent of this API was to rely on third-party services to handle shipping and tax though we made it flexible enough to work in more cases. I definitely agree that when you control both sides, it's easier to pass everything at creation. I just don't think it's something we would get to in the short term.

For now, I'm going to tag as future while we discuss this!

@WillyHonez
Copy link
Author

Could you just parse the JSON and the deserialize what is inside order onto our Order object instead?

It chokes on the OrderItem.Parent, I tried passing in StripeConfiguration.SerializerSettings and StripeObjectConverter to the Newtonsoft.Json.JsonConvert.DeserializeObject but neither handled it.

We'd basically need a separate model for this.

Yeah, but it would be pretty simple, just a Request and Response class really that wrap the Order (assuming the OrderItem.Parent could be expanded). Another fun thing I noticed, the json payload also includes an expanded Product on the Sku that is not shown in the docs : /

The original intent of this API was to rely on third-party services to handle shipping and tax though we made it flexible enough to work in more cases.

We're actually using Shippo & TaxJar, unfortunately the third-party integrations fall apart pretty quickly if:

  • You need to ship from more than one location
  • Your shipments need multiple packages - the calculations we're getting back are seriously terrible so we started down the callback route today

Thanks again for taking the time - if I should also send my "let us pass in shipping options + taxes request" another place as well let me know.

@remi-stripe
Copy link
Contributor

It chokes on the OrderItem.Parent, I tried passing in StripeConfiguration.SerializerSettings and StripeObjectConverter to the Newtonsoft.Json.JsonConvert.DeserializeObject but neither handled it.

Yes sorry I meant once we fix the fact that Parent should be Expandable. You might be able to try my branch/PR for now.

Yeah, but it would be pretty simple, just a Request and Response class really that wrap the Order (assuming the OrderItem.Parent could be expanded).

Agreed, though I would not call this "pretty simple" here. This is something we do not support today in go/java/.NET. I agree that we should consider it but it's not as straightforward as it looks.

For example we're working on auto-generating our libraries from openapi and the shape of the resource you described is quite separate from our API so we'd need to special-case this.

Another fun thing I noticed, the json payload also includes an expanded Product on the Sku that is not shown in the docs : /

The documentation does show that product on the SKU is expandable though. Our API does not auto-expand, but what you receive is completely separate from our API in a sense. Since you have no control over what can be expanded here (since we make the request) and since this needs to be fast (since your endpoint is called synchronously during the Order creation request) we don't want you to have to fetch data from our API. For that reason, we provide the item, its parent and the related parent product already expanded.

I agree this could be documented a bit better, but it is really quite a corner case of the API in a way. Most integrations relying on this logic usually talk to us directly.

Thanks again for taking the time - if I should also send my "let us pass in shipping options + taxes request" another place as well let me know.

I've raised it internally again. It's an ask we've heard before but it's something we'd like to fix with a better approach than just passing a hash with the tax or shipping details. You could imagine a world where you manage tax rates in the API and then pass a tax rate id on creation. This is more a "crazy idea" right now though and we are not making changes to the Orders API at the moment.

@ob-stripe
Copy link
Contributor

As of version 27.0.0, the OrderItem.Parent property is now expandable.

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 a pull request may close this issue.

3 participants