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

feat: Publish onboarding events #1147

Merged
merged 4 commits into from
Feb 11, 2025
Merged

feat: Publish onboarding events #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bde5f6
Add AMPLITUDE_API_KEY setting
spalmurray Feb 10, 2025
584e792
Publish User Created and User Logged in events
spalmurray Feb 10, 2025
2106b3d
Publish GH App Installed event
spalmurray Feb 10, 2025
b16b26a
Fix test
spalmurray Feb 11, 2025
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
2 changes: 2 additions & 0 deletions codecov/settings_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,8 @@
"setup", "stripe", "payment_method_configuration_id", default=None
)

AMPLITUDE_API_KEY = os.environ.get("AMPLITUDE_API_KEY", None)

# Allows to do migrations from another module
MIGRATION_MODULES = {
"codecov_auth": "shared.django_apps.codecov_auth.migrations",
Expand Down
47 changes: 46 additions & 1 deletion codecov_auth/tests/unit/views/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from datetime import datetime, timedelta, timezone
from unittest.mock import Mock, patch
from unittest.mock import Mock, call, patch

import pytest
from django.conf import settings
Expand Down Expand Up @@ -174,6 +174,27 @@ def test_get_or_create_calls_analytics_user_signed_up_when_owner_created(
)
user_signed_up_mock.assert_called_once()

@patch("shared.events.amplitude.AmplitudeEventPublisher.publish")
def test_get_or_create_calls_amplitude_user_created_when_owner_created(
self, amplitude_publish_mock
):
self.mixin_instance._get_or_create_owner(
{
"user": {"id": 12345, "key": "4567", "login": "testuser"},
"has_private_access": False,
},
self.request,
)

owner = Owner.objects.get(service_id=12345, username="testuser")

amplitude_publish_mock.assert_has_calls(
[
call("User Created", {"user_ownerid": owner.ownerid}),
call("set_orgs", {"user_ownerid": owner.ownerid, "org_ids": []}),
]
)

@patch("services.analytics.AnalyticsService.user_signed_in")
def test_get_or_create_calls_analytics_user_signed_in_when_owner_not_created(
self, user_signed_in_mock
Expand All @@ -192,6 +213,30 @@ def test_get_or_create_calls_analytics_user_signed_in_when_owner_not_created(
)
user_signed_in_mock.assert_called_once()

@patch("shared.events.amplitude.AmplitudeEventPublisher.publish")
def test_get_or_create_calls_amplitude_user_logged_in_when_owner_not_created(
self, amplitude_publish_mock
):
owner = OwnerFactory(service_id=89, service="github", organizations=[1, 2])
self.mixin_instance._get_or_create_owner(
{
"user": {
"id": owner.service_id,
"key": "02or0sa",
"login": owner.username,
},
"has_private_access": owner.private_access,
},
self.request,
)

amplitude_publish_mock.assert_has_calls(
[
call("User Logged in", {"user_ownerid": owner.ownerid}),
call("set_orgs", {"user_ownerid": owner.ownerid, "org_ids": [1, 2]}),
]
)

@override_settings(IS_ENTERPRISE=False)
@patch("services.analytics.AnalyticsService.user_signed_in")
def test_set_marketing_tags_on_cookies(self, user_signed_in_mock):
Expand Down
12 changes: 12 additions & 0 deletions codecov_auth/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from django.utils import timezone
from django.utils.timezone import now
from shared.encryption.token import encode_token
from shared.events.amplitude import AmplitudeEventPublisher
from shared.license import LICENSE_ERRORS_MESSAGES, get_current_license

from codecov_auth.models import Owner, OwnerProfile, Session, User
Expand Down Expand Up @@ -393,10 +394,21 @@ def _get_or_create_owner(
owner.save(update_fields=fields_to_update)

marketing_tags = self.retrieve_marketing_tags_from_cookie()
amplitude = AmplitudeEventPublisher()
if was_created:
self.analytics_service.user_signed_up(owner, **marketing_tags)
amplitude.publish("User Created", {"user_ownerid": owner.ownerid})
else:
self.analytics_service.user_signed_in(owner, **marketing_tags)
amplitude.publish("User Logged in", {"user_ownerid": owner.ownerid})
orgs = owner.organizations
amplitude.publish(
"set_orgs",
{
"user_ownerid": owner.ownerid,
"org_ids": orgs if orgs is not None else [],
},
)

return (owner, was_created)

Expand Down
78 changes: 78 additions & 0 deletions webhook_handlers/tests/test_github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,84 @@ def test_installation_creates_new_owner_if_dne_default_app(self, mock_refresh):
repos_affected=[("12321", "R_kgDOG2tZYQ"), ("12343", "R_kgDOG2tABC")],
)

@patch("shared.events.amplitude.AmplitudeEventPublisher.publish")
@patch("services.task.TaskService.refresh")
def test_installation_publishes_amplitude_event_without_installer(
self, mock_refresh, mock_amplitude_publish
):
username, service_id = "newuser", 123456

self._post_event_data(
event=GitHubWebhookEvents.INSTALLATION,
data={
"installation": {
"id": 4,
"repository_selection": "selected",
"account": {"id": service_id, "login": username},
"app_id": DEFAULT_APP_ID,
},
"repositories": [
{"id": "12321", "node_id": "R_kgDOG2tZYQ"},
{"id": "12343", "node_id": "R_kgDOG2tABC"},
],
"sender": {"type": "User"},
},
)

owner_set = Owner.objects.filter(
service="github", service_id=service_id, username=username
)
assert owner_set.exists()
owner = owner_set.first()

mock_amplitude_publish.assert_called_with(
"App Installed",
{
"user_ownerid": owner.ownerid,
"ownerid": owner.ownerid,
},
)

@patch("shared.events.amplitude.AmplitudeEventPublisher.publish")
@patch("services.task.TaskService.refresh")
def test_installation_publishes_amplitude_event_with_installer(
self, mock_refresh, mock_amplitude_publish
):
installer = OwnerFactory(service="github", username="installer_username")

username, service_id = "newuser", 123456

self._post_event_data(
event=GitHubWebhookEvents.INSTALLATION,
data={
"installation": {
"id": 4,
"repository_selection": "selected",
"account": {"id": service_id, "login": username},
"app_id": DEFAULT_APP_ID,
},
"repositories": [
{"id": "12321", "node_id": "R_kgDOG2tZYQ"},
{"id": "12343", "node_id": "R_kgDOG2tABC"},
],
"sender": {"type": "User", "login": "installer_username"},
},
)

owner_set = Owner.objects.filter(
service="github", service_id=service_id, username=username
)
assert owner_set.exists()
owner = owner_set.first()

mock_amplitude_publish.assert_called_with(
"App Installed",
{
"user_ownerid": installer.ownerid,
"ownerid": owner.ownerid,
},
)

@patch(
"services.task.TaskService.refresh",
lambda self,
Expand Down
30 changes: 28 additions & 2 deletions webhook_handlers/views/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from rest_framework.permissions import AllowAny
from rest_framework.response import Response
from rest_framework.views import APIView
from shared.events.amplitude import AmplitudeEventPublisher

from codecov_auth.models import (
GITHUB_APP_INSTALLATION_DEFAULT_NAME,
Expand Down Expand Up @@ -498,9 +499,34 @@ def _handle_installation_events(
# GithubWebhookEvents.INSTALLTION_REPOSITORIES also execute this code
# because of deprecated flow. But the GithubAppInstallation shouldn't be changed
if event == GitHubWebhookEvents.INSTALLATION:
ghapp_installation, _ = GithubAppInstallation.objects.get_or_create(
installation_id=installation_id, owner=owner
ghapp_installation, was_created = (
GithubAppInstallation.objects.get_or_create(
installation_id=installation_id, owner=owner
)
)
if was_created:
installer_username = request.data.get("sender", {}).get(
"login", None
)
installer = (
Owner.objects.filter(
service=self.service_name,
username=installer_username,
).first()
if installer_username
else None
)
# If installer does not exist, just attribute the action to the org owner.
AmplitudeEventPublisher().publish(
"App Installed",
{
"user_ownerid": installer.ownerid
if installer is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check fails if installer is none because we're already doing the access on line 523. I don't really know how python handles it tbh but this example on a python sandbox gave me an error

stuff = {
    "ownerid": 1
}
stuff2 = stuff3["ownerid"] if stuff3 is not None else stuff["ownerid"]
print(stuff2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It think it's okay. installer will be defined but possibly None.

>>> stuff = { "ownerid": 1 }
>>> stuff2 = stuff3["ownerid"] if stuff3 is not None else stuff["ownerid"]
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    stuff2 = stuff3["ownerid"] if stuff3 is not None else stuff["ownerid"]
                                  ^^^^^^
NameError: name 'stuff3' is not defined. Did you mean: 'stuff'?
>>> stuff3 = None
>>> stuff2 = stuff3["ownerid"] if stuff3 is not None else stuff["ownerid"]
>>> stuff2
1

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah okay I missed that part, sounds good

else owner.ownerid,
"ownerid": owner.ownerid,
},
)

app_id = request.data["installation"]["app_id"]
# Either update or set
# But this value shouldn't change for the installation, so doesn't matter
Expand Down
Loading