-
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): [ACI] implement Card Mandates for ACI #1174
Conversation
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.
@Sakilmostak as discussed lets have the try_from for the payment methods
let payment_data = match wallet_data { | ||
api_models::payments::WalletData::MbWay(data) => Self::Wallet(Box::new(WalletPMData { | ||
payment_brand: PaymentBrand::Mbway, | ||
account_id: Some(data.telephone_number.clone()), |
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.
why account_id is mapped with telephone_number?
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.
For MBway, the account Id is accepting in the telephone number format i.e telephone number is the account id for MBway in ACI
#[serde(flatten)] | ||
pub txn_details: TransactionDetails, | ||
#[serde(flatten)] | ||
pub payment_method: PaymentDetails, | ||
#[serde(flatten)] | ||
pub instruction: Option<Instruction>, |
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 there a reason why #[serde(flatten)]
is being used here? Are these types being reused anywhere else?
#[derive(Debug, Clone, Serialize)] | ||
#[serde(rename_all = "UPPERCASE")] | ||
pub enum InstructionSource { | ||
// Cardholder initiated transaction | ||
Cit, | ||
// Merchant initiated transaction | ||
Mit, | ||
} |
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.
#[derive(Debug, Clone, Serialize)] | |
#[serde(rename_all = "UPPERCASE")] | |
pub enum InstructionSource { | |
// Cardholder initiated transaction | |
Cit, | |
// Merchant initiated transaction | |
Mit, | |
} | |
#[derive(Debug, Clone, Serialize)] | |
pub enum InstructionSource { | |
#[serde(rename = "CIT")] | |
CardholderInitiatedTransaction, | |
#[serde(rename = "MIT")] | |
MerchantInitiatedTransaction, | |
} |
This can be done instead, to keep it readable and achieve the desired result. You can take this up on a separate PR.
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.
Ok, resolving this in a seperate PR
Type of Change
Description
Additional Changes
Motivation and Context
In ACI connector Card Mandates were not implemented, but it is important payment method which is needed in market so I implemented it.
How did you test it?
I tested it using Postman
Checklist
cargo +nightly fmt --all
cargo clippy