fix: mailjet source operating on array instead object #2999
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 modifications involve a refactor in the Mailjet integration code. 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2999 +/- ##
========================================
Coverage 87.24% 87.24%
========================================
Files 530 530
Lines 28808 28812 +4
Branches 6860 6860
========================================
+ Hits 25133 25137 +4
Misses 3341 3341
Partials 334 334 ☔ View full report in Codecov by Sentry. |
|
This seems like a breaking change, and mailjet source looks to be broken since start then right @anantjain45823 ? |
|
Yes correct @yashasvibajpai you can refer to linear card for details |
|
@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: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
test/__tests__/data/mailjet_source.jsonis excluded by:!**/*.json
Files selected for processing (2)
- src/v0/sources/mailjet/transform.js (2 hunks)
- test/tests/mailjet_source.test.js (1 hunks)
Additional comments: 3
test/__tests__/mailjet_source.test.js (1)
- 21-21: The test assertion has been updated to check
outputagainstdata.outputdirectly. Ensure that thedata.outputin the test data files reflects the new expected structure after the changes in thetransform.js.Verification successful
The output from the shell script shows that the
data.outputin the test data files contains an array of objects, each representing an event with a structure that includescontext,integrations,type,event,properties, and other relevant fields. This aligns with the PR's objective to handle events as an array of objects instead of a single object.Based on this information, it can be concluded that the test data files have been updated to reflect the new expected structure after the changes in the
transform.jsfile. The test assertion change to checkoutputagainstdata.outputdirectly is consistent with the updated test data.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the test data files have been updated to reflect the new structure. rg --type json --files-with-matches '"output":' test/__tests__/data/mailjet_source.json | xargs jq '.[].output'Length of output: 5106
src/v0/sources/mailjet/transform.js (2)
- 2-19: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [11-46]
The
processEventfunction has been introduced to handle individual events. Ensure that the logic within this function correctly processes the events according to the Mailjet webhook documentation and that the mapping file is correctly applied.
- 48-52: A new
processfunction has been added to handle an array of events. It usesCommonUtils.toArrayto ensure the input is treated as an array, which is a good practice to handle single objects and arrays uniformly. Verify that theCommonUtils.toArrayfunction is robust and handles all edge cases.Verification successful
---
The search results have provided the implementation of the
toArrayfunction withinsrc/util/common.js. Here is the relevant code snippet:toArray(obj) { if (Array.isArray(obj)) { return obj; } return [obj]; },This implementation checks if the input
objis already an array usingArray.isArray(obj). If it is, it returns the object as is. If not, it wraps the object in an array. This is a simple and effective way to ensure that the input is treated as an array, which aligns with the review comment's suggestion to verify that theCommonUtils.toArrayfunction is robust and handles all edge cases.Based on this implementation, the
CommonUtils.toArrayfunction will handle cases where the input is either an array or a single object, but it does not include any additional checks or handling for other potential edge cases, such asnull,undefined, or non-object primitives.Given this information, the review comment is verified to be accurate in its suggestion to ensure that the
CommonUtils.toArrayfunction is robust. However, it should be noted that the function may not handle every possible edge case, such asnullorundefinedinputs.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of CommonUtils.toArray to ensure it handles various edge cases. rg --type js 'CommonUtils.toArray' src/util/common.jsLength of output: 53
Script:
#!/bin/bash # Since the previous command did not yield any results, let's try a broader search for CommonUtils.toArray # across all JavaScript files to find its implementation. rg --type js 'CommonUtils.toArray'Length of output: 413
Script:
#!/bin/bash # The implementation of CommonUtils.toArray was not found in the expected file. # Let's search for the definition of this function in the entire codebase. rg --type js -A 10 'toArray\('Length of output: 4103
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/2999/test-report.html |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/2999/test-report.html |
|
What are the changes introduced in this PR?
All events received by rudderstack are coming in form of array of objects where as we are considering it as object only due which we are just sending empty payloads .
Nothing specifically is mention in mailjet docs about payload type
Mailjet Docs: https://dev.mailjet.com/email/guides/webhooks/#overview
Write a brief explainer on your code changes.
What is the related Linear task?
Resolves INT-1384
Please explain the objectives of your changes below
To solve a basic type mismatch problem in mailjet source
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.
N/A
Any 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
Refactor
Tests