AuthorizeNet / sample-code-php

This repository contains working code samples which demonstrate php integration with the Authorize.Net API
MIT License
177 stars 197 forks source link

Handling responses #77

Closed alexsasharegan closed 6 years ago

alexsasharegan commented 7 years ago

I've been looking through the example code to auth & capture a sale on a credit card found here: PaymentTransactions/charge-credit-card.

The error handling feels fairly clunky and full of potential gotchas. Conditionals get nested up to 4-levels deep, there are weak equality checks against NULL which behaves in tricky ways (almost like empty(), but not... see the php docs ), and the error/message arrays are accessed only at index 0 for codes & descriptions (no looped printing of messages? if always a single element in an array, why an array?).

I understand that an auth & capture can have many conditions to a failure, and that this code is meant to show off as much of the inner workings of the SDK as possible, but is there not a way to expose a success variable that isn't deeply nested and conditional? Why can the result code come back 'ok', but the transaction still fail?

As I dig deeper into these questions by reviewing your reference docs, I feel like there is a lot that is left out of this example. Just trying to wrap my head around the features and the mechanics of this SDK so I can robustly handle transactions.

alexsasharegan commented 7 years ago

This is the more robust response handling I'm attempting:

<?php

$response = $controller->executeWithApiResponse( $this->authNetAPIEndpoint );
$result = TransactionResult::newFromResponse( $response );
echo $result->toJson( JSON_PRETTY_PRINT );

Which outputs:

{
    "responseCode": "Ok",
    "messages": [
        {
            "code": "I00001",
            "text": "Successful."
        }
    ],
    "transactionResponse": {
        "success": true,
        "responseCode": "1",
        "transId": "60020226802",
        "authCode": "68WTTL",
        "accountNumber": "XXXX1111",
        "accountType": "Visa",
        "cvv": {
            "cvvResultCode": "M",
            "description": "Match."
        },
        "avs": {
            "avsResultCode": "Y",
            "description": "Address (Street) and five digit ZIP match."
        },
        "cavv": {
            "cavvResultCode": "2",
            "description": "CAVV passed validation."
        },
        "errors": [],
        "messages": [
            {
                "code": "1",
                "description": "This transaction has been approved."
            }
        ]
    }
}
adavidw commented 7 years ago

Hi @alexsasharegan,

There are essentially two main responses to a card transaction. There's the API response (the first "responseCode" and "messages" elements in your example), and the transaction response (the "transactionResponse" element). transactionResponse in turn might contain other types of responses for specific situations like the AVS response and the CVV response.

It's a lot easier to troubleshoot something when you know which part of it failed, and this kind of granular response enables that. An API request is either going to succeed or fail, where success means "I got a properly formatted request, and I was able to perform actions on the data that it contained". Failure means "I could not perform any action on that request, either because I don't recognize the formatting as valid, or I've got a problem on my end, or whatever" with the details in the "messages" object.

There are a lot of usages of our API that aren't charging a card, so we need a standardized way to indicate success at the API level independent of whatever success would be at the card processing level.

The transactionResponse contains the actual disposition of the transaction itself, i.e. was the card authorized or not.

So, you really need to be testing first if your request was successfully processed by the API (is the resultCode = "Ok") and dealing with any failures there separately. Then, if the API request was successful and your input was sent off for action, test what the result of that action was (look for responseCode=1 within transactionResponse).

I agree it can be a little confusing, and if I could go back in time I would try to name things a little better (i.e. not "resultCode" and "responseCode"), but that's how it is for now.

We're always looking for ways to make the SDKs better, so if that means we have to abstract more in future versions so that responses no longer map to how the actual API is sending them, we'd do that if the net benefit was positive.

alexsasharegan commented 7 years ago

@adavidw ,

Thanks for the explanation!

I would say that understanding the API has a bit of overhead before productivity starts. I completely see now what you are explaining, and I was starting to understand that on my own. I don't hate that granularity at all—it's quite useful—but I almost wish your response here was included as part of the documentation. With an SDK this big, there can be a lot to digest at first, and you explanation would have helped me grock the method to the structure and choose an approach must faster.

That said, I still think the example code could be a bit clearer and use some more strict methods to handle the conditions. I think even commenting the code to basically include the gist of your explanation would be quite beneficial.

More abstraction could be worthwhile in the way of a response parsing class. Something JsonSerializable, ArrayAccessible, etc. to be flexible. Some convenient methods for things like: requestWasSuccessful(), transactionWasAccepted(), getTransactionFailureCode(), getTransactionFailureDescription(), toJson(), toArray(). This could really reduce the time to a successful integration as well as provide a canonical way to handle responses. You can build an Interface that all API responses adhere to for some of these methods, and then implement additional methods that pertain to the specific API actions requested.

Just some thoughts. I appreciate the well explained response!

adavidw commented 7 years ago

Thanks for the followup. I agree with everything you say, and I'm on a crusade to improve things, one little pain point at a time. Better documentation for the SDKs is a must, including better getting started instructions, as well as thorough commenting of the samples and the SDKs themselves.

At some point, though, we do have to allocate resources between improving existing efforts and starting new ones. Something like making the existing SDKs more abstract and JSON-ified can bring real rewards to developers. But, applying the same resources to creating, say, a new fully REST-ful API that's already better organized can bring a different set of rewards.

So, I can't promise which specific way we'll solve any particular problem or remove any particular pain point. However, I can promise we'll be always improving and making things better overall.

I'm going to leave this open as a reminder to me to clean up my previous explanation and try to get that into an appropriate place in the SDK to help others

alexsasharegan commented 7 years ago

@adavidw ,

I've been playing with my own ways to JSONify the response objects returned. Since each request returns a response object with different kinds of nested objects (and all with private props I can't json_encode), I would have to build individual response parsing objects for each request type. Obviously not ideal.

I played with some recursive reflection, and so far, the results are a perfect match to the "pretend" JSON shape of responses as shown in the reference docs. I do have to build the root-level response prop "messages" manually (still not sure why reflection doesn't catch that one), but that's not too big of a price to pay.

Normally, I would consider this kind of code a hack (I'm messing with hidden props), but since all these classes are built on the fly with yaml, I really don't see the harm in exposing the hidden props to an array. They all are built with getters and setters anyways.

I'll include the recursive reflection function if you want to take a look at it and weigh in on how good or bad of an idea this is.

<?php

function extractAllObjectPropsToArray( $object )
{
  $properties = [];
  $reflection = new ReflectionClass( $object );

  foreach ( $reflection->getProperties() as $property )
  {
    $property->setAccessible( TRUE );

    $propName  = $property->getName();
    $propValue = property_exists( $object, $propName )
      ? $property->getValue( $object )
      : NULL;

    switch ( TRUE )
    {
      case is_array( $propValue ):
        $properties[ $propName ] = [];

        foreach ( $propValue as $value )
        {
          $properties[ $propName ][] = is_object( $value )
            ? extractAllObjectPropsToArray( $value )
            : $properties[ $propName ][] = $value;
        }
        break;
      case is_object( $propValue ):
        $properties[ $propName ] = extractAllObjectPropsToArray( $propValue );
        break;
      default:
        $properties[ $propName ] = $propValue;
        break;
    }
  }

  return $properties;
}
alexsasharegan commented 7 years ago

I can't access root-level props like "messages" because they are private members of a parent class. I made some modifications to that function that invoke the parent class getters and then introspect the return value to get their values.

Rather than dump more code in here, let me include a link to the gist.

alexsasharegan commented 7 years ago

I've made some updates to that gist. Essentially now, it just recursively calls every getter on the object while setting the value to the method name without the get, and lower-cased on the first letter. It traverses up the inheritance tree as well.

Seems like the reverse-engineering of generated code...

adavidw commented 7 years ago

I don't have to much to offer specifically on your code, but you hit upon a good point about the generated code. There's a definite difference between an SDK that was designed and one that was generated. Ours looks too much like that latter. A well-designed SDK is created by people who think through how it's going to work and how people are going to use it. The SDK will reflect that with more abstracted functionality in the areas where it makes more sense, but still leave everything broken apart in the areas where that's appropriate.

I think our SDKs have often fallen short there, but the good news is that we're all on the same page here about the idea of what makes a good SDK, so any work on the next major revs of the SDKs should hopefully reflect that.

I fear people might get tired of me promising that the REST API will solve all the problems with the current API and the next version of the SDK will solve all the problems with the current ones. But, that's our intention here at least, to take all of the lessons learned from any problems we've caused in the past and rectify them for the future.

alexsasharegan commented 7 years ago

@adavidw ,

Wait, is this REST API production ready, or is this a coming-soon thing? I was under the impression that using this SDK (v1.9) was the current standard for processing through Authorize.Net. My goal here is to bring our company up to date with your most current tools (coming from v1.1) so we can start taking advantage of your newer payment methods and feature sets. Just want to make sure I didn't miss something!

adavidw commented 7 years ago

No, the REST API isn't production ready yet, but we expect a beta-level release fairly soon, and we'll definitely be in touch with you about that.

The latest SDK does support the latest released technologies on our side, but we'll rev the SDK to support the REST API. At that time, we'll take the opportunity to solve all of its other problems as well.

alexsasharegan commented 7 years ago

Thanks for the update! This is great news! I really appreciate your patience and understanding in all this. Please do reach out as release comes around!

praveenkumarS7 commented 6 years ago

When i'm trying to connect apple pay with my sandbox account,im getting an error "there was an error processing payment data invalid ownership".

ashtru commented 6 years ago

Hi @praveenkumarS7 Can you please provide more information about the error, including:

  1. Is the Apple pay account used by you a developer/sandbox account?
  2. Are you trying to connect your Apple pay with your sandbox account in merchant interface? Or, are you trying to make a transaction using the blob generated by your Apple Pay account?
  3. If you are using the API, what is the error code for the above error message? For this, you can share the response from your phplogfile for this purpose. It is generated in the same folder as your sdk path. In this file, you would see the response in a line containing : Response from AnetApi: <?xml version="1.0" encoding="utf-8"? The response will have your credentials masked, so it is okay to share.
  4. What are the steps you are using to generate the blob for Apple pay?
alexsasharegan commented 6 years ago

This issue has been open for far too long. It doesn't appear there's enough dev resources to act on this. I'm no longer even using authorize.net. If @praveenkumarS7 wishes to add information to his issue, I suggest opening a fresh issue. I'm closing this due to complete inaction.