Automatically fill transactionId

[barryvdh]

I don't know what the 'common' way to do this in Omnipay, but shouldn't the transactionId be set from the request? Or how should you do it? See this example, that also uses the request data: https://github.com/omnipay/paypal/blob/master/src/Omnipay/PayPal/Message/ExpressCompleteAuthorizeRequest.php

This commit should check if the transactionId is not set, and only when the get value is not null/false, set the transactionId. So it should not break existing behavior.

[amacneil]

Generally with Omnipay you should use the transactionId in your returnUrl, so that you can load the transaction details and pass them to the completePurchase() method. Not every gateway returns the transaction ID in the query string as targetpay does.

[barryvdh]

But the transactionId isn't something I set or control, the reference is automatically added to the return url (/gateways/{name}/completePurchase?trxid=30626804185492). So if I have multiple gateways, I have to check of it is a Omnipay\TargetPay\Message\CompletePurchaseRequest; and then add the parameter?

$params = array(
    'transactionId' => $_GET['trxid'],
);
$response = $gateway->completePurchase($params)->send();

I thought it omnipay would make it possible to use (sort of) the same implementation for every gateway, but this way you have to check for every implementation. Or is that meant to be?

[amacneil]

In Omnipay, transactionId referes to an ID which you generated (for example your database primary key), and transactionReference refers to an ID which was generated by the payment gateway. So in the case above, if 30626804185492 was indeed generated by Targetpay and not yourself, then Omnipay would refer to that as the transactionReference.

On 1 April 2014 23:23, Barry vd. Heuvel notifications@github.com wrote:

But the transactionId isn't something I set or control, the reference is automatically added to the return url ( /gateways/{name}/completePurchase?trxid=30626804185492). So if I have multiple gateways, I have to check of it is a Omnipay\TargetPay\Message\CompletePurchaseRequest; and then add the parameter?

$params = array( 'transactionId' => $_GET['trxid'], ); $response = $gateway->completePurchase($params)->send();

I thought it omnipay would make it possible to use (sort of) the same implementation for every gateway, but this way you have to check for every implementation. Or is that meant to be?

Reply to this email directly or view it on GitHubhttps://github.com/omnipay/targetpay/pull/1#issuecomment-39218427 .

[barryvdh]

Okay, so it is not correct to use transactionId here: https://github.com/omnipay/targetpay/blob/master/src/Omnipay/TargetPay/Message/CompletePurchaseRequest.php#L20 In the PurchaseRequest, the same value is referenced as TransactionReference: https://github.com/omnipay/targetpay/blob/master/src/Omnipay/TargetPay/Message/PurchaseResponse.php#L57

So should I change the PR to something like this?

public function getData()
{
     if(!$this->getTransactionReference() && $trixId = $this->httpRequest->query->get('trxid')){
        $this->setTransactionReference($trixId);
    }

    $this->validate('transactionReference');

    return array(
        'rtlo' => $this->getSubAccountId(),
        'trxid' => $this->getTransactionReference(),
        'once' => $this->getExchangeOnce(),
        'test' => $this->getTestMode(),
    );
}

(But that could break existing implementations possibly?)

[amacneil]

I didn't write this gateway so I'm not entirely familiar with the targetpay API. Is trxid definitely generated by the gateway itself?

You should never call setSomething() inside the getData() method. However, if the above is true then it should probably be changed to this:

public function getData()
{
    return array(
        'rtlo' => $this->getSubAccountId(),
        'trxid' => $this->httpRequest->query->get('trxid'),
        'once' => $this->getExchangeOnce(), // also, do you know where this value comes from?
        'test' => $this->getTestMode(),
    );
}

[amacneil]

Ping @aderuwe any input on this?

[barryvdh]

