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

Add bp_plus card type and custom validations #4559

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

dsmcclain
Copy link
Contributor

BP Plus cards come in two formats, both 17 digits long, and both containing two spaces:

  • 7050N NNNNNNNNN NNN
  • 705N NNNNNNNN NNNNN

The first format (7050N) should pass a luhn check once the spaces are removed.

The second format (705N) contains a luhn check digit as the final digit. This PR adds an implementation of the luhn algorithm that accounts for this check digit (details on the algorithm can be found here).

The PR also includes a regex to identify the bp_plus card type, and modifications to the valid_card_number_characters? method to accommodate the spaces in the numbers.

SER-267

Unit Tests:
5295 tests, 76287 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Rubocop:
746 files inspected, no offenses detected


10 - (sum % 10) == check_digit.to_i
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, we could rewrite valid_luhn? to switch based on whether we are passing a check digit or not. That would look something like:

def valid_luhn?(numbers, check_digit = nil)
  sum = 0

  doubler = check_digit.present?

  numbers.reverse.bytes.each do |bytes|
    doubler ? sum += BYTES_TO_DIGITS_DOUBLED[bytes] : sum += BYTES_TO_DIGITS[bytes]
    doubler = !doubler
  end

  if check_digit.present?
    10 - (sum % 10) == check_digit.to_i
  else
    sum % 10 == 0
  end
end

I didn't do this because it's probably more prudent to avoid refactoring the valid_luhn? method which almost every card number hits, and it doesn't necessarily make things more clear. But I'm open to differing opinions on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way on this. I think the refactor example you gave would be easy enough to do and we have the tests coverage to verify implementation, so I think the risk is relatively low.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to leave it as-is out of an abundance of caution.

Copy link
Contributor

@naashton naashton left a comment

Choose a reason for hiding this comment

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

I think this looks good. I'll leave it up to you if you want to condense this down to one method or keep the check_digit as it's own method. Either way, it looks good.


10 - (sum % 10) == check_digit.to_i
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way on this. I think the refactor example you gave would be easy enough to do and we have the tests coverage to verify implementation, so I think the risk is relatively low.

@dsmcclain dsmcclain force-pushed the SER-267_add_bp_plus_card_and_custom_validations branch from c4e5847 to ec3ef34 Compare September 1, 2022 16:49
SER-267

Unit Tests:
5296 tests, 76298 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Rubocop:
746 files inspected, no offenses detected
@dsmcclain dsmcclain force-pushed the SER-267_add_bp_plus_card_and_custom_validations branch from ec3ef34 to 28e06f6 Compare September 1, 2022 17:55
@dsmcclain dsmcclain merged commit 28e06f6 into master Sep 1, 2022
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.

None yet

2 participants