-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 payments update-intent API for v2 #6490
base: main
Are you sure you want to change the base?
Conversation
e4d55e5
to
fdebe64
Compare
42a7bdf
to
923688d
Compare
923688d
to
5465965
Compare
#[diesel(column_name = enable_payment_link)] | ||
pub payment_link_enabled: Option<bool>, | ||
// TODO: Check this type | ||
// pub payment_link_config: Option<PaymentLinkConfigRequestForPayments>, |
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.
PaymentLinkConfigRequestForPayments
conflicts with AsChangeSet
. I'd appreciate some guidance :)
currency: Option<storage_enums::Currency>, | ||
shipping_cost: Option<MinorUnit>, | ||
// TODO: Check how to handle this | ||
// tax_details: Option<diesel_models::TaxDetails>, |
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 presume tax_details
is not a supported field in update
?
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.
there is a default inside TaxDetails, we can allow that field for the update.
@@ -376,48 +390,194 @@ impl From<PaymentIntentUpdate> for diesel_models::PaymentIntentUpdateInternal { | |||
status: Some(status), | |||
active_attempt_id: Some(active_attempt_id), | |||
modified_at: common_utils::date_time::now(), | |||
amount: 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.
Is there a way to avoid explicitly writing so many None
s
payment_data, | ||
customer.clone(), | ||
merchant_account.storage_scheme, | ||
None, //TODO: fix this |
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 OK or do I need to provide proper values here?
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 not update customer related information in update-intent
@@ -7182,3 +7205,217 @@ impl<F: Clone> OperationSessionSetters<F> for PaymentStatusData<F> { | |||
todo!() | |||
} | |||
} | |||
|
|||
#[cfg(feature = "v2")] | |||
impl<F: Clone> OperationSessionGetters<F> for PaymentUpdateData<F> { |
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 there a way to avoid this unnecessary boilerplate?
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.
After all flows where done, we need to remove unnecessary boilerplate, until that we need to keep it.
.map(|order_details| order_details.into_iter().map(Secret::new).collect()); | ||
|
||
// TODO: This should most likely be created_time + session_expiry rather than now + session_expiry | ||
let session_expiry = request.session_expiry.map(|expiry| { |
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.
Please suggest the best course of action here.
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 logic on created_time, so that it gives a meaning like payments will expire after 15mins or some x mins from created time. If its now, we can't able to have a clear picture like that.
), | ||
errors::StorageError, | ||
> { | ||
Ok((Box::new(self), 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.
Do I need to implement this as well?
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.
No
auth.key_store, | ||
payments::operations::PaymentUpdateIntent, | ||
req.payload, | ||
Some(global_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 there a way to avoid cloning here? I get an error because of the closure
#[schema(value_type = Option<String>)] | ||
pub routing_algorithm_id: Option<id_type::RoutingId>, | ||
|
||
#[schema(value_type = Option<CaptureMethod>, example = "automatic")] |
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.
Should the value_type
be omitted for fields having same type as value_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.
No, we are specifying the value type here because the enum is type qualified. Type qualified usages do not work with ToSchema macro
{ | ||
pub flow: PhantomData<F>, | ||
pub payment_intent: PaymentIntent, | ||
pub payment_intent_update: PaymentIntentUpdate, |
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 object is not required, in PaymentsState. Is this necessary to be placed in state?
@@ -7182,3 +7205,217 @@ impl<F: Clone> OperationSessionSetters<F> for PaymentStatusData<F> { | |||
todo!() | |||
} | |||
} | |||
|
|||
#[cfg(feature = "v2")] | |||
impl<F: Clone> OperationSessionGetters<F> for PaymentUpdateData<F> { |
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.
After all flows where done, we need to remove unnecessary boilerplate, until that we need to keep it.
.map(|order_details| order_details.into_iter().map(Secret::new).collect()); | ||
|
||
// TODO: This should most likely be created_time + session_expiry rather than now + session_expiry | ||
let session_expiry = request.session_expiry.map(|expiry| { |
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 logic on created_time, so that it gives a meaning like payments will expire after 15mins or some x mins from created time. If its now, we can't able to have a clear picture like that.
payment_link_enabled: request.payment_link_enabled.clone(), | ||
request_incremental_authorization: request.request_incremental_authorization, | ||
session_expiry, | ||
// TODO: Does frm_metadata need more processing? |
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 todo, we don't need any processing for frm with current feature set.
currency: Option<storage_enums::Currency>, | ||
shipping_cost: Option<MinorUnit>, | ||
// TODO: Check how to handle this | ||
// tax_details: Option<diesel_models::TaxDetails>, |
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.
there is a default inside TaxDetails, we can allow that field for the update.
payment_data, | ||
customer.clone(), | ||
merchant_account.storage_scheme, | ||
None, //TODO: fix this |
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 not update customer related information in update-intent
), | ||
errors::StorageError, | ||
> { | ||
Ok((Box::new(self), 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.
No
Type of Change
Description
Added
update-intent
API for paymentsAdditional Changes
Motivation and Context
How did you test it?
2a. Update Intent Request
2b. Update Intent Response
Checklist
cargo +nightly fmt --all
cargo clippy