-
Notifications
You must be signed in to change notification settings - Fork 461
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 spending_limits
with Issuing Card and Cardholder
#831
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks Remi!
Categories []*string `form:"categories"` | ||
Interval *string `form:"interval"` | ||
} | ||
|
||
// AuthorizationControlsParams is the set of parameters that can be used for the shipping parameter. | ||
type AuthorizationControlsParams struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I never prefixed this with Issuing. Do you agree it's better to rename in the next major? (there will be an API version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I noticed that too. Yeah, I think it's fine to just wait for the next major.
|
||
// The following parameters are considered deprecated and only apply to issuing cards | ||
MaxAmount *int64 `form:"max_amount"` | ||
MaxApprovals *int64 `form:"max_approvals"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this as this is going away in a future API version and is technically unsupported on Cardholder
// IssuingCardAuthorizationControls is the resource representing authorization controls on an issuing card. | ||
// TODO: Rename to IssuingAuthorizationControls in the next major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this as I am using the bad class name in Cardholder
. An API version will be released to change this so I wanted to avoid 2 major versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1.
@brandur-stripe Thanks for the review! For some reason I forgot to post my review/comments after I was done yesterday. Any chance you could have a quick look? |
Thanks for the additional flavor Remi — I'm in agreement on waiting for the next major and just having the TODO for now. LGTM. |
Bumps [sequel](https://github.com/jeremyevans/sequel) from 5.60.1 to 5.61.0. - [Release notes](https://github.com/jeremyevans/sequel/releases) - [Changelog](https://github.com/jeremyevans/sequel/blob/master/CHANGELOG) - [Commits](jeremyevans/sequel@5.60.1...5.61.0) --- updated-dependencies: - dependency-name: sequel dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
r? @brandur-stripe
cc @stripe/api-libraries @zacht-stripe