thephpleague / omnipay-sagepay

Sage Pay driver for the Omnipay PHP payment processing library
MIT License
54 stars 78 forks source link

isSuccess() only considers one of three possible successful statuses #21

Closed judgej closed 9 years ago

judgej commented 10 years ago

The class Response (extends AbstractResponse implements RedirectResponseInterface) returns the following for isSuccessful():

isset($this->data['Status']) && 'OK' === $this->data['Status']

I believe there are three statuses that also indicate a success:

Which one is returned, will depend on the details of the authorisation, but all of them (I believe) indicate that SagePay will honour the payment, even if there are still some upstream messages that need to be handled.

greydnls commented 9 years ago

Do you have a documentation link for this, please?

judgej commented 9 years ago

Page 48:of "Server Protocol and Integration Guideline" v 2.23:

Sorry, no link - you can find it hosted on Google by third parties, or behind a login account on SagePay, but I can't find a direct link. The notification message is the same for v2.2 and v3.0 of the protocol.

Page 62 of the protocol V3.0:

http://www.sagepay.co.uk/file/12246/download-document/SERVER_Integration_and_Protocol_Guidelines_010814.pdf

Here is the text for the Status field sent by SagePay in the notification field:

OK – Transaction completed successfully with authorisation. NOTAUTHED – The Sage Pay system could not authorise the transaction because the details provided by the Customer were incorrect, or not authenticated by the acquiring bank. ABORT – The Transaction could not be completed because the user clicked the CANCEL button on the payment pages, or went inactive for 15 minutes or longer. REJECTED – The Sage Pay System rejected the transaction because of the rules you have set on your account. AUTHENTICATED – The 3D-Secure checks were performed successfully and the card details secured at Sage Pay. REGISTERED – 3D-Secure checks failed or were not performed, but the card details are still secured at Sage Pay. ERROR – An error occurred at Sage Pay which meant the transaction could not be completed successfully.

judgej commented 9 years ago

I've seen many examples of this driver notification processing using isSuccess() to determine whether to return confirm() or error()/invalid(). They have all misunderstood the purpose of isSuccess() and the meaning of confirm() and error(). The application should always return confirm() if it received the confirmation, understood it, and accepted it as valid and expected. I wonder if the documentation needs to make this a little clearer.

greydnls commented 9 years ago

Per the documentation you provided, Authenticated and Registered are only returned if TxType is AUTHENTICATE. From what I can see that's not an action supported by this package.

AUTHENTICATED = The 3D-Secure checks were performed successfully and the card details secured at Sage Pay. Only returned if TxType is AUTHENTICATE. REGISTERED = 3D-Secure checks failed or were not performed, but the card details are still secured at Sage Pay. Only returned if TxType is AUTHENTICATE.

Am I missing something?

judgej commented 9 years ago

True, it does not support that type of transaction at the moment. It is useful when fulfilment may take six days or more (in the US, payment cannot be taken for an online shop until the order is shipped; it is different in the UK).

greydnls commented 9 years ago

So, the best way forward, from what I'm hearing, is that we implement the Authenticate TxType?

judgej commented 9 years ago

Possibly. It depends on how far you want to take the SagePay support. It has a lot of features that may take it well out of scope of the "lowest common denominator" that OmniPay tries to provide. Some would argue that if you want to use features that only one gateway provides, then perhaps you need a more specialised package than OmniPay. Personally I don't know what features all the other gateways and/or drovers provide - authenticate may be something that is sorely needed, or maybe nobody needs it. I think I'm on the fence here.

greydnls commented 9 years ago

For now, I would say leave it as it is, until there is a need to implement it, and then readdress at that time.

Given that that's the only TxType that returns Authenticated and Registered, and we're not currently using that, am I okay to close this issue?

judgej commented 9 years ago

Yes, I'll not argue with what the scope of the driver should be, as that is up to the project leaders/concensus. It can always be extended where needed (I do that now). The discussion is here to refer back to when needed.