-
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 Support for Payments Dynamic Tax Calculation Based on Shipping Address #5619
Conversation
…nto tax-jar-api
…nto tax-jar-api
attempt_count: None, | ||
merchant_decision: None, | ||
payment_confirm_source: None, | ||
updated_by: String::default(), |
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 as default, take it in params and update
customer_details: None, | ||
billing_details: None, | ||
merchant_order_reference_id: None, | ||
shipping_details: 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.
update this field too with the shipping address
@@ -257,6 +258,10 @@ pub enum PaymentIntentUpdate { | |||
status: Option<storage_enums::IntentStatus>, | |||
updated_by: String, | |||
}, | |||
SessionResponseUpdate { | |||
tax_details: diesel_models::TaxDetails, | |||
shipping_address_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.
take update_by
here
#[derive(Debug, Clone)] | ||
pub struct TaxCalculationResponseData { | ||
pub order_tax_amount: MinorUnit, | ||
pub net_amount: MinorUnit, |
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 using this anywhere, please remove this field
_business_profile: &domain::BusinessProfile, | ||
_header_payload: api_models::payments::HeaderPayload, | ||
) -> RouterResult<Self> { | ||
if connector.connector_name == types::Connector::Klarna { |
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 do not need this check here, this will be handled in routing
@@ -488,6 +488,104 @@ impl<F: Clone> PostUpdateTracker<F, PaymentData<F>, types::PaymentsSessionData> | |||
} | |||
} | |||
|
|||
#[async_trait] | |||
impl<F: Clone> PostUpdateTracker<F, PaymentData<F>, types::PaymentsTaxCalculationData> |
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 remove this if this is not used anywhere
.ok_or(errors::ApiErrorResponse::MissingRequiredField { | ||
field_name: "order_tax_amount", | ||
})?; | ||
let amount = MinorUnit::from(payment_data.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.
take this from payment_attempt
amount = amount + order_tax_amount; | ||
} | ||
Ok(services::ApplicationResponse::JsonWithHeaders(( | ||
Self { net_amount: 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.
do we only need net amount in the response? What about the breakup of amount?
cc: @SamraatBansal
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 ideally send the break up, the response api contract needs to be revisited along with SDK team
@@ -1317,3 +1408,124 @@ macro_rules! unimplemented_payment_method { | |||
)) | |||
}; | |||
} | |||
|
|||
impl ForeignTryFrom<String> for UsStatesAbbreviation { |
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.
Would prefer to avoid using ForeignTryFrom
outside the router
crate.
Can we instead implement this method directly on the UsStatesAbbreviation
enum?
This can be taken up in a separate PR.
@@ -139,7 +140,8 @@ impl< | |||
+ ConnectorMandateRevoke | |||
+ ConnectorMandateRevokeV2 | |||
+ ExternalAuthentication | |||
+ ExternalAuthenticationV2, | |||
+ ExternalAuthenticationV2 | |||
+ PaymentTaxCalculation, |
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 don't think we need payment here in the trait name
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.
Need extensive testing, in flows like Payments - Create and Confirm
amount = amount + order_tax_amount; | ||
} | ||
Ok(services::ApplicationResponse::JsonWithHeaders(( | ||
Self { net_amount: 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.
We should ideally send the break up, the response api contract needs to be revisited along with SDK team
#[diesel(sql_type = diesel::sql_types::Jsonb)] | ||
pub struct TaxDetails { | ||
pub default: Option<DefaultTax>, | ||
pub payment_method_type: Option<PaymentMethodTypeTax>, |
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 tamper proof if somebody changes the address in the confirm call?
@@ -1453,6 +1453,7 @@ impl<'a> ConnectorAuthTypeAndMetadataValidation<'a> { | |||
stax::transformers::StaxAuthType::try_from(self.auth_type)?; | |||
Ok(()) | |||
} | |||
api_enums::Connector::Taxjar => Ok(()), |
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.
Auth type validation is missing
_merchant_recipient_data: Option<types::MerchantRecipientData>, | ||
) -> RouterResult<types::SdkSessionUpdateRouterData> { | ||
Box::pin( | ||
transformers::construct_router_data_to_update_calculated_tax::< |
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 file should be renamed as session update flow
let db = state.store.as_ref(); | ||
|
||
let key_manager_state: &KeyManagerState = &state.into(); | ||
|
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.
TODO: add a check if payments_create had a skip_tax_calculation as true or not
) -> CustomResult<(), errors::ApiErrorResponse> { | ||
let db = state.store.as_ref(); | ||
let key_manager_state: &KeyManagerState = &state.into(); | ||
|
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.
TODO: Check for skip_tax flag from intent DB, dont check it from update request
key_store: &domain::MerchantKeyStore, | ||
merchant_account: &domain::MerchantAccount, | ||
) -> errors::CustomResult<(), errors::ApiErrorResponse> { | ||
if business_profile.is_tax_connector_enabled { |
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.
Check for condition as mentioned above
_connectors: &Connectors, | ||
) -> CustomResult<RequestContent, errors::ConnectorError> { | ||
let amount = utils::convert_amount( | ||
self.amount_converter, | ||
req.request.minor_amount, | ||
req.request.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.
req.request.amount, | |
req.request.minor_amount, |
.ok_or(errors::ConnectorError::MissingRequiredField { | ||
field_name: "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.
How are missing_required_params propagated in the api response?
We should give 2xx or SDK should handle a 4xx Scenario? @jarnura
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.
In my opinion, The api is supposed to calculate the tax with the given inputs. If it is not able to calculate it for some reason, it should not send a 2xx response. Missing required fields can occur because of 2 cases
-
Integration errors
We expected some field to be sent from the merchant in the payments create request or while creating the business profile, these should be caught in the integration phase. -
Insufficient data from the customer / wallet
Some fields that are required by the underlying connector to calculate the tax are not received by the api. You check this case by testing the flow with different wallet providers. We can throw 4xx if it can be handled by the sdk to collect these fields.
Yes @SamraatBansal @swangi-kumari we should also send the tax breakup in payments create call. This will be useful for merchant if he wants to display the tax breakup on the checkout page. The payments response should have this response populated |
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.
Dashboard specific changes look fine
Some(ApiEventsType::Payment { | ||
payment_id: self.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.
Is this part of the payment lifecycle ?,
if not this can be 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.
Yes this is part of payment life cycle.
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Create a Payment Connector Create of Stripe for ApplePay
4.Payment Connector Create for TaxJar
Response
Checklist
cargo +nightly fmt --all
cargo clippy