Skip to content

Commit

Permalink
feat(cohorts): add activity logs (#21805)
Browse files Browse the repository at this point in the history
  • Loading branch information
mariusandra authored Jun 14, 2024
1 parent 5f119af commit 0ea5d6a
Show file tree
Hide file tree
Showing 9 changed files with 286 additions and 3 deletions.
3 changes: 3 additions & 0 deletions frontend/src/lib/components/ActivityLog/activityLogLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from 'lib/components/ActivityLog/humanizeActivity'
import { ACTIVITY_PAGE_SIZE } from 'lib/constants'
import { PaginationManual } from 'lib/lemon-ui/PaginationControl'
import { cohortActivityDescriber } from 'scenes/cohorts/activityDescriptions'
import { dataManagementActivityDescriber } from 'scenes/data-management/dataManagementDescribers'
import { flagActivityDescriber } from 'scenes/feature-flags/activityDescriptions'
import { notebookActivityDescriber } from 'scenes/notebooks/Notebook/notebookActivityDescriber'
Expand All @@ -36,6 +37,8 @@ export const describerFor = (logItem?: ActivityLogItem): Describer | undefined =
case ActivityScope.PLUGIN:
case ActivityScope.PLUGIN_CONFIG:
return pluginActivityDescriber
case ActivityScope.COHORT:
return cohortActivityDescriber
case ActivityScope.INSIGHT:
return insightActivityDescriber
case ActivityScope.PERSON:
Expand Down
55 changes: 55 additions & 0 deletions frontend/src/scenes/cohorts/activityDescriptions.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import '../../lib/components/Cards/InsightCard/InsightCard.scss'

import {
ActivityLogItem,
defaultDescriber,
HumanizedChange,
userNameForLogItem,
} from 'lib/components/ActivityLog/humanizeActivity'
import { Link } from 'lib/lemon-ui/Link'
import { urls } from 'scenes/urls'

const nameOrLinkToCohort = (id?: string | null, name?: string | null): string | JSX.Element => {
const displayName = name || '(empty string)'
return id ? <Link to={urls.cohort(id)}>{displayName}</Link> : displayName
}

export function cohortActivityDescriber(logItem: ActivityLogItem, asNotification?: boolean): HumanizedChange {
if (logItem.scope != 'Cohort') {
console.error('cohort describer received a non-cohort activity')
return { description: null }
}

if (logItem.activity == 'created') {
return {
description: (
<>
<strong>{userNameForLogItem(logItem)}</strong> created the cohort:{' '}
{nameOrLinkToCohort(logItem?.item_id, logItem?.detail.name)}
</>
),
}
}

if (logItem.activity == 'deleted') {
return {
description: (
<>
<strong>{userNameForLogItem(logItem)}</strong> deleted the cohort: {logItem.detail.name}
</>
),
}
}

if (logItem.activity == 'updated') {
return {
description: (
<>
<strong>{userNameForLogItem(logItem)}</strong> updated the cohort:{' '}
{nameOrLinkToCohort(logItem?.item_id, logItem?.detail.name)}
</>
),
}
}
return defaultDescriber(logItem, asNotification, nameOrLinkToCohort(logItem?.detail.short_id))
}
1 change: 1 addition & 0 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3686,6 +3686,7 @@ export enum ActivityScope {
SURVEY = 'Survey',
EARLY_ACCESS_FEATURE = 'EarlyAccessFeature',
COMMENT = 'Comment',
COHORT = 'Cohort',
TEAM = 'Team',
}

Expand Down
16 changes: 15 additions & 1 deletion posthog/api/activity_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

from posthog.api.routing import TeamAndOrgViewSetMixin
from posthog.api.shared import UserBasicSerializer
from posthog.models import ActivityLog, FeatureFlag, Insight, NotificationViewed, User
from posthog.models import ActivityLog, FeatureFlag, Insight, NotificationViewed, User, Cohort
from posthog.models.comment import Comment
from posthog.models.notebook.notebook import Notebook

Expand Down Expand Up @@ -114,6 +114,7 @@ def important_changes(self, request: Request, *args: Any, **kwargs: Any) -> Resp
my_comments = list(
Comment.objects.filter(created_by=user, team_id=self.team.pk).values_list("id", flat=True)
)
my_cohorts = list(Cohort.objects.filter(created_by=user, team_id=self.team.pk).values_list("id", flat=True))

# then things they edited
interesting_changes = [
Expand Down Expand Up @@ -168,6 +169,17 @@ def important_changes(self, request: Request, *args: Any, **kwargs: Any) -> Resp
.values_list("item_id", flat=True)
)

my_changed_cohorts = list(
ActivityLog.objects.filter(
team_id=self.team.id,
activity__in=interesting_changes,
user_id=user.pk,
scope="Cohort",
)
.exclude(item_id__in=my_cohorts)
.values_list("item_id", flat=True)
)

last_read_date = NotificationViewed.objects.filter(user=user).first()
last_read_filter = ""

Expand Down Expand Up @@ -218,6 +230,7 @@ def important_changes(self, request: Request, *args: Any, **kwargs: Any) -> Resp
& Q(id__in=deduplicated_notebook_activity_ids)
)
| Q(Q(scope="Comment") & Q(item_id__in=my_comments))
| Q(Q(scope="Cohort") & Q(item_id__in=my_cohorts))
)
| Q(
# don't want to see creation of these things since that was before the user edited these things
Expand All @@ -231,6 +244,7 @@ def important_changes(self, request: Request, *args: Any, **kwargs: Any) -> Resp
& Q(id__in=deduplicated_notebook_activity_ids)
)
| Q(Q(scope="Comment") & Q(item_id__in=my_changed_comments))
| Q(Q(scope="Cohort") & Q(item_id__in=my_changed_cohorts))
)
)
)
Expand Down
68 changes: 67 additions & 1 deletion posthog/api/cohort.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
from posthog.clickhouse.client.connection import Workload

