Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Old-style basket Integration" - Stripping of square brackets #107

Closed
harnasz opened this issue Sep 13, 2018 · 4 comments
Closed

"Old-style basket Integration" - Stripping of square brackets #107

harnasz opened this issue Sep 13, 2018 · 4 comments
Assignees

Comments

@harnasz
Copy link
Contributor

harnasz commented Sep 13, 2018

There was a PR raised and accepted #67 a while back to allow for the Old-style basket Integration which is primarily used for Sage 50 Accounts Software Integration.

One of the features is the ability to link a transaction to a specific product (I have copied the relevant docs below from SagePay).

The way to do this is to prepend the code in-front of the description wrapping it in square brackets which looks like the following:

[SKU]ProductDescription

Unfortunately the [ ] gets stripped. This is due to the method invoking filterName($name) - https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Message/AbstractRequest.php#L492

filterName is actually used for the BasketXML method which states in the documentation that the XML nodes cannot contain square brackets. The method also has : in the allowed special chars which will break the colon-delimited field format if is present within the item name.

I will shortly raise a PR to patch this

SagePay Docs

It is possible to integrate your Sage Pay account with Sage Accounting products to ensure you can
reconcile the transactions on your account within your financial software.

To learn more about the integration options available and which version of Sage Accounts integrate
with Sage Pay please visit sagepay.com, or email tellmemore@sagepay.com.

If you wish to link a transaction to a specific product record this can be done through the Basket field
in the transaction registration post.

Please note the following integration is not currently available when using BasketXML fields.
In order for the download of transactions to affect a product record the first entry in a basket line needs
to be the product code of the item within square brackets.
Example;
4:[PR001]Pioneer NSDV99 DVD-Surround Sound System:1:424.68:74.32:499.00:
499.00:[PR002]Donnie Darko Director’s Cut:3:11.91:2.08:13.99:41.97:[PR003]Finding
Nemo:2:11.05:1.94:12.99:25.98: Delivery:000:000:000:000:4.99

When a transaction with the Basket field containing the items above is imported into Sage 50
Accounts an invoice is created and product codes PR001, PR
@judgej
Copy link
Member

judgej commented Sep 13, 2018

That makes sense - a little over-zealous validation in there.

You could also extend https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Extend/Item.php to include an SKU property that automatically gets added to the name (or description?) in the correct format, if that helps.

I've got quite a bit of refactoring in the issue105 branch you may want to take into account. I haven't touched the basket functionality at all, but the docs have a lot added and tweaked. I'm hoping to get that merged in very soon. Shouldn't make any difference to your changes, but mentioning it in case there is some overlap.

@harnasz
Copy link
Contributor Author

harnasz commented Sep 13, 2018

Hey @judgej,

Many thanks for the above, that's hugely appreciated with giving me heads up of your 105 branch refactor. I think your suggested path of extending Item is the best as it can be argued that specific product record is a key feature of the SagePay Integration?

I am thinking of adding another filter method along the lines of

\Omnipay\SagePay\Message\AbstractRequest::filterNonXmlName

Which follows the conventions of these:

\Omnipay\SagePay\Message\AbstractRequest::filterItemName
\Omnipay\SagePay\Message\AbstractRequest::filterDiscountName

I'll also add some tests around this as I added some breakpoints underneath https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Message/DirectAuthorizeRequest.php#L75 to see if there was any tests that I needed to refactor but it didn't stop the execution.

I'll ping you what I've done first @judgej and you can see if there is any overlap with your work and take it from there!

@judgej
Copy link
Member

judgej commented Sep 13, 2018

It's just a bit of syntactic sugar really. People could just format and add the SKUs themselves, but adding it to the extended item gives an additional method if people want to use it, and it can have its own validation rules if necessary.

harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 16, 2018
…ds tests around non-XML basket integration
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 16, 2018
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 17, 2018
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 17, 2018
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 17, 2018
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 17, 2018
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 17, 2018
harnasz added a commit to harnasz/omnipay-sagepay that referenced this issue Sep 17, 2018
judgej added a commit that referenced this issue Sep 17, 2018
@judgej judgej self-assigned this Sep 25, 2018
@judgej
Copy link
Member

judgej commented Sep 30, 2018

Release 3.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants