Skip to content
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

refactor(payment_intent_v2): payment intent fields refactoring #5880

Open
wants to merge 74 commits into
base: main
Choose a base branch
from

Conversation

Narayanbhat166
Copy link
Member

@Narayanbhat166 Narayanbhat166 commented Sep 13, 2024

Type of Change

  • Refactoring

Description

This PR addresses some comments that were left on #5783. Few fields have been mademandatory and renamed

  • renamed statement_descriptor_name to statement_descriptor
  • make currecy, client_secret and profile_id as a mandatory field in payment intent
  • group amount related fields to a struct to be used in payment intent domain model
pub struct AmountDetails {
    /// The amount of the order in the lowest denomination of currency
    order_amount: MinorUnit,
    /// The currency of the order
    currency: common_enums::Currency,
    /// The shipping cost of the order. This has to be collected from the merchant
    shipping_cost: Option<MinorUnit>,
    /// Tax details related to the order. This will be calculated by the external tax provider
    tax_details: Option<TaxDetails>,
    /// The action to whether calculate tax by calling external tax provider or not
    skip_external_tax_calculation: TaxCalculationOverride,
    /// The action to whether calculate surcharge or not
    skip_surcharge_calculation: SurchargeCalculationOverride,
    /// The surcharge amount to be added to the order, collected from the merchant
    surcharge_amount: Option<MinorUnit>,
    /// tax on surcharge amount
    tax_on_surcharge: Option<MinorUnit>,
}
  • Change all the boolean values in the domain models to enums indicating relevant actions that has to be taken.
  • Add doc comments to all the fields in payment intent domain model.

This PR also move some other parts of the code under v1 flag, so that when the v2 code is written, these functions will be revisited and then they with either be refactored to be reused / rewritten.

This PR also adds the PaymentGlobalId support for PaymentIntent

Additional Changes

  • This PR modifies the database schema
    This affects the v2 payment intent schema

Motivation and Context

Refactor the fields in v1 that will be removed / not supported in v2 and refactor the code for general improvements in code quality.

How did you test it?

This PR does not add any feature. Only v2 code is affected. A sanity of payments should ensure that this PR does not introduce any bugs.

Impact

payment intent v2

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code

hrithikesh026 and others added 30 commits September 2, 2024 13:37
Comment on lines +109 to +116
impl From<Option<bool>> for TaxCalculationOverride {
fn from(value: Option<bool>) -> Self {
match value {
Some(true) => TaxCalculationOverride::Calculate,
_ => TaxCalculationOverride::Skip,
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is a good idea to map None and Some(false) to TaxCalculationOverride::Skip.
During conversion from domain model to diesel model, we will end up populating surcharge_applicable field with Some(false) even if it was None in DB.

Copy link
Contributor

Choose a reason for hiding this comment

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

In KV store

Copy link
Member Author

@Narayanbhat166 Narayanbhat166 Sep 18, 2024

Choose a reason for hiding this comment

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

yes that is expected. We should always have a value, either true or false in the database. Also we will not be supporting redis kv in v2 on day one

Copy link
Contributor

Choose a reason for hiding this comment

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

Since surcharge_applicable in payment_intent is an inferred field. It should be in payment_attempt.

surcharge_applicable is set to true during payment_method_list call. If any surcharge is calculated through the surcharge rules, we'll set surcharge_applicable to true.
Here you have considered it as flag to skip surcharge calculation.
The decision to skip surcharge calculation is made through surcharge_amount field in payment_intent. If the merchant sends some surcharge_amount through payments create call, we'll will skip surcharge calculation and send whatever surcharge sent by the merchant to the connector.

Copy link
Member Author

Choose a reason for hiding this comment

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

All the fields in PaymentIntent are about the intent of the merchant. In this case we cannot use this field for our internal use. This behaviour will need to change in v2. This field will only tell about whether merchant wants us to calculate the surcharge or not.

crates/hyperswitch_domain_models/src/payments.rs Outdated Show resolved Hide resolved
crates/hyperswitch_domain_models/src/payments.rs Outdated Show resolved Hide resolved
crates/hyperswitch_domain_models/src/payments.rs Outdated Show resolved Hide resolved
crates/hyperswitch_domain_models/src/payments.rs Outdated Show resolved Hide resolved
crates/hyperswitch_domain_models/src/payments.rs Outdated Show resolved Hide resolved
pub authentication_type: Option<common_enums::AuthenticationType>,
pub amount_to_capture: Option<MinorUnit>,
/// This contains the pre routing results that are done when routing is done during listing the payment methods.
pub prerouting_algorithm: Option<serde_json::Value>,
Copy link
Member

Choose a reason for hiding this comment

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

Is this for tracking purpose then can we make this as prerouting_algorithm_id

Copy link
Member Author

Choose a reason for hiding this comment

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

No no, we store a json in this field which is calculated in the payment methods list. We will not have a reference of this calculation anywhere in the routing table, so we cannot use the id here. Also this calculation will be for this payment only.

jarnura
jarnura previously approved these changes Sep 19, 2024
Aprabhat19
Aprabhat19 previously approved these changes Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-v2 M-database-changes Metadata: This PR involves database schema changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants