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

several bug fixes to client #192

Closed
wants to merge 4 commits into from

Conversation

@daringordon daringordon changed the title CardBrand had bad expected values from stripe, so I updated accordingly several bug fixes to client Aug 30, 2021
@daringordon
Copy link
Author

  1. detach payment method
  2. create setupintent enhancement
  3. payment_intent update enhancement
  4. aligned the expected serde values for CardBrand with those received from the stripe api
  5. add transfer_data to payment_intent update

@seanpianka seanpianka requested review from arlyon and stearnsc August 30, 2021 19:07
}

impl<'a> CreateSetupIntent<'a> {
pub fn new() -> Self {
CreateSetupIntent {
confirm: Default::default(),
Copy link
Member

Choose a reason for hiding this comment

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

Should this fn just return Default::default()?

@@ -154,26 +154,26 @@ pub enum CheckResult {

#[derive(Copy, Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
pub enum CardBrand {
#[serde(rename = "American Express")]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is safe -
https://stripe.com/docs/api/cards/object#card_object-brand

Would this meet your needs? #188

Copy link
Collaborator

Choose a reason for hiding this comment

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

#188 has now been merged. The enum PaymentMethodCardBrand now holds variants that would be helpful here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, #188 covers. Thanks!

@stearnsc
Copy link
Member

I think this looks good other than the CardBrand issue - if #188 meets your needs for that portion, I'm happy merging the other parts.

@seanpianka seanpianka added the bug Something isn't working label Sep 11, 2021
@seanpianka
Copy link
Collaborator

Thanks for the review, @stearnsc!

@daringordon, would you be interested in submitting separate PRs for these different changes? I believe the first change has now been covered by #141.

@daringordon
Copy link
Author

daringordon commented Dec 6, 2021

OK. I'm closing this PR as two of the commits in it were addressed by 188 and 141. This PR is now replaced by a reduced version: #202

@seanpianka @stearnsc

@daringordon daringordon closed this Dec 6, 2021
@daringordon daringordon deleted the updated_cardbrand branch December 6, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants