When losing focus show field as invalid #3482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… it matches any of the possible values. * If the credit card is not complete and focus changes, show an error. * If the CVC is at least one character check to see if it matches any of the possible values when the brand updates. * If the CVC is not complete and focus changes, show an error. Credit card = 3 -> no error (matches amex and diners) Credit card = 3, move to new field -> show error Credit card = 0 -> error Credit card of 37, CVC of 1234, Credit card to 4 -> CVC should be in error. (brand updates checks cvc) Credit card = any single possible 1 digit, CVC of 1, move to a new field -> Show CVC in error.
* If the credit card number is at least one character check to see if it matches any of the possible values. * If the credit card is not complete and focus changes, show an error. * If the CVC is at least one character check to see if it matches any of the possible values when the brand updates. * If the CVC is not complete and focus changes, show an error. Credit card = 3 -> no error (matches amex and diners) Credit card = 3, move to new field -> show error Credit card = 0 -> error Credit card of 37, CVC of 1234, Credit card to 4 -> CVC should be in error. (brand updates checks cvc) Credit card = any single possible 1 digit, CVC of 1, move to a new field -> Show CVC in error.
mshafrir-stripe
requested changes
Mar 18, 2021
stripe/src/test/java/com/stripe/android/view/CardNumberEditTextTest.kt
Outdated
Show resolved
Hide resolved
| if (hasFocus) { | ||
| cardInputListener?.onFocusChange(CardInputListener.FocusField.CardNumber) | ||
| } | ||
| } | ||
|
|
||
| expiryDateEditText.onFocusChangeListener = OnFocusChangeListener { _, hasFocus -> | ||
| expiryDateEditText.internalFocusChangeListener.add { _, hasFocus -> | ||
| // TODO: Why is this only called when have focus? |
There was a problem hiding this comment.
I intend to remove this comment, but I am curious if you know?
There was a problem hiding this comment.
onFocusChange() is called with the field that now has focus
There was a problem hiding this comment.
Would it make sense to name it onFocusGained? Not considering it for this PR, but might be worth thinking about if we are in this code again.
… simplify the logic on focus loss in the CardNumberEditText.kt
mshafrir-stripe
requested changes
Mar 18, 2021
stripe/src/main/java/com/stripe/android/view/CardNumberEditText.kt
Outdated
Show resolved
Hide resolved
|
I have a few more unit tests to add in the morning. But it tests well. I also have postal code text color setting in another branch. |
stripe/src/main/java/com/stripe/android/model/ExpirationDate.kt
Outdated
Show resolved
Hide resolved
stripe/src/main/java/com/stripe/android/model/ExpirationDate.kt
Outdated
Show resolved
Hide resolved
| // and the Cvc does not validate | ||
| if (unvalidatedCvc.normalized.isNotEmpty()) { | ||
| shouldShowError = unvalidatedCvc.validate(cardBrand.maxCvcLength) == null | ||
| // TODO(michelleb-stripe): Should truncate CVC on a brand name change. |
There was a problem hiding this comment.
Should we auto-truncate or just mark as error?
There was a problem hiding this comment.
Web Checkout truncates
stripe/src/main/java/com/stripe/android/view/CardNumberEditText.kt
Outdated
Show resolved
Hide resolved
stripe/src/main/java/com/stripe/android/view/CardNumberEditText.kt
Outdated
Show resolved
Hide resolved
mshafrir-stripe
requested changes
Mar 19, 2021
stripe/src/main/java/com/stripe/android/model/ExpirationDate.kt
Outdated
Show resolved
Hide resolved
stripe/src/main/java/com/stripe/android/view/CardNumberEditText.kt
Outdated
Show resolved
Hide resolved
stripe/src/main/java/com/stripe/android/view/CardNumberEditText.kt
Outdated
Show resolved
Hide resolved
mshafrir-stripe
previously approved these changes
Mar 22, 2021
mshafrir-stripe
approved these changes
Mar 22, 2021
mshafrir-stripe
approved these changes
Mar 22, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Credit card = 3 -> no error (matches amex and diners)
Credit card = 3, move to new field -> show error
Credit card = 0 -> error
Credit card of 37, CVC of 1234, Credit card to 4 -> CVC should be in error. (brand updates checks cvc)
Credit card = any single possible 1 digit, CVC of 1, move to a new field -> Show CVC in error.
There is still a problem when the month loses focus, but that will be addressed in a separate PR. If desired I can split these into two reviews one for credit card and one for the CVC, just let me know on the early side.
There is also still a problem with postal code validation.
Motivation
MOBILESDK-170
Testing