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

CIM Add ability to create Customer Profile with OpaqueData from Accept.js #117

Merged
merged 10 commits into from
Dec 31, 2018

Conversation

alberto1el
Copy link
Contributor

I made an example app that showcases the accept.js and this addition that you can check here:

https://github.com/alberto1el/omnipay-example

@alberto1el
Copy link
Contributor Author

Tests are failing due to a 'Invalid header syntax', although when travis runs the setup=lowest Job it passes, I see that the main difference is the composer update command, so this might be something else causing the failure?
@barryvdh could you advice?

@judgej
Copy link
Member

judgej commented Dec 30, 2018

Looking at the source messages being loaded in the tests, could it be the "line folding":

https://github.com/thephpleague/omnipay-authorizenet/blob/master/tests/Mock/CIMCreateCardFailureWithDuplicate.txt#L4

I don't know if that was ever valid, but it doesn't look right to me:

HTTP/1.1 200 OK
Cache-Control: private
Content-Length: 746
Content-Type: text/xml;
charset=utf-8
Server: Microsoft-IIS/7.5

charset=utf-8 should be on the same line as Content-Type:, should it not?

@judgej
Copy link
Member

judgej commented Dec 30, 2018

Yes, I think this is the problem. I've made a fix and will merge it into master.

@judgej
Copy link
Member

judgej commented Dec 30, 2018

Try pulling in the master branch now. Should be fixed. I assumed it would be affecting other people running tests, so got the fix (hopefully 🤞) in quickly. Merry Christmas :-)

@alberto1el
Copy link
Contributor Author

@judgej Yes it was that, great catch thank you very much!
I am just missing a couple tests I will add them later on today, I will since I do want this to be merged as soon as possible :)

@judgej
Copy link
Member

judgej commented Dec 30, 2018

Just a heads up though - the CIM Abstract Request class extends the AIM Abstract Request, and that already supports some of the Opaque Data methods you have added (if I'm reading them correctly). It may be worth checking if duplicate and redundant methods have been been added here.

It may also be worth updating the README with an example of how this new feature can be used.

@alberto1el
Copy link
Contributor Author

@judgej Thanks.
Yes, the methods were being duplicated so I removed them, I actually had copied them from that commit but didn't realize that the CIMAbstractRequest was extending it.

I added the tests and added an example to the README file I hope it is clear enough.

@judgej judgej merged commit 8c7ca9c into thephpleague:master Dec 31, 2018
@judgej
Copy link
Member

judgej commented Dec 31, 2018

Looks great, thank you.

I'm going to try to merge in a few more PRs, but give the master branch a try in the meantime. If all is working for you, we can release this as version 3.1.0

I think the whole driver could do with a little refactoring at some point, such as a better class namespaces structure, and converting from/to XML at the interface to the gateway and not all over the place like it is now, but that's just some fun for 2019 :-)

@alberto1el
Copy link
Contributor Author

I tested the master branch with the merged PR and it's working great :)

And yes I have been using this driver for years now, I believe 2019 will be a good year for cleaning it up, I read the really long issue were you talk about the accept.js library and the 'changes' on the Autorize.net API were that guy from authorize.net also comments, and I was wondering, which is the 'roadmap' for this library? , since I saw you worked on a couple repositories (probably to be able to use omnipay 3 with authorize.net which is now possible), will you continue with those or do you want to clean this one?.

Thank you very much for your work @judgej and may you have a very happy new year!

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.

2 participants