refactor: re-structure of page time range to be query param centric

projects/common/src/navigation/ht-route-data.ts

@@ -7,4 +7,5 @@ export interface HtRouteData {
   features?: string[];
   title?: string;
   defaultTimeRange?: TimeRange;
+  shouldSavePageTimeRange?: boolean;
 }

projects/common/src/time/page-time-range-preference-service.test.ts

@@ -21,9 +21,7 @@ describe('Page time range preference service', () => {
     service: PageTimeRangePreferenceService,
     providers: [
       mockProvider(NavigationService, {
-        getCurrentActivatedRoute: jest
-          .fn()
-          .mockReturnValue({ snapshot: { data: { defaultTimeRange: defaultPageTimeRange } } })
+        getRouteConfig: jest.fn().mockReturnValue({ data: { defaultTimeRange: defaultPageTimeRange } })
       }),
       mockProvider(FeatureStateResolver, {
         getFeatureState: jest.fn().mockReturnValue(of(FeatureState.Enabled))

projects/common/src/time/page-time-range-preference.service.ts

@@ -1,5 +1,4 @@
 import { Injectable } from '@angular/core';
-import { ActivatedRoute } from '@angular/router';
 import { isNil } from 'lodash-es';
 import { combineLatest, Observable } from 'rxjs';
 import { map, shareReplay, take } from 'rxjs/operators';
@@ -38,7 +37,7 @@ export class PageTimeRangePreferenceService {
       map(([pageTimeRangeStringDictionary, featureState]) => {
         if (featureState === FeatureState.Enabled) {
           if (isNil(pageTimeRangeStringDictionary[rootLevelPath])) {
-            return () => this.getDefaultTimeRangeForCurrentRoute();
+            return () => this.getDefaultTimeRangeForPath(rootLevelPath);
           }
 
           return () => this.timeRangeService.timeRangeFromUrlString(pageTimeRangeStringDictionary[rootLevelPath]);
@@ -78,11 +77,14 @@ export class PageTimeRangePreferenceService {
       .pipe(shareReplay(1));
   }
 
-  public getDefaultTimeRangeForCurrentRoute(): TimeRange {
-    const currentRoute: ActivatedRoute = this.navigationService.getCurrentActivatedRoute();
-    // Right side for when FF is enabled but 'defaultTimeRange' is not set on AR data
+  public getDefaultTimeRangeForPath(rootLevelPath: string): TimeRange {
+    const routeConfigForPath = this.navigationService.getRouteConfig(
+      [rootLevelPath],
+      this.navigationService.rootRoute()
+    );
 
-    return currentRoute.snapshot.data?.defaultTimeRange ?? this.getGlobalDefaultTimeRange();
+    // Right side for when FF is enabled but 'defaultTimeRange' is not set on AR data
+    return routeConfigForPath?.data?.defaultTimeRange ?? this.getGlobalDefaultTimeRange();
   }
 
   public getGlobalDefaultTimeRange(): TimeRange {

projects/common/src/time/time-range.service.test.ts

@@ -1,13 +1,13 @@
 import { ActivatedRoute, convertToParamMap } from '@angular/router';
-import { runFakeRxjs } from '@hypertrace/test-utils';
+import { recordObservable, runFakeRxjs } from '@hypertrace/test-utils';
 import { createServiceFactory, mockProvider } from '@ngneat/spectator/jest';
-import { NEVER, Observable, of } from 'rxjs';
+import { NEVER, Observable, of, Subject } from 'rxjs';
 import { map } from 'rxjs/operators';
-import { NavigationService } from '../navigation/navigation.service';
+import { NavigationService, QueryParamObject } from '../navigation/navigation.service';
 import { FixedTimeRange } from './fixed-time-range';
 import { TimeRangeService } from './time-range.service';
 
-describe('Time range service', () => {
+describe('Time range(TR) service', () => {
   let timeRange$: Observable<string> = NEVER;
   const buildService = createServiceFactory({
     service: TimeRangeService,
@@ -18,12 +18,19 @@ describe('Time range service', () => {
             map(
               initialTrString =>
                 // tslint:disable-next-line: no-object-literal-type-assertion
-                ({
-                  queryParamMap: of(convertToParamMap({ time: initialTrString }))
-                } as ActivatedRoute)
+                (({
+                  queryParamMap: of(convertToParamMap({ time: initialTrString, refresh: 'true' })),
+                  snapshot: { queryParamMap: convertToParamMap({ time: initialTrString, refresh: 'true' }) }
+                } as unknown) as ActivatedRoute)
             )
           );
-        }
+        },
+        getQueryParameter: jest
+          .fn()
+          .mockReturnValueOnce('1573255100253-1573255111159')
+          .mockReturnValue('1573255111159-1573455111990'),
+        getCurrentActivatedRoute: () =>
+          (({ snapshot: { queryParams: { time: 'test-value' } } } as unknown) as ActivatedRoute)
       })
     ]
   });
@@ -34,57 +41,120 @@ describe('Time range service', () => {
   });
 
   test('returns time range when requested after init', () => {
-    timeRange$ = of('1573255100253-1573255111159');
-    const spectator = buildService();
-    expect(spectator.service.getCurrentTimeRange()).toEqual(
-      new FixedTimeRange(new Date(1573255100253), new Date(1573255111159))
-    );
-  });
-
-  test('returns observable that emits future time range changes including initialization', () => {
-    const lateArrivingTimeRange = new FixedTimeRange(new Date(1573255111159), new Date(1573255111160));
     runFakeRxjs(({ cold, expectObservable }) => {
-      timeRange$ = cold('1ms x', {
+      timeRange$ = cold('x|', {
         x: '1573255100253-1573255111159'
       });
 
       const spectator = buildService();
+
       expect(() => spectator.service.getCurrentTimeRange()).toThrow();
 
-      cold('5ms x').subscribe(() =>
-        spectator.service.setFixedRange(lateArrivingTimeRange.startTime, lateArrivingTimeRange.endTime)
-      );
+      expectObservable(spectator.service.getTimeRangeAndChanges()).toBe('x', {
+        x: new FixedTimeRange(new Date(1573255100253), new Date(1573255111159))
+      });
+    });
+  });
 
-      expectObservable(spectator.service.getTimeRangeAndChanges()).toBe('1ms x 3ms y', {
+  test('returns observable that emits future time range changes including initialization', () => {
+    const firstArrivingTimeRange = new FixedTimeRange(new Date(1573255100253), new Date(1573255111159));
+    const secondArrivingTimeRange = new FixedTimeRange(new Date(1573255111159), new Date(1573455111990));
+
+    runFakeRxjs(({ cold, expectObservable }) => {
+      const spectator = buildService({
+        providers: [
+          mockProvider(NavigationService, {
+            navigation$: cold('-x---y', {
+              x: ({
+                queryParamMap: of(convertToParamMap({ time: firstArrivingTimeRange.toUrlString(), refresh: 'true' })),
+                snapshot: {
+                  queryParamMap: convertToParamMap({ time: firstArrivingTimeRange.toUrlString(), refresh: 'true' })
+                }
+              } as unknown) as ActivatedRoute,
+              y: ({
+                queryParamMap: of(convertToParamMap({ time: secondArrivingTimeRange.toUrlString(), refresh: 'true' })),
+                snapshot: {
+                  queryParamMap: convertToParamMap({ time: secondArrivingTimeRange.toUrlString(), refresh: 'true' })
+                }
+              } as unknown) as ActivatedRoute
+            }),
+            getQueryParameter: jest
+              .fn()
+              .mockReturnValueOnce(firstArrivingTimeRange.toUrlString())
+              .mockReturnValue(secondArrivingTimeRange.toUrlString()),
+            getCurrentActivatedRoute: () =>
+              (({ snapshot: { queryParams: { time: 'test-value' } } } as unknown) as ActivatedRoute)
+          })
+        ]
+      });
+
+      const recordedTimeRanges = recordObservable(spectator.service.getTimeRangeAndChanges());
+
+      expect(() => spectator.service.getCurrentTimeRange()).toThrow();
+
+      expectObservable(recordedTimeRanges).toBe('-x----y', {
         x: new FixedTimeRange(new Date(1573255100253), new Date(1573255111159)),
-        y: lateArrivingTimeRange
+        y: secondArrivingTimeRange
       });
     });
   });
 
-  test('returns observable that emits current time range and later changes', () => {
-    const lateArrivingTimeRange = new FixedTimeRange(new Date(1573255111159), new Date(1573255111160));
-    runFakeRxjs(({ cold, expectObservable }) => {
-      timeRange$ = of('1573255100253-1573255111159');
+  test('Emits default TR when set, then subsequent first and second TRs from query param changes', () => {
+    const defaultTimeRange = new FixedTimeRange(new Date(1573277100277), new Date(1573277100277));
+    const firstArrivingTimeRange = new FixedTimeRange(new Date(1573255100253), new Date(1573255111159));
+    const secondArrivingTimeRange = new FixedTimeRange(new Date(1573255111159), new Date(1573455111990));
+    const mockNavigation$ = new Subject();
+    runFakeRxjs(({ expectObservable, cold }) => {
+      const spectator = buildService({
+        providers: [
+          mockProvider(NavigationService, {
+            navigation$: mockNavigation$.asObservable().pipe(
+              map(
+                timeRangeString =>
+                  (({
+                    queryParamMap: of(convertToParamMap({ time: timeRangeString, refresh: 'true' })),
+                    snapshot: {
+                      queryParamMap: convertToParamMap({ time: timeRangeString, refresh: 'true' })
+                    }
+                  } as unknown) as ActivatedRoute)
+              )
+            ),
+            addQueryParametersToUrl: (newParams: QueryParamObject) => mockNavigation$.next(newParams.time as string),
+            getQueryParameter: jest
+              .fn()
+              .mockReturnValueOnce('1573277100277-1573277100277')
+              .mockReturnValueOnce('1573255100253-1573255111159')
+              .mockReturnValue('1573255111159-1573455111990'),
+            getCurrentActivatedRoute: () =>
+              (({ snapshot: { queryParams: { time: 'test-value' } } } as unknown) as ActivatedRoute),
+            replaceQueryParametersInUrl: jest.fn()
+          })
+        ]
+      });
 
-      const spectator = buildService();
-      expect(spectator.service.getCurrentTimeRange()).toEqual(
-        new FixedTimeRange(new Date(1573255100253), new Date(1573255111159))
+      cold('x').subscribe(() => spectator.service.setDefaultTimeRange(defaultTimeRange));
+
+      cold('2ms y').subscribe(() =>
+        spectator.service.setFixedRange(firstArrivingTimeRange.startTime, firstArrivingTimeRange.endTime)
       );
-      cold('5ms x').subscribe(() =>
-        spectator.service.setFixedRange(lateArrivingTimeRange.startTime, lateArrivingTimeRange.endTime)
+
+      cold('5ms z').subscribe(() =>
+        spectator.service.setFixedRange(secondArrivingTimeRange.startTime, secondArrivingTimeRange.endTime)
       );
 
-      expectObservable(spectator.service.getTimeRangeAndChanges()).toBe('x 4ms y', {
-        x: new FixedTimeRange(new Date(1573255100253), new Date(1573255111159)),
-        y: lateArrivingTimeRange
+      expectObservable(spectator.service.getTimeRangeAndChanges()).toBe('x 1ms y 2ms z', {
+        x: defaultTimeRange,
+        y: firstArrivingTimeRange,
+        z: secondArrivingTimeRange
       });
     });
   });
 
   test('returns custom time filter', () => {
     const spectator = buildService();
-    expect(spectator.service.toQueryParams(new Date(1642296703000), new Date(1642396703000))).toStrictEqual({
+    expect(
+      spectator.service.toQueryParams(new FixedTimeRange(new Date(1642296703000), new Date(1642396703000)))
+    ).toStrictEqual({
       ['time']: new FixedTimeRange(new Date(1642296703000), new Date(1642396703000)).toUrlString()
     });
   });

projects/common/src/time/time-range.service.ts

@@ -1,7 +1,8 @@
 import { Injectable } from '@angular/core';
-import { isEmpty, isNil } from 'lodash-es';
-import { EMPTY, ReplaySubject } from 'rxjs';
-import { catchError, filter, map, switchMap, take } from 'rxjs/operators';
+import { ParamMap } from '@angular/router';
+import { isEmpty, isNil, omit } from 'lodash-es';
+import { concat, EMPTY, Observable, ReplaySubject } from 'rxjs';
+import { catchError, distinctUntilChanged, filter, map, switchMap, take } from 'rxjs/operators';
 import { NavigationService, QueryParamObject } from '../navigation/navigation.service';
 import { ReplayObservable } from '../utilities/rxjs/rxjs-utils';
 import { FixedTimeRange } from './fixed-time-range';
@@ -16,6 +17,7 @@ import { TimeUnit } from './time-unit.type';
 })
 export class TimeRangeService {
   private static readonly TIME_RANGE_QUERY_PARAM: string = 'time';
+  private static readonly REFRESH_ON_NAVIGATION: string = 'refresh';
 
   private readonly timeRangeSubject$: ReplaySubject<TimeRange> = new ReplaySubject(1);
   private currentTimeRange?: TimeRange;
@@ -51,31 +53,65 @@ export class TimeRangeService {
   }
 
   public setRelativeRange(value: number, unit: TimeUnit): this {
-    return this.setTimeRange(TimeRangeService.toRelativeTimeRange(value, unit));
+    return this.setTimeRangeInUrl(TimeRangeService.toRelativeTimeRange(value, unit));
   }
 
   public setFixedRange(startTime: Date, endTime: Date): this {
-    return this.setTimeRange(TimeRangeService.toFixedTimeRange(startTime, endTime));
+    return this.setTimeRangeInUrl(TimeRangeService.toFixedTimeRange(startTime, endTime));
   }
 
   public refresh(): void {
-    this.setTimeRange(this.getCurrentTimeRange());
+    const currentStringTimeRange = this.getCurrentTimeRange().toUrlString();
+    this.applyTimeRangeChange(this.timeRangeFromUrlString(currentStringTimeRange));
+  }
+
+  private getInitialTimeRange(): Observable<TimeRange> {
+    return this.navigationService.navigation$.pipe(
+      take(1), // Wait for first navigation
+      switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it
+      take(1), // Only the first set of params
+      map(paramMap => paramMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM)), // Extract the time range value from it
+      filter((timeRangeString): timeRangeString is string => !isEmpty(timeRangeString)), // Only valid time ranges
+      map(timeRangeString => this.timeRangeFromUrlString(timeRangeString)),
+      catchError(() => EMPTY)
+    );
+  }
+
+  private getPageTimeRangeChanges(): Observable<TimeRange> {
+    return this.navigationService.navigation$.pipe(
+      map(activeRoute => activeRoute.snapshot.queryParamMap),
+      filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))),
+      distinctUntilChanged((prevParamMap, currParamMap) => this.shouldNotUpdateTimeRange(prevParamMap, currParamMap)),
+      map(currQueryParamMap => {
+        const timeRangeQueryParamString = currQueryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM);
+
+        return this.timeRangeFromUrlString(timeRangeQueryParamString!);
+      })
+    );
   }
 
   private initializeTimeRange(): void {
-    this.navigationService.navigation$
-      .pipe(
-        take(1), // Wait for first navigation
-        switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it
-        take(1), // Only the first set of params
-        map(paramMap => paramMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM)), // Extract the time range value from it
-        filter((timeRangeString): timeRangeString is string => !isEmpty(timeRangeString)), // Only valid time ranges
-        map(timeRangeString => this.timeRangeFromUrlString(timeRangeString)),
-        catchError(() => EMPTY)
-      )
-      .subscribe(timeRange => {
-        this.setTimeRange(timeRange);
-      });
+    concat(this.getInitialTimeRange(), this.getPageTimeRangeChanges()).subscribe(timeRange => {
+      if (!this.timeRangeMatchesCurrentUrl(timeRange)) {
+        this.setTimeRangeInUrl(timeRange);
+      } else {
+        this.applyTimeRangeChange(timeRange);
+
+        const queryParams = this.navigationService.getCurrentActivatedRoute().snapshot.queryParams;
+        if (TimeRangeService.REFRESH_ON_NAVIGATION in queryParams) {
+          this.navigationService.replaceQueryParametersInUrl(omit(queryParams, TimeRangeService.REFRESH_ON_NAVIGATION));
+        }
+      }
+    });
+  }
+
+  private shouldNotUpdateTimeRange(prevParamMap: ParamMap, currParamMap: ParamMap): boolean {
+    const refreshQueryParam = currParamMap.get(TimeRangeService.REFRESH_ON_NAVIGATION);
+
+    return (
+      prevParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM) ===
+        currParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM) && isEmpty(refreshQueryParam)
+    );
   }
 
   public timeRangeFromUrlString(timeRangeFromUrl: string): TimeRange {
@@ -91,19 +127,24 @@ export class TimeRangeService {
     throw new Error(); // Caught in observable
   }
 
-  private setTimeRange(newTimeRange: TimeRange): this {
+  private applyTimeRangeChange(newTimeRange: TimeRange): this {
     this.currentTimeRange = newTimeRange;
     this.timeRangeSubject$.next(newTimeRange);
+
+    return this;
+  }
+
+  private setTimeRangeInUrl(timeRange: TimeRange): this {
     this.navigationService.addQueryParametersToUrl({
-      [TimeRangeService.TIME_RANGE_QUERY_PARAM]: newTimeRange.toUrlString()
+      [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString()
     });
 
     return this;
   }
 
   public setDefaultTimeRange(timeRange: TimeRange): void {
     if (!this.currentTimeRange) {
-      this.setTimeRange(timeRange);
+      this.setTimeRangeInUrl(timeRange);
     }
   }
 
@@ -115,12 +156,22 @@ export class TimeRangeService {
     return new FixedTimeRange(startTime, endTime);
   }
 
-  public toQueryParams(startTime: Date, endTime: Date): QueryParamObject {
-    const newTimeRange = new FixedTimeRange(startTime, endTime);
-
-    return {
-      [TimeRangeService.TIME_RANGE_QUERY_PARAM]: newTimeRange.toUrlString()
+  public toQueryParams(timeRange: TimeRange, refreshTimeOnNavigationParam?: boolean): QueryParamObject {
+    const queryParams: QueryParamObject = {
+      [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString()
     };
+
+    if (refreshTimeOnNavigationParam) {
+      return { ...queryParams, [TimeRangeService.REFRESH_ON_NAVIGATION]: true };
+    }
+
+    return queryParams;
+  }
+
+  private timeRangeMatchesCurrentUrl(timeRange: TimeRange): boolean {
+    return (
+      this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, '') === timeRange.toUrlString()
+    );
   }
 
   public isInitialized(): boolean {