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(pm_list): Update required fields for a payment method #1720

Merged
merged 3 commits into from
Jul 16, 2023

Conversation

prasunna09
Copy link
Contributor

Type of Change

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

Description

Refactored required fields for a payment method and removed fields not collected by sdk such as description, browser info.

Additional Changes

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

Motivation and Context

How did you test it?

Checklist

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

@prasunna09 prasunna09 added A-connector-integration Area: Connector integration C-feature Category: Feature request or enhancement S-waiting-on-review Status: This PR has been implemented and needs to be reviewed M-configuration-changes Metadata: This PR involves configuration changes A-payment-methods Area: Payment Methods labels Jul 15, 2023
@prasunna09 prasunna09 added this to the July 2023 Release milestone Jul 15, 2023
@prasunna09 prasunna09 requested a review from a team as a code owner July 15, 2023 06:56
@prasunna09 prasunna09 self-assigned this Jul 15, 2023
@prasunna09 prasunna09 requested a review from a team as a code owner July 15, 2023 06:56
ArjunKarthik
ArjunKarthik previously approved these changes Jul 15, 2023
field_type: enums::FieldType::UserCountry {
options: vec!["US".to_string(), "IN".to_string()],
field_type: enums::FieldType::UserAddressCountry {
options: vec![],
Copy link
Member

Choose a reason for hiding this comment

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

should be ALL or some country name 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.

Here, it is left empty because SDK will take care of countries list if not mentioned. (Discussed with SDK team)

Narayanbhat166
Narayanbhat166 previously approved these changes Jul 16, 2023
@@ -1047,7 +1138,7 @@ impl Default for super::settings::RequiredFields {
(
enums::Connector::Adyen,
vec![RequiredFieldInfo {
required_field: "card_holder_name".to_string(),
required_field: "payment_method_data.bank_debit.ach_bank_debit.card_holder_name".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be generalized, cannot keep using card_holder_name for all the payment methods which are not related to cards.

Chethan-rao
Chethan-rao previously approved these changes Jul 16, 2023
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.

Other than that, looks good to me!

RequiredFieldInfo {
required_field: "billing.address.line1".to_string(),
display_name: "line1".to_string(),
field_type: enums::FieldType::UserAddressline1,
Copy link
Member

Choose a reason for hiding this comment

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

We might have to consider renaming this and UserAddressline2 to UserAddressLine1 and UserAddressLine2 instead. This can be taken up in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this naming was given by sdk, so did the same, will ask them and modify them in next pr

crates/router/src/configs/defaults.rs Outdated Show resolved Hide resolved
@SanchithHegde SanchithHegde added this pull request to the merge queue Jul 16, 2023
Merged via the queue into main with commit 8dd9fcc Jul 16, 2023
9 checks passed
@SanchithHegde SanchithHegde deleted the required-field-for-pm branch July 16, 2023 20:25
@SanchithHegde SanchithHegde removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Jul 19, 2023
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 A-payment-methods Area: Payment Methods C-feature Category: Feature request or enhancement M-configuration-changes Metadata: This PR involves configuration changes
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants