feat: GoCardless Connector for the Payment Service #326
Conversation
![gitstart-app[bot]](https://avatars.githubusercontent.com/in/46283?s=60&v=4){kind=small}
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThe pull request introduces a new GoCardless Connector into the system. It adds comprehensive documentation, client implementations, configuration management, and plugin registration for integrating with the GoCardless payment service provider. New functionality covers creating bank accounts, fetching creditors, customers, external accounts, payments, and users with support for pagination, state management, and metadata transformation. Additionally, robust testing and generated mock setups have been introduced to ensure reliable error handling and behavior across all supported operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant Client
participant GoCardlessAPI
User->>Plugin: Request CreateBankAccount(ba)
Plugin->>Plugin: Validate bank account (check metadata, IDs, account type)
Plugin->>Client: createBankAccount(ctx, ba)
Client->>GoCardlessAPI: Invoke CreateCreditor/CustomerBankAccount API
GoCardlessAPI-->>Client: Return account details or error
Client-->>Plugin: Return formatted account response
Plugin-->>User: Return CreateBankAccountResponse
Assessment against linked issues
Poem
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. 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (41)
internal/connectors/plugins/public/gocardless/client/mandate.go (1)
9-18: Simplify implementation and improve error handlingThe implementation can be more concise by reducing unnecessary blank lines and directly returning service call results. Consider adding error context to help with troubleshooting.
func (c *client) GetMandate(ctx context.Context, mandateId string) (*gocardless.Mandate, error) { - mandate, err := c.service.GetMandate(ctx, mandateId) - if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get mandate from GoCardless: %w", err) } - return mandate, nil }internal/connectors/plugins/public/gocardless/config.go (3)
11-15: Consider adding more explicit comments for each config field.Even though the JSON tags are self-explanatory, adding short comments above each field clarifies usage and validation requirements (e.g., clarifying that
ShouldFetchMandateis a string, but is used as a boolean flag in some contexts).
19-20: Refine error wrapping format.Currently, the error wrapping includes two
%wplaceholders in the same format string. This may cause confusion when reading logs. A clearer approach would be something like:-return Config{}, fmt.Errorf("%w: %w", err, models.ErrInvalidConfig) +return Config{}, fmt.Errorf("unmarshal error: %w: %v", models.ErrInvalidConfig, err)
23-24: Handle validation errors more explicitly.You're returning the validation error directly, which is generally fine. However, consider wrapping or providing a more contextual message to guide troubleshooting (e.g., “invalid config fields”). This can help users identify which fields failed validation without diving into raw errors.
internal/connectors/plugins/public/gocardless/client/creditors.go (3)
12-19: Validate pagination parameters.Before making the API call, consider checking if
pageSizeor the cursor parameters (after,before) are valid or empty. This could help prevent unexpected behavior if zero or negativepageSizeis provided.
27-31: Ensure graceful handling of empty or invalid timestamps.The loop attempts to parse
creditor.CreatedAtwith RFC3339Nano. Ifcreditor.CreatedAtis an empty string, we raise an error. Consider handling empty or malformed timestamps more gracefully (for example, defaulting to the current time or ignoring the record altogether).
33-44: Consider extracting common fields into a helper constructor.Building up a
GocardlessUserstruct is repeated in multiple functions (e.g.,GetCustomers). A shared helper function to map GoCardless structs into your localGocardlessUserstruct might reduce duplication and improve consistency.internal/connectors/plugins/public/gocardless/utils.go (2)
17-25: Improve error reporting inexternalAccountFromGocardlessData.When marshalling
datafails, consider returning a wrapped error with context (e.g.,"failed to marshal gocardless account data"). This can help pinpoint issues with nested or invalid data fields inGocardlessGenericAccount.
27-28: Clarify magic literal "/2".
defaultAsset := data.Currency + "/2"might be a domain-specific convention. Consider consolidating it in a named constant (e.g.,const defaultScale = "/2") or adding a brief comment to explain its purpose.internal/connectors/plugins/public/gocardless/client/customers.go (3)
12-20: Same pagination note as inGetCreditors.Check if
pageSize,after, andbeforeare valid to prevent potential unexpected behavior. This will reduce the risk of 4xx or 5xx errors from the GoCardless API.
28-32: Handle empty or malformed created-at timestamp.Similar to the creditors flow, an empty or invalid
CreatedAtwill cause parsing to fail. A fallback or more descriptive error might help diagnose issues quickly.
34-52: Avoid string concatenation for full names.
customer.GivenName + " " + customer.FamilyNamemay produce leading/trailing spaces if one of the fields is an empty string. Consider a small helper function that conditionally joins these strings or uses a structured format.internal/connectors/plugins/public/gocardless/users_test.go (1)
274-274: Remove extraneous parentheses inBeforeEach.
The double parentheses around the anonymous function appear unintended and may lead to confusion or errors.- BeforeEach((func() { + BeforeEach(func() {internal/connectors/plugins/public/gocardless/bank_account_creation.go (1)
124-125: Add test coverage for error handling.
The lines returning an error fromexternalAccountFromGocardlessDataseem untested. Consider adding a test that triggers this path to improve coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-125: internal/connectors/plugins/public/gocardless/bank_account_creation.go#L124-L125
Added lines #L124 - L125 were not covered by testsinternal/connectors/plugins/public/gocardless/payments_test.go (1)
189-189: Avoid repeating context names.
There is a second context block named"when there are no payments"which might cause confusion in test reports. Consider renaming one of them to clarify the scenario under test.internal/connectors/plugins/public/gocardless/README.md (1)
248-248: Minor grammar improvementConsider adding a comma after "Currently" for better readability.
- - Currently not supported, Gocardless does not support creating webhook from API + - Currently, not supported, Gocardless does not support creating webhook from API🧰 Tools
🪛 LanguageTool
[uncategorized] ~248-~248: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
internal/connectors/plugins/public/gocardless/bank_account_creation_test.go (1)
313-353: Duplicate test cases between contextsThere's duplication between the tests in "create bank account" and "create bank account failed" contexts. Both have similar test cases for error handling with almost identical expectations. Consider consolidating these tests to improve maintainability.
internal/connectors/plugins/public/gocardless/external_accounts_test.go (1)
94-105: Misleading test descriptionThe test description "should return an error when reference is empty" doesn't match the actual test, which is checking for an invalid reference format that doesn't start with 'CR' or 'CU'. Consider updating the description to accurately reflect the test's purpose.
- It("should return an error when reference is empty", func(ctx SpecContext) { + It("should return an error when reference format is invalid", func(ctx SpecContext) {internal/connectors/plugins/public/gocardless/client/bank_account_creation.go (1)
80-86: Add check for nil input in convertToGocardlessMetadataWhile the function works correctly, it doesn't handle a nil input which could lead to a panic.
Add a nil check:
func convertToGocardlessMetadata(input map[string]string) map[string]interface{} { + if input == nil { + return nil + } result := make(map[string]interface{}, len(input)) for k, v := range input { result[k] = v } return result }internal/connectors/plugins/public/gocardless/plugin_test.go (2)
30-33: Add test coverage forshouldFetchMandate:false.Currently, the
BeforeEachblock configuresshouldFetchMandate:"true". It would be beneficial to install the plugin at least once usingshouldFetchMandate:"false"and validate its behavior to ensure consistency and full coverage.
98-190: Reduce duplication in uninstalled-plugin test cases.Each test in this range follows the same pattern of constructing a request, invoking a method, and expecting
plugins.ErrNotYetInstalled. You could refactor these into a table-driven test or a loop-based approach to avoid repetitive code blocks, simplifying maintainability.internal/connectors/plugins/public/gocardless/payments.go (1)
23-27: Add negative test coverage for invalid JSON in thereq.State.Lines 25–26 return an error if the
req.Statecannot be unmarshaled, but there’s no test that provides malformed JSON to confirm that this error path works as intended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-26: internal/connectors/plugins/public/gocardless/payments.go#L25-L26
Added lines #L25 - L26 were not covered by testsinternal/connectors/plugins/public/gocardless/client/payments.go (1)
88-89: Increase test coverage forconvertGocardlessMetadata.Lines 88–89 convert all values to strings. Add tests covering various data types (e.g., numbers, booleans) or nested metadata structures to confirm correctness and avoid unexpected data loss.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-89: internal/connectors/plugins/public/gocardless/client/payments.go#L88-L89
Added lines #L88 - L89 were not covered by testsinternal/connectors/plugins/public/gocardless/client/client.go (1)
91-121: Add test coverage for serviceWrapper methods.Static analysis indicates lines 91–92, 95–96, 99–100, 103–104, 107–108, 111–112, 115–116, and 119–120 are not covered by tests. These methods simply pass requests to the underlying GoCardless service but should still be validated to ensure correct parameters and error handling. Adding basic tests will help maintain reliability.
Would you like help generating these test cases to cover pass-through logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 91-92: internal/connectors/plugins/public/gocardless/client/client.go#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 95-96: internal/connectors/plugins/public/gocardless/client/client.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: internal/connectors/plugins/public/gocardless/client/client.go#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 103-104: internal/connectors/plugins/public/gocardless/client/client.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 107-108: internal/connectors/plugins/public/gocardless/client/client.go#L107-L108
Added lines #L107 - L108 were not covered by tests
[warning] 111-112: internal/connectors/plugins/public/gocardless/client/client.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 115-116: internal/connectors/plugins/public/gocardless/client/client.go#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 119-120: internal/connectors/plugins/public/gocardless/client/client.go#L119-L120
Added lines #L119 - L120 were not covered by testsinternal/connectors/plugins/public/gocardless/users.go (1)
185-208: Consider updating creation time based on the last appended user.Currently,
lastUserCreationDateis updated only if the user is the final element inpagedUsers(even if that user might be skipped). You may want to track the creation date of the actual last appended user to ensure consistent pagination state, especially if some items are filtered out. This can be done by updatinglastUserCreationDatewhenever a user is appended.internal/connectors/plugins/public/gocardless/external_accounts.go (4)
33-35: Add coverage tests for state unmarshalling errors.Lines 33-35 handle JSON unmarshalling errors for
req.State, but according to static analysis, these lines are not covered by tests. Consider adding a negative test to confirm the code handles invalid JSON properly.Would you like help creating a test snippet to cover this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: internal/connectors/plugins/public/gocardless/external_accounts.go#L34-L35
Added lines #L34 - L35 were not covered by tests
43-45: Add coverage tests for 'from' payload unmarshal errors.Lines 43-45 handle JSON unmarshalling for
req.FromPayload; static analysis indicates these lines lack coverage. A test with invalid or malformed payload would help ensure robust error handling.I can propose a negative test case to validate this if needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-45: internal/connectors/plugins/public/gocardless/external_accounts.go#L44-L45
Added lines #L44 - L45 were not covered by tests
79-81: Test fillExternalAccounts error case.Static analysis flags lines 79-81 as uncovered. These lines handle the error returned by
fillExternalAccounts. Consider adding a scenario in whichfillExternalAccountsfails (e.g., data conversion error) to verify error propagation.Let me know if you want assistance devising specific test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-81: internal/connectors/plugins/public/gocardless/external_accounts.go#L80-L81
Added lines #L80 - L81 were not covered by tests
129-130: Add coverage for externalAccountFromGocardlessData failures.Lines 129-130 return an error if
externalAccountFromGocardlessDatafails. Including a test scenario that triggers this error ensures this branch is covered.Happy to provide a sample test snippet if desired.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-130: internal/connectors/plugins/public/gocardless/external_accounts.go#L129-L130
Added lines #L129 - L130 were not covered by testsinternal/connectors/plugins/public/gocardless/plugin.go (2)
35-36: Consider parsing boolean config more robustly.Currently,
shouldFetchMandate := config.ShouldFetchMandate == "true"uses a string comparison. You might consider a dedicated boolean field or more robust parsing (e.g., distortion from uppercase, whitespace, “True”, etc.).- shouldFetchMandate := config.ShouldFetchMandate == "true" + boolVal, _ := strconv.ParseBool(config.ShouldFetchMandate) + shouldFetchMandate := boolVal
61-63: Future functionality placeholders.Most methods return
plugins.ErrNotImplemented. If this is intentional (e.g., phased rollout), that’s fine. Otherwise, consider either removing them or providing minimal viable implementations to avoid confusion.If you need help implementing or want to split it into smaller tasks, let me know.
Also applies to: 65-70, 72-77, 115-121, 123-129, 131-139, 140-146, 148-154, 156-163, 165-173, 175-183
internal/connectors/plugins/public/gocardless/client/client_generated.go (10)
124-129: Add coverage forGetMandatein MockClient.The method (lines 124-129) and its recorder (lines 133-135) are flagged as uncovered. Consider adding a test scenario that invokes the
GetMandatecall on the mock to ensure these lines are exercised.Also applies to: 133-135
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-129: internal/connectors/plugins/public/gocardless/client/client_generated.go#L124-L129
Added lines #L124 - L129 were not covered by tests
155-157: Add coverage forNewWithServicein MockClient.Lines 155-157 and 161-163 remain uncovered. If you rely on
NewWithServicein tests, add a scenario that explicitly calls it. Otherwise, consider removing it if it’s unused.Also applies to: 161-163
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 155-157: internal/connectors/plugins/public/gocardless/client/client_generated.go#L155-L157
Added lines #L155 - L157 were not covered by tests
195-196: Cover credential for-loop inCreateGocardlessCreditorBankAccount.Lines 195-196 handle optional request parameters. If you expect to pass multiple request options, add a test verifying the loop’s logic. If not, consider a simpler approach.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 195-196: internal/connectors/plugins/public/gocardless/client/client_generated.go#L195-L196
Added lines #L195 - L196 were not covered by tests
215-216: Cover for-loop inCreateGocardlessCustomerBankAccount.Lines 215-216 are also uncovered. The same approach applies: test passing multiple request options or remove code if unneeded.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 215-216: internal/connectors/plugins/public/gocardless/client/client_generated.go#L215-L216
Added lines #L215 - L216 were not covered by tests
235-236: Add coverage for iteration logic inGetGocardlessCreditorBankAccounts.Lines 235-236 handle appending request options. Test them by passing multiple request parameters or confirm they’re not strictly necessary.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-236: internal/connectors/plugins/public/gocardless/client/client_generated.go#L235-L236
Added lines #L235 - L236 were not covered by tests
255-256: Add coverage for iteration logic inGetGocardlessCreditors.Similar rationale applies to lines 255-256. Consider a test scenario with multiple request options.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 255-256: internal/connectors/plugins/public/gocardless/client/client_generated.go#L255-L256
Added lines #L255 - L256 were not covered by tests
275-276: Add coverage for iteration logic inGetGocardlessCustomerBankAccounts.Lines 275-276 remain uncovered. If multiple options are used in production, a test verifying that scenario is recommended.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 275-276: internal/connectors/plugins/public/gocardless/client/client_generated.go#L275-L276
Added lines #L275 - L276 were not covered by tests
295-296: Add coverage for iteration logic inGetGocardlessCustomers.As with previous methods, lines 295-296 handle request options. Validate with a test or remove if unnecessary.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 295-296: internal/connectors/plugins/public/gocardless/client/client_generated.go#L295-L296
Added lines #L295 - L296 were not covered by tests
315-316: Add coverage for iteration logic inGetGocardlessPayments.The iteration logic in lines 315-316 remains untested. Consider a scenario passing multiple request options to confirm correctness.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 315-316: internal/connectors/plugins/public/gocardless/client/client_generated.go#L315-L316
Added lines #L315 - L316 were not covered by tests
335-336: Add coverage for iteration logic inGetMandate(GoCardlessService).Lines 335-336 show the same pattern of appending request options. Include a test scenario if you plan on using multiple request options in real usage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 335-336: internal/connectors/plugins/public/gocardless/client/client_generated.go#L335-L336
Added lines #L335 - L336 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (27)
internal/connectors/plugins/public/gocardless/README.md(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/gocardless/capabilities.go(1 hunks)internal/connectors/plugins/public/gocardless/client/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client_generated.go(1 hunks)internal/connectors/plugins/public/gocardless/client/creditors.go(1 hunks)internal/connectors/plugins/public/gocardless/client/customers.go(1 hunks)internal/connectors/plugins/public/gocardless/client/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/client/mandate.go(1 hunks)internal/connectors/plugins/public/gocardless/client/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/client/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/config.go(1 hunks)internal/connectors/plugins/public/gocardless/currencies.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/gocardless/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/payments_test.go(1 hunks)internal/connectors/plugins/public/gocardless/plugin.go(1 hunks)internal/connectors/plugins/public/gocardless/plugin_test.go(1 hunks)internal/connectors/plugins/public/gocardless/users.go(1 hunks)internal/connectors/plugins/public/gocardless/users_test.go(1 hunks)internal/connectors/plugins/public/gocardless/utils.go(1 hunks)internal/connectors/plugins/public/gocardless/workflow.go(1 hunks)internal/connectors/plugins/public/list.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- internal/connectors/plugins/public/gocardless/currencies.go
- internal/connectors/plugins/public/gocardless/capabilities.go
- internal/connectors/plugins/public/gocardless/client/metadata.go
🧰 Additional context used
🪛 LanguageTool
internal/connectors/plugins/public/gocardless/README.md
[uncategorized] ~149-~149: Loose punctuation mark.
Context: ...Cursor-based Pagination**: - after: Points to the last item in the current ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~154-~154: Loose punctuation mark.
Context: ...art after "CU123" ``` - before: Points to the first item in the current...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~162-~162: Loose punctuation mark.
Context: ...ate Tracking**: - lastCreationDate: Timestamp of the most recently processe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~248-~248: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 GitHub Check: codecov/patch
internal/connectors/plugins/public/gocardless/external_accounts.go
[warning] 34-35: internal/connectors/plugins/public/gocardless/external_accounts.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 44-45: internal/connectors/plugins/public/gocardless/external_accounts.go#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 80-81: internal/connectors/plugins/public/gocardless/external_accounts.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 129-130: internal/connectors/plugins/public/gocardless/external_accounts.go#L129-L130
Added lines #L129 - L130 were not covered by tests
internal/connectors/plugins/public/gocardless/client/payments.go
[warning] 88-89: internal/connectors/plugins/public/gocardless/client/payments.go#L88-L89
Added lines #L88 - L89 were not covered by tests
internal/connectors/plugins/public/gocardless/payments.go
[warning] 25-26: internal/connectors/plugins/public/gocardless/payments.go#L25-L26
Added lines #L25 - L26 were not covered by tests
internal/connectors/plugins/public/gocardless/bank_account_creation.go
[warning] 124-125: internal/connectors/plugins/public/gocardless/bank_account_creation.go#L124-L125
Added lines #L124 - L125 were not covered by tests
internal/connectors/plugins/public/gocardless/client/client.go
[warning] 91-92: internal/connectors/plugins/public/gocardless/client/client.go#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 95-96: internal/connectors/plugins/public/gocardless/client/client.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: internal/connectors/plugins/public/gocardless/client/client.go#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 103-104: internal/connectors/plugins/public/gocardless/client/client.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 107-108: internal/connectors/plugins/public/gocardless/client/client.go#L107-L108
Added lines #L107 - L108 were not covered by tests
[warning] 111-112: internal/connectors/plugins/public/gocardless/client/client.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 115-116: internal/connectors/plugins/public/gocardless/client/client.go#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 119-120: internal/connectors/plugins/public/gocardless/client/client.go#L119-L120
Added lines #L119 - L120 were not covered by tests
[warning] 125-126: internal/connectors/plugins/public/gocardless/client/client.go#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 131-132: internal/connectors/plugins/public/gocardless/client/client.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 137-138: internal/connectors/plugins/public/gocardless/client/client.go#L137-L138
Added lines #L137 - L138 were not covered by tests
internal/connectors/plugins/public/gocardless/client/client_generated.go
[warning] 124-129: internal/connectors/plugins/public/gocardless/client/client_generated.go#L124-L129
Added lines #L124 - L129 were not covered by tests
[warning] 133-135: internal/connectors/plugins/public/gocardless/client/client_generated.go#L133-L135
Added lines #L133 - L135 were not covered by tests
[warning] 155-157: internal/connectors/plugins/public/gocardless/client/client_generated.go#L155-L157
Added lines #L155 - L157 were not covered by tests
[warning] 161-163: internal/connectors/plugins/public/gocardless/client/client_generated.go#L161-L163
Added lines #L161 - L163 were not covered by tests
[warning] 195-196: internal/connectors/plugins/public/gocardless/client/client_generated.go#L195-L196
Added lines #L195 - L196 were not covered by tests
[warning] 215-216: internal/connectors/plugins/public/gocardless/client/client_generated.go#L215-L216
Added lines #L215 - L216 were not covered by tests
[warning] 235-236: internal/connectors/plugins/public/gocardless/client/client_generated.go#L235-L236
Added lines #L235 - L236 were not covered by tests
[warning] 255-256: internal/connectors/plugins/public/gocardless/client/client_generated.go#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 275-276: internal/connectors/plugins/public/gocardless/client/client_generated.go#L275-L276
Added lines #L275 - L276 were not covered by tests
[warning] 295-296: internal/connectors/plugins/public/gocardless/client/client_generated.go#L295-L296
Added lines #L295 - L296 were not covered by tests
[warning] 315-316: internal/connectors/plugins/public/gocardless/client/client_generated.go#L315-L316
Added lines #L315 - L316 were not covered by tests
[warning] 335-336: internal/connectors/plugins/public/gocardless/client/client_generated.go#L335-L336
Added lines #L335 - L336 were not covered by tests
🔇 Additional comments (15)
internal/connectors/plugins/public/list.go (1)
10-10: GoCardless connector successfully importedThe GoCardless connector has been properly added to the list of plugins following the established pattern and maintaining alphabetical order.
internal/connectors/plugins/public/gocardless/metadata.go (1)
5-11: Well-implemented metadata extraction functionThe function is clean, follows Go best practices, and provides an informative error message. Good implementation.
internal/connectors/plugins/public/gocardless/workflow.go (1)
12-33: Well-structured workflow definitionThe workflow is clearly organized with appropriate task types and periodic scheduling. The nested structure for fetching external accounts after other data is a logical approach.
internal/connectors/plugins/public/gocardless/users_test.go (2)
25-36: Good overall test initialization structure.
The usage ofBeforeEachto initialize test data and the plugin instance is clear and follows Ginkgo best practices.
51-78: Solid negative testing.
This test effectively validates error scenarios by mocking the client to return errors and verifying that the errors are propagated correctly.internal/connectors/plugins/public/gocardless/bank_account_creation.go (1)
49-62: Clear and consistent US-specific account type validation.
The logic to enforce “checking” or “savings” for US-based accounts is straightforward and ensures compliance with country requirements.internal/connectors/plugins/public/gocardless/payments_test.go (2)
254-283: Robust error testing for mandates.
This test adequately ensures that an error fetching a mandate halts the operation and returns the expected error.
285-324: Comprehensive success-case validation.
The test confirms correct payment parsing and state updates, showing good coverage for the happy path scenario.internal/connectors/plugins/public/gocardless/README.md (1)
1-314: Well-documented connector with comprehensive implementation details!The README provides excellent documentation covering all aspects of the GoCardless connector: features, configuration, supported currencies, usage examples, state management, error handling, and testing. The pagination and state management explanations are particularly detailed and helpful for understanding the connector's behavior.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~149-~149: Loose punctuation mark.
Context: ...Cursor-based Pagination**: -after: Points to the last item in the current ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~154-~154: Loose punctuation mark.
Context: ...art after "CU123" ``` -before: Points to the first item in the current...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~162-~162: Loose punctuation mark.
Context: ...ate Tracking**: -lastCreationDate: Timestamp of the most recently processe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~248-~248: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
internal/connectors/plugins/public/gocardless/bank_account_creation_test.go (1)
17-522: Comprehensive test coverage for bank account creationThe tests thoroughly cover both success and error scenarios for bank account creation, including validation of required fields, format checking, and handling API responses. The test structure with table-driven tests is a good approach for comprehensive coverage.
internal/connectors/plugins/public/gocardless/external_accounts_test.go (1)
18-482: Thorough testing of external accounts functionalityThe tests provide excellent coverage of the external accounts functionality, including pagination, state management, and error handling. Both creditor and customer account fetching are well-tested.
internal/connectors/plugins/public/gocardless/payments.go (1)
52-59: Verify skipping entries with the same creation time.Skipping payments with
creationDate <= oldState.LastCreationDatemight exclude multiple payments sharing identical timestamps. Double-check whether this logic aligns with your desired pagination behavior, especially if the external API can return multiple records with the sameCreatedAt.internal/connectors/plugins/public/gocardless/client/external_accounts.go (1)
19-25: Handle unrecognized ownerID prefixes.Currently, if
ownerIDneither starts with"CR"nor"CU", the function returns empty results without an explicit error. Consider whether you want to treat such cases as valid (i.e., returning no accounts) or raise an error to surface unexpected inputs.internal/connectors/plugins/public/gocardless/client/client.go (1)
125-149: Improve test coverage for theNewfunction.Lines 125–126, 131–132, and 137–138 show incomplete test coverage for the constructor logic. Consider adding test cases to ensure every branch of initialization and error handling is exercised (e.g., empty endpoint, invalid access token).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-126: internal/connectors/plugins/public/gocardless/client/client.go#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 131-132: internal/connectors/plugins/public/gocardless/client/client.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 137-138: internal/connectors/plugins/public/gocardless/client/client.go#L137-L138
Added lines #L137 - L138 were not covered by testsinternal/connectors/plugins/public/gocardless/plugin.go (1)
107-113: Verify test coverage forCreateBankAccount.Although
CreateBankAccountdelegates top.createBankAccount, confirm that these lines are tested end-to-end, especially any error or edge cases. Static analysis didn’t raise a warning here, but ensuring coverage remains a best practice.
| fetchOthers = "fetch_others" | ||
| fetchCustomers = "fetch_accounts" | ||
| fetchNextAccounts = "fetch_external_accounts" | ||
| fetchNextPayments = "fetch_payments" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Naming inconsistency in task constants
There's a mismatch between the constant name fetchCustomers and its value fetch_accounts. This could lead to confusion if these constants are used in the codebase. Consider renaming the constant to match its value or vice versa for consistency.
🏁 Script executed:
#!/bin/bash
# Check if the fetchCustomers constant is being used in the codebase
echo "Searching for usage of fetchCustomers constant in the codebase..."
rg "fetchCustomers" --type goLength of output: 266
Resolve naming inconsistency in task constants
The constant fetchCustomers (line 7 in internal/connectors/plugins/public/gocardless/workflow.go) is defined with the string value "fetch_accounts". A search confirms that this constant appears only in its definition and isn’t referenced elsewhere, which might indicate that its name (suggesting customer-related data) contradicts its actual value (account-related data). Please clarify whether the constant should be renamed (e.g., to fetchAccounts) or its value adjusted (e.g., "fetch_customers"), and verify if it’s needed at all in the current workflow.
| if creditor != "" && creditor[:2] != "CR" { | ||
| return fmt.Errorf("creditor ID must start with 'CR'") | ||
| } | ||
|
|
||
| if customer != "" && customer[:2] != "CU" { | ||
| return fmt.Errorf("customer ID must start with 'CU'") | ||
| } |
There was a problem hiding this comment.
Prevent potential out-of-bounds string indexing.
Accessing creditor[:2] or customer[:2] can panic if the string length is less than 2.
-if creditor != "" && creditor[:2] != "CR" {
- return fmt.Errorf("creditor ID must start with 'CR'")
-}
+if len(creditor) > 1 && creditor[:2] != "CR" {
+ return fmt.Errorf("creditor ID must start with 'CR'")
+}
-if customer != "" && customer[:2] != "CU" {
- return fmt.Errorf("customer ID must start with 'CU'")
-}
+if len(customer) > 1 && customer[:2] != "CU" {
+ return fmt.Errorf("customer ID must start with 'CU'")
+}📝 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.
| if creditor != "" && creditor[:2] != "CR" { | |
| return fmt.Errorf("creditor ID must start with 'CR'") | |
| } | |
| if customer != "" && customer[:2] != "CU" { | |
| return fmt.Errorf("customer ID must start with 'CU'") | |
| } | |
| if len(creditor) > 1 && creditor[:2] != "CR" { | |
| return fmt.Errorf("creditor ID must start with 'CR'") | |
| } | |
| if len(customer) > 1 && customer[:2] != "CU" { | |
| return fmt.Errorf("customer ID must start with 'CU'") | |
| } |
| func (c *client) CreateCreditorBankAccount(ctx context.Context, creditor string, ba models.BankAccount) ( | ||
| *gocardless.CreditorBankAccount, error, | ||
| ) { | ||
|
|
||
| ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_creditor_bank_account") | ||
|
|
||
| setAsDefaultPayoutAccount := false | ||
| var metadata map[string]interface{} | ||
|
|
||
| if ba.Metadata != nil { | ||
| metadata = convertToGocardlessMetadata(ba.Metadata) | ||
| setAsDefaultPayoutAccount = ba.Metadata["set_as_default_payout_account"] == "true" | ||
| } | ||
|
|
||
| bankAccount, err := c.service.CreateGocardlessCreditorBankAccount(ctx, gocardless.CreditorBankAccountCreateParams{ | ||
| AccountHolderName: ba.Name, | ||
| AccountNumber: *ba.AccountNumber, | ||
| AccountType: ba.Metadata["account_type"], | ||
| BankCode: *ba.SwiftBicCode, | ||
| BranchCode: ba.Metadata["branch_code"], | ||
| CountryCode: *ba.Country, | ||
| Currency: ba.Metadata["currency"], | ||
| Iban: *ba.IBAN, | ||
| Links: gocardless.CreditorBankAccountCreateParamsLinks{Creditor: creditor}, | ||
| SetAsDefaultPayoutAccount: setAsDefaultPayoutAccount, | ||
| Metadata: metadata, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return bankAccount, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference in CreateCreditorBankAccount
The method dereferences several pointers (ba.AccountNumber, ba.SwiftBicCode, ba.Country, ba.IBAN) without checking if they're nil. This could lead to a panic if any of these fields are nil.
Add nil checks before dereferencing pointers:
func (c *client) CreateCreditorBankAccount(ctx context.Context, creditor string, ba models.BankAccount) (
*gocardless.CreditorBankAccount, error,
) {
ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_creditor_bank_account")
setAsDefaultPayoutAccount := false
var metadata map[string]interface{}
if ba.Metadata != nil {
metadata = convertToGocardlessMetadata(ba.Metadata)
setAsDefaultPayoutAccount = ba.Metadata["set_as_default_payout_account"] == "true"
}
+ // Validate required fields
+ if ba.AccountNumber == nil {
+ return nil, fmt.Errorf("account number is required")
+ }
+ if ba.SwiftBicCode == nil {
+ return nil, fmt.Errorf("swift bic code is required")
+ }
+ if ba.Country == nil {
+ return nil, fmt.Errorf("country is required")
+ }
+ if ba.IBAN == nil {
+ return nil, fmt.Errorf("IBAN is required")
+ }
bankAccount, err := c.service.CreateGocardlessCreditorBankAccount(ctx, gocardless.CreditorBankAccountCreateParams{
AccountHolderName: ba.Name,
AccountNumber: *ba.AccountNumber,
AccountType: ba.Metadata["account_type"],
BankCode: *ba.SwiftBicCode,
BranchCode: ba.Metadata["branch_code"],
CountryCode: *ba.Country,
Currency: ba.Metadata["currency"],
Iban: *ba.IBAN,
Links: gocardless.CreditorBankAccountCreateParamsLinks{Creditor: creditor},
SetAsDefaultPayoutAccount: setAsDefaultPayoutAccount,
Metadata: metadata,
})
if err != nil {
return nil, err
}
return bankAccount, nil
}📝 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 (c *client) CreateCreditorBankAccount(ctx context.Context, creditor string, ba models.BankAccount) ( | |
| *gocardless.CreditorBankAccount, error, | |
| ) { | |
| ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_creditor_bank_account") | |
| setAsDefaultPayoutAccount := false | |
| var metadata map[string]interface{} | |
| if ba.Metadata != nil { | |
| metadata = convertToGocardlessMetadata(ba.Metadata) | |
| setAsDefaultPayoutAccount = ba.Metadata["set_as_default_payout_account"] == "true" | |
| } | |
| bankAccount, err := c.service.CreateGocardlessCreditorBankAccount(ctx, gocardless.CreditorBankAccountCreateParams{ | |
| AccountHolderName: ba.Name, | |
| AccountNumber: *ba.AccountNumber, | |
| AccountType: ba.Metadata["account_type"], | |
| BankCode: *ba.SwiftBicCode, | |
| BranchCode: ba.Metadata["branch_code"], | |
| CountryCode: *ba.Country, | |
| Currency: ba.Metadata["currency"], | |
| Iban: *ba.IBAN, | |
| Links: gocardless.CreditorBankAccountCreateParamsLinks{Creditor: creditor}, | |
| SetAsDefaultPayoutAccount: setAsDefaultPayoutAccount, | |
| Metadata: metadata, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return bankAccount, nil | |
| } | |
| func (c *client) CreateCreditorBankAccount(ctx context.Context, creditor string, ba models.BankAccount) ( | |
| *gocardless.CreditorBankAccount, error, | |
| ) { | |
| ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_creditor_bank_account") | |
| setAsDefaultPayoutAccount := false | |
| var metadata map[string]interface{} | |
| if ba.Metadata != nil { | |
| metadata = convertToGocardlessMetadata(ba.Metadata) | |
| setAsDefaultPayoutAccount = ba.Metadata["set_as_default_payout_account"] == "true" | |
| } | |
| // Validate required fields | |
| if ba.AccountNumber == nil { | |
| return nil, fmt.Errorf("account number is required") | |
| } | |
| if ba.SwiftBicCode == nil { | |
| return nil, fmt.Errorf("swift bic code is required") | |
| } | |
| if ba.Country == nil { | |
| return nil, fmt.Errorf("country is required") | |
| } | |
| if ba.IBAN == nil { | |
| return nil, fmt.Errorf("IBAN is required") | |
| } | |
| bankAccount, err := c.service.CreateGocardlessCreditorBankAccount(ctx, gocardless.CreditorBankAccountCreateParams{ | |
| AccountHolderName: ba.Name, | |
| AccountNumber: *ba.AccountNumber, | |
| AccountType: ba.Metadata["account_type"], | |
| BankCode: *ba.SwiftBicCode, | |
| BranchCode: ba.Metadata["branch_code"], | |
| CountryCode: *ba.Country, | |
| Currency: ba.Metadata["currency"], | |
| Iban: *ba.IBAN, | |
| Links: gocardless.CreditorBankAccountCreateParamsLinks{Creditor: creditor}, | |
| SetAsDefaultPayoutAccount: setAsDefaultPayoutAccount, | |
| Metadata: metadata, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return bankAccount, nil | |
| } |
| bankAccount, err := c.service.CreateGocardlessCreditorBankAccount(ctx, gocardless.CreditorBankAccountCreateParams{ | ||
| AccountHolderName: ba.Name, | ||
| AccountNumber: *ba.AccountNumber, | ||
| AccountType: ba.Metadata["account_type"], | ||
| BankCode: *ba.SwiftBicCode, | ||
| BranchCode: ba.Metadata["branch_code"], | ||
| CountryCode: *ba.Country, | ||
| Currency: ba.Metadata["currency"], | ||
| Iban: *ba.IBAN, | ||
| Links: gocardless.CreditorBankAccountCreateParamsLinks{Creditor: creditor}, | ||
| SetAsDefaultPayoutAccount: setAsDefaultPayoutAccount, | ||
| Metadata: metadata, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing validation for required metadata fields
The code doesn't validate if required metadata fields like account_type, branch_code, and currency exist in ba.Metadata. If they're missing, empty values will be sent to the API, potentially causing errors.
Add validation for required metadata fields:
if ba.Metadata != nil {
metadata = convertToGocardlessMetadata(ba.Metadata)
setAsDefaultPayoutAccount = ba.Metadata["set_as_default_payout_account"] == "true"
}
+ // Validate required metadata fields
+ if _, ok := ba.Metadata["account_type"]; !ok {
+ return nil, fmt.Errorf("account_type is required in metadata")
+ }
+ if _, ok := ba.Metadata["branch_code"]; !ok {
+ return nil, fmt.Errorf("branch_code is required in metadata")
+ }
+ if _, ok := ba.Metadata["currency"]; !ok {
+ return nil, fmt.Errorf("currency is required in metadata")
+ }| func (c *client) CreateCustomerBankAccount(ctx context.Context, customer string, ba models.BankAccount) ( | ||
| *gocardless.CustomerBankAccount, error, | ||
| ) { | ||
|
|
||
| ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_customer_bank_account") | ||
|
|
||
| var metadata map[string]interface{} | ||
|
|
||
| if ba.Metadata != nil { | ||
| metadata = convertToGocardlessMetadata(ba.Metadata) | ||
| } | ||
|
|
||
| bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{ | ||
| AccountHolderName: ba.Name, | ||
| AccountNumber: *ba.AccountNumber, | ||
| AccountType: ba.Metadata["account_type"], | ||
| BankCode: *ba.SwiftBicCode, | ||
| BranchCode: ba.Metadata["branch_code"], | ||
| CountryCode: *ba.Country, | ||
| Currency: ba.Metadata["currency"], | ||
| Iban: *ba.IBAN, | ||
| Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer}, | ||
| Metadata: metadata, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return bankAccount, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference in CreateCustomerBankAccount
Similar to the previous method, this one also dereferences pointers without nil checks, which could lead to panics.
Add nil checks for required pointers:
func (c *client) CreateCustomerBankAccount(ctx context.Context, customer string, ba models.BankAccount) (
*gocardless.CustomerBankAccount, error,
) {
ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_customer_bank_account")
var metadata map[string]interface{}
if ba.Metadata != nil {
metadata = convertToGocardlessMetadata(ba.Metadata)
}
+ // Validate required fields
+ if ba.AccountNumber == nil {
+ return nil, fmt.Errorf("account number is required")
+ }
+ if ba.SwiftBicCode == nil {
+ return nil, fmt.Errorf("swift bic code is required")
+ }
+ if ba.Country == nil {
+ return nil, fmt.Errorf("country is required")
+ }
+ if ba.IBAN == nil {
+ return nil, fmt.Errorf("IBAN is required")
+ }
bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{
AccountHolderName: ba.Name,
AccountNumber: *ba.AccountNumber,
AccountType: ba.Metadata["account_type"],
BankCode: *ba.SwiftBicCode,
BranchCode: ba.Metadata["branch_code"],
CountryCode: *ba.Country,
Currency: ba.Metadata["currency"],
Iban: *ba.IBAN,
Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer},
Metadata: metadata,
})
if err != nil {
return nil, err
}
return bankAccount, nil
}📝 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 (c *client) CreateCustomerBankAccount(ctx context.Context, customer string, ba models.BankAccount) ( | |
| *gocardless.CustomerBankAccount, error, | |
| ) { | |
| ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_customer_bank_account") | |
| var metadata map[string]interface{} | |
| if ba.Metadata != nil { | |
| metadata = convertToGocardlessMetadata(ba.Metadata) | |
| } | |
| bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{ | |
| AccountHolderName: ba.Name, | |
| AccountNumber: *ba.AccountNumber, | |
| AccountType: ba.Metadata["account_type"], | |
| BankCode: *ba.SwiftBicCode, | |
| BranchCode: ba.Metadata["branch_code"], | |
| CountryCode: *ba.Country, | |
| Currency: ba.Metadata["currency"], | |
| Iban: *ba.IBAN, | |
| Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer}, | |
| Metadata: metadata, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return bankAccount, nil | |
| } | |
| func (c *client) CreateCustomerBankAccount(ctx context.Context, customer string, ba models.BankAccount) ( | |
| *gocardless.CustomerBankAccount, error, | |
| ) { | |
| ctx = context.WithValue(ctx, metrics.MetricOperationContextKey, "create_customer_bank_account") | |
| var metadata map[string]interface{} | |
| if ba.Metadata != nil { | |
| metadata = convertToGocardlessMetadata(ba.Metadata) | |
| } | |
| // Validate required fields | |
| if ba.AccountNumber == nil { | |
| return nil, fmt.Errorf("account number is required") | |
| } | |
| if ba.SwiftBicCode == nil { | |
| return nil, fmt.Errorf("swift bic code is required") | |
| } | |
| if ba.Country == nil { | |
| return nil, fmt.Errorf("country is required") | |
| } | |
| if ba.IBAN == nil { | |
| return nil, fmt.Errorf("IBAN is required") | |
| } | |
| bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{ | |
| AccountHolderName: ba.Name, | |
| AccountNumber: *ba.AccountNumber, | |
| AccountType: ba.Metadata["account_type"], | |
| BankCode: *ba.SwiftBicCode, | |
| BranchCode: ba.Metadata["branch_code"], | |
| CountryCode: *ba.Country, | |
| Currency: ba.Metadata["currency"], | |
| Iban: *ba.IBAN, | |
| Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer}, | |
| Metadata: metadata, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return bankAccount, nil | |
| } |
| bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{ | ||
| AccountHolderName: ba.Name, | ||
| AccountNumber: *ba.AccountNumber, | ||
| AccountType: ba.Metadata["account_type"], | ||
| BankCode: *ba.SwiftBicCode, | ||
| BranchCode: ba.Metadata["branch_code"], | ||
| CountryCode: *ba.Country, | ||
| Currency: ba.Metadata["currency"], | ||
| Iban: *ba.IBAN, | ||
| Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer}, | ||
| Metadata: metadata, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing validation for metadata fields in CreateCustomerBankAccount
Similar to the creditor method, this method also doesn't validate required metadata fields.
Add validation for required metadata fields:
if ba.Metadata != nil {
metadata = convertToGocardlessMetadata(ba.Metadata)
}
+ // Validate required metadata fields
+ if _, ok := ba.Metadata["account_type"]; !ok {
+ return nil, fmt.Errorf("account_type is required in metadata")
+ }
+ if _, ok := ba.Metadata["branch_code"]; !ok {
+ return nil, fmt.Errorf("branch_code is required in metadata")
+ }
+ if _, ok := ba.Metadata["currency"]; !ok {
+ return nil, fmt.Errorf("currency is required in metadata")
+ }📝 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.
| bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{ | |
| AccountHolderName: ba.Name, | |
| AccountNumber: *ba.AccountNumber, | |
| AccountType: ba.Metadata["account_type"], | |
| BankCode: *ba.SwiftBicCode, | |
| BranchCode: ba.Metadata["branch_code"], | |
| CountryCode: *ba.Country, | |
| Currency: ba.Metadata["currency"], | |
| Iban: *ba.IBAN, | |
| Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer}, | |
| Metadata: metadata, | |
| }) | |
| if ba.Metadata != nil { | |
| metadata = convertToGocardlessMetadata(ba.Metadata) | |
| } | |
| // Validate required metadata fields | |
| if _, ok := ba.Metadata["account_type"]; !ok { | |
| return nil, fmt.Errorf("account_type is required in metadata") | |
| } | |
| if _, ok := ba.Metadata["branch_code"]; !ok { | |
| return nil, fmt.Errorf("branch_code is required in metadata") | |
| } | |
| if _, ok := ba.Metadata["currency"]; !ok { | |
| return nil, fmt.Errorf("currency is required in metadata") | |
| } | |
| bankAccount, err := c.service.CreateGocardlessCustomerBankAccount(ctx, gocardless.CustomerBankAccountCreateParams{ | |
| AccountHolderName: ba.Name, | |
| AccountNumber: *ba.AccountNumber, | |
| AccountType: ba.Metadata["account_type"], | |
| BankCode: *ba.SwiftBicCode, | |
| BranchCode: ba.Metadata["branch_code"], | |
| CountryCode: *ba.Country, | |
| Currency: ba.Metadata["currency"], | |
| Iban: *ba.IBAN, | |
| Links: gocardless.CustomerBankAccountCreateParamsLinks{Customer: customer}, | |
| Metadata: metadata, | |
| }) |
internal/connectors/plugins/public/gocardless/client/external_accounts.go
Outdated
Show resolved
Hide resolved
| if !needMore { | ||
| users = users[:pageSize] | ||
| } |
There was a problem hiding this comment.
Same out-of-bounds slice risk for customer users.
The slicing logic in getCustomersUsers mirrors the same risk if len(users) < pageSize. Use the same check to avoid a runtime panic.
| if !needMore { | ||
| users = users[:pageSize] | ||
| } |
There was a problem hiding this comment.
Prevent potential out-of-bounds slice error.
When needMore is false, the slice is trimmed to pageSize with users = users[:pageSize], which panics if len(users) < pageSize. Ensure the slice has at least pageSize elements before slicing. A safe check might be:
- if !needMore {
- users = users[:pageSize]
- }
+ if !needMore && len(users) >= pageSize {
+ users = users[:pageSize]
+ }📝 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.
| if !needMore { | |
| users = users[:pageSize] | |
| } | |
| if !needMore && len(users) >= pageSize { | |
| users = users[:pageSize] | |
| } |
gitstart-app
bot
force-pushed
the
LNK-38
branch
from
9515648 to
7d1f0a4
Compare
Tested cases
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (18)
internal/models/errors_test.go (1)
10-15: Test updated to use new validation error typeThe test correctly uses the new
NewConnectorValidationErrorconstructor with the additional required parameter. This ensures test coverage for the updated error handling.Consider renaming the test function from
TestConnectorMetadataErrortoTestConnectorValidationErrorto better reflect what's being tested, maintaining consistency with the implementation changes.-func TestConnectorMetadataError(t *testing.T) { +func TestConnectorValidationError(t *testing.T) {internal/connectors/plugins/public/gocardless/client/payments.go (2)
23-85: Implementation of GetPayments looks solid but has a potential optimizationThe GetPayments function is well-implemented with proper pagination, error handling, and conditional mandate fetching.
Consider optimizing the metadata handling by creating a helper function. The current approach with multiple individual assignments (lines 61-68) is verbose and more prone to maintenance issues if new metadata fields are added:
- payment.Metadata[GocardlessFxMetadataKey] = payment.Fx - payment.Metadata[GocardlessAmountRefundedMetadataKey] = payment.AmountRefunded - payment.Metadata[GocardlessLinksMetadataKey] = payment.Links - payment.Metadata[GocardlessChargeDateMetadataKey] = payment.ChargeDate - payment.Metadata[GocardlessDescriptionMetadataKey] = payment.Description - payment.Metadata[GocardlessFasterAchMetadataKey] = payment.FasterAch - payment.Metadata[GocardlessRetryIfPossibleMetadataKey] = payment.RetryIfPossible - payment.Metadata[GocardlessReferenceMetadataKey] = payment.Reference + // Add all payment metadata fields at once + addPaymentMetadata(payment.Metadata, payment)Where you would define a helper function like:
func addPaymentMetadata(metadata map[string]interface{}, payment gocardless.Payment) { metadataFields := map[string]interface{}{ GocardlessFxMetadataKey: payment.Fx, GocardlessAmountRefundedMetadataKey: payment.AmountRefunded, GocardlessLinksMetadataKey: payment.Links, GocardlessChargeDateMetadataKey: payment.ChargeDate, GocardlessDescriptionMetadataKey: payment.Description, GocardlessFasterAchMetadataKey: payment.FasterAch, GocardlessRetryIfPossibleMetadataKey: payment.RetryIfPossible, GocardlessReferenceMetadataKey: payment.Reference, } for key, value := range metadataFields { metadata[key] = value } }This approach would make it easier to maintain and extend the metadata handling in the future.
48-59: Consider adding error retry or fallback handling for mandate fetchWhen fetching mandate information, the code currently returns immediately if there's an error. For a more resilient implementation, consider adding retry logic or a fallback mechanism.
if c.shouldFetchMandate { mandate, err := c.GetMandate(ctx, payment.Links.Mandate) if err != nil { - return []GocardlessPayment{}, Cursor{}, err + // Log the error but continue processing other payments + c.logger.Error(ctx, "Failed to fetch mandate", "error", err, "mandate_id", payment.Links.Mandate) + // Set default empty values but allow processing to continue + sourceAccountReference = "" + destinationAccountReference = "" + continue } sourceAccountReference = mandate.Links.Creditor destinationAccountReference = mandate.Links.CustomerThis change would allow the function to continue processing other payments even if one mandate fetch fails, improving resilience. However, this approach depends on your error handling requirements - if mandate information is critical, the current approach may be more appropriate.
internal/connectors/plugins/public/gocardless/bank_account_creation.go (1)
10-45: Consider consolidating customer and creditor logic under a single conditional branch.Right now, if both
creditorandcustomerare set – despite validation elsewhere – the function might create two bank accounts sequentially. Although the validation method prevents this scenario, usingelse if(or an explicit return after the first block) might more clearly reflect the one-or-the-other restriction and prevent accidental modifications from introducing a bug in the future.if creditor != "" { externalBankAccount, err = p.client.CreateCreditorBankAccount(ctx, creditor, ba) if err != nil { return models.CreateBankAccountResponse{}, err } -} - -if customer != "" { +} else if customer != "" { externalBankAccount, err = p.client.CreateCustomerBankAccount(ctx, customer, ba) if err != nil { return models.CreateBankAccountResponse{}, err } }internal/connectors/plugins/public/gocardless/README.md (4)
158-158: Add a line break or punctuation to fix the "loose punctuation" note.Currently, there is an unusual punctuation flow. Consider formatting the bullet for clarity:
- - `after`: Points to the last item in the current page. Used to fetch the next page of results. +- **`after`**: Points to the last item in the current page. Used to fetch the next page of results.🧰 Tools
🪛 LanguageTool
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...Cursor-based Pagination**: -after: Points to the last item in the current ...(UNLIKELY_OPENING_PUNCTUATION)
163-163: Add a line break or punctuation to fix the "loose punctuation" note.Similar to the previous bullet item, consider a minor reformat for clarity:
- - `before`: Points to the first item in the current page. Used for backwards pagination if needed. +- **`before`**: Points to the first item in the current page. Used for backwards pagination if needed.🧰 Tools
🪛 LanguageTool
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...art after "CU123" ``` -before: Points to the first item in the current...(UNLIKELY_OPENING_PUNCTUATION)
171-171: Add punctuation to clarify the phrase "lastCreationDate".This bullet point can be clarified, matching the style of the others:
- - `lastCreationDate`: Timestamp of the most recently processed item +- **`lastCreationDate`**: Timestamp of the most recently processed item.🧰 Tools
🪛 LanguageTool
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ...ate Tracking**: -lastCreationDate: Timestamp of the most recently processe...(UNLIKELY_OPENING_PUNCTUATION)
257-257: Add a comma after "Currently" for conventional usage.A comma after "Currently" helps match standard grammar conventions:
- - Currently not supported, Gocardless does not support creating webhook from API + - Currently, not supported. GoCardless does not support creating webhook from the API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~257-~257: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
internal/connectors/plugins/public/gocardless/client/bank_account_creation.go (1)
13-103: Reduce duplication between CreateCreditorBankAccount and CreateCustomerBankAccount.These methods share a very similar flow (constructing create params, parsing times, building responses). Consider extracting shared logic into a helper function to increase maintainability and reduce potential drift between the two implementations.
func (c *client) CreateCreditorBankAccount(ctx context.Context, creditor string, ba models.BankAccount) (GocardlessGenericAccount, error) { // ... payload := gocardless.CreditorBankAccountCreateParams{ - AccountHolderName: ba.Name, - AccountNumber: *ba.AccountNumber, - AccountType: accountType, - CountryCode: *ba.Country, - Currency: currency, - Links: gocardless.CreditorBankAccountCreateParamsLinks{Creditor: creditor}, - SetAsDefaultPayoutAccount: setAsDefaultPayoutAccount, - } + // Build payload using a common function that sets fields + // based on whether it's creditor or customer. + // ... } // ... } func (c *client) CreateCustomerBankAccount(ctx context.Context, customer string, ba models.BankAccount) (GocardlessGenericAccount, error) { // ... }internal/connectors/plugins/public/gocardless/payments.go (2)
50-52: Clarify cursor vs. payment reference distinction.
When there are no more pages (!hasMore) but you still have gathered payments, you setnewState.Afterto the last payment’sReference. Verify that the external system recognizes that reference as a valid next position in pagination. If the external paging token differs from a payment reference, this could lead to confusion or incorrect future calls.
100-111: Map additional statuses as needed.
mapPaymentStatusis straightforward and covers “pending,” “paid,” and “failed.” For safety, check whether there are other GoCardless statuses like “cancelled” or “customer_approval_granted” that might need to map to an internal state.internal/connectors/plugins/public/gocardless/plugin_test.go (1)
81-87: Uninstall flow coverage.
The test (lines 81–87) confirms that uninstall returns a valid response. Although the uninstall logic is straightforward, consider adding negative or edge-case tests (e.g., repeated uninstalls or uninstalls on a non-installed plugin) to ensure consistent error handling.internal/connectors/plugins/public/gocardless/client/external_accounts.go (1)
19-32: Consider an else-case for undetected owner prefix.
Currently, ifownerIDdoesn’t start with"CR"or"CU", the function offers no error and returns empty results. If that scenario is unexpected, returning an error or logging the unsupported prefix would ensure you don’t silently discard valid IDs.internal/connectors/plugins/public/gocardless/users.go (1)
73-109: Consider unifying repeated pagination logic in getCreditorsUsers and getCustomersUsers.Both functions follow nearly identical patterns for fetching data, updating cursors, and truncating results. Extracting the shared pagination logic into a helper function can reduce duplication, simplify maintenance, and make it easier to implement future enhancements.
Also applies to: 111-145
internal/connectors/plugins/public/gocardless/plugin.go (4)
22-38: Possible duplication or overlap in Swift/BIC-related error messages.
The error constantsErrMissingSwiftBicCodeandErrMissingSwiftCodeboth appear to refer to the same or very similar validation condition. Consider consolidating them into a single consistent error message to avoid confusion.- ErrMissingSwiftCode = fmt.Errorf("field swiftBicCode is required for US bank accounts") - ErrMissingSwiftBicCode = fmt.Errorf("swift bic code is required") + ErrMissingSwiftBicCode = fmt.Errorf("swift BIC code is required for US bank accounts")
79-84: Unused parameter in Install method.
InInstall, thereqparameter is never used. If that’s intentional, consider documenting its purpose or omitting it to reduce confusion.
85-95: Repeated nil check for p.client.
Multiple methods have identical checks forp.client == nil. If you plan to keep these checks, consider extracting them to a common helper function to centralize the logic and improve maintainability.+func (p *Plugin) ensureClientInitialized() error { + if p.client == nil { + return plugins.ErrNotYetInstalled + } + return nil +}Also applies to: 103-111, 113-119, 121-129, 131-137
75-78: Optional: Add debug or info logging to Install method.
Currently, theInstallmethod runs silently. Adding at least an info log here helps track plugin lifecycle events.func (p *Plugin) Install(_ context.Context, req models.InstallRequest) (models.InstallResponse, error) { + p.logger.Info("Installing GoCardless plugin", logging.String("pluginName", p.name)) return models.InstallResponse{ Workflow: Workflow(), }, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (32)
internal/connectors/engine/activities/errors.go(1 hunks)internal/connectors/engine/activities/plugin_create_bank_account_test.go(1 hunks)internal/connectors/plugins/public/gocardless/README.md(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/gocardless/capabilities.go(1 hunks)internal/connectors/plugins/public/gocardless/client/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client_generated.go(1 hunks)internal/connectors/plugins/public/gocardless/client/creditors.go(1 hunks)internal/connectors/plugins/public/gocardless/client/customers.go(1 hunks)internal/connectors/plugins/public/gocardless/client/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/client/mandate.go(1 hunks)internal/connectors/plugins/public/gocardless/client/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/client/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/config.go(1 hunks)internal/connectors/plugins/public/gocardless/currencies.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/gocardless/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/payments_test.go(1 hunks)internal/connectors/plugins/public/gocardless/plugin.go(1 hunks)internal/connectors/plugins/public/gocardless/plugin_test.go(1 hunks)internal/connectors/plugins/public/gocardless/users.go(1 hunks)internal/connectors/plugins/public/gocardless/users_test.go(1 hunks)internal/connectors/plugins/public/gocardless/utils.go(1 hunks)internal/connectors/plugins/public/gocardless/workflow.go(1 hunks)internal/connectors/plugins/public/list.go(1 hunks)internal/connectors/plugins/public/mangopay/bank_account_creation.go(1 hunks)internal/models/errors.go(1 hunks)internal/models/errors_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/connectors/plugins/public/list.go
- internal/connectors/plugins/public/gocardless/currencies.go
- internal/connectors/plugins/public/gocardless/capabilities.go
- internal/connectors/plugins/public/gocardless/metadata.go
- internal/connectors/plugins/public/gocardless/client/mandate.go
- internal/connectors/plugins/public/gocardless/workflow.go
- internal/connectors/plugins/public/gocardless/config.go
- internal/connectors/plugins/public/gocardless/external_accounts_test.go
- internal/connectors/plugins/public/gocardless/bank_account_creation_test.go
- internal/connectors/plugins/public/gocardless/payments_test.go
- internal/connectors/plugins/public/gocardless/client/client.go
- internal/connectors/plugins/public/gocardless/users_test.go
🧰 Additional context used
🪛 LanguageTool
internal/connectors/plugins/public/gocardless/README.md
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...Cursor-based Pagination**: - after: Points to the last item in the current ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...art after "CU123" ``` - before: Points to the first item in the current...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ...ate Tracking**: - lastCreationDate: Timestamp of the most recently processe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~257-~257: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
🔇 Additional comments (25)
internal/models/errors.go (5)
15-16: New error variable added for more specific error handlingThe addition of
ErrMissingConnectorFieldprovides more granular error reporting for validation issues. This complements the existingErrMissingConnectorMetadataerror, allowing the system to distinguish between different types of field validation failures.
19-22: Renamed type for improved semanticsRenaming from
ConnectorMetadataErrortoConnectorValidationErrorbetter reflects the purpose of this error type, which is to represent validation errors rather than just metadata-related errors. This change improves code clarity.
24-25: Good addition of NonRetryableError type for error handlingThe new
NonRetryableErrorvariable will help the system identify validation errors that should not trigger retry mechanisms, improving error handling efficiency in distributed operations.
26-31: Enhanced error constructor with additional contextThe updated constructor now accepts an additional
errparameter, allowing for more detailed error messages that include the specific validation error encountered. This change improves error context and debugging.
33-35: Updated methods to match the new type nameThe
Error()andUnwrap()methods have been properly updated to reference the new type. These changes maintain consistent error handling behavior while benefiting from the improved naming.internal/connectors/engine/activities/errors.go (1)
44-45: Appropriate handling for non-retryable validation errorsThis addition enhances the error handling system by explicitly checking for
models.NonRetryableErrorusingerrors.As(). By marking these errors as non-retryable in the Temporal workflow system, you prevent unnecessary retry attempts for validation failures that cannot be resolved without intervention.internal/connectors/plugins/public/mangopay/bank_account_creation.go (1)
17-17: Updated error handling to use ValidationErrorThe error handling has been appropriately updated to use
NewConnectorValidationErrorinstead of the deprecatedNewConnectorMetadataError. This change provides more context about the validation error by including the underlyingErrMissingConnectorMetadataerror.internal/connectors/engine/activities/plugin_create_bank_account_test.go (1)
94-94: Updated error handling to use new validation error typeThe test case now properly reflects the updated error handling model, switching from the previous
ConnectorMetadataErrorto the more specificConnectorValidationErrorwith the additional error parameter. This change aligns with the broader error handling refactoring in the codebase.internal/connectors/plugins/public/gocardless/client/creditors.go (1)
12-51: Implementation of GetCreditors looks goodThe function correctly implements the retrieval of creditors from the GoCardless API with proper pagination handling, error management, and data transformation.
A few notable strengths in this implementation:
- Proper context value setting for metrics
- Comprehensive error handling, including timestamp parsing errors
- Complete mapping of creditor fields to the GocardlessUser struct
- Proper cursor handling for pagination
internal/connectors/plugins/public/gocardless/client/customers.go (1)
12-59: Implementation of GetCustomers looks goodThe function correctly implements the retrieval of customers from the GoCardless API with proper pagination, error handling, and data transformation.
The implementation has good attention to detail with:
- Proper context value setting for metrics
- Comprehensive error handling including timestamp parsing errors
- Complete mapping of customer fields to the GocardlessUser struct
- Name composition from given and family names
- Proper cursor handling for pagination
internal/connectors/plugins/public/gocardless/client/payments.go (1)
12-21: Well-structured GocardlessPayment type definitionThe payment struct is well-defined with appropriate fields for identifying and tracking payments, including account references, amount, status, and metadata.
internal/connectors/plugins/public/gocardless/bank_account_creation.go (1)
1-9: No concerns with package and imports.This setup for the package and imports looks correctly organized and aligned with Go best practices.
internal/connectors/plugins/public/gocardless/utils.go (2)
15-79: Validation logic is robust and consistent.The explicit checks for pointer fields (e.g.,
accountNumber,country) and currency constraints (e.g., US requiringswiftBicCodeand account type) help ensure data integrity. This approach prevents runtime errors and clarifies domain rules.
81-130: No issues identified in data transformation.The
externalAccountFromGocardlessDataandextractExternalAccountMetadatafunctions are clearly structured and handle nested or complex metadata gracefully.internal/connectors/plugins/public/gocardless/payments.go (2)
17-23: Check for potential empty or nil state usage.
While the unmarshalling ofreq.StateintooldState(lines 20–23) seems correct, consider adding handling for scenarios wherereq.Statemight be empty or improperly formatted. Although the code properly returns an error if unmarshalling fails, auditing related upstream or downstream code to ensurereq.Stateis never inadvertently passed as an empty or nil without reason would improve reliability.
70-98: Ensure all possible payment attributes are handled.
ThefillPaymentsfunction (lines 70–98) marshals the fetchedGocardlessPaymentintoPSPPaymentnicely. However, confirm that all fields from the GoCardless struct that could matter in subsequent processing (e.g., adjustments or fees) are captured. If new fields are introduced in the upstream API later, they may also need to be mapped.internal/connectors/plugins/public/gocardless/plugin_test.go (2)
35-57: Extensive config validation testing.
The tests in lines 35–57 effectively verify how invalid configurations fail. This thorough coverage (e.g., missingaccessToken, missingendpoint) ensures that plugin users are forced to provide complete and correct config data. Nicely done.
271-286: Validate added currencies.
It’s good that you verify the supported currenciesAUD,CAD,DKK,EUR,GBP,NZD,SEK, andUSD(lines 271–286). Should your connector expand to other regions, ensure that the test updates accordingly and that new currency codes are recognized properly.internal/connectors/plugins/public/gocardless/client/external_accounts.go (1)
19-25: Add a length check forownerIDbefore slicing.
This code repeats a previously reported issue: slicing the first two characters ofownerID(lines 19, 23) without validating its length can cause a runtime panic ifownerIDis shorter than two characters.if len(ownerID) < 2 { return []GocardlessGenericAccount{}, Cursor{}, fmt.Errorf("invalid ownerID: must be at least 2 characters") }internal/connectors/plugins/public/gocardless/users.go (1)
104-106: Good job: The slice truncation approach is safe.By checking
len(users) > pageSizebefore slicing, you avoid out-of-bounds errors. This addresses concerns raised in older revisions and demonstrates careful consideration of slice boundaries.Also applies to: 140-142
internal/connectors/plugins/public/gocardless/external_accounts.go (2)
45-49: String prefix validation looks correct.The check ensures the ID starts with "CR" (creditor) or "CU" (customer), aligning with domain requirements. Including this early validation prevents misuse and helps keep the data consistent.
78-80: Safe slice truncation approach.Your condition
if len(externalBankAccounts) > req.PageSizeavoids out-of-bounds errors when slicing. Good job on handling the slice in a defensive manner.internal/connectors/plugins/public/gocardless/client/metadata.go (1)
3-19: Well-defined namespace constants.These metadata keys are neatly organized and consistently apply the GoCardless-specific namespace. This clear structure promotes maintainable code for referencing metadata attributes.
internal/connectors/plugins/public/gocardless/plugin.go (1)
85-203: Implementation placeholders returning NotImplemented.
Many plugin operations (FetchNextAccounts,FetchNextBalances,CreateTransfer, etc.) currently returnplugins.ErrNotImplemented. If these methods are placeholders, please confirm or document your plan for future implementation. Otherwise, consider removing them or implementing partial functionality to avoid confusion.internal/connectors/plugins/public/gocardless/client/client_generated.go (1)
1-349: Auto-generated file by MockGen; minimal manual changes recommended.
As this is an auto-generated mock, most structural or naming decisions are dictated by the mocking framework. The current content appears sufficient and correct for testing purposes.
gitstart-app
bot
force-pushed
the
LNK-38
branch
from
7322764 to
46b0f7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/connectors/plugins/public/gocardless/users.go (2)
104-106:⚠️ Potential issuePrevent potential out-of-bounds slice error.
When the number of users is less than the requested page size, the slicing operation
users = users[:pageSize]will cause a runtime panic. This check should include a bounds check.- if len(users) > pageSize { - users = users[:pageSize] + if len(users) >= pageSize { + users = users[:pageSize]
140-142:⚠️ Potential issueSame out-of-bounds slice risk for customer users.
This slice operation has the same potential panic risk if
len(users) < pageSize. Apply the same fix as for the creditors users.- if len(users) > pageSize { - users = users[:pageSize] + if len(users) >= pageSize { + users = users[:pageSize]
🧹 Nitpick comments (13)
internal/connectors/plugins/public/gocardless/README.md (5)
58-68: Redundant bank account creation example.The first bank account creation example (lines 58-68) appears to be incomplete or redundant compared to the more comprehensive example that follows (lines 69-81). Consider removing the first example to avoid confusion.
24-24: Consider using a boolean instead of string forshouldFetchMandate.The configuration uses strings "true" or "false" for what appears to be a boolean value. Consider using actual boolean values in the JSON configuration to improve type safety.
156-167: Fix formatting in the pagination explanation section.There are loose punctuation marks and spacing issues that affect readability. Additionally, the preposition "in" might be better replaced with "on" in some contexts.
- - `after`: Points to the last item in the current page. Used to fetch the next page of results. + - `after`: Points to the last item on the current page. Used to fetch the next page of results. ```json // Example: Current page ends with item "CU123" { "after": "CU123" } // Next request will start after "CU123" ``` - - `before`: Points to the first item in the current page. Used for backwards pagination if needed. + - `before`: Points to the first item on the current page. Used for backwards pagination if needed. ```json // Example: Current page starts with item "CU789" { "before": "CU789" } // Previous page will end at "CU789" ```🧰 Tools
🪛 LanguageTool
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...Cursor-based Pagination**: -after: Points to the last item in the current ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... -after: Points to the last item in the current page. Used to fetch the nex...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...art after "CU123" ``` -before: Points to the first item in the current...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... -before: Points to the first item in the current page. Used for backwards pa...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
257-257: Add missing comma after the adverb.A comma is missing after the conjunctive adverb "Currently".
- - Currently not supported, Gocardless does not support creating webhook from API + - Currently, not supported, Gocardless does not support creating webhook from API🧰 Tools
🪛 LanguageTool
[uncategorized] ~257-~257: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
79-79: Clarify account_type requirements for currencies other than USD.The documentation states that account_type is required for US accounts, but doesn't clearly explain whether it should be omitted for non-US accounts. The code in utils.go (lines 73-76) suggests that account_type should be empty for non-USD currencies.
"com.gocardless.spec/currency": "USD", "com.gocardless.spec/creditor": "CR123", // For creditor bank account // OR "com.gocardless.spec/customer": "CU123", // For customer bank account - "com.gocardless.spec/account_type": "savings" // Required for US accounts + "com.gocardless.spec/account_type": "savings" // Required for US accounts, must be omitted for non-USD currenciesinternal/connectors/plugins/public/gocardless/utils.go (3)
39-41: Improve creditor ID validation.The current check may not properly validate very short creditor IDs. If the length is exactly 1, it will pass this validation but might cause issues later.
- if len(creditor) > 1 && creditor[:2] != "CR" { + if len(creditor) > 0 && (len(creditor) < 2 || creditor[:2] != "CR") {
43-45: Improve customer ID validation.Similar to the creditor ID validation, the customer ID validation may not properly validate very short IDs.
- if len(customer) > 1 && customer[:2] != "CU" { + if len(customer) > 0 && (len(customer) < 2 || customer[:2] != "CU") {
111-116: Add error handling for JSON marshaling failures.If JSON marshaling fails, the string value will not be set, potentially leading to data loss. Consider adding proper error handling or logging.
jsonBytes, err := json.Marshal(v) if err == nil { stringValue = string(jsonBytes) +} else { + stringValue = fmt.Sprintf("Error marshaling value: %v", err) }internal/connectors/plugins/public/gocardless/client/customers.go (1)
36-37: Improve name concatenation handling.The current implementation concatenates given name and family name without checking if either is empty, which could result in names with leading or trailing spaces.
-Name: customer.GivenName + " " + customer.FamilyName, +Name: strings.TrimSpace(customer.GivenName + " " + customer.FamilyName),Don't forget to add the required import:
import ( "context" "fmt" "time" + "strings" "github.com/formancehq/payments/internal/connectors/metrics" gocardless "github.com/gocardless/gocardless-pro-go/v4" )internal/connectors/plugins/public/gocardless/plugin_test.go (1)
35-40: Unifying negative test cases could improve maintainability.
You have several negative test scenarios that validate incorrect configurations (lines 35-40, 42-46, 48-52). Consider using Ginkgo’sDescribeTableto unify these scenarios, reducing repetitive code and enhancing readability.internal/connectors/plugins/public/gocardless/client/client.go (2)
34-40: Potential concurrency note.
The unexportedclientstruct holds references to service objects and config data. If you plan to share a singleclientinstance across multiple goroutines, confirm that the underlyingGoCardlessServiceis thread-safe. Otherwise, you may need synchronization.
151-153: Method name suggests returning a new instance, but it mutates the existing client.
NewWithServicedoes not create a new client but sets the service on the existing struct. Consider renaming it to clarify that it updates the current client object.internal/connectors/plugins/public/gocardless/external_accounts.go (1)
45-49: Improve error message clarity for ID validation.The error message could be more helpful by specifying exactly what format is expected for the ID.
- return models.FetchNextExternalAccountsResponse{}, fmt.Errorf( - "ownerId field must start with 'CR' for creditor account or 'CU' customer account", - ) + return models.FetchNextExternalAccountsResponse{}, fmt.Errorf( + "ID field must start with 'CR' for creditor account or 'CU' for customer account - got %s", from.ID, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumopenapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (32)
internal/connectors/engine/activities/errors.go(1 hunks)internal/connectors/engine/activities/plugin_create_bank_account_test.go(1 hunks)internal/connectors/plugins/public/gocardless/README.md(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/gocardless/capabilities.go(1 hunks)internal/connectors/plugins/public/gocardless/client/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client_generated.go(1 hunks)internal/connectors/plugins/public/gocardless/client/creditors.go(1 hunks)internal/connectors/plugins/public/gocardless/client/customers.go(1 hunks)internal/connectors/plugins/public/gocardless/client/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/client/mandate.go(1 hunks)internal/connectors/plugins/public/gocardless/client/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/client/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/config.go(1 hunks)internal/connectors/plugins/public/gocardless/currencies.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/gocardless/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/payments_test.go(1 hunks)internal/connectors/plugins/public/gocardless/plugin.go(1 hunks)internal/connectors/plugins/public/gocardless/plugin_test.go(1 hunks)internal/connectors/plugins/public/gocardless/users.go(1 hunks)internal/connectors/plugins/public/gocardless/users_test.go(1 hunks)internal/connectors/plugins/public/gocardless/utils.go(1 hunks)internal/connectors/plugins/public/gocardless/workflow.go(1 hunks)internal/connectors/plugins/public/list.go(1 hunks)internal/connectors/plugins/public/mangopay/bank_account_creation.go(1 hunks)internal/models/errors.go(1 hunks)internal/models/errors_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- internal/connectors/plugins/public/gocardless/client/mandate.go
- internal/connectors/engine/activities/errors.go
- internal/connectors/plugins/public/gocardless/workflow.go
- internal/connectors/engine/activities/plugin_create_bank_account_test.go
- internal/connectors/plugins/public/list.go
- internal/connectors/plugins/public/gocardless/currencies.go
- internal/connectors/plugins/public/mangopay/bank_account_creation.go
- internal/models/errors_test.go
- internal/connectors/plugins/public/gocardless/client/creditors.go
- internal/connectors/plugins/public/gocardless/capabilities.go
- internal/connectors/plugins/public/gocardless/metadata.go
- internal/connectors/plugins/public/gocardless/bank_account_creation.go
- internal/connectors/plugins/public/gocardless/users_test.go
- internal/connectors/plugins/public/gocardless/payments_test.go
- internal/connectors/plugins/public/gocardless/payments.go
- internal/connectors/plugins/public/gocardless/bank_account_creation_test.go
- internal/connectors/plugins/public/gocardless/client/payments.go
- internal/connectors/plugins/public/gocardless/external_accounts_test.go
- internal/connectors/plugins/public/gocardless/config.go
- internal/models/errors.go
- internal/connectors/plugins/public/gocardless/client/metadata.go
🧰 Additional context used
🧬 Code Definitions (9)
internal/connectors/plugins/public/gocardless/client/bank_account_creation.go (4)
internal/connectors/plugins/public/gocardless/client/external_accounts.go (4) (4)
c(12-32)c(34-67)c(69-101)err(17-17)internal/connectors/plugins/public/gocardless/client/client.go (3) (3)
c(151-153)client(34-40)GocardlessGenericAccount(46-53)internal/connectors/plugins/public/gocardless/client/mandate.go (1) (1)
c(9-18)internal/connectors/plugins/public/gocardless/client/metadata.go (2) (2)
GocardlessCurrencyMetadataKey(6-6)GocardlessAccountTypeMetadataKey(9-9)
internal/connectors/plugins/public/gocardless/plugin_test.go (4)
internal/connectors/plugins/public/gocardless/plugin.go (3) (3)
Plugin(46-51)_(203-203)New(53-73)internal/connectors/plugins/public/gocardless/workflow.go (1) (1)
Workflow(11-32)internal/connectors/plugins/public/gocardless/currencies.go (1) (1)
SupportedCurrenciesWithDecimal(6-15)internal/connectors/plugins/public/gocardless/capabilities.go (1) (1)
Capabilities(5-11)
internal/connectors/plugins/public/gocardless/external_accounts.go (3)
internal/connectors/plugins/public/gocardless/client/external_accounts.go (1) (1)
err(17-17)internal/models/errors.go (1) (1)
ErrMissingFromPayloadInRequest(11-11)internal/connectors/plugins/public/gocardless/utils.go (1) (1)
externalAccountFromGocardlessData(81-102)
internal/connectors/plugins/public/gocardless/client/external_accounts.go (4)
internal/connectors/plugins/public/gocardless/client/creditors.go (1) (1)
c(12-51)internal/connectors/plugins/public/gocardless/client/customers.go (1) (1)
c(12-59)internal/connectors/plugins/public/gocardless/client/client.go (4) (4)
c(151-153)client(34-40)GocardlessGenericAccount(46-53)Cursor(42-44)internal/connectors/plugins/public/gocardless/client/mandate.go (1) (1)
c(9-18)
internal/connectors/plugins/public/gocardless/utils.go (3)
internal/models/errors.go (1) (1)
NewConnectorValidationError(26-31)internal/connectors/plugins/public/gocardless/client/metadata.go (5) (5)
GocardlessCurrencyMetadataKey(6-6)GocardlessCreditorMetadataKey(7-7)GocardlessCustomerMetadataKey(8-8)GocardlessAccountTypeMetadataKey(9-9)GocardlessMetadataSpecNamespace(4-4)internal/connectors/plugins/public/gocardless/currencies.go (1) (1)
SupportedCurrenciesWithDecimal(6-15)
internal/connectors/plugins/public/gocardless/client/client.go (6)
internal/connectors/plugins/public/gocardless/client/payments.go (2) (2)
GocardlessPayment(12-21)c(23-85)internal/connectors/plugins/public/gocardless/client/external_accounts.go (4) (4)
err(17-17)c(12-32)c(34-67)c(69-101)internal/connectors/plugins/public/gocardless/client/bank_account_creation.go (2) (2)
c(13-59)c(61-103)internal/connectors/plugins/public/gocardless/client/creditors.go (1) (1)
c(12-51)internal/connectors/plugins/public/gocardless/client/customers.go (1) (1)
c(12-59)internal/connectors/plugins/public/gocardless/client/mandate.go (1) (1)
c(9-18)
internal/connectors/plugins/public/gocardless/users.go (4)
internal/connectors/plugins/public/gocardless/plugin.go (14) (14)
p(75-77)p(79-83)p(85-87)p(89-94)p(96-101)p(103-111)p(113-119)p(121-129)p(131-137)p(139-145)p(147-153)p(155-161)Plugin(46-51)_(203-203)internal/connectors/plugins/public/gocardless/client/external_accounts.go (2) (2)
err(17-17)nextCursor(15-15)internal/connectors/plugins/public/gocardless/client/client.go (2) (2)
client(34-40)GocardlessUser(55-83)internal/connectors/plugins/public/gocardless/users_test.go (1) (1)
_(18-385)
internal/connectors/plugins/public/gocardless/client/customers.go (6)
internal/connectors/plugins/public/gocardless/client/bank_account_creation.go (2) (2)
c(13-59)c(61-103)internal/connectors/plugins/public/gocardless/client/creditors.go (1) (1)
c(12-51)internal/connectors/plugins/public/gocardless/client/external_accounts.go (5) (5)
c(12-32)c(34-67)c(69-101)err(17-17)nextCursor(15-15)internal/connectors/plugins/public/gocardless/client/client.go (4) (4)
c(151-153)client(34-40)GocardlessUser(55-83)Cursor(42-44)internal/connectors/plugins/public/gocardless/client/payments.go (1) (1)
c(23-85)internal/connectors/plugins/public/gocardless/plugin.go (1) (1)
_(203-203)
internal/connectors/plugins/public/gocardless/client/client_generated.go (1)
internal/connectors/plugins/public/gocardless/client/client.go (3) (3)
GocardlessGenericAccount(46-53)GocardlessUser(55-83)GoCardlessService(23-32)
🪛 LanguageTool
internal/connectors/plugins/public/gocardless/README.md
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...Cursor-based Pagination**: - after: Points to the last item in the current ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... - after: Points to the last item in the current page. Used to fetch the nex...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...art after "CU123" ``` - before: Points to the first item in the current...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... - before: Points to the first item in the current page. Used for backwards pa...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ...ate Tracking**: - lastCreationDate: Timestamp of the most recently processe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~257-~257: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (25)
internal/connectors/plugins/public/gocardless/README.md (3)
41-50: Excellent currency support documentation.The supported currencies section is clear and comprehensive, matching the implementation in the code. This will be very helpful for users of the connector.
224-252: Comprehensive error handling documentation.The error handling section provides detailed information about common error scenarios. This will greatly assist users in troubleshooting issues.
127-141: Well-structured pagination and data transformation documentation.The implementation notes section clearly explains how the connector handles pagination, data transformation, and ordering of results. This will help users understand the behavior of the connector.
internal/connectors/plugins/public/gocardless/utils.go (2)
104-130: Properly handle complex metadata.The implementation now correctly handles nested metadata structures by marshaling maps, slices, and arrays to JSON strings. This addresses a previous review comment and improves the robustness of the metadata handling.
15-79: Comprehensive bank account validation.The
validateExternalBankAccountfunction provides thorough validation of bank account data, including country-specific requirements. This ensures that only valid bank accounts are created, reducing errors when interacting with the GoCardless API.internal/connectors/plugins/public/gocardless/client/customers.go (2)
12-59: Well-implemented customer fetching with pagination.The
GetCustomersfunction properly handles retrieval of customers from GoCardless, including pagination, error handling, and data transformation. The implementation is consistent with theGetCreditorsfunction and follows good practices for error handling.
28-31: Robust time parsing with error handling.The implementation correctly parses the creation time using RFC3339Nano format and returns a descriptive error message if parsing fails, which will help with debugging.
internal/connectors/plugins/public/gocardless/client/bank_account_creation.go (3)
32-36: Appropriate handling of country-specific bank codes.The implementation correctly handles the different bank code requirements for US vs. non-US accounts, using BankCode for US accounts and BranchCode for others. This aligns with GoCardless API requirements.
13-59: Properly implemented creditor bank account creation.The
CreateCreditorBankAccountfunction correctly handles the creation of a bank account for a creditor, including proper error handling and conversion of the response to aGocardlessGenericAccount. This implementation addresses previous review comments about nil pointer dereferences and missing validation.
61-102: Properly implemented customer bank account creation.The
CreateCustomerBankAccountfunction correctly handles the creation of a bank account for a customer, with functionality similar to the creditor version. Both functions follow consistent patterns for error handling and response processing.internal/connectors/plugins/public/gocardless/plugin_test.go (4)
54-63: Good validation of workflow correctness.
These lines confirm that the returned workflow is not empty and matches the plugin’s expected workflow definition. This test ensures the install flow is correct.
98-190: Thorough checks for uninstalled plugin state.
Each function that’s expected to fail before installation is appropriately tested, ensuring robust error handling. This comprehensive coverage reduces the risk of regressions.
192-269: Well-structured tests for unimplemented methods.
Verifying that unimplemented methods returnErrNotImplementedclarifies expected behavior and helps maintain accurate plugin capabilities.
290-307: Capabilities test is consistent with defined features.
Ensuring that theCapabilitiesmatch the expected set in line withmodels.Capabilityis excellent for preventing accidental changes to functionality.internal/connectors/plugins/public/gocardless/client/external_accounts.go (3)
12-32: Risk of slicing a short string without length checks.
IfownerIDis shorter than two characters,ownerID[:2]in lines 19 and 23 will result in a runtime panic. This issue was flagged in a previous review.Apply a length check before slicing, similar to this:
+ if len(ownerID) < 2 { + return []GocardlessGenericAccount{}, Cursor{}, fmt.Errorf("ownerID must be at least 2 chars") + } if ownerID[:2] == "CR" {
34-67: Good usage of typed list params and pagination.
The code properly leverages the typedCreditorBankAccountListParamsfrom the GoCardless SDK. TheCursorstruct is returned for pagination, enabling reliable iteration over large sets of results.
69-101: Consistent approach for customer vs. creditor accounts.
Using distinct methods for creditor (getCreditorExternalAccounts) and customer (getCustomerExternalAccounts) external accounts strengthens clarity. The structure parallels the approach used for creditors/customers, promoting uniformity.internal/connectors/plugins/public/gocardless/client/client.go (3)
11-21: Interface design is concise and future-proof.
TheClientinterface enumerates distinct methods for all required operations, clearly partitioning responsibilities. This design makes the code extensible and testable.
91-121: Service wrapper maintains consistency.
Each method delegates to the underlying GoCardless SDK, preserving a clean abstraction layer. The approach is consistent and straightforward to mock.
123-149: Flexible initialization with sandbox default.
Defaulting to the sandbox endpoint if none is provided is convenient and aligns with best practices. Good error handling ensures the config is valid before returning.internal/connectors/plugins/public/gocardless/external_accounts.go (1)
78-80:⚠️ Potential issuePrevent potential out-of-bounds slice error.
Similar to issues in the users.go file, the slice operation
externalBankAccounts = externalBankAccounts[:req.PageSize]will panic if the length of externalBankAccounts is less than req.PageSize.- if len(externalBankAccounts) > req.PageSize { - externalBankAccounts = externalBankAccounts[:req.PageSize] + if len(externalBankAccounts) >= req.PageSize { + externalBankAccounts = externalBankAccounts[:req.PageSize]Likely an incorrect or invalid review comment.
internal/connectors/plugins/public/gocardless/plugin.go (3)
15-38: Well-structured error definitions with descriptive messages.The error constants are clearly named and provide descriptive messages. This approach enhances debuggability and error handling.
40-44: Clean plugin registration with proper separation of concerns.The plugin is registered correctly, and the factory function pattern with dependency injection is well-implemented.
121-129: Correct implementation of FetchNextOthers function.The function properly checks for client initialization and delegates to the fetchNextUsers function. Good error handling pattern.
internal/connectors/plugins/public/gocardless/client/client_generated.go (1)
1-349: Auto-generated mock file looks correct.This auto-generated file contains the expected mock implementations for the Client and GoCardlessService interfaces. It will be valuable for testing the GoCardless connector.
| currency := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessCurrencyMetadataKey) | ||
| accountType := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessAccountTypeMetadataKey) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing metadata.
The code extracts currency and account type from metadata without checking if the extraction was successful. If these values are missing, it might lead to unexpected behavior.
currency := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessCurrencyMetadataKey)
+ if currency == "" {
+ return GocardlessGenericAccount{}, fmt.Errorf("missing required metadata: %s", GocardlessCurrencyMetadataKey)
+ }
accountType := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessAccountTypeMetadataKey)Similar check should be applied to the CreateCustomerBankAccount function.
📝 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.
| currency := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessCurrencyMetadataKey) | |
| accountType := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessAccountTypeMetadataKey) | |
| currency := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessCurrencyMetadataKey) | |
| if currency == "" { | |
| return GocardlessGenericAccount{}, fmt.Errorf("missing required metadata: %s", GocardlessCurrencyMetadataKey) | |
| } | |
| accountType := models.ExtractNamespacedMetadata(ba.Metadata, GocardlessAccountTypeMetadataKey) |
There was a problem hiding this comment.
Currency is not required when creating bank accounts.
There was a problem hiding this comment.
@gitstart-formance Thank you for the clarification. I understand now that currency is not a required field when creating bank accounts in the GoCardless connector. I'll make note of this for future reviews.
---
✏️ Learnings added
Learnt from: gitstart-formance
PR: formancehq/payments#326
File: internal/connectors/plugins/public/gocardless/client/bank_account_creation.go:19-20
Timestamp: 2025-03-21T21:40:49.395Z
Learning: Currency is not required when creating bank accounts in the GoCardless connector.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/connectors/plugins/public/gocardless/client/external_accounts.go (1)
12-18: Prevent potential panic when slicingownerID.Using
ownerID[:2]can trigger a runtime panic ifownerIDis shorter than 2 characters. This was previously flagged in an older commit. Reintroduce a check to ensure the length is sufficient.func (c *client) GetExternalAccounts(ctx context.Context, ownerID string, pageSize int, after string) ([]GocardlessGenericAccount, Cursor, error) { + if len(ownerID) < 2 { + return []GocardlessGenericAccount{}, Cursor{}, fmt.Errorf("invalid ownerID, must be at least 2 characters: %s", ownerID) + } if ownerID[:2] == "CR" { // ...
🧹 Nitpick comments (6)
docs/api/README.md (1)
5021-5282: Configuration schema looks good but could benefit from an explicit description forshouldFetchMandateThe GoCardless configuration schema is well-defined with clear field definitions. However, the description for
shouldFetchMandatecould be more explicit about what type of value it expects.Consider updating the description for
shouldFetchMandateto specify that it should be a string with either "true" or "false" values, to match the implementation and the explanation in the connector's README:-|shouldFetchMandate|string|true|none|none| +|shouldFetchMandate|string|true|none|Boolean as string ("true" or "false") indicating whether mandates should be fetched|🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5077-5077: Multiple headings with the same content
null(MD024, no-duplicate-heading)
5111-5111: Multiple headings with the same content
null(MD024, no-duplicate-heading)
5146-5146: Multiple headings with the same content
null(MD024, no-duplicate-heading)
5181-5181: Multiple headings with the same content
null(MD024, no-duplicate-heading)
5211-5211: Multiple headings with the same content
null(MD024, no-duplicate-heading)
5240-5240: Multiple headings with the same content
null(MD024, no-duplicate-heading)
5271-5271: Multiple headings with the same content
null(MD024, no-duplicate-heading)
internal/connectors/plugins/public/gocardless/README.md (1)
59-81: Duplicate JSON example in bank account creation sectionThere appears to be a duplicate JSON example in the bank account creation section (lines 59-69 and 70-81) that could confuse users.
Remove the first example and keep only the more comprehensive second example, or clearly label them as separate examples for different use cases.
internal/connectors/plugins/public/gocardless/client/external_accounts.go (1)
19-25: Handle unknownownerIDprefixes gracefully.Currently, if
ownerIDdoes not start with"CR"or"CU", the function returns no error and empty data. Consider refactoring to return a clear error or handle additional prefixes.if ownerID[:2] == "CR" { // ... } if ownerID[:2] == "CU" { // ... } +if len(bankAccounts) == 0 && err == nil { + return []GocardlessGenericAccount{}, Cursor{}, + fmt.Errorf("unknown ownerID prefix, must start with either CR or CU: %s", ownerID) +}internal/connectors/plugins/public/gocardless/client/client.go (2)
17-17: Use consistent naming for parameters.Consider renaming
mandateIdtomandateIDfor typical Go naming conventions (e.g., HTTP -> http, ID -> id).-GetMandate(ctx context.Context, mandateId string) (*gocardless.Mandate, error) +GetMandate(ctx context.Context, mandateID string) (*gocardless.Mandate, error)
55-83: Consider splitting large structs.
GocardlessUsercontains many fields and can grow unwieldy. A logical grouping or nested structs might improve maintainability.internal/connectors/plugins/public/gocardless/client/client_generated.go (1)
1-7: Generated code notice.This file is auto-generated by MockGen. Avoid manual edits, and ensure you commit it only when needed for reproducible test environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (19)
docs/api/README.md(2 hunks)internal/connectors/engine/activities/errors.go(1 hunks)internal/connectors/engine/activities/plugin_create_bank_account_test.go(1 hunks)internal/connectors/plugins/public/gocardless/README.md(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/gocardless/capabilities.go(1 hunks)internal/connectors/plugins/public/gocardless/client/bank_account_creation.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client.go(1 hunks)internal/connectors/plugins/public/gocardless/client/client_generated.go(1 hunks)internal/connectors/plugins/public/gocardless/client/creditors.go(1 hunks)internal/connectors/plugins/public/gocardless/client/customers.go(1 hunks)internal/connectors/plugins/public/gocardless/client/external_accounts.go(1 hunks)internal/connectors/plugins/public/gocardless/client/mandate.go(1 hunks)internal/connectors/plugins/public/gocardless/client/metadata.go(1 hunks)internal/connectors/plugins/public/gocardless/client/payments.go(1 hunks)internal/connectors/plugins/public/gocardless/config.go(1 hunks)internal/connectors/plugins/public/gocardless/currencies.go(1 hunks)internal/connectors/plugins/public/gocardless/external_accounts.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/connectors/plugins/public/gocardless/currencies.go
- internal/connectors/engine/activities/errors.go
- internal/connectors/plugins/public/gocardless/config.go
- internal/connectors/engine/activities/plugin_create_bank_account_test.go
- internal/connectors/plugins/public/gocardless/capabilities.go
- internal/connectors/plugins/public/gocardless/client/bank_account_creation.go
- internal/connectors/plugins/public/gocardless/client/mandate.go
- internal/connectors/plugins/public/gocardless/client/creditors.go
- internal/connectors/plugins/public/gocardless/client/payments.go
- internal/connectors/plugins/public/gocardless/bank_account_creation_test.go
- internal/connectors/plugins/public/gocardless/external_accounts.go
- internal/connectors/plugins/public/gocardless/client/metadata.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/connectors/plugins/public/gocardless/client/customers.go (1)
internal/connectors/plugins/public/gocardless/client/client.go (2)
GocardlessUser(55-83)Cursor(42-44)
internal/connectors/plugins/public/gocardless/bank_account_creation.go (4)
internal/connectors/plugins/public/gocardless/plugin.go (1)
Plugin(46-51)internal/models/plugin.go (1)
CreateBankAccountResponse(116-118)internal/connectors/plugins/public/gocardless/client/metadata.go (2)
GocardlessCreditorMetadataKey(7-7)GocardlessCustomerMetadataKey(8-8)internal/connectors/plugins/public/gocardless/client/client.go (1)
GocardlessGenericAccount(46-53)
internal/connectors/plugins/public/gocardless/client/external_accounts.go (3)
internal/connectors/plugins/public/gocardless/client/client.go (2)
GocardlessGenericAccount(46-53)Cursor(42-44)internal/connectors/metrics/transport.go (1)
MetricOperationContextKey(14-14)internal/models/account_type.go (1)
AccountType(3-3)
internal/connectors/plugins/public/gocardless/client/client_generated.go (2)
internal/connectors/plugins/public/gocardless/client/client.go (4)
GocardlessGenericAccount(46-53)GocardlessUser(55-83)Cursor(42-44)GoCardlessService(23-32)internal/connectors/plugins/public/gocardless/client/payments.go (1)
GocardlessPayment(12-21)
🪛 LanguageTool
internal/connectors/plugins/public/gocardless/README.md
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...Cursor-based Pagination**: - after: Points to the last item in the current ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... - after: Points to the last item in the current page. Used to fetch the nex...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...art after "CU123" ``` - before: Points to the first item in the current...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... - before: Points to the first item in the current page. Used for backwards pa...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ...ate Tracking**: - lastCreationDate: Timestamp of the most recently processe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~257-~257: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/api/README.md
5271-5271: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (7)
internal/connectors/plugins/public/gocardless/client/customers.go (1)
1-59: Well-structured implementation of customer fetching with proper error handling.The
GetCustomersfunction is well-implemented with appropriate error handling and data transformation. The code properly sets up metrics context, handles API responses, parses dates correctly, and constructs the response objects with all the necessary customer details.internal/connectors/plugins/public/gocardless/README.md (2)
1-323: Comprehensive documentation with clear examples and implementation detailsThe README provides excellent documentation for the GoCardless connector, covering all aspects from configuration to usage examples and implementation details. The state management and pagination explanations are particularly helpful.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~158-~158: Loose punctuation mark.
Context: ...Cursor-based Pagination**: -after: Points to the last item in the current ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... -after: Points to the last item in the current page. Used to fetch the nex...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~163-~163: Loose punctuation mark.
Context: ...art after "CU123" ``` -before: Points to the first item in the current...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~163-~163: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... -before: Points to the first item in the current page. Used for backwards pa...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ...ate Tracking**: -lastCreationDate: Timestamp of the most recently processe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~257-~257: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...# Known Limitations 1. Webhooks: - Currently not supported, Gocardless does not supp...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
96-100:✅ Verification successful
Ensure ID prefix validation is implemented safely
The documentation states that IDs must start with specific prefixes, which requires validation code. Given a previous review comment about potential string indexing issues, ensure the validation is implemented safely.
🏁 Script executed:
#!/bin/bash # Search for validation of ID prefixes in the codebase echo "Searching for ID prefix validation code..." rg -A 5 "creditor\s*[:=]|customer\s*[:=].*\[:2\]" internal/connectors/plugins/public/gocardless/Length of output: 2219
Action: Confirm Safe ID Prefix Validation Implementation
Based on our verification, there’s no evidence of unsafe string slicing (e.g., using
id[:2]without a length check) in the current validation logic. The code in files such asinternal/connectors/plugins/public/gocardless/utils.goandinternal/connectors/plugins/public/gocardless/bank_account_creation.goperforms its checks via conditional validations rather than direct substring indexing.
- The validation logic safeguards against scenarios where both or neither of the creditor/customer IDs are provided.
- No direct slicing is observed, so the risk of index errors is mitigated.
Please continue to use these safe practices. If you introduce direct substring operations in the future for prefix validation, ensure that you include appropriate length checks.
internal/connectors/plugins/public/gocardless/client/external_accounts.go (1)
83-96: Check consistency ofAccountTypefor customer accounts.
getCreditorExternalAccountspopulates theAccountTypefield, butgetCustomerExternalAccountsomits it. Verify if this differing behavior is intended.internal/connectors/plugins/public/gocardless/client/client.go (2)
46-53: Struct definition looks good.The
GocardlessGenericAccountstruct aligns well with API fields, enabling straightforward serialization and deserialization.
123-149: Configuration and error handling are well-implemented.
New(...)ensures the endpoint and access token are validated, returning meaningful errors from the GoCardless library. This is a solid approach.internal/connectors/plugins/public/gocardless/client/client_generated.go (1)
21-349: Mocks appear to align with interfaces.Overall, the generated mocks reflect the interface methods accurately, enabling comprehensive and flexible tests.
| func (p *Plugin) createBankAccount(ctx context.Context, ba models.BankAccount) (models.CreateBankAccountResponse, error) { | ||
| err := validateExternalBankAccount(ba) | ||
| if err != nil { | ||
| return models.CreateBankAccountResponse{}, err | ||
| } | ||
|
|
||
| creditor := ba.Metadata[client.GocardlessCreditorMetadataKey] | ||
| customer := ba.Metadata[client.GocardlessCustomerMetadataKey] | ||
| var externalBankAccount client.GocardlessGenericAccount | ||
|
|
||
| if creditor != "" { | ||
| externalBankAccount, err = p.client.CreateCreditorBankAccount(ctx, creditor, ba) | ||
|
|
||
| if err != nil { | ||
| return models.CreateBankAccountResponse{}, err | ||
| } | ||
| } | ||
|
|
||
| if customer != "" { | ||
| externalBankAccount, err = p.client.CreateCustomerBankAccount(ctx, customer, ba) | ||
|
|
||
| if err != nil { | ||
| return models.CreateBankAccountResponse{}, err | ||
| } | ||
| } | ||
|
|
||
| bankAccount, err := externalAccountFromGocardlessData(externalBankAccount) | ||
|
|
||
| if err != nil { | ||
| return models.CreateBankAccountResponse{}, err | ||
| } | ||
|
|
||
| return models.CreateBankAccountResponse{ | ||
| RelatedAccount: bankAccount, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Missing validation when both creditor and customer are provided or neither is provided
The current implementation has two potential issues:
- It doesn't validate that at least one of creditor or customer is specified
- It doesn't handle the case where both are provided (it will process creditor first, then customer, potentially overwriting the first result)
Apply this diff to improve the validation logic:
func (p *Plugin) createBankAccount(ctx context.Context, ba models.BankAccount) (models.CreateBankAccountResponse, error) {
err := validateExternalBankAccount(ba)
if err != nil {
return models.CreateBankAccountResponse{}, err
}
creditor := ba.Metadata[client.GocardlessCreditorMetadataKey]
customer := ba.Metadata[client.GocardlessCustomerMetadataKey]
+
+ if creditor == "" && customer == "" {
+ return models.CreateBankAccountResponse{}, fmt.Errorf("either creditor or customer must be specified")
+ }
+
+ if creditor != "" && customer != "" {
+ return models.CreateBankAccountResponse{}, fmt.Errorf("only one of creditor or customer can be specified")
+ }
+
var externalBankAccount client.GocardlessGenericAccount
if creditor != "" {
externalBankAccount, err = p.client.CreateCreditorBankAccount(ctx, creditor, ba)
if err != nil {
return models.CreateBankAccountResponse{}, err
}
}
if customer != "" {
externalBankAccount, err = p.client.CreateCustomerBankAccount(ctx, customer, ba)
if err != nil {
return models.CreateBankAccountResponse{}, err
}
}
bankAccount, err := externalAccountFromGocardlessData(externalBankAccount)
if err != nil {
return models.CreateBankAccountResponse{}, err
}
return models.CreateBankAccountResponse{
RelatedAccount: bankAccount,
}, nil
}Don't forget to add the missing import:
import (
"context"
"fmt"
"github.com/formancehq/payments/internal/connectors/plugins/public/gocardless/client"
"github.com/formancehq/payments/internal/models"
)| |accessToken|string|true|none|none| | ||
| |endpoint|string|true|none|none| | ||
| |name|string|true|none|none| | ||
| |pageSize|integer|false|none|none| |
There was a problem hiding this comment.
pageSize is not required, if not provided, it will default to 25
There was a problem hiding this comment.
Please note that the contents of docs/api/README.md are auto-generated by the just pre-commit command. They are recreated on each run and would be overwritten if edited manually.
| |endpoint|string|true|none|none| | ||
| |name|string|true|none|none| | ||
| |pageSize|integer|false|none|none| | ||
| |pollingPeriod|string|false|none|none| |
There was a problem hiding this comment.
pollingPeriod is not required, if not provided it will default to 2minutes
| |name|string|true|none|none| | ||
| |pageSize|integer|false|none|none| | ||
| |pollingPeriod|string|false|none|none| | ||
| |provider|string|false|none|none| |
There was a problem hiding this comment.
You can remove this field
| defaultAsset := currency.FormatAsset(SupportedCurrenciesWithDecimal, data.Currency) | ||
|
|
||
| metadata := extractExternalAccountMetadata(data.Metadata) | ||
| metadata[client.GocardlessAccountTypeMetadataKey] = data.AccountType |
There was a problem hiding this comment.
AccountType is empty sometimes, should we not add it to the metadata in that case ?
| } | ||
|
|
||
| sourceAccountReference = mandate.Links.Creditor | ||
| destinationAccountReference = mandate.Links.Customer |
There was a problem hiding this comment.
Here, we need the CustomerBankAccount field instead, so that we have the right link to an actual account we inserted with the polling
| SourceAccountReference: &payment.SourceAccountReference, | ||
| DestinationAccountReference: &payment.DestinationAccountReference, | ||
| Raw: raw, | ||
| Type: models.PAYMENT_TYPE_TRANSFER, |
There was a problem hiding this comment.
Should be a payout here
| return []GocardlessPayment{}, Cursor{}, err | ||
| } | ||
|
|
||
| sourceAccountReference = mandate.Links.Creditor |
There was a problem hiding this comment.
Here the creditor will actually not link to any accounts on our side. We have to find a way to get the creditor linked bank account instead, I can see that we have the payout id which contains the creditor bank account (https://developer.gocardless.com/api-reference/#core-endpoints-payments)
| } | ||
|
|
||
| sourceAccountReference = mandate.Links.Creditor | ||
| destinationAccountReference = mandate.Links.Customer |
There was a problem hiding this comment.
From what I understand here in t he description here (https://developer.gocardless.com/api-reference/#core-endpoints-payments), a payments is from a customer to a creditor, so the creditor bank account should be in the destination account reference field and t he customer bank account should be in the source account field
| } | ||
|
|
||
| payments = append(payments, models.PSPPayment{ | ||
| Reference: payment.ID, |
There was a problem hiding this comment.
I think here you should put as reference the payment.ID and as ParentReference the payout ID.
That way, when you have a new payments (as pending), it will be inserted with parent reference so the payout ID, with the adjusment with the payment ID for the pending, and if the payout succeeded, it will create a new adjustment and not create a new payment
| endpoint = SandboxEndpoint | ||
| } | ||
|
|
||
| config, err := gocardless.NewConfig(accessToken, gocardless.WithEndpoint(endpoint)) |
There was a problem hiding this comment.
Please use our custom http client like we did with stripe sdk here: https://github.com/formancehq/payments/blob/main/internal/connectors/plugins/public/stripe/client/client.go#L43
That will allow use to send the right metrics and traces
gitstart-app
bot
force-pushed
the
LNK-38
branch
from
351d2ca to
4117018
Compare
| Status: payment.Status, | ||
| Asset: payment.Currency, | ||
| Metadata: GocardlessMetadata, | ||
| SourceAccountReference: sourceAccountReference, |
There was a problem hiding this comment.
If you don't have a source account reference or a destination account reference, can you leave the field empty instead of putting UNKNOWN please ?
|
|
||
| paymentsResponse, err := c.service.GetGocardlessPayments(ctx, gocardless.PaymentListParams{ | ||
| Limit: pageSize, | ||
| After: after, |
There was a problem hiding this comment.
From what I can see in the API, we can sort the list. Can you add a sort_direction=asc please ? It will be better to have the payments in the right order !
There was a problem hiding this comment.
I tested, the after should work just the same, we just have to add the sort direction
This PR was created by GitStart to address the requirements from this ticket: LNK-38.
Integrate GoCardless Connector for Payment Service
This PR adds a GoCardless connector to the Payments service, making it easy to integrate with GoCardless for processing payments. It allows fetching creditors, customers, external accounts, and payments, and also supports creating bank accounts.
The connector uses the GoCardless Pro SDK for Go and keeps track of data updates using cursor-based pagination to avoid missing or duplicating records.
Key features: