Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Donations App #1

Closed
wants to merge 66 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

tinio commented May 19, 2011

NOTE: Until this notice is removed, this pull request should not be merged. This is an incomplete branch.

@tswicegood tswicegood commented on the diff Jun 7, 2011

armstrong/apps/donations/features/steps.py
@@ -0,0 +1,68 @@
@tswicegood

tswicegood Jun 7, 2011

Owner

Gah -- lettuce does some weird stuff with its output. Two things

  1. Can you do the standard pep8 spacing on these?
  2. Kill the Given|When|Then|And at the beginning of each of the patterns. No need to tie each function to a particular type of step. Given the authorize.net processor is used and When the authorize.net processor is used should almost certainly be identical.
@tinio

tinio Jun 7, 2011

Contributor

Great, I appreciate all of the suggestions and will incorporate them on next commit.

@tswicegood tswicegood commented on an outdated diff Jun 7, 2011

armstrong/apps/donations/features/onetime.feature
@@ -0,0 +1,47 @@
+Feature: One Time Donations
+ In order to support local journalism
+ As a one time donor
+ I want to make a monetary donation online
+
+ Scenario: Successful Credit Card Donation
+ Given I have a one time donation form
+ And specify a donation amount
+ And provide the following personal information:
+ | first_name | last_name | email |
+ | Aurelio | Tinio | atinio@armstrongcms.org |
+ And provide the following billing information:
@tswicegood

tswicegood Jun 7, 2011

Owner

Using tables here might be a bit of overkill. You could specify this as:

And "5105105105105100" is provided as the credit card number
And "05/2013" is provided as the expiration date

Or, better yet:

And a valid testable credit card number is supplied
And a future date is provided as the expiration date

The # and expiration date don't really provide any value here other than they need to be valid and the date needs to be in the future. Using more generic language captures that without us having to change the code in a few months or years when the card #s change or we get hit June 2013.

@tswicegood tswicegood commented on an outdated diff Jun 7, 2011

armstrong/apps/donations/features/onetime.feature
@@ -0,0 +1,47 @@
+Feature: One Time Donations
+ In order to support local journalism
+ As a one time donor
+ I want to make a monetary donation online
+
+ Scenario: Successful Credit Card Donation
+ Given I have a one time donation form
+ And specify a donation amount
+ And provide the following personal information:
+ | first_name | last_name | email |
+ | Aurelio | Tinio | atinio@armstrongcms.org |
@tswicegood

tswicegood Jun 7, 2011

Owner

Let's use Alice and Bob style names with example.com to make sure that the tests convey that the name here is not unique. Like the note below, this could probably be done in one line:

And Bob fills out his name and email address
Owner

tswicegood commented Aug 8, 2011

Where does this PR stand?

Contributor

tinio commented Aug 8, 2011

Unfortunately hasn't been touched in a while, hopefully get back on it this week.

@tswicegood tswicegood and 1 other commented on an outdated diff Aug 29, 2011

