Skip to content

feat(payouts): add country, currency filters for payout methods #5130

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
Merged
merged 20 commits into from
Jul 11, 2024
Merged

feat(payouts): add country, currency filters for payout methods #5130

merged 20 commits into from
Jul 11, 2024

Conversation

srujanchikke
Copy link
Contributor

@srujanchikke srujanchikke commented Jun 26, 2024 •
edited
Loading

Type of Change

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

Description

This PR includes :

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?

Test case 1: create mca with connector adyen with payment method

{
    "connector_type": "payout_processor",
    "connector_name": "adyen",
    "connector_account_details": {
        "auth_type": "SignatureKey",
        "api_key": "api_key",
        "key1": "key1",
        "api_secret": "api_secret"
    },
    "test_mode": false,
    "disabled": false,
    "metadata": {
        "city": "NY",
        "unit": "245",
        "endpoint_prefix": ""
    },
    "payment_methods_enabled": [
        {
            "payment_method": "bank_transfer",
            "payment_method_types": [
                {
                    "payment_method_type": "sepa",
                    "payment_experience": "redirect_to_url",
                    "card_networks": null,
                    "accepted_currencies": null,
                    "accepted_countries": null,
                    "minimum_amount": 0,
                    "maximum_amount": 68607706,
                    "recurring_enabled": true,
                    "installment_payment_enabled": false
                }
            ]
        }
    ]
}
  • create a payout link with currency EUR and NL. You should be seeing sepa as only payout method when you redirect to payout link
{
    "amount": 100,
    "currency": "EUR",
    "customer_id": "cust_123",
    "email": "[email protected]",
    "name": "John Doe",
    "phone": "999999999",
    "phone_country_code": "+65",
    "description": "Its my first payout request",
    "billing": {
        "address": {
            "line1": "1467",
            "line2": "Harrison Street",
            "line3": "Harrison Street",
            "city": "San Fransico",
            "state": "CA",
            "zip": "94122",
            "country": "NL",
            "first_name": "John",
            "last_name": "Doe"
        },
        "phone": {
            "number": "8056594427",
            "country_code": "+91"
        }
    },
    "profile_id" : "{{profile_id}}",
    "payout_link": true,
    "confirm": false,
    "session_expiry": 2592000
}

Test Case 2 : Verify all currency & country configuration. You can verify country & currencies from development.toml -> payout_filters.adyen .

Test Case 3 : Create MCA as created in test case1 , While creating payout link, try creating with different currency other than EUR. Default Payout method card should be present after redirecting to payout list.

Test Case 4 : After WASM deployment, please verify Adyen payout connector doesn't have ach & bacs in bank transfers.

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

@srujanchikke srujanchikke added A-core Area: Core flows C-feature Category: Feature request or enhancement labels Jun 26, 2024
@srujanchikke srujanchikke requested a review from kashif-m June 26, 2024 13:00
@srujanchikke srujanchikke self-assigned this Jun 26, 2024
@srujanchikke srujanchikke requested review from a team as code owners June 26, 2024 13:00
kashif-m
kashif-m reviewed Jun 27, 2024
View reviewed changes
crates/router/src/core/payout_link.rs Outdated
pmts.payment_method_type,
))
});
let address = db
Copy link
Contributor

Choose a reason for hiding this comment

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

We can move address retrieval to outer function to avoid making this db call every time

