fix(connectors): fix webhook idempotency key and tests #445
Conversation
## Walkthrough
This change introduces a new webhook verification mechanism across the plugin architecture, adding a `VerifyWebhook` method to the plugin interface and implementations. The webhook idempotency key is now generated and enforced at the plugin level, stored in the database, and used in the workflow logic. Tests and mocks are updated to cover the new verification flow, and a database migration adds a unique idempotency key constraint.
## Changes
| Files/Paths | Change Summary |
|------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| internal/models/plugin.go, internal/models/plugin_generated.go, internal/models/webhooks.go | Added `VerifyWebhook` to plugin interface, new request/response types, updated mocks, and added `IdempotencyKey` to webhook models. |
| internal/connectors/engine/activities/activity.go, internal/connectors/engine/activities/plugin_verify_webhook.go, internal/connectors/engine/activities/plugin_verify_webhook_test.go | Added new Temporal activity for webhook verification, corresponding helper, and comprehensive tests for plugin verification logic and error handling. |
| internal/connectors/engine/workflow/handle_webhooks.go, internal/connectors/engine/workflow/handle_webhooks_test.go | Integrated plugin webhook verification into the workflow, updated idempotency key handling, and extended tests for verification and error scenarios. |
| internal/connectors/plugins/base_plugin.go, internal/connectors/plugins/public/*/plugin.go, internal/connectors/plugins/public/*/webhooks.go, internal/connectors/plugins/public/*/webhook.go, internal/connectors/plugins/public/*/webhooks_test.go, internal/connectors/plugins/public/*/plugin_test.go | Implemented `VerifyWebhook` in all plugin types, separated verification from translation logic, updated plugin test setups, and added/updated verification tests. |
| internal/connectors/plugins/registry/wrapper.go | Wrapped plugin `VerifyWebhook` with tracing and logging in the registry. |
| internal/storage/migrations/16-webhooks-idempotency-key.sql, internal/storage/migrations/migrations.go | Added SQL migration to introduce a unique `idempotency_key` column to the webhooks table and registered the migration in Go code. |
| internal/storage/webhooks.go, internal/storage/webhooks_test.go | Updated storage logic to handle the new `IdempotencyKey` field in webhooks, and added tests for null and duplicate idempotency key scenarios. |
| internal/connectors/plugins/public/*_test.go, internal/connectors/plugins/public/*/plugin_test.go, internal/connectors/plugins/public/*/webhooks_test.go, internal/connectors/plugins/public/column/reverse_payout_test.go, internal/connectors/plugins/public/column/transfers_test.go, internal/connectors/plugins/public/wise/profiles_test.go, internal/connectors/plugins/public/wise/uninstall_test.go | Refactored plugin test initialization to use interface types and centralized mock setup, updated tests to use new verification and idempotency key logic, and removed redundant or outdated test code. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Workflow
participant Activities
participant PluginRegistry
participant Plugin
participant Storage
Workflow->>Activities: PluginVerifyWebhook(connectorID, VerifyWebhookRequest)
Activities->>PluginRegistry: GetPlugin(connectorID)
PluginRegistry->>Plugin: VerifyWebhook(VerifyWebhookRequest)
Plugin-->>PluginRegistry: VerifyWebhookResponse (with IdempotencyKey)
PluginRegistry-->>Activities: VerifyWebhookResponse
Activities-->>Workflow: VerifyWebhookResponse
alt Verification successful
Workflow->>Storage: StoreWebhook(with IdempotencyKey)
else Verification failed
Workflow-->>Workflow: Return error
endAssessment against linked issues
Suggested reviewers
Poem
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
internal/connectors/plugins/public/atlar/payments_test.go (1)
1-289: 💡 Verification agent🧩 Analysis chain
Verify the new VerifyWebhook function implementation.
While the test structure changes look good, I don't see the implementation of the new
VerifyWebhookfunction mentioned in the PR objectives. This function is crucial for the webhook idempotency key mechanism.
🏁 Script executed:
#!/bin/bash # Check if the VerifyWebhook function is implemented in the Atlar plugin echo "Searching for VerifyWebhook implementation in Atlar plugin..." rg -A 5 "func \(\*Plugin\) VerifyWebhook" internal/connectors/plugins/public/atlar/ # Check if the VerifyWebhook function is defined in the plugin interface echo "Checking if VerifyWebhook is defined in the plugin interface..." rg -A 3 "VerifyWebhook" internal/models/plugin.goLength of output: 744
Implement the missing VerifyWebhook method in the Atlar plugin
The plugin interface (
internal/models/plugin.go) defines:
VerifyWebhook(context.Context, VerifyWebhookRequest) (VerifyWebhookResponse, error)However, I couldn’t find any implementation of:
func (*Plugin) VerifyWebhook(...)underinternal/connectors/plugins/public/atlar/Please add the
VerifyWebhookmethod to yourPlugintype and cover it with tests to support the webhook idempotency key mechanism.
🧹 Nitpick comments (11)
internal/connectors/engine/workflow/handle_webhooks.go (1)
1-123: Consider documenting the new idempotency mechanism.The implementation of the webhook verification and idempotency changes is solid. However, it would be beneficial to add documentation that explains the overall idempotency approach and how it differs from the previous implementation.
Consider adding a comment like:
// HandleWebhooks defines the input for webhook handling workflow type HandleWebhooks struct { ConnectorID models.ConnectorID URLPath string Webhook models.Webhook } +// runHandleWebhooks processes incoming webhooks by: +// 1. Finding the matching webhook configuration +// 2. Verifying the webhook and obtaining an idempotency key for deduplication +// 3. Storing the webhook with its idempotency key in persistent storage +// 4. Translating the webhook to platform objects (accounts, payments) +// 5. Storing the translated objects +// +// The idempotency key is provided by the plugin's VerifyWebhook method and stored +// with the webhook, which prevents duplicate processing even if Temporal workflows +// are deleted after completion. func (w Workflow) runHandleWebhooks( ctx workflow.Context, handleWebhooks HandleWebhooks, ) error {internal/connectors/plugins/public/wise/plugin_test.go (1)
104-168: Good separation of webhook verification from translationThe implementation of a dedicated
VerifyWebhookmethod with proper tests improves the design by separating concerns: authentication/verification vs. content translation. The tests properly validate the error cases for missing headers and the successful case returning an idempotency key.Consider extracting the common setup code between the "verifying webhook" and "translate webhook" contexts to reduce duplication, perhaps by moving the shared mock client setup and test data generation to a higher-level BeforeEach block.
internal/connectors/plugins/public/column/bank_account_creation_test.go (2)
20-26: Callctrl.Finish()to guarantee mock expectations are asserted
gomock.NewControllershould be paired withctrl.Finish()(orDeferCleanup(ctrl.Finish)in Ginkgo-v2) to ensure that all expectations are checked and to release resources.
Without it, silent expectation leaks can creep in and later tests may pass unexpectedly.BeforeEach(func() { ctrl := gomock.NewController(GinkgoT()) + DeferCleanup(ctrl.Finish) // make sure gomock verifies expectations
28-119: Consider a table-driven test to reduce duplicationThe 7 “missing field” scenarios follow the same Arrange-Act-Assert pattern with only the request body and expected error changing. Converting them into a single
DescribeTable(or a loop in plain Go tests) will:
- Cut ~120 lines of repetitive code
- Make it trivial to add new validation cases
- Keep the intent of each case in one concise row
This is optional but will improve maintainability.
internal/connectors/plugins/public/wise/webhooks_test.go (1)
64-75: Assertion on created webhooks may become brittleThe test hard-codes:
Expect(res.Others).To(HaveLen(1))If additional supported webhooks are introduced in the production code (e.g., new event types), this expectation will start failing although the behaviour is still correct.
Two safer alternatives:
- Assert that at least one webhook was created and that the expected one is present.
- Build the expectation from
p.supportedWebhooksso the test automatically adapts.-Expect(res.Others).To(HaveLen(1)) +Expect(res.Others).ToNot(BeEmpty()) +Expect(res.Others).To(ContainElement( + SatisfyAll( + HaveField("ID", expectedWebhookResponseID), + ), +))internal/connectors/plugins/public/adyen/webhooks.go (1)
56-76: Avoid double JSON parsing for performance
verifyWebhookparses the body viaTranslateWebhook, andtranslateStandardWebhookparses it again.
Passing the already parsedwebhooksstructure (or the computed idempotency key) downstream would:
- Eliminate redundant CPU / allocations
- Guarantee both phases operate on identical data
Consider enhancing
models.VerifyWebhookResponseto optionally carry the parsed notifications.internal/connectors/plugins/public/column/webhook.go (1)
300-328: Consider avoiding duplicate JSON unmarshalingThe webhook body is unmarsaled both in
verifyWebhook(line 321) and again intranslateWebhook(line 347). This duplication is inefficient for large webhook payloads.Consider refactoring to unmarshal only once, perhaps by:
- Adding the parsed result to the response
- Caching the parsed result within the plugin instance
- Or optionally modifying the workflow to pass the parsed data between activities
internal/connectors/engine/activities/plugin_verify_webhook.go (1)
30-39: Minor optimization opportunity in helper functionThe function creates a response struct, passes it to
executeActivity, and then returns a pointer to it.This could be slightly simplified:
func PluginVerifyWebhook(ctx workflow.Context, connectorID models.ConnectorID, request models.VerifyWebhookRequest) (*models.VerifyWebhookResponse, error) { - ret := models.VerifyWebhookResponse{} - if err := executeActivity(ctx, PluginVerifyWebhookActivity, &ret, VerifyWebhookRequest{ + var ret models.VerifyWebhookResponse + if err := executeActivity(ctx, PluginVerifyWebhookActivity, ret, VerifyWebhookRequest{ ConnectorID: connectorID, Req: request, }); err != nil { return nil, err } - return &ret, nil + return ret, nil }internal/connectors/plugins/public/column/payouts_test.go (1)
667-714: Consider using interface method for consistency.While the tests now generally use the interface type, this specific test directly calls a concrete method on a local variable of type
Plugin. Consider refactoring this to use the interface method if possible, maintaining consistency with the rest of the test suite.- p := Plugin{} - result := p.mapTransactionStatus(tc.status) + // Either expose the method through the interface or + // use a type assertion if the method is internal + plugin := &Plugin{} + plg := models.Plugin(plugin) + result := plugin.mapTransactionStatus(tc.status)internal/connectors/plugins/public/column/webhook_test.go (2)
150-160: Mock method name should follow the interface – avoid brittle stubbingThe expectation is set on an unexported method name
verifyWebhookSignature. If the interface changes (e.g. exported
VerifySignature) the mock generator will rename the method and the
test will compile but silently stop verifying the call.Consider:
verifierMock.EXPECT(). VerifySignature(gomock.Any(), gomock.Any(), gomock.Any()). Return(errors.New("test error"))and align the interface accordingly.
162-184: Hard-coding the idempotency key couples the test to payload internals
pointer.For("1")assumes the plugin will always return exactly the
payload’sidfield. If the idempotency strategy changes (e.g. SHA-256
hash of the body) the test will fail even though behaviour is still
correct.Instead, check non-nil presence:
Expect(res.WebhookIdempotencyKey).ToNot(BeNil()) Expect(*res.WebhookIdempotencyKey).To(Equal("1")) // only if the spec mandates itor expose a helper constant from the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (92)
internal/connectors/engine/activities/activity.go(1 hunks)internal/connectors/engine/activities/plugin_verify_webhook.go(1 hunks)internal/connectors/engine/activities/plugin_verify_webhook_test.go(1 hunks)internal/connectors/engine/workflow/handle_webhooks.go(2 hunks)internal/connectors/engine/workflow/handle_webhooks_test.go(7 hunks)internal/connectors/plugins/base_plugin.go(1 hunks)internal/connectors/plugins/public/adyen/accounts_test.go(1 hunks)internal/connectors/plugins/public/adyen/plugin.go(1 hunks)internal/connectors/plugins/public/adyen/webhooks.go(3 hunks)internal/connectors/plugins/public/adyen/webhooks_test.go(8 hunks)internal/connectors/plugins/public/atlar/accounts_test.go(1 hunks)internal/connectors/plugins/public/atlar/balances_test.go(1 hunks)internal/connectors/plugins/public/atlar/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/atlar/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/atlar/payments_test.go(1 hunks)internal/connectors/plugins/public/atlar/payouts_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/accounts_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/balances_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/payments_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/payouts_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/transfers_test.go(1 hunks)internal/connectors/plugins/public/column/accounts_test.go(3 hunks)internal/connectors/plugins/public/column/balances_test.go(1 hunks)internal/connectors/plugins/public/column/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/column/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/column/payments_test.go(1 hunks)internal/connectors/plugins/public/column/payouts_test.go(5 hunks)internal/connectors/plugins/public/column/plugin.go(1 hunks)internal/connectors/plugins/public/column/plugin_test.go(2 hunks)internal/connectors/plugins/public/column/reverse_payout_test.go(2 hunks)internal/connectors/plugins/public/column/transfers_test.go(1 hunks)internal/connectors/plugins/public/column/webhook.go(1 hunks)internal/connectors/plugins/public/column/webhook_test.go(5 hunks)internal/connectors/plugins/public/currencycloud/accounts_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/balances_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/payments_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/payouts_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/transfers_test.go(1 hunks)internal/connectors/plugins/public/generic/accounts_test.go(1 hunks)internal/connectors/plugins/public/generic/balances_test.go(1 hunks)internal/connectors/plugins/public/generic/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/generic/payments_test.go(1 hunks)internal/connectors/plugins/public/mangopay/accounts_test.go(1 hunks)internal/connectors/plugins/public/mangopay/balances_test.go(1 hunks)internal/connectors/plugins/public/mangopay/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/mangopay/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/mangopay/payments_test.go(1 hunks)internal/connectors/plugins/public/mangopay/payouts_test.go(1 hunks)internal/connectors/plugins/public/mangopay/plugin.go(1 hunks)internal/connectors/plugins/public/mangopay/transfers_test.go(1 hunks)internal/connectors/plugins/public/mangopay/users_test.go(1 hunks)internal/connectors/plugins/public/mangopay/webhooks.go(4 hunks)internal/connectors/plugins/public/mangopay/webhooks_test.go(2 hunks)internal/connectors/plugins/public/modulr/accounts_test.go(1 hunks)internal/connectors/plugins/public/modulr/balances_test.go(1 hunks)internal/connectors/plugins/public/modulr/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/modulr/payments_test.go(2 hunks)internal/connectors/plugins/public/modulr/payouts_test.go(1 hunks)internal/connectors/plugins/public/modulr/transfers_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/accounts_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/balances_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/payments_test.go(2 hunks)internal/connectors/plugins/public/moneycorp/payouts_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/transfers_test.go(1 hunks)internal/connectors/plugins/public/stripe/accounts_test.go(1 hunks)internal/connectors/plugins/public/stripe/balances_test.go(1 hunks)internal/connectors/plugins/public/stripe/create_payouts_test.go(1 hunks)internal/connectors/plugins/public/stripe/create_transfers_test.go(1 hunks)internal/connectors/plugins/public/stripe/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/stripe/reverse_transfers_test.go(1 hunks)internal/connectors/plugins/public/wise/accounts_test.go(1 hunks)internal/connectors/plugins/public/wise/balances_test.go(1 hunks)internal/connectors/plugins/public/wise/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/wise/payments_test.go(1 hunks)internal/connectors/plugins/public/wise/payouts_test.go(1 hunks)internal/connectors/plugins/public/wise/plugin.go(1 hunks)internal/connectors/plugins/public/wise/plugin_test.go(2 hunks)internal/connectors/plugins/public/wise/profiles_test.go(1 hunks)internal/connectors/plugins/public/wise/transfers_test.go(1 hunks)internal/connectors/plugins/public/wise/uninstall_test.go(1 hunks)internal/connectors/plugins/public/wise/webhooks_test.go(3 hunks)internal/connectors/plugins/registry/wrapper.go(1 hunks)internal/models/plugin.go(2 hunks)internal/models/plugin_generated.go(2 hunks)internal/models/webhooks.go(1 hunks)internal/storage/migrations/16-webhooks-idempotency-key.sql(1 hunks)internal/storage/migrations/migrations.go(2 hunks)internal/storage/webhooks.go(2 hunks)internal/storage/webhooks_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
internal/connectors/plugins/public/moneycorp/balances_test.go (1)
Learnt from: laouji
PR: formancehq/payments#193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
internal/connectors/plugins/public/mangopay/balances_test.go (1)
Learnt from: laouji
PR: formancehq/payments#193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
internal/connectors/plugins/public/generic/balances_test.go (1)
Learnt from: laouji
PR: formancehq/payments#193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
internal/connectors/plugins/public/wise/balances_test.go (1)
Learnt from: laouji
PR: formancehq/payments#193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
internal/connectors/plugins/public/modulr/balances_test.go (1)
Learnt from: laouji
PR: formancehq/payments#193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
internal/connectors/plugins/public/currencycloud/balances_test.go (1)
Learnt from: laouji
PR: formancehq/payments#193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
🧬 Code Graph Analysis (52)
internal/connectors/plugins/public/moneycorp/payments_test.go (1)
internal/connectors/plugins/public/moneycorp/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/bankingcircle/transfers_test.go (4)
internal/connectors/plugins/public/bankingcircle/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/models/plugin.go (1)
Plugin(14-21)internal/connectors/plugins/public/bankingcircle/plugin.go (1)
Plugin(22-29)internal/models/payment_initiations.go (1)
PSPPaymentInitiation(11-33)
internal/connectors/plugins/public/mangopay/payments_test.go (2)
internal/connectors/plugins/public/mangopay/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/mangopay/plugin.go (1)
Plugin(25-33)
internal/connectors/plugins/public/atlar/external_accounts_test.go (1)
internal/connectors/plugins/public/atlar/client/client_generated.go (2)
MockClient(28-32)NewMockClient(40-44)
internal/connectors/plugins/public/moneycorp/accounts_test.go (1)
internal/connectors/plugins/public/moneycorp/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/mangopay/webhooks.go (2)
internal/models/payments.go (1)
Payment(87-127)internal/connectors/plugins/public/mangopay/client/transactions.go (1)
Payment(14-31)
internal/connectors/plugins/public/wise/uninstall_test.go (2)
internal/connectors/plugins/public/wise/plugin.go (1)
Plugin(34-43)internal/connectors/plugins/public/wise/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)
internal/connectors/plugins/public/modulr/external_accounts_test.go (2)
internal/connectors/plugins/public/modulr/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/modulr/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/wise/accounts_test.go (1)
internal/connectors/plugins/public/wise/plugin.go (1)
Plugin(34-43)
internal/connectors/plugins/public/column/plugin.go (3)
internal/models/plugin.go (3)
Plugin(14-21)VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)internal/connectors/plugins/public/mangopay/plugin.go (1)
Plugin(25-33)internal/connectors/plugins/errors.go (1)
ErrNotYetInstalled(9-9)
internal/models/webhooks.go (1)
internal/models/idempotency.go (1)
IdempotencyKey(11-19)
internal/connectors/engine/workflow/handle_webhooks.go (4)
internal/connectors/engine/activities/plugin_verify_webhook.go (2)
PluginVerifyWebhook(30-39)VerifyWebhookRequest(10-13)internal/models/plugin.go (1)
VerifyWebhookRequest(158-161)internal/models/webhooks.go (3)
Webhook(33-41)PSPWebhook(25-31)BasicAuth(20-23)internal/models/idempotency.go (1)
IdempotencyKey(11-19)
internal/connectors/plugins/public/moneycorp/external_accounts_test.go (1)
internal/connectors/plugins/public/moneycorp/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/adyen/plugin.go (2)
internal/models/plugin.go (3)
Plugin(14-21)VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)internal/connectors/plugins/errors.go (1)
ErrNotYetInstalled(9-9)
internal/connectors/plugins/public/column/payments_test.go (4)
internal/connectors/plugins/public/column/client/http_generated.go (2)
MockHTTPClient(21-25)NewMockHTTPClient(33-37)internal/models/plugin.go (1)
Plugin(14-21)internal/connectors/plugins/public/column/plugin.go (2)
Plugin(69-79)New(81-102)internal/connectors/plugins/public/column/client/client.go (1)
New(67-87)
internal/connectors/plugins/public/modulr/transfers_test.go (2)
internal/connectors/plugins/public/modulr/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/modulr/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/generic/accounts_test.go (2)
internal/connectors/plugins/public/generic/client/client_generated.go (2)
MockClient(22-26)NewMockClient(34-38)internal/connectors/plugins/public/generic/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/column/plugin_test.go (3)
internal/models/plugin.go (1)
Plugin(14-21)internal/connectors/plugins/public/column/plugin.go (3)
Plugin(69-79)New(81-102)ProviderName(16-16)internal/connectors/plugins/public/dummypay/plugin.go (2)
Plugin(21-27)New(29-42)
internal/connectors/plugins/public/bankingcircle/bank_account_creation_test.go (2)
internal/connectors/plugins/public/bankingcircle/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/bankingcircle/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/adyen/accounts_test.go (2)
internal/connectors/plugins/public/adyen/client/client_generated.go (2)
MockClient(23-27)NewMockClient(35-39)internal/connectors/plugins/public/adyen/plugin.go (1)
Plugin(23-33)
internal/storage/webhooks.go (2)
internal/models/idempotency.go (1)
IdempotencyKey(11-19)internal/models/webhooks.go (1)
Webhook(33-41)
internal/connectors/plugins/public/wise/balances_test.go (1)
internal/connectors/plugins/public/wise/plugin.go (1)
Plugin(34-43)
internal/connectors/plugins/public/mangopay/external_accounts_test.go (2)
internal/connectors/plugins/public/mangopay/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/mangopay/plugin.go (1)
Plugin(25-33)
internal/connectors/plugins/public/atlar/payments_test.go (1)
internal/connectors/plugins/public/atlar/client/client_generated.go (2)
MockClient(28-32)NewMockClient(40-44)
internal/connectors/plugins/public/wise/profiles_test.go (1)
internal/connectors/plugins/public/wise/plugin.go (1)
Plugin(34-43)
internal/connectors/plugins/public/column/external_accounts_test.go (4)
internal/connectors/plugins/public/column/client/http_generated.go (2)
MockHTTPClient(21-25)NewMockHTTPClient(33-37)internal/models/plugin.go (1)
Plugin(14-21)internal/connectors/plugins/public/column/plugin.go (2)
Plugin(69-79)New(81-102)internal/connectors/plugins/public/column/client/client.go (1)
New(67-87)
internal/connectors/plugins/public/modulr/accounts_test.go (2)
internal/connectors/plugins/public/modulr/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/modulr/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/wise/external_accounts_test.go (1)
internal/connectors/plugins/public/wise/plugin.go (1)
Plugin(34-43)
internal/connectors/plugins/base_plugin.go (3)
internal/models/plugin.go (2)
VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)internal/connectors/engine/activities/plugin_verify_webhook.go (1)
VerifyWebhookRequest(10-13)internal/connectors/plugins/errors.go (1)
ErrNotImplemented(8-8)
internal/connectors/plugins/public/currencycloud/transfers_test.go (2)
internal/connectors/plugins/public/currencycloud/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/currencycloud/plugin.go (1)
Plugin(22-29)
internal/storage/webhooks_test.go (3)
internal/models/idempotency.go (1)
IdempotencyKey(11-19)internal/models/webhooks.go (1)
Webhook(33-41)internal/storage/error.go (1)
ErrDuplicateKeyValue(18-18)
internal/connectors/plugins/public/stripe/external_accounts_test.go (2)
internal/connectors/plugins/public/stripe/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/stripe/plugin.go (1)
Plugin(22-28)
internal/connectors/plugins/public/bankingcircle/accounts_test.go (2)
internal/connectors/plugins/public/bankingcircle/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/bankingcircle/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/atlar/accounts_test.go (1)
internal/connectors/plugins/public/atlar/client/client_generated.go (2)
MockClient(28-32)NewMockClient(40-44)
internal/connectors/plugins/public/generic/external_accounts_test.go (2)
internal/connectors/plugins/public/generic/client/client_generated.go (2)
MockClient(22-26)NewMockClient(34-38)internal/connectors/plugins/public/generic/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/mangopay/bank_account_creation_test.go (2)
internal/connectors/plugins/public/mangopay/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/mangopay/plugin.go (1)
Plugin(25-33)
internal/connectors/plugins/public/mangopay/users_test.go (2)
internal/connectors/plugins/public/mangopay/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/mangopay/plugin.go (1)
Plugin(25-33)
internal/connectors/plugins/public/mangopay/accounts_test.go (2)
internal/connectors/plugins/public/mangopay/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)
internal/connectors/plugins/public/mangopay/plugin.go (1)
Plugin(25-33)
internal/connectors/plugins/public/currencycloud/payments_test.go (2)
internal/connectors/plugins/public/currencycloud/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/currencycloud/plugin.go (1)
Plugin(22-29)
internal/connectors/engine/activities/plugin_verify_webhook_test.go (8)
internal/connectors/engine/activities/activity.go (2)
Activities(17-26)New(348-364)internal/connectors/engine/plugins/plugin_generated.go (2)
MockPlugins(21-25)NewMockPlugins(33-37)internal/storage/storage_generated.go (2)
MockStorage(24-28)NewMockStorage(36-40)internal/models/plugin.go (2)
VerifyWebhookResponse(163-165)VerifyWebhookRequest(158-161)internal/models/plugin_generated.go (2)
MockPlugin(20-24)NewMockPlugin(32-36)internal/connectors/engine/activities/plugin_verify_webhook.go (2)
VerifyWebhookRequest(10-13)PluginVerifyWebhook(30-39)internal/connectors/engine/activities/errors.go (2)
ErrTypeDefault(18-18)ErrTypeUnimplemented(21-21)internal/connectors/plugins/errors.go (1)
ErrNotImplemented(8-8)
internal/models/plugin.go (2)
internal/connectors/engine/activities/plugin_verify_webhook.go (1)
VerifyWebhookRequest(10-13)internal/models/webhooks.go (3)
Webhook(33-41)PSPWebhook(25-31)WebhookConfig(11-18)
internal/connectors/plugins/public/generic/payments_test.go (2)
internal/connectors/plugins/public/generic/client/client_generated.go (2)
MockClient(22-26)NewMockClient(34-38)internal/connectors/plugins/public/generic/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/currencycloud/accounts_test.go (2)
internal/connectors/plugins/public/currencycloud/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/currencycloud/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/bankingcircle/payments_test.go (2)
internal/connectors/plugins/public/bankingcircle/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/bankingcircle/plugin.go (1)
Plugin(22-29)
internal/connectors/plugins/public/stripe/reverse_transfers_test.go (2)
internal/connectors/plugins/public/stripe/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/connectors/plugins/public/stripe/plugin.go (1)
Plugin(22-28)
internal/models/plugin_generated.go (3)
internal/models/plugin.go (2)
VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)internal/connectors/engine/activities/plugin_verify_webhook.go (1)
VerifyWebhookRequest(10-13)pkg/testserver/server.go (1)
T(32-37)
internal/connectors/plugins/public/column/webhook.go (6)
internal/models/plugin.go (5)
Plugin(14-21)VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)TranslateWebhookRequest(142-146)TranslateWebhookResponse(154-156)internal/connectors/plugins/public/wise/plugin.go (3)
Plugin(34-43)HeadersSignature(26-26)ErrWebhookHeaderXSignatureMissing(30-30)internal/connectors/plugins/public/column/plugin.go (1)
Plugin(69-79)internal/models/webhooks.go (1)
Webhook(33-41)internal/connectors/plugins/public/column/client/error.go (3)
ErrWebhookHeaderXSignatureMissing(10-10)ErrWebhookConfigInvalid(11-11)ErrWebhookConfigSecretMissing(13-13)internal/connectors/plugins/public/column/client/webhook.go (1)
WebhookEvent(71-76)
internal/connectors/plugins/public/atlar/bank_account_creation_test.go (1)
internal/connectors/plugins/public/atlar/client/client_generated.go (2)
MockClient(28-32)NewMockClient(40-44)
internal/connectors/plugins/public/wise/payments_test.go (1)
internal/connectors/plugins/public/wise/plugin.go (1)
Plugin(34-43)
internal/connectors/plugins/public/moneycorp/payouts_test.go (3)
internal/connectors/plugins/public/moneycorp/client/client_generated.go (2)
MockClient(21-25)NewMockClient(33-37)internal/models/plugin.go (1)
Plugin(14-21)internal/connectors/plugins/public/moneycorp/plugin.go (1)
Plugin(22-29)
internal/connectors/engine/activities/plugin_verify_webhook.go (2)
internal/models/plugin.go (2)
VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)internal/connectors/engine/activities/activity.go (1)
Activities(17-26)
internal/connectors/plugins/public/column/webhook_test.go (8)
internal/models/plugin.go (3)
Plugin(14-21)VerifyWebhookRequest(158-161)VerifyWebhookResponse(163-165)internal/connectors/plugins/public/column/plugin.go (2)
Plugin(69-79)New(81-102)internal/connectors/plugins/public/column/client/http_generated.go (2)
MockHTTPClient(21-25)NewMockHTTPClient(33-37)internal/connectors/plugins/public/column/webhookverifier_generated.go (2)
MockWebhookVerifier(19-23)NewMockWebhookVerifier(31-35)internal/connectors/plugins/public/column/client/webhook.go (2)
EventSubscription(78-87)EventCategoryACHTransferSettled(28-28)internal/connectors/engine/workflow/create_webhooks.go (1)
CreateWebhooks(13-17)internal/models/webhooks.go (3)
WebhookConfig(11-18)Webhook(33-41)PSPWebhook(25-31)internal/connectors/plugins/public/column/client/error.go (1)
ErrWebhookTypeUnknown(12-12)
🔇 Additional comments (158)
internal/models/webhooks.go (1)
35-37: LGTM: Adding IdempotencyKey field to support webhook deduplicationThe addition of the
IdempotencyKeyfield to theWebhookstruct is well-placed and properly typed as an optional*string. This change supports the new webhook idempotency mechanism described in the PR objectives, shifting from Temporal workflow IDs to persistent storage for deduplication.internal/storage/migrations/16-webhooks-idempotency-key.sql (1)
1-4: LGTM: Well-structured migration for webhook idempotencyThe migration adds the
idempotency_keycolumn to thewebhookstable and creates a unique index on the combination ofconnector_idandidempotency_key. This properly supports the webhook deduplication mechanism by enforcing uniqueness at the database level.internal/connectors/plugins/public/mangopay/webhooks.go (4)
208-210: LGTM: Removed IdempotencyKey from translateTransfer responseCorrectly removed the
IdempotencyKeyfield from theWebhookResponsereturned bytranslateTransfer. This aligns with the architectural change to separate webhook verification and idempotency key generation from translation logic.
251-253: LGTM: Removed IdempotencyKey from translatePayout responseCorrectly removed the
IdempotencyKeyfield from theWebhookResponsereturned bytranslatePayout. This aligns with the architectural change to separate webhook verification and idempotency key generation from translation logic.
290-292: LGTM: Removed IdempotencyKey from translatePayin responseCorrectly removed the
IdempotencyKeyfield from theWebhookResponsereturned bytranslatePayin. This aligns with the architectural change to separate webhook verification and idempotency key generation from translation logic.
334-336: LGTM: Removed IdempotencyKey from translateRefund responseCorrectly removed the
IdempotencyKeyfield from theWebhookResponsereturned bytranslateRefund. This aligns with the architectural change to separate webhook verification and idempotency key generation from translation logic.internal/connectors/engine/activities/activity.go (1)
90-93: LGTM: Added PluginVerifyWebhook activityAdded a new
PluginVerifyWebhookactivity to theDefinitionSetmethod, which is appropriately placed before the existingPluginTranslateWebhookactivity. This addition supports the new webhook verification flow described in the PR objectives.internal/connectors/plugins/public/moneycorp/payments_test.go (2)
21-21: Change from concrete type to interface type is appropriate.Changing
plgfrom a pointer to the concretePluginstruct to themodels.Plugininterface aligns with the PR objective to use interfaces in tests. This improves type checking for function implementations and makes it easier to detect errors in plugin interface implementations.
222-222: Interface type declaration provides better type safety.Consistent with the change in the first test block, this modification ensures the test is using the plugin interface rather than the concrete implementation. This approach helps validate that the plugin properly implements all required methods with the correct signatures.
internal/connectors/plugins/public/moneycorp/external_accounts_test.go (1)
19-19: Interface-based test setup ensures correct implementation.Changing the test variable from a concrete
*Plugintype to themodels.Plugininterface type is consistent with the changes in other test files. This approach ensures that all methods required by the plugin interface are correctly implemented and type-checked at compile time.internal/connectors/plugins/base_plugin.go (1)
79-81: Implementation of new webhook verification method.The addition of the
VerifyWebhookmethod to the base plugin supports the PR's goal of adding webhook idempotency key verification. This default implementation correctly returnsErrNotImplemented, consistent with the pattern used by other unimplemented methods in the base plugin. This allows each plugin to implement its own webhook verification logic while providing a standard interface.internal/connectors/plugins/public/moneycorp/balances_test.go (1)
18-18: Interface type declaration enhances type safety.Changing
plgfrom a concrete type to themodels.Plugininterface type is consistent with the same changes in other test files. This pattern ensures that the Moneycorp plugin correctly implements all interface methods with proper signatures, making type errors apparent at compile time rather than runtime.internal/connectors/plugins/public/adyen/plugin.go (1)
117-123: LGTM - VerifyWebhook implementation follows established patternsThe implementation follows the consistent error-handling pattern used by other plugin methods, correctly checking if the client is initialized before proceeding with webhook verification.
internal/connectors/plugins/public/mangopay/plugin.go (1)
173-177: LGTM - No-op implementation with clear explanationThis implementation correctly returns an empty response with no verification, as explained in the comments. The approach aligns with Mangopay's requirements where webhook verification isn't needed.
internal/connectors/plugins/public/wise/profiles_test.go (1)
18-18: LGTM - Improved test with interface typeGood refactoring to use the interface type (
models.Plugin) instead of the concrete type, allowing for better testing of the interface contract rather than implementation details.Also applies to: 25-25
internal/connectors/plugins/public/wise/uninstall_test.go (1)
15-15: LGTM - Consistent test improvementThis change consistently applies the same pattern of using the interface type for testing as seen in other test files, promoting better interface-based testing practices.
Also applies to: 22-22
internal/connectors/plugins/public/moneycorp/accounts_test.go (1)
18-18: Good use of interface type for better abstractionChanging
plgfrom a concrete type to the interface typemodels.Pluginimproves abstraction and matches production usage patterns where code is typically programmed against interfaces rather than implementations.internal/connectors/plugins/public/moneycorp/transfers_test.go (2)
19-20: Good use of interface type for better abstractionChanging
plgfrom a concrete type to the interface typemodels.Pluginimproves abstraction and makes the test code more maintainable.
24-26: Improved test setup with centralized mock initializationMoving the mock controller and client creation to the outer BeforeEach block reduces redundant setup code and improves test efficiency by avoiding repeated mock creation.
internal/connectors/plugins/public/column/plugin_test.go (3)
24-24: Good use of interface type for better abstractionChanging
plgfrom a concrete type to the interface typemodels.Pluginimproves abstraction and matches production usage patterns.
207-207: Consistent use of interface type in nested contextUsing the interface type consistently throughout the test file improves code clarity and maintainability.
212-215: Improved plugin initialization with proper type handlingThe refactored initialization properly handles the temporary concrete type for field validation before assigning to the interface variable, maintaining the same validation logic while improving abstraction.
internal/connectors/plugins/public/column/plugin.go (1)
196-201: Well-structured webhook verification methodThe new
VerifyWebhookmethod follows the established pattern in the codebase by checking client initialization before delegating to a private implementation method. This addition supports the PR's goal of improving webhook idempotency by adding a dedicated verification step that can generate an idempotency key.This implementation:
- Checks if the client is initialized
- Delegates to a private method for the actual verification
- Maintains consistent error handling with other plugin methods
internal/connectors/plugins/public/wise/balances_test.go (2)
18-18: Good interface change.Switching from concrete type to the
models.Plugininterface improves type safety for the test and aligns with the PR goal of ensuring correct function overrides. This change helps detect type errors at compile time rather than runtime.
25-25: Improved test setup.The plugin initialization is now more direct, removing the need for a separate
SetClientcall. This makes the test setup cleaner and more maintainable.internal/connectors/plugins/public/stripe/balances_test.go (2)
19-20: Good interface adoption pattern.Changing the plugin variable type to the interface
models.Pluginfollows the same pattern as in other plugin tests, improving consistency across the codebase and enabling better type checking.
24-26: Simplified test setup.Centralizing the mock client creation and plugin instantiation in a single
BeforeEachblock makes the test structure cleaner and more maintainable.internal/connectors/plugins/public/modulr/transfers_test.go (2)
19-20: Consistent interface-based testing.This change follows the same interface-based testing pattern as the other plugin tests, improving type safety and consistency across the codebase.
24-26: Streamlined test initialization.The initialization of mock and plugin in a single
BeforeEachblock simplifies the test setup while maintaining the test functionality.internal/storage/webhooks.go (4)
18-18: Good implementation of persistent idempotency key.Adding the
IdempotencyKeyfield to the webhook struct implements the persistent storage approach described in the PR objectives. This addresses the issue where Temporal workflows aren't retained indefinitely, which could lead to duplicate processing of replayed webhooks.
67-67: Proper model mapping for idempotency key.The
IdempotencyKeyfield is correctly mapped between the model and storage structs.
78-78: Model to storage mapping is consistent.The idempotency key field is consistently mapped in both directions between the model and storage structs, which ensures data integrity.
19-21:✅ Verification successful
Consider adding a unique constraint.
The field reordering looks good. For proper idempotency enforcement, ensure there's a unique constraint on (connector_id, idempotency_key) at the database level.
🏁 Script executed:
#!/bin/bash # Check if the migration includes a unique constraint for the idempotency key # Look for migration files related to webhooks idempotency fd -t f -e sql webhooks-idempotency -X cat {} | grep -i "unique\|constraint\|index"Length of output: 184
Unique constraint confirmed
The migration adds a unique index on (connector_id, idempotency_key), satisfying the idempotency requirement. No further action needed.
- Migration file defines:
CREATE UNIQUE INDEX webhooks_unique_idempotency_key ON webhooks (connector_id, idempotency_key);internal/connectors/plugins/public/bankingcircle/accounts_test.go (2)
18-19: Good abstraction using the models.Plugin interface.Changing the concrete struct to the interface type makes tests more flexible and robust against interface changes.
22-25: Improved test setup pattern.Nice refactoring to initialize the mock client and plugin instance only once in the outer BeforeEach block. This reduces redundancy and centralizes the test setup, making it more maintainable.
internal/connectors/plugins/public/atlar/bank_account_creation_test.go (2)
20-21: Good abstraction using the models.Plugin interface.Using the interface type instead of the concrete implementation improves type checking during tests, which aligns with the PR objective to detect type errors in function overrides.
24-27: Improved test setup organization.Centralizing the mock creation and plugin initialization in the top-level BeforeEach block eliminates redundant setup code and makes tests more maintainable.
internal/connectors/plugins/public/mangopay/payouts_test.go (2)
20-21: Good abstraction using the models.Plugin interface.Using the interface type instead of the concrete struct implementation aligns with the PR objective to improve type checking in tests.
24-27: Improved test setup organization.Centralizing mock creation and plugin initialization in the top-level BeforeEach follows a consistent pattern across plugin tests, improving maintainability.
internal/storage/migrations/migrations.go (2)
31-32: New migration for webhook idempotency key.This embedding of the SQL file supports the webhook idempotency feature mentioned in the PR objectives.
219-229:✅ Verification successful
Well-structured migration implementation.
The migration follows the established pattern with proper transaction handling and logging. This addresses the PR objective to shift webhook idempotency checks from Temporal to persistent storage.
However, I'd recommend verifying that the migration includes a proper index on the idempotency_key column for performance, since lookups will happen frequently during webhook processing.
🏁 Script executed:
#!/bin/bash # Examine the SQL migration content to verify the index creation cat internal/storage/migrations/16-webhooks-idempotency-key.sqlLength of output: 235
Index creation verified for webhook idempotency migration
The SQL migration not only adds the
idempotency_keycolumn but also creates a unique index (webhooks_unique_idempotency_key) on(connector_id, idempotency_key), ensuring efficient lookups. This addresses the performance concern—LGTM.internal/connectors/plugins/public/currencycloud/external_accounts_test.go (1)
18-19: Good refactoring to use interface instead of concrete typeThe change from concrete type to interface (
models.Plugininstead of*Plugin) aligns with the PR objective to ensure better type checking in function overrides. This will help catch signature mismatches at compile time rather than runtime.The centralized mock setup in the top-level BeforeEach also improves test maintenance by eliminating duplicate mock creation code.
Also applies to: 23-25, 35-35
internal/connectors/plugins/public/modulr/external_accounts_test.go (1)
18-19: Good refactoring to use interface instead of concrete typeThe change from concrete type to interface (
models.Plugininstead of*Plugin) properly implements the PR objective to use interfaces for better type checking. The centralized mock setup pattern is consistent with the changes in other plugin test files.Also applies to: 23-25
internal/storage/webhooks_test.go (4)
8-8: LGTM: Added pointer package import for idempotency key testsThe pointer package import is necessary for creating string pointer values in the test data.
28-30: LGTM: Updated test webhook data with idempotency keyGood update to include an idempotency key in the test data to support testing the new functionality.
83-98: LGTM: Good test coverage for null idempotency keysThis test case verifies that multiple webhooks with null idempotency keys can be stored without conflicts, which is important behavior to validate.
100-117: LGTM: Good test coverage for duplicate idempotency keysThis test case properly verifies that the database enforces uniqueness on the idempotency key column, which is a critical aspect of the webhook idempotency implementation described in the PR objectives.
internal/connectors/plugins/registry/wrapper.go (1)
298-314: LGTM: Well-implemented VerifyWebhook method with proper tracing and error handlingThe new VerifyWebhook method correctly follows the established pattern for plugin method wrappers:
- Creates and manages OpenTelemetry spans with appropriate attributes
- Provides consistent logging before and after the operation
- Properly handles and translates errors
- Returns the plugin's response
This implementation supports the PR objective of adding webhook verification that generates idempotency keys to prevent duplicate processing.
internal/connectors/plugins/public/wise/transfers_test.go (1)
20-27: Using plugin interface over concrete implementation improves test robustness.The changes refactor the test setup to use the
models.Plugininterface type instead of the concrete implementation, which aligns with the PR objective to make it easier to catch type errors in function overrides. The mock setup is now centralized in the top-levelBeforeEachblock.internal/connectors/plugins/public/modulr/balances_test.go (1)
16-24: Clean refactoring to use plugin interface and centralize mock setup.This change appropriately moves the mock client initialization and plugin creation to the top-level
BeforeEachblock and changes from using a concrete Plugin instance to themodels.Plugininterface. This matches the PR goal of ensuring proper interface compliance and improving type checking.internal/connectors/plugins/public/adyen/accounts_test.go (1)
18-27: Improved test structure using plugin interface for type safety.The test setup has been refactored to use the
models.Plugininterface rather than the concrete Plugin struct, and mock initialization has been centralized in the top-levelBeforeEach. This change ensures proper type checking of interface implementations and is consistent with changes across other plugin tests.internal/connectors/engine/workflow/handle_webhooks.go (2)
48-65: Added webhook verification step for improved idempotency handling.This change implements the new webhook verification mechanism that shifts idempotency enforcement from Temporal workflows to persistent storage. The workflow now calls the
PluginVerifyWebhookactivity to verify the webhook and obtain an idempotency key, which is then stored with the webhook in the database.
94-94: Updated workflow ID generation to use webhook ID.The workflow ID for storing webhook translations now uses the webhook's ID instead of the idempotency key, which aligns with the shift from Temporal-based to storage-based idempotency control.
internal/connectors/plugins/public/bankingcircle/payouts_test.go (1)
19-20: Improved test setup pattern using interface types.The test refactoring now properly uses the
models.Plugininterface type instead of the concretePluginstruct, which aligns with the PR's goal of ensuring type-checking for plugin interface implementations. This change helps detect type errors in function overrides that might be missed when using the concrete type.Also applies to: 24-26
internal/connectors/plugins/public/bankingcircle/balances_test.go (1)
15-16: LGTM: Consistent test setup pattern.This change follows the same improved pattern of using
models.Plugininterface instead of concrete types. The simplified test setup with a centralized mock client creation enhances maintainability.Also applies to: 20-22
internal/connectors/plugins/public/atlar/payouts_test.go (1)
24-25: Consistent interface-based testing approach.Good refactoring to use the
models.Plugininterface type in tests, consistent with other plugin test changes. This ensures the test validates that the plugin correctly implements all interface methods with proper type signatures.Also applies to: 29-31
internal/connectors/plugins/public/atlar/payments_test.go (1)
23-24: Clean interface-based test refactoring.This change aligns with the PR objective of using interface types in tests to ensure proper type checking. The mock client is now created once per test run and injected consistently.
Also applies to: 28-30
internal/connectors/plugins/public/stripe/accounts_test.go (2)
17-19: Good refactoring to use the models.Plugin interfaceThis change correctly implements the interface-based approach by declaring
plgasmodels.Pluginrather than using the concrete struct type. This ensures proper type-checking of all implemented interface methods.
21-25: Improved test setup organizationGood job centralizing the mock client initialization and plugin instantiation in the outer BeforeEach block. This removes redundant setup code in nested contexts and makes the test structure cleaner and more maintainable.
internal/connectors/plugins/public/mangopay/external_accounts_test.go (2)
18-19: Good refactoring to use the models.Plugin interfaceCorrectly declaring
plgasmodels.Pluginrather than a concrete struct type ensures proper interface compliance and type-checking of all implemented methods, which aligns with the PR objective to improve plugin test accuracy.
22-26: Improved test structure with centralized mock initializationCentralizing the mock client creation and plugin instantiation in the top-level BeforeEach block is a good practice. This removes redundant setup code and ensures consistent mock usage across all test cases.
internal/connectors/plugins/public/mangopay/balances_test.go (2)
17-19: Good refactoring to use the models.Plugin interfaceUsing the
models.Plugininterface type rather than a concrete struct ensures proper type-checking of implemented methods, which helps catch potential type errors in function overrides.
21-25: Improved test setup organizationGood job centralizing the mock client initialization and plugin instantiation in the outer BeforeEach block. This eliminates redundant setup code and ensures consistent use of the mock client across tests.
internal/connectors/plugins/public/currencycloud/payouts_test.go (2)
19-20: Good refactoring to use the models.Plugin interfaceCorrectly changing the plugin type from a concrete struct to the
models.Plugininterface ensures proper type-checking of all implemented interface methods, making the tests more robust at detecting function signature mismatches.
23-27: Improved test structure with centralized mock initializationCentralizing the mock client initialization and plugin instantiation in the top-level BeforeEach block removes duplicate code and creates a more maintainable test structure. This is consistent with the refactoring pattern applied across all plugin test suites.
internal/connectors/plugins/public/bankingcircle/payments_test.go (1)
18-20: Good refactoring to use interface typing and centralized mock setup.This change improves the test by:
- Using
models.Plugininterface type instead of a concrete struct type- Creating the mock client once in the outer BeforeEach
- Properly injecting the mock into the plugin instance
This will ensure type checking at compile time for all interface methods and reduces redundant mock initialization in nested BeforeEach blocks.
Also applies to: 23-25
internal/connectors/plugins/public/column/payments_test.go (1)
19-20: Good refactoring to use interface typing and centralized mock setup.The test has been improved by:
- Using
models.Plugininterface type instead of a concrete struct type- Creating the mock HTTP client once in the outer BeforeEach
- Properly initializing the client with the mock and injecting it into the plugin
This ensures proper type checking of interface methods and reduces redundant initialization code in nested contexts.
Also applies to: 24-28
internal/connectors/plugins/public/modulr/payments_test.go (2)
20-21: Good refactoring to use interface typing and centralized mock setup.This change improves the test by:
- Using
models.Plugininterface type instead of a concrete struct type- Creating the mock client once in the outer BeforeEach
- Properly injecting the mock into the plugin instance
This will ensure type checking at compile time for all interface methods and reduces redundant mock initialization in nested BeforeEach blocks.
Also applies to: 25-27
210-210: Corrected context description.Fixed the context description from "fetching next accounts" to "transaction to payments" to accurately reflect what the tests are verifying.
internal/connectors/plugins/public/column/balances_test.go (1)
16-17: Good refactoring to use interface typing and centralized mock setup.The test has been improved by:
- Using
models.Plugininterface type instead of a concrete struct type- Creating the mock HTTP client once in the outer BeforeEach
- Properly initializing the client with the mock and injecting it into the plugin
This ensures proper type checking of interface methods and reduces redundant initialization code in nested contexts.
Also applies to: 21-25
internal/connectors/plugins/public/wise/payouts_test.go (2)
20-21: Refactored test variables to use interface type instead of concrete implementationThe test now declares variables at the top level with
plgtyped asmodels.Plugininstead of a concrete*Plugintype, improving type safety by ensuring tests work against the interface.
25-27: Improved test setup with centralized mock initializationThe test now initializes the mock client and plugin instance once in the top-level
BeforeEach, which reduces duplication and follows best practices for test organization. This change is part of the PR objective to use the interface type for plugins in tests to ensure function overrides are correctly type-checked.internal/connectors/plugins/public/bankingcircle/bank_account_creation_test.go (2)
18-19: Refactored test variables to use interface type instead of concrete implementationThe test now declares variables at the top level with
plgtyped asmodels.Plugininstead of a concrete*Plugintype, improving type safety by ensuring tests work against the interface.
23-25: Improved test setup with centralized mock initializationThe test now initializes the mock client and plugin instance once in the top-level
BeforeEach, which reduces duplication and follows best practices for test organization. This change is part of the PR objective to use the interface type for plugins in tests to ensure function overrides are correctly type-checked.internal/connectors/plugins/public/modulr/accounts_test.go (2)
18-19: Refactored test variables to use interface type instead of concrete implementationThe test now declares variables at the top level with
plgtyped asmodels.Plugininstead of a concrete*Plugintype, improving type safety by ensuring tests work against the interface.
23-25: Improved test setup with centralized mock initializationThe test now initializes the mock client and plugin instance once in the top-level
BeforeEach, which reduces duplication and follows best practices for test organization. This change is part of the PR objective to use the interface type for plugins in tests to ensure function overrides are correctly type-checked.internal/connectors/plugins/public/stripe/reverse_transfers_test.go (2)
21-22: Refactored test variables to use interface type instead of concrete implementationThe test now declares variables at the top level with
plgtyped asmodels.Plugininstead of a concrete*Plugintype, improving type safety by ensuring tests work against the interface.
25-27: Improved test setup with centralized mock initializationThe test now initializes the mock client and plugin instance once in the top-level
BeforeEach, which reduces duplication and follows best practices for test organization. This change is part of the PR objective to use the interface type for plugins in tests to ensure function overrides are correctly type-checked.internal/connectors/plugins/public/mangopay/transfers_test.go (2)
20-21: Good use of the interface type for plugin variable.Changing from a concrete type to the interface type (
models.Plugin) aligns with the PR objective of improving type checking for plugin tests.
25-27: Improved test structure with centralized mock setup.Centralizing the mock client creation and plugin instantiation in the top-level
BeforeEachis a good practice. This eliminates redundant setup code and makes the tests more maintainable.internal/connectors/plugins/public/currencycloud/balances_test.go (2)
16-17: Good use of the interface type for plugin variable.Changing from a concrete type to the interface type (
models.Plugin) aligns with the PR objective of improving type checking for plugin tests.
21-23: Improved test structure with centralized mock setup.Centralizing the mock client creation and plugin instantiation in the top-level
BeforeEachis a good practice. This eliminates redundant setup code and makes the tests more maintainable.internal/connectors/plugins/public/generic/accounts_test.go (2)
19-20: Good use of the interface type for plugin variable.Changing from a concrete type to the interface type (
models.Plugin) aligns with the PR objective of improving type checking for plugin tests.
24-26: Improved test structure with centralized mock setup.Centralizing the mock client creation and plugin instantiation in the top-level
BeforeEachis a good practice. This eliminates redundant setup code and makes the tests more maintainable.internal/connectors/plugins/public/bankingcircle/transfers_test.go (3)
19-20: Good use of the interface type for plugin variable.Changing from a concrete type to the interface type (
models.Plugin) aligns with the PR objective of improving type checking for plugin tests.
24-26: Improved test structure with centralized mock setup.Centralizing the mock client creation and plugin instantiation in the top-level
BeforeEachis a good practice. This eliminates redundant setup code and makes the tests more maintainable.
36-36: Minor spacing change.This empty line addition is just a formatting change with no functional impact.
internal/models/plugin.go (2)
39-39: Good addition of theVerifyWebhookmethod to the interface.This method will enable plugins to verify webhooks and return idempotency keys, supporting the shift from Temporal-based to database-based webhook idempotency tracking.
158-165: LGTM: Well-structured request and response types for webhook verification.The request includes all necessary components (webhook payload and optional config), and the response includes an optional idempotency key pointer which allows plugins to:
- Return a PSP-generated event ID
- Generate a hash from the payload
- Return nil when idempotency isn't needed
This design provides good flexibility for different plugin implementations.
internal/connectors/plugins/public/wise/payments_test.go (2)
19-20: Good refactoring of test variables.Changing
plgfrom concrete type to interface typemodels.Pluginis a good improvement that helps enforce the interface contract and will catch type errors in function implementations.
23-26: Improved test setup organization.Centralizing the mock client creation and plugin initialization in the top-level
BeforeEachimproves test maintainability and avoids redundant setup code in nested test contexts.internal/connectors/plugins/public/mangopay/bank_account_creation_test.go (2)
19-20: Good refactoring of test variables.Changing
plgfrom concrete type to interface typemodels.Pluginhelps enforce the interface contract and will catch type errors in function implementations.
23-26: Improved test setup organization.Centralizing the mock client creation and plugin initialization in the top-level
BeforeEachimproves test maintainability and avoids redundant setup code in nested test contexts.internal/connectors/plugins/public/mangopay/payments_test.go (2)
18-20: Good refactoring of test variables.Changing
plgfrom concrete type to interface typemodels.Pluginhelps enforce the interface contract and will catch type errors in function implementations.
22-26: Improved test setup organization.Centralizing the mock client creation and plugin initialization in the top-level
BeforeEachimproves test maintainability and avoids redundant setup code in nested test contexts.internal/connectors/plugins/public/atlar/accounts_test.go (1)
22-24: Good refactoring to use the plugin interface typeUsing
models.Plugininstead of the concrete struct type improves type safety by ensuring that all interface methods are properly implemented. This change helps catch type errors in function overrides at compile time, aligning with the PR objective.Also applies to: 27-29
internal/connectors/plugins/public/generic/balances_test.go (1)
18-19: Improved test structure with interface-based testingGood refactoring to use the
models.Plugininterface instead of concrete struct types. This approach ensures proper implementation of the interface methods and aligns with the PR objective to make it easier to detect type errors in function overrides.Also applies to: 23-25
internal/connectors/plugins/public/stripe/external_accounts_test.go (1)
17-18: Well-structured interface-based testing approachThe change from concrete struct to
models.Plugininterface type improves type safety and test reliability. This refactoring centralizes mock setup and ensures proper interface implementation, which helps catch potential issues earlier in the development process.Also applies to: 22-24
internal/connectors/plugins/public/wise/external_accounts_test.go (1)
18-19: Good consistent refactoring to use interface typeThis change follows the same pattern as the other plugin test refactorings, using the
models.Plugininterface type instead of concrete struct. This approach ensures proper interface implementation and type checking, aligning with the PR objective to improve plugin testing.Also applies to: 23-25
internal/connectors/plugins/public/wise/accounts_ 10000 test.go (1)
18-19: Good refactoring to use interface type instead of concrete struct.The change from concrete type to
models.Plugininterface improves type safety by ensuring the plugin correctly implements all interface methods. Centralizing the mock client creation in the top-level BeforeEach eliminates redundant initialization code that was likely present in nested contexts before.Also applies to: 23-25
internal/connectors/plugins/public/stripe/create_payouts_test.go (1)
21-22: Good refactoring to use interface type instead of concrete struct.The change from concrete type to
models.Plugininterface improves type safety by ensuring the plugin correctly implements all interface methods. Centralizing the mock client creation in the top-level BeforeEach eliminates redundant initialization code.Also applies to: 26-28
internal/connectors/plugins/public/atlar/balances_test.go (1)
20-21: Good refactoring to use interface type instead of concrete struct.The change from concrete type to
models.Plugininterface improves type safety and consistency. This ensures that the plugin correctly implements all required interface methods, which addresses one of the PR objectives.Also applies to: 25-27
internal/connectors/plugins/public/mangopay/users_test.go (1)
18-19: Good refactoring to use interface type instead of concrete struct.The change from concrete type to
models.Plugininterface ensures proper type checking for function overrides. This directly addresses one of the PR objectives to make it easier to detect type errors in function overrides.Also applies to: 23-25
internal/connectors/plugins/public/currencycloud/transfers_test.go (2)
19-21: Approved: Using the interface type improves type checkingThe change from concrete type to the
models.Plugininterface allows the compiler to verify that all interface methods are correctly implemented, which aligns with the PR objective of improving type checking.
24-27: Good refactoring of test setupMoving the mock client creation and plugin initialization to the top-level BeforeEach centralizes the setup code and removes potential duplication in nested contexts. This improves test maintainability.
internal/connectors/engine/activities/plugin_verify_webhook_test.go (2)
21-86: Well-structured test suite for the new webhook verification functionalityThis test suite thoroughly covers the new
PluginVerifyWebhookactivity with appropriate test cases:
- Success case that verifies the activity correctly forwards webhook verification to the plugin
- Error handling for general plugin errors (retryable)
- Error handling for "not implemented" errors (non-retryable)
This aligns perfectly with the PR objectives of implementing webhook idempotency via the new
VerifyWebhookplugin function.
57-62: Verify correct handling of the webhook idempotency keyThe test correctly verifies that the idempotency key from the plugin's response is passed through by the activity.
internal/connectors/plugins/public/generic/payments_test.go (2)
20-21: Improved type checking with interface typeChanging from the concrete
*Plugintype to themodels.Plugininterface ensures that the compiler verifies all interface methods are correctly implemented, supporting the PR's goal of better type checking in plugin tests.
24-28: Centralized mock setup improves test clarityMoving the mock creation and plugin initialization to the top-level BeforeEach follows a consistent pattern applied across multiple plugin tests, reducing code duplication and improving maintainability.
internal/connectors/plugins/public/column/accounts_test.go (4)
18-20: Improved type checking with interface typeChanging from concrete type to the
models.Plugininterface ensures that the compiler verifies all interface methods are correctly implemented, supporting the PR's goal of better type checking.
22-28: Well-structured test setupThe test setup now properly initializes the mock HTTP client and injects it into the plugin, providing a consistent pattern that matches other plugin tests in this PR.
49-68: More realistic HTTP error testingThe test now explicitly mocks the HTTP status code (500) and error return, providing a more accurate representation of how the client would behave in error scenarios. The error expectation has been updated to match the new error handling path.
69-98: Improved timestamp parsing error testThis test now correctly mocks a successful HTTP response but with invalid timestamp data, which is a more realistic test scenario. The test verifies that the plugin properly handles and propagates parsing errors from invalid response data.
internal/connectors/plugins/public/mangopay/accounts_test.go (2)
18-19: Improved testability by using interface type instead of concrete struct.Changed variable declaration from concrete plugin type to a more testable approach with separate mock client and plugin interface variables.
23-25: Well-structured test setup with centralized mock initialization.Good refactoring to initialize the mock controller and client once in the outer BeforeEach block, and injecting the mock into the plugin instance. This eliminates duplicate initialization in nested test blocks and improves test maintenance.
internal/connectors/plugins/public/atlar/external_accounts_test.go (2)
22-23: Improved testability by using interface type instead of concrete struct.Changed variable declaration from concrete plugin type to a more testable approach with separate mock client and plugin interface variables.
27-29: Well-structured test setup with centralized mock initialization.Good refactoring to initialize the mock controller and client once in the outer BeforeEach block, and injecting the mock into the plugin instance. This eliminates duplicate initialization in nested test blocks and improves test maintenance.
internal/connectors/plugins/public/moneycorp/payouts_test.go (2)
19-20: Improved testability by using interface type instead of concrete struct.Changed variable declaration from concrete plugin type to a more testable approach with separate mock client and plugin interface variables.
24-26: Well-structured test setup with centralized mock initialization.Good refactoring to initialize the mock controller and client once in the outer BeforeEach block, and injecting the mock into the plugin instance. This eliminates duplicate initialization in nested test blocks and improves test maintenance.
internal/connectors/plugins/public/generic/external_accounts_test.go (2)
19-20: Improved testability by using interface type instead of concrete struct.Changed variable declaration from concrete plugin type to a more testable approach with separate mock client and plugin interface variables.
24-26: Well-structured test setup with centralized mock initialization.Good refactoring to initialize the mock controller and client once in the outer BeforeEach block, and injecting the mock into the plugin instance. This eliminates duplicate initialization in nested test blocks and improves test maintenance.
internal/connectors/plugins/public/stripe/create_transfers_test.go (1)
21-29: Improved test structure using interface typesGood refactoring to use the
models.Plugininterface instead of the concrete struct type. This change ensures proper type checking of the plugin interface implementation and centralizes the mock client setup in the top-level BeforeEach, reducing duplication and improving maintainability.internal/connectors/plugins/public/modulr/payouts_test.go (1)
19-27: Improved test structure using interface typesThis refactoring correctly uses the
models.Plugininterface instead of the concrete struct type, which ensures proper type checking of the plugin interface implementation. Centralizing the mock client setup in the top-level BeforeEach is a good practice that reduces duplication.internal/connectors/plugins/public/currencycloud/payments_test.go (2)
22-30: Improved test structure using interface typesGood refactoring to use the
models.Plugininterface instead of the concrete struct type. This ensures proper type checking of the plugin interface implementation and centralizes the mock client setup, reducing duplication in nested test contexts.
39-54: Great handling of samples creation in test setupThe test setup for sample transactions is well structured. Creating a loop to generate 50 sample transactions with incrementing values makes the test data more realistic and provides good coverage for pagination testing.
internal/connectors/plugins/public/currencycloud/accounts_test.go (2)
18-19: Good standardization of test variables.Refactoring the mock client and plugin declarations to the top-level test suite scope improves test organization and eliminates duplicated setup code. Using the
models.Plugininterface instead of the concrete type helps ensure proper interface adherence.
23-26: Good centralized setup pattern.Centralizing the mock controller, mock client, and plugin initialization in the top-level BeforeEach improves test maintainability and reduces code duplication. The initialization pattern ensures consistent test setup across all test contexts.
internal/connectors/plugins/public/column/transfers_test.go (4)
18-19: Good standardization of test variables.Declaring the mock HTTP client and plugin at the test suite level using the
models.Plugininterface type improves test organization and ensures proper interface adherence.
23-27: Good centralized setup pattern.Centralizing the mock controller creation, HTTP client initialization, and plugin setup ensures consistent test environment across all test cases, reducing duplication and improving maintainability.
33-40: Improved test approach using public API.Converting the tests to use the public
CreateTransfermethod instead of the internalvalidateTransferRequestmethod is a significant improvement. This ensures that tests validate the actual API contract that clients will use rather than internal implementation details.Also applies to: 44-57, 61-81, 85-92, 96-104
345-352: Good test isolation pattern.Creating a local plugin variable for specific test contexts helps isolate test conditions. Using an invalid URL to test error handling is a good approach to verify error propagation from HTTP client creation failures.
internal/connectors/plugins/public/column/external_accounts_test.go (2)
18-19: Good standardization of test variables.Declaring the mock HTTP client and plugin at the test suite level using the
models.Plugininterface type improves test organization and ensures proper interface adherence.
23-27: Good centralized setup pattern.Centralizing the mock controller creation, HTTP client initialization, and plugin setup in the top-level BeforeEach ensures consistent test setup across all test cases. This reduces code duplication and improves test maintainability.
internal/models/plugin_generated.go (2)
297-310: Good implementation of mock for new VerifyWebhook method.The implementation of the
VerifyWebhookmock method inMockPluginfollows the established pattern for mock methods in this file. This supports the new webhook verification functionality being added to the plugin interface.
546-559: Good implementation of mock for new VerifyWebhook method in PSPPlugin.The implementation of the
VerifyWebhookmock method inMockPSPPluginproperly mirrors the implementation inMockPlugin, maintaining consistency across the mock implementations. This supports the webhook idempotency key mechanism being introduced in this PR.internal/connectors/plugins/public/column/webhook.go (1)
330-359: Clean separation of translation from verificationThe updated
translateWebhookmethod now focuses solely on retrieving the appropriate webhook translator and translating the event. This is a good separation of concerns from the verification logic, making the code more maintainable and focused.internal/connectors/engine/activities/plugin_verify_webhook.go (2)
10-13: Well-structured request with clear separation of concernsThe
VerifyWebhookRequeststruct properly encapsulates the connector ID and verification request data needed for the activity.
15-26: Good error handling for plugin verification activityThe
PluginVerifyWebhookmethod properly retrieves the plugin by connector ID and delegates to the plugin'sVerifyWebhookmethod, with appropriate error wrapping.internal/connectors/engine/workflow/handle_webhooks_test.go (6)
26-34: Good test coverage for idempotency key propagationThe test correctly mocks the
PluginVerifyWebhookActivityto return an idempotency key and verifies that it's correctly propagated to the webhook storage.
89-97: Consistent testing approach for no-responses caseThis test also properly checks idempotency key propagation, maintaining consistency with the success test.
180-215: Good error case testing for webhook verificationThe test appropriately verifies that errors from the webhook verification activity are propagated to the workflow.
228-229: Consistent mocking for StorageWebhooksStore testThe test case has been correctly updated to include the verification activity.
265-266: Consistent mocking in TranslateWebhook testThe test case has been properly updated to include the verification activity.
303-304: Consistent mocking in RunStoreWebhookTranslation testThe test case has been properly updated to include the verification activity.
internal/connectors/plugins/public/mangopay/webhooks_test.go (4)
20-23: Good refactoring to use interface typeChanging from concrete
*Plugintype tomodels.Plugininterface makes the tests more robust by detecting potential type errors in implementations.
24-30: Improved test setup with centralized mock configurationCentralizing mock client creation and plugin initialization in the top-level
BeforeEachblock reduces duplication and improves test maintainability.
215-215: Updated assertion for length checkThe code now correctly uses the local plugin instance to verify the length of returned webhook configs.
226-232: Consistent test setup patternThe mock setup follows the same pattern as the previous test context, centralizing client mock creation and plugin initialization.
internal/connectors/plugins/public/wise/plugin.go (2)
175-205: Well-structured webhook verification implementation.The new
VerifyWebhookmethod properly separates verification logic from translation, following the single responsibility principle. The method correctly validates headers and verifies signatures before returning an idempotency key derived from theX-Delivery-Idheader.This implementation aligns with the PR objective of shifting idempotency checks to persistent storage by returning a unique identifier from the webhook provider.
207-232: Clean separation of translation concerns.The
TranslateWebhookmethod has been properly refactored to focus solely on translating webhook payloads, removing signature verification and idempotency key assignment that are now handled in the newVerifyWebhookmethod.This separation of concerns improves maintainability and allows for more robust idempotency handling through persistent storage.
internal/connectors/plugins/public/column/reverse_payout_test.go (3)
17-27: Good switch to interface-based testing.Changing the
plgvariable from a concretePlugintype to themodels.Plugininterface improves test maintainability and follows good testing practices by programming to interfaces rather than implementations.The centralized mock setup in the top-level
BeforeEachalso reduces duplication and makes the tests more maintainable.
31-40: Improved test methodology using public API.The test now correctly uses the public
ReversePayoutmethod with a properReversePayoutRequeststruct instead of calling internal validation methods directly. This better reflects how the code will be used in production and ensures the complete call path is tested.
114-123: Better test structure with proper initialization.The HTTP Request Creation Errors test context now follows the same initialization pattern as other tests, improving consistency across the test suite.
internal/connectors/plugins/public/column/payouts_test.go (2)
16-26: Good switch to interface-based testing with centralized setup.Changing the
plgvariable to themodels.Plugininterface type and centralizing the mock setup in the top-levelBeforeEachimproves test maintainability and follows the dependency inversion principle.
29-36: Improved test methodology using public API.The validation tests now correctly use the public
CreatePayoutmethod with a proper request struct instead of calling internal validation methods directly. This ensures the tests verify the behavior as experienced by consumers of the API.internal/connectors/plugins/public/adyen/webhooks_test.go (3)
21-32: Good switch to interface-based testing with proper initialization.Changing
plgto themodels.Plugininterface type and properly initializing the plugin improves test maintainability and follows good testing practices.
70-168: Well-structured tests for the new verification functionality.The new "verifying webhooks" test context thoroughly tests the webhook verification functionality, including basic auth failure, invalid HMAC, and successful verification scenarios.
The successful verification test correctly ensures that a SHA-256 hash of the webhook body is returned as the idempotency key, which aligns with the PR objective of implementing robust idempotency checking.
461-509: Clean helper function refactoring.The
doTranslateCallhelper function has been properly updated to work with the interface type and now focuses solely on testing translation, as verification is tested separately. This maintains the separation of concerns in both the implementation and tests.
292cdcb to
1791068
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
internal/connectors/plugins/public/wise/plugin_test.go (1)
34-34: 💡 Verification agent🧩 Analysis chain
Consider using models.Plugin interface instead of concrete Plugin struct
The PR objective mentions changing tests to use the
models.Plugininterface instead of the concrete struct to improve type checking in function overrides. However, you're still using the concrete*Plugintype here. Consider:- var plg *Plugin + var plg models.PluginThis change would help catch type errors in plugin interface implementations during tests.
🏁 Script executed:
#!/bin/bash # Verify if there are other test files that have been changed to use models.Plugin echo "Checking other test files for similar changes..." rg -t go "var\s+(\w+).*models\.Plugin" --files-with-matches internal/connectors/plugins/Length of output: 987
🏁 Script executed:
#!/bin/bash echo "Listing plugin_test.go files:" find internal/connectors/plugins -type f -name "plugin_test.go" echo echo "Column plugin_test.go snippet:" sed -n '1,200p' internal/connectors/plugins/public/column/plugin_test.go echo echo "Wise plugin_test.go snippet:" sed -n '1,200p' internal/connectors/plugins/public/wise/plugin_test.go echo echo "Looking for 'var plg' in test files:" rg -n "var plg" internal/connectors/plugins -t go | grep plugin_test.goLength of output: 13174
Use the
models.Plugininterface in the Wise plugin testTo keep consistency with other connector tests (e.g. Column) and to catch interface-level type errors, declare
plgas themodels.Plugininterface rather than the concrete*Pluginstruct.• File: internal/connectors/plugins/public/wise/plugin_test.go
Change in thevarblock at the top of theDescribe("Wise Plugin", …)suite:var ( - plg *Plugin + plg models.Plugin block *pem.Block pemKey *bytes.Buffer privatekey *rsa.PrivateKey logger = logging.NewDefaultLogger(GinkgoWriter, true, false, false) )internal/connectors/plugins/public/stripe/payments_test.go (1)
19-20: 💡 Verification agent🧩 Analysis chain
Plugin variable still uses concrete type
The PR objective mentioned changing tests to use the
models.Plugininterface instead of concrete structs, butplgis still typed as*Plugin(concrete type) rather thanmodels.Plugin(interface).
🏁 Script executed:
#!/bin/bash # Check if other test files in this PR have been changed to use the models.Plugin interface rg -A 1 -B 1 "var \(\s*\w+\s+models\.Plugin" --type goLength of output: 56
I’ll pull in the import context from the test and locate the
models.Plugininterface so we can confirm the correct import path and update.
🏁 Script executed:
#!/bin/bash # Show imports and the `plg` declaration in payments_test.go echo "=== payments_test.g 3D11 o (lines 1–100) ===" sed -n '1,100p' internal/connectors/plugins/public/stripe/payments_test.go echo # Find where models.Plugin is defined to get its package path echo "=== models.Plugin interface definition ===" rg "type Plugin interface" -n --type goLength of output: 3537
Use the
models.Plugininterface forplginstead of the concrete*PluginTo align with the PR’s objective of depending on the
models.Plugininterface, update the declaration inpayments_test.go:• File: internal/connectors/plugins/public/stripe/payments_test.go
Change in the top-levelvarblock:- plg *Plugin + plg models.PluginThis lets you assign the concrete
&Plugin{…}inBeforeEachwhile keeping tests focused on the interface.
🧹 Nitpick comments (1)
internal/connectors/plugins/public/wise/plugin_test.go (1)
157-172: Consider testing the transfer_state_changed webhook verificationYou currently only test verification for "balance_update" webhooks. Consider adding similar tests for "transfer_state_changed" webhooks to ensure all supported webhook types are verified correctly.
+ It("it can create the transfer_state_changed webhook", func(ctx SpecContext) { + req := models.VerifyWebhookRequest{ + Config: &models.WebhookConfig{Name: "transfer_state_changed"}, + Webhook: models.PSPWebhook{ + Body: body, + Headers: map[string][]string{ + HeadersDeliveryID: {"different-delivery-id"}, + HeadersSignature: {base64.StdEncoding.EncodeToString(signature)}, + }, + }, + } + + res, err := plg.VerifyWebhook(ctx, req) + Expect(err).To(BeNil()) + Expect(res.WebhookIdempotencyKey).To(Equal(&req.Webhook.Headers[HeadersDeliveryID][0])) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
internal/connectors/plugins/public/adyen/accounts_test.go(1 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(2 hunks)internal/connectors/plugins/public/adyen/webhooks.go(3 hunks)internal/connectors/plugins/public/adyen/webhooks_test.go(7 hunks)internal/connectors/plugins/public/atlar/accounts_test.go(1 hunks)internal/connectors/plugins/public/atlar/balances_test.go(1 hunks)internal/connectors/plugins/public/atlar/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/atlar/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/atlar/payments_test.go(1 hunks)internal/connectors/plugins/public/atlar/payouts_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/accounts_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/balances_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/payments_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/payouts_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/transfers_test.go(1 hunks)internal/connectors/plugins/public/column/accounts_test.go(3 hunks)internal/connectors/plugins/public/column/balances_test.go(1 hunks)internal/connectors/plugins/public/column/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/column/client/error.go(1 hunks)internal/connectors/plugins/public/column/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/column/payments_test.go(1 hunks)internal/connectors/plugins/public/column/payouts_test.go(5 hunks)internal/connectors/plugins/public/column/reverse_payout_test.go(2 hunks)internal/connectors/plugins/public/column/transfers_test.go(1 hunks)internal/connectors/plugins/public/column/uninstall_test.go(5 hunks)internal/connectors/plugins/public/column/webhook.go(1 hunks)internal/connectors/plugins/public/column/webhook_test.go(5 hunks)internal/connectors/plugins/public/currencycloud/accounts_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/balances_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/payments_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/payouts_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/transfers_test.go(1 hunks)internal/connectors/plugins/public/generic/accounts_test.go(1 hunks)internal/connectors/plugins/public/generic/balances_test.go(1 hunks)internal/connectors/plugins/public/generic/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/generic/payments_test.go(1 hunks)internal/connectors/plugins/public/mangopay/accounts_test.go(1 hunks)internal/connectors/plugins/public/mangopay/balances_test.go(1 hunks)internal/connectors/plugins/public/mangopay/bank_account_creation_test.go(1 hunks)internal/connectors/plugins/public/mangopay/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/mangopay/payments_test.go(1 hunks)internal/connectors/plugins/public/mangopay/payouts_test.go(1 hunks)internal/connectors/plugins/public/mangopay/transfers_test.go(1 hunks)internal/connectors/plugins/public/mangopay/users_test.go(1 hunks)internal/connectors/plugins/public/mangopay/webhooks_test.go(2 hunks)internal/connectors/plugins/public/modulr/accounts_test.go(1 hunks)internal/connectors/plugins/public/modulr/balances_test.go(1 hunks)internal/connectors/plugins/public/modulr/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/modulr/payments_test.go(2 hunks)internal/connectors/plugins/public/modulr/payouts_test.go(1 hunks)internal/connectors/plugins/public/modulr/transfers_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/accounts_test.go(2 hunks)internal/connectors/plugins/public/moneycorp/balances_test.go(2 hunks)internal/connectors/plugins/public/moneycorp/external_accounts_test.go(2 hunks)internal/connectors/plugins/public/moneycorp/payments_test.go(5 hunks)internal/connectors/plugins/public/moneycorp/payouts_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/transfers_test.go(1 hunks)internal/connectors/plugins/public/stripe/accounts_test.go(1 hunks)internal/connectors/plugins/public/stripe/balances_test.go(1 hunks)internal/connectors/plugins/public/stripe/create_payouts_test.go(1 hunks)internal/connectors/plugins/public/stripe/create_transfers_test.go(1 hunks)internal/connectors/plugins/public/stripe/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/stripe/payments_test.go(2 hunks)internal/connectors/plugins/public/stripe/reverse_transfers_test.go(1 hunks)internal/connectors/plugins/public/wise/accounts_test.go(1 hunks)internal/connectors/plugins/public/wise/balances_test.go(1 hunks)internal/connectors/plugins/public/wise/external_accounts_test.go(1 hunks)internal/connectors/plugins/public/wise/payments_test.go(1 hunks)internal/connectors/plugins/public/wise/payouts_test.go(1 hunks)internal/connectors/plugins/public/wise/plugin_test.go(3 hunks)internal/connectors/plugins/public/wise/profiles_test.go(1 hunks)internal/connectors/plugins/public/wise/transfers_test.go(1 hunks)internal/connectors/plugins/public/wise/uninstall_test.go(1 hunks)internal/connectors/plugins/public/wise/webhooks_test.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/connectors/plugins/public/column/client/error.go
🚧 Files skipped from review as they are similar to previous changes (68)
- internal/connectors/plugins/public/atlar/accounts_test.go
- internal/connectors/plugins/public/mangopay/external_accounts_test.go
- internal/connectors/plugins/public/moneycorp/transfers_test.go
- internal/connectors/plugins/public/mangopay/webhooks_test.go
- internal/connectors/plugins/public/mangopay/balances_test.go
- internal/connectors/plugins/public/modulr/balances_test.go
- internal/connectors/plugins/public/stripe/accounts_test.go
- internal/connectors/plugins/public/stripe/balances_test.go
- internal/connectors/plugins/public/generic/external_accounts_test.go
- internal/connectors/plugins/public/mangopay/bank_account_creation_test.go
- internal/connectors/plugins/public/generic/accounts_test.go
- internal/connectors/plugins/pub 7438 lic/bankingcircle/transfers_test.go
- internal/connectors/plugins/public/wise/payouts_test.go
- internal/connectors/plugins/public/mangopay/payments_test.go
- internal/connectors/plugins/public/atlar/payouts_test.go
- internal/connectors/plugins/public/currencycloud/external_accounts_test.go
- internal/connectors/plugins/public/bankingcircle/payouts_test.go
- internal/connectors/plugins/public/generic/payments_test.go
- internal/connectors/plugins/public/atlar/external_accounts_test.go
- internal/connectors/plugins/public/currencycloud/accounts_test.go
- internal/connectors/plugins/public/wise/profiles_test.go
- internal/connectors/plugins/public/column/webhook.go
- internal/connectors/plugins/public/wise/payments_test.go
- internal/connectors/plugins/public/wise/uninstall_test.go
- internal/connectors/plugins/public/moneycorp/payments_test.go
- internal/connectors/plugins/public/moneycorp/accounts_test.go
- internal/connectors/plugins/public/modulr/external_accounts_test.go
- internal/connectors/plugins/public/atlar/payments_test.go
- internal/connectors/plugins/public/wise/transfers_test.go
- internal/connectors/plugins/public/generic/balances_test.go
- internal/connectors/plugins/public/mangopay/payouts_test.go
- internal/connectors/plugins/public/atlar/bank_account_creation_test.go
- internal/connectors/plugins/public/adyen/accounts_test.go
- internal/connectors/plugins/public/stripe/reverse_transfers_test.go
- internal/connectors/plugins/public/bankingcircle/bank_account_creation_test.go
- internal/connectors/plugins/public/stripe/create_payouts_test.go
- internal/connectors/plugins/public/mangopay/transfers_test.go
- internal/connectors/plugins/public/column/payments_test.go
- internal/connectors/plugins/public/bankingcircle/accounts_test.go
- internal/connectors/plugins/public/modulr/payouts_test.go
- internal/connectors/plugins/public/column/accounts_test.go
- internal/connectors/plugins/public/wise/external_accounts_test.go
- internal/connectors/plugins/public/moneycorp/external_accounts_test.go
- internal/connectors/plugins/public/wise/webhooks_test.go
- internal/connectors/plugins/public/moneycorp/payouts_test.go
- internal/connectors/plugins/public/wise/balances_test.go
- internal/connectors/plugins/public/column/balances_test.go
- internal/connectors/plugins/public/modulr/payments_test.go
- internal/connectors/plugins/public/column/reverse_payout_test.go
- internal/connectors/plugins/public/moneycorp/balances_test.go
- internal/connectors/plugins/public/column/external_accounts_test.go
- internal/connectors/plugins/public/currencycloud/payments_test.go
- internal/connectors/plugins/public/mangopay/accounts_test.go
- internal/connectors/plugins/public/currencycloud/payouts_test.go
- internal/connectors/plugins/public/bankingcircle/balances_test.go
- internal/connectors/plugins/public/atlar/balances_test.go
- internal/connectors/plugins/public/modulr/accounts_test.go
- internal/connectors/plugins/public/column/payouts_test.go
- internal/connectors/plugins/public/stripe/create_transfers_test.go
- internal/connectors/plugins/public/bankingcircle/payments_test.go
- internal/connectors/plugins/public/currencycloud/transfers_test.go
- internal/connectors/plugins/public/currencycloud/balances_test.go
- internal/connectors/plugins/public/column/transfers_test.go
- internal/connectors/plugins/public/wise/accounts_test.go
- internal/connectors/plugins/public/stripe/external_accounts_test.go
- internal/connectors/plugins/public/modulr/transfers_test.go
- internal/connectors/plugins/public/adyen/webhooks.go
- internal/connectors/plugins/public/mangopay/users_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/connectors/plugins/public/stripe/payments_test.go (1)
internal/connectors/plugins/public/stripe/client/client_generated.go (1)
MockClient(21-25)
internal/connectors/plugins/public/adyen/plugin_test.go (1)
internal/connectors/plugins/public/adyen/client/client_generated.go (1)
NewMockClient(35-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (35)
internal/connectors/plugins/public/adyen/webhooks_test.go (9)
5-6: Good use of crypto libraries for idempotency key generation.Adding the crypto/sha256 and encoding/base64 packages supports the new approach of generating a persistent idempotency key from the webhook payload.
22-25: Excellent improvement using the interface instead of concrete type.Changing from
plg *Plugintoplg models.Pluginenforces the interface contract and will catch type mismatches in function signatures. This addresses one of the PR objectives about fixing plugin tests to use the interface rather than concrete struct.
28-34: Good practice with controller initialization and interface assignment.The initialization logic follows best practices by:
- Creating a controller with proper test lifecycle management
- Initializing the plugin with the mock client
- Assigning the concrete plugin to the interface reference
This ensures plugin methods are called through the interface rather than directly on the concrete type.
37-39: Important fix: Added proper mock controller teardown.Adding the
AfterEachhook withctrl.Finish()ensures that mock expectations are verified at the end of each test, avoiding potential memory leaks and unchecked mock expectations.
76-103: Well-structured test setup for the new webhook verification flow.The test context properly initializes a standard webhook payload that will be used across multiple test cases, making the tests more maintainable and readable.
104-127: Good test coverage for basic auth failure scenario.This test ensures that basic authentication is properly verified before processing webhooks, providing a critical security check.
129-150: Comprehensive test for HMAC verification failure.The test properly:
- Marshals the webhook to JSON
- Sets up the request object
- Verifies basic auth (which succeeds)
- Translates the webhook payload
- Tests that HMAC verification fails
- Ensures no idempotency key is returned on verification failure
This tests the complete verification flow and failure handling.
152-175: Well-implemented success case with idempotency key generation.This test properly verifies the entire successful flow and confirms the idempotency key is correctly generated as a base64-encoded SHA-256 hash of the webhook body. This is a critical component of the PR's objective to move idempotency checks to persistent storage.
468-476: Good refactoring of the helper function signature.Updating
doTranslateCallto accept the interface type (models.Plugin) instead of the concrete type promotes better testing practices and alignment with the PR objectives.internal/connectors/plugins/public/adyen/plugin_test.go (3)
24-25: Good practice: Added mock controller at the suite level.Adding the controller as a suite-level variable enables proper management of mocks across all tests within the suite.
33-34: Properly initialized mock controller with GinkgoT().Using GinkgoT() ensures that test failures are properly reported through the Ginkgo testing framework.
37-39: Important fix: Added mock controller teardown.Adding the
AfterEachhook withctrl.Finish()ensures that mock expectations are verified after each test, avoiding potential memory leaks and ensuring all expected mock calls are made.internal/connectors/plugins/public/column/uninstall_test.go (5)
18-19: Good improvement: Using the interface type for testsChanging from a concrete
Plugintype to the interface typemodels.Pluginhelps ensure type-safety and enforces that the tests use only the public API.
27-28: Properly initialized mock controllerGood practice to initialize the controller at the suite level using
gomock.NewController().
34-38: Improved client initializationBetter structure by creating the client first, then setting the mock HTTP client, and finally assigning to the plugin.
41-43: Added essential mock controller cleanupAdding
ctrl.Finish()inAfterEachensures proper cleanup of mock expectations, preventing leaks between tests.
62-62: Using exported method instead of unexportedSwitching from
uninstalltoUninstallensures tests use the public API, maintaining compatibility with the interface contract.Also applies to: 79-79, 137-137, 199-199
internal/connectors/plugins/public/column/bank_account_creation_test.go (4)
16-19: Using the interface type and better variable organizationGood change to use
models.Plugininstead of concrete type and organize mock-related variables at the suite level.
21-27: Centralized mock initializationProperly initializing the mock controller, HTTP client, and plugin in
BeforeEachsimplifies test setup.
29-31: Added proper mock cleanupAdding
ctrl.Finish()inAfterEachensures expectations are verified and resources cleaned up after each test.
33-43: Using public API for testingTests now use the public
CreateBankAccountmethod instead of internal validation functions, better reflecting real usage patterns.internal/connectors/plugins/public/column/webhook_test.go (7)
19-22: Better test structure with interface type and mocksConverting to
models.Plugininterface and properly organizing mock variables improves the test structure.
26-38: Comprehensive test setup with verifier mockGood initialization of all required components, including the new webhook verifier mock, which is essential for testing the webhook verification functionality.
40-42: Proper mock controller cleanupAdding
ctrl.Finish()inAfterEachensures expectations are verified and resources cleaned up after each test.
79-96: Dynamic webhook testing based on plugin configurationInstead of hardcoding webhook test values, the test now loops through all supported webhooks from the plugin, making the test more maintainable and comprehensive.
125-138: Updated to test new webhook verification flowTest now correctly uses the new
VerifyWebhookmethod with the appropriate request structure and validates the expected error message for missing signature header.
163-185: Good test for successful verification with idempotency keyThis test properly validates that the successful verification returns an idempotency key, which is crucial for the webhook deduplication mechanism.
187-201: Specific error testing for unknown webhook typesTest now correctly expects the specific error for unknown webhook types from the plugin.
internal/connectors/plugins/public/wise/plugin_test.go (4)
104-173: Good separation of verification and translation concernsThe new context for "verifying webhook" properly isolates webhook verification tests from translation tests, which aligns with the architectural change to separate these concerns in the plugin interface. The setup with a gomock controller at the context level and proper cleanup in
AfterEachimproves mock lifecycle management.
135-137: Proper mock lifecycle managementGood practice to call
ctrl.Finish()inAfterEachto ensure that all expected mock calls were made and clean up resources. This fixes a common source of test flakiness where mocks aren't properly verified between tests.
175-274: Well-structured test context separationThe restructured "translate webhook" context now focuses solely on webhook translation without verification concerns. This is a good separation of concerns and aligns with the split into
VerifyWebhookandTranslateWebhookin the plugin interface.
206-208: Consistent mock lifecycle managementGood to see consistent mock lifecycle management across both contexts, with the controller created in
BeforeEachandctrl.Finish()called inAfterEach. This consistency improves test reliability and readability.internal/connectors/plugins/public/stripe/payments_test.go (3)
28-29: Improved mock controller lifecycle managementBy moving the
ctrlvariable declaration to the file-level context, it's now accessible from both BeforeEach and AfterEach blocks, enabling proper cleanup.
37-38: Proper instantiation of mock controllerThe controller is now properly initialized with
GinkgoT()and assigned to the test-context variable.
192-194: Added proper mock verification with AfterEachThis is a critical improvement. Adding
ctrl.Finish()in an AfterEach block ensures:
- All expected mock calls are verified
- Resources are properly cleaned up after each test
- Any unfulfilled expectations are reported as test failures
This change aligns with gomock best practices and prevents potential false positives in tests.
f737ff7 to
e1820d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/connectors/plugins/public/column/webhook_test.go (2)
40-42: Fixed previous issue with controller finalizationAdding
ctrl.Finish()in AfterEach properly addresses a previous review comment about preventing lingering mock expectations. This ensures proper cleanup after each test.
124-138: Updated verification error handling with new header nameThe test properly uses the current header name "Column-Signature" in the error message, addressing a previous review comment about outdated header names.
🧹 Nitpick comments (2)
internal/connectors/plugins/public/column/webhook_test.go (2)
80-96: Improved webhook creation test with better maintainabilityIterating over the actual supported webhooks from the plugin is much more maintainable than hardcoding expectations. However, casting back to the concrete type breaks the abstraction provided by using the interface.
Consider adding a method to the interface to access the webhook configuration:
// in models/plugin.go (or appropriate interface file) +GetSupportedWebhooks() map[string]WebhookConfigThen update the test:
-for name, w := range plg.(*Plugin).supportedWebhooks { +for name, w := range plg.GetSupportedWebhooks() {
100-100: Type safety in assertionThis line references the concrete type to get the supportedWebhooks length, which is consistent with line 80 but breaks the interface abstraction.
If you implement the previously suggested
GetSupportedWebhooks()method, this could be updated to:-Expect(res.Others).To(HaveLen(len(plg.(*Plugin).supportedWebhooks))) +Expect(res.Others).To(HaveLen(len(plg.GetSupportedWebhooks())))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/connectors/engine/activities/errors.go(1 hunks)internal/connectors/plugins/public/adyen/webhooks.go(3 hunks)internal/connectors/plugins/public/adyen/webhooks_test.go(7 hunks)internal/connectors/plugins/public/column/webhook.go(1 hunks)internal/connectors/plugins/public/column/webhook_test.go(5 hunks)internal/connectors/plugins/public/wise/plugin.go(2 hunks)internal/models/errors.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/connectors/plugins/public/column/webhook.go
- internal/models/errors.go
- internal/connectors/engine/activities/errors.go
- internal/connectors/plugins/public/adyen/webhooks.go
- internal/connectors/plugins/public/wise/plugin.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (12)
internal/connectors/plugins/public/adyen/webhooks_test.go (8)
5-6: Added required imports for crypto operations.These imports support the new webhook verification functionality, enabling SHA256 hash calculation and base64 encoding for idempotency key generation.
22-24: Switched to interface-based testing.Good change - using the
models.Plugininterface instead of the concretePluginstruct ensures proper type checking of all plugin interface methods, aligning with the PR objective to detect type errors in function overrides.
28-34: Improved test setup with gomock controller.The test now properly initializes the mock controller and configures the plugin, maintaining the concrete implementation while exposing it through the interface. This approach ensures type safety while testing against the interface contract.
37-39: Added proper mock verification.Adding
AfterEachwithctrl.Finish()ensures all mock expectations are verified after each test, preventing potential mock verification leaks between tests.
47-47: Updated method calls to use the plugin interface.All method calls now properly use the interface instead of concrete implementation, ensuring type safety and consistency with the interface-based testing approach.
Also applies to: 57-57, 70-72
76-176: Added comprehensive verification tests for the new webhook verification flow.This test context thoroughly tests the new
VerifyWebhookmethod, covering:
- Authentication failures (basic auth)
- Signature verification failures (HMAC)
- Successful verification with proper idempotency key generation
The idempotency key implementation (SHA256 hash of the webhook body encoded in base64) provides a robust way to uniquely identify webhooks, addressing the issue mentioned in the PR objectives where replayed webhooks could be processed multiple times if the original Temporal workflow was deleted.
470-470: Updated helper function to use the plugin interface.The
doTranslateCallhelper now accepts themodels.Plugininterface instead of a concrete type, making it consistent with the interface-based testing approach.
511-514: Simplified webhook translation test.The test now focuses only on translation functionality since verification has been separated into its own method and test context. This separation of concerns improves test clarity and maintainability.
internal/connectors/plugins/public/column/webhook_test.go (4)
19-22: Interface type improvement for better type safetyThe change from a concrete
Plugintype to the interfacemodels.Pluginaligns with the PR goal to use interface types in tests. This ensures proper type checking of implemented plugin interface functions.
26-38: Well-structured test setup with proper mockingThe refactored test setup nicely centralizes the initialization of the controller, HTTP client, and webhook verifier, making the tests more maintainable and consistent.
163-185: Well-implemented webhook verification success testThis test properly verifies the new idempotency key functionality. The test confirms that webhook verification returns an idempotency key (the webhook event ID "1") when successful.
183-184:✅ Verification successful
Verify idempotency key extraction logic
The test expects the idempotency key to be "1", which appears to be extracted from the webhook payload's
idfield. Ensure this is the correct field to use as the idempotency key according to your design.
🏁 Script executed:
#!/bin/bash # Check where the idempotency key value is extracted from in the implementation rg -A 5 "WebhookIdempotencyKey" --type goLength of output: 7098
Idempotency key extraction logic for Column plugin verified
The Column connector’s
translateWebhookimplementation sets the idempotency key to the payload’sIDfield (&webhook.ID), which aligns with the test’s use ofpointer.For("1"). No changes are necessary.
- internal/connectors/plugins/public/column/webhook.go:
WebhookIdempotencyKey: &webhook.ID
Description
This PR introduces two fixes:
Webhook Idempotency Key
Context
As of today, we have a kind of Idempotency Key implemented for webhooks. It's filled in the response of the TranslateWebhook call to the plugin and is pass inside the workflowID of temporal with a policy of rejecting duplicates in order to not store the responses in case we already processed this event in the past.
This is partially working because we do not retain all workflows in temporal forever. So if a webhook is replayed after the workflow is deleted from temporal, it will process it again.
Solution
The solution I came by is:
Plugins tests
Context
In most of the plugin tests, we are using the Plugin struct to test the main and sub functions. Since this Plugin structure have in composition the base plugin, we can't tests if we make a type in the override of the function, so we need to ensure that all plugin interface function that we implemented are the right ones.
Solution
The solution I came by is:
Fixes PMNT-105
Fixes PMNT-106