From f724c9b6368f27da74a2c3d7cf1f7ff9b05b0c3d Mon Sep 17 00:00:00 2001 From: Sriram Balakrishnan Date: Wed, 19 Feb 2025 15:24:09 -0500 Subject: [PATCH] Refactored showMeanRetention to meanRetentionCalculation --- .../queries/nodes/InsightQuery/defaults.ts | 2 +- .../utils/filtersToQueryNode.test.ts | 4 +-- .../InsightQuery/utils/filtersToQueryNode.ts | 4 +-- frontend/src/queries/schema.json | 26 ++++++++--------- .../schema/schema-assistant-queries.ts | 2 +- frontend/src/queries/schema/schema-general.ts | 2 +- .../filters/RetentionMeanDropdown.tsx | 8 +++--- frontend/src/scenes/insights/utils.tsx | 4 +-- .../src/scenes/insights/utils/cleanFilters.ts | 4 +-- .../src/scenes/insights/utils/queryUtils.ts | 2 +- .../src/scenes/retention/RetentionTable.tsx | 8 +++--- frontend/src/types.ts | 2 +- .../legacy_compatibility/filter_to_query.py | 2 +- .../test/test_filter_to_query.py | 4 +-- .../commands/migrate_retention_show_mean.py | 6 ++-- posthog/schema.py | 28 +++++++++---------- posthog/schema_helpers.py | 2 +- 17 files changed, 55 insertions(+), 55 deletions(-) diff --git a/frontend/src/queries/nodes/InsightQuery/defaults.ts b/frontend/src/queries/nodes/InsightQuery/defaults.ts index 565d57f7b69922..631b659d5b81a2 100644 --- a/frontend/src/queries/nodes/InsightQuery/defaults.ts +++ b/frontend/src/queries/nodes/InsightQuery/defaults.ts @@ -54,7 +54,7 @@ const retentionQueryDefault: RetentionQuery = { type: 'events', }, retentionType: 'retention_first_time', - showMeanRetention: 'simple', + meanRetentionCalculation: 'simple', }, } diff --git a/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.test.ts b/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.test.ts index ca3aadf5c48ec6..e9566e4e3cb715 100644 --- a/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.test.ts +++ b/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.test.ts @@ -669,7 +669,7 @@ describe('filtersToQueryNode', () => { returning_entity: { id: '1' }, target_entity: { id: '1' }, period: RetentionPeriod.Day, - show_mean_retention: 'simple', + mean_retention_calculation: 'simple', } const result = filtersToQueryNode(filters) @@ -683,7 +683,7 @@ describe('filtersToQueryNode', () => { returningEntity: { id: '1' }, targetEntity: { id: '1' }, period: RetentionPeriod.Day, - showMeanRetention: 'simple', + meanRetentionCalculation: 'simple', }, } expect(result).toEqual(query) diff --git a/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts b/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts index a1020e494a1421..af9b3120a1a148 100644 --- a/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts +++ b/frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts @@ -439,8 +439,8 @@ export const retentionFilterToQuery = (filters: Partial): R returningEntity: sanitizeRetentionEntity(filters.returning_entity), targetEntity: sanitizeRetentionEntity(filters.target_entity), period: filters.period, - showMeanRetention: - filters.show_mean_retention || + meanRetentionCalculation: + filters.mean_retention_calculation || (typeof filters.show_mean === 'boolean' ? (filters.show_mean ? 'simple' : 'none') : 'simple'), cumulative: filters.cumulative, }) diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index ce304cfc6b0704..c9ec5129ae03a4 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -1244,6 +1244,11 @@ "description": "Whether retention should be rolling (aka unbounded, cumulative). Rolling retention means that a user coming back in period 5 makes them count towards all the previous periods.", "type": "boolean" }, + "meanRetentionCalculation": { + "description": "Whether an additional series should be shown, showing the mean conversion for each period across cohorts.", + "enum": ["simple", "weighted", "none"], + "type": "string" + }, "period": { "$ref": "#/definitions/RetentionPeriod", "default": "Day", @@ -1266,11 +1271,6 @@ "description": "DEPRECATED: Whether an additional series should be shown, showing the mean conversion for each period across cohorts.", "type": "boolean" }, - "showMeanRetention": { - "description": "Whether an additional series should be shown, showing the mean conversion for each period across cohorts.", - "enum": ["simple", "weighted", "none"], - "type": "string" - }, "targetEntity": { "$ref": "#/definitions/RetentionEntity", "description": "Activation event (event putting the actor into the initial cohort)." @@ -13240,6 +13240,10 @@ "cumulative": { "type": "boolean" }, + "meanRetentionCalculation": { + "enum": ["simple", "weighted", "none"], + "type": "string" + }, "period": { "$ref": "#/definitions/RetentionPeriod", "default": "Day" @@ -13258,10 +13262,6 @@ "showMean": { "type": "boolean" }, - "showMeanRetention": { - "enum": ["simple", "weighted", "none"], - "type": "string" - }, "targetEntity": { "$ref": "#/definitions/RetentionEntity" }, @@ -13279,6 +13279,10 @@ "cumulative": { "type": "boolean" }, + "mean_retention_calculation": { + "enum": ["simple", "weighted", "none"], + "type": "string" + }, "period": { "$ref": "#/definitions/RetentionPeriod" }, @@ -13296,10 +13300,6 @@ "show_mean": { "type": "boolean" }, - "show_mean_retention": { - "enum": ["simple", "weighted", "none"], - "type": "string" - }, "target_entity": { "$ref": "#/definitions/RetentionEntity" }, diff --git a/frontend/src/queries/schema/schema-assistant-queries.ts b/frontend/src/queries/schema/schema-assistant-queries.ts index 2a5e05326b086d..0b04ae627afd92 100644 --- a/frontend/src/queries/schema/schema-assistant-queries.ts +++ b/frontend/src/queries/schema/schema-assistant-queries.ts @@ -433,7 +433,7 @@ export interface AssistantRetentionFilter { /** DEPRECATED: Whether an additional series should be shown, showing the mean conversion for each period across cohorts. */ showMean?: RetentionFilterLegacy['show_mean'] /** Whether an additional series should be shown, showing the mean conversion for each period across cohorts. */ - showMeanRetention?: RetentionFilterLegacy['show_mean_retention'] + meanRetentionCalculation?: RetentionFilterLegacy['mean_retention_calculation'] /** * Whether retention should be rolling (aka unbounded, cumulative). * Rolling retention means that a user coming back in period 5 makes them count towards all the previous periods. diff --git a/frontend/src/queries/schema/schema-general.ts b/frontend/src/queries/schema/schema-general.ts index a78239c7c69efc..00ce57e5127901 100644 --- a/frontend/src/queries/schema/schema-general.ts +++ b/frontend/src/queries/schema/schema-general.ts @@ -1095,7 +1095,7 @@ export type RetentionFilter = { /** @default Day */ period?: RetentionFilterLegacy['period'] showMean?: RetentionFilterLegacy['show_mean'] - showMeanRetention?: RetentionFilterLegacy['show_mean_retention'] + meanRetentionCalculation?: RetentionFilterLegacy['mean_retention_calculation'] cumulative?: RetentionFilterLegacy['cumulative'] } diff --git a/frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx b/frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx index 66a6aee707fa7a..eed57928648b1e 100644 --- a/frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx +++ b/frontend/src/scenes/insights/filters/RetentionMeanDropdown.tsx @@ -11,7 +11,7 @@ export function RetentionMeanDropdown(): JSX.Element | null { const { retentionFilter } = useValues(insightVizDataLogic(insightProps)) const { updateInsightFilter } = useActions(insightVizDataLogic(insightProps)) - const showMeanRetention = retentionFilter?.showMeanRetention ?? RETENTION_MEAN_NONE + const meanRetentionCalculation = retentionFilter?.meanRetentionCalculation ?? RETENTION_MEAN_NONE if (!canEditInsight) { return null @@ -21,9 +21,9 @@ export function RetentionMeanDropdown(): JSX.Element | null { { - updateInsightFilter({ showMeanRetention }) + value={meanRetentionCalculation} + onChange={(meanRetentionCalculation) => { + updateInsightFilter({ meanRetentionCalculation }) }} options={[ { diff --git a/frontend/src/scenes/insights/utils.tsx b/frontend/src/scenes/insights/utils.tsx index 577626645e6de3..705138d26fbe63 100644 --- a/frontend/src/scenes/insights/utils.tsx +++ b/frontend/src/scenes/insights/utils.tsx @@ -574,11 +574,11 @@ function parseAndMigrateQuery(query: string): T | null { try { const parsedQuery = JSON.parse(query) // We made a database migration to support weighted and simple mean in retention tables. - // To do this we created a new column showMeanRetention and deprecated showMean. + // To do this we created a new column meanRetentionCalculation and deprecated showMean. // This ensures older URLs are parsed correctly. const retentionFilter = parsedQuery?.source?.retentionFilter if (retentionFilter && 'showMean' in retentionFilter && typeof retentionFilter.showMean === 'boolean') { - retentionFilter.showMeanRetention = retentionFilter.showMean ? 'simple' : 'none' + retentionFilter.meanRetentionCalculation = retentionFilter.showMean ? 'simple' : 'none' delete retentionFilter.showMean } return parsedQuery diff --git a/frontend/src/scenes/insights/utils/cleanFilters.ts b/frontend/src/scenes/insights/utils/cleanFilters.ts index 4640eb71c10191..83c34640ca2d21 100644 --- a/frontend/src/scenes/insights/utils/cleanFilters.ts +++ b/frontend/src/scenes/insights/utils/cleanFilters.ts @@ -309,8 +309,8 @@ export function cleanFilters( breakdown_type: filters.breakdown_type, retention_reference: filters.retention_reference, show_mean: filters.show_mean, - ...(filters.show_mean_retention && filters.show_mean_retention !== RETENTION_MEAN_NONE - ? { show_mean_retention: filters.show_mean_retention } + ...(filters.mean_retention_calculation && filters.mean_retention_calculation !== RETENTION_MEAN_NONE + ? { mean_retention_calculation: filters.mean_retention_calculation } : {}), cumulative: filters.cumulative, total_intervals: Math.min(Math.max(filters.total_intervals ?? 11, 0), 100), diff --git a/frontend/src/scenes/insights/utils/queryUtils.ts b/frontend/src/scenes/insights/utils/queryUtils.ts index a9d0da20089ab5..73bd1412ddd767 100644 --- a/frontend/src/scenes/insights/utils/queryUtils.ts +++ b/frontend/src/scenes/insights/utils/queryUtils.ts @@ -142,7 +142,7 @@ const cleanInsightQuery = (query: InsightQueryNode, opts?: CompareQueryOpts): In toggledLifecycles: undefined, showLabelsOnSeries: undefined, showMean: undefined, - showMeanRetention: undefined, + meanRetentionCalculation: undefined, cumulative: undefined, yAxisScaleType: undefined, hiddenLegendIndexes: undefined, diff --git a/frontend/src/scenes/retention/RetentionTable.tsx b/frontend/src/scenes/retention/RetentionTable.tsx index f1f10218801049..9779c86cb75350 100644 --- a/frontend/src/scenes/retention/RetentionTable.tsx +++ b/frontend/src/scenes/retention/RetentionTable.tsx @@ -18,7 +18,7 @@ export function RetentionTable({ inSharedMode = false }: { inSharedMode?: boolea const { openModal } = useActions(retentionModalLogic(insightProps)) const backgroundColor = theme?.['preset-1'] || '#000000' // Default to black if no color found const backgroundColorMean = theme?.['preset-2'] || '#000000' // Default to black if no color found - const showMeanRetention = retentionFilter?.showMeanRetention ?? RETENTION_MEAN_NONE + const meanRetentionCalculation = retentionFilter?.meanRetentionCalculation ?? RETENTION_MEAN_NONE return ( - {showMeanRetention === 'weighted' && tableRows.length > 0 ? ( + {meanRetentionCalculation === 'weighted' && tableRows.length > 0 ? ( {range(0, tableRows[0].length).map((columnIndex) => ( ) : undefined} - {showMeanRetention === 'simple' && tableRows.length > 0 ? ( + {meanRetentionCalculation === 'simple' && tableRows.length > 0 ? ( {range(0, tableRows[0].length).map((columnIndex) => (
@@ -82,7 +82,7 @@ export function RetentionTable({ inSharedMode = false }: { inSharedMode?: boolea
@@ -130,7 +130,7 @@ export function RetentionTable({ inSharedMode = false }: { inSharedMode?: boolea {row.map((column, columnIndex) => ( {columnIndex <= (hideSizeColumn ? 0 : 1) ? ( {column} diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 4a4f5502a8ff79..4f6d1744f6f47e 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -2450,7 +2450,7 @@ export interface RetentionFilterType extends FilterType { //frontend only show_mean?: boolean // deprecated - show_mean_retention?: 'simple' | 'weighted' | typeof RETENTION_MEAN_NONE + mean_retention_calculation?: 'simple' | 'weighted' | typeof RETENTION_MEAN_NONE } export interface LifecycleFilterType extends FilterType { /** @deprecated */ diff --git a/posthog/hogql_queries/legacy_compatibility/filter_to_query.py b/posthog/hogql_queries/legacy_compatibility/filter_to_query.py index 59acf3fe35a10e..b019b2753e1a57 100644 --- a/posthog/hogql_queries/legacy_compatibility/filter_to_query.py +++ b/posthog/hogql_queries/legacy_compatibility/filter_to_query.py @@ -539,7 +539,7 @@ def _insight_filter(filter: dict, allow_variables: bool = False): ), period=filter.get("period"), showMean=filter.get("show_mean"), - showMeanRetention=filter.get("show_mean_retention"), + meanRetentionCalculation=filter.get("mean_retention_calculation"), cumulative=filter.get("cumulative"), ) } diff --git a/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py b/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py index 1d4bdab5bc07d4..5dc876ea7ddf22 100644 --- a/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py +++ b/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py @@ -1505,7 +1505,7 @@ def test_retention_filter(self): "target_entity": {"id": "$pageview", "name": "$pageview", "type": "events"}, "period": "Week", "show_mean": True, - "show_mean_retention": "simple", + "mean_retention_calculation": "simple", "cumulative": True, } @@ -1533,7 +1533,7 @@ def test_retention_filter(self): "order": None, }, showMean=True, - showMeanRetention="simple", + meanRetentionCalculation="simple", cumulative=True, ), ) diff --git a/posthog/management/commands/migrate_retention_show_mean.py b/posthog/management/commands/migrate_retention_show_mean.py index e1b2edc9d0344c..e9bb1c9551b88a 100644 --- a/posthog/management/commands/migrate_retention_show_mean.py +++ b/posthog/management/commands/migrate_retention_show_mean.py @@ -6,7 +6,7 @@ def migrate_show_mean_from_boolean_to_string(batch_size: int, live_run: bool = False) -> None: """ - Migrate the showMean boolean field to showMeanRetention string field in retention insights. + Migrate the showMean boolean field to meanRetentionCalculation string field in retention insights. """ retention_insights = Insight.objects.filter( deleted=False, @@ -25,7 +25,7 @@ def migrate_show_mean_from_boolean_to_string(batch_size: int, live_run: bool = F if live_run: with transaction.atomic(): # Convert boolean to string - if True, use 'simple' else 'none' - insight.query["source"]["retentionFilter"]["showMeanRetention"] = ( + insight.query["source"]["retentionFilter"]["meanRetentionCalculation"] = ( "simple" if show_mean_value else "none" ) insight.save() @@ -42,7 +42,7 @@ def migrate_show_mean_from_boolean_to_string(batch_size: int, live_run: bool = F class Command(BaseCommand): - help = "Migrate retention insights showMean boolean field to showMeanRetention string field" + help = "Migrate retention insights showMean boolean field to meanRetentionCalculation string field" def add_arguments(self, parser): parser.add_argument( diff --git a/posthog/schema.py b/posthog/schema.py index df5f777425d3f4..1e05800aa0d77a 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -144,17 +144,17 @@ class AssistantMessageType(StrEnum): AI_FAILURE = "ai/failure" -class RetentionReference(StrEnum): - TOTAL = "total" - PREVIOUS = "previous" - - -class ShowMeanRetention(StrEnum): +class MeanRetentionCalculation(StrEnum): SIMPLE = "simple" WEIGHTED = "weighted" NONE = "none" +class RetentionReference(StrEnum): + TOTAL = "total" + PREVIOUS = "previous" + + class AssistantSetPropertyFilterOperator(StrEnum): IS_SET = "is_set" IS_NOT_SET = "is_not_set" @@ -5836,6 +5836,7 @@ class RetentionFilter(BaseModel): extra="forbid", ) cumulative: Optional[bool] = None + meanRetentionCalculation: Optional[MeanRetentionCalculation] = None period: Optional[RetentionPeriod] = RetentionPeriod.DAY retentionReference: Optional[RetentionReference] = Field( default=None, @@ -5844,7 +5845,6 @@ class RetentionFilter(BaseModel): retentionType: Optional[RetentionType] = None returningEntity: Optional[RetentionEntity] = None showMean: Optional[bool] = None - showMeanRetention: Optional[ShowMeanRetention] = None targetEntity: Optional[RetentionEntity] = None totalIntervals: Optional[int] = 11 @@ -5854,6 +5854,7 @@ class RetentionFilterLegacy(BaseModel): extra="forbid", ) cumulative: Optional[bool] = None + mean_retention_calculation: Optional[MeanRetentionCalculation] = None period: Optional[RetentionPeriod] = None retention_reference: Optional[RetentionReference] = Field( default=None, @@ -5862,7 +5863,6 @@ class RetentionFilterLegacy(BaseModel): retention_type: Optional[RetentionType] = None returning_entity: Optional[RetentionEntity] = None show_mean: Optional[bool] = None - show_mean_retention: Optional[ShowMeanRetention] = None target_entity: Optional[RetentionEntity] = None total_intervals: Optional[int] = None @@ -6224,6 +6224,12 @@ class AssistantRetentionFilter(BaseModel): " coming back in period 5 makes them count towards all the previous periods." ), ) + meanRetentionCalculation: Optional[MeanRetentionCalculation] = Field( + default=None, + description=( + "Whether an additional series should be shown, showing the mean conversion for each period across cohorts." + ), + ) period: Optional[RetentionPeriod] = Field( default=RetentionPeriod.DAY, description="Retention period, the interval to track cohorts by." ) @@ -6250,12 +6256,6 @@ class AssistantRetentionFilter(BaseModel): " across cohorts." ), ) - showMeanRetention: Optional[ShowMeanRetention] = Field( - default=None, - description=( - "Whether an additional series should be shown, showing the mean conversion for each period across cohorts." - ), - ) targetEntity: Optional[RetentionEntity] = Field( default=None, description="Activation event (event putting the actor into the initial cohort)." ) diff --git a/posthog/schema_helpers.py b/posthog/schema_helpers.py index b2cc785b9d4e40..d5f06228947242 100644 --- a/posthog/schema_helpers.py +++ b/posthog/schema_helpers.py @@ -72,7 +72,7 @@ def serialize_query(self, next_serializer): "showLabelsOnSeries", "showMean", "cumulative", - "showMeanRetention", + "meanRetentionCalculation", "yAxisScaleType", "hiddenLegendIndexes", "hiddenLegendBreakdowns",