SagePay discounts do not appear in Basket

[dignat]

It appear that if you include discounts in Basket XML , in the SagePay account control panel you are not able to see the discounts in the section Additional information

[judgej]

Some background to this: academe/SagePay#27

Discounts are not supported by omnipay-sagepay at all. I'm not sure if this issue was posted here on purpose or by accident, but it is worth noting as a feature that it would be great to support.

More generically, if the basket or cart can be made extendable for OmiPay 3.0, instead of hard-coded into omnipay-common as it is now, making it very difficult to extend, then we may have the possibility that each driver could extend the cart with its own specific features. This would be a great example of how traits could be used, so the application can extend the cart from multiple drivers (but also a great example of how traits can clash if we are not careful).

Sage Pay has a tonne of additional metadata you can add to the cart to support a number of different industries.

[dignat]

It was posted by accident, apologies

[judgej]

No problem - keep it here for now as it raises an interesting point. I really want to get a better cart implemented for 3.0, with much more flexibility for extending with new features when needed. I'll close this ticket when I get an issue more generic (for sagepay-common) written up.

[judgej]

If the basket contains negative item values, then it will now be listed in the basket as discount items. This is specific to omnipay-sagepay, and not a general OmniPay feature. Luckily no additional fields were needed to support this. This was implemented under PR #50 & #51. It needs a specific test to show the discounts working, so I'll leave this ticket open until the tests are done, and the XML Basket goes into an official release (2.3.0, I think).

[dignat]

It is a good idea. Thank you a lot.

[judgej]

We need to test multiple discounts. I have a hunch that may be an issue, as it looks like a new <discounts> element will be created for each discount added. I'll raise a new ticket if it fails during investigation.

[dignat]

Hi Jason,
you are right this what is producing with multi discounts
My item110.000.0010.001010</fi
xed>Discount5Discount

$discounts = $xml->addChild('discounts') should be outside the foreach loop.

if there is no negative prices with will not create the discounts Child.

here is the test with $discounts = $xml->addChild('discounts') outside of the loop
10Discount5DiscountMy item110.00</u
nitNetAmount>0.0010.0010

and with no negative value and $discounts = $xml->addChild('discounts') outside of the loop and new loop for discountItems

My item110.000.0010.001010</fi
xed>Discount5Discount

I will make a new pull request at the sagepay omnipay to fix this bug and will add a test for creating the basketXML with discount as well.

[judgej]

Cool, thanks for checking that. I'll help to get it into a release ASAP.

If the tests tested for 0, 1 and 2 discount items, that would make an awsome unit test. When there are no discount items, I suspect the discount XML element should be left out completely, rather than provided as an empty element.

The Sage Pay documentation states that the elements MUST be in the same order as documented, which to me strikes of an XML parser that does things in an odd way, e.g. string matching rather than proper XML parsing, so we need to give it an easy a time as we can ;-)

[judgej]

Since I am already set up for this, I'm happy to make these changes if it's a hassle at your end. Just say.

[dignat]

Thank you so much, but It is a good experience for me and I would love to do it. I am almost ready, I just need to clear a bit the messy code and put some comments.
Thank you once again sincerely.
Cheers,

[dignat]

Hi Jason,
I made a new pull request and added code which allow zero value item to be included in the basket, fixed the discount child creation, add tests for no discounts , one and many discounts, added fix for special characters in item name as addChild() does not escape & and triggers an error when is trying to create xml basket, added test for items with names including special characters. Please review it, and when you are happy with the investigation ad the changes.
Thank you in advance

[judgej]

That's awesome - I'll have a play.

[judgej]

Check out the first answer to this SO question:

http://stackoverflow.com/questions/552957/rationale-behind-simplexmlelements-handling-of-text-values-in-addchild-and-adda

The escaping is done for you if you assign a text value rather than add a child.

[dignat]

True, but the way BasketXML should be build , requires addChild and we gone need escaping function for odd names

[dignat]

We did tested the BasketXML and it generated error when trying to build the basket with nonescaped & that's why we decided that the best is that the driver should handle the escaping and the application just pass the information.

[judgej]

I agree it needs to be escaped, but implemented this way, < and > in the description would be escaped twice.

This example from the SO like above should work and provide full escaping (though not tried it):

$discount->addChild('description')->{0} = $discountItems->getName();

[dignat]

Yes, I just tried it and yes this is the solution. Shall I make the changes?
Thanks a lot again.

[judgej]

Yep - go ahead :-) The PR will automatically update when you push your change. Probably worth putting a test in with < > & and " in the description too.

This is something I've never tested: what character encoding does the XML basket accept? I would assume it is just ISO8859 and not UTF-8. But curious.

[dignat]

it is not very clear what standard BasketXML is using

[dignat]

