-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(recovery): add support for custom billing api for v2 #8838
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
Conversation
Changed Files
|
pub status: enums::AttemptStatus, | ||
|
||
/// The billing details of the payment attempt. This address will be used for invoicing. | ||
pub billing: Option<Address>, |
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 you improve the naming
pub payment_method_units: CustomBillingPaymentMethodDataWithBilling, | ||
|
||
/// recovery action | ||
pub action: common_payments_types::RecoveryAction, |
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 you give a descriptive comment
crates/api_models/src/payments.rs
Outdated
@@ -4318,6 +4335,12 @@ pub struct PaymentMethodDataResponseWithBilling { | |||
pub billing: Option<Address>, | |||
} | |||
|
|||
#[derive(Debug, Clone, Eq, PartialEq, serde::Deserialize, ToSchema, serde::Serialize)] | |||
pub struct CustomBillingPaymentMethodDataWithBilling { |
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.
pub struct CustomBillingPaymentMethodDataWithBilling { | |
pub struct CustomRecoveryPaymentMethodData { |
)] | ||
pub id: id_type::GlobalPaymentId, | ||
|
||
#[schema(value_type = IntentStatus, example = "failed", default = "requires_confirmation")] |
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 merchant_reference_id in the response to correlate between their input and our response
crates/api_models/src/payments.rs
Outdated
|
||
/// primary payment method token at payment processor end. | ||
#[schema(value_type = String, example = "token_1234")] | ||
pub primary_processor_payment_method_token: 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.
pub primary_processor_payment_method_token: String, | |
pub primary_processor_payment_method_token: Secret<String>, |
crates/api_models/src/payments.rs
Outdated
|
||
/// customer id at payment connector for which mandate is attached. | ||
#[schema(value_type = String, example = "cust_12345")] | ||
pub connector_customer_id: 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.
pub connector_customer_id: String, | |
pub connector_customer_id: Secret<String>, |
crates/api_models/src/payments.rs
Outdated
pub billing_started_at: Option<PrimitiveDateTime>, | ||
|
||
/// Transaction if reference at payment connector end. | ||
pub connector_transaction_id: Option<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.
pub connector_transaction_id: Option<String>, | |
pub connector_transaction_id: Option<Secret<String>>, |
retry_count: None, | ||
next_billing_at: 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.
retry_count: None, | |
next_billing_at: None, | |
retry_count: None, | |
next_billing_at: None, |
can these be provided from api input? or it will not be allowed always in request?
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 will be not allowed in request, it would always be calculated in recovery system with attempt count.
> { | ||
let primary_token = &data.primary_processor_payment_method_token.to_string(); | ||
let card_info = data.payment_method_units.units.get(primary_token); | ||
let recovery_attempt = Self(revenue_recovery::RevenueRecoveryAttemptData { |
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.
let recovery_attempt = Self(revenue_recovery::RevenueRecoveryAttemptData { | |
let recovery_attempt = Self(revenue_recovery::RevenueRecoveryAttemptData::from(data) |
@@ -906,7 +967,8 @@ impl RevenueRecoveryAttempt { | |||
.change_context(errors::RevenueRecoveryError::ProcessTrackerCreationError) | |||
.attach_printable("Failed to construct process tracker entry")?; | |||
|
|||
db.insert_process(process_tracker_entry) | |||
let tracker = db |
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.
let tracker = db | |
db |
remove if not needed
), | ||
) -> CustomResult<webhooks::WebhookResponseTracker, errors::RevenueRecoveryError> { | ||
match self.action { | ||
common_types::payments::RecoveryAction::CancelInvoice => todo!(), |
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 this intentional?
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 need to implment stop flow, which will alter process tracker flow.
} | ||
} | ||
common_types::payments::RecoveryAction::SuccessPaymentExternal => { | ||
router_env::logger::info!("Payment has been succeeded via external system"); |
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.
router_env::logger::info!("Payment has been succeeded via external system"); | |
logger::info!("Payment has been succeeded via external system"); |
change here and in below places also
crates/api_models/src/payments.rs
Outdated
#[serde(default, with = "common_utils::custom_serde::iso8601::option")] | ||
pub billing_started_at: Option<PrimitiveDateTime>, | ||
|
||
/// Transaction if reference at payment connector end. |
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 sentence is not clear
crates/api_models/src/payments.rs
Outdated
pub payment_merchant_connector_id: id_type::MerchantConnectorAccountId, | ||
|
||
#[schema(value_type = AttemptStatus, example = "charged")] | ||
pub status: enums::AttemptStatus, |
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.
status
in response type is IntentStatus. Here it is AttemptStatus.
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 depend on transaction status for external payment, But want to share intent status in response.
@@ -267,6 +267,14 @@ pub async fn construct_payment_router_data_for_authorize<'a>( | |||
.and_then(|customer| customer.email.clone()) | |||
.map(pii::Email::from); | |||
|
|||
let additional_payment_method_data: Option<api_models::payments::AdditionalPaymentData> = |
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.
Where is this additional_payment_method_data
being used?
975cba7
Type of Change
Description
Revenue recovery incoming webhook flow handles events from external 3rd party subscription provider, If merchant has inhouse subscription solution and wants to use revenue recovery,
/payments/recovery
will consume the events same as incoming webhooks.Flow :
Additional Changes
Motivation and Context
How did you test it?
Create payment mca :
create custom billing mca :
Update the profile revenue_recovery_retry_algorithm_type to cascading.
Create revenue recovery
response status indicates intent is failed (Success response)
Checklist
cargo +nightly fmt --all
cargo clippy