search

thephpleague / omnipay-sagepay

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

Issue #58 Naming of repeat methods; additional tests; fix hard-coded … #68

Closed judgej closed 8 years ago

judgej commented 8 years ago

As discussed in Issue #58:

The repeatPayment has been deprecated, but left in for now as an alias of repeatPurchase, as it is in use in production.

coatesap commented 8 years ago

Nice work @judgej - thanks for sorting out some of those issues.

judgej commented 8 years ago

I haven't tested it on my test Sage Pay account yet, so I'll do that before taking it any further.

The overriding card details may need a small rethink, as the card object contains both the CVV and the shipping address as a single object. You may wish to supply a CVV but not change the shipping address on a repeat, and that isn't possible here. Maybe the way to do it is to check all the shipping address fields, and so long as ALL are null, then assume no shipping address has been supplied. I suspect that is what the remote gateway does anyway, so if you supply even ONE shipping field, it will discard the whole saved shipping address and use that single field.

judgej commented 8 years ago

Issue #58

Trying this out on my test instance, and I'm getting a few odd results. Any attempt to do a repeatAuthorize (REPEATDEFERRED) gives me this error:

4006 : The TxType requested is not supported on this account.

I also get that if I attempt a repeatPayment on an authorize before that authorization is captured. My account settings is set to allow REPEATDEFERRED, so I'm sure there is some subtlety that I'm overlooking. I've double checked the documentation, and can't see anything wrong there. I'll raise it with Sage Pay support.

As an aside, the repeat message has introduced this inconsistency with the naming of parameters passed in:

I think the first (transactionReference) should probably be used in both cases, to be more consistent with OmniPay in general, but I have not checked other gateway drivers to see if that is the general consensus.

judgej commented 8 years ago

The REPEATDEFERRED is documented in the server/direct shared protocol docs, the latest of which are dated 2014-08-01. REPEATDEFERRED is not mentioned in the server or direct specific protocol docs (both of which indicate you should read the shared docs for REPEAT payments).

I am wondering if REPEATDEFERRED is something that was removed in protocol 3.00 years (years!) ago, but got left behind in the documentation? The official PHP SDK does not seem to cover REPEAT or REPEATDEFERRED.

judgej commented 8 years ago

All works for me now. Turns out Sage Pay did need to enable this feature on my account - REPEAT and REPEATDEFERRED need to be activated on the account separately.

judgej commented 8 years ago

I'm happy this fills in the gaps of PR #66 for issue #58

I'll check whether transactionReference needs to be an alias for relatedTransactionReference for consistency, then accept this PR #68 to finish off issue #58

Edit: no other gateway seems to support repeat payments on past transactions, so I cannot find any precedence. If transactionReference is used to reference the original transaction, then the returned transactionReference for the new repeat transaction (which would be different) could get a little confusing, so maybe best left as it is for now.

judgej commented 8 years ago

I'll merge this before close today. Please shout if there are any problems with that. There should be no BC breaks in this.

delatbabel commented 8 years ago

I think the first (transactionReference) should probably be used in both cases, to be more consistent with OmniPay in general, but I have not checked other gateway drivers to see if that is the general consensus.

Agree with this. I'm not aware of many other gateways that do this but I believe that Allied Wallet does and it uses transactionReference.

judgej commented 8 years ago

I'll put transactionReference in as an alias method for now, and deprecate the old one.