-
Notifications
You must be signed in to change notification settings - Fork 7
feat(connector): [MIFINITY] Connector Integration #89
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughSupport for the Mifinity wallet payment connector was introduced across the backend, including protocol definitions, connector integration, request/response transformation, error handling, configuration, and a dedicated test suite. The changes span new Rust modules, updates to enums and structs, gRPC proto enhancements, and configuration file additions to enable full Mifinity payment flow integration and testing. Changes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
backend/domain_types/src/connector_types.rs (1)
62-100
: Missing match arm for Mifinity in conversion function.The
Mifinity
variant was added toConnectorEnum
but there's no corresponding match arm in theForeignTryFrom<grpc_api_types::payments::Connector>
implementation. This will cause the conversion to fall through to the default case and return an "INVALID_CONNECTOR" error.Add the missing match arm:
grpc_api_types::payments::Connector::Authorizedotnet => Ok(Self::Authorizedotnet), + grpc_api_types::payments::Connector::Mifinity => Ok(Self::Mifinity), grpc_api_types::payments::Connector::Phonepe => Ok(Self::Phonepe),
🧹 Nitpick comments (5)
backend/grpc-api-types/proto/payment_methods.proto (1)
437-443
: Consider adding validation for date_of_birth format.The MifinityWallet message structure is well-designed, but consider specifying the expected date format for the
date_of_birth
field to ensure consistency across implementations.Consider adding a comment or validation rule to specify the expected date format:
// Mifinity - Online payment service by Mifinity message MifinityWallet { - // Date of birth from Mifinity + // Date of birth from Mifinity (format: YYYY-MM-DD) string date_of_birth = 1; // Language preference optional string language_preference = 2; }backend/grpc-server/tests/mifinity_payment_flows_test.rs (1)
46-82
: Comprehensive metadata handling with room for improvement.The metadata setup is thorough and includes all required headers. Consider making merchant ID and tenant ID configurable via environment variables for flexibility across different test environments.
backend/connector-integration/src/utils.rs (2)
56-58
: Improve error message clarityThe error message is grammatically incomplete. Consider making it more descriptive.
pub(crate) fn get_unimplemented_payment_method_error_message(connector: &str) -> String { - format!("Selected payment method through {connector}") + format!("The selected payment method is not supported by the {connector} connector") }
86-111
: Utilize the connector parameter for better error contextThe
_connector
parameter is unused but could provide valuable context in error messages.pub(crate) fn handle_json_response_deserialization_failure( res: Response, - _connector: &'static str, + connector: &'static str, ) -> CustomResult<ErrorResponse, errors::ConnectorError> { let response_data = String::from_utf8(res.response.to_vec()) .change_context(errors::ConnectorError::ResponseDeserializationFailed)?; // check for whether the response is in json format match serde_json::from_str::<Value>(&response_data) { // in case of unexpected response but in json format Ok(_) => Err(errors::ConnectorError::ResponseDeserializationFailed)?, // in case of unexpected response but in html or string format Err(_error_msg) => Ok(ErrorResponse { status_code: res.status_code, - code: "No error code".to_string(), - message: "Unsupported response type".to_string(), + code: format!("{}_UNSUPPORTED_RESPONSE", connector.to_uppercase()), + message: format!("Unsupported response type from {} connector", connector), reason: Some(response_data), attempt_status: None, connector_transaction_id: None, network_advice_code: None, network_decline_code: None, network_error_message: None, raw_connector_response: None, }), } }backend/connector-integration/src/connectors/mifinity/transformers.rs (1)
191-223
: Simplify unimplemented wallet type handlingThe match arms for unimplemented wallet types can be simplified using a wildcard pattern.
- WalletData::AliPayQr(_) - | WalletData::AliPayRedirect(_) - | WalletData::AliPayHkRedirect(_) - | WalletData::AmazonPayRedirect(_) - | WalletData::MomoRedirect(_) - | WalletData::KakaoPayRedirect(_) - | WalletData::GoPayRedirect(_) - | WalletData::GcashRedirect(_) - | WalletData::ApplePay(_) - | WalletData::ApplePayRedirect(_) - | WalletData::ApplePayThirdPartySdk(_) - | WalletData::DanaRedirect {} - | WalletData::GooglePay(_) - | WalletData::GooglePayRedirect(_) - | WalletData::GooglePayThirdPartySdk(_) - | WalletData::MbWayRedirect(_) - | WalletData::MobilePayRedirect(_) - | WalletData::PaypalRedirect(_) - | WalletData::PaypalSdk(_) - | WalletData::Paze(_) - | WalletData::SamsungPay(_) - | WalletData::TwintRedirect {} - | WalletData::VippsRedirect {} - | WalletData::TouchNGoRedirect(_) - | WalletData::WeChatPayRedirect(_) - | WalletData::WeChatPayQr(_) - | WalletData::CashappQr(_) - | WalletData::SwishQr(_) - | WalletData::RevolutPay(_) => Err(ConnectorError::NotImplemented( + _ => Err(ConnectorError::NotImplemented( utils::get_unimplemented_payment_method_error_message("Mifinity"), ) .into()),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/connector-integration/src/connectors.rs
(2 hunks)backend/connector-integration/src/connectors/mifinity.rs
(1 hunks)backend/connector-integration/src/connectors/mifinity/transformers.rs
(1 hunks)backend/connector-integration/src/types.rs
(2 hunks)backend/connector-integration/src/utils.rs
(2 hunks)backend/domain_types/src/connector_types.rs
(1 hunks)backend/domain_types/src/types.rs
(5 hunks)backend/grpc-api-types/proto/payment_methods.proto
(5 hunks)backend/grpc-server/tests/mifinity_payment_flows_test.rs
(1 hunks)config/development.toml
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/connector-integration/src/connectors/mifinity.rs (4)
backend/connector-integration/src/connectors/macros.rs (2)
router_data
(94-105)request_body
(66-75)backend/connector-integration/src/connectors/mifinity/transformers.rs (6)
try_from
(37-43)try_from
(97-248)try_from
(258-265)try_from
(284-332)try_from
(370-444)utils
(38-38)backend/common_utils/src/pii.rs (1)
expose
(40-42)backend/connector-integration/src/utils.rs (2)
serde_json
(94-94)handle_json_response_deserialization_failure
(86-111)
🔇 Additional comments (20)
config/development.toml (1)
40-40
: LGTM! Configuration follows established pattern.The Mifinity connector base URL configuration follows the same pattern as other connectors and appropriately uses a demo URL for the development environment.
backend/domain_types/src/connector_types.rs (1)
53-53
: LGTM! Enum variant addition follows conventions.The Mifinity variant is correctly added to the ConnectorEnum following the established naming pattern.
backend/connector-integration/src/types.rs (2)
5-6
: LGTM! Import follows established pattern.The Mifinity connector is correctly imported alongside other connectors.
34-34
: LGTM! Match arm follows established pattern.The match arm for Mifinity follows the same pattern as other connectors, correctly instantiating a boxed instance of the connector.
backend/connector-integration/src/connectors.rs (2)
25-25
: LGTM! Module declaration follows established pattern.The mifinity module is correctly declared as public alongside other connector modules.
10-11
: LGTM! Re-export follows established pattern.The Mifinity struct is correctly included in the public re-exports alongside other connectors.
backend/grpc-api-types/proto/payment_methods.proto (3)
37-37
: LGTM! Wallet payment support properly enabled.The enabling of the wallet field aligns with the Mifinity wallet integration and the comment update correctly reflects the supported status.
75-88
: Good protobuf design for wallet type enumeration.The selective enabling of only the MifinityWallet type while keeping other wallet types commented out is a good approach. Field number 11 follows the sequential numbering pattern correctly.
298-298
: Comment update correctly reflects wallet support status.The comment change from "Not yet supported" to "SUPPORTED" is consistent with the enablement of wallet payments.
backend/grpc-server/tests/mifinity_payment_flows_test.rs (5)
1-4
: Appropriate clippy allows for test code.The clippy allows for
expect_used
,unwrap_used
, andpanic
are appropriate in test code where failing fast on errors is desired behavior.
25-44
: Well-organized test constants and helper functions.The centralized constants and timestamp helper function provide good test organization. The hard-coded test values are appropriate for integration testing.
84-167
: Excellent helper functions for payment flow testing.The helper functions are well-designed with proper error handling and comprehensive request building. The separation of concerns between authorization and sync request creation is good practice.
169-186
: Good foundational health check test.The health check test provides essential service availability verification before running payment flows. The test structure is clean and focused.
188-258
: Comprehensive payment flow testing with proper assertions.The payment tests cover essential flows (authorization and sync) with realistic scenarios. The status assertions appropriately handle the expected payment states for Mifinity integration.
backend/domain_types/src/types.rs (5)
57-57
: LGTM!The
mifinity
field follows the established pattern for connector parameters in theConnectors
struct.
228-253
: Well-implemented Mifinity wallet conversion!The implementation correctly:
- Parses
date_of_birth
using ISO8601 format with proper error handling- Protects PII by wrapping the date in
Secret
- Provides clear error messages for invalid formats
- Handles missing wallet type appropriately
315-328
: LGTM!The PaymentMethodType conversion for Mifinity is correctly implemented with consistent error handling.
1231-1239
: Verify the redirect form mapping for Mifinity.The implementation maps Mifinity's
initialization_token
to a URI redirect form. Please confirm this is the intended behavior for Mifinity integration, as the token might require a different form type depending on how the frontend processes it.
1326-1329
: LGTM!The PaymentMethod enum conversion correctly categorizes Mifinity as a Wallet payment method.
backend/connector-integration/src/connectors/mifinity/transformers.rs (1)
287-288
: Verify handling of multiple payloadsThe code only processes the first payload and silently ignores any additional payloads. This could lead to data loss if the API returns multiple payloads.
Consider logging a warning if multiple payloads are received:
let payload = item.response.payload.first(); +if item.response.payload.len() > 1 { + tracing::warn!("Mifinity returned {} payloads, only processing the first one", item.response.payload.len()); +} match payload {
backend/connector-integration/src/connectors/mifinity/transformers.rs
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/connector-integration/src/utils.rs (1)
90-115
: Good error handling logic with unused parameter.The function effectively differentiates between JSON deserialization failures and non-JSON responses. However, the
_connector
parameter is unused.Consider either using the connector parameter in error messages or removing it if not needed:
-pub(crate) fn handle_json_response_deserialization_failure( - res: Response, - _connector: &'static str, -) -> CustomResult<ErrorResponse, errors::ConnectorError> { +pub(crate) fn handle_json_response_deserialization_failure( + res: Response, + connector: &'static str, +) -> CustomResult<ErrorResponse, errors::ConnectorError> { let response_data = String::from_utf8(res.response.to_vec()) .change_context(errors::ConnectorError::ResponseDeserializationFailed)?; // check for whether the response is in json format match serde_json::from_str::<Value>(&response_data) { // in case of unexpected response but in json format Ok(_) => Err(errors::ConnectorError::ResponseDeserializationFailed)?, // in case of unexpected response but in html or string format Err(_error_msg) => Ok(ErrorResponse { status_code: res.status_code, code: "No error code".to_string(), - message: "Unsupported response type".to_string(), + message: format!("Unsupported response type from {}", connector), reason: Some(response_data), attempt_status: None, connector_transaction_id: None, network_advice_code: None, network_decline_code: None, network_error_message: None, raw_connector_response: None, }), } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/connector-integration/src/connectors.rs
(2 hunks)backend/connector-integration/src/connectors/mifinity.rs
(1 hunks)backend/connector-integration/src/connectors/mifinity/transformers.rs
(1 hunks)backend/connector-integration/src/types.rs
(2 hunks)backend/connector-integration/src/utils.rs
(2 hunks)backend/domain_types/src/connector_types.rs
(1 hunks)backend/domain_types/src/types.rs
(5 hunks)backend/grpc-api-types/proto/payment_methods.proto
(5 hunks)config/development.toml
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- config/development.toml
- backend/domain_types/src/connector_types.rs
- backend/connector-integration/src/types.rs
- backend/domain_types/src/types.rs
- backend/grpc-api-types/proto/payment_methods.proto
- backend/connector-integration/src/connectors.rs
- backend/connector-integration/src/connectors/mifinity/transformers.rs
🔇 Additional comments (8)
backend/connector-integration/src/utils.rs (2)
1-11
: LGTM on module organization and imports.The module declaration, imports, and re-exports are well-organized and appropriate for the utility functions provided.
63-88
: Well-implemented function that addresses previous feedback.The function correctly handles both JSON string and object formats as suggested in the previous review. Error handling is comprehensive with appropriate context messages.
backend/connector-integration/src/connectors/mifinity.rs (6)
1-43
: Well-structured imports and module organization.The imports are comprehensive and properly organized. The module structure with the transformers submodule is appropriate for this connector implementation.
48-111
: Comprehensive trait implementations for connector integration.All required trait implementations are present with appropriate generic type constraints. The empty implementations correctly delegate to the framework's default behavior.
113-222
: Effective use of macros for boilerplate reduction.The macro usage effectively reduces code duplication while maintaining clarity. The API version constant is properly defined and used consistently. The custom member functions for header building and URL construction are well-implemented.
224-289
: Appropriate empty implementations for unsupported flows.The empty ConnectorIntegrationV2 implementations correctly indicate that these flows are not supported by the Mifinity connector while maintaining framework compatibility.
291-425
: Complete source verification integration.All flows properly implement source verification interfaces, ensuring security and compliance requirements are met across the connector's operations.
427-520
: Well-implemented ConnectorCommon trait with improved error handling.The implementation correctly provides all required connector functionality. The error response building logic effectively handles both empty responses and JSON error responses. The code duplication in error message concatenation has been addressed by extracting the concatenation logic into variables.
pub(crate) fn get_unimplemented_payment_method_error_message(connector: &str) -> String { | ||
format!("Selected payment method through {connector}") | ||
} |
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.
🛠️ Refactor suggestion
Improve error message clarity.
The error message "Selected payment method through {connector}" is incomplete and doesn't clearly indicate that the payment method is unimplemented.
Consider making the message more descriptive:
-pub(crate) fn get_unimplemented_payment_method_error_message(connector: &str) -> String {
- format!("Selected payment method through {connector}")
-}
+pub(crate) fn get_unimplemented_payment_method_error_message(connector: &str) -> String {
+ format!("Selected payment method is not implemented for {connector}")
+}
🤖 Prompt for AI Agents
In backend/connector-integration/src/utils.rs around lines 59 to 61, the error
message returned by get_unimplemented_payment_method_error_message is unclear
and incomplete. Update the format string to explicitly state that the selected
payment method through the given connector is unimplemented, making the message
more descriptive and informative for users or developers encountering this
error.
Description
Added Mifinity as connector with following flows:
End to End testing is done through mifinity_payment_flows_test.rs
Motivation and Context
Additional Changes
How did you test it?
use cargo test to test mifinity_payment_flows_test.rs
Enable payments in HS via UCS
Create Payment
Request
Response
Sync Payment
Response