-
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
refactor(connector): add amount conversion framework to Riskified #6359
base: main
Are you sure you want to change the base?
refactor(connector): add amount conversion framework to Riskified #6359
Conversation
Review changes with SemanticDiff. Analyzed 4 of 4 files. Overall, the semantic diff is 29% smaller than the GitHub diff.
|
Hi @Sidharth-Singh10 , Thanks for the PR, There has been some correction in the issue, Actually Riskified accepts amount as Major unit(ex : Dollars in US), could you please make the same changes with amount type as Major Unit. Please let us know if need any other info. Thanks |
Hi @srujanchikke, so the change is to replace |
Yes, you're right |
@srujanchikke, I’ve made the requested changes. |
cart_token: payment_data.attempt_id.clone(), | ||
line_items: payment_data | ||
.request | ||
.get_order_details()? | ||
.iter() | ||
.map(|order_detail| LineItem { | ||
price: order_detail.amount, | ||
price: StringMajorUnit::new(order_detail.amount.to_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.
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.
I made the requested changes at other places.
but here the amount
' is field of OrderDetailsWithAmount
struct which is i64
, I am unable to grasp how to use it directly.
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.
@Sidharth-Singh10 , But the change you made actually passes 1000 cents as 1000 dollars to connector. StringMajor constructor doesn't do the amount conversion logic. You can pass line items to ConnectorRouterData (RisikifiedRouterData) and add you conversion logic over there, Also it's better removing constructor of StringMajor as public 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.
@Sidharth-Singh10 , But the you made change actually passes 1000 cents as 1000 dollars to connector. StringMajor constructor doesn't do the amount conversion logic. You can pass line items to ConnectorRouterData (RisikifiedRouterData) and add you conversion logic over there, Also it's better removing constructor of StringMajor as public function.
Hi @Sidharth-Singh10 , can you please address this comment.
@@ -156,14 +176,14 @@ impl TryFrom<&frm_types::FrmCheckoutRouterData> for RiskifiedPaymentsCheckoutReq | |||
created_at: common_utils::date_time::now(), | |||
updated_at: common_utils::date_time::now(), | |||
gateway: payment_data.request.gateway.clone(), | |||
total_price: payment_data.request.amount, | |||
total_price: StringMajorUnit::new(payment_data.request.amount.to_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.
total_price: StringMajorUnit::new(payment_data.request.amount.to_string()), | |
total_price: payment.amount.clone(), |
let amount = convert_amount( | ||
self.amount_converter, | ||
MinorUnit::new(req.request.amount), | ||
req.request | ||
.currency | ||
.ok_or(errors::ConnectorError::MissingRequiredField { | ||
field_name: "currency", | ||
})?, | ||
)?; |
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.
Similar change needs to be done wherever amount is being used like FrmTransactionRouterData
.
Hi @Sidharth-Singh10 , |
Type of Change
Description
This PR does amount conversion changes for Riskified Connector and it accepts the amount in MinorUnit
Additional Changes
Motivation and Context
Fixes #6139
Checklist
cargo +nightly fmt --all
cargo clippy