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

feat: add partner's api's #351

Merged
merged 13 commits into from
Jun 30, 2023
Merged

feat: add partner's api's #351

merged 13 commits into from
Jun 30, 2023

Conversation

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2023

Codecov Report

Merging #351 (c0bbe20) into master (69edcbc) will decrease coverage by 1.58%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
- Coverage   93.02%   91.44%   -1.58%     
==========================================
  Files          21       27       +6     
  Lines         961     1076     +115     
==========================================
+ Hits          894      984      +90     
- Misses         67       92      +25     
Impacted Files Coverage Δ
lib/resources/webhooks.js 40.62% <40.62%> (ø)
lib/resources/tokens.js 92.30% <92.30%> (ø)
lib/resources/products.js 94.11% <94.11%> (ø)
lib/resources/accounts.js 94.44% <94.44%> (ø)
lib/resources/stakeholders.js 96.77% <96.77%> (ø)
lib/resources/iins.js 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@ankitdas13 ankitdas13 self-assigned this Jun 15, 2023
return {
create(params, callback) {
let { notes, ...rest } = params
let data = Object.assign(rest, normalizeNotes(notes));
Copy link

@1995navinkumar 1995navinkumar Jun 22, 2023

Choose a reason for hiding this comment

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

What does this normalizeNotes do? I saw the implementation, it just adds notes to every key? The curl request didn't have this in req body.

var data = { "phone": "9000090000" } ; 
// gets converted to
var normalised = { "notes[phone]" : "9000090000" } ;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar i had checked this one, but notes are not working without normalizeNotes.

sending notes without normalizeNotes , its throw error

Screenshot 2023-06-26 at 10 39 27 AM

sending notes with normalizeNotes.

Screenshot 2023-06-26 at 10 40 40 AM

Choose a reason for hiding this comment

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

@ankitdas13 How did you test this? Using a test account? Also, can you share the request payload before and after normalizeNotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar Yes, I have a test key, instead of creating a new object by Object.assign, I pass the params to data and it's working without normalizeNotes

create(params, callback) {
    return api.post({
        version: 'v2',
        url: `${BASE_URL}`,
        data: params
    }, callback);
},

payload

{
  url: '/v2/accounts',
  form: {
    email: '[email protected]',
    phone: '9000090000',
    legal_business_name: 'Acme Corp',
    business_type: 'partnership',
    customer_facing_business_name: 'Example',
    profile: {
      category: 'healthcare',
      subcategory: 'clinic',
      description: 'Healthcare E-commerce platform',
      addresses: [Object],
      business_model: 'Online Clothing ( men, women, ethnic, modern ) fashion and lifestyle, accessories, t-shirt, shirt, track pant, shoes.'
    },
    legal_info: { pan: 'AAACL1234C', gst: '18AABCU9603R1ZM' },
    brand: { color: 'FFFFFF' },
    notes: { internal_ref_id: '123123' },
    contact_name: 'Gaurav Kumar',
    contact_info: { chargeback: [Object], refund: [Object], support: [Object] },
    apps: { websites: [Array], android: [Array], ios: [Array] }
  }
}

Should I remove the normalizeNotes from other methods after testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar reverted previous changes except client validation

uploadAccountDoc(accountId, params, callback) {

if (!accountId) {
return Promise.reject("Account Id is mandatroy");

Choose a reason for hiding this comment

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

Question : Do we usually validate params in client? Why do we have validation only for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar We only validate the id, Is this the right way to do it or should we remove it ?

Copy link

@dhwanilvyas dhwanilvyas Jun 23, 2023

Choose a reason for hiding this comment

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

Wouldn't the API return an error if account id is not passed? If yes, should we just throw the error which API returns?

Also, I don't see the account ID check in other functions in this file. Account id should be mandatory in all cases right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhwanilvyas yes its manadatory in all also if the account id is not passed then api throw The requested URL was not found on the server.

let { amount, currency, receipt, partial_payment,payment_capture, notes, method,
...otherParams } = params
currency = currency || 'INR'

if(params.hasOwnProperty("first_payment_min_amount")){

Choose a reason for hiding this comment

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

question: is this param first_payment_min_amount removed from api request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhwanilvyas earlier if the param has first_payment_min_amount property then we had to send the data in body otherwise it's not working. for your reference.

but know it is working without this check, even the param has first_payment_min_amount

@@ -305,7 +305,7 @@ module.exports = function subscriptionsApi (api) {
}, normalizeNotes(notes))

return api.post({
url: 'subscription_registration/auth_links',
url: '/subscription_registration/auth_links',

Choose a reason for hiding this comment

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

Please test this before merging if double slash goes in request, it might fail

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, I checked this and there is no double slash.

attach url screenshot
Screenshot 2023-06-23 at 4 27 46 PM

method: 'POST'
})

rzpInstance.accounts.create(mockRequest).then((response) => {
Copy link

@1995navinkumar 1995navinkumar Jun 27, 2023

Choose a reason for hiding this comment

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

@ankitdas13 What exactly are we testing here? If we are testing for response, then we should not Mock it. It defeats the purpose of API testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar We are only testing API methods here by checking the URL and methods with mock url and method and we are following this in all API entity test cases.

Choose a reason for hiding this comment

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

Got it. But IMO, since we are integration something, we would also need to ensure the integration actually works instead of just testing the implementation details.
We can just write test cases for happy flows and check whether we get status 200 for all requests. We can take this up as a separate discussion as well.

Copy link

@1995navinkumar 1995navinkumar left a comment

Choose a reason for hiding this comment

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

LGTM. Just added one opinion.Take final approval from Dhwanil.

@ankitdas13 ankitdas13 changed the title Add partner endpoint feat: add partner's api's Jun 27, 2023
};
document_type: string;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar added form-data params type check for document upload api

@@ -47,7 +48,7 @@ export declare namespace Stakeholders {

interface RazorpayStakeholderCreateRequestBody extends RazorpayStakeholderBaseRequestBody { }

interface RazorpayStakeholderUpdateRequestBody extends Omit<RazorpayStakeholderBaseRequestBody, 'email'> { }
interface RazorpayStakeholderUpdateRequestBody extends Partial<Omit<RazorpayStakeholderBaseRequestBody, 'email'>> { }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1995navinkumar added partial because all params is optional , also checked with empty objects as well in postmen.

for your reference

Copy link

@1995navinkumar 1995navinkumar left a comment

Choose a reason for hiding this comment

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

LGTM. Do create separate task for using correct Content-Type for request and for testing the integration with a test account instead of mocking.

@ankitdas13 ankitdas13 merged commit 69dd174 into master Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants