Check if Delivery* fields are set

This patch won't always work as expected when delivery addresses are enabled, for example if street address 2 is left blank in the delivery address but is used in the billing address, the billing address value will be incorrectly sent here.

The SagePay integration manual suggests that instead we should disable AVS/CV2 checks where there is no delivery information available, perhaps an option for the ApplyAVSCV2 value on the payment gateway settings form would work better?

I just found https://support.sagepay.com/forum/FindPost7909.aspx which suggests that copying the billing address to the delivery address is indeed okay for non-deliverable items. I think the patch should test if the checkout delivery details pane is enabled when deciding which fields to send.

Hi,
I will look into this tomorrow and post an updated patch.
Shouldn't we check_plain all the fields before sending them, or they are already sanitized ?

There is also a uc_order_is_shippable() function which sounds better and more reliable than checking for the delivery pane.

I don't think check_plain is needed as the data is not being shown to users, but POSTed to another server, and so should not be HTML encoded; doing this may cause problems if characters such as &, ", < or > are included in the data.

New patch that uses uc_order_is_shippable.

And this one without the additional space...

I have given a thought to this and read a little bit on the protocol. While using uc_order_is_shippable() looks like the most sensible way, I have checked in ubercart what conditions are used to decide whether or not the delivery pane should be shown, and in reality, there is none. It seems to be completely up to the site's admin to whether include those fields or not. While I can't think of any useful scenario in which a non-shippable item can have a delivery address, it's possible code-wise and on the other hand, I can think of a shipping-item not having a delivery address (say if you want to only deliver to the billing address).

Also I have looked at the protocol and about the relationship between the AVS/CV2 and with some delivery fields that make the other compulsory, but all of this should be perfectly handled properly configuring both sagepay and the site.

Said this, I believe that the best way to handle this is to just check for the data availability, and in that case, add it or not to the transaction array. Something like removing all the delivery fields from the array and adding the following below:

And doing that for each delivery field with the proper length limits as it's currently is.

Doing this, if no delivery info is present, no delivery will be send to protx and protx won't complain (unless you have AVS/CV2 set to force or you are using paypal express)

I can handle this directly so no need to submit a patch, but I'd like to hear what you think.

Hi hanoii,

I think you didn't read the link that longwave put in #2 :)
I'll just paste here the relevant information from that post:

For you all who have the patience to read through this thread, I had to call tech support to get an answer because Joe is the only guy manning these forums and is out of the country for a few days. So, here's the answer I got from support. They basically said, yeah, they are getting a lot of calls on 2.23 regarding the delivery fields and the case of selling intangibles like digital subscriptions. They said that their advice is to, yes, collect the billing information fields before sending on the transaction to them, but for the delivery columns, just carry over the billing fields into those fields just to not cause an issue in the system.

So it seems that the approach suggested in #7 might not work.

About uc_order_is_shippable: you are right, it won't work probably.

So, correct me if I'm wrong, but it seems that the first patch posted here is the one with the biggest chance of success (unfortunately).

Another idea would be to just check all the Delivery* fields and if one of them is missing then replace all of them with the Billing* fields.

Cheers,
Andrei

I haven't read that post, but I did now.

According to the protocol, delivery fields are optional. You are experiencing problems because the data is being sent to sagepay, but it's empty. With what I am suggesting, no delivery information will be sent whatsoever in your particular case and this, according to their protocol description has to work. Now if this doesn't work, I would agree that some extra measures should be taken, but I believe, from what I understand from the protocol and even to the post that it has to work. Now I can try that against the simulator and the test server but if you are currently working on it, what would happen if you indeed don't send any delivery field? Would SagePay server compalin? By that I don't mean sending them empty, but just not send them at all. Would the transaction go through?

a.=

Thanks for your suggestion.
I will try tomorrow to send requests to SagePAy without the Delivery* fields in the request.
I'll post back asap.

Cheers

