Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 1 addition & 1 deletion frontend/src/products.tsx

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 19 additions & 6 deletions products/data_catalog/frontend/DataCatalogMetricScene.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
IconSparkles,
IconTrash,
} from '@posthog/icons'
import { LemonBanner, LemonButton, LemonDialog, LemonDivider, LemonTag } from '@posthog/lemon-ui'
import { LemonBanner, LemonButton, LemonDialog, LemonDivider, LemonSkeleton, LemonTag } from '@posthog/lemon-ui'

import { NotFound } from 'lib/components/NotFound'
import { TZLabel } from 'lib/components/TZLabel'
Expand All @@ -21,7 +21,6 @@ import { LemonCollapse } from 'lib/lemon-ui/LemonCollapse'
import { LemonField } from 'lib/lemon-ui/LemonField'
import { LemonInput } from 'lib/lemon-ui/LemonInput'
import { LemonTabs } from 'lib/lemon-ui/LemonTabs'
import { Spinner } from 'lib/lemon-ui/Spinner'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { ButtonPrimitive } from 'lib/ui/Button/ButtonPrimitives'
import { maxGlobalLogic } from 'scenes/max/maxGlobalLogic'
Expand Down Expand Up @@ -137,7 +136,7 @@ export function DataCatalogMetricScene({ name }: DataCatalogMetricSceneLogicProp
}

if (metricLoading && !metric) {
return <Spinner className="text-2xl" />
return <MetricSceneSkeleton />
}
if (!metric) {
return <NotFound object="metric" />
Expand Down Expand Up @@ -366,7 +365,7 @@ export function DataCatalogMetricScene({ name }: DataCatalogMetricSceneLogicProp
{ key: 'definition', label: 'Definition' },
{ key: 'lineage', label: 'Lineage' },
metricChecksEnabled && {
key: 'tests',
key: 'data-quality',
label: <MetricTestsTabLabel metricId={metric.id} />,
},
]}
Expand Down Expand Up @@ -404,8 +403,8 @@ export function DataCatalogMetricScene({ name }: DataCatalogMetricSceneLogicProp
</div>
)}

