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

Sage Pay Form integration - case mis-match for the SendEmail parameter? #139

Closed
davesmall9xb opened this issue Aug 8, 2019 · 6 comments
Closed
Assignees
Labels

Comments

@davesmall9xb
Copy link
Contributor

davesmall9xb commented Aug 8, 2019

I've come across a possible issue with the SendEmail option in Omnipay\SagePay\Message\Form\AuthorizeRequest

At https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Message/Form/AuthorizeRequest.php#L127
the SendEmail data is set up, but it's then filtered out because of the mis-match in case sensitivity at https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Message/Form/AuthorizeRequest.php#L29

So $data['SendEmail'] is set up properly but then filtered out because it's not in the $validFields array (the $validFields array contains SendEMail but not SendEmail).

I'd propose updating $validFields to allow 'SendEmail' rather than 'SendEMail', but I wanted to check this with the creators of this useful project :)

I have created a PR for this change at #140

@judgej judgej self-assigned this Aug 13, 2019
@judgej judgej added the bug label Aug 13, 2019
@judgej
Copy link
Member

judgej commented Aug 13, 2019

Hi @davesmall9xb thanks for the heads up and the fix.

Looking at the specs (for Sage Pay Form), the correct field name to send is SendEMail. I'm guessing it is derived from "send e-mail" and converted into upper camel case, and that's how it comes out.

The Sage Pay servers may be case-insensitive to these names, so it may not ultimately matter what case is used, but that is out of our control and so we should stick to the spec.

So, I actually thinking it is this line that needs to change. And I guess we need a test too, to prove that the flag gets through to the request message.

@judgej
Copy link
Member

judgej commented Aug 13, 2019

It looks like the same fix is also needed here:

https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Message/Form/AuthorizeRequest.php#L103

But not here:

https://github.com/thephpleague/omnipay-sagepay/blob/master/src/Message/Form/AuthorizeRequest.php#L107

Consistency, eh?

Do you fancy making that change to your fork, so it can be merged in?

@judgej judgej added the awaiting response Awaiting response label Aug 14, 2019
@davesmall9xb
Copy link
Contributor Author

Thanks @judgej - I have updated the PR for this change at #140

Can this be merged in, please?

@judgej judgej removed the awaiting response Awaiting response label Aug 19, 2019
@judgej
Copy link
Member

judgej commented Aug 19, 2019

Merged - that fix works great. Thank you!

I tried it out setting the SendEMail option to 1 (both vendor and customer emails) and received both. The customer email used the language set for the customer and referred to "your order". The email for the vendor used the language set for the vendor account and referred to the order for "a customer".

I'll try to get it tagged for release in a bit.

@judgej
Copy link
Member

judgej commented Aug 19, 2019

Now tagged for release:

https://github.com/thephpleague/omnipay-sagepay/releases/tag/3.2.3

I've tried it out locally, and works fine. I will work some tests in soon, when I have a little more time, and expand those tests to cover every field that can be sent over the API.

@judgej
Copy link
Member

judgej commented Aug 24, 2019

I'm closing this - works for me in production.

@judgej judgej closed this as completed Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants