Skip to content

Commit

Permalink
feat(hogql): test account filters for EventsQuery and HogQLQuery (#20308
Browse files Browse the repository at this point in the history
)
  • Loading branch information
mariusandra authored Feb 14, 2024
1 parent 535e5e6 commit ca20bac
Show file tree
Hide file tree
Showing 16 changed files with 176 additions and 9 deletions.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
77 changes: 77 additions & 0 deletions frontend/src/queries/nodes/DataNode/TestAccountFilters.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { useActions, useValues } from 'kea'
import { IconSettings } from 'lib/lemon-ui/icons'
import { LemonButton } from 'lib/lemon-ui/LemonButton'
import { LemonSwitch } from 'lib/lemon-ui/LemonSwitch'
import { filterTestAccountsDefaultsLogic } from 'scenes/settings/project/filterTestAccountDefaultsLogic'
import { teamLogic } from 'scenes/teamLogic'
import { urls } from 'scenes/urls'

import { DataNode, EventsQuery, HogQLQuery } from '~/queries/schema'
import { isEventsQuery, isHogQLQuery } from '~/queries/utils'

interface TestAccountFiltersProps {
query: DataNode
setQuery?: (query: EventsQuery | HogQLQuery) => void
}
export function TestAccountFilters({ query, setQuery }: TestAccountFiltersProps): JSX.Element | null {
const { currentTeam } = useValues(teamLogic)
const hasFilters = (currentTeam?.test_account_filters || []).length > 0
const { setLocalDefault } = useActions(filterTestAccountsDefaultsLogic)

if (!isEventsQuery(query) && !isHogQLQuery(query)) {
return null
}
const checked = hasFilters
? !!(isHogQLQuery(query)
? query.filters?.filterTestAccounts
: isEventsQuery(query)
? query.filterTestAccounts
: false)
: false
const onChange = isHogQLQuery(query)
? (checked: boolean) => {
const newQuery: HogQLQuery = {
...query,
filters: {
...query.filters,
filterTestAccounts: checked,
},
}
setQuery?.(newQuery)
}
: isEventsQuery(query)
? (checked: boolean) => {
const newQuery: EventsQuery = {
...query,
filterTestAccounts: checked,
}
setQuery?.(newQuery)
}
: undefined

return (
<LemonSwitch
checked={checked}
onChange={(checked: boolean) => {
onChange?.(checked)
setLocalDefault(checked)
}}
id="test-account-filter"
bordered
label={
<div className="flex items-center">
<span>Filter out internal and test users</span>
<LemonButton
icon={<IconSettings />}
to={urls.settings('project-product-analytics', 'internal-user-filtering')}
size="small"
noPadding
className="ml-1"
/>
</div>
}
disabledReason={!hasFilters ? "You haven't set any internal and test filters" : null}
/>
)
return null
}
5 changes: 5 additions & 0 deletions frontend/src/queries/nodes/DataTable/DataTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { DateRange } from '~/queries/nodes/DataNode/DateRange'
import { ElapsedTime } from '~/queries/nodes/DataNode/ElapsedTime'
import { LoadNext } from '~/queries/nodes/DataNode/LoadNext'
import { Reload } from '~/queries/nodes/DataNode/Reload'
import { TestAccountFilters } from '~/queries/nodes/DataNode/TestAccountFilters'
import { BackToSource } from '~/queries/nodes/DataTable/BackToSource'
import { ColumnConfigurator } from '~/queries/nodes/DataTable/ColumnConfigurator/ColumnConfigurator'
import { DataTableExport } from '~/queries/nodes/DataTable/DataTableExport'
Expand Down Expand Up @@ -130,6 +131,7 @@ export function DataTable({
const {
showActions,
showDateRange,
showTestAccountFilters,
showSearch,
showEventFilter,
showPropertyFilter,
Expand Down Expand Up @@ -420,6 +422,9 @@ export function DataTable({
].filter((x) => !!x)

const firstRowRight = [
showTestAccountFilters && sourceFeatures.has(QueryFeature.testAccountFilters) ? (
<TestAccountFilters key="test-account-filters" query={query.source} setQuery={setQuerySource} />
) : null,
showSavedQueries && sourceFeatures.has(QueryFeature.savedEventsQueries) ? (
<SavedQueries key="saved-queries" query={query} setQuery={setQuery} />
) : null,
Expand Down
1 change: 1 addition & 0 deletions frontend/src/queries/nodes/DataTable/dataTableLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ export const dataTableLogic = kea<dataTableLogicType>([
showSearch: query.showSearch ?? showIfFull,
showActions: query.showActions ?? true,
showDateRange: query.showDateRange ?? showIfFull,
showTestAccountFilters: query.showTestAccountFilters ?? showIfFull,
showExport: query.showExport ?? showIfFull,
showReload: query.showReload ?? showIfFull,
showTimings: query.showTimings ?? flagQueryTimingsEnabled,
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/queries/nodes/DataTable/queryFeatures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export enum QueryFeature {
selectAndOrderByColumns,
displayResponseError,
hideLoadNextButton,
testAccountFilters,
}

export function getQueryFeatures(query: Node): Set<QueryFeature> {
Expand All @@ -34,6 +35,7 @@ export function getQueryFeatures(query: Node): Set<QueryFeature> {
features.add(QueryFeature.eventPropertyFilters)
features.add(QueryFeature.resultIsArrayOfArrays)
features.add(QueryFeature.displayResponseError)
features.add(QueryFeature.testAccountFilters)
}

if (isEventsQuery(query)) {
Expand Down
15 changes: 15 additions & 0 deletions frontend/src/queries/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,10 @@
"description": "Include a free text search field (PersonsNode only)",
"type": "boolean"
},
"showTestAccountFilters": {
"description": "Show filter to exclude test accounts",
"type": "boolean"
},
"showTimings": {
"description": "Show a detailed query timing breakdown",
"type": "boolean"
Expand Down Expand Up @@ -1132,6 +1136,10 @@
"description": "Limit to events matching this string",
"type": ["string", "null"]
},
"filterTestAccounts": {
"description": "Filter test accounts",
"type": "boolean"
},
"fixedProperties": {
"description": "Fixed properties in the query, can't be edited in the interface (e.g. scoping down by person)",
"items": {
Expand Down Expand Up @@ -1962,6 +1970,9 @@
"dateRange": {
"$ref": "#/definitions/DateRange"
},
"filterTestAccounts": {
"type": "boolean"
},
"properties": {
"items": {
"$ref": "#/definitions/AnyPropertyFilter"
Expand Down Expand Up @@ -4341,6 +4352,10 @@
"showTable": {
"type": "boolean"
},
"showTestAccountFilters": {
"description": "Show filter to exclude test accounts",
"type": "boolean"
},
"showTimings": {
"description": "Show a detailed query timing breakdown",
"type": "boolean"
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/queries/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ export interface HogQLQueryResponse {
export interface HogQLFilters {
properties?: AnyPropertyFilter[]
dateRange?: DateRange
filterTestAccounts?: boolean
}

export interface HogQLQuery extends DataNode {
Expand Down Expand Up @@ -409,6 +410,8 @@ export interface EventsQuery extends DataNode {
properties?: AnyPropertyFilter[]
/** Fixed properties in the query, can't be edited in the interface (e.g. scoping down by person) */
fixedProperties?: AnyPropertyFilter[]
/** Filter test accounts */
filterTestAccounts?: boolean
/** Limit to events matching this string */
event?: string | null
/**
Expand Down Expand Up @@ -508,6 +511,8 @@ interface DataTableNodeViewProps {
showSearch?: boolean
/** Include a property filter above the table */
showPropertyFilter?: boolean
/** Show filter to exclude test accounts */
showTestAccountFilters?: boolean
/** Include a HogQL query editor above HogQL tables */
showHogQLEditor?: boolean
/** Show the kebab menu at the end of the row */
Expand Down
4 changes: 0 additions & 4 deletions mypy-baseline.txt
Original file line number Diff line number Diff line change
Expand Up @@ -590,10 +590,6 @@ posthog/hogql_queries/insights/test/test_paginators.py:0: error: Argument 2 to "
posthog/hogql_queries/insights/test/test_paginators.py:0: error: Value of type "object" is not indexable [index]
posthog/hogql_queries/insights/test/test_paginators.py:0: error: Value of type "object" is not indexable [index]
posthog/hogql_queries/insights/test/test_paginators.py:0: error: Value of type "object" is not indexable [index]
posthog/hogql_queries/insights/test/test_events_query_runner.py:0: error: Item "Expr" of "Expr | None" has no attribute "exprs" [union-attr]
posthog/hogql_queries/insights/test/test_events_query_runner.py:0: error: Item "None" of "Expr | None" has no attribute "exprs" [union-attr]
posthog/hogql_queries/insights/test/test_events_query_runner.py:0: error: Item "Expr" of "Expr | None" has no attribute "exprs" [union-attr]
posthog/hogql_queries/insights/test/test_events_query_runner.py:0: error: Item "None" of "Expr | None" has no attribute "exprs" [union-attr]
posthog/hogql/test/test_timings.py:0: error: No overload variant of "__setitem__" of "list" matches argument types "int", "float" [call-overload]
posthog/hogql/test/test_timings.py:0: note: Possible overload variants:
posthog/hogql/test/test_timings.py:0: note: def __setitem__(self, SupportsIndex, int, /) -> None
Expand Down
4 changes: 4 additions & 0 deletions posthog/hogql/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ def visit_placeholder(self, node):
)
)

if self.filters.filterTestAccounts:
for prop in self.team.test_account_filters or []:
exprs.append(property_to_expr(prop, self.team))

if len(exprs) == 0:
return ast.Constant(value=True)
if len(exprs) == 1:
Expand Down
26 changes: 25 additions & 1 deletion posthog/hogql/test/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def _print_ast(self, node: ast.Expr):
context=HogQLContext(team_id=self.team.pk, enable_select_queries=True),
)

def test_replace_filters(self):
def test_replace_filters_empty(self):
select = replace_filters(self._parse_select("SELECT event FROM events"), HogQLFilters(), self.team)
self.assertEqual(self._print_ast(select), "SELECT event FROM events LIMIT 10000")

Expand All @@ -42,6 +42,7 @@ def test_replace_filters(self):
)
self.assertEqual(self._print_ast(select), "SELECT event FROM events WHERE true LIMIT 10000")

def test_replace_filters_date_range(self):
select = replace_filters(
self._parse_select("SELECT event FROM events where {filters}"),
HogQLFilters(dateRange=DateRange(date_from="2020-02-02")),
Expand All @@ -62,6 +63,7 @@ def test_replace_filters(self):
"SELECT event FROM events WHERE less(timestamp, toDateTime('2020-02-02 00:00:00.000000')) LIMIT 10000",
)

def test_replace_filters_event_property(self):
select = replace_filters(
self._parse_select("SELECT event FROM events where {filters}"),
HogQLFilters(
Expand All @@ -74,6 +76,7 @@ def test_replace_filters(self):
"SELECT event FROM events WHERE equals(properties.random_uuid, '123') LIMIT 10000",
)

def test_replace_filters_person_property(self):
select = replace_filters(
self._parse_select("SELECT event FROM events where {filters}"),
HogQLFilters(
Expand All @@ -100,3 +103,24 @@ def test_replace_filters(self):
self._print_ast(select),
"SELECT event FROM events WHERE and(equals(properties.random_uuid, '123'), equals(person.properties.random_uuid, '123')) LIMIT 10000",
)

def test_replace_filters_test_accounts(self):
self.team.test_account_filters = [
{
"key": "email",
"type": "person",
"value": "posthog.com",
"operator": "not_icontains",
}
]
self.team.save()

select = replace_filters(
self._parse_select("SELECT event FROM events where {filters}"),
HogQLFilters(filterTestAccounts=True),
self.team,
)
self.assertEqual(
self._print_ast(select),
"SELECT event FROM events WHERE notILike(person.properties.email, '%posthog.com%') LIMIT 10000",
)
4 changes: 4 additions & 0 deletions posthog/hogql_queries/events_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ def to_query(self) -> ast.SelectQuery:
timings=self.timings,
)
)
if self.query.filterTestAccounts:
with self.timings.measure("test_account_filters"):
for prop in self.team.test_account_filters or []:
where_exprs.append(property_to_expr(prop, self.team))

with self.timings.measure("timestamps"):
# prevent accidentally future events from being visible by default
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
from typing import Tuple, Any
from typing import Tuple, Any, cast

from freezegun import freeze_time

from posthog.hogql import ast
from posthog.hogql.ast import CompareOperationOp
from posthog.hogql_queries.events_query_runner import EventsQueryRunner
from posthog.models import Person, Team
from posthog.models.organization import Organization
Expand Down Expand Up @@ -123,9 +125,30 @@ def test_person_id_expands_to_distinct_ids(self):

# matching team
query_ast = EventsQueryRunner(query=query, team=self.team).to_query()
self.assertEqual(query_ast.where.exprs[0].right.value, ["id1", "id2"])
where_expr = cast(ast.CompareOperation, cast(ast.And, query_ast.where).exprs[0])
right_expr = cast(ast.Constant, where_expr.right)
self.assertEqual(right_expr.value, ["id1", "id2"])

# another team
another_team = Team.objects.create(organization=Organization.objects.create())
query_ast = EventsQueryRunner(query=query, team=another_team).to_query()
self.assertEqual(query_ast.where.exprs[0].right.value, [])
where_expr = cast(ast.CompareOperation, cast(ast.And, query_ast.where).exprs[0])
right_expr = cast(ast.Constant, where_expr.right)
self.assertEqual(right_expr.value, [])

def test_test_account_filters(self):
self.team.test_account_filters = [
{
"key": "email",
"type": "person",
"value": "posthog.com",
"operator": "not_icontains",
}
]
self.team.save()
query = EventsQuery(kind="EventsQuery", select=["*"], filterTestAccounts=True)
query_ast = EventsQueryRunner(query=query, team=self.team).to_query()
where_expr = cast(ast.CompareOperation, cast(ast.And, query_ast.where).exprs[0])
right_expr = cast(ast.Constant, where_expr.right)
self.assertEqual(right_expr.value, "%posthog.com%")
self.assertEqual(where_expr.op, CompareOperationOp.NotILike)
9 changes: 8 additions & 1 deletion posthog/models/test/test_insight_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
{},
{
"dateRange": {"date_from": "-14d", "date_to": "-7d"},
"filterTestAccounts": None,
"properties": None,
},
),
Expand All @@ -153,6 +154,7 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
{"date_from": "-14d", "date_to": "-7d"},
{
"dateRange": {"date_from": "-14d", "date_to": "-7d"},
"filterTestAccounts": None,
"properties": None,
},
),
Expand All @@ -162,6 +164,7 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
{"date_from": "-4d", "date_to": "-3d"},
{
"dateRange": {"date_from": "-4d", "date_to": "-3d"},
"filterTestAccounts": None,
"properties": None,
},
),
Expand All @@ -171,21 +174,23 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
{"date_from": "all"},
{
"dateRange": {"date_from": "all", "date_to": None},
"filterTestAccounts": None,
"properties": None,
},
),
(
# test that if no filters are set then none are outputted
{},
{},
{"dateRange": {"date_from": None, "date_to": None}, "properties": None},
{"dateRange": {"date_from": None, "date_to": None}, "filterTestAccounts": None, "properties": None},
),
(
# test that properties from the query are used when there are no dashboard properties
{"properties": [browser_equals_firefox]},
{},
{
"dateRange": {"date_from": None, "date_to": None},
"filterTestAccounts": None,
"properties": [browser_equals_firefox],
},
),
Expand All @@ -195,6 +200,7 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
{"properties": [browser_equals_chrome]},
{
"dateRange": {"date_from": None, "date_to": None},
"filterTestAccounts": None,
"properties": [browser_equals_chrome],
},
),
Expand All @@ -204,6 +210,7 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
{"properties": [browser_equals_chrome]},
{
"dateRange": {"date_from": None, "date_to": None},
"filterTestAccounts": None,
"properties": [browser_equals_firefox, browser_equals_chrome],
},
),
Expand Down
Loading

0 comments on commit ca20bac

Please sign in to comment.