feat(connector): [Multisafepay] Add support for Ideal and Giropay #4398

TakshPanchal · 2024-04-19T08:53:03Z

Type of Change

Description

Add iDEAL and Giropay payment processors in Multisafepay connector.

Whole description can be found in #4143 discussion.

Additional Changes

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

Motivation and Context

Solves the issue #4397

How did you test it?

I have tested the Multisafe payment connector for Giropay and iDeal methods in Postman suit. You can find my fork of Postman's collection below. In Payments - Create iDeal Payment and Payments - Create Giropay Direct in Quick Start are the requests for the payments.

iDeal

By requesting the iDeal payment method with "Ing" bank name, a redirection link can be found, redirecting to the test portal of Multisafepay, where we can see the bank name we selected.

Also, passing bank names not supported by iDeal raises an Error.

Not Passing the bank name in the payment_method_data returns an error, too.

Giropay

Giropay doesn't support the direct method, so it is not compulsory to enter bank_name into the payment_method_data. After a successful request to Multisafepay, We will get a redirection link to the Mutisafepay payment's page.

Mutisafepay payment page.

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

…ctor

srujanchikke

@TakshPanchal, Thanks for the PR! I have few comments, please resolve them. And also please make sure all CI checks are passed after you push final commit.

srujanchikke · 2024-04-19T09:21:30Z

crates/router/src/connector/multisafepay/transformers.rs

@@ -181,6 +185,46 @@ pub enum WalletInfo {
    GooglePay(GpayInfo),
 }

+#[derive(Clone, Debug, Eq, PartialEq, Serialize)]


Are these all derives required? If not please remove the redundant ones from all the enums and structs. #[derive(Debug, Serialize, Clone)] should work for requests and #[derive(Debug, Deserialize, Clone)] should work for response.

For BankRedirectInfo and IdealInfo, Eq and PartialEq will be used for comparing different bank's Info that is why Eq is needed.

Can you please point to the code where you are comparing different bank's info?

srujanchikke · 2024-04-19T09:31:59Z

crates/router/src/connector/multisafepay/transformers.rs

