feat: fawry connector scaffold #1643
Conversation
Signed-off-by: Clément Salaün <[email protected]>
WalkthroughThe updates introduce a comprehensive payment processing connector for the Fawry payment system, featuring structures for client requests, invoicing, notifications, and configuration management. New methods facilitate secure request signing, enhance task management, and streamline asynchronous notification handling, improving the overall modularity and error handling in payment workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Connector
participant Loader
participant Webhook
Client->>Connector: Initiate Payment Request
Connector->>Loader: Load Configuration
Loader->>Webhook: Route Notification
Webhook->>Connector: Process Notification
Connector->>Client: Send Response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (6)
components/payments/cmd/connectors/internal/connectors/fawry/client/client.go (1)
20-22: Consider adding documentation for theInitmethod.The
Initmethod currently returns nil, and it might be beneficial to add documentation or comments indicating its intended use or future expansion.+ // Init initializes the client. Currently, it is a placeholder for future setup logic.components/payments/cmd/connectors/internal/connectors/fawry/config.go (1)
12-14: Consider Adding Custom Error Handling for Marshaling.The
Marshalmethod uses the defaultjson.Marshal, which might not provide detailed error information. Consider adding custom error handling if detailed logging or error tracing is required.components/payments/cmd/connectors/internal/connectors/fawry/webhook.go (2)
15-16: Consider adding fields to theWebhookstruct.The
Webhookstruct is currently empty. If there are any configurations or dependencies needed for the webhook, consider adding them as fields.
30-30: Ensure Context Cancellation infawry/webhook.goThe context created with
contextutil.DetachedWithTimeoutinfawry/webhook.godoes not capture the cancel function, which may result in resource leaks. Please ensure that the cancel function is captured and called appropriately to manage the context lifecycle.
components/payments/cmd/connectors/internal/connectors/fawry/webhook.go: Line 30Analysis chain
Check for context cancellation.
The context created with
contextutil.DetachedWithTimeoutignores the cancel function. Ensure that the context is properly managed and canceled if necessary to avoid resource leaks.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the context cancellation is handled in the codebase. # Test: Search for context cancellation handling. Expect: Proper cancellation logic. rg --type go 'contextutil.DetachedWithTimeout'Length of output: 2967
Script:
#!/bin/bash # Check if the cancel function is called in files where it is captured # Search for the usage of the cancel function in the atlar/task_payments.go file rg --type go 'cancel\(\)' components/payments/cmd/connectors/internal/connectors/atlar/task_payments.go # Search for the usage of the cancel function in the atlar/task_fetch_transactions.go file rg --type go 'cancel\(\)' components/payments/cmd/connectors/internal/connectors/atlar/task_fetch_transactions.go # Search for the usage of the cancel function in the atlar/task_fetch_accounts.go file rg --type go 'cancel\(\)' components/payments/cmd/connectors/internal/connectors/atlar/task_fetch_accounts.goLength of output: 522
components/payments/cmd/connectors/internal/connectors/fawry/resolve.go (1)
18-21: Consider adding a mutex forfetchTransactionsOncemap access.The
fetchTransactionsOncemap is accessed and modified without explicit synchronization mechanisms. To ensure thread safety, especially in concurrent environments, consider using a mutex to protect access to this map.
- File:
components/payments/cmd/connectors/internal/connectors/mangopay/task_fetch_wallets.go- Concern: Access to
fetchTransactionsOnceshould be synchronized to avoid race conditions.Analysis chain
Consider thread safety for
TaskMemoryState.The
fetchTransactionsOncemap is used for synchronization. Ensure that access to this map is thread-safe, especially if it is modified concurrently.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify thread safety for `fetchTransactionsOnce`. # Test: Search for concurrent access to `fetchTransactionsOnce`. Expect: Proper synchronization mechanisms. rg --type go 'fetchTransactionsOnce'Length of output: 994
Script:
#!/bin/bash # Check for synchronization mechanisms around access to `fetchTransactionsOnce`. rg --type go 'fetchTransactionsOnce' -A 5Length of output: 3276
components/payments/cmd/connectors/internal/connectors/fawry/connector.go (1)
16-32: Ensure idempotency in theInstallmethod.The
Installmethod does not handle theErrAlreadySchedulederror, which may lead to duplicate task scheduling. Consider adding error handling forErrAlreadyScheduledto ensure idempotency and avoid unintended side effects.
- File:
components/payments/cmd/connectors/internal/connectors/fawry/connector.go- Lines: 16-32
Analysis chain
Ensure proper task scheduling in
Installmethod.The
Installmethod schedules a task immediately. Verify that this behavior is intended and that the task is idempotent to avoid unintended side effects.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify task scheduling behavior in `Install` method. # Test: Search for task scheduling logic. Expect: Idempotent task behavior. rg --type go 'Schedule'Length of output: 72554
Script:
#!/bin/bash # Description: Check for handling of ErrAlreadyScheduled in the `connector.go` file and related logic. # Test: Search for ErrAlreadyScheduled handling in the relevant files. rg --type go 'ErrAlreadyScheduled'Length of output: 8881
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- components/payments/cmd/connectors/internal/connectors/fawry/client/client.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/client/invoice.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/client/notification.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/config.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/connector.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/currencies.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/ingest_payment.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/loader.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/resolve.go (1 hunks)
- components/payments/cmd/connectors/internal/connectors/fawry/webhook.go (1 hunks)
Files skipped from review due to trivial changes (1)
- components/payments/cmd/connectors/internal/connectors/fawry/ingest_payment.go
Additional comments not posted (9)
components/payments/cmd/connectors/internal/connectors/fawry/currencies.go (1)
1-8: Currency mapping looks good.The use of the
currency.ISO4217Currenciesmap ensures consistency and accuracy for currency information.components/payments/cmd/connectors/internal/connectors/fawry/client/invoice.go (1)
3-7: Consider using a consistent date type forExpiryDate.The
ExpiryDatefield is currently defined asuint64, which might not be consistent with the string type used forDueDate. Consider using a consistent date format or type for both fields to avoid confusion and ensure proper handling of date values.components/payments/cmd/connectors/internal/connectors/fawry/client/client.go (2)
9-18: Request signing implementation is effective.The
SignRequestmethod securely hashes inputs using SHA-256, which is a good practice for ensuring data integrity.
24-26: Client initialization looks good.The
NewClientfunction provides a simple way to create a newClientinstance with default values.components/payments/cmd/connectors/internal/connectors/fawry/config.go (2)
5-10: Ensure Secure Handling of Sensitive Information.The
SecureKeyfield contains sensitive information. Ensure that this key is securely managed and not exposed in logs or error messages.
16-18: MethodConnectorNameis Simple and Effective.The method correctly returns the connector's name, which enhances the readability and usability of the
Configstruct.components/payments/cmd/connectors/internal/connectors/fawry/loader.go (2)
26-35: Review Webhook Security inRouterMethod.Ensure that the webhook handler in the
Routermethod includes proper authentication and validation to prevent unauthorized access and ensure data integrity.
41-43: Clarify the Purpose ofnewConnectorinLoadMethod.The
Loadmethod returns a new connector usingnewConnector(). Ensure that this function initializes the connector with the necessary configurations and resources.components/payments/cmd/connectors/internal/connectors/fawry/connector.go (1)
67-69: Verify currency support logic.The
SupportedCurrenciesAndDecimalsmethod returns a map of supported currencies. Ensure this map is correctly initialized and updated as needed.Verification successful
Currency support logic is correctly implemented for the Fawry connector.
The
supportedCurrenciesWithDecimalmap is correctly initialized with the "EGP" currency, using the ISO 4217 standard for its decimal precision. There are no updates to this map elsewhere in thefawryconnector.
- Initialization found in
components/payments/cmd/connectors/internal/connectors/fawry/currencies.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify currency support logic. # Test: Search for initialization and updates to `supportedCurrenciesWithDecimal`. Expect: Correct initialization and updates. rg --type go 'supportedCurrenciesWithDecimal'Length of output: 16657
Script:
#!/bin/bash # Extract the initialization details of `supportedCurrenciesWithDecimal` in the fawry connector. rg --type go 'supportedCurrenciesWithDecimal = map' -A 10 components/payments/cmd/connectors/internal/connectors/fawry/currencies.goLength of output: 233
| type Notification struct { | ||
| RequestID string `json:"requestId"` | ||
| FawryRefNumber string `json:"fawryRefNumber"` | ||
| MerchantRefNumber string `json:"merchantRefNumber"` | ||
| PaymentAmount string `json:"paymentAmount"` | ||
| OrderAmount string `json:"orderAmount"` | ||
| FawryFees string `json:"fawryFees"` | ||
| OrderStatus string `json:"orderStatus"` | ||
| PaymentMethod string `json:"paymentMethod"` | ||
| MessageSignature string `json:"messageSignature"` | ||
| InvoiceInfo InvoiceInfo `json:"invoiceInfo"` |
There was a problem hiding this comment.
Consider Using Numeric Types for Monetary Values.
Fields like PaymentAmount, OrderAmount, and FawryFees are defined as strings. Consider using a numeric type or a decimal library to avoid issues with arithmetic operations and ensure precision.
| func NewWebhook() *Webhook { | ||
| return &Webhook{} | ||
| } |
There was a problem hiding this comment.
Consider dependency injection for NewWebhook.
The NewWebhook function currently returns an empty Webhook. If the Webhook requires dependencies, consider using dependency injection to pass them during initialization.
func NewWebhook(dependencies ...interface{}) *Webhook {
return &Webhook{
// Initialize with dependencies
}
}| func (w *Webhook) Handle() http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| conn := task.ConnectorContextFromContext(r.Context()) | ||
|
|
||
| b, err := io.ReadAll(r.Body) | ||
|
|
||
| n := client.Notification{} | ||
| if err := json.Unmarshal(b, &n); err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| ctx, _ := contextutil.DetachedWithTimeout(r.Context(), 30*time.Second) | ||
| td, err := models.EncodeTaskDescriptor(TaskDescriptor{ | ||
| Name: TaskIngestPayment, | ||
| Key: TaskIngestPayment, | ||
| Payload: string(b), | ||
| }) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| conn.Scheduler().Schedule(ctx, td, models.TaskSchedulerOptions{ | ||
| ScheduleOption: models.OPTIONS_RUN_NOW, | ||
| RestartOption: models.OPTIONS_RESTART_IF_NOT_ACTIVE, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Enhance error handling in Handle method.
The method currently returns a 500 error for any JSON parsing issues. Consider using more specific status codes, such as 400 for bad requests, to better inform the client of the nature of the error.
- http.Error(w, err.Error(), http.StatusInternalServerError)
+ http.Error(w, "Invalid JSON payload", http.StatusBadRequest)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (w *Webhook) Handle() http.HandlerFunc { | |
| return func(w http.ResponseWriter, r *http.Request) { | |
| conn := task.ConnectorContextFromContext(r.Context()) | |
| b, err := io.ReadAll(r.Body) | |
| n := client.Notification{} | |
| if err := json.Unmarshal(b, &n); err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| ctx, _ := contextutil.DetachedWithTimeout(r.Context(), 30*time.Second) | |
| td, err := models.EncodeTaskDescriptor(TaskDescriptor{ | |
| Name: TaskIngestPayment, | |
| Key: TaskIngestPayment, | |
| Payload: string(b), | |
| }) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| conn.Scheduler().Schedule(ctx, td, models.TaskSchedulerOptions{ | |
| ScheduleOption: models.OPTIONS_RUN_NOW, | |
| RestartOption: models.OPTIONS_RESTART_IF_NOT_ACTIVE, | |
| }) | |
| } | |
| func (w *Webhook) Handle() http.HandlerFunc { | |
| return func(w http.ResponseWriter, r *http.Request) { | |
| conn := task.ConnectorContextFromContext(r.Context()) | |
| b, err := io.ReadAll(r.Body) | |
| n := client.Notification{} | |
| if err := json.Unmarshal(b, &n); err != nil { | |
| http.Error(w, "Invalid JSON payload", http.StatusBadRequest) | |
| return | |
| } | |
| ctx, _ := contextutil.DetachedWithTimeout(r.Context(), 30*time.Second) | |
| td, err := models.EncodeTaskDescriptor(TaskDescriptor{ | |
| Name: TaskIngestPayment, | |
| Key: TaskIngestPayment, | |
| Payload: string(b), | |
| }) | |
| if err != nil { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| conn.Scheduler().Schedule(ctx, td, models.TaskSchedulerOptions{ | |
| ScheduleOption: models.OPTIONS_RUN_NOW, | |
| RestartOption: models.OPTIONS_RESTART_IF_NOT_ACTIVE, | |
| }) | |
| } |
| func Resolve( | ||
| logger logging.Logger, | ||
| config Config, | ||
| taskMemoryState *TaskMemoryState, | ||
| ) func(taskDefinition TaskDescriptor) task.Task { | ||
| client := client.NewClient() | ||
| err := client.Init() | ||
| if err != nil { | ||
| logger.Error(err) | ||
| return func(taskDescriptor TaskDescriptor) task.Task { | ||
| return func() error { | ||
| return fmt.Errorf("cannot build fawry client: %w", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return func(taskDescriptor TaskDescriptor) task.Task { | ||
| switch taskDescriptor.Key { | ||
| case TaskMain: | ||
| return func() error { | ||
| return nil | ||
| } | ||
| case TaskIngestPayment: | ||
| return taskIngestPayment | ||
| default: | ||
| return func() error { | ||
| return fmt.Errorf("unknown task key: %s", taskDescriptor.Key) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in Resolve function.
The Resolve function logs an error and returns a task that will always fail if the client initialization fails. Consider implementing a retry mechanism or alerting system to handle such failures more gracefully.
if err != nil {
logger.Error(err)
// Implement retry logic or alerting
return func(taskDescriptor TaskDescriptor) task.Task {
return func() error {
return fmt.Errorf("cannot build fawry client: %w", err)
}
}
}| func (c *Connector) Uninstall(ctx task.ConnectorContext) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Implement logic for Uninstall method.
The Uninstall method currently returns nil. If there are resources to clean up or tasks to cancel, implement the necessary logic here.
func (c *Connector) Uninstall(ctx task.ConnectorContext) error {
// Implement uninstallation logic
return nil
}| func (c *Connector) UpdateConfig(ctx task.ConnectorContext, config models.ConnectorConfigObject) error { | ||
| panic("not supported") | ||
| } | ||
|
|
||
| func (c *Connector) InitiatePayment(ctx task.ConnectorContext, transfer *models.TransferInitiation) error { | ||
| panic("not supported") | ||
| } | ||
|
|
||
| // ReversePayment | ||
| func (c *Connector) ReversePayment(ctx task.ConnectorContext, reversal *models.TransferReversal) error { | ||
| panic("not supported") | ||
| } | ||
|
|
||
| // CreateExternalBankAccount | ||
| func (c *Connector) CreateExternalBankAccount(ctx task.ConnectorContext, account *models.BankAccount) error { | ||
| panic("not supported") | ||
| } |
There was a problem hiding this comment.
Handle unsupported operations gracefully.
The methods UpdateConfig, InitiatePayment, ReversePayment, and CreateExternalBankAccount currently panic. Consider returning a descriptive error instead to avoid disrupting the application flow.
func (c *Connector) UpdateConfig(ctx task.ConnectorContext, config models.ConnectorConfigObject) error {
return fmt.Errorf("operation not supported")
}| func newConnector() *Connector { | ||
| return &Connector{} | ||
| } |
There was a problem hiding this comment.
Enhance newConnector function with initialization logic.
The newConnector function currently returns an empty Connector. If there are configurations or dependencies to initialize, consider adding them here.
func newConnector(logger logging.Logger, cfg Config) *Connector {
return &Connector{
logger: logger,
cfg: cfg,
// Initialize other fields
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/payments/cmd/connectors/internal/connectors/fawry/client/notification.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/payments/cmd/connectors/internal/connectors/fawry/client/notification.go
No description provided.