Skip to content
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

payment link end point #233

Merged
merged 4 commits into from
Sep 16, 2021
Merged

payment link end point #233

merged 4 commits into from
Sep 16, 2021

Conversation

neera11
Copy link
Contributor

@neera11 neera11 commented Jul 15, 2021

No description provided.

@neera11 neera11 changed the title payment end point payment link end point Jul 15, 2021
@isaumya
Copy link

isaumya commented Jul 16, 2021

Hey @neera11,
so if I am using this Razorpay PHP client to greater standard Payment Links, so, do I have to change the structure of the data that is passed? For example, current as per the readme example, it is stayed that I have to call the client the following way to create a payment link:

$link  = $api->invoice->create(
	arary(
		'type' => 'link', 
		'amount' => 500, 
		'description' => 'For XYZ purpose', 
		'customer' => array(
			'email' => 'test@test.test'
		)
	)
);

So, do I have to make any changes to this structure if I am only generating standard payment links?

@neera11
Copy link
Contributor Author

neera11 commented Jul 16, 2021

Hi @isaumya
No need to pass type in Payment link call
with invoice we were passing type as "link"

For payment link data will be

$orderData = json_encode(
[
'amount' => 98765,
'description' => 'For XYZ purpose',
'customer' => array('email' => 'test@test.test')
]);

$link = $api->payment_link->create($orderData, 'application/json' );

@isaumya
Copy link

isaumya commented Jul 16, 2021

Hi @neera11,
Thanks for your reply. So, no we need to pass application/json as a second parameter to the create() function? As this was not there before.

I will highly recommend you to update the readme.md file with example of the new way of calling the API.

So, just to be clear, currently, I am doing the following for generating payment link:

// Include the composer autoload file
require_once( __DIR__ . '/../vendor/autoload.php' );

// Use Razorpay API
use Razorpay\Api\Api;

/**
 * Function to generate Razorpay Payment Link
 * @var $payment_dtls Array
 * @return $link Object
 */
function generate_payment_link( $payment_dtls ) {
  // Initiate New API
  $api = new Api( 'rzp_live_123456', 'abc123' );

  // Create Payment Link
  $link = $api->invoice->create(
    [
      'type'            =>  'link',
      'amount'          =>  ( $payment_dtls['amount'] * 100 ),
      'currency'        =>  $payment_dtls['currency'],
      'customer_id'     =>  $payment_dtls['cust_id'], // Customer ID
      'description'     =>  $payment_dtls['description'],
      'reminder_enable' =>  true,
      'expire_by'       =>  strtotime( $payment_dtls['expiry'] ) // Payment link will expire in X time from generation 
    ]
  );

  // Return the payment link details send by Razorpay after payment link creation
  return $link;
}

and once this new version of the package is released, it would done as following:

// Include the composer autoload file
require_once( __DIR__ . '/../vendor/autoload.php' );

// Use Razorpay API
use Razorpay\Api\Api;

/**
 * Function to generate Razorpay Payment Link
 * @var $payment_dtls Array
 * @return $link Object
 */
function generate_payment_link( $payment_dtls ) {
  // Initiate New API
  $api = new Api( 'rzp_live_123456', 'abc123' );

  // Create Payment Link
  $link = $api->payment_link->create(
    json_encode(
      [
        'amount'          =>  ( $payment_dtls['amount'] * 100 ),
        'currency'        =>  $payment_dtls['currency'],
        'customer_id'     =>  $payment_dtls['cust_id'], // Customer ID
        'description'     =>  $payment_dtls['description'],
        'reminder_enable' =>  true,
        'expire_by'       =>  strtotime( $payment_dtls['expiry'] ) // Payment link will expire in X time from generation 
      ]
    ),
    'application/json'
  );

  // Return the payment link details send by Razorpay after payment link creation
  return $link;
}

Right? Or I am missing/misunderstanding something here?

@neera11
Copy link
Contributor Author

neera11 commented Jul 16, 2021

Hi @isaumya
You are using correctly , we need to pass second parameter as 'application/json'
Is it woking fine for you
or you getting any error?
Will mention all these dependencies in readme.md file also

@isaumya
Copy link

isaumya commented Jul 16, 2021

Hi @neera11,
I haven't tested the above code, I just modified the code to show you. I am not sure if I can test it until you guys release the update and I update the package via composer. I've running v2.7.0

@isaumya
Copy link

isaumya commented Jul 20, 2021

Hey @neera11,
You still haven't updated the readme file which still suggests users to create the payment link in the old ways.

@neera11
Copy link
Contributor Author

neera11 commented Jul 20, 2021

Hi @isaumya
sorry for the delay
I have updated readme file. Please check

@isaumya
Copy link

isaumya commented Jul 20, 2021

Thanks, @neera11. Yes, I have checked. Now I am waiting for this PR to be accepted and pushing a new release with this code so that I can update the dependency and use the new methods.

@isaumya
Copy link

isaumya commented Aug 13, 2021

Hey @neera11,
When is this getting released in a new version of the PHP client? As we have to update to the new endpoint by Aug 15, right? So, not many days left.

src/Entity.php Outdated
* @param string $contentType
* @return Payment_link
*/
protected function create($attributes = null, $contentType= 'application/x-www-form-urlencoded')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why default content type is 'application/x-www-form-urlencoded'?

Copy link
Contributor Author