{metricChecksEnabled && mountedTabs.includes('tests') && (
<div className={tabPanelClassName('tests', activeTab)}>
{metricChecksEnabled && mountedTabs.includes('data-quality') && (
<div className={tabPanelClassName('data-quality', activeTab)}>
<MetricTestsTab />
</div>
)}
Expand Down Expand Up @@ -443,6 +442,20 @@ export function DataCatalogMetricScene({ name }: DataCatalogMetricSceneLogicProp
)
}

function MetricSceneSkeleton(): JSX.Element {
return (
<SceneContent>
<LemonSkeleton className="h-8 w-80" />
<LemonSkeleton className="h-4 w-1/2" />
<LemonSkeleton className="h-9 w-72" />
<div className="grid grid-cols-2 gap-x-8 gap-y-3 max-w-2xl">
<LemonSkeleton className="h-10" repeat={4} />
</div>
<LemonSkeleton className="h-64 w-full" />
</SceneContent>
)
}

function StatusRow({ metric }: { metric: DataCatalogMetricApi }): JSX.Element {
return (
<div className="flex items-center gap-1">
Expand Down
19 changes: 10 additions & 9 deletions products/data_catalog/frontend/dataCatalogMetricSceneLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,28 +262,29 @@ describe('dataCatalogMetricSceneLogic', () => {
expect(logic.values.lineageRetried).toBe(expected === 'not_ready')
})

it('synchronizes the Tests tab with navigation and preserves it on rename', async () => {
it('redirects the legacy tests key to the data quality tab and preserves it on rename', async () => {
router.actions.push(urls.dataCatalogMetric('weekly_active_users'), { tab: 'tests' })
await expectLogic(logic).toFinishAllListeners()
expect(logic.values.activeTab).toBe('tests')
expect(logic.values.mountedTabs).toEqual(['tests'])
expect(logic.values.activeTab).toBe('data-quality')
expect(router.values.searchParams.tab).toBe('data-quality')
expect(logic.values.mountedTabs).toEqual(['data-quality'])
logic.actions.setActiveTab('definition')
expect(router.values.searchParams.tab).toBeUndefined()
expect(logic.values.mountedTabs).toEqual(['tests', 'definition'])
logic.actions.setActiveTab('tests')
expect(router.values.searchParams.tab).toBe('tests')
expect(logic.values.mountedTabs).toEqual(['data-quality', 'definition'])
logic.actions.setActiveTab('data-quality')
expect(router.values.searchParams.tab).toBe('data-quality')
;(dataCatalogMetricsPartialUpdate as jest.Mock).mockResolvedValue(buildMetric({ name: 'wau' }))
logic.actions.renameMetric('wau')
await expectLogic(logic).toFinishAllListeners()
expect(router.values.searchParams.tab).toBe('tests')
expect(router.values.searchParams.tab).toBe('data-quality')
})

// The metric check endpoints are gated on the same flag, so a link to the tab from a project
// outside the rollout has to land on Definition. Otherwise the panel mounts against endpoints
// that reject every request.
it('refuses the Tests tab while the data quality flag is off', async () => {
it('refuses the data quality tab while the data quality flag is off', async () => {
featureFlagLogic.actions.setFeatureFlags([], {})
router.actions.push(urls.dataCatalogMetric('weekly_active_users'), { tab: 'tests' })
router.actions.push(urls.dataCatalogMetric('weekly_active_users'), { tab: 'data-quality' })
await expectLogic(logic).toFinishAllListeners()
expect(logic.values.metricChecksEnabled).toBe(false)
expect(logic.values.activeTab).toBe('definition')
Expand Down
13 changes: 8 additions & 5 deletions products/data_catalog/frontend/dataCatalogMetricSceneLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export interface DataCatalogMetricSceneLogicProps {
name: string
}

export type MetricSceneTab = 'definition' | 'tests' | 'lineage'
export type MetricSceneTab = 'definition' | 'data-quality' | 'lineage'

/** What stopped the lineage load, in the terms the tab has a screen for. */
export type LineageLoadProblem = 'not_ready' | 'no_warehouse_access' | 'failed'
Expand Down Expand Up @@ -453,7 +453,7 @@ export const dataCatalogMetricSceneLogic = kea<dataCatalogMetricSceneLogicType>(
visitedTabs.includes(activeTab) ? visitedTabs : [...visitedTabs, activeTab],
],
// The metric check endpoints are gated on the same flag, so a project outside the rollout
// must not reach the Tests tab: every request it makes is rejected.
// must not reach the data quality tab: every request it makes is rejected.
metricChecksEnabled: [
(s) => [s.featureFlags],
(featureFlags: FeatureFlagsSet): boolean => !!featureFlags[FEATURE_FLAGS.DATA_QUALITY_CHECKS],
Expand Down Expand Up @@ -677,10 +677,13 @@ export const dataCatalogMetricSceneLogic = kea<dataCatalogMetricSceneLogicType>(
if (name !== props.name) {
return
}
// `tests` is the old key for the data quality tab; links in sent notifications still use it.
const tabParam = searchParams.tab === 'tests' ? 'data-quality' : searchParams.tab
const requestedTab: MetricSceneTab =
searchParams.tab === 'tests' || searchParams.tab === 'lineage' ? searchParams.tab : 'definition'
const activeTab = requestedTab === 'tests' && !values.metricChecksEnabled ? 'definition' : requestedTab
if (activeTab !== requestedTab) {
tabParam === 'data-quality' || tabParam === 'lineage' ? tabParam : 'definition'
const activeTab =
requestedTab === 'data-quality' && !values.metricChecksEnabled ? 'definition' : requestedTab
if (activeTab !== searchParams.tab && searchParams.tab !== undefined) {
// Drop the param so a refresh or a back navigation does not ask for the tab again.
router.actions.replace(urls.dataCatalogMetric(props.name, activeTab))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export function MetricTestsTabLabel({ metricId }: { metricId: string }): JSX.Ele

return (
<span className="flex items-center gap-1">
Tests{failing > 0 && <LemonTag type="danger">{failing} failing</LemonTag>}
Data quality{failing > 0 && <LemonTag type="danger">{failing} failing</LemonTag>}
</span>
)
}
2 changes: 1 addition & 1 deletion products/data_catalog/manifest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const manifest: ProductManifest = {
redirects: {},
urls: {
dataCatalog: (tab?: string): string => `/data-catalog${tab ? `?tab=${tab}` : ''}`,
dataCatalogMetric: (name: string, tab?: 'definition' | 'tests' | 'lineage'): string =>
dataCatalogMetric: (name: string, tab?: 'definition' | 'data-quality' | 'lineage'): string =>
`/data-catalog/metrics/${name}${tab && tab !== 'definition' ? `?tab=${tab}` : ''}`,
},
fileSystemTypes: {},
Expand Down
2 changes: 1 addition & 1 deletion products/data_modeling/frontend/ModelsOverviewTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ const CHECK_COLUMNS: LemonTableColumns<DataQualityOverviewCheckApi> = [
key: 'subject',
render: (_, check) =>
check.subject_node_id ? (
<Link to={urls.nodeDetail(check.subject_node_id, 'tests')}>{check.subject_name}</Link>
<Link to={urls.nodeDetail(check.subject_node_id, 'data-quality')}>{check.subject_name}</Link>
) : (
check.subject_name
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ const TAB_LABELS: Record<NodeDetailSceneTab, string> = {
query: 'Query',
lineage: 'Lineage',
materialization: 'Materialization',
tests: 'Data quality',
'data-quality': 'Data quality',
history: 'History',
}

function tabLabel(
tab: NodeDetailSceneTab,
dataQualitySubject: NodeDetailDataQualitySubject | null
): JSX.Element | string {
if (tab === 'tests' && dataQualitySubject) {
if (tab === 'data-quality' && dataQualitySubject) {
return <NodeDetailTestsTabLabel {...dataQualitySubject} />
}
return TAB_LABELS[tab]
Expand Down Expand Up @@ -104,7 +104,7 @@ export function NodeDetailScene({ id }: NodeDetailSceneLogicProps): JSX.Element
id={savedQueryId ?? ''}
/>
)
case 'tests':
case 'data-quality':
return <NodeDetailTests id={id} />
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ describe('nodeDetailSceneLogic', () => {

await mountScene(urls.nodeDetail(NODE_ID))

expect(logic.values.availableTabs).toEqual(['lineage', 'tests'])
expect(logic.values.availableTabs).toEqual(['lineage', 'data-quality'])
expect(logic.values.dataQualitySubject).toEqual({ subjectType: 'table', subjectId: 'table-1' })
expect(logic.values.tableDetails).toMatchObject({
table: { id: 'table-1' },
Expand Down Expand Up @@ -184,7 +184,7 @@ describe('nodeDetailSceneLogic', () => {

await mountScene(urls.nodeDetail(NODE_ID))

expect(logic.values.availableTabs).toEqual(['lineage', 'tests'])
expect(logic.values.availableTabs).toEqual(['lineage', 'data-quality'])
expect(logic.values.dataQualitySubject).toEqual({
subjectType: 'posthog_table',
subjectId: `${tableName}-subject`,
Expand Down Expand Up @@ -232,7 +232,7 @@ describe('nodeDetailSceneLogic', () => {
})
}).toFinishAllListeners()

expect(logic.values.availableTabs).toEqual(['lineage', 'tests'])
expect(logic.values.availableTabs).toEqual(['lineage', 'data-quality'])
expect(logic.values.dataQualitySubject).toMatchObject({
subjectType: 'posthog_table',
subjectId: 'events-subject',
Expand All @@ -249,9 +249,10 @@ describe('nodeDetailSceneLogic', () => {
},
})

await mountScene(urls.nodeDetail(NODE_ID, 'tests'))
await mountScene(`/models/${NODE_ID}/tests`)

expect(logic.values.effectiveTab).toEqual('tests')
expect(logic.values.effectiveTab).toEqual('data-quality')
expect(router.values.location.pathname).toEqual(`/project/997${urls.nodeDetail(NODE_ID, 'data-quality')}`)
expect(logic.values.dataQualitySubject).toBeNull()

render(createElement(NodeDetailTests, { id: NODE_ID }))
Expand All @@ -271,7 +272,7 @@ describe('nodeDetailSceneLogic', () => {

await mountScene(urls.nodeDetail(NODE_ID))

expect(logic.values.availableTabs).toEqual(['lineage', 'tests'])
expect(logic.values.availableTabs).toEqual(['lineage', 'data-quality'])
expect(logic.values.lineageGraph).toEqual({ currentNodeId: NODE_ID, nodes: [], edges: [] })
expect(logic.values.postHogSubjectError).toBe(true)

Expand Down Expand Up @@ -346,7 +347,7 @@ describe('nodeDetailSceneLogic', () => {
it('lists every tab a saved query supports when data quality checks are on', async () => {
await mountScene(urls.nodeDetail(NODE_ID))

expect(logic.values.availableTabs).toEqual(['query', 'lineage', 'materialization', 'tests', 'history'])
expect(logic.values.availableTabs).toEqual(['query', 'lineage', 'materialization', 'data-quality', 'history'])
})

// The tab is gated on the node's saved_query_id, not the loaded saved query, so a load failure
Expand Down Expand Up @@ -391,7 +392,7 @@ describe('nodeDetailSceneLogic', () => {
it('falls back to the default tab when the URL names a tab this model does not have', async () => {
flagsLogic.actions.setFeatureFlags([], {})

await mountScene(urls.nodeDetail(NODE_ID, 'tests'))
await mountScene(urls.nodeDetail(NODE_ID, 'data-quality'))

expect(logic.values.effectiveTab).toEqual('query')
expect(logic.values.currentTab).toEqual('query')
Expand Down
19 changes: 12 additions & 7 deletions products/data_modeling/frontend/nodeDetail/nodeDetailSceneLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import type { DataQualitySubjectApi } from 'products/data_quality/frontend/gener
import { MATERIALIZING_TYPES } from '../freshness'
import type { NodeTypeEnumApi } from '../generated/api.schemas'

export const NODE_DETAIL_SCENE_TABS = ['query', 'lineage', 'materialization', 'tests', 'history'] as const
export const NODE_DETAIL_SCENE_TABS = ['query', 'lineage', 'materialization', 'data-quality', 'history'] as const
export type NodeDetailSceneTab = (typeof NODE_DETAIL_SCENE_TABS)[number]

export interface NodeDetailSceneLogicProps {
Expand Down Expand Up @@ -220,7 +220,7 @@ export interface nodeDetailSceneLogicActions {
value: true
}
setCurrentTab: (tab: NodeDetailSceneTab | null) => {
tab: 'history' | 'lineage' | 'materialization' | 'query' | 'tests' | null
tab: 'data-quality' | 'history' | 'lineage' | 'materialization' | 'query' | null
}
updateNodeDescription: (description: string) => {
description: string
Expand Down Expand Up @@ -264,9 +264,9 @@ export interface nodeDetailSceneLogicMeta {
isMaterialized: (node: DataModelingNode | null, savedQuery: DataWarehouseSavedQuery | null) => boolean
defaultTab: (node: DataModelingNode | null, isMaterialized: boolean) => NodeDetailSceneTab
effectiveTab: (
currentTab: 'history' | 'lineage' | 'materialization' | 'query' | 'tests' | null,
availableTabs: ('history' | 'lineage' | 'materialization' | 'query' | 'tests')[],
defaultTab: 'history' | 'lineage' | 'materialization' | 'query' | 'tests'
currentTab: 'data-quality' | 'history' | 'lineage' | 'materialization' | 'query' | null,
availableTabs: ('data-quality' | 'history' | 'lineage' | 'materialization' | 'query')[],
defaultTab: 'data-quality' | 'history' | 'lineage' | 'materialization' | 'query'
) => NodeDetailSceneTab | null
effectiveLastRunAt: (node: DataModelingNode | null, savedQuery: DataWarehouseSavedQuery | null) => string | null
effectiveLastRunStatus: (
Expand Down Expand Up @@ -489,7 +489,7 @@ export const nodeDetailSceneLogic = kea<nodeDetailSceneLogicType>([
'lineage',
...(featureFlags[FEATURE_FLAGS.DATA_QUALITY_CHECKS] &&
(node.warehouse_table_id || node.origin === 'posthog')
? ['tests' as const]
? ['data-quality' as const]
: []),
]
}
Expand All @@ -506,7 +506,7 @@ export const nodeDetailSceneLogic = kea<nodeDetailSceneLogicType>([
tabs.push('materialization')
}
if (featureFlags[FEATURE_FLAGS.DATA_QUALITY_CHECKS] && node.saved_query_id) {
tabs.push('tests')
tabs.push('data-quality')
}
if (node.saved_query_id) {
tabs.push('history')
Expand Down Expand Up @@ -662,6 +662,11 @@ export const nodeDetailSceneLogic = kea<nodeDetailSceneLogicType>([
if (id !== props.id) {
return
}
// `tests` is the tab's old URL segment; keep links built before the rename working.
if (tab === 'tests') {
router.actions.replace(urls.nodeDetail(id, 'data-quality'))
return
}
actions.setCurrentTab(isNodeDetailSceneTab(tab) ? tab : null)
actions.canonicalizeTab()
}
Expand Down
2 changes: 1 addition & 1 deletion products/data_quality/backend/logic/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def _gate_of(self, user: User) -> ReferenceGate:

def _source_url(team_id: int, subject_type: str, subject_name: str) -> str:
if subject_type == SubjectType.METRIC:
return f"/project/{team_id}/data-catalog/metrics/{quote(subject_name, safe='')}?tab=tests"
return f"/project/{team_id}/data-catalog/metrics/{quote(subject_name, safe='')}?tab=data-quality"
if subject_type == SubjectType.POSTHOG_TABLE:
return f"/project/{team_id}/models?tab=data-quality"
return ""
Expand Down
2 changes: 1 addition & 1 deletion products/data_quality/backend/tests/test_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def change_metric(*args: object, **kwargs: object) -> _Response:
notification = notifications.call_args.args[0]
assert notification.resource_type == "data_catalog"
assert notification.resource_id == str(metric.id)
assert notification.source_url == f"/project/{self.team.id}/data-catalog/metrics/signups?tab=tests"
assert notification.source_url == f"/project/{self.team.id}/data-catalog/metrics/signups?tab=data-quality"
assert notification.title == "Data quality check failed on signups"
with CaptureQueriesContext(connection) as queries:
recipients = notification.resolver.resolve(TargetType.TEAM, str(self.team.id), self.team.id)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ describe('DataQualityOverview', () => {
expect(within(disclosure.parentElement!).getByText('Failing')).toBeTruthy()
// Suffix match: the rendered href carries the /project/:id prefix, so an exact match on the
// path would find nothing and the assertion below would pass on a null link.
const link = document.querySelector('a[href$="/models/node-1/tests"]')
const link = document.querySelector('a[href$="/models/node-1/data-quality"]')
expect(link).not.toBeNull()
expect(disclosure.contains(link)).toBe(false)
expect(disclosure.contains(runSubjectButtons()[0])).toBe(false)
Expand Down
Loading