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

fix(finance): correct Discover card number format #3336

Merged
merged 2 commits into from
Dec 21, 2024

Conversation

julsql
Copy link
Contributor

@julsql julsql commented Dec 14, 2024

Partially fixes #2350

This PR addresses an issue with the Discover card number format in the finance module. The previous implementation allowed invalid card number formats that did not align with Discover's official specifications. Discover card number can't have 20-digit card numbers.
https://www.discoverglobalnetwork.com/content/dam/discover/en_us/dgn/docs/IPP-VAR-Enabler-Compliance.pdf

Changes Introduced:

  • Removed incorrect 20-digit Discover card numbers.

No breaking changes are introduced.

@julsql julsql requested a review from a team as a code owner December 14, 2024 22:47
Copy link

netlify bot commented Dec 14, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 051c4de
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/675e0afd55ea8e0008548b09
😎 Deploy Preview https://deploy-preview-3336.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 14, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 30d7a71
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/67628045acf6b20008a24f79
😎 Deploy Preview https://deploy-preview-3336.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent m: finance Something is referring to the finance module labels Dec 14, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Dec 14, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Dec 14, 2024

Have you checked the other locales as well?

@julsql julsql force-pushed the correct-discover-card-format branch from ad69bfd to 794cf32 Compare December 14, 2024 23:33
@julsql
Copy link
Contributor Author

julsql commented Dec 14, 2024

Have you checked the other locales as well?

You're right, I've missed one. I've just added it.

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (e0016ad) to head (30d7a71).
Report is 1 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3336   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files        2811     2811           
  Lines      217012   217006    -6     
  Branches      939      937    -2     
=======================================
- Hits       216959   216953    -6     
  Misses         53       53           
Files with missing lines Coverage Δ
src/locales/el/finance/credit_card/discover.ts 100.00% <ø> (ø)
src/locales/en/finance/credit_card/discover.ts 100.00% <ø> (ø)

@ST-DDT ST-DDT requested a review from a team December 15, 2024 08:14
@ST-DDT
Copy link
Member

ST-DDT commented Dec 15, 2024

Could you please link the official spec for the discover card numbers for easier verification of your changes?

@ST-DDT
Copy link
Member

ST-DDT commented Dec 15, 2024

If possible, please also add a test that uses validatorjs' isCreditcard with provider discover.

const discover = faker.finance.creditCardNumber('discover');
expect(discover).toSatisfy(luhnCheck);

https://github.com/validatorjs/validator.js/blob/86911d817fbcd177c295b13d7ca0fdff87341191/src/lib/isCreditCard.js#L7

Is the validation pattern there correct?

@matthewmayer
Copy link
Contributor

If possible, please also add a test that uses validatorjs' isCreditcard with provider discover.

const discover = faker.finance.creditCardNumber('discover');
expect(discover).toSatisfy(luhnCheck);

https://github.com/validatorjs/validator.js/blob/86911d817fbcd177c295b13d7ca0fdff87341191/src/lib/isCreditCard.js#L7

Is the validation pattern there correct?

When i previously tested this validator.js incorrectly disallows the 64[4-9]#-####-####-###L pattern

#2350 (comment)

@julsql
Copy link
Contributor Author

julsql commented Dec 17, 2024

If possible, please also add a test that uses validatorjs' isCreditcard with provider discover.

const discover = faker.finance.creditCardNumber('discover');
expect(discover).toSatisfy(luhnCheck);

https://github.com/validatorjs/validator.js/blob/86911d817fbcd177c295b13d7ca0fdff87341191/src/lib/isCreditCard.js#L7
Is the validation pattern there correct?

When i previously tested this validator.js incorrectly disallows the 64[4-9]#-####-####-###L pattern

#2350 (comment)

I've tried with validator.js but indeed it throws an error for the 64[4-9]#-####-####-###L pattern. After checking on the web the pattern seems correct (https://neapay.com/post/bin-list-range-for-mastercard-visa-amex-diners-discover-jcb-cup_94.html)

Should I add a test using validatorjs' isCreditcard and remove the 64[4-9]#-####-####-###L pattern as well? Or I leave it that way and wait for validator.js to be corrected?

@ST-DDT
Copy link
Member

ST-DDT commented Dec 17, 2024

Or I leave it that way and wait for validator.js to be corrected?

We can omit the check for now and add it once validatorjs fixes their pattern.
Is there a PR for that fix in validatorjs?

@julsql
Copy link
Contributor Author

julsql commented Dec 17, 2024

We can omit the check for now and add it once validatorjs fixes their pattern. Is there a PR for that fix in validatorjs?

I looked for an existing PR in validatorjs addressing this issue, but I couldn't find any.

@ST-DDT
Copy link
Member

ST-DDT commented Dec 18, 2024

looked for an existing PR in validatorjs addressing this issue, but I couldn't find any.

If you would like to, you could create it when you have time.

@ST-DDT ST-DDT requested a review from a team December 18, 2024 08:04
@matthewmayer
Copy link
Contributor

looked for an existing PR in validatorjs addressing this issue, but I couldn't find any.

If you would like to, you could create it when you have time.

I'm not sure the official info from Discover provides enough info for a canonical validation regex. It shows the correct prefixes but not what lengths are valid for each prefix?

@julsql
Copy link
Contributor Author

julsql commented Dec 18, 2024

I'm not sure the official info from Discover provides enough info for a canonical validation regex. It shows the correct prefixes but not what lengths are valid for each prefix?

Based on the official documentation from Discover, we know that the valid lengths for cards with the prefix 644-9 are between 16 and 19 digits. I think it should be enough to build the regex in validator.js? I will open an issue when I will have time.

@matthewmayer
Copy link
Contributor

Right I mean it's fine if faker.js only generates a small subset of valid Discover card numbers. But validator.js should ideally validate all valid card numbers and I'm not sure we have detailed enough info to create a PR on validator.js to do that.

@ST-DDT ST-DDT added this pull request to the merge queue Dec 21, 2024
Merged via the queue into faker-js:next with commit 69c0063 Dec 21, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: finance Something is referring to the finance module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify all credit card issuer patterns are valid with validator.js
3 participants