-
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
refactor(payment_methods_v2): rename payment_method
and payment_method_type
fields and use concrete type for payment_method_data
#6555
base: main
Are you sure you want to change the base?
Conversation
…nceRecord` for v2 and rename `payment_method_type` field to `payment_method_subtype`
…ethod_type_v2` and `payment_method_type` to `payment_method_subtype` respectively
… `payment_method_type` and `payment_method_type` to `payment_method_subtype` respectively
…d_type` and `payment_method_type` to `payment_method_subtype` respectively
…dle JSON serialization and encryption
…ld in `PaymentMethod` to use `EncryptedJsonType`
payment_method
and payment_method_type
fields and use conrete type for payment_method_data
payment_method
and payment_method_type
fields and use concrete type for payment_method_data
|
||
#[async_trait] | ||
impl< | ||
T: std::fmt::Debug + Clone + serde::Serialize + serde::de::DeserializeOwned + Send, |
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.
What are case T required Debug? is that necessary.
…pe for v2 and rename `payment_method_type` to `payment_method_subtype`
fn get_api_event_type(&self) -> Option<ApiEventsType> { | ||
Some(ApiEventsType::PaymentMethod { | ||
payment_method_id: self.payment_method_id.clone(), | ||
payment_method: self.payment_method_type, |
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 not changing the field name her?
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.
Since renaming the field affects the analytics pipeline, we'd have to include ClickHouse migrations as well.
For now, I'm not including v2 changes in the analytics pipeline.
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 renaming payment_method to payment_method_type ?
Type of Change
Description
This PR involves the following changes in payment methods v2 code:
payment_method
field topayment_method_type
, andpayment_method_type
topayment_method_subtype
. This change affect the database columns itself, the diesel models, the domain models and the API models.payment_method_data
field in the domain model to have the concrete type wrapped byEncryptedJsonType
, instead of the existingserde_json::Value
type.EncryptedJsonType
was also introduced in this PR. It adds support to use a concrete type in domain models, while handling the JSON serialization and deserialization before encryption and after decryption of data, respectively.In addition, this PR removes the
PaymentMethodList
struct from the API models, which isn't being used anywhere.By the way, I'm open to better name suggestions for the newly introduced things!
Additional Changes
Motivation and Context
How did you test it?
This PR mostly affects the payment methods v2 code, with focus on the customer payment methods list v2 code. I haven't been able to test these changes, there are still a couple of panics remaining in the customer payment methods list v2 code.
Ideally, nothing in the payment methods v1 code should be affected and existing Postman and Cypress tests should pass.
Checklist
cargo +nightly fmt --all
cargo clippy