Update CC brand detection regex #1477
Conversation
The existing hash was incomplete, missing some brands. It was also missing MasterCard's new (October 2016) BIN range (222100-272099). This hash was originally added in spree/spree#3918 and has not been changed since. I don't know the original source, but I would rather trust the Hash taken from ActiveMerchant. One notable difference is that this will treat maestro and mastercard as separate brands. NB: For most stores, this code will not be run, and instead jquery.payment on the frontend will be responsible for providing the credit card brand.
The removal of whitespace is already done (more completely) in number=.
The existing code returned from within a find, and if the find ever succeeded, would return '' because nil (the return of the find) .to_s is ''. This works, but is difficult to read. I prefer being explicit about the return value being an empty string if there isn't an early return.
jhawthorn
force-pushed
the
credit_card_regex
branch
from
7513f09 to
735a8e5
Compare
| @@ -97,8 +105,10 @@ def set_last_digits | |||
| # @return [String] the credit card type if it can be determined from the | |||
| # number, otherwise the empty string | |||
| def try_type_from_number | |||
| numbers = number.delete(' ') if number | |||
| CARD_TYPES.find{ |type, pattern| return type.to_s if numbers =~ pattern }.to_s | |||
| CARD_TYPES.each do |type, pattern| | |||
There was a problem hiding this comment.
Wouldn't detect have the same effect?
Also, I think you don't have to cast strings on type Your hash keys are all strings already.
There was a problem hiding this comment.
Oh, nasty. You have the early return because you return empty string below if a number matches.
I would prefer .select {...} || '', but it's on you to decide
There was a problem hiding this comment.
.detect would return us (for example) ['visa', /^4\d{12}(\d{3})?(\d{3})?$/] so the code would need to be something like
card_type_pair = CARD_TYPES.detect { |type, pattern| numbers =~ pattern }
if card_type_pair
card_type_pair[0]
else
''
end
I think the return is more clear.
I will remove the .to_s
| discover: /^6(?:011|5[0-9]{2})[0-9]{12}$/, | ||
| jcb: /^(?:2131|1800|35\d{3})\d{11}$/ | ||
| } | ||
| 'visa' => /^4\d{12}(\d{3})?(\d{3})?$/, |
There was a problem hiding this comment.
Why not take the Regex from ActiveMerchant instead of copy it over. This way we have to always have to have an eye on this.
There was a problem hiding this comment.
I'm pro reducing and removing our reliance on active_merchant in core, so I'm 👍 on copying it in (it also isn't something that changes frequently).
There was a problem hiding this comment.
reducing and removing our reliance on active_merchant
Ditto
| discover: /^6(?:011|5[0-9]{2})[0-9]{12}$/, | ||
| jcb: /^(?:2131|1800|35\d{3})\d{11}$/ | ||
| } | ||
| 'visa' => /^4\d{12}(\d{3})?(\d{3})?$/, |
There was a problem hiding this comment.
I'm pro reducing and removing our reliance on active_merchant in core, so I'm 👍 on copying it in (it also isn't something that changes frequently).
Our CARD_TYPES hash now uses strings as keys so we no longer need to stringify them.
The existing hash was incomplete, missing some brands. It was also missing MasterCard's new (October 2016) BIN range (222100-272099).
This hash was originally added in spree/spree#3918 and has not been changed since. I don't know the original source, but I would rather trust the Hash taken from ActiveMerchant.
One notable difference is that this will treat maestro and MasterCard's as separate brands.
NB: For most stores, this code will not be run, and instead
jqyery.paymenton the frontend will be responsible for providing the credit card brand (already updated in #608 and understands MasterCard's new BIN. Thanks @mtomov).