I'm currently testing Express Checkout using the PayPal sandbox but when transactions undergo a payment review, PayPal sends an IPN with the same txn_id as the subsequent IPN.

See below IPN chain:

payment_status=Pending
txn_id=6M061502SS003073U
pending_reason=paymentreview

payment_status=Pending
txn_id=6M061502SS003073U
pending_reason=authorization

payment_status=Pending
txn_id=5U160197RA036471T
pending_reason=paymentreview

payment_status=Completed
txn_id=5U160197RA036471T

This causes non-paymentreview IPN's to fail validation because the validate() function in PayPalPaymentIPNController.inc checks whether the txn_id has already been processed before.

The end result is that the payment never completes in the Drupal site because the completion IPN is never processed. Payments just remain stuck with a status of "Being reviewed for risk".

CommentFileSizeAuthor
#10 2550693-10-paypal_payment_ipn-per-psiid.patch11.93 KBtorotil
#8 2550693-8-paypal_payment_ipn-per-psiid.patch12.35 KBtorotil
#7 2550693-7-paypal_payment_ipn-per-psiid.patch11.51 KBtorotil
#4 paypal_payment-ipn-validation-2550693-3.patch7.29 KBmorbiD
#3 paypal_payment-ipn-validation-2550693-2.patch7.24 KBmorbiD
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

morbiD created an issue. See original summary.

morbiD’s picture

morbiD CreditAttribution: morbiD commented

As far as I can see, there is no single identifier in an IPN that can be reliably used to test if the IPN has been processed already.

txn_id can be shared by multiple IPN's and so, apparently, can ipn_track_id (ignoring the fact that it's documented as being for internal use only). I can't find any other suitable parameters either.

It seems the only reliable way would be to track some combination of parameters such as txn_id, payment_status and pending_reason, which I guess would require modifications to the database schema.

morbiD’s picture

morbiD CreditAttribution: morbiD commented
Status: Active » Needs review
FileSize
paypal_payment-ipn-validation-2550693-2.patch7.24 KB

I would tentatively say that this patch seems to solve the problem for me, but I can't verify that the modified tests work since I seem to be having issues with drupal_http_request() on my server. Edit: looks like they do!

However, the problem this patch presents is that once the new columns are added to the database table, the existing table rows will be useless for validation and due to #2555297: IPN's always have pid's of 0 in database I don't think there's even a way to trace the old rows to specific payments and update the payment statuses in the rows accordingly.

morbiD’s picture

morbiD CreditAttribution: morbiD commented
FileSize
paypal_payment-ipn-validation-2550693-3.patch7.29 KB

Previous patch didn't contain documentation for the hook_update_N() implementation so here's an updated version.

torotil’s picture

torotil CreditAttribution: torotil at more onion commented

IMHO each paypal_payment_ipn should be linked to a payment_status_item [that should be created in the IPN request]. Then the whole issue is moot. Having the pending reason in there is nice anyway.

morbiD’s picture

morbiD CreditAttribution: morbiD commented
Status: Needs review » Needs work

@torotil: I haven't done any work on this for nearly 18 months so I'm not that familiar with it any more, but I think you're right.

In fact, a couple of months after I wrote the patch in #3, I actually ended up linking paypal_payment_ipn and paypal_status_item together for #2569467: Record and display refund amounts, so your suggestion makes even more sense.

If I get a chance I'll try and update this issue with a new patch at some point.

torotil’s picture

torotil CreditAttribution: torotil at more onion commented
Status: Needs work » Needs review
FileSize
2550693-7-paypal_payment_ipn-per-psiid.patch11.51 KB

Here is my first attempt on implementing this. It changes paypal_payment_ipn to have the psiid as primary key. This makes it possible to have multiple IPNs per payment. For the sake of simplicity I've ignored #2569467: Record and display refund amounts for the while.

torotil’s picture

torotil CreditAttribution: torotil at more onion commented
FileSize
2550693-8-paypal_payment_ipn-per-psiid.patch12.35 KB

After some more testing:

  • Fix a small mistake in the migration.
  • Also adapt paypal_payment_pps_return()
morbiD’s picture

morbiD CreditAttribution: morbiD commented

Not sure there's any point in keeping the pending_reason column in paypal_payment_ipn.

If the IPN's status is "Pending" then payment_status_item.status will be derived from PayPalPaymentIPNController::pendingStatusMap() rather than PayPalPaymentIPNController::statusMap(), which means the paypal_payment_ipn's pending_reason can always be inferred from the linked payment_status_item if I'm not mistaken.

torotil’s picture

torotil CreditAttribution: torotil at more onion commented
FileSize
2550693-10-paypal_payment_ipn-per-psiid.patch11.93 KB

Yes, I agree. The information is redundant (except for 'authorization' and 'order' which point to the same status).

Here is an updated patch without that column. I've also added an additional update that deletes all the items with pid=0 from the table as they are pretty useless anyway. Without removing them we'd have duplicate entries for the new psiid primary key.

  • torotil committed a4a3093 on 7.x-1.x 
    Issue #2550693 by torotil, morbiD: Fix bogus entries in {...
torotil’s picture

torotil CreditAttribution: torotil at more onion commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.