crates/router/src/core/payout_link.rs Outdated
Comment on lines 283 to 294
if currency_country_filter != Some(false) {
if payment_method == common_enums::PaymentMethod::Card {
card_hs.insert(pmts.payment_method_type);
payment_method_list_hm.insert(payment_method, card_hs.clone());
} else if payment_method == common_enums::PaymentMethod::Wallet {
wallet_hs.insert(pmts.payment_method_type);
payment_method_list_hm.insert(payment_method, wallet_hs.clone());
} else if payment_method == common_enums::PaymentMethod::BankTransfer {
bank_transfer_hs.insert(pmts.payment_method_type);
payment_method_list_hm.insert(payment_method, bank_transfer_hs.clone());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional - Possible to use match here to avoid if-else-if ladders?

crates/router/src/core/payout_link.rs Outdated
@@ -147,7 +148,7 @@ pub async fn initiate_payout_link(
let enabled_payment_methods = link_data
.enabled_payment_methods
.unwrap_or(fallback_enabled_payout_methods.to_vec());

logger::debug!("enabled_payment_methods : {:?}", enabled_payment_methods);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this serve any good purpose other than just logging? If not, we can remove it

@srujanchikke srujanchikke force-pushed the currency_filter_payouts branch from 1926dcf to b205fea Compare June 28, 2024 07:57
kashif-m
kashif-m previously approved these changes Jun 28, 2024
View reviewed changes
sai-harsha-vardhan
sai-harsha-vardhan reviewed Jun 28, 2024
View reviewed changes
crates/router/src/core/payout_link.rs Outdated
@@ -225,11 +225,12 @@ pub async fn initiate_payout_link(

#[cfg(feature = "payouts")]
pub async fn filter_payout_methods(
db: &dyn StorageInterface,
state: SessionState,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we take the reference here, to avoid the additional clones?

crates/router/src/core/payout_link.rs Outdated
@@ -256,6 +267,7 @@ pub async fn filter_payout_methods(
let mut bank_transfer_hs: HashSet<common_enums::PaymentMethodType> = HashSet::new();
let mut card_hs: HashSet<common_enums::PaymentMethodType> = HashSet::new();
let mut wallet_hs: HashSet<common_enums::PaymentMethodType> = HashSet::new();
let payout_filter_config = &state.conf.clone().payout_filters;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clone payout_filters here instead?

sai-harsha-vardhan
sai-harsha-vardhan reviewed Jun 28, 2024
View reviewed changes
crates/router/src/core/payout_link.rs Outdated
))
});
let country_filter = country.as_ref().and_then(|country| {
pmt_filter.and_then(|pmt_filter_hm| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please use the proper naming convention here for better clarity?

sai-harsha-vardhan
sai-harsha-vardhan reviewed Jun 28, 2024
View reviewed changes
crates/router/src/core/payout_link.rs Outdated
&address.country,
)
.await?;
if currency_country_filter != Some(false) {
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
if currency_country_filter != Some(false) {
if currency_country_filter == Some(true) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None here means Payoutmethod for that connector doesn't have configs in the application configs. For which we still want to show the payout method. Some(false) indicates that payout method filter is available but currency/ country is not present in the list. So we need to check for Some(true) or None.

Copy link
Member

Choose a reason for hiding this comment

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

You can also write this as:

Suggested change
if currency_country_filter != Some(false) {
if currency_country_filter.unwrap_or(true) {

Optional change.

crates/router/src/core/payout_link.rs
request_payout_method_type: &api_models::payment_methods::RequestPaymentMethodTypes,
currency: &common_enums::Currency,
country: &Option<enums::CountryAlpha2>,
) -> errors::RouterResult<Option<bool>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we return bool here, instead of Option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no filter available for connector-> payout method configuration we will still show the payout method by default. We can't remove payout method just because filters are not declared for that.

kashif-m
kashif-m previously approved these changes Jun 28, 2024
View reviewed changes
kashif-m
kashif-m reviewed Jul 2, 2024
View reviewed changes
config/development.toml Outdated
wallet = ["paypal", "pix", "venmo"]

[payout_filters.adyen]
sepa = { country = "ES,SK,AT,NL,DE,BE,FR,FI,PT,IE,EE,LT,LV,IT", currency = "EUR" }
Copy link
Contributor

@kashif-m kashif-m Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

--

@srujanchikke srujanchikke dismissed stale reviews from sai-harsha-vardhan and kashif-m via 0897681 July 2, 2024 09:15
kashif-m
kashif-m previously approved these changes Jul 2, 2024
View reviewed changes
config/development.toml Outdated
[payout_filters.stripe]
ach = { country = "US", currency = "USD" }

[payout_filters.wise]
Copy link
Contributor

Choose a reason for hiding this comment

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

--

@srujanchikke srujanchikke dismissed stale reviews from kashif-m and sai-harsha-vardhan via 1806204 July 4, 2024 07:15
kashif-m
kashif-m previously approved these changes Jul 8, 2024
View reviewed changes
Sakilmostak
Sakilmostak previously approved these changes Jul 10, 2024
View reviewed changes
@srujanchikke srujanchikke requested a review from a team as a code owner July 10, 2024 15:15
@Gnanasundari24 Gnanasundari24 added this pull request to the merge queue Jul 11, 2024
Merged via the queue into main with commit d6f7f3c Jul 11, 2024
@Gnanasundari24 Gnanasundari24 deleted the currency_filter_payouts branch July 11, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanchithHegde SanchithHegde SanchithHegde approved these changes

@kashif-m kashif-m kashif-m approved these changes

@sai-harsha-vardhan sai-harsha-vardhan sai-harsha-vardhan approved these changes

@Sakilmostak Sakilmostak Sakilmostak approved these changes

Assignees

@srujanchikke srujanchikke

Labels
A-core Area: Core flows C-feature Category: Feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@srujanchikke @SanchithHegde @kashif-m @sai-harsha-vardhan @Sakilmostak @Gnanasundari24