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

PR #107 fixes character stripping allowed chars and adds tests ar… #108

Merged
merged 7 commits into from
Sep 17, 2018

Conversation

harnasz
Copy link
Contributor

@harnasz harnasz commented Sep 16, 2018

Fixes stripping for [] which breaks the Sage 50 Accounts Software Integration. For more details please see #107 .

The following work has been done:

  • Added individual sanitisation method for non
    XML integration \Omnipay\SagePay\Message\AbstractRequest::filterNonXmlItemName
  • Added tests for Non XML Basket Integration
  • Extended \Omnipay\SagePay\Extend\Item with syntactical sugar for setting the product record for Sage 50 Accounts Software Integration as suggested by @judgej

@judgej
Copy link
Member

judgej commented Sep 16, 2018

That looks great :-) The only thing I would change is the parameter key "product_code" to lower camel-case "productCode". It's really minor, but is just a consistency thing with the rest of the package and other gateway drivers.

@judgej
Copy link
Member

judgej commented Sep 16, 2018

Oh, also try merging in master. We can get a few things into the next release, hopefully a fix for issue #104 too, which I think will also open up more features without any BC breaks.

Also you have setProductCode() but getProductRecord(), which I assume is a typo?

@harnasz
Copy link
Contributor Author

harnasz commented Sep 17, 2018

Hey @judgej - thank you! I've just refactored based upon the feedback you sent yesterday along with merging in master. Let me know if there is anything else I need to do 😄 .

@judgej
Copy link
Member

judgej commented Sep 17, 2018

Just wondering, you'v used productRecord everywhere, while the Sage Pay docs call it productCode. Any reason for that?

@harnasz
Copy link
Contributor Author

harnasz commented Sep 17, 2018

Hey @judgej,

From the docs here https://www.sagepay.co.uk/file/12246/download-document/SERVER_Integration_and_Protocol_Guidelines_010814.pdf?token=zlCqBGKB7BlA503YTOOtRsBXWKcI4aVrqNSETHDIspk under 10.0 Sage 50 Accounts Software Integration they refer to it as productRecord. Would you rather it be productCode?

@harnasz
Copy link
Contributor Author

harnasz commented Sep 17, 2018

@judgej - my bad, it is productCode. I'll get this fixed.

@judgej
Copy link
Member

judgej commented Sep 17, 2018

It talks about using the product codes to link the basket lines to the product records in Sage 50. It's not very clear, I know.

@harnasz
Copy link
Contributor Author

harnasz commented Sep 17, 2018

There we go @judgej - hugely appreciate your time.

@judgej judgej merged commit a571515 into thephpleague:master Sep 17, 2018
@judgej
Copy link
Member

judgej commented Sep 17, 2018

Okay, merged in, with an additional to the extended item interface.

Thank you.

@harnasz
Copy link
Contributor Author

harnasz commented Sep 18, 2018

Hi @judgej , many thanks.

@harnasz harnasz deleted the issue107 branch September 18, 2018 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants