From eaa835307fa5e1e79269aacde0b7aa140e763f8e Mon Sep 17 00:00:00 2001 From: Riley Jones <78179109+rileyajones@users.noreply.github.com> Date: Fri, 17 Mar 2023 12:17:59 -0700 Subject: [PATCH] Feature: Update the showFlags feature to allow filtering (#6245) ## Motivation for features / changes As we add more feature flags it's starting to get hard to find them. This is particularly true when trying to help someone else find a feature flag in the modal. The `enableShowFlags` feature was a weird edge case that wasn't being fully utilized. Now it supports string values. When a string value is supplied it filters the feature flags shown. ## Technical description of changes I changed the name and value of `enableShowFlags: boolean` to `showFlags: string`. I then updated the `feature_flag_page_container` to filter the feature flags rendered based on the value of the `showFlags` feature. ## Screenshots of UI changes Modal With a Filter ![image](https://user-images.githubusercontent.com/78179109/225770098-5460517e-8946-42f2-8f6a-b26980b63ffc.png) Modal without a filter ![image](https://user-images.githubusercontent.com/78179109/225770279-034c0eb7-a566-4376-b265-39d6adae4b9d.png) ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Navigate to localhost:6006 3) Assert the feature flags modal does not appear 4) Navigate to localhost:6006?showFlags 5) Assert the feature flags modal appears 6) Assert the no filter is applied 7) Navigate to localhost:6006?showFlags=enable 8) Assert the feature flags modal appears and has a filter applied ## Alternate designs / implementations considered Adding the filter could automatically toggle a feature? Adding an input field to filter things would make sense... --- tensorboard/webapp/BUILD | 1 + .../store/feature_flag_metadata.ts | 6 +- .../store/feature_flag_selectors.ts | 2 +- tensorboard/webapp/feature_flag/types.ts | 4 +- .../feature_flag_modal_trigger_container.ts | 4 +- ...ature_flag_modal_trigger_container_test.ts | 4 +- .../feature_flag_modal_trigger_module.ts | 2 +- .../views/feature_flag_page_component.ng.html | 6 ++ .../views/feature_flag_page_component.ts | 2 + .../views/feature_flag_page_container.ts | 60 ++++++++++----- .../views/feature_flag_page_test.ts | 74 +++++++++++++++++++ 11 files changed, 134 insertions(+), 31 deletions(-) diff --git a/tensorboard/webapp/BUILD b/tensorboard/webapp/BUILD index 873b05e9d3..004a47120c 100644 --- a/tensorboard/webapp/BUILD +++ b/tensorboard/webapp/BUILD @@ -268,6 +268,7 @@ tf_ng_web_test_suite( "//tensorboard/webapp/core/views:test_lib", "//tensorboard/webapp/customization:customization_test_lib", "//tensorboard/webapp/deeplink:deeplink_test_lib", + "//tensorboard/webapp/feature_flag/views:views_test", "//tensorboard/webapp/header:test_lib", "//tensorboard/webapp/metrics:integration_test", "//tensorboard/webapp/metrics:test_lib", diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts index 2c5003e406..52d7ad11ce 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts @@ -110,10 +110,10 @@ export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType = defaultValue: true, queryParamOverride: null, }, - enableShowFlags: { - defaultValue: false, + showFlags: { + defaultValue: undefined, queryParamOverride: 'showFlags', - parseValue: parseBoolean, + parseValue: (str) => str, }, allowRangeSelection: { defaultValue: true, diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts b/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts index 54c9e77849..b0d6d5c3f4 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts @@ -150,7 +150,7 @@ export const getIsDataTableEnabled = createSelector( export const getShowFlagsEnabled = createSelector( getFeatureFlags, (flags: FeatureFlags): boolean => { - return flags.enableShowFlags; + return flags.showFlags !== undefined; } ); diff --git a/tensorboard/webapp/feature_flag/types.ts b/tensorboard/webapp/feature_flag/types.ts index 2d069a3292..3f9ea33ebc 100644 --- a/tensorboard/webapp/feature_flag/types.ts +++ b/tensorboard/webapp/feature_flag/types.ts @@ -44,8 +44,8 @@ export interface FeatureFlags { forceSvg: boolean; // Whether to enable the "sticky" data table in scalar cards. enabledScalarDataTable: boolean; - // If enabled causes the feature flags modal to appear. - enableShowFlags: boolean; + // If defined causes the feature flags modal to appear. + showFlags: string | undefined; // Adds check box in settings which allows users to enter step selection range. allowRangeSelection: boolean; // In Linked Time, if enabled, show a prospective fob user to turn on the feature or select a step. diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts b/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts index 68b8c79028..0e5e4b6593 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts +++ b/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts @@ -49,12 +49,12 @@ export class FeatureFlagModalTriggerContainer implements OnInit { // dialog from appearing again after the page is refreshed. this.store.dispatch( featureFlagOverridesReset({ - flags: ['enableShowFlags'], + flags: ['showFlags'], }) ); // Reload the page so that the application restarts with stable // feature flag values. - // Wait one tick before reloading the page so the 'enableShowFlags' + // Wait one tick before reloading the page so the 'showFlags' // reset has a chance to be reflected in the URL before page reload. setTimeout(() => { util.reloadWindow(); diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container_test.ts b/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container_test.ts index 07b282f948..8ae9285b65 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container_test.ts +++ b/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container_test.ts @@ -77,7 +77,7 @@ describe('feature_flag_modal_trigger_container', () => { rootLoader = TestbedHarnessEnvironment.documentRootLoader(fixture); } - it('creates modal when enableShowFlags is true', async () => { + it('creates modal when showFlags is true', async () => { store.overrideSelector(getDefaultFeatureFlags, {} as FeatureFlags); store.overrideSelector(getOverriddenFeatureFlags, {}); store.overrideSelector(getShowFlagsEnabled, true); @@ -90,7 +90,7 @@ describe('feature_flag_modal_trigger_container', () => { expect(dialog).toBeDefined(); }); - it('does not create modal when enableShowFlags is false', async () => { + it('does not create modal when showFlags is false', async () => { store.overrideSelector(getDefaultFeatureFlags, {} as FeatureFlags); store.overrideSelector(getOverriddenFeatureFlags, {}); store.overrideSelector(getShowFlagsEnabled, false); diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_module.ts b/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_module.ts index 2578bd92d1..389565cdbc 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_module.ts +++ b/tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_module.ts @@ -19,7 +19,7 @@ import {FeatureFlagPageModule} from './feature_flag_module'; /** * Provides the wrapper component that triggers the opening of the feature flag modal. - * The modal appears based on the value of the enableShowFlags feature flag. + * The modal appears based on the value of the showFlags feature flag. */ @NgModule({ declarations: [FeatureFlagModalTriggerContainer], diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ng.html b/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ng.html index ad4e5785ca..b7760da39a 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ng.html +++ b/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ng.html @@ -30,6 +30,12 @@

WARNING: EXPERIMENTAL FEATURES AHEAD!

Only flags with non default values are sent to the backend. + +
+ Feature Flags are filtered to only show features containing + "{{showFlagsFilter}}" +
+ diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts b/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts index da68de2e4b..e83944be5f 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts +++ b/tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts @@ -27,6 +27,8 @@ export class FeatureFlagPageComponent { @Input() hasFlagsSentToServer: boolean = false; + @Input() showFlagsFilter: string | undefined; + @Output() flagChanged = new EventEmitter(); @Output() allFlagsReset = new EventEmitter(); diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts b/tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts index 5622ee651e..4906135d3c 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts +++ b/tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts @@ -44,6 +44,7 @@ import { template: ``, @@ -51,6 +52,12 @@ import { export class FeatureFlagPageContainer { constructor(private readonly store: Store) {} + readonly showFlagsFilter$ = this.store.select(getOverriddenFeatureFlags).pipe( + map((overriddenFeatureFlags) => { + return overriddenFeatureFlags.showFlags?.toLowerCase(); + }) + ); + readonly hasFlagsSentToServer$: Observable = this.store .select(getFeatureFlagsMetadata) .pipe( @@ -66,27 +73,40 @@ export class FeatureFlagPageContainer { this.store.select(getOverriddenFeatureFlags).pipe( withLatestFrom( this.store.select(getDefaultFeatureFlags), - this.store.select(getFeatureFlagsMetadata) + this.store.select(getFeatureFlagsMetadata), + this.showFlagsFilter$ ), - map(([overriddenFeatureFlags, defaultFeatureFlags, flagMetadata]) => { - return Object.entries(defaultFeatureFlags).map( - ([flagName, defaultValue]) => { - const status = getFlagStatus( - flagName as keyof FeatureFlags, - overriddenFeatureFlags - ); - const metadata = flagMetadata[flagName as keyof FeatureFlags]; - return { - flag: flagName, - defaultValue, - status, - sendToServerWhenOverridden: ( - metadata as AdvancedFeatureFlagMetadata - ).sendToServerWhenOverridden, - } as FeatureFlagStatus; - } - ); - }) + map( + ([ + overriddenFeatureFlags, + defaultFeatureFlags, + flagMetadata, + showFlagsFilter, + ]) => { + return Object.entries(defaultFeatureFlags) + .filter(([flagName]) => { + if (!showFlagsFilter) { + return true; + } + return flagName.toLowerCase().includes(showFlagsFilter); + }) + .map(([flagName, defaultValue]) => { + const status = getFlagStatus( + flagName as keyof FeatureFlags, + overriddenFeatureFlags + ); + const metadata = flagMetadata[flagName as keyof FeatureFlags]; + return { + flag: flagName, + defaultValue, + status, + sendToServerWhenOverridden: ( + metadata as AdvancedFeatureFlagMetadata + ).sendToServerWhenOverridden, + } as FeatureFlagStatus; + }); + } + ) ); onFlagChanged({flag, status}: FeatureFlagStatusEvent) { diff --git a/tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts b/tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts index d44e590035..fd42c5892c 100644 --- a/tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts +++ b/tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts @@ -234,4 +234,78 @@ describe('feature_flag_page_container', () => { expect(component.formatFlagValue('')).toEqual(''); }); }); + + describe('filters flags based on the value of showFlags feature', () => { + beforeEach(() => { + store.overrideSelector(getDefaultFeatureFlags, { + enabledLinkedTime: true, + enabledProspectiveFob: true, + inColab: false, + allowRangeSelection: true, + } as FeatureFlags); + }); + + it('shows all flags when value is undefined', () => { + store.overrideSelector(getOverriddenFeatureFlags, { + showFlags: undefined, + }); + createComponent(); + const component = getComponent(); + + const rows = component.querySelectorAll('tr'); + expect(rows.length).toEqual(4); + }); + + it('shows all flags when value is empty string', () => { + store.overrideSelector(getOverriddenFeatureFlags, { + showFlags: '', + }); + createComponent(); + const component = getComponent(); + + const rows = component.querySelectorAll('tr'); + expect(rows.length).toEqual(4); + }); + + it('only shows flags whose name includes filter', () => { + store.overrideSelector(getOverriddenFeatureFlags, { + showFlags: 'enable', + }); + createComponent(); + const component = getComponent(); + + expect(component.querySelectorAll('tr').length).toEqual(2); + + store.overrideSelector(getOverriddenFeatureFlags, { + showFlags: 'linked', + }); + store.refreshState(); + fixture.detectChanges(); + expect(component.querySelectorAll('tr').length).toEqual(1); + }); + + it('shows message when flags are filtered', () => { + store.overrideSelector(getOverriddenFeatureFlags, { + showFlags: 'enable', + }); + + createComponent(); + const component = getComponent(); + expect(component.innerText).toContain( + 'Feature Flags are filtered to only show features containing' + ); + }); + + it('does not show message when flags are not filtered', () => { + store.overrideSelector(getOverriddenFeatureFlags, { + showFlags: undefined, + }); + + createComponent(); + const component = getComponent(); + expect(component.innerText).not.toContain( + 'Feature Flags are filtered to only show features containing' + ); + }); + }); });