+            BankNames::VanLanschot => Self("0161"),
+            BankNames::Yoursafe => Self("0806"),
+            BankNames::Handelsbanken => Self("1235"),
+            _ => Err(errors::ConnectorError::NotSupported {


Please don't use default here, rather mention all the enums. It will help us easily debug any issues when new enum is added.

srujanchikke · 2024-04-19T09:32:32Z

crates/router/src/connector/multisafepay/transformers.rs

+            api::PaymentMethodData::BankRedirect(ref bank_data) => match bank_data {
+                api::BankRedirectData::Giropay { .. } => Type::Redirect,
+                api::BankRedirectData::Ideal { .. } => Type::Direct,
+                _ => Err(errors::ConnectorError::NotImplemented(


Same here, don't use defaults

srujanchikke · 2024-04-19T09:32:44Z

crates/router/src/connector/multisafepay/transformers.rs

+            api::PaymentMethodData::BankRedirect(ref bank_data) => Some(match bank_data {
+                api::BankRedirectData::Giropay { .. } => Gateway::Giropay,
+                api::BankRedirectData::Ideal { .. } => Gateway::Ideal,
+                _ => Err(errors::ConnectorError::NotImplemented(


srujanchikke · 2024-04-19T09:34:02Z

crates/router/src/connector/multisafepay/transformers.rs

+                                        field_name: "eps.bank_name",
+                                    },


This should be ideal.bank_name .

srujanchikke · 2024-04-19T09:41:44Z

crates/router/src/types/api/payments.rs

+    AcceptanceType, Address, AddressDetails, Amount, AuthenticationForStartResponse,
+    BankRedirectData, Card, CryptoData, CustomerAcceptance, HeaderPayload, MandateAmountData,
+    MandateData, MandateTransactionType, MandateType, MandateValidationFields, NextActionType,
+    OnlineMandate, PayLaterData, PaymentIdType, PaymentListConstraints,
+    PaymentListFilterConstraints, PaymentListFilters, PaymentListResponse, PaymentListResponseV2,
+    PaymentMethodData, PaymentMethodDataRequest, PaymentMethodDataResponse, PaymentOp,
+    PaymentRetrieveBody, PaymentRetrieveBodyWithCredentials, PaymentsApproveRequest,
+    PaymentsCancelRequest, PaymentsCaptureRequest, PaymentsExternalAuthenticationRequest,


If this are format changes, please revert them.

This is because I have imported BankRedirectData and Formatting is done after that. So it is just one addition of t he datatype here.

srujanchikke · 2024-04-22T09:30:25Z

Hey @TakshPanchal, Your branch has conflicts. Please update it to main.

TakshPanchal · 2024-04-22T09:42:46Z

Hey @TakshPanchal, Your branch has conflicts. Please update it to main.

Yes, I am aware of that, working on it. Thank you.

TakshPanchal · 2024-04-22T09:56:01Z

#4397 Linking the issue

TakshPanchal · 2024-04-22T10:17:42Z

I think I have to do some changes after resolving these merge conflicts. I will look into that.

TakshPanchal · 2024-04-22T13:27:48Z

I have resolved the conflicts and ran clippy. At the end code is compiled and ran successfully.

srujanchikke

@TakshPanchal Apologies, I missed one review comment last time. Could you please resolve this?

srujanchikke · 2024-04-25T10:02:01Z

crates/router/src/connector/multisafepay/transformers.rs

+    pub issuer_id: Option<String>,
+}
+
+pub struct MultisafepayBankNames<'a>(&'a str);


Can you convert this to an enum. You can use serde rename here.

Completed the change @srujanchikke

SanchithHegde

Looks good to me!

Thanks for the PR, @TakshPanchal!

srujanchikke · 2024-05-07T07:23:20Z

Hey @TakshPanchal , Could you please resolve the conflicts ?

SamraatBansal · 2024-05-07T07:14:30Z

crates/router/src/connector/multisafepay/transformers.rs

+                    domain::BankRedirectData::Ideal {
+                        billing_details: _,
+                        bank_name,
+                        country: _,


Suggested change

domain::BankRedirectData::Ideal {

billing_details: _,

bank_name,

country: _,

domain::BankRedirectData::Ideal {

bank_name,

..

SamraatBansal · 2024-05-07T07:17:31Z

crates/router/src/connector/multisafepay/transformers.rs

@@ -181,6 +185,46 @@ pub enum WalletInfo {
    GooglePay(GpayInfo),
 }

+#[derive(Clone, Debug, Eq, PartialEq, Serialize)]


Can you please point to the code where you are comparing different bank's info?

SamraatBansal · 2024-05-07T07:23:32Z

crates/router/src/connector/multisafepay/transformers.rs

+    Ideal,
+    Giropay,


Please add Currency and Country configs for both Ideal and Giropay in config/development.toml, config/deployments/sandbox.toml, config/deployments/integration_test.toml, config/deployments/production.toml for Multisafepay

Add support for Ideal and Giropay bank redirect in Multisafepay conne…

0824c5d

…ctor

TakshPanchal requested review from a team as code owners April 19, 2024 08:53

srujanchikke requested changes Apr 19, 2024

View reviewed changes

srujanchikke assigned TakshPanchal Apr 19, 2024

srujanchikke added A-connector-integration Area: Connector integration S-waiting-on-author Status: This PR is incomplete or needs to address review comments hiring-challenge labels Apr 19, 2024

Updated default hands

b729cdd

Merge branch 'main' into ideal-geropay-with-multisafe

5cff0ad

TakshPanchal changed the title ~~Add support for Ideal and Giropay bank redirect in Multisafepay conne…~~ feat(connector): [Multisafepay] Add support for Ideal and Giropay Apr 22, 2024

srujanchikke linked an issue Apr 22, 2024 that may be closed by this pull request

[FEATURE] Implementing iDEAL and Giropay payment methods with Multisafe #4397

Open

2 tasks

Refactor payment method data types in Multisafepay connector

c495c6a

srujanchikke requested changes Apr 25, 2024

View reviewed changes

TakshPanchal added 2 commits April 26, 2024 16:01

Convert MultisafepayBankNames to enum

d125f9a

Refactor MultisafepayBankNames enum in transformer.rs

7ad9ab8

srujanchikke approved these changes Apr 29, 2024

View reviewed changes

SanchithHegde added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels May 5, 2024

SanchithHegde approved these changes May 6, 2024

View reviewed changes

SamraatBansal requested changes May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(connector): [Multisafepay] Add support for Ideal and Giropay #4398

feat(connector): [Multisafepay] Add support for Ideal and Giropay #4398

TakshPanchal commented Apr 19, 2024 •

edited

srujanchikke left a comment

srujanchikke Apr 19, 2024

TakshPanchal Apr 19, 2024 •

edited

SamraatBansal May 7, 2024

srujanchikke Apr 19, 2024

srujanchikke Apr 19, 2024

srujanchikke Apr 19, 2024

srujanchikke Apr 19, 2024

srujanchikke Apr 19, 2024

TakshPanchal Apr 22, 2024

srujanchikke commented Apr 22, 2024

TakshPanchal commented Apr 22, 2024 •

edited

TakshPanchal commented Apr 22, 2024

TakshPanchal commented Apr 22, 2024

TakshPanchal commented Apr 22, 2024

srujanchikke left a comment

srujanchikke Apr 25, 2024

TakshPanchal Apr 26, 2024

SanchithHegde left a comment

srujanchikke commented May 7, 2024

SamraatBansal May 7, 2024

SamraatBansal May 7, 2024

SamraatBansal May 7, 2024

feat(connector): [Multisafepay] Add support for Ideal and Giropay #4398

Are you sure you want to change the base?

feat(connector): [Multisafepay] Add support for Ideal and Giropay #4398

Conversation

TakshPanchal commented Apr 19, 2024 • edited

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

iDeal

Giropay

Checklist

srujanchikke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TakshPanchal Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srujanchikke commented Apr 22, 2024

TakshPanchal commented Apr 22, 2024 • edited

TakshPanchal commented Apr 22, 2024

TakshPanchal commented Apr 22, 2024

TakshPanchal commented Apr 22, 2024

srujanchikke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanchithHegde left a comment

Choose a reason for hiding this comment

srujanchikke commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TakshPanchal commented Apr 19, 2024 •

edited

TakshPanchal Apr 19, 2024 •

edited

TakshPanchal commented Apr 22, 2024 •

edited