-
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
feat(core): add payments post_session_tokens flow #6202
Conversation
…into create-order
…into create-order
} | ||
|
||
#[derive(Debug, serde::Deserialize, serde::Serialize, Clone, ToSchema)] | ||
pub struct PaymentsPostSessionTokensResponse { |
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 we using this response anywhere? I think we are sending the payments response. Instead of that can we use this struct?
) | ||
.await?; | ||
|
||
let payment_method_billing = helpers::get_address_by_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.
we will not have this billing address in this flow, can be removed
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.
Billing address would be there if merchant has sent it in payments create request right? As, this is create order call and connector expects billing address, we should be sending it? @Narayanbhat166
let m_db = db.clone().store; | ||
let payment_attempt_update = | ||
storage::PaymentAttemptUpdate::PostSessionTokensUpdate { | ||
connector_transaction_id: connector_transaction_id.clone(), |
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 connector_transaction_id
in this flow. There can be cases where confirm happens through a different payment method data and a new connector might be selected, we will have the wrong mapping of connector and transaction id.
connector: payment_data | ||
.payment_attempt | ||
.connector | ||
.clone() |
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 will not store the connector in this case. We should not update the error message in this case. This should be a bad request error.
@@ -475,6 +475,11 @@ pub enum PaymentAttemptUpdate { | |||
unified_message: Option<String>, | |||
connector_transaction_id: Option<String>, | |||
}, | |||
PostSessionTokensUpdate { | |||
updated_by: String, | |||
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.
do not update this
let req = match payment_method_data { | ||
domain::PaymentMethodData::Wallet(domain::WalletData::PaypalSdk(_)) => { | ||
services::RequestBuilder::new() | ||
.method(services::Method::Post) |
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 is this method Post if we are not sending a body?
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 using this flow to authorize the payment.
It should be Post Method - doc
_is_latency_header_enabled: Option<bool>, | ||
) -> RouterResponse<Self> { | ||
let papal_sdk_next_action = | ||
paypal_sdk_next_steps_check(payment_data.get_payment_attempt().clone())?; |
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 avoid doing this, the core orchestration layer should not handle connector specific logic. This refactor can be taken up separately
@@ -1774,6 +1809,7 @@ impl<F: Clone> TryFrom<PaymentAdditionalData<'_, F>> for types::PaymentsAuthoriz | |||
charges, | |||
merchant_order_reference_id, | |||
integrity_object: None, | |||
connector_transaction_id: payment_data.payment_attempt.connector_transaction_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.
please remove this field from the request, we do not need connector_transaction_id
to be present before the authorization has happened
.shipping_cost | ||
.unwrap_or_default(); | ||
// amount here would include amount, surcharge_amount and shipping_cost | ||
let amount = payment_data.payment_intent.amount + shipping_cost + 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.
should we not include order_tax_amount
in case this was provided by the merchant in payments create? @sai-harsha-vardhan I think we should have the net amount struct for intent as well to handle these cases where some amount might be missed
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, that is the ideal way. We can introduce net_amount in intent domain models.
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.
@swangi-kumari we should include order_tax amount here right?
.clone(); | ||
Ok(Self { | ||
amount, //need to change after we move to connector module | ||
currency: payment_data.currency, |
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: use this from payment intent. We do not want two source of truth for the same field. We plan on removing currency from payment data later in time
@@ -9,7 +9,8 @@ pub use api_models::payments::{ | |||
PaymentsApproveRequest, PaymentsCancelRequest, PaymentsCaptureRequest, | |||
PaymentsCompleteAuthorizeRequest, PaymentsDynamicTaxCalculationRequest, | |||
PaymentsDynamicTaxCalculationResponse, PaymentsExternalAuthenticationRequest, | |||
PaymentsIncrementalAuthorizationRequest, PaymentsManualUpdateRequest, PaymentsRedirectRequest, | |||
PaymentsIncrementalAuthorizationRequest, PaymentsManualUpdateRequest, |
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.
@SanchithHegde what do you think about such re exports? This adds friction when a developer is trying to move his way to from where and how this struct has been imported. In my opinion we should always qualify specific imports from the parent module. This might take some time, but I think it will be better to move away from 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.
I believe this was kept this way when we moved out API models to a separate crate, to reduce the number of changes elsewhere in code. But yeah, I think we should avoid such public re-exports.
Of course, this can be taken up in a separate PR.
.to_not_found_response(errors::ApiErrorResponse::PaymentNotFound)?; | ||
payment_data.payment_attempt = updated_payment_attempt; | ||
} | ||
Err(_) => { |
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't ignore errors here, we should send back the relevant error code and error message back in the response
.shipping_cost | ||
.unwrap_or_default(); | ||
// amount here would include amount, surcharge_amount and shipping_cost | ||
let amount = payment_data.payment_intent.amount + shipping_cost + 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.
Yes, that is the ideal way. We can introduce net_amount in intent domain models.
@@ -71,6 +71,18 @@ pub struct PaymentsAuthorizeData { | |||
/// In case the connector supports only one reference id, Hyperswitch's Payment ID will be sent as reference. | |||
pub merchant_order_reference_id: Option<String>, | |||
pub integrity_object: Option<AuthoriseIntegrityObject>, | |||
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.
This is not required anymore
crates/openapi/src/openapi_v2.rs
Outdated
@@ -579,6 +579,8 @@ Never share your secret api keys. Keep them guarded and secure. | |||
api_models::payments::PaymentsDynamicTaxCalculationRequest, | |||
api_models::payments::PaymentsDynamicTaxCalculationResponse, | |||
api_models::payments::DisplayAmountOnSdk, | |||
api_models::payments::PaymentsPostSessionTokensRequest, |
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 we are not adding the route, please remove the req / res structs from openapi_v2
event_builder: Option<&mut ConnectorEvent>, | ||
res: Response, | ||
) -> CustomResult<types::PaymentsPostSessionTokensRouterData, errors::ConnectorError> { | ||
let response: paypal::PaypalRedirectResponse = res |
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 should be PaypalOrdersResponse instead of PaypalRedirectResponse 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.
The response which get for PaypaSdk Create Order , matches the struct PaypalRedirectResponse.
@@ -955,7 +955,7 @@ fn create_paypal_sdk_session_token( | |||
connector: connector.connector_name.to_string(), | |||
session_token: paypal_sdk_data.data.client_id, | |||
sdk_next_action: payment_types::SdkNextAction { | |||
next_action: payment_types::NextActionCall::Confirm, | |||
next_action: payment_types::NextActionCall::PostSessionTokens, |
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.
These changes can only go after SDK deployment right?
Due to this flow change, it would be backward incompatible. How would we handle staggered deployment here? @Narayanbhat166 @swangi-kumari
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, this will go in sandbox after SDK sandbox deployment.
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, this will go in sandbox after SDK sandbox deployment.
What's our strategy for production rollout of this change, would it be rolled out after SDK deployment on production as well?
.to_not_found_response(errors::ApiErrorResponse::ProfileNotFound { | ||
id: profile_id.get_string_repr().to_owned(), | ||
})?; | ||
payment_attempt.payment_method = Some(request.payment_method); |
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 change required?
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, this is required for routing.
_key_store: &domain::MerchantKeyStore, | ||
storage_scheme: enums::MerchantStorageScheme, | ||
_locale: &Option<String>, | ||
#[cfg(all(feature = "v1", feature = "dynamic_routing"))] _routable_connector: Vec< |
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.
These feature flags are redundant as we have v1 on top level
}); | ||
Ok(services::ApplicationResponse::JsonWithHeaders(( | ||
Self { | ||
payment_id: payment_data.get_payment_attempt().payment_id.clone(), |
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 payment_id from payment_intent here
@@ -498,6 +498,8 @@ pub enum Flow { | |||
PaymentsManualUpdate, | |||
/// Dynamic Tax Calcultion | |||
SessionUpdateTaxCalculation, | |||
/// Payments create order 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.
/// Payments create order flow | |
/// Payments post session tokens flow |
/// The unique identifier for the payment | ||
#[serde(skip_deserializing)] | ||
#[schema(value_type = String)] | ||
pub payment_id: id_type::PaymentId, |
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 we're not obtaining this from the request (not deserializing it), why are we including it in the 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.
This is for ApiEventMetrics and ApiLocking
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.
@Narayanbhat166 Should we consider having an "internal" struct for such purposes, or are we okay with this practice?
.url(&types::PaymentsPostSessionTokensType::get_url( | ||
self, req, connectors, | ||
)?) | ||
.headers(types::PaymentsPostSessionTokensType::get_headers( |
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 call attach_default_headers()
as well?
.url(&types::PaymentsAuthorizeType::get_url( | ||
self, req, connectors, | ||
)?) | ||
.headers(types::PaymentsAuthorizeType::get_headers( |
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.
"Payment for invoice {}", | ||
item.router_data.connector_request_reference_id | ||
), | ||
quantity: 1, |
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.
Any reason this is hardcoded?
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.
Initially we had only 1 order, so we are treating whole order as single line item in order
We will discuss with with team, and refactor this in upcoming PRs.
@@ -955,7 +955,7 @@ fn create_paypal_sdk_session_token( | |||
connector: connector.connector_name.to_string(), | |||
session_token: paypal_sdk_data.data.client_id, | |||
sdk_next_action: payment_types::SdkNextAction { | |||
next_action: payment_types::NextActionCall::Confirm, | |||
next_action: payment_types::NextActionCall::PostSessionTokens, |
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, this will go in sandbox after SDK sandbox deployment.
What's our strategy for production rollout of this change, would it be rolled out after SDK deployment on production as well?
@@ -9,7 +9,8 @@ pub use api_models::payments::{ | |||
PaymentsApproveRequest, PaymentsCancelRequest, PaymentsCaptureRequest, | |||
PaymentsCompleteAuthorizeRequest, PaymentsDynamicTaxCalculationRequest, | |||
PaymentsDynamicTaxCalculationResponse, PaymentsExternalAuthenticationRequest, | |||
PaymentsIncrementalAuthorizationRequest, PaymentsManualUpdateRequest, PaymentsRedirectRequest, | |||
PaymentsIncrementalAuthorizationRequest, PaymentsManualUpdateRequest, |
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.
I believe this was kept this way when we moved out API models to a separate crate, to reduce the number of changes elsewhere in code. But yeah, I think we should avoid such public re-exports.
Of course, this can be taken up in a separate PR.
|
Co-authored-by: hyperswitch-bot[bot] <148525504+hyperswitch-bot[bot]@users.noreply.github.com>
Co-authored-by: hyperswitch-bot[bot] <148525504+hyperswitch-bot[bot]@users.noreply.github.com>
Type of Change
Description
Use Case
/confirm
call, and in response, we received theorder_id
/connector_transaction_id
. We would then send this information to the HS SDK, which forwarded it to PayPal. PayPal used this data to render their SDK, allowing the user to log in and modify the shipping address. The user would then click "Complete Purchase," triggering the OnApprove callback, which was handled by the /complete_authorize call.Additional Changes
Motivation and Context
How did you test it?
Response
In Response Check for Next action - It should be
post_session_tokens
Response
Response
In Response Check for Next action - It should be confirm
Response
For end to end testing, we need to do the payment via SDK
Checklist
cargo +nightly fmt --all
cargo clippy