Skip to content

fix(revenue_recovery): get recovery list and get recovery intent response missing values fix #10508

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.

Sign up for GitHub

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

Open
sagarnaikjuspay wants to merge 1 commit into
base: main
Choose a base branch
from
+84 −10
Open

fix(revenue_recovery): get recovery list and get recovery intent response missing values fix #10508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion crates/api_models/src/payments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ pub struct RevenueRecoveryGetIntentResponse {
pub status: common_enums::RecoveryStatus,

/// The amount details for the payment
pub amount_details: AmountDetailsResponse,
pub amount_details: PaymentAmountDetailsResponse,

/// It's a token used for client side verification.
#[schema(value_type = String, example = "cs_0195b34da95d75239c6a4bf514458896")]
Expand Down Expand Up @@ -832,6 +832,10 @@ pub struct RevenueRecoveryGetIntentResponse {
#[serde(with = "common_utils::custom_serde::iso8601")]
pub expires_on: PrimitiveDateTime,

/// Time when the payment was created
#[serde(with = "common_utils::custom_serde::iso8601")]
Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generate openapi spec for the new introduced fields?

Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please generate openapi spec

pub created_at: PrimitiveDateTime, // Add this new field

/// Additional data related to some frm(Fraud Risk Management) connectors
#[schema(value_type = Option<Object>, example = r#"{ "coverage_request" : "fraud", "fulfillment_method" : "delivery" }"#)]
pub frm_metadata: Option<pii::SecretSerdeValue>,
Expand Down
77 changes: 72 additions & 5 deletions crates/router/src/core/payments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2727,10 +2727,38 @@ pub async fn revenue_recovery_get_intent_core(
0
};

let payment_attempt = state
.store
.find_payment_attempts_by_payment_intent_id(
&payment_intent.id,
platform.get_processor().get_key_store(),
platform.get_processor().get_account().storage_scheme,
)
.await
.ok()
.and_then(|attempts| {
payment_intent
.active_attempt_id
.as_ref()
.and_then(|active_attempt_id| {
attempts
.iter()
.find(|attempt| attempt.id == *active_attempt_id)
Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would active_attempt_id not be populated in intent?

Copy link
Contributor Author

@sagarnaikjuspay sagarnaikjuspay Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the payment enters revenue recovery from an external source and the attempts are marked as TriggeredBy::External so in this case active_attempt_id is set to None cause they're the external attempts and not managed by hyperswitch so active attempt is not tracked. When hyperswitch revenuer recovery creates a internal retry attempt ,in this case active attempt id is set to attempt id.

Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't list payment attempt from DB and filter it. Find a way to point query the appropriate attempt from DB.

Like if active_attempt_id is present, fetch attempt using that id.
If not present, fetch only the latest attempt for the intent_id

.cloned()
})
.or_else(|| {
attempts
.iter()
.max_by_key(|attempt| attempt.created_at)
.cloned()
})
});

let response = transformers::generate_revenue_recovery_get_intent_response(
payment_data,
recovery_status,
card_attached,
payment_attempt.as_ref(),
);

Ok(services::ApplicationResponse::Json(response))
Expand Down Expand Up @@ -8458,25 +8486,64 @@ pub async fn revenue_recovery_list_payments(
let workflow_results = join_all(workflow_futures).await;
let billing_connector_results = join_all(billing_connector_futures).await;

let attempt_futures: Vec<_> = list
.iter()
.map(|(payment_intent, payment_attempt)| {
let platform_clone = platform.clone();
Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let platform_clone = platform.clone();
let platform_ref = &platform;

Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an intent has an active attempt, its id should be populated in active_attempt_id of intent.

Copy link
Contributor Author

@sagarnaikjuspay sagarnaikjuspay Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10508 (comment)

Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this change https://github.com/juspay/hyperswitch/pull/10508/files#r2584064017

we should not clone any object. instead use reference wherever possible


async move {
if payment_attempt.is_some() {
payment_attempt.clone()
} else {
db.find_payment_attempts_by_payment_intent_id(
&payment_intent.id.clone(),
platform_clone.get_processor().get_key_store(),
platform_clone.get_processor().get_account().storage_scheme,
)
.await
.ok()
.and_then(|attempts| {
payment_intent
.active_attempt_id
.as_ref()
.and_then(|active_attempt_id| {
attempts
.iter()
.find(|attempt| attempt.id == *active_attempt_id)
.cloned()
})
.or_else(|| {
attempts
.iter()
.max_by_key(|attempt| attempt.created_at)
.cloned()
})
Comment on lines +8498 to +8520
Copy link
Contributor

@hrithikesh026 hrithikesh026 Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't list payment attempt from DB and filter it. Find a way to point query the appropriate attempt from DB.

Like if active_attempt_id is present, fetch attempt using that id.
If not present, fetch only the latest attempt for the intent_id

})
}
}
})
.collect();

let attempt_results = join_all(attempt_futures).await;

let data: Vec<api_models::payments::RecoveryPaymentsListResponseItem> = list
.into_iter()
.zip(workflow_results.into_iter())
.zip(billing_connector_results.into_iter())
.zip(attempt_results.into_iter())
.map(
|(
((payment_intent, payment_attempt), workflow_result),
billing_connector_account,
(((payment_intent, _), workflow_result), billing_connector_account),
payment_attempt,
)| {
let (calculate_workflow, execute_workflow) =
workflow_result.unwrap_or((None, None));

// Get retry threshold from billing connector account
let max_retry_threshold = billing_connector_account
.as_ref()
.and_then(|mca| mca.get_retry_threshold())
.unwrap_or(0); // Default fallback
.unwrap_or(0);

// Use custom mapping function
map_to_recovery_payment_item(
payment_intent,
payment_attempt,
Expand Down
11 changes: 7 additions & 4 deletions crates/router/src/core/payments/transformers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2177,6 +2177,7 @@ pub fn generate_revenue_recovery_get_intent_response<F, D>(
payment_data: D,
recovery_status: common_enums::RecoveryStatus,
card_attached: u32,
payment_attempt: Option<&storage::PaymentAttempt>,
) -> RevenueRecoveryGetIntentResponse
where
F: Clone,
Expand All @@ -2188,10 +2189,12 @@ where
RevenueRecoveryGetIntentResponse {
id: payment_intent.id.clone(),
profile_id: payment_intent.profile_id.clone(),
status: recovery_status, // Note: field is named 'status' not 'recovery_status'
amount_details: api_models::payments::AmountDetailsResponse::foreign_from(
payment_intent.amount_details.clone(),
),
status: recovery_status,
amount_details: api_models::payments::PaymentAmountDetailsResponse::foreign_from((
&payment_intent.amount_details,
payment_attempt.map(|pa| &pa.amount_details), // Same pattern as list API
)),
created_at: payment_intent.created_at,
client_secret: client_secret.clone(),
merchant_reference_id: payment_intent.merchant_reference_id.clone(),
routing_algorithm_id: payment_intent.routing_algorithm_id.clone(),
Expand Down
Loading