-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(connector): [Paybox] Add mandates Flow for Paybox #6378
base: main
Are you sure you want to change the base?
Conversation
// payment_method_data is not required during recurring mandate payment, in such case keep default PaymentMethodData as MandatePayment | ||
let additional_payment_method_data = if payment_data.mandate_id.is_some() { |
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 this comment applicable for below code too?
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.
Not needed
@@ -145,6 +146,9 @@ impl | |||
integrity_check: Ok(()), | |||
additional_merchant_data: None, | |||
header_payload, | |||
connector_mandate_request_reference_id: Some(common_utils::generate_id_with_len( |
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.
Same here
@@ -1557,7 +1561,9 @@ async fn payment_response_update_tracker<F: Clone, T: types::Capturable>( | |||
None | |||
}; | |||
|
|||
if router_data.status == enums::AttemptStatus::Charged { | |||
if router_data.status == enums::AttemptStatus::Charged |
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.
can use matches!
here and please add the relevant code comments
@@ -142,6 +142,8 @@ pub const DEFAULT_UNIFIED_ERROR_MESSAGE: &str = "Something went wrong"; | |||
// Recon's feature tag | |||
pub const RECON_FEATURE_TAG: &str = "RECONCILIATION AND SETTLEMENT"; | |||
|
|||
pub const CONNECTOR_MANDATE_REQUEST_REFERENCE_ID_LENGTH: usize = 18; |
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 a relevant code comment here
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.
added
card_exp_month: String, | ||
card_exp_year: 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.
Can this function accept Secret<String>
instead? We can call peek()
within the function.
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.
changed
pub card_exp_month: Secret<String>, | ||
pub card_exp_year: Secret<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.
You can also use CardExpirationMonth
and CardExpirationMonth
from the cards
crate here, instead of Secret<String>
. They also allow formatting the year with 2/4 digits, as required.
@@ -145,6 +146,9 @@ impl | |||
integrity_check: Ok(()), | |||
additional_merchant_data: None, | |||
header_payload, | |||
connector_mandate_request_reference_id: Some(common_utils::generate_id_with_len( |
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 we generate a random ID / string but don't store it at all in our database, then it would be of no use to us. Can we please avoid such usages?
@@ -329,7 +329,7 @@ impl<F: Send + Clone> GetTracker<F, PaymentData<F>, api::PaymentsRequest> for Pa | |||
api_models::payments::MandateIds { | |||
mandate_id: Some(mandate_obj.mandate_id), | |||
mandate_reference_id: Some(api_models::payments::MandateReferenceId::ConnectorMandateId( | |||
api_models::payments::ConnectorMandateReferenceId {connector_mandate_id:connector_id.connector_mandate_id,payment_method_id:connector_id.payment_method_id, update_history: None, mandate_metadata:connector_id.mandate_metadata, }, | |||
api_models::payments::ConnectorMandateReferenceId {connector_mandate_id:connector_id.connector_mandate_id,payment_method_id:connector_id.payment_method_id, update_history: None, mandate_metadata:connector_id.mandate_metadata,connector_mandate_request_reference_id:connector_id.connector_mandate_request_reference_id}, |
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.
The formatting seems to be messed up here. 🤔
connector_mandate_request_reference_id: Some(common_utils::generate_id_with_len( | ||
consts::CONNECTOR_MANDATE_REQUEST_REFERENCE_ID_LENGTH.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.
Same here, generating a random ID and sharing it with the connector, but not storing it on our database is useless. We won't have a track of the value we'd sent to the connector, and we can't take any further actions where we need the value.
We should ideally avoid such usages in the first place.
connector_mandate_request_reference_id: Some(common_utils::generate_id_with_len( | ||
consts::CONNECTOR_MANDATE_REQUEST_REFERENCE_ID_LENGTH.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.
Same here.
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.
Removed
serde_json::Value::Null => None, // Handle null case | ||
_ => Some(data.parse_value("AdditionalPaymentData")), |
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.
Do we explicitly need to handle the serde_json::Value::Null
case? Won't parse_value()
handle that: if the JSON value is not the expected type (say a map in this case), then deserialization should fail?
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.
Not really , That was redundant
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 deserialization will fail
cb851dc
to
8d4780a
Compare
connector_mandate_ids.get_connector_mandate_id()?, | ||
connector_mandate_ids | ||
.get_connector_mandate_id() | ||
.ok_or_else(missing_field_err("mandate_id"))?, |
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.
Optional change:
There also seems to be a errors::ConnectorError::MissingConnectorMandateID
enum variant, confirm with the connector team if that enum variant would have to be raised instead.
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.
@deepanshu-iiitu Can you verify this?
crates/api_models/src/payments.rs
Outdated
pub fn update(&mut self, other: Self) { | ||
*self = other; | ||
} |
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.
Do we need this method? Is this being used anywhere?
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.
Not used anywhere, WIll remove
.and_then(|v| match serde_json::from_value::<PaymentMethodsData>(v) { | ||
Ok(data) => Some(data), | ||
Err(err) => { | ||
router_env::logger::info!( | ||
"PaymentMethodsData deserialization failed : {err}" | ||
); | ||
None | ||
} | ||
}) |
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.
Can use parse_value()
here instead, you can then log the error and call .ok()
.
Also, are we okay with proceeding further if we can't deserialize payment method from the locker?
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.
@Aprabhat19 Can you verify if we can pass None to the additional_payment_method
if deserialization fails?
.payment_method_data | ||
.clone() | ||
.map(|x| x.into_inner().expose()) | ||
.and_then(|v| match serde_json::from_value::<PaymentMethodsData>(v) { |
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.
Same here, can use parse_value()
here.
} | ||
.and_then(|v| { | ||
v.parse_value("PaymentMethodsData") | ||
.map_err(|err| { |
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.
Can use inspect_err
also instead of map_err
here (and then can skip the error being returned from the closure).
Optional change, by the way.
Type of Change
Description
1)Added Mandate flow for Paybox
2) Added a new field
additional_payment_method_data
in PaymentsAuthorizeData type3) Added
connector_mandate_request_reference_id
in RouterDataAdditional Changes
Motivation and Context
1)Paybox requires the expiration year, month, and mandate ID in our request, added
additional_payment_method
to fetch card info.2)Paybox also needs a unique ID(
connector_mandate_request_reference_id
) from us during CIT, which they'll return with their own ID for MIT payments.How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy