-
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
chore(release): pull release/v1.51.0 into main #2865
Conversation
* feat: onboard gladly destination * chore: code review changes * chore: code review changes * chore: code review changes * chore: code review changes * chore: code review changes * chore: code review changes * chore: code review changes * chore: code review changes * fix: package.json changes * chore: added utility tests * chore: added gladly router and processor tests * chore: added gladly rETL tests * fix: sonar code smell * chore: code review changes
chore: add date mock to fix timestamp validation
* feat: initial commit * feat: making sure existing functionality is intact * fix: edits for exclusion keys * fix: edits for supporting property paths * fix: delete wrong test case * fix: test cases * fix: removed unnecessary code * fix: adding unit test cases for trimTraits * fix: changing the order of priority in property mapping * fix: edited distinct id logic * fix: small edit * fix: review comments addressed * fix: adding dedicated mappingJson for setOnce * fix: adding all the fields to the dedicated json * fix: addressing review comments * feat: review comments addressed
* feat: update facebook destinations API version to v18.0 * feat: updated fb_pixel tests to pick version dynamically from config.js * feat: updated fb tests to pick version dynamically from config.js * feat: updated fb_custom_audience tests to pick version dynamically from config.js
* feat: cm360 transformerproxy, V1 features.json update * feat: add support for both v0,v1 in handler for old server support * fix: addressed comments * chore: added tests for V1 too
* fix: encode &, < and > to html counterparts in adobe * fix: use encodeurl on url valuez
* fix: timestamp microseconds input cm360 * feat: batching in cm360 * chore: addressed comments * chore: addressed comments * fix: return aborted inside cath block * feat: update batching partial events * feat: update batching partial events * feat: update batching partial events * feat: update batching partial events * chore: removed hardcoded response * feat: added new error class * chore: develop merge * feat: handle metadata as obj * feat: handle metadata as obj * chore: removed commented code * fix: merged develop * fix: merged develop * chore: added tests for batching * chore: added tests for batching
* feat: marketo: migrate config fields and fix test cases * Update destination_config.json * chore: fix test cases * Update utils.js * Update destinationCanonicalNames.js * Update cdkV2Integration.ts * Update index.js * Update am_input.json * Update am_output.json * Update am_input.json * Update am_output.json * Update data.ts * Update am_input.json * Update data.ts * Update data.ts * chore: small fix
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 TipsChat with CodeRabbit Bot (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2865 +/- ##
==========================================
+ Coverage 87.11% 87.20% +0.09%
==========================================
Files 768 775 +7
Lines 28653 28927 +274
Branches 6726 6792 +66
==========================================
+ Hits 24960 25226 +266
- Misses 3350 3359 +9
+ Partials 343 342 -1 ☔ View full report in Codecov by Sentry. |
public static async deliverToDestination(ctx: Context) { | ||
logger.debug('Native(Delivery):: Request to transformer::', JSON.stringify(ctx.request.body)); | ||
let deliveryResponse: DeliveryResponse; | ||
const requestMetadata = MiscService.getRequestMetadata(ctx); | ||
const event = ctx.request.body as ProcessorTransformationOutput; | ||
const { destination }: { destination: string } = ctx.params; | ||
const { version }: { version: string } = ctx.params; |
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.
As it's contract update, better to add test case here - test/apitests/service.api.test.ts
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.
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.
will add test cases there, though we use v0 as default version in tests ... for route version is fetched from path param
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.
can we delete obs.delivery.js
file as DestProxyController
also calls networkHandlerFactory.getNetworkHandler
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.
We have some old tests cases I think for marketo bulk upload which is directly calling it ew can take it up as separate cleanup task as a separate PR
@@ -7,7 +7,7 @@ import stats from '../../util/stats'; | |||
import logger from '../../logger'; | |||
import tags from '../../v0/util/tags'; | |||
|
|||
export default class DeliveryTestService { | |||
export class DeliveryTestService { | |||
public static async doTestDelivery( |
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.
how are the test cases passing as version
should also be passed now ?
https://github.com/rudderlabs/rudder-transformer/pull/2865/files#diff-223989b90529aa3a55fbd07c48aa218b7ece2e1d77fadb70805dbf28f0cd31d5R18
Should add some assertions in proxy destination test cases on whether genericNetworkHandler is expected or not ?
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.
We have default version as v0
route = `/${join(tcData.version || DEFAULT_VERSION, 'destinations', tcData.name, 'proxy')}`; |
We can add tests at service.api to not send version and make sure things fall back to v0
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.
Endpoint defaulting to v0
is fine but the comment is about getNetworkHandler
call from deliveryTest.ts
As version is not being passed, it returns handlers.generic
fix: remove ErrorResponse type from postTransfomration delivery
Kudos, SonarCloud Quality Gate passed! |
import tags from '../../v0/util/tags'; | ||
import stats from '../../util/stats'; | ||
|
||
export default class DestinationPostTransformationService { | ||
type ErrorResponse = { | ||
status?: number; |
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.
can clean up ErrorResponse
👑 An automated PR