fix(currencycloud): fixes after full testing #297
Conversation
WalkthroughThe pull request introduces modifications to the CurrencyCloud plugin's payment and payout handling. Changes include updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 73.05% 73.07% +0.01%
==========================================
Files 537 537
Lines 27115 27126 +11
==========================================
+ Hits 19810 19822 +12
- Misses 6238 6239 +1
+ Partials 1067 1065 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/connectors/plugins/public/currencycloud/payouts.go (1)
45-47: Consider differentiating between Reference and ReasonCurrently,
Descriptionis used for bothReferenceandReasonfields. These fields typically serve different purposes:
- Reference: A unique identifier for the transaction
- Reason: The purpose or context of the payment
Consider using more specific fields:
- Reference: pi.Description, - Reason: pi.Description, + Reference: pi.Reference, + Reason: pi.Description,internal/connectors/plugins/public/currencycloud/client/transactions.go (1)
23-24: LGTM! Consider adding field documentation.The new fields enhance transaction context and follow existing conventions. Consider adding documentation comments to explain the purpose and expected values of these fields.
type Transaction struct { ID string `json:"id"` AccountID string `json:"account_id"` Currency string `json:"currency"` Type string `json:"type"` Status string `json:"status"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` Action string `json:"action"` + // RelatedEntityType specifies the type of entity this transaction is related to (e.g., "payment", "transfer") RelatedEntityType string `json:"related_entity_type"` + // RelatedEntityID contains the unique identifier of the related entity RelatedEntityID string `json:"related_entity_id"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/connectors/plugins/public/currencycloud/client/payouts.go(1 hunks)internal/connectors/plugins/public/currencycloud/client/transactions.go(1 hunks)internal/connectors/plugins/public/currencycloud/payments.go(2 hunks)internal/connectors/plugins/public/currencycloud/payouts.go(1 hunks)internal/connectors/plugins/public/currencycloud/payouts_test.go(2 hunks)
🔇 Additional comments (5)
internal/connectors/plugins/public/currencycloud/client/payouts.go (2)
20-20: LGTM: Addition of Reason field to PayoutRequestThe addition of the
Reasonfield provides better context for payouts.
29-30: Consider making payment_type configurableThe
payment_typeis hardcoded as "regular". This might be too restrictive if CurrencyCloud supports other payment types.Run this script to check supported payment types:
internal/connectors/plugins/public/currencycloud/payouts_test.go (1)
112-113: LGTM: Test cases updated correctlyThe test cases have been properly updated to reflect the changes in the
PayoutRequeststructure, including the newReasonfield and removal ofOnBehalfOf.Also applies to: 141-142
internal/connectors/plugins/public/currencycloud/payments.go (2)
121-126: LGTM! Clean implementation of reference handling.Good implementation of fallback logic for payment reference, prioritizing RelatedEntityID while maintaining backward compatibility with transaction ID.
148-148: LGTM! Function signature enhancement improves type classification.The addition of entityType parameter allows for more precise payment type determination.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/connectors/plugins/public/currencycloud/payments_test.go (2)
390-430: Enhance test coverage with additional test cases.The test cases are well-structured but could be more comprehensive. Consider adding:
- Edge cases with empty strings for both
entityTypeandtransactionType- Additional entity types that might be handled by the function
- A description field in the test struct to document the purpose of each test case
Apply this diff to enhance the test cases:
tests := []struct { + name string entityType string transactionType string expectedPaymentType models.PaymentType }{ { + name: "inbound funds credit should be payin", entityType: "inbound_funds", transactionType: "credit", expectedPaymentType: models.PAYMENT_TYPE_PAYIN, }, + { + name: "empty entity type with credit should default to payin", + entityType: "", + transactionType: "credit", + expectedPaymentType: models.PAYMENT_TYPE_PAYIN, + }, + { + name: "empty transaction type should default to other", + entityType: "payment", + transactionType: "", + expectedPaymentType: models.PAYMENT_TYPE_OTHER, + }, // ... existing test cases ... }
432-439: Improve test implementation for better debugging and maintainability.The test implementation is good but could be enhanced for better debugging and maintainability.
Apply this diff to improve the test implementation:
for _, test := range tests { - t.Run(test.entityType+"-"+test.transactionType, func(t *testing.T) { + t.Run(test.name, func(t *testing.T) { t.Parallel() paymentType := matchTransactionType(test.entityType, test.transactionType) - require.Equal(t, test.expectedPaymentType, paymentType) + require.Equal(t, test.expectedPaymentType, paymentType, + "expected payment type %s for entityType=%q and transactionType=%q", + test.expectedPaymentType, test.entityType, test.transactionType) }) }
No description provided.