Add status_transitions filters to the List Orders params #683
Conversation
spastorelli-stripe
force-pushed
the
spastorelli-add-status-transitions-param
branch
from
c49858e to
04dbeb9
Compare
spastorelli-stripe
changed the title
There was a problem hiding this comment.
And thanks for taking this as well @spastorelli-stripe! Looks great. I left a couple minor comments below.
order.go
Outdated
| // StatusTransitionsFilterParams are parameters that can used to filter on status_transition when listing orders. | ||
| type StatusTransitionsFilterParams struct { | ||
| StatusTransitions `form:"*"` | ||
| CanceledRange *RangeQueryParams `form:"canceled"` |
There was a problem hiding this comment.
Could you also add a field for each of Canceled, Fulfilled, Paid, and Returned which is a *int64? This is how we generally support either a range or a basic Unix timestamp like for Created. From the docs:
A filter on the list based on the object canceled field. The value can be a string with an integer Unix timestamp, or it can be a dictionary with the following options:
There was a problem hiding this comment.
I had implicitly provided these fields by embedding StatusTransitions in StatusTransitionsFilterParams.
Should I instead explicitly include each of the fields in StatusTransitionsFilterParams?
There was a problem hiding this comment.
Yeah, can we do that please? e.g.
type StatusTransitionsFilterParams struct {
StatusTransitions `form:"-"`
Canceled *int64 `form:"canceled"`
CanceledRange *RangeQueryParams `form:"canceled"`
FulfilledRange *RangeQueryParams `form:"fulfilled"`
...It's nicely symmetrical with what we're doing with Created / CreatedRange just above.
ptal @spastorelli-stripe
There was a problem hiding this comment.
Thanks. Done.
order.go
Outdated
| Canceled int64 `json:"canceled" form:"canceled"` | ||
| Fulfilled int64 `json:"fulfiled" form:"fulfilled"` | ||
| Paid int64 `json:"paid" form:"paid"` | ||
| Returned int64 `json:"returned" form:"returned"` |
There was a problem hiding this comment.
And let's drop the form annotations from this struct — they're not needed.
order/client_test.go
Outdated
| PaidRange: &stripe.RangeQueryParams{ | ||
| GreaterThan: int64(123456), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I don't think adding the new predicate is strictly needed here because both the form annotations and RangeQueryParams serialization are pretty well tested already (see form/form_test.go and params_test.go).
There was a problem hiding this comment.
Makes sense. Removed.
spastorelli-stripe
force-pushed
the
spastorelli-add-status-transitions-param
branch
from
04dbeb9 to
24a8d26
Compare
|
Thanks @brandur-stripe for the review. PTAL |
spastorelli-stripe
force-pushed
the
spastorelli-add-status-transitions-param
branch
from
24a8d26 to
50a6890
Compare
|
Thanks @brandur-stripe for the follow up. PTAL |
order.go
Outdated
| type StatusTransitionsFilterParams struct { | ||
| Canceled *int64 `form:"canceled"` | ||
| CanceledRange *RangeQueryParams `form:"canceled"` | ||
| Fulfilled *int64 `form:"fulfiled"` |
There was a problem hiding this comment.
Looks great!
One more very minor bug here: this form annotation should be "fulfilled" (with two Ls) like the one below it.
ptal @spastorelli-stripe
There was a problem hiding this comment.
Thanks for catching this. Done.
spastorelli-stripe
force-pushed
the
spastorelli-add-status-transitions-param
branch
from
50a6890 to
d4cab50
Compare
|
Thanks @brandur-stripe. PTAL |
|
LGTM. Thanks! |
|
Released as 48.1.0. |
Add
status_transitionsfilters to the List Orders params.Attempts to fix #682
r? @brandur-stripe
cc @stripe/api-libraries