Skip to content

Add Dinersclub 309 prefix validation#57

Open
ghost wants to merge 4 commits intojondavidjohn:masterfrom
frappacchio:master
Open

Add Dinersclub 309 prefix validation#57
ghost wants to merge 4 commits intojondavidjohn:masterfrom
frappacchio:master

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Dec 4, 2018

It solves the issue #56

according to this document https://en.wikipedia.org/wiki/Payment_card_number Dinersclub International could starts with 309
Thanks,
Francesco

@ddayguerrero
Copy link
Copy Markdown
Collaborator

Hi @FrancescoColonna! This is good! We've recently made changes to our BIN patterns and documented everything under CARDRULES.md. Do you mind adding tests in test/validateCardNumber_spec.coffee and possible source associated to the Wikipedia page?

@ghost
Copy link
Copy Markdown
Author

ghost commented Dec 5, 2018

Hi @ddayguerrero
i've added a basic test in test/validateCardNumber_spec.coffee and a reference to Wikipedia in CARDRULES.md
Please let me know if i can help in some other ways.
Thanks for your job!
Francesco

Comment thread dist/payform.js Outdated
@ddayguerrero
Copy link
Copy Markdown
Collaborator

Looks good to me! Before we move on, can we omit all generated distribution files from the list of staged changes? (dist/payform.js and dist/payform.min.js)

@frappacchio
Copy link
Copy Markdown

Hi @ddayguerrero i've removed dist/*.js files from staged changes.
Please let me know if everything it's ok.
Thanks,
Francesco

@ddayguerrero
Copy link
Copy Markdown
Collaborator

Sorry, I wasn't clear earlier. What I meant is to keep the files on disk, but remove the changes from the git change log. Something along the lines of doing git rm --cached

We should end up with 3 files changed:

src/payform.coffee
test/validateCardNumber_spec.coffee
CARDRULES.md

I truly appreciate your time and effort in creating this PR, but it is only best that we follow the guidelines shared by @jondavidjohn

@ddayguerrero
Copy link
Copy Markdown
Collaborator

Will close as soon #59 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants