-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
feat: add Elo as a supported credit card provider #326
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Rai Siqueira <[email protected]>
Thank you very much! Where did you get the regex from? Is there a good source that I can use to check what's a valid Elo credit card? |
Hi @fabian-hiller, The source is from this project: https://github.com/raisiqueira/react-payment-inputs/blob/master/src/utils/cardTypes.js. I've used this library when I was working on a Brazilian fintech. To check if Elo credit card number is valid, you can use this website (Portuguese only) to generate a valid credit card number. |
Thanks @raisiqueira for PR, we will review it. @fabian-hiller here are tests cards also for Elo https://docs.adyen.com/development-resources/testing/test-card-numbers/ we could also extend it for other cards like Bancontact, Cartes Bancaires, Dankort, Hipercard in separate PRs. @fabian-hiller usually is good that, we would know what type of card it is, visa, mastercard, since in the forms there is implemented usually change of logotip based on the provider. Enhancement would be that output returns a provider name, if card is valid? |
Thank you for this PR. @ariskemper feel free to create another PR for other providers. How would you output the provider name? Not sure if this should be part of Valibot. But we could export the specific provider regex we use so that the same regex can be used to determine the provider with custom code. |
@raisiqueira would be enough maybe shorter and less complex Regex, because Elo credit card numbers usually starts with the following prefixes and are 16 digits in length:
we need to account for these prefixes and ensure that the total length of the card number is 16 digits? |
Hey @ariskemper I believe that this regex that you shared is enough. I'll update the merge request. |
Signed-off-by: Rai Siqueira <[email protected]>
@raisiqueira if you could add more tests to cover different valid and invalid card numbers would be great. |
Signed-off-by: Rai Siqueira <[email protected]>
@ariskemper Done. Check if the new assertions are enough. |
@raisiqueira for now should be fine, we could add extra tests in next releases for missing prefixes. @fabian-hiller what do you think? |
Looks good on the first view. I will focus on other parts of the project for this and maybe the next week. After that I will review and merge everything. |
b997a0f
to
6d0cde4
Compare
This is an additional merge request for the #292 adding Elo as a credit card provider.
closes #325