fix: Relax Hubspot cookie tracking #4905
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
|
Uffizzi Preview |
| try: | ||
| HubspotTracker.objects.update_or_create( | ||
| user=request.user, | ||
| defaults={ | ||
| "hubspot_cookie": hubspot_cookie, | ||
| }, | ||
| ) | ||
| logger.info( | ||
| f"Created HubspotTracker instance for user {request.user.email} with cookie {hubspot_cookie}" | ||
| ) | ||
| except IntegrityError: | ||
| logger.info( | ||
| f"HubspotTracker could not be created for user {request.user.email}" | ||
| f" due to cookie conflict with cookie {hubspot_cookie}" | ||
| ) |
There was a problem hiding this comment.
I'm not sure if I'm missing something here as to why we're using update_or_create here, but I don't love the implementation - mostly because IntegrityError might not always indicate what we're assuming here.
I think we should just be more explicit, and do something like:
if HubspotTracker.objects.filter(hubspot_cookie=hubspot_cookie).exists():
logger.info(
f"HubspotTracker could not be created for user {request.user.email}"
f" due to cookie conflict with cookie {hubspot_cookie}"
)
else:
HubspotTracker.objects.create(...)
logger.info(...)There was a problem hiding this comment.
Ok I've switched to a filter and exclude for the check. I'm keeping the update_or_create here because if a users hubspot cookie is changed we want to include that change in their tracking data.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4905 +/- ##
=======================================
Coverage 97.38% 97.39%
=======================================
Files 1189 1189
Lines 41453 41468 +15
=======================================
+ Hits 40371 40386 +15
Misses 1082 1082 ☔ View full report in Codecov by Sentry. |
| .exists() | ||
| ): | ||
| logger.info( | ||
| f"HubspotTracker could not be created for user {request.user.email}" |
There was a problem hiding this comment.
Sorry, I'd actually rather that we just log the user id here (and below) - I don't like the idea of logging emails if we can avoid it.
There was a problem hiding this comment.
Ok that's no problem. I've switched the loggers to use user ids.
Changes
Fixes #4900
Since Hubspot cookies are used to merge identities, it is preferable to ignore a Hubspot token for different users on the same machine.
How did you test this code?
Proved that the error was raised without it then wrote a test to verify that multiple users can supply the same Hubspot cookie without an error being raised.