-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(pm_list): add required fields for giropay #3194
Conversation
crates/api_models/src/payments.rs
Outdated
billing_details: Option<BankRedirectBilling>, | ||
|
||
/// The country for bank payment | ||
#[schema(value_type = CountryAlpha2, example = "US")] | ||
country: api_enums::CountryAlpha2, | ||
country: Option<api_enums::CountryAlpha2>, | ||
|
||
/// The preferred language | ||
#[schema(example = "en")] | ||
preferred_language: String, | ||
preferred_language: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have changes for Sofort pm type in this pr? I observed there was another PR for sofort, please undo these changes if not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is another separate PR for Sofort. Sofort changes appeared in this branch as dynamic_field/giropay
branch is built upon dynamic_field/sofort
branch whilst the PR pointed to main.
Now Sofort changes are removed. Requesting your review
.ok_or(errors::ConnectorError::MissingRequiredField { | ||
field_name: "giropay.billing_details", | ||
})? | ||
.billing_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is billing_name
mandatory?
if yes, please use get_billing_name()
method from utils to throw the error.
country: country | ||
.ok_or(errors::ConnectorError::MissingRequiredField { | ||
field_name: "country", | ||
})? | ||
.to_owned(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add utils function get_billing_country
for BankRedirectBilling and reuse this function in other places as well
.ok_or(errors::ConnectorError::MissingRequiredField { | ||
field_name: "giropay.billing_details", | ||
})? | ||
.billing_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use the utils method - get_billing_name()
here
( | ||
enums::Connector::Adyen, | ||
RequiredFieldFinal { | ||
mandate: HashMap::new(), | ||
non_mandate: HashMap::new(), | ||
common: HashMap::new(), | ||
} | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Adyen supports mandates, could you please confirm if this config is present in toml file?
[mandates.supported_payment_methods]
bank_redirect.giropay
Could you please check for other connectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adyen supports mandates. I will add it to the config files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, make sure you update the env files which is located at https://github.com/juspay/hyperswitch/tree/main/config/deployments
9b18843
to
f87f157
Compare
"payment_method_data.bank_redirect.giropay.country".to_string(), | ||
RequiredFieldInfo { | ||
required_field: "payment_method_data.bank_redirect.giropay.country".to_string(), | ||
display_name: "bank_account_iban".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the display name bank_account_iban
for field country?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a typo. Changed it
Type of Change
Description
Resolves #3622
Test Cases
Must be tested for each of this connectors
1.ACI
-> Create a payment with confirm
false
-> list payment methods, with client secret and publishable key
Required fields must be populated, according to the connector
Checklist
cargo +nightly fmt --all
cargo clippy