Closed alexw23 closed 9 years ago
Will review shortly. For the record, you can also force push (git push -f) to your existing branch and github will update the pull request with the squashed commit.
Thanks for the tip. :)
Need anything else for final merge on this? Really looking to get this in
Sorry for the slow review. Looks good - I've added a few comments inline. Also can you lay the tests out like the other Rapid gateway - with separate tests for each request class, and check the test coverage is at 100%.
Test layout example see https://github.com/omnipay/eway/tree/master/tests/Omnipay/Eway
Test coverage stats are here: https://travis-ci.org/omnipay/eway/jobs/14298372
Will need to fix these up shortly and get it merged in once and for all. It's been working fine for me for the last 8 months.
@adrianmacneil had a few issues downloading omnipay/tests given the recent name change.
Had to use "omnipay/tests": "2.0.*@dev"
in composer.json to conduct the tests. This may fail due to that reason. Once you've resolved all of that please merge this in as testing has been conducted and it is all working fine.
I've added a comment to one of your inline responses - otherwise everything else has been implemented.
@adrianmacneil Can we merge this?
Paging @kayladnls for review
Hi guys, I want to add some more functionality to the Eway Direct option. Like the possibility to capture a card, update a card, make a payment with a captured card etc. Should I wait for this to be merged before I start developing?
@alexw23 This looks good, save for that single question I had. Should be able to merge this in soon.
@Mihai-P You can make your PR now if you're ready. If not, this branch should be merged in soon.
@kayladnls I want to build on top of what alexw23 did, I already have it up as a package but only the Credit cart management stuff, so together with that part would make a great package. I will wait for the merge.
@alexw23 any movement in this? Also, can you remove DS_STORE?
@kayladnls done. See feedback on optional field. I think it's time to merge it in :+1:
@Mihai-P when you do can you tag me in so I can see what's going on :+1:
Is this going to be merged?
On 13 Feb 2015, at 8:33 am, alexw23 notifications@github.com wrote:
@Mihai-P when you do can you tag me in so I can see what's going on
— Reply to this email directly or view it on GitHub.
Thanks @kayladnls can you also merge PR #6
Everything is working great, tests are completed and I have now squashed into a single commit for readability.
Ready for merge @adrianmacneil :+1: :package: