diff --git a/frontend/__snapshots__/lemon-ui-textfit--basic--dark.png b/frontend/__snapshots__/lemon-ui-textfit--basic--dark.png index 838cc6a9e982a..4f4bdb16f4b86 100644 Binary files a/frontend/__snapshots__/lemon-ui-textfit--basic--dark.png and b/frontend/__snapshots__/lemon-ui-textfit--basic--dark.png differ diff --git a/frontend/__snapshots__/lemon-ui-textfit--basic--light.png b/frontend/__snapshots__/lemon-ui-textfit--basic--light.png index 880237b5eb6a3..ee8181260f814 100644 Binary files a/frontend/__snapshots__/lemon-ui-textfit--basic--light.png and b/frontend/__snapshots__/lemon-ui-textfit--basic--light.png differ diff --git a/frontend/__snapshots__/scenes-app-dashboards--new--dark.png b/frontend/__snapshots__/scenes-app-dashboards--new--dark.png index 172e03c10b837..7248234a801b1 100644 Binary files a/frontend/__snapshots__/scenes-app-dashboards--new--dark.png and b/frontend/__snapshots__/scenes-app-dashboards--new--dark.png differ diff --git a/frontend/__snapshots__/scenes-app-dashboards--new--light.png b/frontend/__snapshots__/scenes-app-dashboards--new--light.png index 3c22220e0ff40..9bf080734d628 100644 Binary files a/frontend/__snapshots__/scenes-app-dashboards--new--light.png and b/frontend/__snapshots__/scenes-app-dashboards--new--light.png differ diff --git a/frontend/__snapshots__/scenes-app-insights--funnel-left-to-right-breakdown-edit--light--webkit.png b/frontend/__snapshots__/scenes-app-insights--funnel-left-to-right-breakdown-edit--light--webkit.png index 2d0d46a5a5626..32844b0dc0344 100644 Binary files a/frontend/__snapshots__/scenes-app-insights--funnel-left-to-right-breakdown-edit--light--webkit.png and b/frontend/__snapshots__/scenes-app-insights--funnel-left-to-right-breakdown-edit--light--webkit.png differ diff --git a/frontend/__snapshots__/scenes-app-insights--lifecycle-edit--light--webkit.png b/frontend/__snapshots__/scenes-app-insights--lifecycle-edit--light--webkit.png index 04f60dc54859c..59c11eff3baf4 100644 Binary files a/frontend/__snapshots__/scenes-app-insights--lifecycle-edit--light--webkit.png and b/frontend/__snapshots__/scenes-app-insights--lifecycle-edit--light--webkit.png differ diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index 5255a2015116e..93d80167f6ca7 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -32,6 +32,7 @@ export const defaultMocks: Mocks = { '/api/projects/:team_id/event_definitions/': EMPTY_PAGINATED_RESPONSE, '/api/projects/:team_id/cohorts/': toPaginatedResponse([MOCK_DEFAULT_COHORT]), '/api/projects/:team_id/dashboards/': EMPTY_PAGINATED_RESPONSE, + '/api/projects/:team_id/dashboard_templates': EMPTY_PAGINATED_RESPONSE, '/api/projects/:team_id/dashboard_templates/repository/': [], '/api/projects/:team_id/notebooks': () => { // this was matching on `?contains=query` but that made MSW unhappy and seems unnecessary diff --git a/frontend/src/scenes/dashboard/DashboardTemplateChooser.tsx b/frontend/src/scenes/dashboard/DashboardTemplateChooser.tsx index 1d2f55a0270dd..0efb9876bd5fd 100644 --- a/frontend/src/scenes/dashboard/DashboardTemplateChooser.tsx +++ b/frontend/src/scenes/dashboard/DashboardTemplateChooser.tsx @@ -3,20 +3,20 @@ import './DashboardTemplateChooser.scss' import clsx from 'clsx' import { useActions, useValues } from 'kea' import { FallbackCoverImage } from 'lib/components/FallbackCoverImage/FallbackCoverImage' +import { Spinner } from 'lib/lemon-ui/Spinner' import BlankDashboardHog from 'public/blank-dashboard-hog.png' import { useState } from 'react' -import { dashboardTemplatesLogic } from 'scenes/dashboard/dashboards/templates/dashboardTemplatesLogic' +import { + DashboardTemplateProps, + dashboardTemplatesLogic, +} from 'scenes/dashboard/dashboards/templates/dashboardTemplatesLogic' import { newDashboardLogic } from 'scenes/dashboard/newDashboardLogic' -import { DashboardTemplateScope, DashboardTemplateType } from '~/types' +import { DashboardTemplateType } from '~/types' -interface DashboardTemplateChooserProps { - scope?: DashboardTemplateScope -} - -export function DashboardTemplateChooser({ scope = 'global' }: DashboardTemplateChooserProps): JSX.Element { +export function DashboardTemplateChooser({ scope = 'default' }: DashboardTemplateProps): JSX.Element { const templatesLogic = dashboardTemplatesLogic({ scope }) - const { allTemplates } = useValues(templatesLogic) + const { allTemplates, allTemplatesLoading } = useValues(templatesLogic) const { isLoading, newDashboardModalVisible } = useValues(newDashboardLogic) const { @@ -49,35 +49,38 @@ export function DashboardTemplateChooser({ scope = 'global' }: DashboardTemplate index={0} data-attr="create-dashboard-blank" /> - {allTemplates.map((template, index) => ( - { - if (isLoading) { - return - } - setIsLoading(true) - // while we might receive templates from the external repository - // we need to handle templates that don't have variables - if ((template.variables || []).length === 0) { - if (template.variables === null) { - template.variables = [] + {allTemplatesLoading ? ( + + ) : ( + allTemplates.map((template, index) => ( + { + if (isLoading) { + return } - createDashboardFromTemplate(template, template.variables || []) - } else { - if (!newDashboardModalVisible) { - showVariableSelectModal(template) + setIsLoading(true) + // while we might receive templates from the external repository + // we need to handle templates that don't have variables + if ((template.variables || []).length === 0) { + if (template.variables === null) { + template.variables = [] + } + createDashboardFromTemplate(template, template.variables || []) } else { - setActiveDashboardTemplate(template) + if (!newDashboardModalVisible) { + showVariableSelectModal(template) + } else { + setActiveDashboardTemplate(template) + } } - } - }} - index={index + 1} - data-attr="create-dashboard-from-template" - /> - ))} - {/*TODO @lukeharries should we have an empty state here? When no templates let people know what to do?*/} + }} + index={index + 1} + data-attr="create-dashboard-from-template" + /> + )) + )} ) diff --git a/frontend/src/scenes/dashboard/NewDashboardModal.tsx b/frontend/src/scenes/dashboard/NewDashboardModal.tsx index fa8373f21c170..5cd2d74953f4c 100644 --- a/frontend/src/scenes/dashboard/NewDashboardModal.tsx +++ b/frontend/src/scenes/dashboard/NewDashboardModal.tsx @@ -1,7 +1,8 @@ -import { LemonButton } from '@posthog/lemon-ui' +import { LemonButton, LemonInput } from '@posthog/lemon-ui' import { useActions, useMountedLogic, useValues } from 'kea' import { LemonModal } from 'lib/lemon-ui/LemonModal' import { pluralize } from 'lib/utils' +import { dashboardTemplatesLogic } from 'scenes/dashboard/dashboards/templates/dashboardTemplatesLogic' import { newDashboardLogic } from 'scenes/dashboard/newDashboardLogic' import { DashboardTemplateChooser } from './DashboardTemplateChooser' @@ -43,6 +44,12 @@ export function NewDashboardModal(): JSX.Element { const { hideNewDashboardModal } = useActions(newDashboardLogic) const { newDashboardModalVisible, activeDashboardTemplate } = useValues(newDashboardLogic) + const templatesLogic = dashboardTemplatesLogic({ + scope: builtLogic.props.featureFlagId ? 'feature_flag' : 'default', + }) + const { templateFilter } = useValues(templatesLogic) + const { setTemplateFilter } = useActions(templatesLogic) + const _dashboardTemplateChooser = builtLogic.props.featureFlagId ? ( ) : ( @@ -61,7 +68,18 @@ export function NewDashboardModal(): JSX.Element { {pluralize((activeDashboardTemplate.variables || []).length, 'event', 'events', true)}.

) : ( - 'Choose a template or start with a blank slate' +
+
Choose a template or start with a blank slate
+
+ +
+
) } > diff --git a/frontend/src/scenes/dashboard/dashboards/templates/dashboardTemplatesLogic.tsx b/frontend/src/scenes/dashboard/dashboards/templates/dashboardTemplatesLogic.tsx index eeceeeb28d23a..f199b7cd2eeaf 100644 --- a/frontend/src/scenes/dashboard/dashboards/templates/dashboardTemplatesLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboards/templates/dashboardTemplatesLogic.tsx @@ -1,4 +1,4 @@ -import { actions, afterMount, connect, kea, key, path, props } from 'kea' +import { actions, afterMount, connect, kea, key, listeners, path, props, reducers } from 'kea' import { loaders } from 'kea-loaders' import api from 'lib/api' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' @@ -8,7 +8,8 @@ import { DashboardTemplateScope, DashboardTemplateType } from '~/types' import type { dashboardTemplatesLogicType } from './dashboardTemplatesLogicType' export interface DashboardTemplateProps { - scope?: DashboardTemplateScope + // default is to present global templates _and_ those visible only in the current team + scope?: 'default' | DashboardTemplateScope } export const dashboardTemplatesLogic = kea([ @@ -20,18 +21,40 @@ export const dashboardTemplatesLogic = kea([ }), actions({ setTemplates: (allTemplates: DashboardTemplateType[]) => ({ allTemplates }), + setTemplateFilter: (search: string) => ({ search }), }), - loaders(({ props }) => ({ + reducers({ + templateFilter: [ + '' as string, + { + setTemplateFilter: (_, { search }) => { + return search + }, + }, + ], + }), + loaders(({ props, values }) => ({ allTemplates: [ [] as DashboardTemplateType[], { getAllTemplates: async () => { - const page = await api.dashboardTemplates.list(props) + const params = { + // the backend doesn't know about a default scope + scope: props.scope !== 'default' ? props.scope : undefined, + search: values.templateFilter.length > 2 ? values.templateFilter : undefined, + } + const page = await api.dashboardTemplates.list(params) return page.results }, }, ], })), + listeners(({ actions }) => ({ + setTemplateFilter: async (_, breakpoint) => { + await breakpoint(100) + actions.getAllTemplates() + }, + })), afterMount(({ actions }) => { actions.getAllTemplates() }), diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 836e272388f38..b4911bbc14adc 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -1445,6 +1445,8 @@ export interface DashboardBasicType { export interface DashboardTemplateListParams { scope?: DashboardTemplateScope + // matches on template name, description, and tags + search?: string } export type DashboardTemplateScope = 'team' | 'global' | 'feature_flag' diff --git a/posthog/api/dashboards/dashboard_templates.py b/posthog/api/dashboards/dashboard_templates.py index c35986ef5431f..4c3092bbe6099 100644 --- a/posthog/api/dashboards/dashboard_templates.py +++ b/posthog/api/dashboards/dashboard_templates.py @@ -90,5 +90,22 @@ def json_schema(self, request: request.Request, **kwargs) -> response.Response: def get_queryset(self, *args, **kwargs): filters = self.request.GET.dict() - default_condition = Q(team_id=self.team_id) | Q(scope=DashboardTemplate.Scope.GLOBAL) - return DashboardTemplate.objects.filter(Q(**filters) if filters else default_condition) + scope = filters.pop("scope", None) + search = filters.pop("search", None) + + # if scope is feature flag, then only return feature flag templates + # they're implicitly global, so they are not associated with any teams + if scope == DashboardTemplate.Scope.FEATURE_FLAG: + query_condition = Q(scope=DashboardTemplate.Scope.FEATURE_FLAG) + elif scope == DashboardTemplate.Scope.GLOBAL: + query_condition = Q(scope=DashboardTemplate.Scope.GLOBAL) + # otherwise we are in the new dashboard context so show global templates and ones from this team + else: + query_condition = Q(team_id=self.team_id) | Q(scope=DashboardTemplate.Scope.GLOBAL) + + if search: + query_condition &= ( + Q(template_name__search=search) | Q(dashboard_description__search=search) | Q(tags__contains=[search]) + ) + + return DashboardTemplate.objects.filter(query_condition) diff --git a/posthog/api/dashboards/test/test_dashboard_templates.py b/posthog/api/dashboards/test/test_dashboard_templates.py index 2c44b421f1782..f07610ba90351 100644 --- a/posthog/api/dashboards/test/test_dashboard_templates.py +++ b/posthog/api/dashboards/test/test_dashboard_templates.py @@ -1,5 +1,8 @@ +from typing import Dict, Optional, List + from rest_framework import status +from posthog.models import User from posthog.models.dashboard_templates import DashboardTemplate from posthog.models.organization import Organization from posthog.models.team.team import Team @@ -444,19 +447,120 @@ def test_cant_make_templates_without_teamid_private(self) -> None: assert response.status_code == status.HTTP_200_OK assert response.json()["results"][0]["scope"] == "global" + def test_search_when_listing_templates(self): + # ensure there are no templates + DashboardTemplate.objects.all().delete() + + self.create_template({"scope": DashboardTemplate.Scope.GLOBAL, "template_name": "pony template"}) + self.create_template( + { + "scope": DashboardTemplate.Scope.GLOBAL, + "template_name": "wat template", + "dashboard_description": "description with pony", + } + ) + self.create_template( + {"scope": DashboardTemplate.Scope.GLOBAL, "template_name": "tagged wat template", "tags": ["pony", "horse"]} + ) + self.create_template( + { + "scope": DashboardTemplate.Scope.GLOBAL, + "template_name": "tagged ponies template", + "tags": ["goat", "horse"], + } + ) + not_pony_template_id = self.create_template( + {"scope": DashboardTemplate.Scope.GLOBAL, "template_name": "goat template"} + ) + + default_response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates/") + assert default_response.status_code == status.HTTP_200_OK + assert len(default_response.json()["results"]) == 5 + + # will match pony and ponies + pony_response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates/?search=pony") + assert pony_response.status_code == status.HTTP_200_OK + assert len(pony_response.json()["results"]) == 4 + assert not_pony_template_id not in [r["id"] for r in pony_response.json()["results"]] + def test_filter_template_list_by_scope(self): + # ensure there are no templates + DashboardTemplate.objects.all().delete() + + # create a flag and a global scoped template + flag_template_id = self.create_template( + {"scope": DashboardTemplate.Scope.FEATURE_FLAG, "template_name": "flag scoped template"} + ) + global_template_id = self.create_template( + {"scope": DashboardTemplate.Scope.GLOBAL, "template_name": "globally scoped template"} + ) + + default_response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates/") + assert default_response.status_code == status.HTTP_200_OK + assert [(r["id"], r["scope"]) for r in default_response.json()["results"]] == [ + (flag_template_id, "feature_flag"), + (global_template_id, "global"), + ] + + global_response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates/?scope=global") + assert global_response.status_code == status.HTTP_200_OK + assert [(r["id"], r["scope"]) for r in global_response.json()["results"]] == [(global_template_id, "global")] + + flag_response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates/?scope=feature_flag") + assert flag_response.status_code == status.HTTP_200_OK + assert [(r["id"], r["scope"]) for r in flag_response.json()["results"]] == [(flag_template_id, "feature_flag")] + + def create_template(self, overrides: Dict[str, str | List[str]], team_id: Optional[int] = None) -> str: + template = {**variable_template, **overrides} response = self.client.post( - f"/api/projects/{self.team.pk}/dashboard_templates", - variable_template, + f"/api/projects/{team_id or self.team.pk}/dashboard_templates", + template, ) + assert response.status_code == status.HTTP_201_CREATED id = response.json()["id"] - self.client.patch( - f"/api/projects/{self.team.pk}/dashboard_templates/{id}", - {"scope": "feature_flag"}, + return id + + def test_cannot_escape_team_when_filtering_template_list(self): + # create another team, and log in as a user from that team + another_team = Team.objects.create(name="Another Team", organization=self.organization) + another_team_user = User.objects.create_and_join( + organization=self.organization, first_name="Another", email="another_user@email.com", password="wat" ) + another_team_user.current_team = another_team + another_team_user.is_staff = True + another_team_user.save() - response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates/?scope=feature_flag") + self.client.force_login(another_team_user) - assert response.status_code == status.HTTP_200_OK - assert response.json()["results"][0]["scope"] == "feature_flag" + # create a dashboard template in that other team + id = self.create_template( + {"scope": DashboardTemplate.Scope.ONLY_TEAM, "template_name": "other teams template"}, + team_id=another_team.pk, + ) + + # the user from another_team can access the new dashboard via the API on their own team + list_response = self.client.get(f"/api/projects/{another_team.pk}/dashboard_templates/") + assert list_response.status_code == status.HTTP_200_OK + assert id in [r["id"] for r in list_response.json()["results"]] + + # the user from the home team cannot see the dashboard by default + self.client.force_login(self.user) + home_list_response = self.client.get(f"/api/projects/{self.team.pk}/dashboard_templates") + + assert home_list_response.status_code == status.HTTP_200_OK + assert id not in [r["id"] for r in home_list_response.json()["results"]] + + # the user form the home team cannot escape their permissions by passing filters + attempted_escape_response = self.client.get( + f"/api/projects/{self.team.pk}/dashboard_templates/?team_id={another_team.pk}" + ) + assert attempted_escape_response.status_code == status.HTTP_200_OK + assert id not in [r["id"] for r in attempted_escape_response.json()["results"]] + + # searching by text doesn't get around the team filtering + another_attempted_escape_response = self.client.get( + f"/api/projects/{self.team.pk}/dashboard_templates/?search=other" + ) + assert another_attempted_escape_response.status_code == status.HTTP_200_OK + assert id not in [r["id"] for r in another_attempted_escape_response.json()["results"]] diff --git a/posthog/api/test/__snapshots__/test_cohort.ambr b/posthog/api/test/__snapshots__/test_cohort.ambr index 0bc59147eed51..5dcaa0f2c01ad 100644 --- a/posthog/api/test/__snapshots__/test_cohort.ambr +++ b/posthog/api/test/__snapshots__/test_cohort.ambr @@ -1,7 +1,7 @@ # serializer version: 1 # name: TestCohort.test_async_deletion_of_cohort ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ SELECT count(DISTINCT person_id) FROM cohortpeople WHERE team_id = 2 @@ -11,7 +11,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.1 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ INSERT INTO cohortpeople SELECT id, 2 as cohort_id, @@ -84,7 +84,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.2 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ SELECT count(DISTINCT person_id) FROM cohortpeople WHERE team_id = 2 @@ -94,7 +94,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.3 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.clear_stale_cohort */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.clear_stale_cohort */ SELECT count() FROM cohortpeople WHERE team_id = 2 @@ -104,7 +104,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.4 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ SELECT count(DISTINCT person_id) FROM cohortpeople WHERE team_id = 2 @@ -114,7 +114,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.5 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ INSERT INTO cohortpeople SELECT id, 2 as cohort_id, @@ -148,7 +148,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.6 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.calculate_cohort_ch */ SELECT count(DISTINCT person_id) FROM cohortpeople WHERE team_id = 2 @@ -158,7 +158,7 @@ # --- # name: TestCohort.test_async_deletion_of_cohort.7 ''' - /* user_id:119 celery:posthog.tasks.calculate_cohort.clear_stale_cohort */ + /* user_id:120 celery:posthog.tasks.calculate_cohort.clear_stale_cohort */ SELECT count() FROM cohortpeople WHERE team_id = 2 diff --git a/posthog/api/test/__snapshots__/test_feature_flag.ambr b/posthog/api/test/__snapshots__/test_feature_flag.ambr index b0bcd6d3ec081..68a540a818172 100644 --- a/posthog/api/test/__snapshots__/test_feature_flag.ambr +++ b/posthog/api/test/__snapshots__/test_feature_flag.ambr @@ -1680,7 +1680,7 @@ # --- # name: TestFeatureFlag.test_creating_static_cohort.14 ''' - /* user_id:192 celery:posthog.tasks.calculate_cohort.insert_cohort_from_feature_flag */ + /* user_id:193 celery:posthog.tasks.calculate_cohort.insert_cohort_from_feature_flag */ SELECT count(DISTINCT person_id) FROM person_static_cohort WHERE team_id = 2