[Hubspot] Add de-duplication logic in Hubspot #2924
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2924 +/- ##
==========================================
- Coverage 78.11% 77.99% -0.12%
==========================================
Files 1050 1099 +49
Lines 19438 20277 +839
Branches 3734 3910 +176
==========================================
+ Hits 15184 15816 +632
- Misses 2972 3136 +164
- Partials 1282 1325 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @harsh-joshi99 please let me know when this PR is ready to review. I think I should be involved with this PR as I contributed a lot to the Actions being updated. |
|
Hi @harsh-joshi99 can we schedule a call please? |
packages/destination-actions/src/destinations/hubspot/upsertObject/common-fields.ts
Outdated
Show resolved
Hide resolved
| @@ -52,6 +52,10 @@ const send = async ( | |||
| subscriptionMetadata?: SubscriptionMetadata, | |||
| statsContext?: StatsContext | |||
| ) => { | |||
| if (syncMode === 'upsert') { | |||
There was a problem hiding this comment.
Won't we also have the same issue if the customer is using syncMode === 'update' ?
There was a problem hiding this comment.
Also @harsh-joshi99 @varadarajan-tw can we put this code behind a feature flag to control rollout?
| const id = incoming.object_details?.id_field_value | ||
| if (!id) continue | ||
|
|
||
| const incomingTimestamp = incoming.timestamp ? new Date(incoming.timestamp).getTime() : 0 |
There was a problem hiding this comment.
If the customer misconfigures the timestamp field or sends something that isn't a timestamp, then you're falling back to creating a timestamp. However the timestamp you create might correctly order the event.
It might be better to grab the timestamp directly from the raw payload data instead. i.e not via a field. Note that there will always be a timestamp in the raw payload data.
Have a look at how to access the rawData object here.
At least this way you will always be guaranteed to order the events in the order they arrived at the Segment ingestion point.
There was a problem hiding this comment.
So what I'm recommending is:
const incomingTimestamp = isTimestamp(incoming.timestamp) ? incoming.timestamp : getTimestampFromRawPayload()
| } | ||
|
|
||
| const existing = mergedMap.get(id)! | ||
| const existingTimestamp = existing.timestamp ? new Date(existing.timestamp).getTime() : 0 |
There was a problem hiding this comment.
I think that we should ensure that there's always a timestamp in the payload that gets added to the mergeMap (see my comment above). This means that if there is an existing payload, you can assume it has a timestamp.
.../destination-actions/src/destinations/hubspot/upsertObject/functions/validation-functions.ts
Show resolved
Hide resolved
.../destination-actions/src/destinations/hubspot/upsertObject/functions/validation-functions.ts
Outdated
Show resolved
Hide resolved
|
hi @harsh-joshi99 and @varadarajan-tw I reviewed the PR and left some comments. As well as the comments I think it would be a good idea to add some unit tests to test the new mergeAndDeduplicateById function. And I recommend we put this code behind a feature flag - but that's up to you. |
- Remove dynamic from timestamp mapping - Add validation for timestamps - Add test cases
|
Hi @harsh-joshi99 just checking what's happening with this PR. When are you planning to progress it? |
Hi @joe-ayoub-segment, I have made some changes. Could I get a re review please? |
This PR adds a function to de-duplicate properties in hubspot's custom object v2 action.
Hubspot does not allow properties with same ID.
JIRA -> https://twilio-engineering.atlassian.net/browse/STRATCONN-5746?atlOrigin=eyJpIjoiMzU3ZWIxYzkzODU2NDIyMjg0MWFkYzM4OTc2MjEwZjkiLCJwIjoiaiJ9
Testing
https://docs.google.com/document/d/17cett1m8KfzEvLxxBfzbhiFBgeRwDhzMZYnTzuRZG30/edit?tab=t.mb5dleeya8xs
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.