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

update payment #249

Merged
merged 10 commits into from
May 27, 2024
Merged

update payment #249

merged 10 commits into from
May 27, 2024

Conversation

@ankitdas13 ankitdas13 requested a review from bhavyay May 2, 2024 06:20
### Fetch a Payment (With Expanded EMI Details)

```rb
Razorpay::Payment.fetch("pay_XXXXXXXXXXXXXX").expendDetails({"expand[]": "emi"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we making two requests to the BE for this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's like a chain request for better readablity.

We also used this in payment.
https://github.com/razorpay/razorpay-ruby/blob/master/documents/payment.md?plain=1#L250

Should I change it to a single request ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Request chaining does not seem optimal here. Two http requests are being made instead of a single request here ? Is there any reason why we have adopted this pattern in ruby sdk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a single request instead of chaining

@@ -298,6 +298,12 @@ def test_validate_vpa
stub_post(%r{payments/validate/vpa$}, 'fake_validate_vpa',param_attr.to_json)
payment = Razorpay::Payment.validate_vpa param_attr.to_json
assert_equal param_attr[:vpa], payment.vpa
end

def test_expendDetails
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add test cases for other options like cards and offers and error scenarios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for card

@ankitdas13 ankitdas13 requested a review from bhavyay May 15, 2024 05:37
| Name | Type | Description |
|-------------|---------|----------------------------------------------------------------|
| paymentId* | integer | Unique identifier of the payment |
| expand[] | string | Use to expand the emi details when the payment method is emi. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the description for offers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed emi to offer

| expand[] | string | Use to expand the offers applied to a payment. |

"error_source": null,
"error_step": null,
"error_reason": null,
"emi": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This response seems to contain expanded details for emi. Can you update it relevant for offers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to offer response

"offers": {

**Parameters:**

| Name | Type | Description |
|-------------|---------|--------------------------------------------------------------|
Copy link
Contributor

Choose a reason for hiding this comment

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

please format the table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update table format

| Name | Type | Description |

end
end

def test_expendDetails_emi
Copy link
Contributor

Choose a reason for hiding this comment

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

please add test case for 5xx response for fetch api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't mock the response status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added BAD REQUEST json to test the failure cases

def test_expend_details_failure

@ankitdas13 ankitdas13 requested a review from bhavyay May 24, 2024 11:54
@@ -112,5 +112,9 @@ def self.create_upi(data={})
def self.validate_vpa(data={})
request.post "validate/vpa" , data
end

def self.expend_details(id, options={})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function name be expand_details instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made correction in payment.rb, payment.md & in unit test file

def self.expand_details(id, options={})

@ankitdas13 ankitdas13 requested a review from bhavyay May 27, 2024 07:16
documents/payment.md Show resolved Hide resolved
### Fetch a Payment (With Expanded EMI Details)

```rb
Razorpay::Payment.expendDetails("pay_XXXXXXXXXXXXXX",{"expand[]": "emi"})
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the function name here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Razorpay::Payment.expand_details("pay_XXXXXXXXXXXXXX",{"expand[]": "emi"})

@ankitdas13 ankitdas13 requested a review from bhavyay May 27, 2024 07:53
@ankitdas13 ankitdas13 added the TestingNotRequired TestingNotRequired label for BVT label May 27, 2024
@ankitdas13 ankitdas13 merged commit 277f49b into master May 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TestingNotRequired TestingNotRequired label for BVT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants