fix(flags): Validate rollout % backwards compatibly (#20092)

PostHog · Feb 2, 2024 · f0c4908 · f0c4908
1 parent 0a37a48
commit f0c4908
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 16 deletions.
diff --git a/ee/clickhouse/views/experiments.py b/ee/clickhouse/views/experiments.py
@@ -215,7 +215,7 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment:
         ]
 
         filters = {
-            "groups": [{"properties": properties, "rollout_percentage": None}],
+            "groups": [{"properties": properties, "rollout_percentage": 100}],
             "multivariate": {"variants": variants or default_variants},
             "aggregation_group_type_index": aggregation_group_type_index,
         }

diff --git a/ee/clickhouse/views/test/test_clickhouse_experiments.py b/ee/clickhouse/views/test/test_clickhouse_experiments.py
@@ -998,7 +998,7 @@ def test_create_experiment_updates_feature_flag_cache(self):
                 "groups": [
                     {
                         "properties": [],
-                        "rollout_percentage": None,
+                        "rollout_percentage": 100,
                     }
                 ],
                 "multivariate": {
@@ -1049,7 +1049,7 @@ def test_create_experiment_updates_feature_flag_cache(self):
                 "groups": [
                     {
                         "properties": [],
-                        "rollout_percentage": None,
+                        "rollout_percentage": 100,
                     }
                 ],
                 "multivariate": {
@@ -1116,7 +1116,7 @@ def test_create_experiment_updates_feature_flag_cache(self):
                 "groups": [
                     {
                         "properties": [],
-                        "rollout_percentage": None,
+                        "rollout_percentage": 100,
                     }
                 ],
                 "multivariate": {

diff --git a/frontend/src/scenes/feature-flags/FeatureFlagReleaseConditions.tsx b/frontend/src/scenes/feature-flags/FeatureFlagReleaseConditions.tsx
@@ -2,12 +2,13 @@ import './FeatureFlag.scss'
 
 import { LemonInput, LemonSelect, Link } from '@posthog/lemon-ui'
 import { useActions, useValues } from 'kea'
-import { Field, Group } from 'kea-forms'
+import { Group } from 'kea-forms'
 import { router } from 'kea-router'
 import { allOperatorsToHumanName } from 'lib/components/DefinitionPopover/utils'
 import { PropertyFilters } from 'lib/components/PropertyFilters/PropertyFilters'
 import { isPropertyFilterWithOperator } from 'lib/components/PropertyFilters/utils'
 import { FEATURE_FLAGS, INSTANTLY_AVAILABLE_PROPERTIES } from 'lib/constants'
+import { Field } from 'lib/forms/Field'
 import { groupsAccessLogic, GroupsAccessStatus } from 'lib/introductions/groupsAccessLogic'
 import { GroupsIntroductionOption } from 'lib/introductions/GroupsIntroductionOption'
 import { IconCopy, IconDelete, IconErrorOutline, IconOpenInNew, IconPlus, IconSubArrowRight } from 'lib/lemon-ui/icons'
@@ -229,7 +230,7 @@ export function FeatureFlagReleaseConditions({
                                               return message.value ? (
                                                   <div
                                                       key={index}
-                                                      className="text-danger flex items-center gap-1 text-sm"
+                                                      className="text-danger flex items-center gap-1 text-sm Field--error"
                                                   >
                                                       <IconErrorOutline className="text-xl" /> {message.value}
                                                   </div>
@@ -268,11 +269,7 @@ export function FeatureFlagReleaseConditions({
                             <div className="flex items-center gap-1">
                                 Roll out to{' '}
                                 <LemonSlider
-                                    value={
-                                        group.rollout_percentage
-                                            ? Math.max(Math.min(group.rollout_percentage, 100), 0)
-                                            : undefined
-                                    }
+                                    value={group.rollout_percentage !== null ? group.rollout_percentage : 100}
                                     onChange={(value) => {
                                         updateConditionSet(index, value)
                                     }}
@@ -290,7 +287,7 @@ export function FeatureFlagReleaseConditions({
                                             onChange={(value): void => {
                                                 updateConditionSet(index, value === undefined ? 0 : value)
                                             }}
-                                            value={group.rollout_percentage ?? undefined}
+                                            value={group.rollout_percentage !== null ? group.rollout_percentage : 100}
                                             min={0}
                                             max={100}
                                             step="any"

diff --git a/frontend/src/scenes/feature-flags/featureFlagLogic.ts b/frontend/src/scenes/feature-flags/featureFlagLogic.ts
@@ -716,6 +716,13 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
                 )
             }
         },
+        submitFeatureFlagFailure: async () => {
+            // When errors occur, scroll to the error, but wait for errors to be set in the DOM first
+            setTimeout(
+                () => document.querySelector(`.Field--error`)?.scrollIntoView({ block: 'center', behavior: 'smooth' }),
+                1
+            )
+        },
         saveFeatureFlagSuccess: ({ featureFlag }) => {
             lemonToast.success('Feature flag saved')
             featureFlagsLogic.findMounted()?.actions.updateFlag(featureFlag)
@@ -1005,9 +1012,7 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
                                     : undefined,
                         })),
                         rollout_percentage:
-                            rollout_percentage === null || rollout_percentage === undefined
-                                ? 'You need to set a rollout % value'
-                                : undefined,
+                            rollout_percentage === undefined ? 'You need to set a rollout % value' : undefined,
                     })
                 )
             },
@@ -1016,7 +1021,11 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
             (s) => [s.affectedUsers, s.totalUsers],
             (affectedUsers, totalUsers) => (rolloutPercentage, index) => {
                 let effectiveRolloutPercentage = rolloutPercentage
-                if (rolloutPercentage === undefined || rolloutPercentage === null) {
+                if (
+                    rolloutPercentage === undefined ||
+                    rolloutPercentage === null ||
+                    (rolloutPercentage && rolloutPercentage > 100)
+                ) {
                     effectiveRolloutPercentage = 100
                 }