thephpleague / omnipay-sagepay

Sage Pay driver for the Omnipay PHP payment processing library
MIT License
55 stars 78 forks source link

Issue #107 - Basket Format - Sage 50 Accounts fixes #109

Closed harnasz closed 6 years ago

harnasz commented 6 years ago

I raised an issue previously for #107 -and then raised a PR for version 3.x. Unfortunately the package of https://github.com/craftcms/commerce-sagepay has a dependancy requiresominipay-sagepay version 2.x.

I have migrated the code which had the patch for 3.x from the original PR that I raised. https://github.com/thephpleague/omnipay-sagepay/pull/108

Is this something that we accept and pull into 2.x?

judgej commented 6 years ago

Merged for 2.x

judgej commented 6 years ago

Could you give the 2.x head a final confidence check. If it works, I'll tag it for release.

harnasz commented 6 years ago

Thanks @judgej - I've just given that a look over and all seems fine.

judgej commented 6 years ago

Does it actually work for you though, if you pull in dev-master or dev-2.x? It's just good to see it working without any surprises before tagging it for the other X-thousand users of the package to see :-)

harnasz commented 6 years ago

Sorry @judgej , I get you.

The Sagepay package was a dependency of a few other packages but I have managed to test 2.x against the SagePay test server and it submitted the request fine and got redirected as usual for payment for the server integration.

Here is the result below from var_dumping \Omnipay\Common\Message\MessageInterface::getData

'VPSProtocol' => string '3.00' (length=4)
'TxType' => string 'PAYMENT' (length=7)
'Vendor' => string '...' (length=12)
'AccountType' => string 'E' (length=1)
'Description' => string 'Order #856' (length=10)
'Amount' => string '69.98' (length=5)
'Currency' => string 'GBP' (length=3)
'VendorData' => null
'VendorTxCode' => string '14130e91d26eb53601c9664f1fb2da50' (length=32)
'ClientIPAddress' => string '192.168.100.1' (length=13)
'ApplyAVSCV2' => int 0
'Apply3DSecure' => int 0
'BillingFirstnames' => string 'Tom' (length=3)
'BillingSurname' => string 'Harnasz' (length=7)
'BillingAddress1' => string '2' (length=1)
'BillingAddress2' => string 'Test View' (length=10)
'BillingCity' => string 'Newcastle' (length=5)
'BillingPostCode' => string 'NE1' (length=8)
'BillingState' => string '' (length=0)
'BillingCountry' => string 'GB' (length=2)
'BillingPhone' => string '0123457890' (length=11)
'DeliveryFirstnames' => string 'Tom' (length=3)
'DeliverySurname' => string 'Harnasz' (length=7)
'DeliveryAddress1' => string '2' (length=1)
'DeliveryAddress2' => string 'Test View' (length=10)
'DeliveryCity' => string 'Newcastle' (length=5)
'DeliveryPostCode' => string 'NE1' (length=8)
'DeliveryState' => string '' (length=0)
'DeliveryCountry' => string 'GB' (length=2)
'DeliveryPhone' => string '0123457890' (length=11)
'CustomerEMail' => string 'tom@test.com' (length=15)
'Basket' => string '1:[SKU-1]Product1:2:34.99:0.00:34.99:69.98' (length=80)
'AllowGiftAid' => int 0
'NotificationURL' => string '---' (length=148)

and what I get from var_dumping \Omnipay\Common\Message\RequestInterface::send

  array (size=6)
      'VPSProtocol' => string '3.00' (length=4)
      'Status' => string 'OK' (length=2)
      'StatusDetail' => string '2014 : The Transaction was Registered Successfully.' (length=51)
      'VPSTxId' => string '{3387AC68-3912-48A6-3B92-80E6637B8D}' (length=38)
      'SecurityKey' => string 'JYDMT3DX' (length=10)
      'NextURL' => string 'https://test.sagepay.com/gateway/service/cardselection?vpstxid={3387AC68-3912-48A6-3B92-80E66B8D}' (length=101)

Let me know if you need anything else :)

judgej commented 6 years ago

Marvelous. I'll make this a minor release, since an interface has changed, but it shouldn't break anything. I'll roll the 3.x release into some other changes, and we can see how this goes in production for a few days anyway. Thank you for your hard work and patience :-)

harnasz commented 6 years ago

That's marvellous, thank you very much. We really appreciate your time helping us with this. Glad we can help some other people down the line...