-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(connector): Add support for passive churn recovery webhooks #7109
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
crates/api_models/src/webhooks.rs
Outdated
#[cfg(all(feature = "v2", feature = "revenue_recovery"))] | ||
impl IncomingWebhookEvent { | ||
pub fn is_recovery_transaction_event(&self) -> bool { | ||
match 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.
nit: can use matches!
crates/common_enums/src/enums.rs
Outdated
#[strum(serialize_all = "snake_case")] | ||
#[serde(rename_all = "snake_case")] | ||
pub enum TriggeredBy { | ||
/// Denotes payment attempt is been created by hyperswitch 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.
nit: prefer not refer hyperswitch in codebase
/// merchant reference id at billing connector. ex: invoice_id | ||
pub merchant_reference_id: common_utils::id_type::PaymentReferenceId, | ||
/// transaction id reference at payment connector | ||
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.
Can we please use ConnectorTransactionId
domain type here?
/// error message sent by billing connector. | ||
pub error_message: Option<String>, | ||
/// mandate token at payment processor end. | ||
pub processor_payment_token: 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.
mandate token, is more of processor_payment_method_token right? not payment token?
/// customer id at payment connector for which mandate is attached. | ||
pub connector_customer_id: Option<String>, | ||
/// payment merchant connnector account reference id at billing connector. | ||
pub connector_account_reference_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.
what exactly is merchant connnector account reference id?
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.
Payment gateway identifier id at billing processor. Add the same comment.
{ | ||
Ok(None) | ||
} | ||
Ok(_) | Err(_) => Err(errors::RevenueRecoveryError::PaymentIntentFetchFailed) |
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 be logging Err(err) here, and any other Ok() response is unexpected
hyperswitch_domain_models::payments::HeaderPayload::default(), | ||
)) | ||
.await; | ||
router_env::logger::info!(?attempt_response); |
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.
Are these loggers required? If yes, please add relevant keys if not there's no use of this loggers
}); | ||
Ok(payment_attempt) | ||
} | ||
Ok(_) | Err(_) => Err(errors::RevenueRecoveryError::PaymentAttemptFetchFailed) |
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
#[cfg(all(feature = "revenue_recovery", feature = "v2"))] | ||
{ | ||
route = route.service( | ||
web::resource("/recovery/{merchant_id}/{profile_id}/{connector_id}").route( |
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.
preferably stricter routes should be on top of generic routes
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 not required here since, since number of parameters vary for payment webhooks and recovery webhooks
@@ -310,6 +310,7 @@ impl From<Flow> for ApiIdentifier { | |||
Flow::RetrievePollStatus => Self::Poll, | |||
|
|||
Flow::FeatureMatrix => Self::Documentation, | |||
Flow::RecoveryIncomingWebhookReceive => Self::Webhooks, |
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.
nit: can you move this to existing webhooks match statement instead
Type of Change
Description
Recovery Incoming Webhooks service :
Consume incoming webhooks of billing platforms to schedule revenue recovery retries in hs.
Endpoint :
v2/webhooks/revenue_recovery/{{profile_id}}/{{merchant_connector_account_id}}
Authentication :
Source verification.
Method :
POST
Request :
Accepts connector specific requests , unified fields that we are going to consume and verified across billing platforms(Chargebee, Recurly, Stripe Billing).
Response :
They expect us to send 200 back.
DB changes :
triggered_by : Flag to find out whether an attempt was created external or internal. Possible enums of triggered_by EXTERNAL/ INTERNAL
Flow
Note: Only handler functions of payments core are called in webhooks flow to make revenue recovery flow modular. No direct DB call is made in recovery incoming webhook flow to payments(intent/attempt) table.
why do we need attempt.triggered_by and event type to take the decision?
Based on event we take the action but we need to triggered by to know whether it was created by Hyperswitch or External system.
Mapping between (event, triggered_by) and Actions
Note : Pending Task From this PR
Additional Changes
Motivation and Context
Cannot test this flow, will be tested once #7236 and #7110 are merged.
Checklist
cargo +nightly fmt --all
cargo clippy