Skip to content
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.

Sign up for GitHub

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

fix: Relax Hubspot cookie tracking #4905

Merged
merged 6 commits into from
Dec 10, 2024
Merged

fix: Relax Hubspot cookie tracking #4905

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
987fabc
Catch the integrity error
zachaysan Dec 9, 2024
ced5902
Test that the hubspot cookie collision doesn't raise an error
zachaysan Dec 9, 2024
02a0246
Switch to inclusion check
zachaysan Dec 9, 2024
0b526ff
Merge branch 'main' into fix/relax_hubspot_cookie_tracking
zachaysan Dec 9, 2024
bcd9037
Switch to user ids from emails
zachaysan Dec 10, 2024
2380431
Merge branch 'main' into fix/relax_hubspot_cookie_tracking
zachaysan Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions api/integrations/lead_tracking/hubspot/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging

from django.db import IntegrityError
from rest_framework.request import Request

from integrations.lead_tracking.hubspot.constants import HUBSPOT_COOKIE_NAME
Expand All @@ -17,13 +18,18 @@ def register_hubspot_tracker(request: Request) -> None:
)
return

logger.info(
f"Creating HubspotTracker instance for user {request.user.email} with cookie {hubspot_cookie}"
)

HubspotTracker.objects.update_or_create(
user=request.user,
defaults={
"hubspot_cookie": hubspot_cookie,
},
)
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}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's no problem. I've switched the loggers to use user ids.

f" due to cookie conflict with cookie {hubspot_cookie}"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

34 changes: 34 additions & 0 deletions api/tests/unit/organisations/test_unit_organisations_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,40 @@ def test_non_superuser_can_create_new_organisation_by_default(
assert HubspotTracker.objects.filter(user=staff_user).exists()


def test_colliding_hubspot_cookies_are_ignored(
staff_client: APIClient,
staff_user: FFAdminUser,
admin_user: FFAdminUser,
) -> None:
# Given
org_name = "Test create org"
webhook_notification_email = "[email protected]"
url = reverse("api-v1:organisations:organisation-list")
colliding_cookie = "test_cookie_tracker"
HubspotTracker.objects.create(
user=admin_user,
hubspot_cookie=colliding_cookie,
)
data = {
"name": org_name,
"webhook_notification_email": webhook_notification_email,
HUBSPOT_COOKIE_NAME: colliding_cookie,
}

assert not HubspotTracker.objects.filter(user=staff_user).exists()

# When
response = staff_client.post(url, data=data)

# Then
assert response.status_code == status.HTTP_201_CREATED
assert (
Organisation.objects.get(name=org_name).webhook_notification_email
== webhook_notification_email
)
assert not HubspotTracker.objects.filter(user=staff_user).exists()


@override_settings(RESTRICT_ORG_CREATE_TO_SUPERUSERS=True)
def test_create_new_orgnisation_returns_403_with_non_superuser(
staff_client: APIClient,
Expand Down
Loading