eileenmcnaughton / nz.co.fuzion.omnipaymultiprocessor

Omnipay Multi Processor Payment Processor For CiviCRM
Other
13 stars 44 forks source link

Usage of completePurchase #3

Closed sergej-koscejev closed 9 years ago

sergej-koscejev commented 9 years ago

I've been continuing the work on implementing my Omnipay gateway to use in conjunction with your multiprocessor, and I've stumbled upon what I think is a potential problem.

The code uses completePurchase() to process payment notifications, but the Omnipay design is AFAICT to use this method for outgoing calls, as evidenced by the fact that it should accept the same parameters as purchase(). According to thephpleague/omnipay#83 it was considered to have some other method like notify() on gateways that support incoming notifications but apparently it hasn't yet been implemented.

So I propose the code to be changed to look for a "notify" method on the gateway, and call that one instead of completePurchase, if found (and throw an error if it wasn't found). And if this works well, we could contribute the method support to omnipay (which would mean just describing its contract).

eileenmcnaughton commented 9 years ago

I don' t think that is a fault per se with completePurchase - ie.

"Anyway, to answer your question - the developer using Omnipay is expected to store the order details in their database, and pass them to the completePurchase() method, so you don't need to worry about sessions. To identify the order they are expected to pass through some sort of custom return URL, e.g. https://www.example.com/checkout/return/123456. Then in that controller action they would load the order details and call completePurchase()."

The majority of the processors I have touched DO return amount in the IPN calls - but you can determine within completePurchase what you validate / make compulsory. The fact it accepts amount doesn't mean it has to require it.

I don't have an objection to merging a PR that will use notify if it exists because I can't see a downside to that but I wouldn't want to disable the completePurchase which worked fine for me on Cybersource

sergej-koscejev commented 9 years ago

Amount doesn't concern me, I wasn't the one who raised that omnipay issue. I'm just afraid that my gateway will not be compatible with other Omnipay clients (e.g. somebody using Omnipay for their e-shop or whatever) if it doesn't behave in the way Omnipay specifies. But maybe it's the Omnipay spec that's wrong, not the multiprocessor code, I'll need to clarify this with the Omnipay developers.

I'll look into preparing a PR for you to merge.

eileenmcnaughton commented 9 years ago

The Authorize.net SIM gateway is an example of an existing gateway that uses IPNs / Silent POSTS

sergej-koscejev commented 9 years ago

Right, if you look into the SIMGatewayTest, there's this piece of code:

        $this->getHttpRequest()->request->replace(
            array(
                'x_response_code' => '1',
                'x_trans_id' => '12345',
                'x_MD5_Hash' => md5('elpmaxeexample9910.00'),
            )
        );

        $response = $this->gateway->completeAuthorize($this->options)->send();

where $this->options is:

        $this->options = array(
            'amount' => '10.00',
            'transactionId' => '99',
            'returnUrl' => 'https://www.example.com/return',
        );

Contrast this with the code in OmnipayMultiProcessor::processPaymentNotification:

    $originalRequest = $_REQUEST;
    $_REQUEST = $params;
    $response = $this->gateway->completePurchase($params)->send();

Here the HTTP parameters are passed directly, but the SIMGateway will find them implicitly, in Symfony's HttpRequest which is constructed from globals.

sergej-koscejev commented 9 years ago

But actually the way SIMGateway expects it to work isn't appropriate because it's impossible to find out transactionId from the incoming notification in a generic way. So Omnipay should provide for a better way to handle notifications.

eileenmcnaughton commented 9 years ago

I'm confused then - because I also use similar code in Cybersource & was able to run it with the api (payment_notification.create - which is part of the extension & which lifts the data out of the system log & which I use for development because it means you only need to capture the IPN once & then you can do your dev without having to do the whole transaction)

eileenmcnaughton commented 9 years ago

NB - the code that figures out if it was a success or not is the CompletePurchaseResponse

/**

eileenmcnaughton commented 9 years ago

I'm closing this - since you did a PR for GoPay I assume you got it working

sergej-koscejev commented 9 years ago

Haven't got it confirmed from the client still, but I agree, the issue can be closed for now.