Add support for returning orders #1908
Conversation
There was a problem hiding this comment.
The code looks good to me. @cjavilla-stripe did you confirm that we're not missing any field on Order or OrderReturn too? If so, re-assign to ob to do the final review
| /// List of items to return. | ||
| /// </summary> | ||
| [JsonProperty("items")] | ||
| public List<OrderReturnItemOptions> Items { get; set; } |
There was a problem hiding this comment.
This naming feels like a new pattern. Usually we don't put the action/word name in the options class. But if we don't here, it conflicts with the one for Create which is the same hash but wit currency on top of it.
@ob-stripe Can you check and confirm that's the right naming scheme?
There was a problem hiding this comment.
Yeah, I think so. We're just "lucky" that in most cases the shape is shared between e.g. create and update methods, but more generally we probably shouldn't be sharing parameters, at least until the OpenAPI spec actually gives us that information.
|
Thanks @remi-stripe, I reviewed Order and OrderReturn against the API ref and they have all the fields listed (added some of the doc comments). r? @ob-stripe Can you take a pass at this one? |
| /// List of items to return. | ||
| /// </summary> | ||
| [JsonProperty("items")] | ||
| public List<OrderReturnItemOptions> Items { get; set; } |
There was a problem hiding this comment.
Yeah, I think so. We're just "lucky" that in most cases the shape is shared between e.g. create and update methods, but more generally we probably shouldn't be sharing parameters, at least until the OpenAPI spec actually gives us that information.
While deprecated, this is currently available in the API so we should support it.
This is my first time adding a custom method to a service like this where the return type is different than the underlying entity. I.e. returning an order, returns an OrderReturn object, I think I did this right, but could use another set of eyes. Another note, I created OrderReturnItemOptions as a new type because OrderItemOptions has the additional
currencyfield. LMK if that was the wrong move.r? @remi-stripe
cc @stripe/api-libraries @ob-stripe