@neera11 neera11 Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are passing default as "application/x-www-form-urlencoded" and in case of payment link we need to pass "application/json" for that taking $contentType variable and passing default value as "application/x-www-form-urlencoded"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think clients should pass content type separately. Idea of sdk is using the functions without knowing the internals. it should be just paymentlink->create(attributes) imo. content type if required can be passed internally to the generic function

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neera11 yes we should not explicitly pass content type. The idea of SDK is to abstract out common things and only allow users to make changes in the input values, also check other APIs implemented, I don’t see anywhere we passed explicitly content type="application/json"
// @abhayp

src/Entity.php Outdated
{
$entityUrl = $this->getEntityUrl();

return $this->request('POST', $entityUrl, $attributes);
return $this->request('POST', $entityUrl, $attributes,$contentType);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting issue, space after comma

Copy link
Contributor Author

@neera11 neera11 Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will do that

src/Request.php Outdated
{

$ContentTypeHeader = array('Content-Type' => $contentType );
self::$headers = array_merge(self::$headers, $ContentTypeHeader);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting issue need extra line

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhayp @sofiya02 can we ask each developer to use the same code editor and have some linters in place, to avoid such issues in the code review

src/Entity.php Outdated
*
* @param array $attributes
* @param string $contentType
* @return Payment_link
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not payment link class. why these comments?

src/Request.php Outdated
@@ -153,12 +151,17 @@ protected function throwServerError($body, $httpStatusCode)
$httpStatusCode);
}

protected function getRequestHeaders()
protected function getRequestHeaders($contentType = 'application/x-www-form-urlencoded')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using only one content type everywhere? payment links will work with this content type?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the default content type before? if we start passing default content type as above for all apis, will there be any issues for other functionalities?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhayp, All of other RZP API is accepting POST request with Content-Type: application/x-www-form-urlencoded then why payment Links not ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all out apis are json rest apis. we shouldnt ideally support application/x-www-form-urlencoded for json request. but since legacy code base is like that, we cannot remove it. for new ones we should enforce application/json

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can release the new SDK version with application/json @ramth05 do you see any challenges or drawbacks?
Anyway we want to move to application/json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chirag-patel-rp , don't see any issue as all our API supporting application/json and no changes required by enduser . The only thing is that all the API end points we should test it before release new version. and if there is any corner case it should be handled.

@isaumya
Copy link

isaumya commented Sep 9, 2021

@abhayp, @chirag-patel-rp please hurry up on pushing this release to the SDK as my account has been upgraded to using the new API and all of my automation system which is using the SDK might fail as I am not using the new API for payment link generation.

@chirag-patel-rp
Copy link

@isaumya contenty type relate comment is still not resolved
@neera11 any update on that?

@neera11
Copy link
Contributor Author

neera11 commented Sep 11, 2021

@chirag-patel-rp @isaumya I have added content type related issue

src/Request.php Outdated
{
if(isset($additionHeader)){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont check isset. it will always return true since the variable is set. check if(empty(additionHeader) === false please

@@ -153,12 +152,18 @@ protected function throwServerError($body, $httpStatusCode)
$httpStatusCode);
}

protected function getRequestHeaders()
protected function getRequestHeaders($additionHeader)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add param type. this should always be an array. empty array for non pl

src/Entity.php Outdated
@@ -7,12 +7,19 @@
class Entity extends Resource implements ArrayableInterface
{
protected $attributes = array();

private $additionHeader;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow how attributes are done please. this should be initialized as empty array


return $this->request('POST', $entityUrl, $attributes);
return $this->request('POST', $entityUrl, $attributes, $this->additionHeader);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional header is passed here. cannot see the function param in request function

src/Entity.php Outdated
@@ -83,10 +90,9 @@ protected function request($method, $relativeUrl, $data = null)
{
$request = new Request();

$response = $request->request($method, $relativeUrl, $data);
$response = $request->request($method, $relativeUrl, $data, $this->additionHeader);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why pass the additional header from create function if u are referencing this here

src/Request.php Outdated
@@ -37,7 +37,7 @@ class Request
* @return array Response data in array format. Not meant
* to be used directly
*/
public function request($method, $url, $data = array())
public function request($method, $url, $data = array(),$additionHeader)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize as empty array please ..just like data

@neera11 neera11 force-pushed the payment-link branch 2 times, most recently from b914963 to dce1111 Compare September 15, 2021 10:31
Copy link

@abhayp abhayp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@neera11 neera11 merged commit f562c91 into master Sep 16, 2021

$link = $api->payment_link->fetch('plink_GiwM9xbIZqbkJp'); // fetch payment link with id

$data = json_encode(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neera11, why we need to pass data as JSON for this entity only?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep..should be array. @neera11

use Requests;


class Payment_link extends Entity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neera11 , why it's Payment_link not PaymentLink

@neera11
Copy link
Contributor Author

neera11 commented Sep 23, 2021

Hi @isaumya

We have updated the paymentlink.
Now you don't need to pass second parameter as application/json while creating new paymentlink and also no need to pass data in json format.
just use as below
$link = $api->payment_link->create(array('amount' => 98765,'description' => 'For XYZ purpose', 'customer' => array('email' => 'test@test.test'))); // create payment link

Thank you

@isaumya
Copy link

isaumya commented Sep 23, 2021

Hi @neera11,
Thank you so much for letting me know and also for releasing the update.
Just have one small request, in the readme file for each call, please mention the respective API doc link for that call so that users can easily see what are the list of arguments that they can pass to that call. As in your example, you may have used a few parameters while I might use parameters like currency, cusermeID etc. So, if you guys provide a link to the API docs, users can easily see what they can pass to that call.

P.S.; Also in a future release, try testing your codebase in PHP 8, there are a few minor warnings in PHP 8 for the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants