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 is_valid fake method to enable beanstream credit cards #115

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Chris7
Copy link

@Chris7 Chris7 commented Nov 21, 2014

The credit card beanstream uses does not have an is_valid method so the validation method fails. This puts in a fake is_valid method if one doesn't exist (so if they ever do add a method it will work seamlessly).

@Chris7 Chris7 changed the title add id_valid fake method to enable beanstream credit cards add is_valid fake method to enable beanstream credit cards Nov 21, 2014
@tuxcanfly
Copy link
Member

Might be better to limit this to if validate, otherwise looks correct.

@Chris7
Copy link
Author

Chris7 commented Nov 22, 2014

Do you mean in the validate_card method?

@tuxcanfly
Copy link
Member

Well, something like:

     if validate:
        if not hasattr(card, 'is_valid'):
            # there is no is_valid on the beanstream credit card object
            def is_valid():
                return True
            card.is_valid = is_valid
         self.validate_card(card)

This way if someone passes validate=False, we can avoid the monkey patch.

@Chris7
Copy link
Author

Chris7 commented Nov 24, 2014

Here's a different approach I like better since it extends to all cards and avoids monkey patches as well as an extra lookup of is_valid.

@tuxcanfly
Copy link
Member

I think it's far better to just get rid of validation in convert_cc. validate_card expects a merchant.billing.utils.CreditCard, but we're passing it a beanstream.billing.CreditCard, which happens to have some attrs in common, which is asking for trouble. Ideally convert_cc should have been private but it's not a part of the Gateway interface so I think there's no harm in modifying it.

@Chris7
Copy link
Author

Chris7 commented Dec 1, 2014

You would know better. I agree that combining libraries with the same method signatures is dangerous as well, but is probably just one of those unfortunate choices that are outside of our control and we just have to deal with. However, because I'm unaware if other cards may have the same issue, I opted to add this logic here. It's a hard choice, for instance you can convert everything to a merchant CC and reject anything that isn't a subclass of it to avoid complications arising from common attributes, but then you'd have to keep up with god knows how many APIs that using their own CC instance handles.

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.

2 participants