No, it seems not working :(
When sending the request without including the Delivery* fields this is what I get back:
"The DeliverySurname field is missing from the POST."

This is what I found in testing too, the Delivery* fields are required if AVS/CV2 checks are switched on in your account (and this is enabled by default in new accounts). This is noted on page 52 of the integration guide:

NB: If AVS/CV2 is ON for your account, and this is NOT a PayPal Express transaction (or if this is a PayPal Mark transaction), these fields become compulsory.

As for:

Another idea would be to just check all the Delivery* fields and if one of them is missing then replace all of them with the Billing* fields.

This won't work because of the DeliveryStreet2 example I mentioned above, this will not necessarily be filled in for all addresses. Maybe checking just DeliveryCountry would be enough?

@lonwave #12, DeliveryCountry sounds good enough - so if this field does not exist then just replace Delivery* with Billing*.
hanoii, what do you think about this ?

DeliveryStreet2 should be optional even when the other Delivery fields are mandatory. At least that's what I get from the documentation.

@andreiashu: One question, when you tried not sending the delivery fields and receiving the error, how is your AVS/CV2 rule set on the protx account you are trying it against? was it OFF? And just in case it was not, what happens if you do switch it off?

I may do a few tests on my own and also comment on this.

If you do switch AVS/CV2 off, transactions go through successfully with the Delivery* fields missing. But if it's on then transactions will fail without those fields. As we're sending ApplyAVSCV2 as 0, we can't determine from the module itself whether the checks will be done or not.

That's all what I am saying. If you are not going to send a delivery address because you don't need it, then you can safely switch off AVS/CV2 in protx account, or am I missing something?

But now I give another thought of this. This might not be flexible enough, considering you use this module for the same vendor on different sites or even if you do sell both tangible and non-tangible goods in the same site.

I don't like the idea to just send the billing address in the delivery fields, as I don't think is flexible either. But I am thinking on something else.

I have committed a feature that let you have fine control over the Apply3dSecure field by using conditional actions (or workflow-ng on D5). Meaning that you can control what value that would be set on each transaction depending of this value (a client of mine didn't want 3dsecure to be applied to most lower amount transactions but on Maestro, it is mandatory).

I am thinking that a similar approach can be taking with the ApplyAVSCV2 field. The default would be to use what's set on sagepay account (which IMO it will be the most used scenario) but if a finer control of it is needed then this way is probable the most flexible one. You can use any of the conditions available in Ubercart to control how te set that value.

Thoughts? Too much?

I can still see a scenario where on the same site you want to sell both tangible and intangible goods, and you want AVS/CV2 fraud checking done on the delivery addresses of the tangible goods. Simply allowing admin control over ApplyAVSCV2 still doesn't solve this scenario (though this would still be a useful feature to add).

What we need to do in this issue is compensate for what is effectively a bug in the protocol, in that delivery addresses are required if you want fraud checks to be carried out, even if the items don't need a delivery address. I think that for this situation checking $order->delivery_country and if it isn't set sending the billing data as Delivery* is the best solution. This is backed up by the quote from SagePay in #8.

Oh wait, I see what you mean about allowing CA/workflow_ng to control that value. I'm not sure but I think that is really too complicated for this situation, most users will want the fraud checks switched on wherever possible (I'm not really sure why you would want to disable them!) and I think that adding rules to this would make things too complex for the store owner when initially setting up the module. I feel there's already enough configuration options as it is!

To try to keep everyone happy and cover all scenarios, how about:

- Add the workflow_ng/CA rules to let the store owner change ApplyAVSCV2 on a per-order basis if they need to.

- Add a checkbox to the configuration page labelled "Send billing address if no delivery address is available" with a note that "This is required by the SagePay protocol if AVS/CV2 fraud checks are enabled on orders without delivery addresses" or similar. This should be switched on by default so the module works out of the box with new SagePay accounts in stores that sell intangible items.

Maybe. CA/workflow_ng is not the happiest for regular users. This is generally setup by the developer for the client and once it's working, it's hardly ever changed. But it might be too much (although I am tempted to add it as I figured most of the logic out for the 3DSecure flag), but maybe too much now.

Anyway, to sum up, what you are suggesting is to check on deliverycountry, and, if not present, replace all delivery fields with the billing ones? Can the delivery country not be available on the form but so the other fields?

Delivery country can theoretically be disabled at /admin/store/settings/checkout/edit/fields but I'm not sure how that will work in practice, because the country code is used to determine which items to put in the delivery zone (state/county) dropdown. Would users disable the country field even if their store is country-specific? Otherwise I guess we could read the "required" checkboxes from that page and check the relevant delivery address fields before copying the billing address, maybe?

I just checked uc_paypal and it uses the following to decide whether to send the shipping address:
uc_order_is_shippable($order) && !empty($order->delivery_first_name)

And uc_google_checkout appears to only use uc_order_is_shippable() when deciding to send shipping details.

I am also thinking that we can actually use what andreiashu suggested in the first place. Checking the shipping pane by using:

Haven't checked it but it should work, any caveat? Otherwise the other options also seem fine.

I discovered it's not as simple as that, if the delivery pane is enabled but the cart contains no shippable items, the pane isn't actually shown. This check is done in uc_cart_checkout_form(), at around line 143 of uc_cart.pages.inc.

I think uc_order_is_shippable() will be best, it's how the checkout itself and at least two payment modules decide whether an order has a delivery address or not.

Implementing the uc_paypal method described in #23 seems the right thing to do for now.

@hanoii @#15: I just saw that message but now I'm not at work anymore so I can't test it. But even if you don't get errors from Protx when you don't send the Delivery* fields (if AVS/CV2 is off) it doesn't really matter... First of all our code cannot check if the AVS/CV2 is off (you can just send a dummy request and parse the response, but it doesn't seems really stable) and second: you may have products that are deliverable and products that aren't on the same site instance.

All in all, in my opinion, the simplest and most straight forward solution seems to be the one described by longwave in comment #23.
Also I think this should be committed sooner than later: this is a blocker issue for sites that sell products that are not deliverable.

@andreiashu: I wasn't saying that the module would have to know if the checks are off. It's a job for the human integrator to properly set both things up so they can work together properly. But I agree with longwave that those check would be wanted ON.

So, I'd think #25 is just fine, just using uc_order_is_shippable() and copying the billing data to the delivery one if that condition proves false.

If someones is willing to submit a patch of this fix only, please do so against the -dev version, or even better, CVS. But there's no need really. I can take care of this one as I am planning on releasing a new version pretty soon and I'll include this one. Probably soon after next Tuesday.

@hanoii: patch in #6 should apply to the dev (with some offset) :)
I'll be out of office too until next Tuesday.

Cheers

Committed with a small tweak. Thanks. Please try the -dev version of today if you have time to check if deliveryfields are properly sent in your case.

Hi hanoii,
I'll test this out by the end of this week (big project to finish here :).
I'll report back with my findings.

Thanks

It works. Just tested it.
Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.