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 ability to set card number and validate it #671

Merged
merged 3 commits into from
Dec 19, 2018

Conversation

ksun2-stripe
Copy link
Contributor

In response to:
#508

/**
* Checks whether the current card number is valid
*/
public boolean validateCardNumber() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Appears to be unused, is that intentional? Does it need to be a public method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is intentional. We are adding the ability for our users to call this. I think there's cases where they will want to just add the number and cases when they will want to add it validate it so we allow them to do both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a test for this method then?

*
* @param cardNumber card number to be set
*/
public void setCardNumber(String cardNumber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add tests for the various types of card numbers (e.g. cards with spaces, cards without?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added them. Though, TBH, I don't think adding tests here do adds that much safety. More test is better than fewer and this doesn't feel like a fragile test so I'm in the it can't hurt camp.

This is a one line wrapper file that exists to expose existing functionality to users on the SDK. That existing functionality is already tested

public void getCard_whenInputIsValidVisaWithZip_returnsCardObjectWithLoggingToken() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also one more nit, might be good practice to annotate new methods we add (@NonNull in this case)

Copy link
Collaborator

@mshafrir-stripe mshafrir-stripe left a comment

Choose a reason for hiding this comment

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

didn't mean to approve

@ksun2-stripe ksun2-stripe merged commit 7fc0d1d into master Dec 19, 2018
@mshafrir-stripe mshafrir-stripe mentioned this pull request Jan 10, 2019
@ksun2-stripe ksun2-stripe mentioned this pull request Jan 15, 2019
@mshafrir-stripe mshafrir-stripe deleted the ksun-add-multiline-listener branch January 24, 2019 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants