fix(flags): say why a save stops on an incomplete release condition

frontend/src/scenes/feature-flags/FeatureFlagReleaseConditions.incompleteFilterOnSave.test.tsx

@@ -0,0 +1,140 @@
+import '@testing-library/jest-dom'
+
+import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'
+import { Provider } from 'kea'
+import { expectLogic } from 'kea-test-utils'
+
+import { useMocks } from '~/mocks/jest'
+import { initKeaTests } from '~/test/init'
+import { mockGetEventDefinitions, mockGetPropertyDefinitions } from '~/test/mocks'
+import { FeatureFlagGroupType, FeatureFlagType, PropertyFilterType, PropertyOperator } from '~/types'
+
+import { NEW_FLAG, featureFlagLogic } from './featureFlagLogic'
+import { FeatureFlagReleaseConditionsCollapsible } from './FeatureFlagReleaseConditionsCollapsible'
+import { featureFlagReleaseConditionsLogic } from './featureFlagReleaseConditionsLogic'
+
+jest.mock('lib/components/AutoSizer', () => ({
+    AutoSizer: ({ renderProp }: { renderProp: (size: { height: number; width: number }) => React.ReactNode }) =>
+        renderProp({ height: 400, width: 400 }),
+}))
+
+const INCOMPLETE_FILTER_MESSAGE = 'Add a value or remove this filter'
+
+// A property picked from the taxonomic list starts with a null value, and the form blocks the save
+// while that half-built row is on the flag.
+function buildFilters(planValue: string | null = null): FeatureFlagType['filters'] {
+    const groups: FeatureFlagGroupType[] = [
+        {
+            properties: [
+                {
+                    key: 'email',
+                    value: 'is_set',
+                    operator: PropertyOperator.IsSet,
+                    type: PropertyFilterType.Person,
+                },
+            ],
+            rollout_percentage: 100,
+            variant: null,
+            sort_key: 'group-1',
+        },
+        {
+            properties: [
+                {
+                    key: 'plan',
+                    value: planValue,
+                    operator: PropertyOperator.Exact,
+                    type: PropertyFilterType.Person,
+                },
+            ],
+            rollout_percentage: 100,
+            variant: null,
+            sort_key: 'group-2',
+        },
+    ]
+    return { groups, multivariate: null, payloads: {} }
+}
+
+describe('a save blocked by a collapsed condition set', () => {
+    let logic: ReturnType<typeof featureFlagLogic.build>
+
+    beforeEach(() => {
+        initKeaTests()
+        useMocks({
+            get: {
+                '/api/projects/:team/event_definitions': mockGetEventDefinitions,
+                '/api/projects/:team/property_definitions': mockGetPropertyDefinitions,
+                '/api/projects/:team/feature_flags/1234/': {
+                    id: 1234,
+                    key: 'test-flag',
+                    filters: { groups: [], multivariate: null, payloads: {} },
+                },
+                '/api/projects/:team/feature_flags/1234/status': { status: 'active', reason: 'mock reason' },
+                '/api/projects/:team/actions': { results: [] },
+            },
+            post: {
+                '/api/environments/:team/query': { results: [] },
+                '/api/projects/:team/feature_flags/user_blast_radius': () => [200, { affected: 0, total: 2 }],
+            },
+        })
+        logic = featureFlagLogic({ id: 1234 })
+        logic.mount()
+    })
+
+    afterEach(() => {
+        cleanup()
+        logic.unmount()
+    })
+
+    it('opens the set that holds the value-less filter, and only that set', async () => {
+        const filters = buildFilters()
+        logic.actions.setFeatureFlag({ ...NEW_FLAG, id: 1234, key: 'test-flag', filters } as FeatureFlagType)
+        render(
+            <Provider>
+                <FeatureFlagReleaseConditionsCollapsible
+                    id="1234"
+                    flagId={1234}
+                    filters={filters}
+                    onChange={logic.actions.setFeatureFlagFilters}
+                />
+            </Provider>
+        )
+
+        // Both sets start collapsed, which renders neither the inline error nor a scroll target.
+        await waitFor(() => expect(screen.getByLabelText('Toggle condition 2 details')).toBeInTheDocument())
+        expect(document.body).not.toHaveTextContent(INCOMPLETE_FILTER_MESSAGE)
+
+        await expectLogic(logic, () => {
+            logic.actions.submitFeatureFlag()
+        }).toFinishAllListeners()
+
+        await waitFor(() => expect(document.body).toHaveTextContent(INCOMPLETE_FILTER_MESSAGE))
+        expect(featureFlagReleaseConditionsLogic.findMounted({ id: '1234' })?.values.openConditions).toEqual([
+            'condition-group-2',
+        ])
+    })
+
+    it('blocks the save when a person removes the last value from a filter', async () => {
+        const filters = buildFilters('free')
+        logic.actions.setFeatureFlag({ ...NEW_FLAG, id: 1234, key: 'test-flag', filters } as FeatureFlagType)
+        render(
+            <Provider>
+                <FeatureFlagReleaseConditionsCollapsible
+                    id="1234"
+                    flagId={1234}
+                    filters={filters}
+                    onChange={logic.actions.setFeatureFlagFilters}
+                />
+            </Provider>
+        )
+
+        fireEvent.click(await screen.findByLabelText('Toggle condition 2 details'))
+        const valueChip = (await screen.findByTitle('free')).parentElement as HTMLElement
+        fireEvent.click(valueChip.querySelector('.LemonSnack__close button') as HTMLElement)
+
+        await expectLogic(logic, () => {
+            logic.actions.submitFeatureFlag()
+        }).toFinishAllListeners()
+
+        await waitFor(() => expect(document.body).toHaveTextContent(INCOMPLETE_FILTER_MESSAGE))
+    })
+})

frontend/src/scenes/feature-flags/FeatureFlagReleaseConditionsCollapsible.tsx

@@ -137,7 +137,7 @@
         return `All ${capitalizedTarget}`
     }
 
     const parts = properties.slice(0, 2).map((property) => {
         let key: string
         if (property.type === PropertyFilterType.Cohort) {
             key = 'Cohort'
@@ -419,7 +419,7 @@
     )
 }
 
 const ConditionContent = ({
     group,
     index,
     totalGroups,
@@ -650,6 +650,7 @@
                                             )}
                                             taxonomicFilterOptionsFromProp={filtersTaxonomicOptions}
                                             hasRowOperator={false}
+                                            sendAllKeyUpdates
                                             errorMessages={getPropertySelectErrorMessages(propertySelectErrors, index)}
                                             hideBehavioralCohorts={!realtimeCohortFlagTargeting}
                                             showCohortFlagTargeting
@@ -868,7 +869,7 @@
     )
 }
 
 export function FeatureFlagReleaseConditionsCollapsible({
     id,
     flagId,
     filters,

frontend/src/scenes/feature-flags/featureFlagLogic.ts

@@ -126,12 +126,14 @@
 import { FeatureFlagArchivedSource, reportFeatureFlagArchived } from './featureFlagArchiveDialog'
 import { checkFeatureFlagConfirmation } from './featureFlagConfirmationLogic'
 import type { FlagIntent } from './featureFlagIntentWarningLogic'
+import { featureFlagReleaseConditionsLogic } from './featureFlagReleaseConditionsLogic'
 import {
     ProjectSelectOption,
     aggregateCopyResponse,
     errorMessageFrom,
     projectSelectOptions,
 } from './flagSelectionLogic'
+import { PropertySelectError, getConditionSetErrors } from './propertySelectErrorMessages'
 import {
     ScheduleOccurrence,
     expandScheduleOccurrences,
@@ -152,6 +154,22 @@
     posthog.capture('feature flag scheduled')
 }
 
+// A collapsed condition-set panel renders none of its children, so an inline release-condition
+// error has no element to render into and `scrollToFormError` has nothing to scroll to. Open every
+// set that holds a blocking error before the scroll runs.
+function openConditionSets(flagId: string, conditionSetErrors: { index: number }[]): void {
+    const releaseConditionsLogic = featureFlagReleaseConditionsLogic.findMounted({ id: flagId })
+    if (!releaseConditionsLogic) {
+        return
+    }
+    for (const { index } of conditionSetErrors) {
+        const sortKey = releaseConditionsLogic.values.filters.groups[index]?.sort_key
+        if (sortKey) {
+            releaseConditionsLogic.actions.openCondition(sortKey)
+        }
+    }
+}
+
 const VALID_INTENTS: FlagIntent[] = ['local-eval', 'first-page-load']
 
 const TAB_SEARCH_PARAM = 'tab'
@@ -545,7 +563,7 @@
  * Reports the per-project outcome itself and never throws: the flag already exists,
  * so a copy failure must not fail the save.
  */
 async function copyNewFlagToAdditionalProjects(
     organizationId: string,
     flagKey: string,
     fromProjectId: number,
@@ -2999,7 +3017,7 @@
     })),
     loaders(({ values, props, actions }) => ({
         featureFlag: {
             loadFeatureFlag: async () => {
                 const sourceId = router.values.searchParams.sourceId
 
                 if (props.id === 'new' && sourceId) {
@@ -3141,7 +3159,7 @@
                     ensure_experience_continuity: values.currentTeam?.flags_persistence_default ?? false,
                 }
             },
             saveFeatureFlag: async (updatedFlag: Partial<FeatureFlagType>) => {
                 // Destructure all fields we want to exclude or handle specially
                 const flag = cleanFlag(updatedFlag)
                 const preparedFlag = indexToVariantKeyFeatureFlagPayloads(flag)
@@ -3627,7 +3645,7 @@
         },
     }),
     listeners(({ actions, values, props, sharedListeners, cache }) => ({
         loadCopyDependencyRequirements: async (_, breakpoint): Promise<void> => {
             const { copyDestinationProject, currentOrganizationId, currentProjectId, featureFlag } = values
 
             if (
@@ -3744,7 +3762,7 @@
                 actions.setCronExpression(def.enableCron)
             }
         },
         createPairedSchedule: async () => {
             const resetScheduleForm = (): void => {
                 actions.resetScheduleFormExpanded()
                 actions.setSchedulePreset(null)
@@ -3879,13 +3897,17 @@
             if (formErrors?.tags && !values.advancedPanelOpen) {
                 actions.setAdvancedExpanded(true)
             }
+            const conditionSetErrors = getConditionSetErrors(filtersErrors?.groups as PropertySelectError[] | undefined)
+            openConditionSets(String(props.id), conditionSetErrors)
             // Yield so React flushes the expand-actions re-render before scrollToFormError schedules
             // its requestAnimationFrame callback — otherwise on browsers/scheduler combinations where
             // the render lands after RAF, `.Field--error` isn't in the DOM yet and the fallback toast
             // fires instead of scrolling to the error.
             await Promise.resolve()
             scrollToFormError({
-                fallbackErrorMessage: 'This flag has validation errors. Please review the highlighted fields above.',
+                fallbackErrorMessage: conditionSetErrors.length
+                    ? `Condition set ${conditionSetErrors[0].index + 1} needs a fix: ${conditionSetErrors[0].message}`
+                    : 'This flag has validation errors. Please review the highlighted fields above.',
             })
         },
         updateFeatureFlagActiveFailure: ({ errorObject }) => {
@@ -3895,7 +3917,7 @@
 
             // For non-approval errors, let the global error handler show the toast to avoid duplicates
         },
         saveFeatureFlagSuccess: ({ featureFlag }) => {
             lemonToast.success('Feature flag saved')
             dismissAgentNotices(props.id, cache)
             actions.setFeatureFlag(featureFlag)
@@ -4030,7 +4052,7 @@
             // The stale banner is a server verdict, so it outlives the change without this.
             actions.loadFeatureFlagStatus()
         },
         refreshFeatureFlagSuccess: ({ featureFlagRefresh, payload }) => {
             if (!featureFlagRefresh) {
                 return
             }

frontend/src/scenes/feature-flags/featureFlagReleaseConditionsLogic.ts

@@ -91,7 +91,7 @@ export function getBlastRadiusErrorMessage(error: BlastRadiusError, pluralName:
 // surfaced inline instead of failing with an opaque 400 on submit.
 function getPropertyValueError(property: AnyPropertyFilter): string | undefined {
     if (isEmptyProperty(property)) {
-        return "Property filters can't be empty"
+        return 'Add a value or remove this filter'
     }
     if (isPropertyFilterWithOperator(property) && isOperatorSemver(property.operator)) {
         const allowWildcard = property.operator === PropertyOperator.SemverWildcard

frontend/src/scenes/feature-flags/propertySelectErrorMessages.tsx

@@ -35,3 +35,15 @@ export function getPropertySelectErrorMessages(
     })
     return hasError ? messages : null
 }
+
+// Condition sets that hold a blocking error, with the first message for each. A blocked save uses
+// this to open the sets it has to open, and to name one when it can't scroll to the field.
+export function getConditionSetErrors(
+    propertySelectErrors: PropertySelectError[] | null | undefined
+): { index: number; message: string }[] {
+    return (propertySelectErrors ?? []).flatMap((error, index) => {
+        const propertyMessage = error?.properties?.find((property) => typeof property?.value === 'string')?.value
+        const message = propertyMessage ?? error?.rollout_percentage
+        return message ? [{ index, message }] : []
+    })
+}