Integrating Bank Account information to save within CIM

Thanks for the patch! That's really awesome! We were planning on adding support for bank account payment methods eventually, but there wasn't an immediate need for it in our projects. Thanks for taking the initiative on this!

I'll mark this as "Needs Review" for now. I'm traveling right now, so I don't have time to look at it in detail, but I will when I get back. It's a pretty large patch, so I might try to break it up a bit, if possible. Not sure until I can look at it closer.

Thanks again!

Alright, finally got around to working on this. Looks good for the most part. I made a couple of changes (outlined below for discussion) and attached a new patch for testing/review. This still needs some work before we can pull it in, but we're close I think...

1. Squashed the 31 separate commits into one patch file for easier review. I might commit the changes in chunks, but for now, while we're reviewing it in this thread it will be easier to read one patch file.
2. Ran the Coder module's code review, and cleaned up the whitespace and coding standards of the new code. I also added a few more comments here and there (your plentiful comments are much appreciated, btw).
3. Changed the menu callback for the new Ajax response so that it doesn't have any dependency on the authnet_user module. The authnet_user module depends on the authnet_ui, not vice versa. To do this, I changed the path of the menu item from 'user/%user/billing/form-type' to 'authnet/ajax/payment-type' and I got rid of the access callback (as far as I can tell, it isn't necessary). I also added a hook_forms() implementation to authnet_user to tell Drupal where to find the form build function, so that the Ajax callback works (this I committed already, so it's not in the patch).
4. Reworked some of the code in authnet_ui_payment_profile_form(). I made the payment type field (dropdown) into a radio field, that only gets displayed if a new payment profile is being created (users shouldn't be allowed to change the type of an existing payment profile, I don't think). Also, the patch had changed the fields 'customer_profile_id' and 'payment_profile_id' from type 'value' to 'hidden'. We are trying to keep those values in the background as much as possible, as a security precaution. By using type 'value', it still get's passed to the submit function in $form_state['values'], but it doesn't show up in the HTML printed in the form (like a 'hidden' type field).
5. Removed the $show_values variable and related code from the credit card and bank account forms, because they're redundant. The default value is the current value, so there's no need to show it twice.

There's probably some other things I forgot to mention here, but those are the big ones I think.

There are still some things that need to be done:

• We should only store the last 4 digits of the routing and account numbers in the database. This sort of happens naturally, because the database records use what Authorize.net returns after saving a bank account, which is in the form: XXXX1234. But I would rather just trim it to the 4 digits, like we're doing with the CC number. The database column names should probably change, too, that means (maybe "bank_routing_last_four" and "bank_account_last_four"). This also means reworking the theme function a bit (I added a @todo there).
• On a related note: there's currently a bug when updating bank accounts that have already been saved. The account number does not get updated in the database record, so the old account number is displayed when the payment profile is themed. I haven't looked closely, but this probably also means that the other fields aren't being updated either.
• I'm inclined to say we should add a setting to the module that allows the site admin to turn bank accounts on/off. I'm not sure that everyone would want bank account support on their site, so maybe it should be optional. I'm open to discussion on that.
• Do you know what the "eCheck Type" is used for? We're not setting it anywhere, nor is there a field for it, but we have a database table for it. I know it's one of the fields that Authorize.net accepts with their API, but what is it exactly? I'll need to look deeper to see, but if you have any ideas please share.

Please update the module to the latest dev release before applying this new patch. It depends on some recent commits.

I accidentally committed some pieces of this issue with a recent commit, so I reverted them in the dev release and rolled them into the new patch which is attached.

All the stuff I said in #3 still applies to this patch.

One more small tweak: changed the language of an empty payment profile table from 'You don't have any saved cards. Click "Add a credit card" above to save a new one.' to 'You don't have any saved payment methods. Click "Add a payment method" above to save a new one.'

See comment #3 above for stuff that still needs to be done.

Thanks for keeping this going @pfrilling. I haven't had much time to dedicate to this recently either. Such is life (and volunteer development).

I applied your patch and made a couple of changes:

• I updated the patch to work with the latest 6.x-1.x branch (there was a conflict with the commit I made in: #1894914: Can't select current year in card expiration).
• Merged both hook_update_n() functions into one. We haven't released any of this code yet, so there's no need to maintain multiple updates that all pertain to the same patch.
• Changed the storage size of the bank account and routing numbers to 5, since we're only storing the last four digits. (Should probably be 4, but we're using 5 for the credit card digits already, so I set this to 5 for consistency. We can address that at another time.)
• I moved the settings for allowing user to add credit cards and bank accounts into the authnet_user module. I created a new menu callback for them at /admin/settings/authnet/user. The form alterations to remove options are performed in the authnet_user_payment_profile_form() function. I did this to keep dependencies separate. I thought about putting them in authnet_ui... but ended up going with authnet_user. I also added the necessary variable_del() calls to the uninstall hook for proper cleanup.
• Similarly, I see that you added options for selecting which card and bank account types are available. I split that out into a separate issue with a separate patch: #1900244: Allow admins to specify which card and bank types to allow (depends on this patch).
• Cleaned up some whitespace that Git was complaining about.
• Added some more comments here and there (minor stuff).
• Changed "Payment information" to "Payment method information" to avoid any possible confusion with payments that have been made.

I did some initial testing of adding/editing/deleting bank accounts, and it seems to be working well. I haven't tried performing transactions on bank accounts yet, though, but based on the Authorize.net CIM API documentation, I think everything should work similarly to the existing credit card transactions, which we know are working.

If this all makes sense, I think this is ready to be committed. Let me know if you can think of anything else we need to consider.

Very minor update to make the patch work with some recent commits (added newlines at the ends of files, which caused some of these changes to create conflicts). Attached patch works against current 6.x-1.x.

Is there a version of this patch for the 7.x branch?

No, the patch is only for 6.x right now, but it would be worth trying it on the 7.x to see if it applies. The 6.x and 7.x are not very different, so there's a chance it will. Please let us know what you find! We'll have to port this to 7.x eventually anyway.

I've applied the changes from the patch in #10 to the 7.x branch, and it seems to be functioning well in my very limited testing. Attaching here in case anyone finds it useful.

My previous patch did not include the new files that were created in the 6.x version - please see the updated patch below.

Thanks lektro! Were there any major differences in the changes between the 6.x and 7.x patches? I will try to commit these soon...

The only major difference was in how Ajax is handled in D7, but even then it was only a matter of editing the callback.

D6 is end-of-life tomorrow, so if we do this we should get the 7.x patch in as a first priority.