Issues with contribution PayPal Improved

[bruce]

As noted in the installation notes, the callback section was not tested and asif has correctly noted the following error http://forum.opencart.com/index.php/topic,630.msg3709.html#msg3709 I would like to keep the posts for this in one place. Here if possible. So I will repeat his findings.

following code in callback() method

Code: Select all

$sql = "update `order` set `order_status_id` = '?' where `reference` = '?' and `order_status_id` = '?'";
$parsed = $this->database->parse($sql, $paidUnconfirmedStatus, $reference, $pendingStatus);

it is changing status from "Pending" to "PendingUnconfirmed", it should be changing from PendingUnconfirmed to Pending status
i think it should be...

Code: Select all

$sql = "update `order` set `order_status_id` = '?' where `reference` = '?' and `order_status_id` = '?'";
$parsed = $this->database->parse($sql, $pendingStatus, $reference, $paidUnconfirmedStatus);

1. another comment is that My site started workin fine with the slight change in Header creation for Paypal "post" as i mentioned in above thread. I get paypal verification even before customer get to success page sometime... in this case, Callback() method willl not find order with this reference n hence it will fail. and when customer hits success page then it will process the order n its status will be never changed.
2. so if we have to tackle this problem, we need 1 "if-else" each in process() and callback() methods. please if u got time do us a favour...or i'll try to make it if i got time...

from bruce
Hi asif. Thanks for the feedback. The race between paypal and your store to process the order still exists with the "if-else" you propose. Both paypal and your store could be trying to process the order at the same time, one deleting data that the other is trying to copy because the process function does not use locking or transactions. Your issue could be addressed, but the solution is not that simple.

However, the beauty of this mechanism is that there is no reliance on the callback. If the callback fails, the situation is exactly the same as if the callback does not come.

I have posted the bug fix that you provided http://www.opencart.com/contribution/info/category_id/2/contribution_id/33

Cheers

Bruce

[asif]

thnx for acknowledgin my comments.... oh yeh... i havn't think it that way... thats possible n if it can happen it will happen... n is it possible with simple locking..i think transactions will definitly work here as rest lockin has to be done by mysql...wat do u say???
n if u r workin on it...when it will be completed???

[gary]

the way to solve this - is to process order just BEFORE going to paypal. So that callback cannot in any way arrive before order is processed.
As I understand - next version of opencart will do it the right way....

[bruce]

I am looking forward to seeing how it will be done.

What you have suggested does eliminate the race condition but makes it difficult to handle the circumstance where the user fails to complete the payment. Now we have an order, but the customer cannot try to make the payment again because the order data is gone and the cart is empty.

Lets keep the new workflow and eliminate the race condition. First we need another table to hold order verifications. The callback can try to update the order status but if it fails then it puts a record in the verifications table. Second, we need some code to check for records in the verifications table, look for the processed order and update it's status, deleting the record in the verifications table on success. Finally, we need an event to trigger that code every so often.

[asif]

well i agree Bruce and its serious too bcoz store owner has an order but no money...and customer also hav an order in his order history too...
well i think we dont need any separate triger... There will be entry in varification table when we received an IPN. so in Process() method,

check the Varification table by Order reference
if we get an entry,
    this means IPN is already received, so order is processed with "Pending" status
else
    order is processed with "pending unconfirmed" , means we still didnt get any IPN, when IPN will be received, callback will change status

now there comes another possibilty, we received IPN and an entry in varification table but customer didnt come back to our success page, now we can check entries in varification table for deletion when store owner change any order from admin.

But adding another table and doin all this, why don't we use transcations in process and callback methods to first check the order entry and then change its status using lock "For Upadate" in both queries. I think we have to change the "database" class fot this and has to pass a connection variable for query too. pls also give feedback on trasaction method.

[gary]

I am looking forward to seeing how it will be done.

What you have suggested does eliminate the race condition but makes it difficult to handle the circumstance where the user fails to complete the payment. Now we have an order, but the customer cannot try to make the payment again because the order data is gone and the cart is empty.

Lets keep the new workflow and eliminate the race condition. First we need another table to hold order verifications. The callback can try to update the order status but if it fails then it puts a record in the verifications table. Second, we need some code to check for records in the verifications table, look for the processed order and update it's status, deleting the record in the verifications table on success. Finally, we need an event to trigger that code every so often.

order processing right now deletes order information from order_data table. Obviously we need to keep it until we confirmed that order was paid for - or it is canceled form admin. We should also allow customer to retry payment - so we need to keep order in his order history with status - "unpaid" (or something) until he pays - or until we cancel order.

[Saberu]

I'm having exactly the same issue, my customers are ordering products through Paypal but the orders aren't showing up in the admin panel OR the database so I have no idea whats being ordered and my only option is to contact them and ask what they ordered :/

Also it appears to be completely random, for example one order showed up but another 2 didn't etc.

If anyone has any way of fixing this please tell me, i'm desperate!

[bruce]

