Skip to content

fix : Encoded type to body#369

Merged
ankitdas13 merged 14 commits intomasterfrom
refactor/encoded-to-body
Aug 16, 2023
Merged

fix : Encoded type to body#369
ankitdas13 merged 14 commits intomasterfrom
refactor/encoded-to-body

Conversation

@ankitdas13
Copy link
Contributor

Improvement: Change Encoded Type to Request Body

  • Update post , put & patch method & replace encoded type from form to body
  • Removed normalizeNotes method & normalizeBoolean methods from entities
  • Updated the mocked unit-test cases

Note : Tested all Api's from my end , let me know we you want to test all api's in workflow then i will setup.

Screenshot 2023-07-26 at 5 01 04 PM

@ankitdas13 ankitdas13 requested a review from dhwanilvyas August 2, 2023 09:08

Choose a reason for hiding this comment

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

This will be hitting prod right? Please disable it or remove it from the PR if it is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These will not be going on prod. i am going to remove these e2e and share this file in slack

Choose a reason for hiding this comment

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

Remove this from the PR. Please share this file in the Slack thread

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

e2e/razorpay.js Outdated
Copy link

@1995navinkumar 1995navinkumar Aug 7, 2023

Choose a reason for hiding this comment

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

Remove this as well since we are yet to figure out on e2e approach

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 for testing purposes I added this . will remove before merge

lib/api.js Outdated
}

postBody(params, cb) {
postformData(params, cb){

Choose a reason for hiding this comment

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

Suggestion : Can rename to postFormData

lib/api.js Outdated
}

postBody(params, cb) {
postformData(params, cb){

Choose a reason for hiding this comment

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

Write a comment that this has been used when uploading files

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2023

Codecov Report

Merging #369 (982b1bd) into master (85e2fc7) will decrease coverage by 1.47%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
- Coverage   91.41%   89.95%   -1.47%     
==========================================
  Files          27       27              
  Lines        1072      906     -166     
==========================================
- Hits          980      815     -165     
+ Misses         92       91       -1     
Files Changed Coverage Δ
lib/resources/accounts.js 88.88% <ø> (-5.56%) ⬇️
lib/resources/products.js 85.71% <ø> (-8.41%) ⬇️
lib/resources/stakeholders.js 88.88% <ø> (-7.89%) ⬇️
lib/resources/tokens.js 88.88% <ø> (-3.42%) ⬇️

... and 15 files with indirect coverage changes

}
}

interface ConvenienceFeeConfig {

Choose a reason for hiding this comment

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

Why is this getting included in this PR? Does this have any relation with changing content-type to application-json @ankitdas13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

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. Please test it thoroughly before releasing

@ankitdas13 ankitdas13 added Tested TestingNotRequired TestingNotRequired label for BVT and removed Tested labels Aug 16, 2023
@ankitdas13 ankitdas13 merged commit 6672b52 into master Aug 16, 2023
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.

3 participants