Properly validate direct debit fields #57
Conversation
|
My ACHEFT field swapping is still working.
|
|
So why did you end up needing this? Sorry - I missed the beginning. |
|
Since you used !empty instead of isset, if bank_account_type wasn't set then the credit card fields were still flagged as required. If bank_account_type was submitted, then the rest of the debit fields would be accepted even if they were empty, because they were not part of the $billing_fields. The way I have it now, all debit fields validate properly as being required, empty fields are not accepted. This all came about because I've been working on making the Authorize.net eCheck processor 4.6 compatible. |
|
@KarinG I think if you submit your fields empty, that the names of the fields reported in the validation errors will not match yours. |
|
Let me try a submit/checkout - but we'll make this work somehow for both ACHEFT and eCheck. |
|
Getting a: |
|
That's exactly the kind of thing I was seeing when empty fields were being accepted; they get sent off to the payment processor empty and result in error. You mentioned renaming the direct billing fields based on currency; add each variant to the array of fields I merged with
I think that might do it. |
|
Meant to mention, if you undo the changes I made and submit empty fields, I'm pretty certain you would get the same result you're seeing now. |
The payment processor in this case would need to provide their own validation as needed.
The simple webform I set up to track down and fix the issue of why debit fields weren't validating has both a credit card and echeck processor. Only the appropriate fields are validated based on selected payment processor. I just added another commit to address payment processors that choose to rename the debit fields. |
|
Hold on I made a mistake I was using >$2000 and the specific iATS test account I'm using on this instance only allows up to $2000 (doh...) - hence the error message I got when trying to check out. So forget what I said above about breaking Credit Card checkout. Sorry about that! Question still remains to validate in webform or in payment processors - we've done it in the iATS extension - example:
I do understand the issue better now - it's currently possible to not fill in field marked with a red asterix and get away with it [not in the two fields the iATS extension adds the js to check the number of digits] - but in the others you can. I'll give this another QA/test this weekend. Please ping me @konadave if you don't get back to you by Monday. |
|
@KarinG so what did you find? |
|
@KarinG Have you had a chance yet to confirm this PR doesn't break iATS? |
|
Not yet - massive project deadlines and husband's ACL reconstruction got moved up and happened last Tuesday. It's not just re: iATS - it's a more general question - this needs to work for any payment processing wishing to rename its direct debit fields. Where should validation on direct debit fields happen - at the processor/extension/code level - or in webform. I'm leaning towards to extension/code level - as only there you know what they are called. But I will confirm if it breaks anything as it stands. Hopefully by tomorrow evening. |
| @@ -1562,16 +1562,24 @@ class wf_crm_webform_postprocess extends wf_crm_webform_base { | |||
| $billing_fields['credit_card_number'] = FALSE; | |||
| $billing_fields['cvv2'] = FALSE; | |||
| } | |||
| if (!empty($_POST['bank_account_type'])) { | |||
| if (isset($_POST['bank_account_type'])) { | |||
There was a problem hiding this comment.
I had followed the lead for the Stripe implementation: that's why it's an !empty
if (!empty($_POST['stripe_token'])) {
| 'bank_identification_number' => ts('Routing Number'), | ||
| 'bank_name' => ts('Bank Name'), | ||
| 'bank_account_type' => ts('Account Type'), | ||
| )); |
There was a problem hiding this comment.
This makes sense - same reasoning as above for CC
| } | ||
| foreach ($billing_fields as $field => $label) { | ||
| if (empty($_POST[$field]) && $label !== FALSE) { | ||
| if (isset($_POST[$field]) && empty($_POST[$field]) && $label !== FALSE) { |
There was a problem hiding this comment.
I don't think this is needed - I can transact iATS ACHEFT without this.
|
Ok - sorry that took some time to get back to. I've added some comments to the PR. |
|
This PR is now stale due to other related changes. |
|
This will be resolved by #100 for all payment processors (whether they be CC, direct debit or gold bars). I just need to verify 4.6 support on that one... |
|
Ok so should this PR be closed? |
No description provided.