refactor(connector): [Shift4] refactor connector authorize request struct #1888
Conversation
srujanchikke
added
A-connector-integration
prasunna09
added
R-waiting-on-L2
srujanchikke
added
the
S-waiting-on-review
| description: item.description.clone(), | ||
| })) | ||
| }; | ||
| Ok(Shift4PaymentsRequest { |
There was a problem hiding this comment.
Since we moved the payment method data into enum's, there is no need to return entire Shift4PaymentsRequest in get_card_payment_request & get_bank_redirect_request request. We should return only Shift4PaymentMethod from these method.
In fact we should move from get_card_payment_request & get_bank_redirect_request methods to try_from implementation, since these are type conversion.
| } | ||
|
|
||
| fn get_bank_redirect_request<T>( | ||
| item: &types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>, | ||
| redirect_data: &payments::BankRedirectData, | ||
| ) -> Result<Shift4PaymentsRequest, Error> { | ||
| let submit_for_settlement = item.request.is_auto_capture()?; | ||
| let captured = item.request.is_auto_capture()?; |
There was a problem hiding this comment.
submit_for_settlement gives more meaning to the variable rather than captured. Please keep the existing name itself
| @@ -242,20 +249,20 @@ fn get_flow<T>( | |||
|
|
|||
| fn get_billing<T>( | |||
| item: &types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>, | |||
| ) -> Result<Option<Billing>, Error> { | |||
| ) -> Result<Billing, Error> { | |||
There was a problem hiding this comment.
implement try_from for this conversion instead of get_billing method
| method_type, | ||
| billing, | ||
| }); | ||
| }; | ||
| let flow = get_flow(item); |
There was a problem hiding this comment.
implement from instead of get_flow method
| @@ -157,66 +169,61 @@ fn get_card_payment_request<T>( | |||
| item: &types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>, | |||
| card: &api_models::payments::Card, | |||
| ) -> Result<Shift4PaymentsRequest, Error> { | |||
| let submit_for_settlement = item.request.is_auto_capture()?; | |||
| let captured = item.request.is_auto_capture()?; | |||
There was a problem hiding this comment.
submit_for_settlement gives more meaning to the variable rather than captured. Please keep the existing name itself
ArjunKarthik
added
S-waiting-on-author
| item: ( | ||
| &types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>, | ||
| &api_models::payments::Card, | ||
| ), |
There was a problem hiding this comment.
(item, card): (
&types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>,
&api_models::payments::Card,
),
| items: ( | ||
| &types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>, | ||
| &payments::BankRedirectData, | ||
| ), |
There was a problem hiding this comment.
(item, bank_redirect_data): (
&types::RouterData<T, types::PaymentsAuthorizeData, types::PaymentsResponseData>,
&payments::BankRedirectData,
)
| | payments::PaymentMethodData::Upi(_) | ||
| | payments::PaymentMethodData::Voucher(_) | ||
| | payments::PaymentMethodData::GiftCard(_) => { | ||
| Err(errors::ConnectorError::NotImplemented( |
There was a problem hiding this comment.
Let's have separate try_from for each payment_method_data and inside that throw NotImplemented for unimplemented PM's and throw NotSupported for PM's which is not supported by connector
ArjunKarthik
removed
the
S-waiting-on-author
ArjunKarthik
added
S-ready-for-merge
and removed
S-waiting-on-review
SanchithHegde
removed
S-ready-for-merge
R-L1-completed
Type of Change
Description
problem statement : Currently Authorize request to the connector is a common struct and all payment method uses the same struct, So to support multiple payment methods, mandatory field of payment method is made optional. This will have impact on dynamic fields since in our system we never have validation so it is impossible for us to add in the dynamic fields config
Solution : Creating Payment Method or Flow specific Authorize request using enums, So that fields necessary for payment method can be made mandatory with out disturbing the other payment methods. On the longer run we will have the confident of making change in one payment method will not impact the others
Additional Changes
Motivation and Context
How did you test it?
Checklist
cargo +nightly fmt --allcargo clippy