Show flags page as modal when showFlags query parameter is set #5800

rileyajones · 2022-07-11T21:39:40Z

I'm breaking this change into two parts and blocking this with #5868

Description of Changes

Add new feature flag to control showing the feature flag modal, add modal to wrapper component to show flags page when the showFlags query parameter is set.

~~Currently blocked by 5717~~

Motivation for features / changes

As of 5760 we have a /flags page which is not accessible due to the backend routing code.

Last week I suggested updating the routing code to enable /flags to be shown (5796). And it was decided that it was undesirable to complicate the routing code and thus decided to show the page as a modal instead.

Technical description of changes

I've chosen to show the FlagsPageContainer as the contents of a MatDialog modal based on the status of the enableShowFlags feature flag which is controlled by the query parameter showFlags.

Given that we want to use the modal as an alternative to a route, it should appear on all pages and that the modal is unrelated to any existing part of the page, I have elected to create and manage it as part of the page wrapper.

Screenshots of UI changes

`showFlags=true`

`showFlags` not set

Detailed steps to verify changes work correctly (as executed by you)

Launch and navigate to TensorBoard.
Note that the flags modal does not appear.
Add the query param showFlags=true or showFlags.
Assert that the modal is shown.
Assert that clicking behind the modal results in it being hidden.
Remove the query param or set showFlags=false and assert the modal no longer appears.

Alternate designs / implementations considered

tensorboard/webapp/tb_wrapper/tb_wrapper_component.ts

tensorboard/webapp/feature_flag/views/feature_flag_page.ng.html

tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts

tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts

tensorboard/webapp/tb_wrapper/tb_wrapper_component.ts

tensorboard/webapp/feature_flag/views/feature_flag_module.ts

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_component.ts

bmd3k · 2022-08-19T11:19:26Z

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_component.ts

