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 spending_limits on Issuing Card and Cardholder #1576

Closed
wants to merge 1 commit into from

Conversation

remi-stripe
Copy link
Contributor

Related to stripe/stripe-go#831

r? @ob-stripe
cc @stripe/api-libraries

@remi-stripe
Copy link
Contributor Author

cc @zacht-stripe to triple check the changes and make sure I did not miss anything

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

LGTM minus one minor comment.

[JsonProperty("spending_limits")]
public SpendingLimits SpendingLimits { get; set; }

[Obsolete("This is now unsupported")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we improve this message? This isn't super helpful 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I was not sure what to put. Like there's no replacement or anything. It's just removed as a property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ob-stripe What do you think of this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to change it? The message hasn't changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am asking you what I should put. This option is just entirely removed, it's not replaced by anything. So I don't know what to put beyond "please stop using it". Do you have any idea what string would be enough for you?

@remi-stripe
Copy link
Contributor Author

Turns out spending_limits is an array, not a hash. So I've pushed a new version of it. @ob-stripe should we rename SpendingLimits to SpendingLimit instead so that you have List<SpendingLimit> SpendingLimits;? I'm assuming yes

@remi-stripe remi-stripe assigned ob-stripe and unassigned ob-stripe Apr 16, 2019
@ob-stripe
Copy link
Contributor

should we rename SpendingLimits to SpendingLimit instead so that you have List<SpendingLimit> SpendingLimits;? I'm assuming yes

Yes please.

@ob-stripe ob-stripe assigned remi-stripe and unassigned ob-stripe Apr 17, 2019
@remi-stripe remi-stripe force-pushed the remi-issuing-authorization-controls branch from daa24bc to 868fb9a Compare April 17, 2019 02:03
@remi-stripe
Copy link
Contributor Author

@ob-stripe okay fixed. Can I ask you to re-review carefully? I broke stripe-go so want to make ~sure it's correct :p

@remi-stripe remi-stripe assigned ob-stripe and unassigned remi-stripe Apr 17, 2019
@ob-stripe ob-stripe assigned remi-stripe and unassigned ob-stripe Apr 17, 2019
@the800kid
Copy link

Hi @ob-stripe / @remi-stripe
Just wondering if these changes are going to make it into an official release? I am in need of this feature. Thanks.

@remi-stripe
Copy link
Contributor Author

@the800kid Thanks for reaching out! I paused those changes for now as we're changing the shape of the API a little bit so I was waiting for someone to need this before releasing it.

Since you need it though, I'll work on finishing those today!

@remi-stripe
Copy link
Contributor Author

Closing in favour of #1877

@remi-stripe remi-stripe deleted the remi-issuing-authorization-controls branch December 12, 2019 17:25
@the800kid
Copy link

@remi-stripe Thank you for the quick reply and action. I see you have made changes to the models since the original work. Looking forward to being able to use the feature. Thanks again!

@remi-stripe
Copy link
Contributor Author

@the800kid This was released in 34.4.0! Let me know if you encounter any issues!

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.

3 participants