-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: onboard msl changes for new record event #2644
feat: onboard msl changes for new record event #2644
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2644 +/- ##
===========================================
- Coverage 87.25% 87.23% -0.03%
===========================================
Files 775 779 +4
Lines 28869 29042 +173
Branches 6780 6802 +22
===========================================
+ Hits 25189 25334 +145
- Misses 3340 3366 +26
- Partials 340 342 +2 ☔ View full report in Codecov by Sentry. |
Kudos, SonarCloud Quality Gate passed! |
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe updates involve significant enhancements to a Marketo Static List integration, focusing on the addition of new constants, the export of these constants, and the restructuring of objects and functions to support batch processing. The Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (5)
- test/tests/data/marketo_static_list.json
- test/tests/data/marketo_static_list_router_input.json
- test/tests/data/marketo_static_list_router_metadata_input.json
- test/tests/data/marketo_static_list_router_metadata_output.json
- test/tests/data/marketo_static_list_router_output.json
Files selected for processing (7)
- src/v0/destinations/marketo_static_list/testData/constants.js (1 hunks)
- src/v0/destinations/marketo_static_list/transform.js (4 hunks)
- src/v0/destinations/marketo_static_list/transform.test.js (1 hunks)
- src/v0/destinations/marketo_static_list/transformV2.js (1 hunks)
- src/v0/destinations/marketo_static_list/util.js (1 hunks)
- test/integrations/destinations/marketo_static_list/processor/data.ts (2 hunks)
- test/integrations/destinations/marketo_static_list/router/data.ts (6 hunks)
Files not summarized due to errors (1)
- test/integrations/destinations/marketo_static_list/router/data.ts: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- src/v0/destinations/marketo_static_list/util.js
Additional comments: 14
src/v0/destinations/marketo_static_list/testData/constants.js (5)
1-45: The addition and export of new constants are correctly implemented.
8-8: Verify that the updated
staticListId
value inDEST_CONFIG
is correct and intended.25-25: Verify that the updated
staticListId
value inDEST_OBJECT.Config
is correct and intended.29-29: The addition of the
IsProcessorEnabled
property toDEST_OBJECT
is correctly implemented.31-35: The
MESSAGE_SOURCES_CONTEXT
constant is correctly implemented.src/v0/destinations/marketo_static_list/transform.js (1)
- 95-103: The
_processParams
parameter is unused in theprocess
function. If this parameter is intended for future use or to match a function signature interface, consider adding a comment to clarify its purpose.src/v0/destinations/marketo_static_list/transform.test.js (2)
1-138: The test cases provided in the file cover a variety of scenarios and assert the expected behavior of the
processRouterDest
function with different input sizes and types. The use of a mock Axios adapter to simulate API responses is a good practice for unit testing external API interactions. The assertions check for the correct API endpoints, HTTP methods, and metadata handling, which aligns with the changes described in the pull request summary.14-138: Verify that the hardcoded endpoints used in the test cases are not subject to frequent changes. If they are, consider making them configurable to avoid having to update the tests frequently.
src/v0/destinations/marketo_static_list/transformV2.js (3)
74-141: The logic for processing record inputs, including error handling and response building, appears to be well-structured and comprehensive.
57-66: The
batchResponseBuilder
function correctly chunks the lead IDs to avoid long URLs and creates appropriate batched responses.74-141: Verify that the caller of
processRecordInputs
properly handles the promise returned by this asynchronous function.test/integrations/destinations/marketo_static_list/processor/data.ts (2)
65-67: The change in the HTTP method from "POST" to "DELETE" and the update of the
id
parameters in the endpoint URL are consistent with the summary provided. Ensure that the backend service supports these changes and that the corresponding API endpoints are expecting these newid
values.83-85: The change in the HTTP method from "DELETE" to "POST" and the update of the
id
parameters in the endpoint URL are consistent with the summary provided. As with the previous hunk, verify that the backend service supports these changes and that the corresponding API endpoints are expecting these newid
values.test/integrations/destinations/marketo_static_list/router/data.ts (1)
- 1663-3643: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-3643]
The test data for the new 'record' event type and the existing 'audiencelist' event type is structured correctly, with appropriate endpoint URLs, headers, and metadata. The batchedRequest objects are formed as expected, and the batched flag is set correctly according to the test case requirements. The placeholders used for tokens and IDs are appropriate for test data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/v0/destinations/marketo_static_list/transform.js (4 hunks)
- src/v0/destinations/marketo_static_list/transformV2.js (1 hunks)
Kudos, SonarCloud Quality Gate passed! |
What are the changes introduced in this PR?
We are adding support for a new event type
record
for Marketo Static List. This is a new alternative to the existingaudiencelist
event.Please explain the objectives of your changes below:
Through record events, we aim to eliminate some observability inconsistencies that were present along with audiencelist events.
There are some dependent changes in rudder-server
Type of change
If the pull request is a bug-fix, enhancement or a refactor, please fill in the details on the changes made.
If the pull request is a new feature,
Any technical or performance related pointers to consider with the change?
New record event spec:
The other features of MSL, like batching maximum of 300 lead IDs into a single API call are intact. As of now, both record and audiencelist event types are supported, and the changes made here are backward-compatible.
Any new dependencies introduced with this change?
No generic utility has been modified in these changes, all logic concerning the new supported event type
record
is in a separatetransformV2.js
file.If the PR has changes in more than 5 files, please mention why the changes were not split into multiple PRs.
There is a overall functionality change with a new transformation flow being introduced, hence all related changes needed to be in one PR for easier reference.
If there are multiple linear items associated with the PR changes, please elaborate on the reason:
INT-1007 : We had to move out of the earlier done modular approach on top of audiencelist event structure.
Developer checklist
Reviewer checklist
Notion
Summary by CodeRabbit
New Features
responseBuilder
andbatchResponseBuilder
functions.Improvements
Bug Fixes
Tests
processRouterDest
function to validate batch creation and API interaction.Chores
InstrumentationError
for enhanced error tracking and handling.