Are you using the paypal extension that comes with OpenCart, or the modified one from Contributions?

[D4T]

Hi,

I am having problems with the improved paypal extension too, my problem is that if the user pays and gets the paypal confirmation page then quits there browser before they are redirected back to my site I end up with no order in the Admin -> Customer -> Order even though the Paypal callback to the site has been successful.

Maybe I am doing something wrong but I cannot see what I have done if that is the case.

Also I am no PHP man unfortunately, but in your process() function I cannot see in the code where the order status is updated to "Paid Unconfirmed". Maybe you can point this out to me as I only see a sql select and no update?

[asif]

Hi,

I am having problems with the improved paypal extension too, my problem is that if the user pays and gets the paypal confirmation page then quits there browser before they are redirected back to my site I end up with no order in the Admin -> Customer -> Order even though the Paypal callback to the site has been successful.

Maybe I am doing something wrong but I cannot see what I have done if that is the case.

Also I am no PHP man unfortunately, but in your process() function I cannot see in the code where the order status is updated to "Paid Unconfirmed". Maybe you can point this out to me as I only see a sql select and no update?

yeh it will not insert the record even if u get a postback from PayPal in the case when ur customer closed the browser before getting back to ur site. It is so because Process() function gets executed when user hits ur site after paying at PayPal, and in Process() function it inserts the order with paid_unconfirmed status. When u receive postback from paypal, callback() gets exeecuted and it just changes the order status. So if customer didnt hit back, there will be no order in the database, so nothing will be updated with Paid status. Yes, its an issue with this contribution. Bruce may be workin on this issue to improve...

[Saberu]

Use my temporary fix which will allow you to track all purchases through paypal if used in conjunction with the paypal email you receive:
http://forum.opencart.com/index.php/topic,942.0.html

It's really basic and isn't a proper modification of the cart but I'm sure bruce could easily modify it to be so, I just don't have time to do so.

[D4T]

Hi,

I am having problems with the improved paypal extension too, my problem is that if the user pays and gets the paypal confirmation page then quits there browser before they are redirected back to my site I end up with no order in the Admin -> Customer -> Order even though the Paypal callback to the site has been successful.

Maybe I am doing something wrong but I cannot see what I have done if that is the case.

Also I am no PHP man unfortunately, but in your process() function I cannot see in the code where the order status is updated to "Paid Unconfirmed". Maybe you can point this out to me as I only see a sql select and no update?

yeh it will not insert the record even if u get a postback from PayPal in the case when ur customer closed the browser before getting back to ur site. It is so because Process() function gets executed when user hits ur site after paying at PayPal, and in Process() function it inserts the order with paid_unconfirmed status. When u receive postback from paypal, callback() gets exeecuted and it just changes the order status. So if customer didnt hit back, there will be no order in the database, so nothing will be updated with Paid status. Yes, its an issue with this contribution. Bruce may be workin on this issue to improve...

In my opinion thats not improvement over the Opencart default paypal module and in fact is somewhat worse.

Care you weigh in Bruce?

[asif]

actually it was to cover the problem when PayPal callback is not recevied or it is late. In this case, order is atleast placed when customer hits back at ur site. Its like loose one thing to get another...can't have both at a time till we implement something using locks or Transaction in this page.
btw may bruce will give an appropriate anser...

[bruce]

I am having problems with the improved paypal extension too, my problem is that if the user pays and gets the paypal confirmation page then quits there browser before they are redirected back to my site I end up with no order in the Admin -> Customer -> Order even though the Paypal callback to the site has been successful.

The customer has gone to another web site (paypal) to make the payment. If they do not return, there is no way of knowing what they have done. If you try this yourself with the paypal sandbox, you will probably find that your shopping cart is not cleared either. This scenario is a genuine issue with the "improved" workflow. The original does not have this problem *only* if the callback succeeds and I remind you that the reason for producing this example was to demonstate a way of avoiding order losses in the case of callback failure. It also came with the warning that it was not tested.

A partial solution to customers not returning is to to have the callback function look to see if the order is processed and if it is not, then do so if the payment is verified. But, again, the unreliability of the callback initiated this contribution.

What Saberu has implemented has merit too because it generates an audit trail of potential order activity every time the customer visits the checkout_payment screen. At least this way, the paypal email and audit trail can be compared.

Also I am no PHP man unfortunately, but in your process() function I cannot see in the code where the order status is updated to "Paid Unconfirmed". Maybe you can point this out to me as I only see a sql select and no update?

The select is getting the value for "Paid Unconfirmed" which is passed as a parameter to order->process as shown below.

Code: Select all

        $parsed = $this->database->parse($sql, $this->language->get('order_status_paid_unconfirmed'), $this->language->getId());
        $results = $this->database->getRow($parsed);
        if ($results)
        {
            $this->order->process($results['order_status_id']);
        }
  

I am sorry but I have no plans to make this contribution bulletproof. I do not have any spare time and hence am waiting for the 0.8 version before reviewing paypal payments again.

If you have more customers not returning from paypal than failed callbacks, then you should revert to the original paypal payment extension as the lesser of two evils.

[D4T]

The customer has gone to another web site (paypal) to make the payment. If they do not return, there is no way of knowing what they have done.

I understand that but then why ignore the IPN from Paypal? Surely there is a way of ensuring that 1)If customer comes back to site the order is processed 2)If customer doesn't come back to site but IPN received then the order is processed?

I don't see how that is such a complex task tbh, maybe I will learn more PHP

Either way I appreciate your responses, I know its more then likely that I'm wasting your time 

[bruce]

You are right, making use of the IPN is not difficult. Here is a patch with some surrounding code for reference. The function is callback() in the paypal "improved" code. This will reduce your pain.

Note also that in the case of the customer not returning AND the paypal IPN failing, the order data has already been saved in the table order_data. Saberu, this means you can remove your extra code from getMethod and look in the order_data table instead. If the email from paypal contains the order reference, then all that is required now is for orders administration to be able to retrieve an order from order_data using the reference in the paypal email and have the code call order->process(). This task I will leave to one of you.

Code: Select all

                $sql = "select `order_status_id` from `order_status` where `name` = '?' and `language_id` = '?'";
                $parsed = $this->database->parse($sql, $this->language->get('order_status_pending'), $this->language->getId());
                $results = $this->database->getRow($parsed);
                if ($results)
                {
                    $pendingStatus = $results['order_status_id'];
                }
                //
                //  Perhaps the IPN from paypal has arrived before the user has returned from
                //  the paypal web site OR the user has not returned at all.
                //  In response to either case, we should load and process the order.
                //            
      $reference = $this->request->get('invoice', 'post');
                if ($this->order->load($reference))
                {
                    //  IPN is winner or customer never returned and IPN arrived.
                    //  default status should be Pending so the update below will have no
                    //  effect if we process the order here but the end result is still correct.
                    $this->order->process();
                }
               
                $sql = "update `order` set `order_status_id` = '?' where `reference` = '?' and `order_status_id` = '?'";
                // bug fix from asif
               //$parsed = $this->database->parse($sql, $paidUnconfirmedStatus, $reference, $pendingStatus);
                $parsed = $this->database->parse($sql,  $pendingStatus, $reference, $paidUnconfirmedStatus);
                $this->database->query($parsed);
         }



A final warning on the strength of the paypal solution we have built here.

We still have the customer return from paypal and the IPN from paypal racing to complete order->process() and potentially starting it at almost the same time. This is bad (however unlikely) because order->process() contains multiple sql steps without transactions OR even more importantly, race condition protection in the form of a semaphore or perhaps some form of sql locking. I have never used semaphores with php or explicit row locking with mysql so if any of you has this experience, here is the chance to shine.

***  Note that you are responsible for testing this too as I cannot receive callbacks from paypal. So please do not just throw this into a production system and expect it to work.

[Saberu]

It works! It actually records the order when it's made, thanks Bruce your a life saver!

I'm still a little confused as to why such an essential part of the Paypal system wasn't originally there anyway though, as it's more important to register orders than worry about doing it the 'exact way'. I'm not too bothered about the status auto changing from pending to paid because thats something I can easily do myself but obviously it's something to look forward to in version 0.8.

[D4T]

Hi Bruce,

Thanks for that, its greatly appreciated

So in the worst case scenario are you suggesting we could end up with duplicate orders?

[bruce]

My pleasure.

The two bad scenarios are:

• Customer pays and does not return and the IPN fails => Customer cart is still intact and order is still in order_data ie not processed.
• Customer pays and returns and the IPN returns at the same time => This could get messy but duplicates are a possiblity.

It would be a very good idea to wrap the order->process sql statements in a database transaction.

Code: Select all


   function process($order_status_id = NULL)
   {
      if ($this->data)
      {

         $database->query( "START TRANSACTION" );

         // existing code here... except for email send

         $database->query("COMMIT;");

         // move the email  send to here because we do not want errors in email to rollback the database work
      }
   }



[asif]

hi bruce..efforts appreciated but transaction on this wouldn't work because the table types are ISAM. Transactions only works on InnoDB type tables in MySQL. I think of this before but didn't code because of this problem. I didnt change the table types as i wasn't aware of the effect overall. As InnoDB doesnt commit any SQL if they are not autocommit or explicitly write "commit" command. If u know all this then post it...

i only know that we have to change the table types to InnoDB for those we want transactions to work. One possibility can be that we change the SQL scripts which make the tables during opencart installation and make them InnoDB(only for those table which r in transaction). Secondly,execute "set AutoCommit=1" after creation of tables. now why i m enabling autocommit is that otherwise we have to write "commit" after each sql and that will be difficult to change in many pages. so during Process(), disable autocommit by setting it to 0 and then your code will go there...
as far as i know, semaphore woudn't work here...transaction are the best way...lets explore and do it...it will be realy appreciatbale if Daniel cover this problem in version 0.8.