-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(core): Added Dual Refunds Validation - Chargeback+Refund #10533
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
base: main
Are you sure you want to change the base?
Conversation
6e6a36b to
bfbf4bf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10533 +/- ##
=======================================
Coverage ? 6.46%
=======================================
Files ? 1251
Lines ? 312647
Branches ? 0
=======================================
Hits ? 20210
Misses ? 292437
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
crates/api_models/src/disputes.rs
Outdated
| pub merchant_connector_id: Option<common_utils::id_type::MerchantConnectorAccountId>, | ||
| /// Shows if the disputed amount is already refunded in the payment | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub is_already_refunded: Option<bool>, |
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 any usecase for this being null? We can try to avoid nullable case for booleans as much as possible
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.
Sure, we can make it a req field
crates/diesel_models/src/types.rs
Outdated
| Eq, | ||
| AsExpression, | ||
| FromSqlRow, | ||
| utoipa::ToSchema, |
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 shouldn't be using utoipa in diesel_models. If you want to use this in API models, please move this to common_types or api_models. If not, please remove utoipa::ToSchema
crates/diesel_models/Cargo.toml
Outdated
| thiserror = "1.0.69" | ||
| time = { version = "0.3.41", features = ["serde", "serde-well-known", "std"] } | ||
|
|
||
| utoipa = { version = "4.2.3", features = ["preserve_order", "preserve_path_order"] } |
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.
diesel_models shouldn't import utoipa
| connector.id(), | ||
| ) | ||
| .await?; | ||
| if (option_dispute.is_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.
Try to use a single .map with a impl on dispute object to derive the condition
crates/router/src/core/disputes.rs
Outdated
| let current_state = payment_intent.state_metadata.unwrap_or_default(); | ||
|
|
||
| dispute_response.is_already_refunded = Some( | ||
| payment_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.
Can we move all the validation logic into a single 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.
but dispute response validation and refund request validation are two different cases, they don't coincide with each other, i mean the same code isn't written twice.
| } | ||
|
|
||
| #[cfg(feature = "v1")] | ||
| pub trait PaymentIntentStateMetadataExt { |
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 trait, because there's no polymorphism here. We always operate on defined type with defined behaviours. We can have impl based functions right, you may construct a struct to unify the behaviours into impls?
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.
but i'm declaring my struct PaymentIntentStateMetadata in common_types crate and if i move these trait fn's inside common_types crates, then i have to import SessionState which depends on router crate. router crate and common_types is inter dependent on each other creating a cyclic dependency. Hence i have to keep these impl's outside of where i declare them. Hence would need a trait.
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 actually create a struct wrapping your type in router to have impls on it, traits can create unnecessary overhead in absence of polymorphism
| impl PaymentIntentStateMetadataExt for diesel_models::types::PaymentIntentStateMetadata { | ||
| fn validate_refund_against_intent_state_metadata( | ||
| self, | ||
| refund: &api::RefundRequest, |
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.
It's not advisable to operate on refund_request in payments core flow
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.
but we would need the requested refund amount in this fn. so instead of the whole object should we accept just refund 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.
You can get refund_amount as an input
crates/router/src/core/payments.rs
Outdated
| utils::when( | ||
| total_disputed + total_refunded + requested > captured, | ||
| || { | ||
| Err(report!(errors::ApiErrorResponse::InvalidDataFormat { |
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 not a InvalidDataFormat error right?
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.
changing to InvalidRequestData, that looks to be the most relevant one amongst the variants
|
|
||
| let total_refunded_amount: i64 = all_refunds_for_payment | ||
| .iter() | ||
| .filter(|r| r.refund_status == common_enums::RefundStatus::Success) |
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 be a impl
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 already inside an impl right?
sai-harsha-vardhan
left a comment
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 would also need changes in refund core flows to update the intent state right? @bsayak03
| ) | ||
| .await?; | ||
| if diesel_models::dispute::Dispute::is_not_lost_or_none(&option_dispute) | ||
| && dispute_object.dispute_status == common_enums::DisputeStatus::DisputeLost |
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 already checked in is_not_lost_or_none right?
| option | ||
| .as_ref() | ||
| .map(|d| d.dispute_status != common_enums::DisputeStatus::DisputeLost) | ||
| .unwrap_or(true) |
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 it is None, we do not want to update the intent state right?
| ) | ||
| })?; | ||
|
|
||
| payment_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.
We may have to call this only when refund reaches success state right?
| { | ||
| let current_state = payment_intent.state_metadata.unwrap_or_default(); | ||
|
|
||
| dispute_response.is_already_refunded = payment_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.
Can we move this validation logic to a function?
| } | ||
|
|
||
| #[cfg(feature = "v1")] | ||
| pub trait PaymentIntentStateMetadataExt { |
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 actually create a struct wrapping your type in router to have impls on it, traits can create unnecessary overhead in absence of polymorphism
| impl PaymentIntentStateMetadataExt for diesel_models::types::PaymentIntentStateMetadata { | ||
| fn validate_refund_against_intent_state_metadata( | ||
| self, | ||
| refund: &api::RefundRequest, |
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 get refund_amount as an input
| // Block refund if total disputed amount + total refunded amount + requested refund amount > amount captured | ||
| if let Some(amount_captured) = payment_intent.amount_captured { | ||
| let captured = amount_captured.get_amount_as_i64(); | ||
| let total_disputed = self |
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 can merge dispute and refund validations into a single function which can be impl on payment_intent
It takes, blocked amount as an input whether it's disputed or refunded, it can take requested_amount as one more paremeter. It validates it with the amount_captured value. Give a though on this to merge behaviours and reuse the functionality
Type of Change
Description
In this PR, we have added a
state_metadatacolumn inpayment_intenttable which stores the state of thetotal_refunded_amountandtotal_disputed_amountfor that particular payment_intent. The goal is to add a validation check whenever refunds are being attempted after a chargeback has already occured.In cases where refund occurs first and then chargeback we cannot stop this event from happening but to flag these cases out in the dashboard, we have added a query param
expand_allwhich can be set to true for the Dispute Sync API. Once this is called from the FE, BE will send thetotal_refunded_amountandtotal_disputed_amountDispute Sync which will allow the dashboard to flag such cases.Additional Changes
Motivation and Context
How did you test it?
Case 1: Chargeback happens and then Refund
Send a chargeback webhook to HS Server with the help of Novalnet Webhook Simulator (you being the connector) :
Step 1: Make a SEPA Payment Create + Confirm
cURL :
Response :
Step 2: Make a POST callback to HS server. Take Connector Transaction Id from Response and paste in Novalnet's webhook Simulator. Paste payment access key which is merchant secret :
cURL :
Check Intent and Dispute Table :
Step 3: Initiate a Partial Refund.
cURL :
Response :
Step 4: Intiate another Partial Refund (total disputed amount + previous refunds + new refund amount requested > amount captured => should throw error)
cURL :
Response :
Step 5: Intiate a Refund amount which is within the limits (total disputed amount + previous refunds + new refund amount requested <= amount captured => should go through)
cURL :
Response :
Case 2 : Refund happens then Chargeback (can't stop this but can definitely flag these cases in dashboard)
Step 1 : Make a SEPA Payment
cURL :
Response:
Step 2 : Make a Refund happen
Note: Refund will not get succeeded directly in test env. Need to submit connector tx id to Novalnet support team before making a refund. They will do some adjustments in their system upon which a refund can be triggered.
cURL :
Response :
Step 3: Make a POST callback to HS server for Chargeback
cURL :
Step 4 : Make a Dispute Sync API call with
expand_allquery param set to true.cURL :
Response :
Checklist
cargo +nightly fmt --allcargo clippy