From caff879a713256a4546a515475d0e1c9744487ef Mon Sep 17 00:00:00 2001 From: Dylan Martin Date: Thu, 17 Oct 2024 18:38:19 +0200 Subject: [PATCH 1/2] fix(flags): update payloads object when removing variants (#25580) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- .../src/scenes/feature-flags/FeatureFlag.tsx | 2 +- .../scenes/feature-flags/featureFlagLogic.ts | 35 ++++++++++++++----- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/frontend/src/scenes/feature-flags/FeatureFlag.tsx b/frontend/src/scenes/feature-flags/FeatureFlag.tsx index 4e219519fd837..21bd124d956c9 100644 --- a/frontend/src/scenes/feature-flags/FeatureFlag.tsx +++ b/frontend/src/scenes/feature-flags/FeatureFlag.tsx @@ -638,7 +638,7 @@ function UsageTab({ featureFlag }: { id: string; featureFlag: FeatureFlagType }) ) { enrichUsageDashboard() } - }, [dashboard]) + }, [dashboard, hasEnrichedAnalytics, enrichUsageDashboard]) const propertyFilter: AnyPropertyFilter[] = [ { diff --git a/frontend/src/scenes/feature-flags/featureFlagLogic.ts b/frontend/src/scenes/feature-flags/featureFlagLogic.ts index db2811ef1f44b..65026ccfd3453 100644 --- a/frontend/src/scenes/feature-flags/featureFlagLogic.ts +++ b/frontend/src/scenes/feature-flags/featureFlagLogic.ts @@ -38,6 +38,7 @@ import { FilterType, InsightModel, InsightType, + JsonType, MultivariateFlagOptions, MultivariateFlagVariant, NewEarlyAccessFeatureType, @@ -133,9 +134,11 @@ export const variantKeyToIndexFeatureFlagPayloads = (flag: FeatureFlagType): Fea return flag } - const newPayloads = {} + const newPayloads: Record = {} flag.filters.multivariate?.variants.forEach((variant, index) => { - newPayloads[index] = flag.filters.payloads?.[variant.key] + if (flag.filters.payloads?.[variant.key] !== undefined) { + newPayloads[index] = flag.filters.payloads[variant.key] + } }) return { ...flag, @@ -148,11 +151,10 @@ export const variantKeyToIndexFeatureFlagPayloads = (flag: FeatureFlagType): Fea const indexToVariantKeyFeatureFlagPayloads = (flag: Partial): Partial => { if (flag.filters?.multivariate) { - const newPayloads = {} - flag.filters?.multivariate?.variants.forEach(({ key }, index) => { - const payload = flag.filters?.payloads?.[index] - if (payload) { - newPayloads[key] = payload + const newPayloads: Record = {} + flag.filters.multivariate.variants.forEach(({ key }, index) => { + if (flag.filters?.payloads?.[index] !== undefined) { + newPayloads[key] = flag.filters.payloads[index] } }) return { @@ -319,6 +321,22 @@ export const featureFlagLogic = kea([ } const variants = [...(state.filters.multivariate?.variants || [])] variants.splice(index, 1) + + const currentPayloads = { ...state.filters.payloads } + const newPayloads: Record = {} + + // TRICKY: In addition to modifying the variant array, we also need to shift the payload indices + // because the variant array is being modified and we need to make sure that the payloads object + // stays in sync with the variant array. + Object.keys(currentPayloads).forEach((key) => { + const payloadIndex = parseInt(key) + if (payloadIndex > index) { + newPayloads[payloadIndex - 1] = currentPayloads[payloadIndex] + } else if (payloadIndex < index) { + newPayloads[payloadIndex] = currentPayloads[payloadIndex] + } + }) + return { ...state, filters: { @@ -327,6 +345,7 @@ export const featureFlagLogic = kea([ ...state.filters.multivariate, variants, }, + payloads: newPayloads, }, } }, @@ -642,7 +661,7 @@ export const featureFlagLogic = kea([ createScheduledChange: async () => { const { scheduledChangeOperation, scheduleDateMarker, currentTeamId, schedulePayload } = values - const fields = { + const fields: Record = { [ScheduledChangeOperationType.UpdateStatus]: 'active', [ScheduledChangeOperationType.AddReleaseCondition]: 'filters', } From 683d5522fd609fe8c36ede5ccf92784d718990df Mon Sep 17 00:00:00 2001 From: Dylan Martin Date: Thu, 17 Oct 2024 19:17:39 +0200 Subject: [PATCH 2/2] fix(flags): flag UI didn't support value filtering for group property filters on the backend (#25548) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- ee/clickhouse/views/groups.py | 54 ++++++++---------- .../views/test/test_clickhouse_groups.py | 56 ++++++++++++++++++- 2 files changed, 79 insertions(+), 31 deletions(-) diff --git a/ee/clickhouse/views/groups.py b/ee/clickhouse/views/groups.py index bfbb375e70990..4970a770854a2 100644 --- a/ee/clickhouse/views/groups.py +++ b/ee/clickhouse/views/groups.py @@ -177,38 +177,32 @@ def property_definitions(self, request: request.Request, **kw): return response.Response(group_type_index_to_properties) - @extend_schema( - parameters=[ - OpenApiParameter( - "group_type_index", - OpenApiTypes.INT, - description="Specify the group type to find property values of", - required=True, - ), - OpenApiParameter( - "key", - OpenApiTypes.STR, - description="Specify the property key to find values for", - required=True, - ), - ] - ) @action(methods=["GET"], detail=False) def property_values(self, request: request.Request, **kw): - rows = sync_execute( - f""" - SELECT {trim_quotes_expr("tupleElement(keysAndValues, 2)")} as value + value_filter = request.GET.get("value") + + query = f""" + SELECT {trim_quotes_expr("tupleElement(keysAndValues, 2)")} as value, count(*) as count FROM groups ARRAY JOIN JSONExtractKeysAndValuesRaw(group_properties) as keysAndValues - WHERE team_id = %(team_id)s AND group_type_index = %(group_type_index)s AND tupleElement(keysAndValues, 1) = %(key)s - GROUP BY tupleElement(keysAndValues, 2) - ORDER BY value ASC - """, - { - "team_id": self.team.pk, - "group_type_index": request.GET["group_type_index"], - "key": request.GET["key"], - }, - ) + WHERE team_id = %(team_id)s + AND group_type_index = %(group_type_index)s + AND tupleElement(keysAndValues, 1) = %(key)s + {f"AND {trim_quotes_expr('tupleElement(keysAndValues, 2)')} ILIKE %(value_filter)s" if value_filter else ""} + GROUP BY value + ORDER BY count DESC, value ASC + LIMIT 20 + """ + + params = { + "team_id": self.team.pk, + "group_type_index": request.GET["group_type_index"], + "key": request.GET["key"], + } + + if value_filter: + params["value_filter"] = f"%{value_filter}%" + + rows = sync_execute(query, params) - return response.Response([{"name": name[0]} for name in rows]) + return response.Response([{"name": name, "count": count} for name, count in rows]) diff --git a/ee/clickhouse/views/test/test_clickhouse_groups.py b/ee/clickhouse/views/test/test_clickhouse_groups.py index 10e064095c421..22e0d6e21b5ae 100644 --- a/ee/clickhouse/views/test/test_clickhouse_groups.py +++ b/ee/clickhouse/views/test/test_clickhouse_groups.py @@ -309,17 +309,71 @@ def test_property_values(self): group_key="org:6", properties={"industry": "technology"}, ) + create_group( + team_id=self.team.pk, + group_type_index=0, + group_key="org:7", + properties={"industry": "finance-technology"}, + ) create_group( team_id=self.team.pk, group_type_index=1, group_key="org:1", properties={"industry": "finance"}, ) + + # Test without query parameter response_data = self.client.get( f"/api/projects/{self.team.id}/groups/property_values/?key=industry&group_type_index=0" ).json() + self.assertEqual(len(response_data), 3) + self.assertEqual( + response_data, + [ + {"name": "finance", "count": 1}, + {"name": "finance-technology", "count": 1}, + {"name": "technology", "count": 1}, + ], + ) + + # Test with query parameter + response_data = self.client.get( + f"/api/projects/{self.team.id}/groups/property_values/?key=industry&group_type_index=0&value=fin" + ).json() + self.assertEqual(len(response_data), 2) + self.assertEqual(response_data, [{"name": "finance", "count": 1}, {"name": "finance-technology", "count": 1}]) + + # Test with query parameter - case insensitive + response_data = self.client.get( + f"/api/projects/{self.team.id}/groups/property_values/?key=industry&group_type_index=0&value=TECH" + ).json() self.assertEqual(len(response_data), 2) - self.assertEqual(response_data, [{"name": "finance"}, {"name": "technology"}]) + self.assertEqual( + response_data, [{"name": "finance-technology", "count": 1}, {"name": "technology", "count": 1}] + ) + + # Test with query parameter - no matches + response_data = self.client.get( + f"/api/projects/{self.team.id}/groups/property_values/?key=industry&group_type_index=0&value=healthcare" + ).json() + self.assertEqual(len(response_data), 0) + self.assertEqual(response_data, []) + + # Test with query parameter - exact match + response_data = self.client.get( + f"/api/projects/{self.team.id}/groups/property_values/?key=industry&group_type_index=0&value=technology" + ).json() + self.assertEqual(len(response_data), 2) + self.assertEqual( + response_data, [{"name": "finance-technology", "count": 1}, {"name": "technology", "count": 1}] + ) + + # Test with different group_type_index + response_data = self.client.get( + f"/api/projects/{self.team.id}/groups/property_values/?key=industry&group_type_index=1&value=fin" + ).json() + self.assertEqual(len(response_data), 1) + self.assertEqual(response_data, [{"name": "finance", "count": 1}]) def test_empty_property_values(self): create_group(