from django.db import DatabaseError
from loginas.utils import is_impersonated_session
from sentry_sdk import start_span
import structlog

from posthog.models.activity_logging.activity_log import log_activity, Detail, changes_between, load_activity
from posthog.models.activity_logging.activity_page import activity_page_response
from posthog.models.feature_flag.flag_matching import (
FeatureFlagMatcher,
FlagsMatcherCache,
Expand All @@ -22,7 +25,7 @@
from django.db.models import QuerySet, Prefetch, prefetch_related_objects, OuterRef, Subquery
from django.db.models.expressions import F
from django.utils import timezone
from rest_framework import serializers, viewsets
from rest_framework import serializers, viewsets, request, status
from rest_framework.decorators import action
from rest_framework.exceptions import ValidationError
from rest_framework.request import Request
Expand Down Expand Up @@ -403,6 +406,69 @@ def persons(self, request: Request, **kwargs) -> Response:

return Response({"results": serialized_actors, "next": next_url, "previous": previous_url})

@action(methods=["GET"], url_path="activity", detail=False, required_scopes=["activity_log:read"])
def all_activity(self, request: request.Request, **kwargs):
limit = int(request.query_params.get("limit", "10"))
page = int(request.query_params.get("page", "1"))

activity_page = load_activity(scope="Cohort", team_id=self.team_id, limit=limit, page=page)

return activity_page_response(activity_page, limit, page, request)

@action(methods=["GET"], detail=True, required_scopes=["activity_log:read"])
def activity(self, request: request.Request, **kwargs):
limit = int(request.query_params.get("limit", "10"))
page = int(request.query_params.get("page", "1"))

item_id = kwargs["pk"]
if not Cohort.objects.filter(id=item_id, team_id=self.team_id).exists():
return Response("", status=status.HTTP_404_NOT_FOUND)

activity_page = load_activity(
scope="Cohort",
team_id=self.team_id,
item_ids=[str(item_id)],
limit=limit,
page=page,
)
return activity_page_response(activity_page, limit, page, request)

def perform_create(self, serializer):
serializer.save()
log_activity(
organization_id=self.organization.id,
team_id=self.team_id,
user=serializer.context["request"].user,
was_impersonated=is_impersonated_session(serializer.context["request"]),
item_id=serializer.instance.id,
scope="Cohort",
activity="created",
detail=Detail(name=serializer.instance.name),
)

def perform_update(self, serializer):
instance_id = serializer.instance.id

try:
before_update = Cohort.objects.get(pk=instance_id)
except Cohort.DoesNotExist:
before_update = None

serializer.save()

changes = changes_between("Cohort", previous=before_update, current=serializer.instance)

log_activity(
organization_id=self.organization.id,
team_id=self.team_id,
user=serializer.context["request"].user,
was_impersonated=is_impersonated_session(serializer.context["request"]),
item_id=instance_id,
scope="Cohort",
activity="updated",
detail=Detail(changes=changes, name=serializer.instance.name),
)


class LegacyCohortViewSet(CohortViewSet):
derive_current_team_from_user_only = True
Expand Down
1 change: 1 addition & 0 deletions posthog/api/test/__snapshots__/test_api_docs.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
'Warning: operationId "batch_exports_pause_create" has collisions [(\'/api/organizations/{organization_id}/batch_exports/{id}/pause/\', \'post\'), (\'/api/projects/{project_id}/batch_exports/{id}/pause/\', \'post\')]. resolving with numeral suffixes.',
'Warning: operationId "batch_exports_unpause_create" has collisions [(\'/api/organizations/{organization_id}/batch_exports/{id}/unpause/\', \'post\'), (\'/api/projects/{project_id}/batch_exports/{id}/unpause/\', \'post\')]. resolving with numeral suffixes.',
'Warning: operationId "app_metrics_historical_exports_retrieve" has collisions [(\'/api/projects/{project_id}/app_metrics/{plugin_config_id}/historical_exports/\', \'get\'), (\'/api/projects/{project_id}/app_metrics/{plugin_config_id}/historical_exports/{id}/\', \'get\')]. resolving with numeral suffixes.',
'Warning: operationId "cohorts_activity_retrieve" has collisions [(\'/api/projects/{project_id}/cohorts/{id}/activity/\', \'get\'), (\'/api/projects/{project_id}/cohorts/activity/\', \'get\')]. resolving with numeral suffixes.',
'Warning: operationId "event_definitions_retrieve" has collisions [(\'/api/projects/{project_id}/event_definitions/\', \'get\'), (\'/api/projects/{project_id}/event_definitions/{id}/\', \'get\')]. resolving with numeral suffixes.',
'Warning: operationId "feature_flags_activity_retrieve" has collisions [(\'/api/projects/{project_id}/feature_flags/{id}/activity/\', \'get\'), (\'/api/projects/{project_id}/feature_flags/activity/\', \'get\')]. resolving with numeral suffixes.',
'Warning: operationId "insights_activity_retrieve" has collisions [(\'/api/projects/{project_id}/insights/{id}/activity/\', \'get\'), (\'/api/projects/{project_id}/insights/activity/\', \'get\')]. resolving with numeral suffixes.',
Expand Down
18 changes: 18 additions & 0 deletions posthog/api/test/__snapshots__/test_insight.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -1628,6 +1628,24 @@
LIMIT 21
'''
# ---
# name: TestInsight.test_listing_insights_does_not_nplus1.30
'''
SELECT "posthog_taggeditem"."id",
"posthog_taggeditem"."tag_id",
"posthog_taggeditem"."dashboard_id",
"posthog_taggeditem"."insight_id",
"posthog_taggeditem"."event_definition_id",
"posthog_taggeditem"."property_definition_id",
"posthog_taggeditem"."action_id",
"posthog_taggeditem"."feature_flag_id"
FROM "posthog_taggeditem"
WHERE "posthog_taggeditem"."insight_id" IN (1,
2,
3,
4,
5 /* ... */)
'''
# ---
# name: TestInsight.test_listing_insights_does_not_nplus1.4
'''
SELECT "posthog_team"."id",
Expand Down
118 changes: 117 additions & 1 deletion posthog/api/test/test_cohort.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json
from datetime import datetime, timedelta
from typing import Any
from typing import Optional, Any
from unittest import mock
from unittest.mock import patch

from django.core.files.uploadedfile import SimpleUploadedFile
Expand Down Expand Up @@ -34,6 +35,31 @@ class TestCohort(TestExportMixin, ClickhouseTestMixin, APIBaseTest, QueryMatchin
def capture_select_queries(self):
return self.capture_queries(("INSERT INTO cohortpeople", "SELECT", "ALTER", "select", "DELETE"))

def _get_cohort_activity(
self,
flag_id: Optional[int] = None,
team_id: Optional[int] = None,
expected_status: int = status.HTTP_200_OK,
):
if team_id is None:
team_id = self.team.id

if flag_id:
url = f"/api/projects/{team_id}/cohorts/{flag_id}/activity"
else:
url = f"/api/projects/{team_id}/cohorts/activity"

activity = self.client.get(url)
self.assertEqual(activity.status_code, expected_status)
return activity.json()

def assert_cohort_activity(self, cohort_id: Optional[int], expected: list[dict]):
activity_response = self._get_cohort_activity(cohort_id)

activity: list[dict] = activity_response["results"]
self.maxDiff = None
assert activity == expected

@patch("posthog.api.cohort.report_user_action")
@patch("posthog.tasks.calculate_cohort.calculate_cohort_ch.delay", side_effect=calculate_cohort_ch)
@patch("posthog.models.cohort.util.sync_execute", side_effect=sync_execute)
Expand Down Expand Up @@ -281,6 +307,96 @@ def test_cohort_list(self):
self.assertEqual(response["results"][0]["name"], "whatever")
self.assertEqual(response["results"][0]["created_by"]["id"], self.user.id)

def test_cohort_activity_log(self):
self.team.app_urls = ["http://somewebsite.com"]
self.team.save()
Person.objects.create(team=self.team, properties={"prop": 5})
Person.objects.create(team=self.team, properties={"prop": 6})

self.client.post(
f"/api/projects/{self.team.id}/cohorts",
data={"name": "whatever", "groups": [{"properties": {"prop": 5}}]},
)

cohort = Cohort.objects.filter(team=self.team).last()
assert cohort is not None

self.assert_cohort_activity(
cohort_id=cohort.pk,
expected=[
{
"user": {"first_name": "", "email": "[email protected]"},
"activity": "created",
"scope": "Cohort",
"item_id": str(cohort.pk),
"detail": {"changes": None, "trigger": None, "name": "whatever", "short_id": None, "type": None},
"created_at": mock.ANY,
}
],
)

self.client.patch(
f"/api/projects/{self.team.id}/cohorts/{cohort.pk}",
data={"name": "woohoo", "groups": [{"properties": {"prop": 6}}]},
)
cohort.refresh_from_db()
assert cohort.name == "woohoo"

self.assert_cohort_activity(
cohort_id=cohort.pk,
expected=[
{
"user": {"first_name": "", "email": "[email protected]"},
"activity": "updated",
"scope": "Cohort",
"item_id": str(cohort.pk),
"detail": {
"changes": [
{
"type": "Cohort",
"action": "changed",
"field": "name",
"before": "whatever",
"after": "woohoo",
},
{
"type": "Cohort",
"action": "changed",
"field": "groups",
"before": [
{
"days": None,
"count": None,
"label": None,
"end_date": None,
"event_id": None,
"action_id": None,
"properties": [{"key": "prop", "type": "person", "value": 5}],
"start_date": None,
"count_operator": None,
}
],
"after": [{"properties": [{"key": "prop", "type": "person", "value": 6}]}],
},
],
"trigger": None,
"name": "woohoo",
"short_id": None,
"type": None,
},
"created_at": mock.ANY,
},
{
"user": {"first_name": "", "email": "[email protected]"},
"activity": "created",
"scope": "Cohort",
"item_id": str(cohort.pk),
"detail": {"changes": None, "trigger": None, "name": "whatever", "short_id": None, "type": None},
"created_at": mock.ANY,
},
],
)

def test_csv_export_new(self):
# Test 100s of distinct_ids, we only want ~10
Person.objects.create(
Expand Down
Loading

0 comments on commit 0ea5d6a

Please sign in to comment.