router: warn user if there are unsaved edits when navigating away (#5223)

yatbear · web-flow · commit 7bf7e510f309 · 2021-08-11T09:57:39.000-07:00
* warn user if there are unsaved edits when navigating away from the current route

* fire discardDirtyUpdates action when user wants to discard unsaved changes
diff --git a/tensorboard/webapp/app_routing/BUILD b/tensorboard/webapp/app_routing/BUILD
@@ -11,6 +11,7 @@ tf_ng_module(
     ],
     deps = [
         ":app_root",
+        ":dirty_updates_registry",
         ":location",
         ":programmatical_navigation_module",
         ":route_registry",
diff --git a/tensorboard/webapp/app_routing/actions/app_routing_actions.ts b/tensorboard/webapp/app_routing/actions/app_routing_actions.ts
@@ -19,6 +19,13 @@ import {Navigation, Route, RouteKind} from '../types';
 /** @typehack */ import * as _typeHackModels from '@ngrx/store/src/models';
 /** @typehack */ import * as _typeHackStore from '@ngrx/store';
 
+/**
+ * Created when user wants to discard unsaved edits and navigate away.
+ */
+export const discardDirtyUpdates = createAction(
+  '[App Routing] Discarding Unsaved Updates'
+);
+
 /**
  * Created when router rehydrates state from the URL after a browser initiated
  * event. Please do note that the action is fired before `navigated` so make
diff --git a/tensorboard/webapp/app_routing/app_routing_module.ts b/tensorboard/webapp/app_routing/app_routing_module.ts
@@ -18,6 +18,7 @@ import {StoreModule} from '@ngrx/store';
 
 import {AppRootModule} from './app_root_module';
 import {AppRoutingEffects} from './effects';
+import {DirtyUpdatesRegistryModule} from './dirty_updates_registry_module';
 import {LocationModule} from './location_module';
 import {ProgrammaticalNavigationModule} from './programmatical_navigation_module';
 import {RouteRegistryModule} from './route_registry_module';
@@ -32,6 +33,6 @@ import {APP_ROUTING_FEATURE_KEY} from './store/app_routing_types';
     AppRootModule,
     LocationModule,
   ],
-  providers: [ProgrammaticalNavigationModule],
+  providers: [DirtyUpdatesRegistryModule, ProgrammaticalNavigationModule],
 })
 export class AppRoutingModule {}
diff --git a/tensorboard/webapp/app_routing/effects/BUILD b/tensorboard/webapp/app_routing/effects/BUILD
@@ -13,6 +13,7 @@ tf_ng_module(
     deps = [
         "//tensorboard/webapp:app_state",
         "//tensorboard/webapp/app_routing:app_root",
+        "//tensorboard/webapp/app_routing:dirty_updates_registry",
         "//tensorboard/webapp/app_routing:internal_utils",
         "//tensorboard/webapp/app_routing:location",
         "//tensorboard/webapp/app_routing:programmatical_navigation_module",
@@ -38,6 +39,7 @@ tf_ng_module(
         "//tensorboard/webapp/angular:expect_angular_core_testing",
         "//tensorboard/webapp/angular:expect_ngrx_store_testing",
         "//tensorboard/webapp/app_routing:app_root",
+        "//tensorboard/webapp/app_routing:dirty_updates_registry",
         "//tensorboard/webapp/app_routing:location",
         "//tensorboard/webapp/app_routing:programmatical_navigation_module",
         "//tensorboard/webapp/app_routing:route_registry",
diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts
@@ -15,7 +15,7 @@ limitations under the License.
 import {Injectable} from '@angular/core';
 import {Actions, createEffect, ofType} from '@ngrx/effects';
 import {Action, createAction, Store} from '@ngrx/store';
-import {merge, Observable, of} from 'rxjs';
+import {combineLatest, merge, Observable, of} from 'rxjs';
 import {
   debounceTime,
   delay,
@@ -28,6 +28,7 @@ import {
 
 import {State} from '../../app_state';
 import {
+  discardDirtyUpdates,
   navigated,
   navigating,
   navigationRequested,
@@ -40,6 +41,7 @@ import {
   serializeCompareExperimentParams,
 } from '../internal_utils';
 import {Location} from '../location';
+import {DirtyUpdatesRegistryModule} from '../dirty_updates_registry_module';
 import {ProgrammaticalNavigationModule} from '../programmatical_navigation_module';
 import {RouteConfigs} from '../route_config';
 import {RouteRegistryModule} from '../route_registry_module';
@@ -64,6 +66,7 @@ export class AppRoutingEffects {
     private readonly actions$: Actions,
     private readonly store: Store<State>,
     private readonly location: Location,
+    private readonly dirtyUpdatesRegistry: DirtyUpdatesRegistryModule<State>,
     registry: RouteRegistryModule,
     private readonly programmaticalNavModule: ProgrammaticalNavigationModule,
     private readonly appRootProvider: AppRootProvider
@@ -198,8 +201,32 @@ export class AppRoutingEffects {
    * @export
    */
   navigate$ = createEffect(() => {
+    const hasDirtyUpdates$ = combineLatest(
+      this.dirtyUpdatesRegistry
+        .getDirtyUpdatesSelectors()
+        .map((selector) => this.store.select(selector))
+    ).pipe(
+      map(
+        (updates) =>
+          updates[0].experimentIds !== undefined &&
+          updates[0].experimentIds.length > 0
+      )
+    );
     const dispatchNavigating$ = this.validatedRoute$.pipe(
-      tap(({routeMatch, options}) => {
+      withLatestFrom(hasDirtyUpdates$),
+      filter(([, hasDirtyUpdates]) => {
+        if (hasDirtyUpdates) {
+          const discardChanges = window.confirm(
+            `You have unsaved edits, are you sure you want to discard them?`
+          );
+          if (discardChanges) {
+            this.store.dispatch(discardDirtyUpdates());
+          }
+          return discardChanges;
+        }
+        return true;
+      }),
+      tap(([{routeMatch, options}]) => {
         if (options.browserInitiated && routeMatch.deepLinkProvider) {
           // Query paramter formed by the redirector is passed to the
           // deserializer instead of one from Location.getSearch(). This
@@ -223,7 +250,7 @@ export class AppRoutingEffects {
           );
         }
       }),
-      switchMap(({routeMatch, options}) => {
+      switchMap(([{routeMatch, options}]) => {
         const navigationOptions = {
           replaceState: options.replaceState ?? false,
         };
diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts
@@ -16,15 +16,15 @@ limitations under the License.
 import {Component} from '@angular/core';
 import {fakeAsync, flush, TestBed, tick} from '@angular/core/testing';
 import {provideMockActions} from '@ngrx/effects/testing';
-import {Action, createAction, props, Store} from '@ngrx/store';
+import {Action, createAction, createSelector, props, Store} from '@ngrx/store';
 import {MockStore, provideMockStore} from '@ngrx/store/testing';
 import {of, ReplaySubject} from 'rxjs';
 
 import {State} from '../../app_state';
 import * as actions from '../actions';
-import {navigationRequested} from '../actions';
 import {AppRootProvider, TestableAppRootProvider} from '../app_root';
 import {Location} from '../location';
+import {DirtyUpdatesRegistryModule} from '../dirty_updates_registry_module';
 import {
   NavigateToCompare,
   NavigateToExperiments,
@@ -33,7 +33,13 @@ import {
 import {RouteRegistryModule} from '../route_registry_module';
 import {getActiveRoute} from '../store/app_routing_selectors';
 import {buildRoute, provideLocationTesting, TestableLocation} from '../testing';
-import {Navigation, Route, RouteKind, SerializableQueryParams} from '../types';
+import {
+  DirtyUpdates,
+  Navigation,
+  Route,
+  RouteKind,
+  SerializableQueryParams,
+} from '../types';
 
 import {AppRoutingEffects, TEST_ONLY} from './app_routing_effects';
 
@@ -45,6 +51,14 @@ const testNavToCompareAction = createAction(
   '[TEST] test nav to compare',
   props<NavigateToCompare['routeParams']>()
 );
+const testDirtyExperimentsSelector = createSelector(
+  (s) => s,
+  (state: any) => {
+    return {
+      experimentIds: [],
+    } as DirtyUpdates;
+  }
+);
 
 describe('app_routing_effects', () => {
   let effects: AppRoutingEffects;
@@ -138,6 +152,10 @@ describe('app_routing_effects', () => {
       };
     }
 
+    function dirtyUpdatesFactory() {
+      return testDirtyExperimentsSelector;
+    }
+
     await TestBed.configureTestingModule({
       imports: [
         RouteRegistryModule.registerRoutes(routeFactory),
@@ -147,6 +165,9 @@ describe('app_routing_effects', () => {
         ProgrammaticalNavigationModule.registerProgrammaticalNavigation(
           programmaticalCompareNavigationFactory
         ),
+        DirtyUpdatesRegistryModule.registerDirtyUpdates<State>(
+          dirtyUpdatesFactory
+        ),
       ],
       providers: [
         provideMockActions(action),
@@ -157,6 +178,7 @@ describe('app_routing_effects', () => {
           provide: AppRootProvider,
           useClass: TestableAppRootProvider,
         },
+        DirtyUpdatesRegistryModule,
       ],
     }).compileComponents();
 
@@ -255,6 +277,108 @@ describe('app_routing_effects', () => {
       ]);
     });
 
+    describe('dispatchNavigating$ with dirty updates', () => {
+      beforeEach(() => {
+        const activeRoute = buildRoute({
+          routeKind: RouteKind.EXPERIMENTS,
+          pathname: '/experiments',
+          queryParams: [],
+          navigationOptions: {
+            replaceState: false,
+          },
+        });
+        const dirtyExperimentsFactory = () => {
+          return {experimentIds: ['otter']};
+        };
+
+        store.overrideSelector(getActiveRoute, activeRoute);
+        store.overrideSelector(
+          testDirtyExperimentsSelector,
+          dirtyExperimentsFactory()
+        );
+        store.refreshState();
+      });
+
+      it('warns user when navigating away', () => {
+        spyOn(window, 'confirm');
+        action.next(
+          actions.navigationRequested({
+            pathname: '/experiment/meow',
+          })
+        );
+
+        expect(window.confirm).toHaveBeenCalledWith(
+          'You have unsaved edits, are you sure you want to discard them?'
+        );
+      });
+
+      it('noops if user cancels navigation', () => {
+        spyOn(window, 'confirm').and.returnValue(false);
+        action.next(
+          actions.navigationRequested({
+            pathname: '/experiment/meow',
+          })
+        );
+
+        expect(window.confirm).toHaveBeenCalledTimes(1);
+        expect(actualActions).toEqual([]);
+      });
+
+      it(
+        'fires discardDirtyUpdates, navigating and navigated if user ' +
+          'discards dirty updates',
+        fakeAsync(() => {
+          spyOn(window, 'confirm').and.returnValue(true);
+          action.next(
+            actions.navigationRequested({
+              pathname: '/experiment/meow',
+            })
+          );
+
+          expect(window.confirm).toHaveBeenCalledTimes(1);
+
+          expect(actualActions).toEqual([
+            actions.discardDirtyUpdates(),
+            actions.navigating({
+              after: buildRoute({
+                routeKind: RouteKind.EXPERIMENT,
+                params: {experimentId: 'meow'},
+                pathname: '/experiment/meow',
+                queryParams: [],
+              }),
+            }),
+          ]);
+
+          tick();
+          expect(actualActions).toEqual([
+            actions.discardDirtyUpdates(),
+            actions.navigating({
+              after: buildRoute({
+                routeKind: RouteKind.EXPERIMENT,
+                params: {experimentId: 'meow'},
+                pathname: '/experiment/meow',
+                queryParams: [],
+              }),
+            }),
+            actions.navigated({
+              before: buildRoute({
+                routeKind: RouteKind.EXPERIMENTS,
+                params: {},
+                pathname: '/experiments',
+                queryParams: [],
+              }),
+              after: buildRoute({
+                routeKind: RouteKind.EXPERIMENT,
+                params: {experimentId: 'meow'},
+                pathname: '/experiment/meow',
+                queryParams: [],
+              }),
+            }),
+          ]);
+        })
+      );
+    });
+
     describe('order of events', () => {
       it(
         'dispatches navigating, waits (for UI to clear prev route page), ' +
@@ -1019,7 +1143,7 @@ describe('app_routing_effects', () => {
       setAppRootAndSubscribe('/foo/bar/');
 
       // Do note that this path name does not contain the appRoot.
-      action.next(navigationRequested({pathname: '/experiment/123'}));
+      action.next(actions.navigationRequested({pathname: '/experiment/123'}));
 
       expect(actualActions).toEqual([
         actions.navigating({
@@ -1046,7 +1170,7 @@ describe('app_routing_effects', () => {
         .and.returnValue('/foo/bar/experiment/123');
 
       // Do note that this path name does not contain the appRoot.
-      action.next(navigationRequested({pathname: '../experiment/123'}));
+      action.next(actions.navigationRequested({pathname: '../experiment/123'}));
 
       expect(actualActions).toEqual([
         actions.navigating({
