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

Add support for listing source transactions #488

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe
cc @stripe/api-libraries @stan-stripe

Adds support for the /v1/sources/src_.../source_transactions endpoint.

I wrote a test, but as stripe-mock doesn't support this endpoint yet the test is skipped for the time being. Not great, but it doesn't seem like we have an easy way to stubbing requests in stripe-go tests.

@ob-stripe ob-stripe force-pushed the ob-source-transactions branch 2 times, most recently from d469f2b to fdc6346 Compare October 26, 2017 14:39
@brandur-stripe
Copy link
Contributor

@ob-stripe Can you double check the test code? Even if it's not run, it's still compiled, and I think the new test has a couple problems:

$ make
go test -race ./...
# github.com/stripe/stripe-go
./sourcetransaction.go:35:9: undefined: json
./sourcetransaction.go:39:3: undefined: t
./sourcetransaction.go:42:8: undefined: json
./sourcetransaction.go:46:18: undefined: t
./sourcetransaction.go:48:4: undefined: t
# github.com/stripe/stripe-go
./sourcetransaction.go:35:9: undefined: json
./sourcetransaction.go:39:3: undefined: t
./sourcetransaction.go:42:8: undefined: json
./sourcetransaction.go:46:18: undefined: t
./sourcetransaction.go:48:4: undefined: t

Live bool `json:"livemode"`
Source string `json:"source"`
Type string `json:"type"`
TypeData map[string]string `form:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be mixing json and form annotations (this might have been a bad copy + paste). It also looks like you'll have to run a gofmt on this.

}

return &Iter{stripe.GetIter(lp, body, func(b *form.Values) ([]interface{}, stripe.ListMeta, error) {
list := &stripe.SourceTransactionList{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably not convention elsewhere, but it might be worth failing fast here by returning an error if params.Source wasn't set. Currently I believe it'll fail with kind of a confusing couldn't find URL /sources//source_transactions 404 (or something of the sort).

@brandur-stripe
Copy link
Contributor

@ob-stripe Left a couple comments above, but thanks!

@ob-stripe ob-stripe force-pushed the ob-source-transactions branch 3 times, most recently from 7cf85cd to d3636cc Compare October 26, 2017 22:05
@ob-stripe
Copy link
Contributor Author

Not sure how I managed to commit a file in such a borked state, but it should be all fixed now. I also added a check for params.Source as requested.

@brandur-stripe brandur-stripe merged commit eaee0d6 into master Oct 27, 2017
@brandur-stripe brandur-stripe deleted the ob-source-transactions branch October 27, 2017 16:32
@brandur-stripe
Copy link
Contributor

Not sure how I managed to commit a file in such a borked state, but it should be all fixed now. I also added a check for params.Source as requested.

Happens to the best of us :) Thanks!

Release cut as 28.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants