Giter Club home page Giter Club logo

Comments (22)

judgej avatar judgej commented on September 26, 2024

Certainly looks like it can be improved. It should return everything it can return.

There are a number of different XML formats from Authorize.Net, depending on just where the error occurred (e.g. error codes and messages will be found in different places depending on whether there was an authentication or field error, or an authorisation error), so that needs to be taken into account.

The way it works now was likely a consequence of just looking at one of these formats.

from omnipay-authorizenet.

VinceG avatar VinceG commented on September 26, 2024

@judgej i am willing to submit a PR to have this fixed. is this even being maintained?

from omnipay-authorizenet.

judgej avatar judgej commented on September 26, 2024

Yes, it is maintained, and relies on fixes from the community.

from omnipay-authorizenet.

VinceG avatar VinceG commented on September 26, 2024

@judgej Perfect. i'll submit a PR to remove that check to have the transaction reference available when the transaction fails as well.

from omnipay-authorizenet.

judgej avatar judgej commented on September 26, 2024

Change merged - works okay on my test account. Thank you.

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

I'm struggling with the getTransactionReference for a different reason

The standard per the docs is for the transactionReference to be a reference code or string. When implementing multiple processors it's really important to have some consistency on key methods - otherwise the integration code has to know about the workings of every processor. So I would argue the correct thing to do would be to have a method like getTransactionReferenceObject that returns 'whatever it is that is A.net specific & can't fit a generic model', and for getTransactionReference to return the value that getTransId currently does

Successful Response

For a successful responses, a reference will normally be generated, which can be used to capture or refund the transaction at a later date. The following methods are always available:

$response = $gateway->purchase(array('amount' => '10.00', 'card' => $card))->send();

$response->isSuccessful(); // is the response successful?
$response->isRedirect(); // is the response a redirect?
$response->getTransactionReference(); // a reference generated by the payment gateway
$response->getTransactionId(); // the reference set by the originating website if available.
$response->getMessage(); // a message generated by the payment gateway

I realise that the change I'm proposing would break backwards-compatibility so I think it should be considered at the point of going to 3.0.

In the meantime I think I have to fork this gateway if I want to integrate it alongside others

from omnipay-authorizenet.

judgej avatar judgej commented on September 26, 2024

Could you give some examples of the inconsistencies? The transactionReference should be returned as a string, just an arbitrary length string, with no shared patterns between the gateways, but a string that can be stored and reused.

Some gateways have multi-part transactionReference values, and OmniPay serializes those to bring them into line with the reference being a string. I can see how an object would help there, since parts of that reference can be useful in their own right, but being able to serialize/deserialize for storage can be handled by the class.

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

You are right, I do wind up with the object json'd to a string. But the string is not similar to the transaction references for any of the other processors. ie. I get

{"approvalCode":"Z0FTGK","transId":"60019251799","card":{"number":"1111","expiry":"022018"}}

Whereas for other processors I would see
$response->getTransactionReference()// 60019251799

At the moment we use the transactionReference on the receipts - and it is a part of a database key. The longer format isn't really appropriate for either of those uses.

I can see that the purpose of the longer format is to provide all the details that might be used to do a refund later. Which is kind of vexing. The idea of Omnipay is to create a layer where the application developer does not need to know about the internals of every gateway. In the case of Authorize.net we already have the credit card pan & expiry as they are entered on our site, but that would not always be the case.

OK, I'm stuck now..... The assumption that works on other gateways that the transactionReference can be used for auditing purposes and presenting to the customer is inconsistent with the refund process here.

I've been working on creating some actions to retrieve the transactions from authorize.net to do a nightly audit. It's working well & I'll create a PR so you can see. But, I think I'm going to have to stay forked because I can work around the refund issue much more easily in my own code than I can the fact the transactionReference is not actually the gateway reference for the transaction

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

Hmm, if there was a consistent way to get the actual transactionReference I guess it could be a different function name & if there was evidence of json I could call that e.g

$transactionReference = $response->getTransactionReference();
if (substr($transactionReference, 0, 1) == '{') {
  $refundReference = $transactionReference;
  $transactionReference =  $response->getTransactionReferenceString();
}

I'll sleep on it. There is a part of my mind that won't accept the idea that the merchant's unique reference is not being treated like a unique identifier. Perhaps in the morning I'll be better able to think of a way to get the data that is important for auditing etc in a generic way

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

Oh last thought on this - the query() action in the PR I mentioned (#79 ) can actually retrieve the card_number & expiry_date from Authorize.net - although I might have left off the latter at this stage

from omnipay-authorizenet.

dcaswel avatar dcaswel commented on September 26, 2024

@eileenmcnaughton We ran into a similar issue when implementing Omnipay into our system. We also need to implement multiple gateways at the same time which was the main draw to Omnipay. Our situation was actually made worse by the fact that we were moving from another solution to implementing Omnipay so we needed a way to be able to run refunds when the data was not stored in the serialized object as is done with the Authorize.net driver. Other issues that we ran into included the fact that different drivers require different methods/credentials for setting up the gateway and different data requirements to run things like a purchase or a refund. Our solution was to use polymorphism by creating an abstract class that acts basically the way that you expect a generic Omnipay driver to act and then create a concrete class for each gateway that makes any needed adjustments for that gateway driver. It's not the ideal situation that we were hoping for and you probably weren't either but it is a fairly elegant way to make sure that you are not duplicating code while keeping the solution fairly clean. In the case of the issue that you are running into, because of our need to be backwards compatible with old transactions, we actually don't store our references as the serialized object. We already store all the data from that object in separate fields so we recreate the object every time we need it. Again, it's not ideal but it works.

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

So, I guess the options are

  1. maintain a fork
  2. maintain a wrapper class
  3. find something that would be acceptable in the Omnipay library - so far I can think of changing the refund class so that it can handle either a json or a transaction reference, and if the latter queries A.net for the extra details, or the rather unclean variant about

I guess the disadvantage of a fork is that it needs to incorporate changes from upstream. On the bright side however, it's fairly easier just to point at my fork through composer and I tend to be happy to lose history on forks, making a rebase over master flow work well.

I guess a last option I hadn't thought of would be to create my own Authorize.net plugin which basically extends this one but overrides the specific method getTransactionReference and the Refund class. I guess that is not incompatible with the fork.

from omnipay-authorizenet.

dcaswel avatar dcaswel commented on September 26, 2024

Unfortunately, because of the issues I stated above with the different drivers requiring different parameters in each transaction type, we didn't really see a better way then to create a wrapper class. If you try to do it by creating your own fork of the different drivers or creating your own driver that extends the existing one, there is a good chance you are going to end up having to do that for every driver so you can compensate for the differences and make them uniform.

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

hmm so far I haven't found others that are unresolvable, but you may be right. Do you just have something that basically has a handling for each non-standard driver?

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

(I guess I had hoped that with the v3 it would be possible to standardise things like this - I can see the issue with refund, but would also imaging that if all refunds actions supported setExpiryDate & setCardNumberTruncated then we could add a look up within refund to query these if not set

from omnipay-authorizenet.

judgej avatar judgej commented on September 26, 2024

In release 2.5.0

from omnipay-authorizenet.

judgej avatar judgej commented on September 26, 2024

Sorry, meant to leave this one open.

from omnipay-authorizenet.

dcaswel avatar dcaswel commented on September 26, 2024

@eileenmcnaughton I went back and looked at our implementation again this morning and realized that I may have misled you a little on how we solved this issue. Basically what we do is we have our own version of an AbstractGateway class. This class CAN interact with the Omnipay driver in a standard way but it doesn't have to. In fact, depending on your system's requirements, you might be able to accomplish this with an interface instead of an abstract class. The methods needed in the interface/abstract class are something like getRefundData, getPurchaseData, etc. Those methods are very similar to the getData methods that you see in the drivers but it is getting the specific data needed for that driver in that method.
Then for each gateway driver that we are using, we create a concrete class or a class which implements the interface. The constructor of that class runs the Omipay::create method to instantiate the object and initializes it with whatever credentials that driver needs. Then since we are extending the abstract class (or implementing an interface) we define the getRefundData method and any other methods needed to take a standard set of data from our system and create the specific data set that is needed for that driver with that method. This is where we have the logic to recreate the TransactionReference Object for Auth.net but this only happens in the Auth.net's version of getRefundData. You may also need to create methods for extracting what you need from the TransactionReference object which is one reason why we are using an abstract class so we can have a method that just calls getTransactionReference in the abstract class but does more complex stuff in the Auth.net concrete class. Another reason for using an abstract class is because our system requirements actually necessitate that we take this a step further and actually run the purchase and refund methods in either the abstract or concrete class depending on the driver.

I am curious, what was your solution for creating your Omnipay objects with different drivers needing different credentials? Also, how did you resolve the issue that one driver requires different fields then another for a specific method like a purchase or refund?

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

Is your code in a public space? I think I am following you but it would be good to see it.

Regarding credentials. Our database implements other non-Omnipay gateways and has a row for each type it supports. It has fields like the password_label, user_label, subject_label. These were intended for UI display but I make sure they are a camel case of the credential fields.

"how did you resolve the issue that one driver requires different fields then another for a specific method like a purchase or refund"

Mostly I have integrated gateways that are pretty good so far, I've done a few PRs to address things, but Authorize.net is more challenging on that front than many, so I haven't fully worked through it.

from omnipay-authorizenet.

dcaswel avatar dcaswel commented on September 26, 2024

Sorry that I didn't respond to this sooner. Unfortunately, we have some stuff in that code that makes it so we can't put it in the public space. I did, however, create a new repo in my account that has the stripped down version that I think will illustrate what I am talking about. The repo is at https://github.com/caswell-wc/omnipay-multi-gateway-demo. I tried to explain what I could in the comments of the code. Hopefully it makes a little more sense to you with that.

from omnipay-authorizenet.

eileenmcnaughton avatar eileenmcnaughton commented on September 26, 2024

Hey - that's for that! you rock!

from omnipay-authorizenet.

judgej avatar judgej commented on September 26, 2024

Closing as fixed.

from omnipay-authorizenet.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.