Hi Jason,
I found another solution. I am just waiting for travic to finish
$item->description = $basketItem->getName()
Thank you so much.

[dignat]

Hi Jason,
It passed travis and it escapes very nicely the name of the item. I hope you will accept it and merged it.
Thank you so much.
Cheers.

[judgej]

Just for reference, this is all in PR #53

[delatbabel]

Can we close this because the PR is now merged?

[judgej]

Here is a discount showing up in a Sage Pay test account payment process:

So yes, it is now working to this point at least. Whether Sage Pay take that value further into the back-end control panel, or not, is another thing altogether and out of scope here.

[judgej]

However, noticing just now that discounts don't have a quantity, and from memory that is something this driver handles for discounts, I think we possibly need some more checks in place.

[judgej]

Well, this driver does multiply the discount by the quantiry of that discount. The quantity is not sent to Sage Pay, so it's an internal thing. I don't think it is going to cause any harm, but I also don't think this is the place to do that calculation. I think we can leave it for now, but recommend that a discount always has a quantity of one.

[ntsim]

Hi @judgej,

I've been trying to use the discount functionality and been getting the (ever so helpful) 3021 : INVALID : The Basket format is invalid. error code.

I can't see any actual issues with the transaction or the Basket XML. If I remove the discounts from the basket, the transaction can proceed as normal... This is all in the testing environments of course.

For an example, I have a cart with the Amount being 17.99 and I want the final Amount to be 11.99. The following basket XML is generated to achieve this:

 <basket>
     <item>
        <description>Item 1</description>
        <quantity>1</quantity>
        <unitNetAmount>11.99</unitNetAmount>
        <unitTaxAmount>0.00</unitTaxAmount>
        <unitGrossAmount>11.99</unitGrossAmount>
        <totalGrossAmount>11.99</totalGrossAmount>
     </item>
     <discounts>
         <discount>
         <fixed>6.00</fixed>
         <description>Voucher code (FREEDEL)</description>
         </discount>
     </discounts>
 </basket>

I'm wondering if you/anyone has seen similar problems? Or is there something i'm really missing here?

[judgej]

I haven't checked the docs to confirm this yet, but my first thought is that the amount is not correct. The discounts are treated like items, just like any other line, though with a negative amount. Your basket contains 11.99 minus 6.00 or 5.99, and that does not match your total amount being authorized.

Try it with the item net unit cost (17.99) before any basket discounts are applied.

Unlike some gateways, the basket is not just information for logging. All items in it must correctly add up to the order total.

[ntsim]

I'll give it a go and get back to you on that.

Also, it's funny that you mention adding up items. In my experience so far with SagePay, it has never once complained about Basket items not matching the transaction Amount. In our use case, we don't actually include the shipping as an item, so these two values are always mismatching 😄

I don't think there is currently support in this package for shipping parameters either. The SagePay integrations docs do say it is possible via a Basket deliveryNetAmount node though.

[judgej]

It may have been the old style basket that did this, rather than the XML basket, so I may be wrong. It was just something to try anyway. I'll have a look at the XML when I get ten minutes and see if there is anything in there that looks wrong to me.

[ntsim]

Given it another go. Tried using:

• Amount = 11.99, basket.item.unitNetAmount = 17.99 with basket.discounts.discount.fixed = 6.00
• Amount = 11.99, basket.item.unitNetAmount = 9.99 with basket.discounts.discount.fixed = 6.00
• Amount =17.99, basket.item.unitNetAmount = 17.99 with basket.discounts.discount.fixed = 6.00

Note I originally got the basket.item.unitNetAmount wrong as this is supposed to be 9.99 (SagePay automatically adds on the actual tax amount if you check the completed transaction).

I am tempted to think this is a problem on their side really, as it's not clear to me why it shouldn't work.

[judgej]

The XML looks good to me (correct names and order, and all the mandatory fields). The spec lists TotalGrossAmount with a capital T though, but I suspect that is a documentation error. Edit: a later spec has this corrected, so the lower-case t is correct.

There is a changelog in the spec:

27/08/2015 Remove totals validation from BasketXML

So I wasn't going mad thinking the totals had to add up correctly. It now says:

No validation is performed on the totals of the basket, it is your responsibility to ensure that the amounts are correct and that the total of the basket matches the transaction
amount sent in the Registration

[judgej]

Just to check, is this basketXML sample you have supplied here generated by this OmniPay driver? The formatting is something you have done for readability?

[ntsim]

That just makes it even more baffling that it's complaining about the Basket format being invalid then, no?

And yes, this was XML generated by the Omnipay driver and I adjusted the formatting just for readability purposes.

[delatbabel]

Unlike some gateways, the basket is not just information for logging. All items in it must correctly add up to the order total.

PayPal and PaymentWall also require that the items correctly add up to the order total.