Refactor the Paypal WPS IPN code

Please note this related issue:
#849350: paypal refund status / transition, comment #4

Since this feature won't break anything currently available in D6 release i strongly vote for its consideration for the 6.x-2.x branch. Any chance to make this in?

One option would be a more generic hook like:

module_invoke_all('uc_payment_transaction', 'paypal', $ipn->txn_type, $ipn, $order);

instead of

module_invoke_all('uc_paypal_ipn_transaction', $ipn->txn_type, $ipn, $order);

@miro_dietiker: see uc_payment_enter():

module_invoke_all('uc_payment_entered', $order, $method, $amount, $account, $data, $comment);

I'm pretty sure this covers what you want to do.

Damien: No it does not.
There are certain notifications from a CC gateway that should trigger a hook but don't change balance directly.
(e.g. authorization of a payment only - NOT settlement)...
You find in the paypal implementation (patch) that for every cc trigger a hook is called.

univate: Right ipn is custom per cc provider. But the mechanism is always the same and we get a form submission in POST. Some others even name it also IPN.

If we don't provide full hooks, we're limited in extensibility and thus limit uc.

This sounds like a good patch to get in, but no one who uses PayPal has taken the trouble of reviewing it yet. I'm not going to commit a patch that hasn't had some review...

(note : I have raised my issue in a new node - http://drupal.org/node/973832 - in case it is not relevant in this thread : )

Hi

I'm at the point of using this patch .. here is my problem ..

Setup :D6 + UC + PP sandbox with auto return on.
Only products are file downloads.

Scenario 1:
A customer completes checkout and is taken to PayPal.
Log in .. complete payment .. wait and PP directs them back to a download page.

Scenario 2:
Same as above EXCEPT .. when completing payment, get taken to PP redirect page and click the 'click here to be redirected to site' BEFORE PP auto redirects. Still landing on download page.

The results in the order logs are different !? Different sequence and changes !? See below :

Scenario 1:
Tue, 2010-11-16 15:16 -
PayPal payment for £0.50 entered by -.
Tue, 2010-11-16 15:16 -
Order status changed from In checkout to Pending.
Tue, 2010-11-16 15:16 -
Order status changed from Pending to Completed.

Scenario 2:
Tue, 2010-11-16 15:23
User 34 Order status changed from In checkout to Pending.
Tue, 2010-11-16 15:23 -
PayPal payment for £1.00 entered by -.
Tue, 2010-11-16 15:23 -
Order status changed from Pending to Payment received.
Tue, 2010-11-16 15:23 -
Order status changed from Payment received to Completed.

I'm assuming that the IPN is getting screwed somewhere but not sure where .. it's obvioulsy a cause of the time the process is being given ?

Will this patch help here .. ?

Thanks
G.

Just want to +1 the idea of architectural cleanup related to PayPal notifications. I just burned a ton of time getting my store happy with recurring transactions: Paypal WPS: Order status inconsistent, sometimes set to "complete" sometimes "payment received"

I ended up hacking at both Ubercart and uc_recurring code to get a happy result. If there's a way to refactor things so that modules like uc_recurring can be on a more solid footing, I'm all for it.

My available time for Ubercart-related coding is limited. But if it'd advance the suggested improvements I'd be happy to throw together a bare-bones dev site and do some quick testing. The original issue and patch were posted back in April, so I'm a little unsure at this point what would be helpful. Can I apply the originally posted patch to the current Ubercart 6.x-2.x-dev (2010-Dec-10) and (if so) what should I look for to know if it is doing its job?

sub...

subscribe

Reroll of #2 for 7.x-3.x as it is now. Fixed the broken braces in a couple of places, and added documentation for the new hook.

I have to go dig up my sandbox credentials again to test this, but if someone will beat me to it to test this, it won't hurt my feelings.

As long as you're working on IPN, here are three other issues that look relevant and should be considered as part of the cleanup:

#1307954: Store more response data in uc_payment_paypal_ipn
#952648: Paypal response not checked for failure when submitting express checkout
#467456: Paypal pro mixes up server settings with paypal standard

I can't even get Sandbox IPNs to find my test site, so I don't know how much good I can do with this issue, much less those three. http://lyle.ubercart.org/seven resolves for you doesn't it? I don't think I have any funky DNS issues to make it work.

@Island Usurper

It resolves in DNS to 66.93.231.248, but doesn't respond to HTTP requests.

Hmph. I bet we've got that server behind a firewall. And that's not something I'm going to mess with right now. Blargh.

Retesting just for fun, to make sure the tests still run green.

#17: 781720_paypal_ipn.patch queued for re-testing.

No bug report here; the table rename mentioned in #3 doesn't seem to be valid, the table has never been called 'uc_paypal_ipn' that I can see.

I think it is still safe to refactor this now without breaking contrib.

#17: 781720_paypal_ipn.patch queued for re-testing.

Reposting from https://www.drupal.org/node/2522014 at suggestion of TR.
----------
We encountered a situation where a customer reversed a PayPal payment, disputed payment, then cancelled the reversal, but the dispute and reversal cancellations were never recorded in Ubercart.

The attached PDF shows a screen grab from the watchdog log (newest at top), showing the original order, the reversal, a posting of a dispute, and the cancellation. You can see that on the latter two, watchdog posted "IPN transaction ID has been processed before."

The relevant code in file uc_paypal.pages.inc is this (beginning at line 92), which happens BEFORE any IPN processing takes place:

    $duplicate = (bool) db_query_range('SELECT 1 FROM {uc_payment_paypal_ipn} WHERE txn_id = :id AND status <> :status', 0, 1, array(':id' => $txn_id, ':status' => 'Pending'))->fetchField();
    if ($duplicate) {
      if ($order->payment_method != 'credit') {
        watchdog('uc_paypal', 'IPN transaction ID has been processed before.', array(), WATCHDOG_NOTICE);
      }
      return;
    }

Clearly, if 'IPN transaction ID has been processed before.' is getting posted to the watchdog, there is no further processing of the order, which means that the program flow never gets down to the case 'Canceled_Reversal' at line 114 that is supposed to handle the reversal.

In addition to ignoring the Canceled_Reversal IPN action, it looks like there is no code branch to handle the posting of the dispute, even if the duplicate check hadn't eliminated it.

The duplicate-checking code above seems to ignore any IPNs with the same txn_id if the order status is anything but Pending. But clearly, reversals, reversal cancellations, refunds, and disputes can all happen with Completed orders. So it would seem that the duplicate-checking code logic should be a bit more complex than the above.
---------
Further commenting on this issue:

Patches #1, #2, and #17 don't apply cleanly to current 7.x-3.x-dev (perhaps not surprisingly, given their age). If I can figure out what still needs to be applied, I'll roll a new one.

We've patched our local installation to add Rules events for each Paypal IPN transaction, to make it possible, for example, for the store manager to get an email notification if a payment is reversed. (Which was how we discovered the issue above; one of the events wasn't firing.)

If the fundamental question is "how does one gracefully add additional response to PayPal IPN transactions?", univate's solution is to add hook_uc_paypal_ipn_transaction(). That surely works, but an alternative might be to create Rules events (which is what we've done in our local patching). Since Ubercart already uses Rules extensively, it seems like Rules might be a more fitting solution; also, it allows non-programmer we admins to add actions via the Rules interface, e.g., sending a notification email. So I would suggest that creating Rules events that pass both the order and ipn as variables might be more appropriate. Is that agreeable? (If so, I'll work on it.)