+        this.featureFlagsDialog = this.dialog.open(FeatureFlagPageContainer);
+        this.featureFlagsDialog.afterClosed().subscribe(() => {
+          this.store.dispatch(
+            featureFlagOverrideChanged({


Interesting. Can you add a comment explaining why you do this? (I guess you are just trying to keep the dialog from reopening?)

I'll go ahead and add a comment. This is to stop the dialog from opening again after the page is refreshed as I imagine people do not want this to always appear.

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_component.ts

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_module.ts

tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts

tensorboard/webapp/feature_flag/views/types.ts

tensorboard/webapp/feature_flag/views/feature_flag_page.ng.html

tensorboard/webapp/feature_flag/views/feature_flag_page_container.ts

bmd3k

It's a big change so I can only give you a partial review. I've reviewed everything in-depth except for the modal code and tests.

bmd3k · 2022-08-23T17:44:03Z

tensorboard/webapp/BUILD

        "//tensorboard/webapp/customization:customization_test_lib",
        "//tensorboard/webapp/deeplink:deeplink_test_lib",
        "//tensorboard/webapp/feature_flag/effects:effects_test_lib",
+        "//tensorboard/webapp/feature_flag/views:views_test",


I realize this is confusing but feature_flag already has its own test_suite here:
https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/feature_flag/BUILD;l=64;drc=025eb00cef2bbcc48f28667f6819a7bec7e701a6

Can we move this there? And feature_flag/effects:effects_test_lib is duplicated in both test_suites so the one here should also be deleted

The rationale: At some point Stephan believed we should pile all tests into a single test_suite because there is overhead in bringing up the browser. But it turns out there is also overhead in running all your tests serially. So he started advocating for breaking up our test_suites. Feature flags is one of the few that has since been broken up.

That was in fact confusing but I'll go ahead and consolidate these. Out of curiosity do you have any idea as to why it has its own test suite? Looking deeper it seems there are actually 13 test suites?

tensorboard/webapp/feature_flag/views/BUILD

tensorboard/webapp/feature_flag/views/feature_flag_page_container_test.ts

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container_test.ts

bmd3k · 2022-08-24T13:23:16Z

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts

+        this.featureFlagsDialog = this.dialog.open(FeatureFlagPageContainer);
+        this.featureFlagsDialog.afterClosed().subscribe(() => {
+          // By disabling the flag when the dialog is closed to prevent it from appearing again after the page is refreshed.
+          this.store.dispatch(


Thinking about this after playing with the code locally: I think when the dialog closes the page should be refreshed.

My reasoning: This is the first time a user can change feature flag state AFTER the app has been loaded. This probably opens up a ton of subtle bugs that we never had to consider in the past. We shouldn't allow the user to play with the app in this state. Refreshing the page after the dialog is closed would put the app back into a proper state.

I agree with your logic on this but I'm not sure on the UX. It seems unintuitive that closing a modal would refresh the page.
Perhaps we should add a "Close and Refresh" button along with a message explaining why? I would really like to make the user aware of the page refresh.

Sure. Let's address in a followup PR. The key is that users should not be able to use the app after changing the flag state.

bmd3k · 2022-08-24T13:24:47Z

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts

+        return;
+      }
+      if (this.featureFlagsDialog) {
+        this.featureFlagsDialog.close();


This doesn't seem right. The only times I can imagine this code path being invoked are:

User sets showFlags to false in the dialog.

User presses "Reset All" in the dialog.

In neither case would I expect the dialog to be immediately closed.

I think it depends on how you think about showFlags. If you really think it is a feature flag then it makes sense that it should close, but I can definitely see why you would view that as confusing so I'll go ahead and remove this.

bmd3k

Thanks, I think the rest looks pretty good. I think the biggest thing I'd recommend is to reorganize/refactor the feature_flag_page tests a bit to adhere to team norms. Otherwise just some nits.

bmd3k · 2022-08-25T14:22:20Z

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container.ts

+        this.featureFlagsDialog = this.dialog.open(FeatureFlagPageContainer);
+        this.featureFlagsDialog.afterClosed().subscribe(() => {
+          // By disabling the flag when the dialog is closed to prevent it from appearing again after the page is refreshed.
+          this.store.dispatch(


Sure. Let's address in a followup PR. The key is that users should not be able to use the app after changing the flag state.

tensorboard/webapp/feature_flag/views/feature_flag_modal_trigger_container_test.ts

tensorboard/webapp/feature_flag/views/feature_flag_page.ng.html

bmd3k · 2022-08-25T14:36:08Z

tensorboard/webapp/feature_flag/views/feature_flag_page_component_test.ts

+    return fixture.nativeElement.querySelector('feature-flag-page-component');
+  }
+
+  it('creates rows for each feature flag', async () => {


Normally we prefer to have a single test file for each container/component combination. We think it strikes a good balance between writing isolated tests but also ensuring a particular container/component combination works as expected when integrated together. The fact that they are split into a container/component is (mostly) an implementation detail that we hide from the tests.

In this case we would have a feature_flag_page_test instead of both feature_flag_page_component_test and feature_flag_page_container_test.

I mention it here specifically because for these tests where you introduce TestableComponent, you could instead test by constructing a FeatureFlagPageContainer and specifying appropriate values for your ngrx selectors and then checking that the UI renders as expected.

bmd3k · 2022-08-25T14:38:28Z

tensorboard/webapp/feature_flag/views/feature_flag_page_container_test.ts

+      store.overrideSelector(getDefaultFeatureFlags, {} as FeatureFlags);
+      store.overrideSelector(getOverriddenFeatureFlags, {});
+      const component = getComponent();
+      component.onFlagChanged({


Again in the context of a single test file for each container/component combination, we generally prefer to test dispatching actions starting at the UI. That is: setup your ngrx selectors as appropriate, render the container/component/UI, interact with an element, verify the action is fired.

I've merged the files and refactored the tests to move away from componentInstance except in the instance of formatFlagValue tests which I believe are significantly simpler as is.

tensorboard/webapp/feature_flag/views/feature_flag_page_component_test.ts

bmd3k · 2022-08-25T14:44:52Z

tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts

 @Component({
  selector: 'feature-flag-page-component',
+  styleUrls: ['feature_flag_page_style.css'],
  templateUrl: `feature_flag_page.ng.html`,


style and template files should be named feature_flag_page_component.css and feature_flag_page_component.ng.html. "style" is redundant (in our opinion). we include "component" because it differentiates them from possible "container" css and html files, too.

Alright. I didn't actually create either file but I'll go ahead and rename them.

JamesHollyer

I think this is getting pretty close. In the future try to split up PRs into smaller chunks for review.

tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts

bmd3k · 2022-08-25T20:41:04Z

tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts

+      imports: [CommonModule, FeatureFlagPageModule],
+      providers: [
+        provideMockStore({
+          initialState: buildState(


An alternative to constructing initialState like this that we usually use is to just override all the selectors that the components rely on. I'm fine with what you have since its simple but just wanted to let you know in case you find that simpler in this case.

I tried that initially and I was getting errors from the selectors indicating that state was undefined.

bmd3k · 2022-08-25T20:45:10Z

tensorboard/webapp/feature_flag/views/feature_flag_page_test.ts

+    const dataCells = component.querySelectorAll('td');
+    expect(dataCells.length).toEqual(3);
+    const selectors = component.querySelectorAll('mat-select');
+    expect(selectors.length).toEqual(1);


Would it be worth to make the tests a litlte stricter - ie actually verify that the "inColab" row has a mat-select while the "enabledExperimentalPlugins" is marked as unsupported? The test is unusually vague about this.

You're right, this is pretty vague. I've gone ahead and added another expect checking the value of the td which I think should be specific enough.

…odal to wrapper component to show flags page when the showFlags query parameter is set

…pp_container

…rflow#5800) * Add new feature flag to control showing the feature flag modal, add modal to wrapper component to show flags page when the showFlags query parameter is set

rileyajones force-pushed the feature-flags-modal branch from 0610d2e to f1914f6 Compare July 28, 2022 20:37

rileyajones force-pushed the feature-flags-modal branch 3 times, most recently from 9fe0aa0 to 2b693d4 Compare August 11, 2022 20:47

rileyajones mentioned this pull request Aug 11, 2022

Feature: Add NgRx Actions, Reducers, Selectors for the Feature Flag Modal #5868

Merged

rileyajones force-pushed the feature-flags-modal branch 5 times, most recently from 8295782 to 1eee2a1 Compare August 17, 2022 22:38

rileyajones marked this pull request as ready for review August 18, 2022 17:07

rileyajones requested review from JamesHollyer, bmd3k and japie1235813 and removed request for JamesHollyer August 18, 2022 17:07

rileyajones force-pushed the feature-flags-modal branch 2 times, most recently from a4bbc17 to b0b43b3 Compare August 18, 2022 17:17

rileyajones commented Aug 18, 2022

View reviewed changes

tensorboard/webapp/tb_wrapper/tb_wrapper_component.ts Outdated Show resolved Hide resolved

JamesHollyer reviewed Aug 18, 2022

View reviewed changes

bmd3k reviewed Aug 18, 2022

View reviewed changes

tensorboard/webapp/feature_flag/views/feature_flag_module.ts Show resolved Hide resolved

rileyajones force-pushed the feature-flags-modal branch from 566656e to cc56f03 Compare August 18, 2022 22:51

rileyajones requested review from JamesHollyer and bmd3k August 18, 2022 22:53

bmd3k reviewed Aug 19, 2022

View reviewed changes

rileyajones force-pushed the feature-flags-modal branch from f43b878 to 935fc26 Compare August 22, 2022 23:29

rileyajones requested a review from bmd3k August 23, 2022 17:00

bmd3k reviewed Aug 24, 2022

View reviewed changes

rileyajones requested a review from bmd3k August 24, 2022 19:14

rileyajones force-pushed the feature-flags-modal branch from 243b8a0 to 79fb1d8 Compare August 24, 2022 22:22

bmd3k reviewed Aug 25, 2022

View reviewed changes

rileyajones force-pushed the feature-flags-modal branch from 0eecf96 to 9eff1bb Compare August 25, 2022 19:57

JamesHollyer reviewed Aug 25, 2022

View reviewed changes

tensorboard/webapp/feature_flag/views/feature_flag_page_component.ts Show resolved Hide resolved

bmd3k approved these changes Aug 25, 2022

View reviewed changes

rileyajones force-pushed the feature-flags-modal branch from 45f88fa to 27003ba Compare August 25, 2022 21:35

JamesHollyer approved these changes Aug 25, 2022

View reviewed changes

rileyajones added 10 commits August 26, 2022 08:45

Add new feature flag to control showing the feature flag modal, add m…

9e5b290

…odal to wrapper component to show flags page when the showFlags query parameter is set

address basic code review commends

0bb732a

move modal trigger to new component which is imported directly into a…

56a74bc

…pp_container

address pr comments

7d36b56

add tests

b802487

address pr comments

aee646e

fix and reorganize tests

f34c1b3

address pr comments, merge component/container tests

d1026d9

refactor test

31ff08c

tighten down test, rename function

4093f4b

rileyajones force-pushed the feature-flags-modal branch from 27003ba to 4093f4b Compare August 26, 2022 15:45

rileyajones merged commit 15602e9 into tensorflow:master Aug 26, 2022

Show flags page as modal when showFlags query parameter is set #5800

Show flags page as modal when showFlags query parameter is set #5800

Uh oh!

Conversation

rileyajones commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

Motivation for features / changes

Technical description of changes

Screenshots of UI changes

showFlags=true

showFlags not set

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmd3k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmd3k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesHollyer left a comment

Choose a reason for hiding this comment

Uh oh!

rileyajones commented Jul 11, 2022 •

edited

Loading

`showFlags=true`

`showFlags` not set