feat(events): add task updated events #457
Conversation
WalkthroughThis change introduces a new task update event type, adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Activities
participant Events
participant Publish
Workflow->>Activities: StorageTasksStore(task)
Activities-->>Workflow: Store result
Workflow->>Activities: EventsSendTaskUpdated(task)
Activities->>Events: NewEventUpdatedTask(task)
Events->>Publish: Publish EventMessage (UPDATED_TASK)
Publish-->>Events: Publish result
Activities-->>Workflow: Send event result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
=======================================
Coverage 69.09% 69.10%
=======================================
Files 599 601 +2
Lines 30534 30605 +71
=======================================
+ Hits 21098 21150 +52
- Misses 8323 8339 +16
- Partials 1113 1116 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
internal/connectors/engine/workflow/create_transfer_test.go (1)
183-194: Consistent implementation of task update event validation.internal/connectors/engine/workflow/fetch_accounts_test.go (1)
189-206: Error message standardization.
🧹 Nitpick comments (3)
internal/connectors/engine/workflow/handle_webhooks_test.go (1)
11-11: Remove unused import.The
go.temporal.io/sdk/workflowimport is not used in this test file.- "go.temporal.io/sdk/workflow"internal/events/task.go (1)
22-53: Consider simplifying the inline functions for better readability.The implementation is correct and follows good practices. However, the inline functions could be simplified.
Consider this more concise approach:
func (e Events) NewEventUpdatedTask(task models.Task) publish.EventMessage { + var connectorID *string + if task.ConnectorID != nil { + connectorID = pointer.For(task.ConnectorID.String()) + } + + var errorMsg *string + if task.Error != nil { + errorMsg = pointer.For(task.Error.Error()) + } + payload := taskMessagePayload{ ID: task.ID.String(), - ConnectorID: func() *string { - if task.ConnectorID == nil { - return nil - } - - return pointer.For(task.ConnectorID.String()) - }(), + ConnectorID: connectorID, Status: string(task.Status), CreatedAt: task.CreatedAt, UpdatedAt: task.UpdatedAt, CreatedObjectID: task.CreatedObjectID, - Error: func() *string { - if task.Error == nil { - return nil - } - - return pointer.For(task.Error.Error()) - }(), + Error: errorMsg, }internal/connectors/engine/activities/events_send_task_updated.go (1)
10-18: Add test coverage for the new activity.The implementation follows the established pattern for event activities. However, static analysis indicates that these lines lack test coverage.
Would you like me to generate test cases for this new activity to improve coverage?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-11: internal/connectors/engine/activities/events_send_task_updated.go#L10-L11
Added lines #L10 - L11 were not covered by tests
[warning] 16-17: internal/connectors/engine/activities/events_send_task_updated.go#L16-L17
Added lines #L16 - L17 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
internal/connectors/engine/activities/activity.go(1 hunks)internal/connectors/engine/activities/events_send_task_updated.go(1 hunks)internal/connectors/engine/workflow/create_bank_account_test.go(10 hunks)internal/connectors/engine/workflow/create_payout_test.go(20 hunks)internal/connectors/engine/workflow/create_transfer_test.go(20 hunks)internal/connectors/engine/workflow/create_webhooks_test.go(6 hunks)internal/connectors/engine/workflow/fetch_accounts_test.go(15 hunks)internal/connectors/engine/workflow/fetch_balances_test.go(15 hunks)internal/connectors/engine/workflow/fetch_external_accounts_test.go(15 hunks)internal/connectors/engine/workflow/fetch_others_test.go(11 hunks)internal/connectors/engine/workflow/fetch_payments_test.go(24 hunks)internal/connectors/engine/workflow/handle_webhooks_test.go(10 hunks)internal/connectors/engine/workflow/install_connector_test.go(6 hunks)internal/connectors/engine/workflow/plugin_workflow_test.go(6 hunks)internal/connectors/engine/workflow/poll_payout_test.go(12 hunks)internal/connectors/engine/workflow/poll_transfer_test.go(12 hunks)internal/connectors/engine/workflow/reset_connector_test.go(15 hunks)internal/connectors/engine/workflow/reverse_payout_test.go(36 hunks)internal/connectors/engine/workflow/reverse_transfer_test.go(36 hunks)internal/connectors/engine/workflow/send_events.go(2 hunks)internal/connectors/engine/workflow/send_events_test.go(25 hunks)internal/connectors/engine/workflow/terminate_schedules_test.go(3 hunks)internal/connectors/engine/workflow/terminate_workflows_test.go(4 hunks)internal/connectors/engine/workflow/uninstall_connector_test.go(32 hunks)internal/connectors/engine/workflow/update_payment_initiation_from_payment_test.go(4 hunks)internal/connectors/engine/workflow/update_tasks.go(2 hunks)internal/connectors/plugins/public/atlar/bank_account_creation_test.go(1 hunks)internal/events/task.go(1 hunks)internal/models/tasks.go(1 hunks)pkg/events/event.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/connectors/engine/workflow/update_tasks.go
[warning] 72-73: internal/connectors/engine/workflow/update_tasks.go#L72-L73
Added lines #L72 - L73 were not covered by tests
internal/models/tasks.go
[warning] 35-36: internal/models/tasks.go#L35-L36
Added lines #L35 - L36 were not covered by tests
internal/connectors/engine/workflow/send_events.go
[warning] 221-222: internal/connectors/engine/workflow/send_events.go#L221-L222
Added lines #L221 - L222 were not covered by tests
internal/connectors/engine/activities/activity.go
[warning] 330-333: internal/connectors/engine/activities/activity.go#L330-L333
Added lines #L330 - L333 were not covered by tests
internal/connectors/engine/activities/events_send_task_updated.go
[warning] 10-11: internal/connectors/engine/activities/events_send_task_updated.go#L10-L11
Added lines #L10 - L11 were not covered by tests
[warning] 16-17: internal/connectors/engine/activities/events_send_task_updated.go#L16-L17
Added lines #L16 - L17 were not covered by tests
🔇 Additional comments (55)
internal/connectors/plugins/public/atlar/bank_account_creation_test.go (1)
104-108: LGTM! Test error message standardization.The error message change from "test-error" to "error-test" aligns with the standardization across other test files in the codebase, improving consistency.
internal/connectors/engine/workflow/terminate_workflows_test.go (2)
86-96: LGTM! Improved error verification in workflow tests.The standardization to "error-test" ensures consistent error message verification across temporal workflow tests, aligning with the PR objective to improve test accuracy.
115-125: LGTM! Consistent error handling in termination test.The error message standardization maintains consistency with the previous test case, ensuring uniform error verification across the test suite.
internal/connectors/engine/workflow/terminate_schedules_test.go (2)
100-111: LGTM! Consistent error message standardization.The error message updates to "error-test" maintain consistency with the standardization pattern across other workflow test files.
130-130: LGTM! Error standardization in schedule deletion test.The change aligns with the project-wide test error message standardization.
pkg/events/event.go (1)
19-19: LGTM: New event type constant follows established patterns.The addition of
EventTypeUpdatedTaskis consistent with existing naming conventions and properly positioned in the constants list.internal/connectors/engine/activities/activity.go (1)
330-333: LGTM: Activity definition follows established patterns.The new
EventsSendTaskUpdatedactivity definition is properly structured and follows the existing pattern. However, static analysis indicates these lines lack test coverage.Consider adding test coverage for the new activity definition to ensure it's properly registered and functional.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 330-333: internal/connectors/engine/activities/activity.go#L330-L333
Added lines #L330 - L333 were not covered by testsinternal/connectors/engine/workflow/update_payment_initiation_from_payment_test.go (4)
65-65: LGTM: Standardizing error messages for test reliability.Good improvement to use consistent error messages across the test suite.
75-75: LGTM: Consistent error message standardization.The error assertion correctly matches the updated error message format.
84-84: LGTM: Error message consistency maintained.The temporal error creation uses the standardized message format.
94-94: LGTM: Error assertion matches standardized format.The error containment check properly validates the standardized error message.
internal/connectors/engine/workflow/create_webhooks_test.go (6)
52-52: LGTM: Standardized error message format.Good improvement to use consistent error messages across test scenarios.
64-64: LGTM: Error assertion matches standardized format.The error containment check properly validates the updated error message.
83-83: LGTM: Consistent temporal error creation.The storage activity error uses the standardized message format.
95-95: LGTM: Error assertion consistency maintained.The error validation correctly checks for the standardized message.
113-113: LGTM: Workflow error standardization.The workflow error creation follows the consistent message format.
125-125: LGTM: Final error assertion standardized.The error containment check completes the consistent error handling pattern.
internal/connectors/engine/workflow/create_transfer_test.go (3)
79-90: LGTM! Correctly validates task update event dispatch.The mock properly verifies that after successful task completion, a
SendEventsrequest is made with only theTaskfield populated, ensuring task update events are sent to subscribers.
259-282: Good error handling pattern.The changes correctly ensure that task update events are sent even in failure scenarios, and the standardized error message improves test consistency.
570-593: Valid edge case test for storage failure.This test correctly handles the scenario where the task storage operation itself fails, which is distinct from storing a task with failed status. The implementation is appropriate for this edge case.
internal/connectors/engine/workflow/fetch_accounts_test.go (1)
235-236: Good practice: Explicit error presence check.Adding
s.Error(err)befores.ErrorContains()ensures the error exists before checking its content, preventing potential nil pointer issues.internal/connectors/engine/workflow/fetch_balances_test.go (2)
188-191: Clean error handling pattern.Using
expectedErrvariables improves code readability and makes error reuse cleaner than inline creation.
499-503: Proper Temporal error wrapping.Correctly wraps errors with
temporal.NewNonRetryableApplicationErrorto ensure proper error propagation through the workflow system.internal/connectors/engine/workflow/poll_payout_test.go (3)
59-70: LGTM! Proper task event verification added.The test correctly verifies that task update events are sent after successful payout polling, with appropriate field validation.
116-139: LGTM! Consistent error handling and event verification.The changes properly standardize the error message format and ensure task update events are sent even in error scenarios.
168-171: Consistent test improvements across all error scenarios.All error test cases now follow a standardized pattern with:
- Consistent "error-test" error messages
- Proper task event verification via RunSendEvents mocks
- Appropriate error assertions
This improves test maintainability and ensures task events are properly sent in all scenarios.
Also applies to: 197-207, 239-249, 276-286, 315-325, 363-366, 405-408, 443-460
internal/connectors/engine/workflow/fetch_others_test.go (1)
170-172: Excellent test standardization and error validation improvements.The changes consistently:
- Standardize all error messages to "error-test" for better test maintainability
- Add explicit
s.Error(err)assertions beforeErrorContainschecks, ensuring errors are actually present- Use proper error construction with
temporal.NewNonRetryableApplicationErrorThese improvements enhance test reliability and consistency across the test suite.
Also applies to: 188-188, 194-197, 218-218, 235-238, 259-259, 283-285, 306-306, 331-333, 354-354, 380-384, 400-400
internal/connectors/engine/workflow/plugin_workflow_test.go (1)
349-349: Consistent error message standardization.All error messages have been standardized to "error-test", maintaining consistency with the test suite improvements across other files.
Also applies to: 373-373, 382-382, 406-406, 416-416, 440-440
internal/connectors/engine/workflow/create_payout_test.go (2)
79-90: LGTM! Task event propagation properly tested.The test correctly verifies that the Task object is propagated through the SendEvents workflow after successful payout creation.
256-282: LGTM! Error handling and task propagation properly tested.The test correctly:
- Standardizes the error message to "error-test"
- Verifies that Task events are sent even in failure scenarios
- Adds proper error assertion with
ErrorContainsThis ensures task status updates are sent regardless of workflow outcome.
internal/connectors/engine/workflow/fetch_external_accounts_test.go (1)
187-208: LGTM! Enhanced error verification in tests.The test improvements:
- Standardize error messages to "error-test" for consistency
- Add explicit error presence verification with
s.Error(err)before checking error contentThis two-step verification ensures tests fail fast if no error is returned when one is expected.
internal/connectors/engine/workflow/reset_connector_test.go (2)
45-48: LGTM! Task event propagation verified in successful reset.The test correctly verifies that Task objects are propagated through the SendEvents workflow after successful connector reset.
99-123: LGTM! Comprehensive task event handling in error scenarios.The test properly verifies that:
- Task status is set to FAILED when operations fail
- Task update events are sent even in failure scenarios
- Error messages are properly propagated and verified
This ensures users receive task update notifications regardless of operation outcome.
internal/connectors/engine/workflow/handle_webhooks_test.go (1)
156-156: LGTM! Error message standardization improves test consistency.The consistent use of "error-test" across all test error scenarios makes it easier to identify and debug test-specific errors.
Also applies to: 178-178, 194-194, 216-216, 232-232, 254-254, 271-271, 293-293, 321-321, 343-343
internal/connectors/engine/workflow/uninstall_connector_test.go (2)
11-11: LGTM! Workflow import and task event handling added correctly.The workflow import is properly used in the new RunSendEvents mocks, and the assertion that
req.Taskis not nil ensures task update events are properly propagated.Also applies to: 63-66
89-92: LGTM! Consistent task event handling across all test scenarios.The RunSendEvents workflow is consistently mocked across all test cases, ensuring task update events are sent regardless of success or failure outcomes. This aligns perfectly with the PR objectives of adding task updated events.
Also applies to: 117-120, 147-150, 178-181, 210-213, 243-246, 277-280, 312-315, 348-351, 385-388, 423-426, 462-465, 502-505, 543-546, 585-588
internal/connectors/engine/workflow/install_connector_test.go (1)
59-59: LGTM! Error message standardization applied correctly.The error messages have been consistently updated to "error-test" across all test cases, maintaining the pattern established in other test files.
Also applies to: 71-71, 80-80, 92-92, 99-104, 114-114, 123-123
internal/events/task.go (1)
12-20: LGTM! Well-structured event payload.The
taskMessagePayloadstruct is properly designed with appropriate use of pointer types for optional fields.internal/connectors/engine/workflow/send_events.go (1)
26-26: LGTM! Struct field addition is consistent.The addition of the
Taskfield follows the existing pattern for other event types.internal/connectors/engine/workflow/create_bank_account_test.go (2)
48-58: LGTM! Test properly validates task event sending.The additional mock correctly verifies that a task event is sent after the main operation completes successfully.
73-221: Well-structured test improvements for error scenarios.The test modifications consistently:
- Standardize error messages to "error-test" for better clarity
- Verify that task events are sent even in error scenarios
- Ensure the Task field is properly populated in SendEvents
These changes improve test maintainability and coverage.
internal/connectors/engine/workflow/reverse_transfer_test.go (1)
115-119: LGTM! Test improvements align with the PR objectives.The test modifications correctly implement verification for the new task updated events functionality. The consistent pattern of:
- Adding
RunSendEventsworkflow calls that verifyreq.Taskis not nil- Standardizing error handling with
temporal.NewNonRetryableApplicationError("error-test", "error-test", errors.New("error-test"))- Adding explicit error assertions with
s.ErrorContains(err, "error-test")ensures that task update events are properly triggered and validated across all test scenarios.
Also applies to: 189-189, 207-210, 224-224, 230-230, 236-239, 253-253, 263-263, 269-272, 286-286, 300-300, 306-309, 323-323, 403-403, 409-412, 426-426, 553-553, 559-562, 576-576, 625-628, 690-690, 696-699, 713-713, 765-765, 771-774, 788-788, 849-849, 855-858, 872-872, 935-935, 941-944, 958-958, 1020-1020, 1027-1030, 1044-1044, 1108-1108, 1114-1117, 1131-1131, 1197-1197, 1203-1206, 1220-1220, 1287-1287, 1302-1302, 1357-1357, 1360-1360, 1366-1369, 1383-1383, 1438-1438, 1442-1442, 1448-1451, 1465-1465
internal/connectors/engine/workflow/fetch_payments_test.go (1)
201-203: LGTM! Consistent error handling improvements in tests.The error handling standardization across all test functions improves test reliability by:
- Using consistent
"error-test"error messages- Properly creating errors with
temporal.NewNonRetryableApplicationError- Adding explicit
s.Error(err)assertions befores.ErrorContainsThese changes ensure tests properly validate error scenarios.
Also applies to: 219-219, 225-228, 249-249, 266-269, 290-290, 314-316, 337-337, 362-364, 385-385, 411-413, 434-434, 461-463, 484-484, 512-514, 535-535, 564-568, 584-584, 626-628, 638-638, 670-672, 683-683, 714-716, 726-726, 732-734, 744-744
internal/connectors/engine/workflow/update_tasks.go (2)
13-29: LGTM! Clean refactoring of updateTasksError.The method now properly constructs a
Taskobject with failed status and delegates to the newupdateTaskmethod, eliminating code duplication.
31-46: LGTM! Clean refactoring of updateTaskSuccess.The method properly constructs a
Taskobject with succeeded status and the created object ID, then delegates to the sharedupdateTaskmethod.internal/connectors/engine/workflow/reverse_payout_test.go (2)
115-119: LGTM! Consistent pattern for task update events.The addition of
RunSendEventsworkflow mocks with non-nilTaskvalidation is consistently applied across all test functions. This ensures that task update events are properly sent when task statuses are stored.Also applies to: 207-211, 236-240, 269-273, 306-310, 348-352, 409-413, 490-494, 559-563, 625-629, 696-700, 771-775, 855-859, 941-945, 1026-1030, 1113-1117, 1202-1206, 1365-1369, 1447-1451
189-189: Minor: Consistent error message update.The error messages have been uniformly changed from "test-error" to "error-test". While this is a minor change, it improves consistency across the test suite.
Also applies to: 224-224, 230-230, 253-253, 263-263, 286-286, 300-300, 323-323, 403-403, 426-426, 553-553, 576-576, 690-690, 713-713, 765-765, 788-788, 849-849, 872-872, 935-935, 958-958, 1020-1020, 1043-1043, 1107-1107, 1130-1130, 1196-1196, 1219-1219, 1286-1286, 1301-1301, 1356-1356, 1359-1359, 1382-1382, 1437-1437, 1441-1441, 1464-1464
internal/connectors/engine/workflow/send_events_test.go (3)
32-38: LGTM! Well-structured task model for testing.The
taskvariable is properly defined with all necessary fields for testing task update events.
186-204: LGTM! Comprehensive test for task update events.The test case follows the established pattern and properly validates:
- Event idempotency key generation
- Task update activity execution
- Event storage
This ensures the new task update event functionality works correctly.
119-119: Good improvement: Consistent error handling with temporal errors.Replacing simple error strings with
temporal.NewNonRetryableApplicationErrorimproves error handling consistency and provides better context for workflow failures.Also applies to: 139-139, 149-149, 212-212, 222-222, 252-252, 262-262, 288-288, 298-298, 391-391, 401-401, 421-421, 431-431, 459-459, 469-469, 495-495, 505-505, 535-535, 545-545, 573-573, 583-583, 611-611, 621-621
internal/connectors/engine/workflow/poll_transfer_test.go (5)
59-70: LGTM! Properly validates task update event publishing.The mock correctly ensures that when a task is stored with a
SUCCEEDEDstatus, theRunSendEventsworkflow is triggered with only theTaskfield populated, which aligns with the PR objective of adding task updated events.
116-138: LGTM! Consistent error handling and event publishing.The changes correctly:
- Standardize the error message to "error-test" for consistency
- Add proper validation that task update events are published when a task fails
- Ensure only the
Taskfield is populated in theSendEventsrequest
167-170: LGTM! Task update event properly published on plugin error.The mock correctly ensures that task update events are published when a task fails due to a plugin error.
362-365: LGTM! Task update events properly published for schedule deletion errors.Both test functions correctly add the
RunSendEventsmock to ensure task update events are published when tasks fail due to schedule deletion errors.Also applies to: 404-407
442-459: LGTM! Error handling properly updated.The error string standardization to "error-test" is correctly implemented with matching assertion. No
RunSendEventsmock is needed here since the task storage itself fails.
paul-nicolas
force-pushed
the
feat/task-events
branch
from
ba9a044 to
45fac0a
Compare
Description
This PR does two things:
Context
Tasks
When a user creates something via the API (bank accounts, payouts, transfers...), he currently has to poll the api to get the object id when the task is updated which is a bit of a pain.
Tests
When testing temporal workflows, sometimes we test that the workflow returns an error, but we never test what is the error. Some workflows were failing because we missed the mock of a specific functions and the actual error was not catched.
Solution
Tasks
This PR adds a new event for tasks, that the user can listen through nats/webhooks in order to get the created object.
Tests
This PR adds specific errors to catch in the test to be sure that we have the right error