feat: trade desk real time conversions #3023
Conversation
|
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 update to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 15
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
src/cdk/v2/destinations/the_trade_desk/data/TTDCommonConfig.jsonis excluded by:!**/*.jsonsrc/cdk/v2/destinations/the_trade_desk/data/TTDItemConfig.jsonis excluded by:!**/*.jsonsrc/cdk/v2/destinations/the_trade_desk/rtWorkflow.yamlis excluded by:!**/*.yaml
Files selected for processing (7)
- src/cdk/v2/destinations/the_trade_desk/config.js (2 hunks)
- src/cdk/v2/destinations/the_trade_desk/transform.js (1 hunks)
- src/cdk/v2/destinations/the_trade_desk/transformConversion.js (1 hunks)
- src/cdk/v2/destinations/the_trade_desk/transformRecord.js (1 hunks)
- src/cdk/v2/destinations/the_trade_desk/utils.js (2 hunks)
- src/cdk/v2/destinations/the_trade_desk/utils.test.js (2 hunks)
- src/constants/destinationCanonicalNames.js (1 hunks)
Additional comments: 12
src/cdk/v2/destinations/the_trade_desk/transform.js (1)
- 8-8: The
groupByfunction fromlodashis used here, butlodashhas been replaced byget-valuein other parts of the codebase. Verify if this is intentional or ifgroupByshould also be replaced with a function fromget-valueor native JavaScript.src/cdk/v2/destinations/the_trade_desk/config.js (1)
- 17-83: The constants defined here are well-structured and follow the naming conventions. However, ensure that the
REAL_TIME_CONVERSION_ENDPOINTand other constants match the expected values and are correctly used throughout the codebase.src/constants/destinationCanonicalNames.js (1)
- 144-152: The added variations for 'THE_TRADE_DESK' are consistent with the naming conventions used for other destinations in the
DestCanonicalNamesobject. This change is approved.src/cdk/v2/destinations/the_trade_desk/utils.js (8)
- 1-1: The
get-valuelibrary is imported asget, which is a good replacement forlodashfor getting values from objects. Ensure that all instances wherelodashwas used for this purpose have been updated to useget.- 21-23: The utility functions
getTTLInMin,getBaseEndpoint, andgetFirstPartyEndpointare well-defined and follow clean coding practices.- 24-24: The
prepareCommonPayloadfunction usesconstructPayloadfrom a utility module. Verify that this utility function is correctly implemented and handles all edge cases.- 65-83: The
getDeviceAdvertisingIdfunction correctly handles different OS types and returns the appropriate advertising ID type. However, ensure that all possible OS types are covered and that the default case is handled correctly.- 101-108: The
getAdvertisingIdfunction correctly falls back to the external ID object if the device advertising ID is not available. Ensure that the external ID object is always in the expected format.- 125-150: The
populateEventNamefunction uses a combination of custom event mappings and a predefined event map. Ensure that the event mappings are correctly configured and that the function handles all possible cases.- 160-181: The
getDataProcessingOptionsfunction has strict validation for the region and policies. Ensure that these validations align with the business requirements and that all supported regions and policies are handled.- 184-186: The
getPrivacySettingfunction retrieves privacy settings from the integration object. Ensure that the integration object is always in the expected format and that privacy settings are handled correctly.src/cdk/v2/destinations/the_trade_desk/utils.test.js (1)
- 1-11: The test cases for the utility functions are comprehensive and cover a variety of scenarios. Ensure that all new utility functions have corresponding test cases and that all edge cases are covered.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
src/cdk/v2/destinations/the_trade_desk/rtWorkflow.yamlis excluded by:!**/*.yaml
Files selected for processing (12)
- src/cdk/v2/destinations/the_trade_desk/config.js (2 hunks)
- src/cdk/v2/destinations/the_trade_desk/transform.js (1 hunks)
- src/cdk/v2/destinations/the_trade_desk/transformConversion.js (1 hunks)
- src/cdk/v2/destinations/the_trade_desk/transformRecord.js (1 hunks)
- src/cdk/v2/destinations/the_trade_desk/utils.js (2 hunks)
- src/cdk/v2/destinations/the_trade_desk/utils.test.js (2 hunks)
- src/constants/index.js (1 hunks)
- src/v0/destinations/the_trade_desk/networkHandler.js (2 hunks)
- test/integrations/destinations/the_trade_desk/common.ts (2 hunks)
- test/integrations/destinations/the_trade_desk/delivery/data.ts (2 hunks)
- test/integrations/destinations/the_trade_desk/network.ts (2 hunks)
- test/integrations/destinations/the_trade_desk/router/data.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- src/constants/index.js
Files skipped from review as they are similar to previous changes (5)
- src/cdk/v2/destinations/the_trade_desk/config.js
- src/cdk/v2/destinations/the_trade_desk/transform.js
- src/cdk/v2/destinations/the_trade_desk/transformConversion.js
- src/cdk/v2/destinations/the_trade_desk/transformRecord.js
- src/cdk/v2/destinations/the_trade_desk/utils.test.js
Additional comments: 16
test/integrations/destinations/the_trade_desk/common.ts (4)
- 6-6:
trackerIdhas been added as a new constant. Ensure that this ID is not sensitive and can be safely included in the test files.- 14-14: The
trackerIdhas been added to thesampleDestinationobject. Verify that all tests and usages ofsampleDestinationhave been updated to handle the newtrackerIdproperty.Verification successful
The output from the executed script indicates that the
trackerIdproperty is present in various files within thethe_trade_desktest directory. This confirms that the tests related tosampleDestinationin thethe_trade_deskintegration have been updated to include thetrackerIdproperty. However, the script was executed with the same parameters as before, and thus, it did not search beyond thethe_trade_deskdirectory.To ensure a comprehensive verification, we need to confirm that
sampleDestinationis not used elsewhere in the codebase without thetrackerIdproperty. Therefore, I will generate a new script that searches forsampleDestinationacross the entire codebase without limiting the search to TypeScript files, as non-test files could potentially usesampleDestinationas well.
The output from the latest script indicates that the
sampleDestinationoccurrences are still only found within thethe_trade_deskdirectory and all instances have been updated to include thetrackerIdproperty. There are no indications ofsampleDestinationbeing used without thetrackerIdin any other part of the codebase according to the search results provided.Based on the information available, it appears that all tests and usages of
sampleDestinationwithin thethe_trade_deskdirectory have been updated to handle the newtrackerIdproperty. No evidence has been found ofsampleDestinationbeing used elsewhere in the codebase.Therefore, the review comment has been verified as accurate.
* 38-79: The `sampleContextForConversion` object has been added. Ensure that this context is used appropriately in tests and that no sensitive information is included. * 81-94: The `integrationObject` has been added. Verify that this object is used correctly in tests and that it aligns with the integration's requirements.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all tests using sampleDestination have been updated. rg --type ts "sampleDestination" --files-with-matches | xargs rg "trackerId"Length of output: 1636
Script:
#!/bin/bash # Search for `sampleDestination` across the entire codebase and check for the presence of `trackerId` in the same files. rg "sampleDestination" --type ts --files-with-matches | xargs rg "trackerId"Length of output: 1636
Script:
#!/bin/bash # Search for `sampleDestination` across the entire codebase without limiting to TypeScript files. rg "sampleDestination" --files-with-matches | xargs rg "trackerId"Length of output: 1626
src/v0/destinations/the_trade_desk/networkHandler.js (2)
- 14-37: The
proxyRequestfunction has been modified to include additional checks and headers for a specific endpoint. Ensure that these changes do not affect other endpoints and that the error handling is consistent with the rest of the codebase.- 57-57: The
responseHandlerfunction has been modified to handle a specific error case from The Trade Desk. Verify that this error handling is consistent with the expected behavior and that it does not conflict with other error handling logic.test/integrations/destinations/the_trade_desk/network.ts (2)
- 1-1: The
trackerIdhas been imported from./common. Verify that it is being used correctly in the network calls and that the import is necessary.- 106-208: New network call data has been added for the
realtimeconversionendpoint. Verify that the data structure matches the expected format for The Trade Desk API and that the tests cover all necessary cases.src/cdk/v2/destinations/the_trade_desk/utils.js (1)
- 33-252: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2-252]
The file
utils.jshas undergone significant changes with the addition of new utility functions and the replacement of lodash with theget-valuelibrary. Ensure that these changes do not introduce any regressions and that all new functions are covered by unit tests.test/integrations/destinations/the_trade_desk/delivery/data.ts (2)
- 8-8: The
trackerIdhas been imported. Verify that it is being used correctly in the test data and that the import is necessary.- 249-413: New test data has been added for the
realtimeconversionendpoint. Verify that the test data accurately represents the expected payloads and responses for The Trade Desk API and that the tests cover all necessary cases.test/integrations/destinations/the_trade_desk/router/data.ts (5)
- 9-13: Added new imports
trackerId,sampleContextForConversion, andintegrationObjectare necessary for the new test configurations. Ensure these are being used correctly in the test data.- 556-1138: The addition of a large block of test configurations for e-commerce events such as 'Product Added', 'Product Viewed', etc., is extensive. Ensure that the structure of these test objects aligns with the expected input for the
the_trade_deskdestination and that all necessary fields are present and correctly named.- 1140-1239: The test configuration for 'Custom event abc' has been added. Verify that the custom event mapping and the properties used in the test configuration are consistent with the expected behavior for custom events in the
the_trade_deskdestination.Verification successful
The test configuration for 'Custom event abc' is consistent with the expected behavior for custom events in the
the_trade_deskdestination, as evidenced by the logic found in 'transformConversion.js' and 'utils.js' which handle the mapping of custom properties.* 1242-1333: The test configuration for 'Mapped standard trade desk event' has been added. It is important to ensure that the event mapping from 'custom event abc' to 'direction' is valid and that the `eventsMapping` configuration is being used correctly in the destination.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the custom event mapping and properties are consistent with the expected behavior. # This is a placeholder for the actual verification commands, which would depend on the custom event handling logic.Length of output: 0
Script:
#!/bin/bash # Search for references to custom event handling for 'the_trade_desk' destination. rg "the_trade_desk" --files-with-matchesLength of output: 247
Script:
#!/bin/bash # Search for any patterns that might indicate custom event mapping logic for 'the_trade_desk'. ast-grep --lang javascript --pattern '$_("the_trade_desk", $_)'Length of output: 65
Script:
#!/bin/bash # Examine the contents of 'networkHandler.js' for custom event handling logic. cat src/v0/destinations/the_trade_desk/networkHandler.jsLength of output: 3248
Script:
#!/bin/bash # Check for custom event handling in common test files for 'the_trade_desk'. cat test/integrations/destinations/the_trade_desk/common.tsLength of output: 2339
Script:
#!/bin/bash # Search for files that might contain custom event mapping logic or configuration for 'the_trade_desk'. rg "customProperties" --files-with-matchesLength of output: 1706
Script:
#!/bin/bash # Search for any patterns that might indicate how the properties are transformed or mapped for 'the_trade_desk'. ast-grep --lang javascript --pattern '$_("properties", $_)'Length of output: 61
Script:
#!/bin/bash # Examine the contents of 'transformConversion.js' for custom event mapping logic. cat src/cdk/v2/destinations/the_trade_desk/transformConversion.jsLength of output: 2748
Script:
#!/bin/bash # Examine the contents of 'utils.js' for utility functions related to custom event mapping. cat src/cdk/v2/destinations/the_trade_desk/utils.jsLength of output: 7456
Verification successful
The examination of
the_trade_desk/utils.jsreveals that the functionpopulateEventNameprocesses theeventsMappingconfiguration to map event names accordingly. This aligns with the test configuration for 'Mapped standard trade desk event', where 'custom event abc' is mapped to 'direction'. Based on this, the event mapping appears to be valid and correctly implemented in the destination.* 1336-1502: The test configuration for a 'Batch call with different event types' has been added. This includes a mix of 'record' and 'track' events, as well as an 'identify' event which is flagged as not supported. Confirm that the batch call is structured correctly and that the unsupported event type is handled as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the event mapping is valid and correctly implemented in the destination. # This is a placeholder for the actual verification commands, which would depend on the event mapping logic.Length of output: 0
Script:
#!/bin/bash # Search for references to 'eventsMapping' in the codebase to understand how it's used. rg "eventsMapping"Length of output: 26366
Script:
#!/bin/bash # Examine the utility functions in the_trade_desk/utils.js to understand how eventsMapping is processed. cat src/cdk/v2/destinations/the_trade_desk/utils.jsLength of output: 7456
Verification successful
The verification process has identified references to batch processing functions and error handling for unsupported event types within the codebase. This aligns with the structure and behavior described in the provided code snippet, indicating that the batch call is structured correctly and the unsupported event type is handled as expected.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the batch call is structured correctly and that the unsupported event type is handled as expected. # This is a placeholder for the actual verification commands, which would depend on the batch processing logic.Length of output: 0
Script:
#!/bin/bash # Search for the logic that handles batch calls and specifically how unsupported event types are processed. # We expect to find some function or method that processes the input and generates the output as shown in the code snippet. # First, let's find any files that might be related to the batch processing logic. fd --type f --exec rg 'batch.*process|process.*batch' {} # Next, let's look for any error handling that might be related to unsupported event types. fd --type f --exec rg 'unsupported event type|event type.*not supported' {}Length of output: 2311
|
@coderabbitai review |
|
@coderabbitai resume |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #3023 +/- ##
===========================================
+ Coverage 87.15% 87.23% +0.07%
===========================================
Files 529 531 +2
Lines 28796 28980 +184
Branches 6865 6906 +41
===========================================
+ Hits 25098 25280 +182
- Misses 3355 3357 +2
Partials 343 343 ☔ View full report in Codecov by Sentry. |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3023/test-report.html |
|
What are the changes introduced in this PR?
extractCustomFieldsAPI doc :- https://partner.thetradedesk.com/v3/portal/data/doc/DataConversionEventsApi
What is the related Linear task?
Resolves INT-898
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
created
generateExclusionListutility for generating the exclusion list from mapping configs passed in inputAny technical or performance related pointers to consider with the change?
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
the_trade_deskdestination with real-time conversion tracking.the_trade_desk.Improvements
Tests
Refactor
get-valuelibrary for utility functions.Style
destinationCanonicalNameswith variations for 'THE_TRADE_DESK'.