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.

Sign up for GitHub

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

Jump to bottom

refactor(connector): [Shift4]Enhance currency Mapping with ConnectorCurrencyCommon Trait #2557

Closed
wants to merge 7 commits into from
Closed

refactor(connector): [Shift4]Enhance currency Mapping with ConnectorCurrencyCommon Trait #2557

wants to merge 7 commits into from

Conversation

sanketmp
Copy link

@sanketmp sanketmp commented Oct 12, 2023 •
edited
Loading

Type of Change

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

Description

This pull request introduces the get_currecny_unit from ConnectorCommon trait for shift4. This function allows connectors to declare their accepted currency unit as either "Base" or "Minor".

Additional Changes

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

Motivation and Context

Closes #2245

How did you test it?

(Please review the changes and let me know if any further issues or changes to be made.)

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

@sanketmp sanketmp requested a review from a team as a code owner October 12, 2023 07:33
@srujanchikke srujanchikke added A-connector-integration Area: Connector integration S-waiting-on-review Status: This PR has been implemented and needs to be reviewed C-refactor Category: Refactor hacktoberfest Issues that are up for grabs for Hacktoberfest participants labels Oct 12, 2023
@srujanchikke
Copy link
Contributor

Hi @sanketmp ,

Please use following cargo commands -

cargo clippy --all-features
ref - Cargo commands, and fix the errors thrown by clippy for the checks to pass and also use cargo +nightly fmt to fix the failing formatting check

@srujanchikke srujanchikke added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Oct 12, 2023
@sanketmp
Copy link
Author

ran the cargo +nightly fmt and gave no errors. Please review @srujanchikke

@srujanchikke
Copy link
Contributor

cargo clippy --all-features

You might also should run cargo clippy --all-features

@sanketmp
Copy link
Author

cargo clippy --all-features

You might also should run cargo clippy --all-features

I have done that too. I don't know what is happening.

prasunna09
prasunna09 requested changes Oct 18, 2023
View reviewed changes
crates/router/src/connector/shift4.rs Outdated
@@ -61,6 +61,10 @@ impl ConnectorCommon for Shift4 {
"shift4"
}

fn get_currency_unit(&self) -> api::CurrencyUnit {
api::CurrencyUnit::Base
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
api::CurrencyUnit::Base
api::CurrencyUnit::Minor

This is because, shift4 accepts amount in minor units

Copy link
Author

Choose a reason for hiding this comment

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

made the changes

crates/router/src/connector/shift4/transformers.rs Outdated
{
type Error = error_stack::Report<errors::ConnectorError>;
fn try_from(
(currency_unit, currency, amount, item): (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(currency_unit, currency, amount, item): (
(currency_unit, currency, amount, router_data): (

crates/router/src/connector/shift4/transformers.rs Outdated
let amount = utils::get_amount_as_string(currency_unit, amount, currency)?;
Ok(Self {
amount,
router_data: item,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
router_data: item,
router_data,

crates/router/src/connector/shift4/transformers.rs Outdated
{
type Error = Error;
type Error = error_stack::Report<errors::ConnectorError>;
// fn try_from(
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 please remove these comments?

crates/router/src/connector/shift4/transformers.rs Outdated
fn try_from(
item: &types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>,
value: (&Shift4RouterData<
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 please rename value to item

Suggested change
value: (&Shift4RouterData<
item: (&Shift4RouterData<

crates/router/src/connector/shift4/transformers.rs Outdated
let currency = item.request.currency;
let payment_method = Shift4PaymentMethod::try_from(item)?;
let submit_for_settlement = value.request.is_auto_capture()?;
let amount = value.request.amount.to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let amount = value.request.amount.to_string();
let amount = item.amount.clone();

@sanketmp
Copy link
Author

Hello @prasunna09, made the requested changes.

@SanchithHegde
Copy link
Member

Hey @sanketmp, could you address the failing CI checks?

@sanketmp
Copy link
Author

I couldn't figure out @SanchithHegde

@SanchithHegde
Copy link
Member

@sanketmp As mentioned earlier in #2557 (comment), you must run cargo clippy --all-features and address the errors thrown.

@sanketmp sanketmp closed this Oct 20, 2023
@SanchithHegde SanchithHegde removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prasunna09 prasunna09 prasunna09 requested changes

Assignees

@sanketmp sanketmp

Labels
A-connector-integration Area: Connector integration C-refactor Category: Refactor hacktoberfest Issues that are up for grabs for Hacktoberfest participants
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE]: [Shift4] Currency Unit Conversion
4 participants
@sanketmp @srujanchikke @SanchithHegde @prasunna09