bitpay / zencart-plugin

Zencart payment plugin for Bitpay.com
MIT License
16 stars 37 forks source link

callback is not working #17

Closed CcMarc closed 8 years ago

CcMarc commented 8 years ago

The bitpay callback is not working correctly, it is taking the very first zencart orderid and changing the status of that order instead of the correct orderid associated with the bitpay order.

kleetus commented 8 years ago

Thanks for the report:

Would you, kindly, specify the versions of the following:

  1. Zencart
  2. BitPay plugin for Zencart, tag or sha
  3. PHP
  4. OS version and patch level (this is just to be able to reproduce if we can't from numbers 1-3).

thanks,

CcMarc commented 8 years ago

Zencart - v1.5.4 BitPay plugin for Zencart, tag or sha - not sure what your asking I'm using the version on here PHP - v5.6.11-1~ OS - debian wheezy

kleetus commented 8 years ago

ok, not a problem regarding the version, sha or tag. I'll assume that you used the zip file from:

https://github.com/bitpay/zencart-plugin/releases/tag/v1.0.0

I'll look into this callback issue right away.

CcMarc commented 8 years ago

Yes I downloaded it from here but the version I have is slightly different then the one from the URL you posted above. Mine is: https://github.com/bitpay/zencart-plugin/archive/master.zip

kleetus commented 8 years ago

TL;DR:

2 things need to happen:

  1. I need to add some defensive coding in the after_process function to bail out if $insert_id is null. But, this won't fix this issue for you in order to process orders.
  2. $insert_id global ought to set by Zencart. Apart from relying on after_order_create to get this ID, this is the only way to know what order needs to get processed. The built-in payment modules, authorize.net and others rely on the $insert_id global in after_process. This leads me to believe that something is not properly setting $insert_id before the BitPay plugin is accessed. Maybe you aren't using the same checkout process flow in order to call BitPay after_process.

upon creation of the BitPay invoice:

$db->Execute("update ". TABLE_ORDERS. " set orders_status = " . intval(MODULE_PAYMENT_BITPAY_UNPAID_STATUS_ID) . " where orders_id = ". intval($insert_id));

handling of the ipn callback from BitPay's webhook servers:

$db->Execute("update ". TABLE_ORDERS. " set orders_status = " . MODULE_PAYMENT_BITPAY_PAID_STATUS_ID . " where orders_id = ". intval($order_id));

$insert_id and $order_id in the SQL will be the same value and both come from the global $insert_id.

Based on what you've said about the plugin taking the first $order_id and changing the status instead of the correct order, I would conclude that $insert_id is most likely null and therefore intval(null) is 0. This means that under these conditions, the plugin would create new BitPay invoices with the posData value as null. When the ipn happens the null value in posData is then used to update the order table where orders_id=0.

So, this whole work flow relies on the global, $insert_id. This should get set in checkout_process.php (a zencart script) which is required in header_php.php before the call to bitpay after_process function, which needs the right $insert_id.The global $insert_id should be set in the global scope on line 86 of check_process.php.

CcMarc commented 8 years ago

I'm not much of a coder so not sure what this means, however if it helps my site uses authorize.net module which works just fine.

CcMarc commented 8 years ago

Also to be clear when I say the callback not working I'm referring to the bitpay_callback.php. Orders are being created just fine, the issue is the order status of a paid order is not getting updated properly from pending to processing when Bitpay call's back to my website.

kleetus commented 8 years ago

Thanks for the clarification. I do realize that you are referring to bitpay_callback.php.

I will get right back to you on this issue.

kleetus commented 8 years ago

please see this release

https://github.com/bitpay/zencart-plugin/releases/tag/v1.0.1-beta

I've repaired the issue, thanks for point it out, it is pretty serious to get that repaired.

CcMarc commented 8 years ago

Ok installed I will report back if any issues thanks for getting that resolved so quickly!

CcMarc commented 8 years ago

Question in file bitpay_callback.php you have the following:

    case 'expired':
        if (true === function_exists('zen_remove_order')) {
            zen_remove_order($order_id, $restock = true);
        }
        break;

how does that work, if it is expired Bitpay for what I understand does not callback letting us know this so this function never runs. I have always removed expired orders manually but if it can be automatic that would be nice. Just curious if there a was actually a way to make this work?

kleetus commented 8 years ago

Yes, thanks for the question about this. This is not very clear how this could work, but here is why this bit of code needs to there.

  1. Zencart can get the "expired" ipn state if the invoice is created with "extendedNotifications = true". Granted this plugin doesn't do this, so you will never get this case, but this handler was added anyway. Further, extendedNotifications isn't even officially documented, so how anyone would to handle this case is somewhat of a mystery. i would say that it is a useful stub for someone needing to auto-remove the order. You would need to edit the bp_lib.php and just add "extendedNotifications" key and value.
  2. the 'zen_remove_order' function scope is highly dependent on the version of Zencart. For example, older versions exposed this function to payment plugins, but in later versions, this function is buried in the admin section behind a directory name that is obfuscated and generated at zencart install time. There is probably an api for this callback script to find this function, but I haven't looked into it. Removing orders is pretty heavy duty and can be dangerous. If you really need this functionality, I can add it, but it will always be fairly dangerous I would think.
CcMarc commented 8 years ago

I was just curious it is not necessary and I agree that would be a dangerous feature. I guess the real issue is the order being created before a verified payment has even been made. Now PayPal module does not create an order until payment is verified.

Suggestion: How about do whatever the PayPal module is doing because when when a customer leaves the zencart site and goes to PayPal to send their Payment an order is not yet created. I believe the order is created once that payment is made and the customer RETURNS to the zencart site but in BitPay's module case have the call back be the trigger to create the order. That way if a payment is not made then the order will never be created.

kleetus commented 8 years ago

I think the Paypal payment gateway and the BitPay payment gateway work similarly with respect to cleaning up orders if a payment notification advertises a certain known state. I quickly reviewed "paypal.php" in the admin directory of my zencart test machine and it appears to handle the ipn state change there. Maybe placing the bitpay_callback.php within that directory would allow the call of zen_remove_order function. This would assume that you are creating the BitPay invoice with extendedNotifications = true. Either way, it would be up to you or your developers on how you want to implement this.