Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor(core): Use hyperswitch_domain_models within the Payments Core instead of api_models #5511

Merged
merged 27 commits into from
Aug 9, 2024

Conversation

prasunna09
Copy link
Contributor

@prasunna09 prasunna09 commented Aug 1, 2024

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

  • Use hyperswitch_domain_models within the Payments Core instead of api_models
  • introduced a new payment method data Network Token in domain models

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

TestCases:-
Sanity testing for all the payments flows including save card flow and pm_auth flow.

stripe testing -
Screenshot 2024-08-06 at 5 00 14 PM
cybersource -
Screenshot 2024-08-06 at 5 07 50 PM
saved card payment -
Screenshot 2024-08-06 at 4 36 05 PM

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible

@prasunna09 prasunna09 added A-core Area: Core flows C-refactor Category: Refactor labels Aug 1, 2024
@prasunna09 prasunna09 self-assigned this Aug 1, 2024
@prasunna09 prasunna09 requested review from a team as code owners August 1, 2024 21:13
lsampras
lsampras previously approved these changes Aug 6, 2024
let mut updated_card = card.clone();
let mut is_card_updated = false;

// The card_holder_name from locker retrieved card is considered if it is a non-empty string or else card_holder_name is picked
// from payment_method_data.card_token object
let name_on_card = if let Some(name) = card.card_holder_name.clone() {
let name_on_card = if let Some(name) = card.nick_name.clone() {
//todo!
Copy link
Member

Choose a reason for hiding this comment

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

this needed?

@@ -1770,7 +1772,7 @@ pub async fn retrieve_payment_method_with_temporary_token(
})
.or(Some(name))
} else {
card.card_holder_name.clone()
card.nick_name.clone() //todo!
Copy link
Member

Choose a reason for hiding this comment

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

can we add details about what has to be done

SamraatBansal
SamraatBansal previously approved these changes Aug 6, 2024
Copy link
Contributor

@SamraatBansal SamraatBansal left a comment

Choose a reason for hiding this comment

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

LGTM for connector changes

@@ -2028,7 +2030,7 @@ pub async fn make_pm_data<'a, F: Clone, R>(
}

// TODO: Handle case where payment method and token both are present in request properly.
let (payment_method, pm_id) = match (request, payment_data.token_data.as_ref()) {
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 this clone is required? u can match on reference of request itself

@@ -2059,7 +2061,7 @@ pub async fn make_pm_data<'a, F: Clone, R>(

(Some(_), _) => {
let (payment_method_data, payment_token) = payment_methods::retrieve_payment_method(
request,
&request.map(Into::into),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just pass request if u make this a reference above. No type conversion required as request type is already Option<domain::PaymentMethodData>

@@ -314,7 +314,7 @@ impl<F: Send + Clone> GetTracker<F, PaymentData<F>, api::PaymentsRequest> for Co
payment_method_data: request
.payment_method_data
.as_ref()
.and_then(|pmd| pmd.payment_method_data.clone()),
.and_then(|pmd| pmd.payment_method_data.clone().map(Into::into).clone()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.and_then(|pmd| pmd.payment_method_data.clone().map(Into::into).clone()),
.and_then(|pmd| pmd.payment_method_data.clone().map(Into::into)),

Clone not required

Comment on lines 1081 to 1083

// payment_data.payment_attempt.clone();

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Comment on lines 1136 to 1140
let business_sub_label = payment_data
.payment_attempt
.clone()
.business_sub_label
.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let business_sub_label = payment_data
.payment_attempt
.clone()
.business_sub_label
.clone();
let business_sub_label = payment_data
.payment_attempt
.business_sub_label
.clone();

same here

.clone()
.business_sub_label
.clone();
let authentication_type = payment_data.payment_attempt.clone().authentication_type;
Copy link
Contributor

@Chethan-rao Chethan-rao Aug 6, 2024

Choose a reason for hiding this comment

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

Suggested change
let authentication_type = payment_data.payment_attempt.clone().authentication_type;
let authentication_type = payment_data.payment_attempt.authentication_type.clone();

same here

Comment on lines 1147 to 1150
.payment_attempt
.clone()
.payment_method_billing_address_id
.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.payment_attempt
.clone()
.payment_method_billing_address_id
.clone(),
.payment_attempt
.payment_method_billing_address_id
.clone(),

same here

Comment on lines 863 to 865
.clone()
.and_then(|payment_method_data_request| payment_method_data_request.payment_method_data)
.async_and_then(|payment_method_data| async {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to clone entire payment_method_data of request. Instead take request.payment_method_data as reference and clone payment_method_data_request.payment_method_data inside and_then

Comment on lines 13 to 16
use crate::types::{
api::{self},
domain::{self},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use crate::types::{
api::{self},
domain::{self},
};
use crate::types::{
api,domain
};

@prasunna09 prasunna09 dismissed stale reviews from SamraatBansal and lsampras via 3d260c0 August 7, 2024 07:13
Chethan-rao
Chethan-rao previously approved these changes Aug 7, 2024
Narayanbhat166
Narayanbhat166 previously approved these changes Aug 7, 2024
@prasunna09 prasunna09 dismissed stale reviews from Narayanbhat166 and Chethan-rao via dca748d August 8, 2024 09:52
Narayanbhat166
Narayanbhat166 previously approved these changes Aug 8, 2024
Chethan-rao
Chethan-rao previously approved these changes Aug 8, 2024
…juspay/hyperswitch into add-network-token-data-in-domain-models
@prasunna09 prasunna09 dismissed stale reviews from Chethan-rao and Narayanbhat166 via 1f91849 August 9, 2024 07:22
@Gnanasundari24 Gnanasundari24 added this pull request to the merge queue Aug 9, 2024
Merged via the queue into main with commit f81416e Aug 9, 2024
14 checks passed
@Gnanasundari24 Gnanasundari24 deleted the add-network-token-data-in-domain-models branch August 9, 2024 09:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-core Area: Core flows C-refactor Category: Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants