feat(connector): [JPMORGAN] add Payment flows for cards #6668
Conversation
hyperswitch-bot
bot
added
the
M-api-contract-changes
bsayak03
force-pushed
the
jpmorgan-cards
branch
5 times, most recently
from
6d7ae12 to
99e5223
Compare
bsayak03
force-pushed
the
jpmorgan-cards
branch
from
b5683d4 to
106fb95
Compare
bsayak03
force-pushed
the
jpmorgan-cards
branch
2 times, most recently
from
808ae0e to
678b508
Compare
config/deployments/sandbox.toml
Outdated
There was a problem hiding this comment.
does jpmorgan use same base url in both sbx and prod env?
There was a problem hiding this comment.
no, it uses different
There was a problem hiding this comment.
add pm filters in toml files.
config/docker_compose.toml
Outdated
There was a problem hiding this comment.
check if jpmorgan is needs to be added in temp_locker_enable_config
There was a problem hiding this comment.
we don't need to add that here
| @@ -945,6 +945,7 @@ default_imp_for_payouts!( | |||
| connector::Gpayments, | |||
| connector::Iatapay, | |||
| connector::Itaubank, | |||
| //connector::Jpmorgan, | |||
There was a problem hiding this comment.
will we add support for payouts also for jp morgan?
There was a problem hiding this comment.
no, removing the commented code
cypress-tests/package-lock.json
Outdated
There was a problem hiding this comment.
revert this file change
cypress-tests/package.json
Outdated
There was a problem hiding this comment.
revert this change
package-lock.json
Outdated
package.json
Outdated
| @@ -1,10 +1,13 @@ | |||
| pub mod transformers; | |||
| use std::convert::TryFrom; | |||
There was a problem hiding this comment.
remove unused imports from the files
There was a problem hiding this comment.
we get warnings for unused imports, isn't it?
i got them and removed those, no more warnings remaining
bsayak03
force-pushed
the
jpmorgan-cards
branch
2 times, most recently
from
3e3ded0 to
a818e57
Compare
| @@ -320,6 +321,10 @@ debit.currency = "USD" | |||
| ali_pay.currency = "GBP,CNY" | |||
| we_chat_pay.currency = "GBP,CNY" | |||
|
|
|||
| [pm.filters.jpmorgan] | |||
There was a problem hiding this comment.
please verify the currencies supported
| client_id.peek(), | ||
| client_secret.unwrap_or_default().peek() | ||
| ); | ||
| // let creds = format!("{}:{}", client_id.peek(), client_secret.unwrap_or_default().peek()); |
There was a problem hiding this comment.
remove unused commented code
| ) -> CustomResult<String, errors::ConnectorError> { | ||
| Err(errors::ConnectorError::NotImplemented("get_url method".to_string()).into()) | ||
| let endpoint = self.base_url(connectors); |
There was a problem hiding this comment.
Ok(format!("{}/payments", self.base_url(connectors)))
| }) | ||
| } | ||
| _ => Err(errors::ConnectorError::NotImplemented("Payment method".to_string()).into()), | ||
| _ => Err(errors::ConnectorError::NotImplemented( |
There was a problem hiding this comment.
mention all enums exclusively, in case of payment methods
| }) | ||
| } | ||
| _ => Err(errors::ConnectorError::NotImplemented("Payment method".to_string()).into()), | ||
| _ => Err(errors::ConnectorError::NotImplemented( | ||
| "Selected payment method through jpmorgan".to_string(), |
There was a problem hiding this comment.
Err(errors::ConnectorError::NotImplemented(
get_unimplemented_payment_method_error_message("jpmorgan"),
))?
| let capture_method = if let Some(method) = item.router_data.request.capture_method { | ||
| map_capture_method(method) | ||
| } else { | ||
| String::from("AUTOMATIC") |
There was a problem hiding this comment.
cover this condition in map_capture_method itself
| let mut transaction_state = item.response.transaction_state.to_string(); | ||
|
|
||
| if transaction_state == "Closed" { | ||
| let cm = item.response.capture_method.clone(); |
There was a problem hiding this comment.
let capture_method = item.response.capture_method.clone();
| let cm = item.response.capture_method.clone(); | ||
| if cm == Some("NOW".to_string()) { | ||
| transaction_state = String::from("Closed"); | ||
| } else if cm == Some("MANUAL".to_string()) { |
There was a problem hiding this comment.
please make enums instead of string comparison.
|
|
||
| let currency = Some(item.router_data.request.currency.to_string()); | ||
|
|
||
| // let company_name : Option<String> = Some(String::from("JPMC")); |
config/deployments/production.toml
Outdated
| [pm.filters.jpmorgan] | ||
| debit = { country = "CA, EU, UK, US", currency = "AED, AFN, ALL, AMD, ANG, AOA, ARS, AUD, AWG, AZN, BAM, BBD, BDT, BGN, BIF, BMD, BND, BOB, BRL, BSD, BTN, BWP, BYN, BZD, CAD, CDF, CHF, CLP, CNY, COP, CRC, CVE, CZK, DJF, DKK, DOP, DZD, EGP, ETB, EUR, FJD, FKP, GBP, GEL, GHS, GIP, GMD, GTQ, GYD, HKD, HNL, HRK, HTG, HUF, IDR, ILS, INR, ISK, JMD, JPY, KES, KHR, KMF, KRW, KYD, KZT, LAK, LBP, LKR, LRD, LSL, MAD, MDL, MGA, MKD, MMK, MNT, MOP, MRU, MUR, MVR, MWK, MXN, MYR, MZN, NAD, NGN, NIO, NOK, NPR, NZD, PAB, PEN, PGK, PHP, PKR, PLN, PYG, QAR, RON, RSD, RWF, SAR, SBD, SCR, SEK, SGD, SHP, SLL, SOS, SRD, STN, SZL, THB, TJS, TOP, TRY, TTD, TWD, TZS, UAH, UGX, USD, UYU, UZS, VND, VUV, WST, XAF, XCD, XOF, XPF, YER, ZAR, ZMW" } | ||
| credit = { country = "CA, EU, UK, US", currency = "AED, AFN, ALL, AMD, ANG, AOA, ARS, AUD, AWG, AZN, BAM, BBD, BDT, BGN, BIF, BMD, BND, BOB, BRL, BSD, BTN, BWP, BYN, BZD, CAD, CDF, CHF, CLP, CNY, COP, CRC, CVE, CZK, DJF, DKK, DOP, DZD, EGP, ETB, EUR, FJD, FKP, GBP, GEL, GHS, GIP, GMD, GTQ, GYD, HKD, HNL, HRK, HTG, HUF, IDR, ILS, INR, ISK, JMD, JPY, KES, KHR, KMF, KRW, KYD, KZT, LAK, LBP, LKR, LRD, LSL, MAD, MDL, MGA, MKD, MMK, MNT, MOP, MRU, MUR, MVR, MWK, MXN, MYR, MZN, NAD, NGN, NIO, NOK, NPR, NZD, PAB, PEN, PGK, PHP, PKR, PLN, PYG, QAR, RON, RSD, RWF, SAR, SBD, SCR, SEK, SGD, SHP, SLL, SOS, SRD, STN, SZL, THB, TJS, TOP, TRY, TTD, TWD, TZS, UAH, UGX, USD, UYU, UZS, VND, VUV, WST, XAF, XCD, XOF, XPF, YER, ZAR, ZMW" } | ||
|
|
There was a problem hiding this comment.
Why is this added only in config/deployments/production.toml
| [[jpmorgan.debit]] | ||
| payment_method_type = "Visa" | ||
| [jpmorgan.connector_auth.BodyKey] | ||
| api_key="Access Token" |
There was a problem hiding this comment.
Is the API key from JPMorgan referred to as an access_token in their documentation?
There was a problem hiding this comment.
api_key is client_id and key1 is client_secret
| @@ -1,10 +1,13 @@ | |||
| pub mod transformers; | |||
| use std::convert::TryFrom; | |||
| @@ -56,84 +139,258 @@ impl TryFrom<&JpmorganRouterData<&PaymentsAuthorizeRouterData>> for JpmorganPaym | |||
| ) -> Result<Self, Self::Error> { | |||
| match item.router_data.request.payment_method_data.clone() { | |||
| PaymentMethodData::Card(req_card) => { | |||
| if item.router_data.is_three_ds() { | |||
| return Err(errors::ConnectorError::NotSupported { | |||
| message: "Three_ds payments".to_string(), | |||
There was a problem hiding this comment.
| message: "Three_ds payments".to_string(), | |
| message: "3DS Payments".to_string(), |
| let capture_method = if let Some(method) = item.router_data.request.capture_method { | ||
| map_capture_method(method) | ||
| } else { | ||
| String::from("AUTOMATIC") | ||
| }; |
There was a problem hiding this comment.
Handle the option in map_capture_method only
|
|
||
| let merchant_software = JpmorganMerchantSoftware { | ||
| company_name: String::from("JPMC"), | ||
| product_name: String::from("Hyperswitch"), //could be Amazon or something else, subject to change |
There was a problem hiding this comment.
Why is this hardcoded?
There was a problem hiding this comment.
RouterData doesn't have this, hence hardcoded
| complete: item.router_data.request.is_auto_capture()?, | ||
| account_number, | ||
| expiry, | ||
| is_bill_payment: item.router_data.request.is_auto_capture()?, |
There was a problem hiding this comment.
What is the use of this value is_bill_payment?
There was a problem hiding this comment.
nothing as such, just a field inside cards. Should i remove this?
There was a problem hiding this comment.
Remove it if it is not mandatory
| //in jpm, we get a client id and secret and using these two, we have a curl, we make an api call and we get a access token in res with an expiry time as well | ||
| #[derive(Debug)] | ||
| pub struct JpmorganAuthType { | ||
| pub(super) api_key: Secret<String>, | ||
| #[allow(dead_code)] | ||
| pub(super) key1: Secret<String>, | ||
| } |
There was a problem hiding this comment.
Why do we have dead code?
bsayak03
force-pushed
the
jpmorgan-cards
branch
2 times, most recently
from
d7378fd to
17d3b31
Compare
| }; | ||
| use hyperswitch_interfaces::errors; | ||
| use masking::Secret; | ||
| use serde::{Deserialize, Serialize}; | ||
| use strum::Display; |
There was a problem hiding this comment.
Apply match statement on enum
| fn map_capture_method(capture_method: CaptureMethod) -> CapMethod { | ||
| match capture_method { | ||
| CaptureMethod::Automatic => CapMethod::Now, | ||
| CaptureMethod::Manual | CaptureMethod::ManualMultiple => CapMethod::Manual, | ||
| CaptureMethod::Scheduled | CaptureMethod::SequentialAutomatic => CapMethod::Delayed, | ||
| } |
There was a problem hiding this comment.
Only map the capture methods that you actually support
| complete: item.router_data.request.is_auto_capture()?, | ||
| account_number, | ||
| expiry, | ||
| is_bill_payment: item.router_data.request.is_auto_capture()?, |
There was a problem hiding this comment.
Remove it if it is not mandatory
| pub trait FromTransactionState { | ||
| fn from_transaction_state(transaction_state: JpmorganTransactionState) -> Self; | ||
| } | ||
|
|
||
| pub trait FromResponseStatus { | ||
| fn from_response_status(transaction_state: String) -> Self; | ||
| } |
There was a problem hiding this comment.
No need to use trait. Directly write down the functions like attempt_status_from_transaction_state and refund_status_from_transaction_state
| JpmorganResponseStatus::Error => String::from("Error"), | ||
| }; | ||
|
|
||
| let status = common_enums::AttemptStatus::from_response_status(response_status); |
There was a problem hiding this comment.
Directly do status mapping here. No need to define a function for a single time use
bsayak03
force-pushed
the
jpmorgan-cards
branch
from
4aaf099 to
00115ff
Compare
| req: &RefreshTokenRouterData, | ||
| connectors: &Connectors, | ||
| ) -> CustomResult<Option<Request>, errors::ConnectorError> { | ||
| let req = Some( |
There was a problem hiding this comment.
Ok(Some( RequestBuilder::new() .method(Method::Post) .attach_default_headers() .headers(RefreshTokenType::get_headers(self, req, connectors)?) .url(&RefreshTokenType::get_url(self, req, connectors)?) .set_body(RefreshTokenType::get_request_body(self, req, connectors)?) .build(), ))
|
|
||
| let connector_response_reference_id = Some(item.response.transaction_id.clone()); | ||
|
|
||
| let resource_id = ResponseId::ConnectorTransactionId(item.response.transaction_id); |
There was a problem hiding this comment.
these values can be directly assigned while returning
bsayak03
force-pushed
the
jpmorgan-cards
branch
from
bb66934 to
8415fe3
Compare
deepanshu-iiitu
requested review from
SanchithHegde,
sai-harsha-vardhan and
likhinbopanna
config/config.example.toml
Outdated
| @@ -223,6 +223,7 @@ iatapay.base_url = "https://sandbox.iata-pay.iata.org/api/v1" | |||
| inespay.base_url = "https://apiflow.inespay.com/san/v21" | |||
| itaubank.base_url = "https://sandbox.devportal.itau.com.br/" | |||
| jpmorgan.base_url = "https://api-mock.payments.jpmorgan.com/api/v2" | |||
| jpmorgan.secondary_base_url= "https://id.payments.jpmorgan.com/am/oauth2/alpha/access_token" | |||
There was a problem hiding this comment.
Why are we including the entire URL here instead of just the base URL?
There was a problem hiding this comment.
the secondary base url is different from the base url, it is actually used for retrieving the access token, it is a separate api call
There was a problem hiding this comment.
Shouldn't we be including only the base URL here instead of the entire URL?
There was a problem hiding this comment.
so where should we include the access token flow's url?
| pub(super) _api_key: Secret<String>, | ||
| pub(super) _key1: Secret<String>, |
There was a problem hiding this comment.
Can we remove the underscore prefix if we are using this struct?
If we are not using this struct, why are we not using it?
There was a problem hiding this comment.
no we can't remove that struct, because if we do then the Payments Connector Create flow will be successfully even if im using HeaderKey, which shouldn't be the case. I should get a success response only when using the BodyKey
There was a problem hiding this comment.
These keys are unused because JPMorgan only requires an access token. The access token is obtained during the token flow using the JPMorgan API keys, which are provided through the AccessTokenRequestData.
| expiry: Option<ExpiryResponse>, | ||
| card_type: Option<String>, | ||
| card_type_name: Option<String>, | ||
| masked_account_number: Option<String>, |
There was a problem hiding this comment.
Should we consider wrapping this within a Secret?
| month: Option<i32>, | ||
| year: Option<i32>, |
There was a problem hiding this comment.
Can we wrap this within a Secret? This would otherwise log these values in plaintext...
|
|
||
| #[derive(Default, Debug, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ExpiryCapReq { |
There was a problem hiding this comment.
Wrap fields carrying sensitive information within Secret.
|
|
||
| #[derive(Default, Debug, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct SoftMerchantCapReq { |
There was a problem hiding this comment.
Wrap fields carrying sensitive information within Secret.
| let capture_method = | ||
| map_capture_method(item.router_data.request.capture_method.unwrap_or_default()); | ||
| Ok(Self { | ||
| capture_method: Some(capture_method?), |
There was a problem hiding this comment.
Nit: Can you add the ? on the map_capture_method() call instead of here?
| pub struct CardCapRes { | ||
| pub card_type: Option<String>, | ||
| pub card_type_name: Option<String>, | ||
| unmasked_account_number: Option<String>, |
| pub struct TransactionData { | ||
| payment_type: Option<String>, | ||
| status_code: i32, | ||
| txn_secret: Option<String>, |
There was a problem hiding this comment.
Should this field be wrapped within a Secret?
bsayak03
dismissed stale reviews from sai-harsha-vardhan, mrudulvajpayee4935, and deepanshu-iiitu
via
2d09c26
bsayak03
force-pushed
the
jpmorgan-cards
branch
3 times, most recently
from
dc135fe to
1666b1a
Compare
| impl From<JpmorganTransactionStatus> for common_enums::AttemptStatus { | ||
| fn from(item: JpmorganTransactionStatus) -> Self { |
There was a problem hiding this comment.
Why is this function in code if it is not being used anywhere?
| impl From<JpmorganTransactionState> for common_enums::AttemptStatus { | ||
| fn from(item: JpmorganTransactionState) -> Self { | ||
| match item { | ||
| JpmorganTransactionState::Authorized => Self::Authorized, | ||
| JpmorganTransactionState::Closed => Self::Charged, | ||
| JpmorganTransactionState::Declined | JpmorganTransactionState::Error => Self::Failure, | ||
| JpmorganTransactionState::Pending => Self::Pending, | ||
| JpmorganTransactionState::Voided => Self::Voided, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this function in code if it is not being used anywhere?
bsayak03
force-pushed
the
jpmorgan-cards
branch
2 times, most recently
from
0713c41 to
5d10e37
Compare
loadtest/config/development.toml
Outdated
| @@ -120,6 +120,7 @@ iatapay.base_url = "https://sandbox.iata-pay.iata.org/api/v1" | |||
| inespay.base_url = "https://apiflow.inespay.com/san/v21" | |||
| itaubank.base_url = "https://sandbox.devportal.itau.com.br/" | |||
| jpmorgan.base_url = "https://api-mock.payments.jpmorgan.com/api/v2" | |||
| jpmorgan.secondary_base_url="https://id.payments.jpmorgan.com/am/oauth2/alpha/access_token" | |||
There was a problem hiding this comment.
Nit:
| jpmorgan.secondary_base_url="https://id.payments.jpmorgan.com/am/oauth2/alpha/access_token" | |
| jpmorgan.secondary_base_url="https://id.payments.jpmorgan.com" |
bsayak03
force-pushed
the
jpmorgan-cards
branch
from
5d10e37 to
2045fba
Compare
Type of Change
Description
Additional Changes
Motivation and Context
https://developer.payments.jpmorgan.com/docs/commerce/online-payments/capabilities/online-payments/how-to/auth-and-capture-payment
https://developer.payments.jpmorgan.com/api/commerce/online-payments/online-payments#/operations/V2PaymentPost
Closes juspay/hyperswitch-cloud#7569
How did you test it?
Cypress Tests
Test Passes for
1. No3DSAutoCapture
2. 3DSAutoCapture
3. No3DSManualCapture
4. VoidPayment
5. SyncPayment
6. RefundPayment
7. SyncRefund
Test Cases failing for
1. RefundPayment : The connector currently sends the same transaction ID for every request in the sandbox environment, causing refund failures. While the refund flow has been implemented in the code, it has been commented out, and an error message is thrown by default until the connector resolves this issue.
2. SyncRefund : Since refunds are not being processed, the sync refund functionality is expected to fail. To address this, the TRIGGER_FLAG is used in Cypress tests to prevent failures. However, the Sync Refund logic has already been implemented in the code.
Following flows need to be tested for card payments for new connector Jpmorgan:
1. Authorize (Manual)
2. Capture (Manual)
3. Authorize + Capture
4. PSync
5. Refund (Issue in the refund flow, because the connector sends the same transaction id for every hit on sandbox, hence conflicting)
6. RSync
7. Cancel/Void
Checklist
cargo +nightly fmt --allcargo clippy