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

Issue #2790551 by steveoliver, bojanz, borisson_: Implement a JS libr… #663

Open
wants to merge 1 commit into
base: 8.x-2.x
Choose a base branch
from

Conversation

mglaman
Copy link
Contributor

@mglaman mglaman commented Mar 4, 2017

…ary for the credit card form

'0604',
'6390'
],
lenghts: [12, 13, 14, 15, 16, 17, 18, 19]

Choose a reason for hiding this comment

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

Should be lengths

var MAESTRO = 'maestro';

types[VISA] = {
niceType: 'Visa',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why niceType and not label?
And why aren't the labels passed through Drupal.t()?

niceType: 'Visa',
type: VISA,
pattern: ['4'],
gaps: [4, 8, 12],
Copy link
Contributor

Choose a reason for hiding this comment

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

Gaps are not used anywhere, and don't exist on the PHP side, let's remove them.

types[MASTERCARD] = {
niceType: 'MasterCard',
type: MASTERCARD,
pattern: ['51-55', '222100-272099'],
Copy link
Contributor

Choose a reason for hiding this comment

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

"pattern" should be "prefixes" to match the PHP code.

var AMEX = 'amex';
var DINERSCLUB = 'dinersclub';
var DISCOVER = 'discover';
var MAESTRO = 'maestro';
Copy link
Contributor

Choose a reason for hiding this comment

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

We're missing JCB and UnionPay.

* @return {object|boolean}
*/
var detectType = function (number) {
// Loop over all available types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments that retell what the code is doing ("Loop over all the available types") aren't useful, let's remove them.


// Loop over all patterns in the type.
for (var i in type.pattern) {
var pattern = type.pattern[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we introduce a matchPrefix() helper like we did in PHP?
On the other hand, validatePrefix() could probably be inlined.

var range;
exploded_pattern = pattern.split('-');

while (exploded_pattern[0] <= exploded_pattern[1]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels overly complex.
We reimplementing the following code:

      list($start, $end) = explode('-', $prefix);
      $number = substr($number, 0, strlen($start));
      return $number >= $start && $number <= $end;

}
}

ValidationDiv.append('CC is of type: ' + type.niceType);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like debug code?

@bojanz
Copy link
Contributor

bojanz commented Feb 7, 2018

This PR needs work. We basically want a JS version of our CreditCard.php code.
In comparison to it, this JS code lacks certain credit card types (JCB, UnionPay) and Luhn validation.

Compared to 11 months ago, we have a real world need for the code in Commerce AuthNet: https://www.drupal.org/project/commerce_authnet/issues/2932486 which should help us ensure that the merged code is solid.

Also, ouch, we have no way to write JS unit tests yet. Issue to follow is https://www.drupal.org/project/drupal/issues/2815199

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