-
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(payment_methods): Implemented diesel/domain models in PaymentMethods for v2 #5515
base: main
Are you sure you want to change the base?
Conversation
|
||
/// The name of the bank/ provider issuing the payment method to the end user | ||
#[schema(example = "Citibank")] | ||
pub payment_method_issuer: 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 field should be removed.
|
||
/// A standard code representing the issuer of payment method | ||
#[schema(value_type = Option<PaymentMethodIssuerCode>,example = "jp_applepay")] | ||
pub payment_method_issuer_code: Option<api_enums::PaymentMethodIssuerCode>, |
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 field also should be removed
/// The connector mandate details of the payment method, this is added only for cards migration | ||
/// api and is skipped during deserialization of the payment method create request as this | ||
/// it should not be passed in the request | ||
pub connector_mandate_details: Option<PaymentsMandateReference>, |
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 field also not added in the documentation, we need to add this?
/// The transaction id of a CIT (customer initiated transaction) associated with the payment method, | ||
/// this is added only for cards migration api and is skipped during deserialization of the | ||
/// payment method create request as it should not be passed in the request | ||
pub network_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 field also not added in the documentation, we need to add this?
|
||
/// The name of the bank/ provider issuing the payment method to the end user | ||
#[schema(example = "Citibank")] | ||
pub payment_method_issuer: 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.
Remove this field
|
||
/// A standard code representing the issuer of payment method | ||
#[schema(value_type = Option<PaymentMethodIssuerCode>,example = "jp_applepay")] | ||
pub payment_method_issuer_code: Option<api_enums::PaymentMethodIssuerCode>, |
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.
Remove this field
|
||
/// Indicates whether the payment method is eligible for installment payments | ||
#[schema(example = true)] | ||
pub installment_payment_enabled: bool, |
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.
Remove this field
|
||
/// Type of payment experience enabled with the connector | ||
#[schema(value_type = Option<Vec<PaymentExperience>>,example = json!(["redirect_to_url"]))] | ||
pub payment_experience: Option<Vec<api_enums::PaymentExperience>>, |
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.
Remove this field
|
||
/// Masked bank details from PM auth services | ||
#[schema(example = json!({"mask": "0000"}))] | ||
pub bank: Option<MaskedBankDetails>, |
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 moved payment_method_data
pub last_used_at: Option<time::PrimitiveDateTime>, | ||
/// Indicates if the payment method has been set to default or not | ||
#[schema(example = true)] | ||
pub default_payment_method_set: bool, |
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.
rename to is_default
|
||
#[cfg(all(feature = "v2", feature = "payment_methods_v2"))] | ||
impl PaymentMethod { | ||
pub async fn delete_by_payment_method_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.
delete_by_id
.await | ||
} | ||
|
||
pub async fn delete_by_merchant_id_payment_method_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.
this function is not required
.await | ||
} | ||
|
||
pub async fn find_by_locker_id(conn: &PgPooledConn, locker_id: &str) -> StorageResult<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.
In which scenario we need to query the payment_method by locker_id?
.await | ||
} | ||
|
||
pub async fn find_by_payment_method_id(conn: &PgPooledConn, id: &str) -> StorageResult<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.
rename to find_by_id
.await | ||
} | ||
|
||
pub async fn find_by_merchant_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.
In which scenario we need this query?
.await | ||
} | ||
|
||
pub async fn find_by_customer_id_merchant_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.
Add a todo this function should use global customer id and not &common_utils::id_type::CustomerId
.await | ||
} | ||
|
||
pub async fn get_count_by_customer_id_merchant_id_status( |
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.
Add a todo this function should use global customer id and not &common_utils::id_type::CustomerId
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 function used only in analytics, add olap feature flag
.attach_printable("Failed to get a count of payment methods") | ||
} | ||
|
||
pub async fn find_by_customer_id_merchant_id_status( |
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.
Add a todo this function should use global customer id and not &common_utils::id_type::CustomerId and merchant_id is not required.
.await | ||
} | ||
|
||
pub async fn update_with_payment_method_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.
rename to update_by_id
)] | ||
#[diesel(table_name = payment_methods, primary_key(id), check_for_backend(diesel::pg::Pg))] | ||
pub struct PaymentMethod { | ||
pub customer_id: common_utils::id_type::CustomerId, |
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 the global id and not common_utils::id_type::CustomerId
, since the customer PR is not merged make it string for now
pub payment_method: Option<storage_enums::PaymentMethod>, | ||
pub payment_method_type: Option<storage_enums::PaymentMethodType>, | ||
pub metadata: Option<pii::SecretSerdeValue>, | ||
pub payment_method_data: Option<Encryption>, |
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 whole payment_method_data?
pub payment_method_data: Option<Encryption>, | ||
pub locker_id: Option<String>, | ||
pub last_used_at: PrimitiveDateTime, | ||
pub connector_mandate_details: Option<serde_json::Value>, |
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.
Make the type as OptionPii::SecretSerdeValue
#[derive(Debug, Serialize, Deserialize)] | ||
pub enum PaymentMethodUpdate { | ||
MetadataUpdateAndLastUsed { | ||
metadata: Option<serde_json::Value>, |
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.
Change the type to pii::SecretSerdeValue
payment_method_type: Option<storage_enums::PaymentMethodType>, | ||
}, | ||
ConnectorMandateDetailsUpdate { | ||
connector_mandate_details: Option<serde_json::Value>, |
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.
Change the type to pii::SecretSerdeValue
pub merchant_id: common_utils::id_type::MerchantId, | ||
pub created_at: PrimitiveDateTime, | ||
pub last_modified: PrimitiveDateTime, | ||
pub payment_method: Option<storage_enums::PaymentMethod>, |
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 have a separate type for payments domain where payment_method
payment_method_type
payment_method_data
all these should be mandatory
pub payment_method_data: OptionalEncryptableValue, | ||
pub locker_id: Option<String>, | ||
pub last_used_at: PrimitiveDateTime, | ||
pub connector_mandate_details: Option<serde_json::Value>, |
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.
change the type to pii::SecretSerdeValue
@@ -222,12 +222,29 @@ impl From<api::CustomerPaymentMethodsListResponse> for CustomerPaymentMethodList | |||
} | |||
} | |||
|
|||
// Check this in review |
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.
For compatibilility we don't want to support v2 for now
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy