-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(router): modify net_amount
to be a struct in the domain model of payment_attempt and handle amount changes across all flows
#6252
Conversation
…nto refactor-net-amount
|
||
pub fn set_surcharge_details( | ||
&mut self, | ||
surcharge_details: Option<crate::router_request_types::SurchargeDetails>, |
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.
surcharge_details: Option<crate::router_request_types::SurchargeDetails>, | |
surcharge_details: Option<router_request_types::SurchargeDetails>, |
@@ -303,9 +290,10 @@ impl PaymentAttemptNew { | |||
/// returns amount + surcharge_amount + tax_amount (surcharge) + shipping_cost + order_tax_amount | |||
pub fn calculate_net_amount(&self) -> MinorUnit { |
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 this function is required when we have get_total_amount
from NetAmount
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.
This impl
is on diesel models of PaymentAttemptNew, in which net_amount is still a MinorUnit
surcharge: common_utils::types::Surcharge::Fixed( | ||
request_surcharge_details.surcharge_amount, | ||
), | ||
tax_on_surcharge: None, | ||
surcharge_amount, | ||
tax_on_surcharge_amount, | ||
final_amount: payment_attempt.amount + surcharge_amount + tax_on_surcharge_amount, | ||
final_amount: payment_attempt.net_amount.get_order_amount() |
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 we are not considering get_total_amount
here? Reason for again doing calculation 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.
We are trying to calculate surcharge_details from the RequestSurchargeDetails, net_amount is not computed yet. That's why we are getting surcharge details from RequestSurchargeDetails and calculating SurchargeDetails
crates/router/src/core/payments.rs
Outdated
.net_amount | ||
.get_tax_on_surcharge() | ||
.unwrap_or_default(); | ||
let final_amount = payment_attempt.net_amount.get_order_amount() |
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 we are calculation net_amount instead of using get_total_amount
function?
.or(payment_attempt | ||
.and_then(|payment_attempt| payment_attempt.net_amount.get_shipping_cost())) | ||
.unwrap_or_default(); | ||
let total_capturable_amount = original_amount |
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 we are calculating this instead of using get_total_amount
?
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.
This is being called in validate_request flow also, at that point payment_attempt is not present yet. So, we have to take details from request and calculate the amount
@@ -106,6 +100,12 @@ impl<F: Send + Clone> | |||
.await | |||
.to_not_found_response(errors::ApiErrorResponse::PaymentNotFound)?; | |||
|
|||
if payment_attempt.get_total_amount() > request.amount { |
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.
This can't be true with get_total_amount
, because request.amount
is a order_amount
which can be lower than the get_total_amount. Or In incremental_authotization they can pass total amount in request?
If genuine cases also this can throw errors.
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.
In incremental_authotization, request.amount is total_amount which is on top of net_amount (not order_amount 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.
Add comments explain the case
@@ -232,7 +232,7 @@ pub fn make_dsl_input( | |||
}; | |||
|
|||
let payment_input = dsl_inputs::PaymentInput { | |||
amount: payments_dsl_input.payment_intent.amount, | |||
amount: payments_dsl_input.payment_attempt.get_total_amount(), |
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 Is routing need total_amount or just order_amount? In other places dsl_inputs do we need total amount?
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.
Ideally we should be doing on net_amount right? @Aprabhat19
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 we would do it on total_amount, cause he could have a rule saying amount
>1000, route it to connector_b
. So total_amount would be a more accurate field for making such decisions
@@ -1340,8 +1331,11 @@ impl<F: Clone> UpdateTracker<F, PaymentData<F>, api::PaymentsRequest> for Paymen | |||
client_source, | |||
client_version, | |||
customer_acceptance: payment_data.payment_attempt.customer_acceptance, | |||
shipping_cost, | |||
order_tax_amount, | |||
shipping_cost: payment_data.payment_intent.shipping_cost, |
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.
we need not have these fields in the flattened format in update right? can we use the amount struct in update as well?
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.
Based on the flow, we would need to restrict the updating of fields. Let's say in payments update flow, we don't want to update surcharge and tax amount.
@@ -694,6 +694,10 @@ impl<F: Clone + Send> Domain<F, api::PaymentsRequest, PaymentData<F>> for Paymen | |||
}), | |||
payment_method_type: None, | |||
}); | |||
payment_data |
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.
we should not update payment attempt when calculating the tax, since this operation is not dependent on payment attempt. @swangi-kumari can you please confirm this once?
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 ideally it shouldn't depend on payment attempt but in v1, we are storing tax calculation details in net_amount of payment_attempt. In v2, we can have different way to handle this
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.
There is separate field storing this data in intent itself, @swangi-kumari can you confirm this once?
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.
We store tax_details in payment_intent
pub struct PaymentIntent {
pub tax_details: Option<TaxDetails>,
}
pub struct TaxDetails {
pub default: Option<DefaultTax>,
pub payment_method_type: Option<PaymentMethodTypeTax>,
}
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, we store tax_details in payment_intent, but we would need to update payment_attempt.net_amount with order_tax_amount too so that all the subsequent flows would use the updated net_amount which includes order_tax_amount
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.
After confirm only we need to compute total_amount using intent, for create the related information will be stored on intent and the computation also on intent.
MinorUnit::from(amount), | ||
request.shipping_cost, | ||
None, | ||
surcharge_amount, |
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.
since all the fields here are MinorUnit, there might be chances where one amount is passed as another amount. Can we instead have this as a from function from the request instead of the new function, we can preserve the context of fields in that case when converting.
@@ -694,6 +694,10 @@ impl<F: Clone + Send> Domain<F, api::PaymentsRequest, PaymentData<F>> for Paymen | |||
}), | |||
payment_method_type: None, | |||
}); | |||
payment_data |
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.
There is separate field storing this data in intent itself, @swangi-kumari can you confirm this once?
@@ -106,6 +100,12 @@ impl<F: Send + Clone> | |||
.await | |||
.to_not_found_response(errors::ApiErrorResponse::PaymentNotFound)?; | |||
|
|||
if payment_attempt.get_total_amount() > request.amount { |
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.
Add comments explain the case
@@ -212,8 +212,9 @@ impl ForeignTryFrom<(&SurchargeDetails, &PaymentAttempt)> for SurchargeDetailsRe | |||
.tax_on_surcharge_amount | |||
.get_amount_as_i64(), | |||
)?; | |||
let display_final_amount = currency | |||
.to_currency_base_unit_asf64(surcharge_details.final_amount.get_amount_as_i64())?; | |||
let display_final_amount = currency.to_currency_base_unit_asf64( |
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.
@hrithikesh026 Is this final_amount is total_amount including all amount related calculation or only order_amount + surcharge?
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.
Currently display_final_amount
is not being used, removing it
@@ -528,6 +526,10 @@ impl SurchargeDetails { | |||
pub fn get_total_surcharge_amount(&self) -> MinorUnit { | |||
self.surcharge_amount + self.tax_on_surcharge_amount | |||
} | |||
|
|||
pub fn get_final_amount(&self) -> MinorUnit { |
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 really needed this get_final_amount with just order_amount?
…nto refactor-net-amount
a653f88
to
efc6856
Compare
@@ -694,6 +694,10 @@ impl<F: Clone + Send> Domain<F, api::PaymentsRequest, PaymentData<F>> for Paymen | |||
}), | |||
payment_method_type: None, | |||
}); | |||
payment_data |
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.
After confirm only we need to compute total_amount using intent, for create the related information will be stored on intent and the computation also on intent.
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.
LGTM
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.
@Gnanasundari24 This is a critical change, please test all payment, refund and dispute flows.
…del of payment_attempt and handle amount changes across all flows (#6252) Co-authored-by: swangi-kumari <swangi.12015941@lpu.in> Co-authored-by: Swangi Kumari <85639103+swangi-kumari@users.noreply.github.com> Co-authored-by: hyperswitch-bot[bot] <148525504+hyperswitch-bot[bot]@users.noreply.github.com>
Type of Change
Description
Modify
net_amount
to be a struct in the domain model of payment_attempt and handle amount changes across all flows.Additional Changes
Motivation and Context
How did you test it?
Tested Manually
shipping_cost
sent in the request, amount = 123, shipping_cost = 100. So, net_amount would be 223. The amount sent to the connector for authorize, capture and refunds should be able to be done against the net_amount of 223Create CURL
Response
Confirm CURL
Response
Amount sent to connector
Capture the payment for net_amount 223
CURL
Response
Refund for net_amount of 223
CURL
Response
Checklist
cargo +nightly fmt --all
cargo clippy