As far as I can see from the documentation (https://www.targetpay.com/docs/TargetPay_iDEAL_V1.0_en.pdf), the documentation refers to an transactionId (trixd), which is automatically returned first when requesting the purchase link (chapter 3.2):

When the transfer is prepared succesfully you'll receive a result in the format: <responsecode><space><transactionID><pipe><url>

The same value is appended to the return url (chapter 4)

If customers have finished paying or press 'cancel' during the banking process, they will automatically be redirected to your site (to the return URL you supplied). An additional parameter 'trxid' will be supplied, which contains the transaction ID. This is the same as in the previous step.

That transactionId is used to get the purchase status (chapter 5.1).

once is a mandatory field to define wether we can ask the same result multiple times. I don't know if such a feature exists in Omnipay, but the suggested value (when in doubt) is 1 (so the result only returns OK the first time it is requested). So it would make more sense that once defaults to 1 (which isn't the case right now I think)

If you supply '1' our application will only return the 'OK' status once. If you'd request the URL again with the same Transaction ID you'll receive the TP00014 (already used) response code. If you'd used '0' our application will keep returning 'OK' for each request with this Transaction ID. If you doubt on this value, use '1' to be sure your application won’t deliver the service or product twice if the visitor would Refresh his browser window.

The confusion is probably because the documentation speaks of a TransactionId every time, but doesn't have the same meaning in Omnipay terminology.

[amacneil]

Ok cool. I suggest we implement the fix I described above then. We should automatically retrieve values from the GET/POST data in a CompletePurchaseRequest, not require the user to submit them.

Given your description above I think the once field makes sense how it is. I don't think it's a major issue what the default is (I'd rather Omnipay just follows the same defaults as the gateway), and as long as your application properly marks orders as paid in the database, duplicate transaction checks aren't an issue.

On 2 April 2014 15:23, Barry vd. Heuvel notifications@github.com wrote:

As far as I can see from the documentation ( https://www.targetpay.com/docs/TargetPay_iDEAL_V1.0_en.pdf), the documentation refers to an transactionId (trixd), which is automatically returned first when requesting the purchase link (chapter 3.2):

When the transfer is prepared succesfully you'll receive a result in the format:

The same value is appended to the return url (chapter 4) If customers have finished paying or press 'cancel' during the banking process, they will automatically be redirected to your site (to the return URL you supplied). An additional parameter 'trxid' will be supplied, which contains the transaction ID. This is the same as in the previous step. That transactionId is used to get the purchase status (chapter 5.1). once is a mandatory field to define wether we can ask the same result multiple times. I don't know if such a feature exists in Omnipay, but the suggested value (when in doubt) is 1 (so the result only returns OK the first time it is requested). So it would make more sense that oncedefaults to 1 (which isn't the case right now I think) If you supply '1' our application will only return the 'OK' status once. If you'd request the URL again with the same Transaction ID you'll receive the TP00014 (already used) response code. If you'd used '0' our application will keep returning 'OK' for each request with this Transaction ID. If you doubt on this value, use '1' to be sure your application won't deliver the service or product twice if the visitor would Refresh his browser window. ## Reply to this email directly or view it on GitHubhttps://github.com/omnipay/targetpay/pull/1#issuecomment-39295696 .

[barryvdh]

Ok. Let's wait for @aderuwe for a while, otherwise I'll submit a new PR soon.

[aderuwe]

It's been a while since I worked on this. I can't see anything wrong with changing things as described. I think I made the conceptual error of referring to this trxid as transactionId in requests and transactionReference in responses, regardless of the intended way (id is ours, reference is theirs).

I won't be able to verify things or make sure BC is maintained, as I don't use this gateway anywhere anymore.

[amacneil]

Ok, happy to make this change then if you can update the PR.

I don't think BC is an issue since we're simply switching to automatically reading the txnid instead or requiring a passed variable.

On 2 Apr 2014, at 3:50 pm, Alexander Deruwe notifications@github.com wrote:

It's been a while since I worked on this. I can't see anything wrong with changing things as described. I think I made the conceptual error of referring to this trxid as transactionId in requests and transactionReference in responses, regardless of the intended way (id is ours, reference is theirs).

I won't be able to verify things or make sure BC is maintained, as I don't use this gateway anywhere anymore.

— Reply to this email directly or view it on GitHub.

[barryvdh]

I submitted a new PR in #2