armstrong/apps/donations/fields/__init__.py
@@ -0,0 +1,59 @@
+states = (
@tswicegood

tswicegood Aug 29, 2011

Owner

Any reason not to use the django.contrib.localflavor version of this?

@tinio

tinio Aug 29, 2011

Contributor

nope, i'll change now.

Owner

tswicegood commented Aug 29, 2011

The specs currently fail: https://gist.github.com/77d2b7e2920407ac74ce

Sorry for the crappy output, Lettuce is a POS.

@tswicegood tswicegood and 1 other commented on an outdated diff Aug 29, 2011

fabfile.py
@@ -0,0 +1,20 @@
+from armstrong.dev.tasks import *
+
+from local_settings import AUTHORIZE_LOGIN, AUTHORIZE_KEY
@tswicegood

tswicegood Aug 29, 2011

Owner

Instead of putting these in the local_settings module, let's sign up for a dummy test account here and just check it in the credentials for it. You can throw the username and password in passpack.

@tinio

tinio Aug 30, 2011

Contributor

@tswicegood ok, I've gone ahead and just added test account credentials into the settings found in the fabfile. Associated authorize.net account is on passpack.

@tswicegood tswicegood and 1 other commented on an outdated diff Aug 30, 2011

armstrong/apps/donations/backends.py
+from datetime import datetime
+
+from django.conf import settings
+
+from authorize import aim, arb
+
+from armstrong.utils.backends import DID_NOT_HANDLE
+
+
+class PaymentBackend(object):
+ """
+ Processes payment by specified amount
+ """
+
+ def process(self, amount, **kwargs):
+ assert amount, 'A payment amount must be specified.'
@tswicegood

tswicegood Aug 30, 2011

Owner

What if you wanted to use process(0) for some reason?

@tinio

tinio Aug 31, 2011

Contributor

Alright, I've gone ahead and explicitly checked for a numeric (int, long, or float) value here instead.

@tswicegood tswicegood and 1 other commented on an outdated diff Aug 30, 2011

armstrong/apps/donations/backends.py
+from authorize import aim, arb
+
+from armstrong.utils.backends import DID_NOT_HANDLE
+
+
+class PaymentBackend(object):
+ """
+ Processes payment by specified amount
+ """
+
+ def process(self, amount, **kwargs):
+ assert amount, 'A payment amount must be specified.'
+ return DID_NOT_HANDLE
+
+
+class FakeCCBackend(PaymentBackend):
@tswicegood

tswicegood Aug 30, 2011

Owner

This should probably be in a donations_support app instead of part of its public API just to make sure someone doesn't accidentally use it.

@tinio

tinio Aug 30, 2011

Contributor

Ok, FakeCCBackend now moved to donations_support.

@tswicegood tswicegood and 1 other commented on an outdated diff Aug 30, 2011

armstrong/apps/donations/models.py
@@ -0,0 +1 @@
+pass
@tswicegood

tswicegood Aug 30, 2011

Owner

This convention isn't used anywhere else in Armstrong, we just include an empty models.py.

@tinio

tinio Aug 30, 2011

Contributor

Ok, changed to an empty models.py.

@tswicegood tswicegood and 1 other commented on an outdated diff Aug 30, 2011

armstrong/apps/donations/fields/creditcard.py
+ digit = digit * 2
+ if digit > 9:
+ digit = digit - 9
+
+ sum = sum + digit
+
+ return ((sum % 10) == 0)
+
+# Regex for valid card numbers
+CC_PATTERNS = {
+ 'mastercard': '^5[12345]([0-9]{14})$',
+ 'visa': '^4([0-9]{12,15})$',
+}
+
+
+def ValidateCharacters(number):
@tswicegood

tswicegood Aug 30, 2011

Owner

This needs to be pep8'ified.

@tinio

tinio Aug 31, 2011

Contributor

Ok, I see what you mean. I've gone ahead and removed the use of camelCase in the function names.

Owner

tswicegood commented Aug 30, 2011

It currently looks the Authorize backend is there, but nothing other than the tests are talking to it. That's still something to be wired up I take it?

Also, we'll need to trigger a few signals for actions about donations being received. Maybe donation, donation_successful, donation_failed, or something like that? We'll need these for the CRM module. Not something that has to be in this PR, but something to be thinking about.

@Jbonnett Jbonnett commented on the diff Sep 9, 2011

armstrong/apps/donations/backends.py
+class PaymentBackend(object):
+ """
+ Processes payment by specified amount
+ """
+
+ def process(self, amount, **kwargs):
+ assert isinstance(amount, int) or isinstance(amount, long) or \
+ isinstance(amount, float), 'A numeric amount must be specified.'
+ return DID_NOT_HANDLE
+
+
+class AuthorizeNetBackend(PaymentBackend):
+ """
+ Authorize.net payment backend
+ """
+ def __init__(self, settings=settings, onetime_api_class=aim.Api,
@Jbonnett

Jbonnett Sep 9, 2011

Owner

Why are you passing settings as a argument to the init?

@Jbonnett Jbonnett and 1 other commented on an outdated diff Sep 9, 2011

armstrong/apps/donations/backends.py
+ """
+ Authorize.net payment backend
+ """
+ def __init__(self, settings=settings, onetime_api_class=aim.Api,
+ recurring_api_class=arb.Api):
+ self.settings = settings
+ self.onetime_api_class = onetime_api_class
+ self.recurring_api_class = recurring_api_class
+ self.response = None
+
+ def process(self, amount, credit_card, exp_month, exp_year,
+ is_recurring=False, interval='monthly', occurrences=12,
+ start_date=datetime.today(), first_name=u'', last_name=u'',
+ **kwargs):
+ super(AuthorizeNetBackend, self).process(amount, **kwargs)
+ if not is_recurring:
@Jbonnett

Jbonnett Sep 9, 2011

Owner

totally a style thing, but it might be easier to read this if the top if: else just called two different functions. It doesn't look like they are currently sharing any logic.

@tinio

tinio Sep 9, 2011

Contributor

point taken on clarity, but just adding comments and a docstring instead.

@Jbonnett Jbonnett commented on the diff Sep 9, 2011

armstrong/apps/donations/fields/creditcard.py
+ """ A forms field for a creditcard number """
+ def clean(self, value):
+
+ value = forms.CharField.clean(self, value)
+ if not validate_characters(value):
+ raise forms.ValidationError('Can only contain numbers and spaces.')
+ value = strip_to_numbers(value)
+ if not validate_luhn_checksum(value):
+ raise forms.ValidationError('Not a valid credit card number.')
+
+ return value
+
+
+class CreditCardExpiryField(forms.CharField):
+ """ A forms field for a creditcard expiry date """
+ def clean(self, value):
@Jbonnett

Jbonnett Sep 9, 2011

Owner

does this clean handle current year + some month in the past?

for example, its currently 09/11, if i put in 07/11 it appears that it would accept it as valid.

Owner

tswicegood commented Sep 14, 2011

This has really come together. Good work. The only thing that I'm still slightly uneasy about is the lack of any models. Currently at the Tribune we work this way as well, but it's always bothered me that something could get lost in the shuffle with no record on the Django side showing what happened.

Obviously, we don't want to store credit card information and so forth, but having a record that a card was charged for X person/user in the amount of N on YY/MM/DD would be useful. Do you think it's worth trying to get that added in now, or should we leave it as a future enhancement?

@niran niran commented on the diff Sep 14, 2011

README.rst
@@ -2,6 +2,11 @@ armstrong.apps.donations
=========================
This package provides donations.
+In order to test make sure to create a local_settings.py which has the
@niran

niran Sep 14, 2011

local_settings.py isn't the way Armstrong handles local settings. Regardless, I think we'd be okay just mentioning which settings need to be set and letting the user decide where to put them.

@niran niran commented on an outdated diff Sep 14, 2011

armstrong/apps/donations/fields/creditcard.py
+ if m == None:
+ raise forms.ValidationError('Must be in the format MM/YY.\
+ i.e. "11/10" for Nov 2010.')
+
+ # Check that the month is 1-12
+ month = int(m.groups()[0])
+ if month < 1 or month > 12:
+ raise forms.ValidationError('Month must be in the range 1 - 12.')
+
+ # Check that the year is not too far into the future
+ year = int(m.groups()[1])
+ curr_year = datetime.datetime.now().year % 100
+ max_year = curr_year + 10
+ if year > max_year or year < curr_year:
+ raise forms.ValidationError('Year must be in the range %s - %s.'\
+ % (str(curr_year).zfill(2), str(max_year).zfill(2),))
@niran

niran Sep 14, 2011

I'm not sure if this check is worth it. Are you absolutely sure that credit cards don't get issued with expiration dates ten years in the future? Authorize.net or whatever backend is being used would raise an error for an invalid date regardless. Avoiding the API call doesn't seem like much of a win.

niran commented Sep 14, 2011

Looks good to me besides the comments I made. There are probably some useful views that could be added, but that can be done later while someone is actually putting this code to use.

On storing records of transactions, it sounds like logging to me. It'd be very simple to add a logger to that form and let the implementor hook up a log handler to do whatever they want. Make sure you pass the python versions of the pertinent data as the extra_data kwarg, not just a text summary of what happened.

Contributor

tinio commented Sep 15, 2011

@tswicegood Thanks. And completely agree with you in having models to save the basic info of the transaction (This is also what we do at TBC). I did create a donation-models branch (https://github.com/tinio/armstrong.apps.donations/tree/donation-models) last friday, with the intent on having that functionality as a separate PR, since the thread on this current one is getting fairly long.

Contributor

tinio commented Sep 15, 2011

@niran agreed. i've gone ahead and removed the 10 year future credit card check.

@dmclain dmclain commented on an outdated diff Sep 23, 2011

armstrong/apps/donations/fields/creditcard.py
+
+
+def validate_characters(number):
+ """ Checks to make sure string only contains valid characters """
+ return re.compile('^[0-9 ]*$').match(number) != None
+
+
+def strip_to_numbers(number):
+ """ remove spaces from the number """
+ if validate_characters(number):
+ result = ''
+ rx = re.compile('^[0-9]$')
+ for d in number:
+ if rx.match(d):
+ result += d
+ return result
@dmclain

dmclain Sep 23, 2011

Owner

There a reason this isn't the simpler::

def strip_to_numbers(number):
    if not validate_characters(number):
         raise Exception('Number has invalid digits')
     return number.replace(' ', '')
Contributor

tinio commented Nov 1, 2011

Alright, closing out this pull request and start a new one based of the 'merchant' branch that uses the django-merchant backend which works for multiple gateways.

@tinio tinio closed this Nov 1, 2011

crccheck added a commit that referenced this pull request Apr 23, 2012

Merge pull request #1 from tswicegood/fix-missing-kwargs
Updated to make sure kwargs and context are handled properly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment