-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Webhook Support #18
Webhook Support #18
Conversation
razorpay-payments.php
Outdated
'description' => esc_url( admin_url('admin-post.php') ) . '?action=rzp_webhook', | ||
'label' => __('Enable Razorpay Webhook with the URL listed below.', 'razorpay'), | ||
'default' => 'yes' | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a webhook_action
here that decides what we do with the order on payment success. There are multiple scenarios:
- Late Authorized (we get payment in authorized status, let the store owner decide)
- payment.failed (we can mark as failed) (very tricky, let us leave)
- Actual authorized, but post to woocommerce failed (we let store decide)
Store decisions:
- Mark the payment as successful, if there is stock/inventory
- Inform the store owner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Update or Inform store are my 2 options.
razorpay-payments.php
Outdated
require_once __DIR__.'/razorpay-sdk/Razorpay.php'; | ||
use Razorpay\Api\Api; | ||
|
||
add_action('plugins_loaded', 'woocommerce_razorpay_init', 0); | ||
add_action('admin_post_nopriv_rzp_webhook', 'razorpay_webhook_init'); // second - admin_post_nopriv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need both of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 if you want to have both logged in and not logged in users submitting, you have to add both actions!
https://codex.wordpress.org/Plugin_API/Action_Reference/admin_post_(action)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have logged-in-users submitting in this case, it is the razorpay servers
razorpay-payments.php
Outdated
@@ -217,85 +230,12 @@ function get_order_creation_data($order_id) | |||
**/ | |||
function generate_order_form($redirect_url, $json, $order_id) | |||
{ | |||
$checkout_html = file_get_contents(__DIR__.'/js/checkout.phtml'); | |||
$keys = array("#liveurl#", "#json#", "#redirect_url#"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need to be rebased against master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Will do towards the end once we finalize the functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to review, because it does HTML very differently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Merged
includes/razorpay-webhook.php
Outdated
|
||
$data = json_decode($post, true); | ||
|
||
if ($razorpay->enable_webhook == 'yes' && !empty($data['event'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make yes into constant, or use a boolean instead if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 I believe wordpress needs a yes or no here.
includes/razorpay-webhook.php
Outdated
if ($razorpay->enable_webhook == 'yes' && !empty($data['event'])) | ||
{ | ||
// if payment.authorized webhook is enabled, we will update woocommerce about captured payments | ||
if ($data['event'] == "payment.authorized") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if order is paid at WooCommerce end before running this code. For most payments, that will be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performing this check after we get the $order object.
if ($order->needs_payment() === false)
{
return;
}
includes/razorpay-webhook.php
Outdated
$order = new WC_Order($order_id); | ||
|
||
$key_id = $razorpay->key_id; | ||
$key_secret = $razorpay->key_secret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- get the payment id from the order
- update the order (mark as successful)
- do the same steps as we do in normal payment success flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Don't get it? Don't we already mark the order as successful here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote (1) based on order.paid, so ignore. However, do try to re-use the success/failure code, instead of copying here
includes/razorpay-webhook.php
Outdated
$payment = $api->payment->fetch($razorpay_payment_id); | ||
|
||
if ($payment['status'] == 'captured') | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try re-using the existing code here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Will have to rebase against master first.
razorpay-payments.php
Outdated
'description' => esc_url( admin_url('admin-post.php') ) . '?action=rzp_webhook', | ||
'label' => __('Enable Razorpay Webhook with the URL listed below.', 'razorpay'), | ||
'default' => 'yes' | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add instructions about how to configure webhooks on the website. Something like:
Use https://www.woocommerce.com/adlkasd/asfdsaf.php as the Webhook URL here and enable the
payment.authorized
event only.
We will also need Signature verification in place (add another input for webhook secret (minlength=8))
razorpay-payments.php
Outdated
function razorpay_webhook_init() | ||
{ | ||
new RZP_Webhook(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- newline at the end.
- I don't like doing everything in the constructor here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Using only $api in the constructor. IE. $api.
@@ -0,0 +1,78 @@ | |||
<?php | |||
|
|||
require_once __DIR__.'/../razorpay-payments.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a couple of nice suggestions on how to do webhooks in wordpress cleanly:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do an early die
as well, so other plugins and wordpress does not interfere with our webhooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 I tried adding an endpoint. Although, I think using admin post is the way to go, I remember not finding results with the approach above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to give a absolute URL to the user that they can enter in the Razorpay dashboard in the settings page.
includes/razorpay-webhook.php
Outdated
protected $razorpay; | ||
|
||
protected $keyId; | ||
protected $keySecret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks duplication. What about creating a WC_Razorpay_Webhook instance in our primary class and using that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 As discussed just pulling out the $api from getRazorpayApiInstance()
js/checkout.phtml
Outdated
@@ -0,0 +1,71 @@ | |||
<script src="#liveurl#"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not needed
razorpay-payments.php
Outdated
require_once __DIR__.'/includes/razorpay-webhook.php'; | ||
require_once __DIR__.'/razorpay-sdk/Razorpay.php'; | ||
|
||
if ( ! defined( 'ABSPATH' ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ABSPATH check should be the first thing in the file
razorpay-payments.php
Outdated
@@ -44,6 +51,10 @@ public function __construct() | |||
$this->key_secret = $this->settings['key_secret']; | |||
$this->payment_action = $this->settings['payment_action']; | |||
|
|||
$this->enable_webhook = $this->settings['enable_webhook']; | |||
|
|||
$this->liveurl = 'https://checkout.razorpay.com/v1/checkout.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
razorpay-payments.php
Outdated
@@ -44,6 +51,10 @@ public function __construct() | |||
$this->key_secret = $this->settings['key_secret']; | |||
$this->payment_action = $this->settings['payment_action']; | |||
|
|||
$this->enable_webhook = $this->settings['enable_webhook']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke the last time we did a release with payment_action
. We had to ask users to visit settings and submit it again so it entered the database. Have a default value here, so it doesn't give errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 the enable_webhook setting has a default in the settings set to yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had that for payment_capture as well, and it broke. Can you try deleting the entry from your database and checking if it raises a notice?
razorpay-payments.php
Outdated
'description' => __('Would you like our webhooks to update your store, or inform you so that you can update it yourself?', 'razorpay'), | ||
'default' => 'update', | ||
'options' => array( | ||
'update' => 'Update Store', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very confusing. Needs helper text about what each option does.
razorpay-payments.php
Outdated
'webhook_secret' => array( | ||
'title' => __('Webhook Secret', 'razorpay'), | ||
'type' => 'text', | ||
'description' => __('Webhook secret is used for webhook signature verification. This has to match the one added <a href="https://dashboard.razorpay.com/#/app/webhooks">here</a>', 'razorpay') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be some helper guide about how to integrate the webhook properly. Perhaps link them to a page on the repo wiki on github?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Will add it after creating the wiki
razorpay-payments.php
Outdated
@@ -371,86 +404,6 @@ private function enqueueCheckoutScripts($data) | |||
function generateOrderForm($redirectUrl, $data) | |||
{ | |||
$this->enqueueCheckoutScripts($data); | |||
|
|||
return <<<EOT | |||
<form name='razorpayform' action="$redirectUrl" method="POST"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 My bad. Added it.
a6126f7
to
44ec014
Compare
1ebef76
to
0e7889e
Compare
razorpay-payments.php
Outdated
@@ -44,6 +51,8 @@ public function __construct() | |||
$this->key_secret = $this->settings['key_secret']; | |||
$this->payment_action = $this->settings['payment_action']; | |||
|
|||
$this->enable_webhook = $this->settings['enable_webhook'] ?? 'yes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null coalescing is only PHP7.0
razorpay-payments.php
Outdated
), | ||
'webhook_action' => array( | ||
'title' => __('Webhook Action', 'razorpay'), | ||
'type' => 'select', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see webhook_action being used anywhere
includes/razorpay-webhook.php
Outdated
$success = false; | ||
$errorMessage = 'The payment has failed.'; | ||
|
||
if ($payment['status'] === 'captured') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be capturing the payment in case it was authorized?
223aecf
to
4282134
Compare
razorpay-payments.php
Outdated
} | ||
|
||
$this->webhook_secret = $this->settings['webhook_secret']; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value needed.
includes/razorpay-webhook.php
Outdated
|
||
$this->api = $this->razorpay->getRazorpayApiInstance(); | ||
|
||
$this->auto_capture_webhook(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have a public process function instead, and call the appropriate method depending on the event (payment_authorized in our case). Makes it pluggable for subscriptions
includes/razorpay-webhook.php
Outdated
// | ||
// If the webhook secret isn't set on wordpress, return | ||
// | ||
if (isset($razorpayWebhookSecret) === false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isset will be true for empty strings
includes/razorpay-webhook.php
Outdated
if ($data['event'] === "payment.authorized") | ||
{ | ||
global $woocommerce; | ||
$orderId = $data['payload']['payment']['entity']['notes']['woocommerce_order_id']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use $order->receipt instead. I'm not very sure if we have order entity there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 As discussed let us leave as is
includes/razorpay-webhook.php
Outdated
$order = new WC_Order($orderId); | ||
|
||
// Move this to the base class as well | ||
if ($order->needs_payment() === false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the same check on the other side as well, right?
…er status to success
523fde1
to
ed0dc40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test all the cases thoroughly before doing a squash merge
{ | ||
$post = file_get_contents('php://input'); | ||
|
||
$data = json_decode($post, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test it against an empty post body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Handled. Tested.
razorpay-webhook.php - process function.
We check for whether data has event set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break if the body is empty or non-valid JSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 If it is empty, it will return null. But yeah, if it isn't a valid json, it'll break.
Either ways, json_last_error
will be anything but 0 if json_decode failed for whatever reason. So I'll add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid json without event
key will also break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 How? We're doing an empty check on line 32!
// If the payment is only authorized, we capture it | ||
// If the merchant has enabled auto capture | ||
// | ||
$payment->capture(array('amount' => $amount)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't capture with the amount from the webhook, we need to capture with the order entity in WC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Made getOrderAmountAsInteger public
'label' => __('Enable Razorpay Webhook <a href="https://dashboard.razorpay.com/#/app/webhooks">here</a> with the URL listed below.', 'razorpay'), | ||
'default' => 'yes' | ||
), | ||
'webhook_secret' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us make it mandatory if the webhook is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 How about generating a random timestamp based string and storing that as a default value?
We can add instructions on how to set up the secret, ie, by copying the string we generated into the dashboard, or by creating another one and storing it in both of the locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea, but there are complications around when you generate this secret. You don't want to do it every time they open or submit the settings page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Right, I had thought of the case as well. What then, we'll just leave it to null then?
} | ||
|
||
protected function redirectUser($order) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the early return case, $this->msg is not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 You're right. My bad. Added the code below:
if ($order->needs_payment() === false)
{
$razorpayPaymentId = $order->get_transaction_id();
$this->updateOrder($order, true, null, $razorpayPaymentId);
$this->redirectUser($order);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 This should be good to go unless the $razorpayPaymentId
isn't set. Do we handle that case too? If the order is already marked paid by the webhook, then surely the $razorpayPaymentId should be saved right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update the order at all in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 You don't.
I moved
$this->add_notice($this->msg['message'], $this->msg['class']);
to
public function updateOrder(& $order, $success, $errorMessage, $razorpayPaymentId)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't the order be updated twice in that case? The payment note gets added twice, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Changed this.
…yment using webhook
{ | ||
$actualSignature = hash_hmac(self::SHA256, $payload, Api::getSecret()); | ||
if (isset($webhookSecret) === true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always return true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 Changed this to empty() === false
Also, default is set to ''
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayankamencherla I'd pointed this out, and you'd mentioned you fixed it 👎
{ | ||
$post = file_get_contents('php://input'); | ||
|
||
$data = json_decode($post, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break if the body is empty or non-valid JSON
includes/razorpay-webhook.php
Outdated
$_SERVER['HTTP_X_RAZORPAY_SIGNATURE'], | ||
$razorpayWebhookSecret); | ||
} | ||
catch (Exception $e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't catch all exceptions and we need to log here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captn3m0 write_log(['message' => $e->getMessage()]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do a squash merge and then a beta release
No description provided.