-
Notifications
You must be signed in to change notification settings - Fork 295
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
[V7] Update BTCard
#1443
[V7] Update BTCard
#1443
Conversation
/// - number: The card number. | ||
/// - expirationMonth: The expiration month as a one or two-digit number on the Gregorian calendar. | ||
/// - expirationYear:The expiration year as a two or four-digit number on the Gregorian calendar. | ||
/// - cvv: The card verification code (like CVV or CID). |
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.
Based on this doc string I wonder if we should have 2 inits:
- CVV only init
- Init with number, expirationMonth, expirationYear, CVV, and postal code required
What do you think?
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.
Though I don't think postal code is actually required. We can confirm with the gateway and if not we should update the docstring to denote optional.
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.
After confirming with the Cards team, the only required parameters are number
, expirationMonth
, expirationYear
, and cvv
, the others are optional. Regarding overloading initializers, it’s not necessary, after the latest changes I made
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.
Gotcha, does this mean that we no longer support CVV only nonces for validation? Based on this docs string If you wish to create a CVV-only payment method nonce to verify a card already stored in your Vault, omit all other properties to only collect CVV.
was the intent for an overload. If this is no longer supported we should remove that note since currently this would not be possible.
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.
Thanks Jax, for the heads up! I’ve added a new initializer with CVV as the only parameter. As you mentioned, this is only to verify cards that are already stored, let me know what do you think.
/// - number: The card number. | ||
/// - expirationMonth: The expiration month as a one or two-digit number on the Gregorian calendar. | ||
/// - expirationYear:The expiration year as a two or four-digit number on the Gregorian calendar. | ||
/// - cvv: The card verification code (like CVV or CID). |
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.
Gotcha, does this mean that we no longer support CVV only nonces for validation? Based on this docs string If you wish to create a CVV-only payment method nonce to verify a card already stored in your Vault, omit all other properties to only collect CVV.
was the intent for an overload. If this is no longer supported we should remove that note since currently this would not be possible.
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!
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 very small take it or leave it cleanup comment and a general question that you may have already confirmed. Thanks again for looking into all of my questions! 🚀
Summary of changes
Docstrings:
Checklist
Authors