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

refactor(connector): [Worldpay] Currency Unit Conversion #2436

Merged

Conversation

Suraj3240
Copy link
Contributor

@Suraj3240 Suraj3240 commented Oct 3, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

This pull request implements the get_currency_unit from the ConnectorCommon trait for the Worldpay connector. This function allows connectors to declare their accepted currency unit as either Base or Minor. For Worldpay it accepts currency as Minor units.

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Links to the files with corresponding changes.

  1. crates\router\src\connector\worldpay.rs
  2. crates\router\src\connector\worldpay\transformers.rs

Motivation and Context

Closes #2250.

How did you test it?

I tried setting up and testing using VS code and codesandbox but was not able to do it.
So kindly review the changes and do let me know if any further issues.

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@Suraj3240 Suraj3240 requested a review from a team as a code owner October 3, 2023 16:27
@github-actions github-actions bot added the S-conventions-not-followed Status: This PR does not follow contributing guidelines label Oct 3, 2023
@Suraj3240 Suraj3240 changed the title Feature/worldpay currency unit conversion worldpay currency unit conversion Oct 3, 2023
@Suraj3240 Suraj3240 changed the title worldpay currency unit conversion worldpay_currency_unit_conversion Oct 3, 2023
@Suraj3240 Suraj3240 closed this Oct 3, 2023
@Suraj3240 Suraj3240 reopened this Oct 3, 2023
@Suraj3240 Suraj3240 changed the title worldpay_currency_unit_conversion Feature : Worldpay Currency Unit Conversion Oct 3, 2023
@Suraj3240 Suraj3240 changed the title Feature : Worldpay Currency Unit Conversion Feature:Worldpay Currency Unit Conversion Oct 3, 2023
@Suraj3240 Suraj3240 changed the title Feature:Worldpay Currency Unit Conversion Feature: Worldpay Currency Unit Conversion Oct 3, 2023
@Suraj3240 Suraj3240 changed the title Feature: Worldpay Currency Unit Conversion feature(connector): [Worldpay] Currency Unit Conversion Oct 3, 2023
@Suraj3240 Suraj3240 changed the title feature(connector): [Worldpay] Currency Unit Conversion refactor(connector): [Worldpay] Currency Unit Conversion Oct 3, 2023
@github-actions github-actions bot removed the S-conventions-not-followed Status: This PR does not follow contributing guidelines label Oct 3, 2023
@Suraj3240
Copy link
Contributor Author

@knutties I have raised a PR. What do I need to do now?

