Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds a search box to the toolbar featureflag list #6527

Merged
merged 9 commits into from
Oct 20, 2021
Merged

adds a search box to the toolbar featureflag list #6527

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
876959e
adds a search box to the toolbar featureflag list
pauldambra Oct 18, 2021
7203229
test flakes because react is re-rendering between the get and the cli…
pauldambra Oct 19, 2021
ed57718
move filtering toolbar feature flags to a Set and out of CSS
pauldambra Oct 19, 2021
634ded7
support initKeaTestLogic() with no args
mariusandra Oct 20, 2021
992a4aa
fix query-selector-all-deep jest bug
mariusandra Oct 20, 2021
7e93b41
add simple test case for feature flags logic
mariusandra Oct 20, 2021
93759fc
combine selectors
pauldambra Oct 20, 2021
d3153e0
with more understanding of Fuse
pauldambra Oct 20, 2021
2b48d52
add simple test for flag filtering
pauldambra Oct 20, 2021
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 cypress/integration/invitesMembers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe('Invite Signup', () => {
cy.get('[data-attr=invite-teammate-button]').first().click()
// Enter invite the user
cy.get('[data-attr=invite-email-input]').type(`fake+${Math.floor(Math.random() * 10000)}@posthog.com`)
cy.get('[data-attr=invite-team-member-submit]').click()
cy.get('[data-attr=invite-team-member-submit]').should('not.be.disabled').click()

// Log in as invited user
cy.get('[data-attr=invite-link]')
Expand Down
18 changes: 11 additions & 7 deletions frontend/src/test/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ export function initKeaTestLogic<L extends Logic = Logic>({
props,
onLogic,
}: {
logic: LogicWrapper<L>
logic?: LogicWrapper<L>
props?: LogicWrapper<L>['props']
onLogic?: (l: BuiltLogic<L>) => any
}): void {
} = {}): void {
let builtLogic: BuiltLogic<L>
let unmount: () => void

Expand All @@ -33,13 +33,17 @@ export function initKeaTestLogic<L extends Logic = Logic>({
;(history as any).pushState = history.push
;(history as any).replaceState = history.replace
initKea({ beforePlugins: [testUtilsPlugin], routerLocation: history.location, routerHistory: history })
builtLogic = logic.build({ ...props })
await onLogic?.(builtLogic)
unmount = builtLogic.mount()
if (logic) {
builtLogic = logic.build({ ...props })
await onLogic?.(builtLogic)
unmount = builtLogic.mount()
}
})

afterEach(async () => {
unmount()
await expectLogic(logic).toFinishAllListeners()
if (logic) {
unmount()
await expectLogic(logic).toFinishAllListeners()
}
})
}
149 changes: 82 additions & 67 deletions frontend/src/toolbar/flags/FeatureFlags.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ import './featureFlags.scss'
import React from 'react'
import { useActions, useValues } from 'kea'
import { featureFlagsLogic } from '~/toolbar/flags/featureFlagsLogic'
import { Radio, Switch, Row, Typography, List, Button } from 'antd'
import { Radio, Switch, Row, Typography, List, Button, Input } from 'antd'
import { AnimatedCollapsible } from './AnimatedCollapsible'
import { PostHog } from 'posthog-js'
import { toolbarLogic } from '~/toolbar/toolbarLogic'
import { urls } from 'scenes/urls'
import { IconExternalLinkBold } from 'lib/components/icons'

export function FeatureFlags(): JSX.Element {
const { userFlagsWithCalculatedInfo, showLocalFeatureFlagWarning } = useValues(featureFlagsLogic)
const { setOverriddenUserFlag, deleteOverriddenUserFlag, setShowLocalFeatureFlagWarning } =
const { showLocalFeatureFlagWarning, searchTerm, filteredFlags } = useValues(featureFlagsLogic)
const { setOverriddenUserFlag, deleteOverriddenUserFlag, setShowLocalFeatureFlagWarning, setSearchTerm } =
useActions(featureFlagsLogic)
const { apiURL } = useValues(toolbarLogic)

Expand Down Expand Up @@ -40,79 +40,94 @@ export function FeatureFlags(): JSX.Element {
</div>
</div>
) : (
<List
dataSource={userFlagsWithCalculatedInfo}
renderItem={({
feature_flag,
value_for_user_without_override,
override,
hasVariants,
currentValue,
}) => {
return (
<div className="feature-flag-row">
<Row
className={
override ? 'feature-flag-row-header overridden' : 'feature-flag-row-header'
}
>
<Typography.Text ellipsis className="feature-flag-title">
{feature_flag.key}
</Typography.Text>
<a
className="feature-flag-external-link"
href={`${apiURL}${
feature_flag.id ? urls.featureFlag(feature_flag.id) : urls.featureFlags()
}`}
target="_blank"
rel="noopener noreferrer"
>
<IconExternalLinkBold />
</a>
<Switch
checked={!!currentValue}
onChange={(checked) => {
const newValue =
hasVariants && checked
? (feature_flag.filters?.multivariate?.variants[0]?.key as string)
: checked
if (newValue === value_for_user_without_override && override) {
deleteOverriddenUserFlag(override.id as number)
} else {
setOverriddenUserFlag(feature_flag.id as number, newValue)
}
}}
/>
</Row>

<AnimatedCollapsible collapsed={!hasVariants || !currentValue}>
<>
<Input.Search
allowClear
autoFocus
placeholder="Search"
value={searchTerm}
className={'feature-flag-row'}
onChange={(e) => setSearchTerm(e.target.value)}
/>
<List
dataSource={filteredFlags}
renderItem={({
feature_flag,
value_for_user_without_override,
override,
hasVariants,
currentValue,
}) => {
return (
<div className={'feature-flag-row'}>
<Row
className={override ? 'variant-radio-group overridden' : 'variant-radio-group'}
className={
override ? 'feature-flag-row-header overridden' : 'feature-flag-row-header'
}
>
<Radio.Group
disabled={!currentValue}
value={currentValue}
onChange={(event) => {
const newValue = event.target.value
<Typography.Text ellipsis className="feature-flag-title">
{feature_flag.key}
</Typography.Text>
<a
className="feature-flag-external-link"
href={`${apiURL}${
feature_flag.id
? urls.featureFlag(feature_flag.id)
: urls.featureFlags()
}`}
target="_blank"
rel="noopener noreferrer"
>
<IconExternalLinkBold />
</a>
<Switch
checked={!!currentValue}
onChange={(checked) => {
const newValue =
hasVariants && checked
? (feature_flag.filters?.multivariate?.variants[0]
?.key as string)
: checked
if (newValue === value_for_user_without_override && override) {
deleteOverriddenUserFlag(override.id as number)
} else {
setOverriddenUserFlag(feature_flag.id as number, newValue)
}
}}
>
{feature_flag.filters?.multivariate?.variants.map((variant) => (
<Radio key={variant.key} value={variant.key}>
{`${variant.key} - ${variant.name} (${variant.rollout_percentage}%)`}
</Radio>
))}
</Radio.Group>
/>
</Row>
</AnimatedCollapsible>
</div>
)
}}
/>

<AnimatedCollapsible collapsed={!hasVariants || !currentValue}>
<Row
className={
override ? 'variant-radio-group overridden' : 'variant-radio-group'
}
>
<Radio.Group
disabled={!currentValue}
value={currentValue}
onChange={(event) => {
const newValue = event.target.value
if (newValue === value_for_user_without_override && override) {
deleteOverriddenUserFlag(override.id as number)
} else {
setOverriddenUserFlag(feature_flag.id as number, newValue)
}
}}
>
{feature_flag.filters?.multivariate?.variants.map((variant) => (
<Radio key={variant.key} value={variant.key}>
{`${variant.key} - ${variant.name} (${variant.rollout_percentage}%)`}
</Radio>
))}
</Radio.Group>
</Row>
</AnimatedCollapsible>
</div>
)
}}
/>
</>
)}
</div>
)
Expand Down
22 changes: 22 additions & 0 deletions frontend/src/toolbar/flags/featureFlagsLogic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { expectLogic } from 'kea-test-utils'
import { initKeaTestLogic } from '~/test/init'
import { featureFlagsLogic } from '~/toolbar/flags/featureFlagsLogic'
import { toolbarLogic } from '~/toolbar/toolbarLogic'

describe('feature flags logic', () => {
let logic: ReturnType<typeof featureFlagsLogic.build>

initKeaTestLogic()

beforeEach(() => {
toolbarLogic({ apiURL: 'http://localhost' }).mount()
logic = featureFlagsLogic()
logic.mount()
})

it('has expected defaults', () => {
expectLogic(logic).toMatchValues({
userFlags: [],
})
})
})
33 changes: 27 additions & 6 deletions frontend/src/toolbar/flags/featureFlagsLogic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import { featureFlagsLogicType } from './featureFlagsLogicType'
import { PostHog } from 'posthog-js'
import { toolbarFetch } from '~/toolbar/utils'
import { toolbarLogic } from '~/toolbar/toolbarLogic'
import Fuse from 'fuse.js'

export const featureFlagsLogic = kea<featureFlagsLogicType>({
actions: {
getUserFlags: true,
setOverriddenUserFlag: (flagId: number, overrideValue: string | boolean) => ({ flagId, overrideValue }),
deleteOverriddenUserFlag: (overrideId: number) => ({ overrideId }),
setShowLocalFeatureFlagWarning: (showWarning: boolean) => ({ showWarning }),
setSearchTerm: (searchTerm: string) => ({ searchTerm }),
},

loaders: ({ values }) => ({
Expand All @@ -23,8 +25,7 @@ export const featureFlagsLogic = kea<featureFlagsLogicType>({
if (!response.ok) {
return []
}
const results = await response.json()
return results
return await response.json()
},
setOverriddenUserFlag: async ({ flagId, overrideValue }, breakpoint) => {
const response = await toolbarFetch(
Expand Down Expand Up @@ -67,6 +68,12 @@ export const featureFlagsLogic = kea<featureFlagsLogicType>({
],
}),
reducers: {
searchTerm: [
'',
{
setSearchTerm: (_, { searchTerm }) => searchTerm,
},
],
showLocalFeatureFlagWarning: [
false,
{
Expand All @@ -92,12 +99,26 @@ export const featureFlagsLogic = kea<featureFlagsLogicType>({
})
},
],
countFlagsOverridden: [
(s) => [s.userFlags],
(userFlags) => {
return userFlags.filter((flag) => !!flag.override).length
flagNames: [(s) => [s.userFlags], (userFlags) => userFlags.map((uf) => uf.feature_flag.name)],
searchMatches: [
(s) => [s.searchTerm, s.flagNames],
(searchTerm, flagNames) => {
const filteredNames = searchTerm
? new Fuse(flagNames, {
threshold: 0.3,
})
.search(searchTerm)
.map(({ item }) => item)
: flagNames
return new Set(filteredNames)
},
],
filteredFlags: [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mariusandra I thought this was what you were suggesting. Is it worth syncing up if it isn't cos I don't understand if so :)

Copy link
Collaborator

@mariusandra mariusandra Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the Fuse logic into filteredFlags. There's no need to hold in memory a separate list of matching names of flags.

Basically, change this:

userFlags 
  |-> userFlagsWithCalculatedInfo \
  \-> flagNames                   |-> filteredFlags
       ^   \-> searchMatches------/
searchFilter

into this:

userFlags & searhFilter -> userFlagsWithCalculatedInfo -> filteredFlags

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smooth laminar data flow, no loops ➰

(s) => [s.searchMatches, s.userFlagsWithCalculatedInfo],
(searchMatches, userFlagsWithCalculatedInfo) =>
userFlagsWithCalculatedInfo.filter((uf) => searchMatches.has(uf.feature_flag.name)),
],
countFlagsOverridden: [(s) => [s.userFlags], (userFlags) => userFlags.filter((flag) => !!flag.override).length],
},
events: ({ actions }) => ({
afterMount: () => {
Expand Down
5 changes: 1 addition & 4 deletions jest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,7 @@ export default {
// transform: undefined,

// An array of regexp pattern strings that are matched against all source file paths, matched files will skip transformation
// transformIgnorePatterns: [
// "/node_modules/",
// "\\.pnp\\.[^\\/]+$"
// ],
transformIgnorePatterns: ['node_modules/(?!(query-selector-shadow-dom)/)'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I wondered if it was that! 🙌


// An array of regexp pattern strings that are matched against all modules before the module loader will automatically return a mock for them
// unmockedModulePathPatterns: undefined,
Expand Down