match
request.order_details.clone(){
Some(order_details)=>

Ok(Self {
instruction: Instruction {
value: PaymentValue {
Copy link
Contributor

Choose a reason for hiding this comment

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

The amount field needs to be retrieved from the WorldpayRouterData

Suggested change
value: PaymentValue {
value: PaymentValue {
amount : item.amount ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I think this will solve the issue.
I will commit the changes said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The amount field needs to be retrieved from the WorldpayRouterData

@srujanchikke please check and inform if any further changes to be made.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Suraj3240 Why do we need to match with order_details ,As We are not using order details anywhere in payment request ?

Please refer to this pr for more info #2192

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Suraj3240 Why do we need to match with order_details ,As We are not using order details anywhere in payment request ?

Please refer to this pr for more info #2192

image

So I will have to code for all the payment methods like wallet,cards,etc. as given in this file, right?

image
And the amount will be like this, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to worry about the other payment methods mapping . But yes , as you mentioned amount should be mapped that way .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Suraj3240 Why do we need to match with order_details ,As We are not using order details anywhere in payment request ?

Please refer to this pr for more info #2192

In this PR link the fields are derived from the Order_details. And hence I used that method.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Suraj3240 Why do we need to match with order_details ,As We are not using order details anywhere in payment request ?

Please refer to this pr for more info #2192

And following the method used in this PR, it is giving out errors.
image

So pls someone kindly help me.

Copy link
Contributor

@srujanchikke srujanchikke left a comment

Choose a reason for hiding this comment

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

Please update the branch to ensure the spell check in the CI workflow functions properly.

@srujanchikke srujanchikke added A-connector-integration Area: Connector integration S-waiting-on-author Status: This PR is incomplete or needs to address review comments C-refactor Category: Refactor labels Oct 3, 2023
@VedantKhairnar VedantKhairnar added the hacktoberfest Issues that are up for grabs for Hacktoberfest participants label Oct 5, 2023
@srujanchikke
Copy link
Contributor

Hey @Suraj3240

Please use the following cargo commands -

cargo check --all-features
cargo clippy --all-features

ref - Cargo commands, and fix the errors thrown by clippy for the checks to pass .

Please run cargo +nightly fmt to fix failing formatting check

@Suraj3240 Suraj3240 force-pushed the feature/worldpay_currency_unit_conversion branch from 5e6d903 to 7890aeb Compare October 5, 2023 19:44
@Suraj3240
Copy link
Contributor Author

@srujanchikke can you confirm the changes made?

@srujanchikke
Copy link
Contributor

@srujanchikke can you confirm the changes made?

Can you remove this additional package-lock.json, package.json files

@Suraj3240
Copy link
Contributor Author

Suraj3240 commented Oct 6, 2023

@srujanchikke can you confirm the changes made?

Can you remove this additional package-lock.json, package.json files

@srujanchikke
Done

@Suraj3240 Suraj3240 requested a review from srujanchikke October 6, 2023 15:40
@srujanchikke
Copy link
Contributor

Hey @Suraj3240

Please use the following cargo commands -

cargo check --all-features cargo clippy --all-features

ref - Cargo commands, and fix the errors thrown by clippy for the checks to pass .

Please run cargo +nightly fmt to fix failing formatting check

Hey @Suraj3240 ,

Could you run this commands locally , so that the checks won't fail after pushing code .

@Suraj3240 Suraj3240 force-pushed the feature/worldpay_currency_unit_conversion branch from efa49e7 to 37d5a7d Compare October 7, 2023 19:51
@Suraj3240
Copy link
Contributor Author

Hey @Suraj3240

Please use the following cargo commands -

cargo check --all-features cargo clippy --all-features

ref - Cargo commands, and fix the errors thrown by clippy for the checks to pass .

Please run cargo +nightly fmt to fix failing formatting check

cargo clippy commands is giving errors which are asking to modify codes in other files (apart from Worldpay files).
So do i have to change all the other files or just ignore those?

@srujanchikke
Copy link
Contributor

Please run cargo +nightly fmt to fix failing formatting check and revert package.json file .Other than that looks good to me

Copy link
Contributor

@srujanchikke srujanchikke left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@SanchithHegde SanchithHegde left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Thanks for the PR, @Suraj3240!

@SanchithHegde SanchithHegde added this pull request to the merge queue Oct 12, 2023
@SanchithHegde SanchithHegde added hacktoberfest-accepted Pull requests accepted as Hacktoberfest contributions and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Oct 12, 2023
Merged via the queue into juspay:main with commit b78109b Oct 12, 2023
10 checks passed
@Suraj3240
Copy link
Contributor Author

Thanks for your help and support throughout. :)) @srujanchikke @SanchithHegde

@swangi-kumari
Copy link
Contributor

Hey @Suraj3240 ,
Thanks a bunch for all your contributions! We've got a little something for you to show our appreciation.
Just take a moment to fill out this form, and get ready for some awesome swag coming your way.
Thanks!

@Suraj3240 Suraj3240 deleted the feature/worldpay_currency_unit_conversion branch November 23, 2023 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-connector-integration Area: Connector integration C-refactor Category: Refactor hacktoberfest Issues that are up for grabs for Hacktoberfest participants hacktoberfest-accepted Pull requests accepted as Hacktoberfest contributions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: [Worldpay] Currency Unit Conversion
5 participants