From 1de4d4d4b59534c8e7434a984dd051e47670b127 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Tue, 29 Mar 2022 15:27:14 -0300 Subject: [PATCH 01/14] refactor: re-structure of page time range to be query param centric --- .../common/src/navigation/ht-route-data.ts | 1 + .../page-time-range-preference.service.ts | 14 +++-- .../common/src/time/time-range.service.ts | 63 +++++++++++++++---- .../navigation/nav-item/nav-item.component.ts | 27 +++++--- .../navigation/navigation-list.component.ts | 4 -- .../page-time-range.component.ts | 2 +- src/app/routes/root-routing.module.ts | 18 ++++-- .../shared/navigation/navigation.component.ts | 13 +--- 8 files changed, 93 insertions(+), 49 deletions(-) diff --git a/projects/common/src/navigation/ht-route-data.ts b/projects/common/src/navigation/ht-route-data.ts index 72af980d3..5365c6569 100644 --- a/projects/common/src/navigation/ht-route-data.ts +++ b/projects/common/src/navigation/ht-route-data.ts @@ -7,4 +7,5 @@ export interface HtRouteData { features?: string[]; title?: string; defaultTimeRange?: TimeRange; + shouldSavePageTimeRange?: boolean; } diff --git a/projects/common/src/time/page-time-range-preference.service.ts b/projects/common/src/time/page-time-range-preference.service.ts index 9978b4f42..7dd82ad02 100644 --- a/projects/common/src/time/page-time-range-preference.service.ts +++ b/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.getDefaultTimeRangeForCurrentRoute(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 getDefaultTimeRangeForCurrentRoute(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 { diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 95195734b..e40cf281a 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,7 +1,10 @@ import { Injectable } from '@angular/core'; import { isEmpty, isNil } from 'lodash-es'; -import { EMPTY, ReplaySubject } from 'rxjs'; +import { EMPTY, Observable, ReplaySubject } from 'rxjs'; import { catchError, filter, map, switchMap, take } from 'rxjs/operators'; +import { ApplicationFeature } from '../constants/application-constants'; +import { FeatureStateResolver } from '../feature/state/feature-state.resolver'; +import { FeatureState } from '../feature/state/feature.state'; import { NavigationService, QueryParamObject } from '../navigation/navigation.service'; import { ReplayObservable } from '../utilities/rxjs/rxjs-utils'; import { FixedTimeRange } from './fixed-time-range'; @@ -22,7 +25,8 @@ export class TimeRangeService { public constructor( private readonly navigationService: NavigationService, - private readonly timeDurationService: TimeDurationService + private readonly timeDurationService: TimeDurationService, + private readonly featureStateResolver: FeatureStateResolver ) { this.initializeTimeRange(); this.navigationService.registerGlobalQueryParamKey(TimeRangeService.TIME_RANGE_QUERY_PARAM); @@ -62,16 +66,44 @@ export class TimeRangeService { this.setTimeRange(this.getCurrentTimeRange()); } + private globalTimeRangeInitialization(): Observable { + 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 pageTimeRangeInitialization(): Observable { + return this.navigationService.navigation$.pipe( + filter( + activatedRoute => !isNil(activatedRoute.snapshot.queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM)) + ), + map(activeRoute => { + const timeRangeQueryParamString = activeRoute.snapshot.queryParamMap.get( + TimeRangeService.TIME_RANGE_QUERY_PARAM + ); + + return this.timeRangeFromUrlString(timeRangeQueryParamString!); + }) + ); + } + private initializeTimeRange(): void { - this.navigationService.navigation$ + this.featureStateResolver + .getFeatureState(ApplicationFeature.PageTimeRange) .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) + switchMap(featureState => { + if (featureState === FeatureState.Enabled) { + return this.pageTimeRangeInitialization(); + } + + return this.globalTimeRangeInitialization(); + }) ) .subscribe(timeRange => { this.setTimeRange(timeRange); @@ -115,11 +147,16 @@ export class TimeRangeService { return new FixedTimeRange(startTime, endTime); } - public toQueryParams(startTime: Date, endTime: Date): QueryParamObject { - const newTimeRange = new FixedTimeRange(startTime, endTime); + public toQueryParams(startTime: Date, endTime: Date, timeRangeIfRelative?: TimeRange): QueryParamObject { + if (timeRangeIfRelative && timeRangeIfRelative instanceof RelativeTimeRange) { + return { + [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeIfRelative.toUrlString() + }; + } + const newFixedTimeRange = new FixedTimeRange(startTime, endTime); return { - [TimeRangeService.TIME_RANGE_QUERY_PARAM]: newTimeRange.toUrlString() + [TimeRangeService.TIME_RANGE_QUERY_PARAM]: newFixedTimeRange.toUrlString() }; } diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index f201a573c..4f56d0a0c 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -1,6 +1,6 @@ import { ChangeDetectionStrategy, Component, Input } from '@angular/core'; import { ActivatedRoute } from '@angular/router'; -import { FeatureState, NavigationParams, NavigationParamsType } from '@hypertrace/common'; +import { FeatureState, NavigationParams, NavigationParamsType, TimeRangeService } from '@hypertrace/common'; import { IconSize } from '../../icon/icon-size'; import { NavItemLinkConfig, NavViewStyle } from '../navigation.config'; @@ -57,12 +57,23 @@ export class NavItemComponent { @Input() public readonly navItemViewStyle?: NavViewStyle; - public buildNavigationParam = (item: NavItemLinkConfig): NavigationParams => ({ - navType: NavigationParamsType.InApp, - path: item.matchPaths[0], - relativeTo: this.activatedRoute, - replaceCurrentHistory: item.replaceCurrentHistory - }); + public buildNavigationParam = (item: NavItemLinkConfig): NavigationParams => { + const timeRange = this.config.timeRangeResolver ? this.config.timeRangeResolver() : undefined; - public constructor(private readonly activatedRoute: ActivatedRoute) {} + return { + navType: NavigationParamsType.InApp, + path: item.matchPaths[0], + relativeTo: this.activatedRoute, + replaceCurrentHistory: item.replaceCurrentHistory, + queryParams: + timeRange && this.config.pageLevelTimeRangeIsEnabled + ? this.timeRangeService.toQueryParams(timeRange.startTime, timeRange.endTime, timeRange) + : undefined + }; + }; + + public constructor( + private readonly activatedRoute: ActivatedRoute, + private readonly timeRangeService: TimeRangeService + ) {} } diff --git a/projects/components/src/navigation/navigation-list.component.ts b/projects/components/src/navigation/navigation-list.component.ts index ba7238b5a..ab88b3d6f 100644 --- a/projects/components/src/navigation/navigation-list.component.ts +++ b/projects/components/src/navigation/navigation-list.component.ts @@ -39,7 +39,6 @@ import { = new EventEmitter(); - @Output() public readonly activeItemChange: EventEmitter = new EventEmitter(); diff --git a/projects/components/src/page-time-range/page-time-range.component.ts b/projects/components/src/page-time-range/page-time-range.component.ts index d416b2f97..a5fccfa90 100644 --- a/projects/components/src/page-time-range/page-time-range.component.ts +++ b/projects/components/src/page-time-range/page-time-range.component.ts @@ -35,7 +35,7 @@ export class PageTimeRangeComponent { } public shouldSavePageTimeRange(currentRoute: ActivatedRoute): boolean { - return !isNil(currentRoute.snapshot.data?.defaultTimeRange); + return !isNil(currentRoute.snapshot.data?.shouldSavePageTimeRange); } public savePageTimeRange(selectedTimeRange: TimeRange, segment: UrlSegment): void { diff --git a/src/app/routes/root-routing.module.ts b/src/app/routes/root-routing.module.ts index d5500aa18..bb6fc4b5d 100644 --- a/src/app/routes/root-routing.module.ts +++ b/src/app/routes/root-routing.module.ts @@ -26,7 +26,8 @@ const ROUTE_CONFIG: HtRoute[] = [ icon: IconType.Dashboard, label: 'Dashboard' }, - defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)) + defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)), + shouldSavePageTimeRange: true }, loadChildren: () => import('../home/home.module').then(module => module.HomeModule) }, @@ -37,7 +38,8 @@ const ROUTE_CONFIG: HtRoute[] = [ icon: ObservabilityIconType.ApplicationFlow, label: 'Application Flow' }, - defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)) + defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)), + shouldSavePageTimeRange: true }, loadChildren: () => import('./application-flow/application-flow-routing.module').then( @@ -51,7 +53,8 @@ const ROUTE_CONFIG: HtRoute[] = [ icon: ObservabilityIconType.Backend, label: 'Backends' }, - defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)) + defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)), + shouldSavePageTimeRange: true }, loadChildren: () => import('./backends/backends-routing.module').then(module => module.BackendsRoutingModule) @@ -63,7 +66,8 @@ const ROUTE_CONFIG: HtRoute[] = [ icon: ObservabilityIconType.Service, label: 'Services' }, - defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)) + defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)), + shouldSavePageTimeRange: true }, loadChildren: () => import('./services/services-routing.module').then(module => module.ServicesRoutingModule) @@ -75,7 +79,8 @@ const ROUTE_CONFIG: HtRoute[] = [ icon: ObservabilityIconType.Api, label: 'Endpoints' }, - defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)) + defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)), + shouldSavePageTimeRange: true }, loadChildren: () => import('./endpoints/endpoint-routing.module').then(module => module.EndpointRoutingModule) @@ -95,7 +100,8 @@ const ROUTE_CONFIG: HtRoute[] = [ icon: IconType.Search, label: 'Explorer' }, - defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)) + defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)), + shouldSavePageTimeRange: true }, loadChildren: () => import('./explorer/explorer-routing.module').then(module => module.ExplorerRoutingModule) diff --git a/src/app/shared/navigation/navigation.component.ts b/src/app/shared/navigation/navigation.component.ts index eabe574a0..2c8a7ba2e 100644 --- a/src/app/shared/navigation/navigation.component.ts +++ b/src/app/shared/navigation/navigation.component.ts @@ -24,8 +24,7 @@ import { Observable } from 'rxjs'; *htLetAsync="this.isCollapsed$ as isCollapsed" [collapsed]="isCollapsed" (collapsedChange)="this.onViewToggle($event)" - (navItemClick)="this.setPageTimeRangeForSelectedNavItem($event)" - (activeItemChange)="this.updateDefaultTimeRangeIfUnset($event)" + (activeItemChange)="this.setPageTimeRangeForActiveNavItem($event)" > ` @@ -101,7 +100,7 @@ export class NavigationComponent { this.isCollapsed$ = this.preferenceService.get(NavigationComponent.COLLAPSED_PREFERENCE, false); } - public setPageTimeRangeForSelectedNavItem(navItemLink: NavItemLinkConfig): void { + public setPageTimeRangeForActiveNavItem(navItemLink: NavItemLinkConfig): void { if (!isNil(navItemLink.timeRangeResolver) && navItemLink.pageLevelTimeRangeIsEnabled) { const timeRange = navItemLink.timeRangeResolver(); if (timeRange instanceof FixedTimeRange) { @@ -112,14 +111,6 @@ export class NavigationComponent { } } - public updateDefaultTimeRangeIfUnset(activeItem: NavItemLinkConfig): void { - // Initialize the time range service - // Depending on FF status, the TR will be either global or page level for the init - if (!this.timeRangeService.isInitialized()) { - this.timeRangeService.setDefaultTimeRange(activeItem.timeRangeResolver!()); - } - } - public onViewToggle(collapsed: boolean): void { this.preferenceService.set(NavigationComponent.COLLAPSED_PREFERENCE, collapsed); } From 90aca7d03ef69deed19850b460aeea0340cf161d Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Tue, 29 Mar 2022 16:57:03 -0300 Subject: [PATCH 02/14] test: support for recent changes --- .../page-time-range-preference-service.test.ts | 4 +--- .../common/src/time/time-range.service.test.ts | 10 +++++++--- .../nav-item/nav-item.component.test.ts | 3 ++- .../page-time-range/page-time-range.test.ts | 10 +++++----- .../explore-visualization-builder.test.ts | 13 ++++++++++++- .../shared/navigation/navigation.component.ts | 18 +++++++----------- 6 files changed, 34 insertions(+), 24 deletions(-) diff --git a/projects/common/src/time/page-time-range-preference-service.test.ts b/projects/common/src/time/page-time-range-preference-service.test.ts index d9f4f4b17..55ca18ae1 100644 --- a/projects/common/src/time/page-time-range-preference-service.test.ts +++ b/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)) diff --git a/projects/common/src/time/time-range.service.test.ts b/projects/common/src/time/time-range.service.test.ts index f6b37c48e..51771ea31 100644 --- a/projects/common/src/time/time-range.service.test.ts +++ b/projects/common/src/time/time-range.service.test.ts @@ -1,4 +1,5 @@ import { ActivatedRoute, convertToParamMap } from '@angular/router'; +import { FeatureState, FeatureStateResolver } from '@hypertrace/common'; import { runFakeRxjs } from '@hypertrace/test-utils'; import { createServiceFactory, mockProvider } from '@ngneat/spectator/jest'; import { NEVER, Observable, of } from 'rxjs'; @@ -18,12 +19,15 @@ describe('Time range service', () => { map( initialTrString => // tslint:disable-next-line: no-object-literal-type-assertion - ({ - queryParamMap: of(convertToParamMap({ time: initialTrString })) - } as ActivatedRoute) + (({ + snapshot: { queryParamMap: convertToParamMap({ time: initialTrString }) } + } as unknown) as ActivatedRoute) ) ); } + }), + mockProvider(FeatureStateResolver, { + getFeatureState: () => of(FeatureState.Enabled) }) ] }); diff --git a/projects/components/src/navigation/nav-item/nav-item.component.test.ts b/projects/components/src/navigation/nav-item/nav-item.component.test.ts index ed04911d1..c9ac3bcd9 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.test.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.test.ts @@ -33,7 +33,8 @@ describe('Navigation Item Component', () => { getCurrentActivatedRoute: jest.fn().mockReturnValue(of(activatedRoute)) }), mockProvider(FeatureStateResolver, { - getCombinedFeatureState: () => of(FeatureState.Enabled) + getCombinedFeatureState: () => of(FeatureState.Enabled), + getFeatureState: () => of(FeatureState.Enabled) }) ] }); diff --git a/projects/components/src/page-time-range/page-time-range.test.ts b/projects/components/src/page-time-range/page-time-range.test.ts index ff1160af0..fbf8c79df 100644 --- a/projects/components/src/page-time-range/page-time-range.test.ts +++ b/projects/components/src/page-time-range/page-time-range.test.ts @@ -18,7 +18,7 @@ describe('Page time range component', () => { const route = { snapshot: { data: { - defaultTimeRange: undefined + shouldSavePageTimeRange: undefined } }, pathFromRoot: { flatMap: jest.fn().mockReturnValue(['foo']) } @@ -52,14 +52,14 @@ describe('Page time range component', () => { expect(spectator.component.savePageTimeRange).not.toHaveBeenCalled(); }); - test('should not attempt to save time range when route is not a first level route', () => { + test('should not attempt to save time range when saving flag property is not set', () => { spectator = createHost(``, { providers: [ mockProvider(NavigationService, { getCurrentActivatedRoute: jest.fn().mockReturnValue({ snapshot: { data: { - defaultTimeRange: undefined + shouldSavePageTimeRange: undefined } }, pathFromRoot: { flatMap: jest.fn().mockReturnValue(['parent-path', 'child-path']) } @@ -77,14 +77,14 @@ describe('Page time range component', () => { expect(spectator.component.savePageTimeRange).not.toHaveBeenCalled(); }); - test('should save time range when route is first level, and the defaultTimeRange property is present', () => { + test('should save time range when route has truthy property shouldSavePageTimeRange', () => { spectator = createHost(``, { providers: [ mockProvider(NavigationService, { getCurrentActivatedRoute: jest.fn().mockReturnValue({ snapshot: { data: { - defaultTimeRange: new RelativeTimeRange(new TimeDuration(30, TimeUnit.Minute)) + shouldSavePageTimeRange: true } }, pathFromRoot: { flatMap: () => [{ path: 'parent-path' }] as UrlSegment[] } diff --git a/projects/observability/src/shared/components/explore-query-editor/explore-visualization-builder.test.ts b/projects/observability/src/shared/components/explore-query-editor/explore-visualization-builder.test.ts index d46c5bd84..74570a11d 100644 --- a/projects/observability/src/shared/components/explore-query-editor/explore-visualization-builder.test.ts +++ b/projects/observability/src/shared/components/explore-query-editor/explore-visualization-builder.test.ts @@ -1,6 +1,14 @@ import { fakeAsync, tick } from '@angular/core/testing'; import { RouterTestingModule } from '@angular/router/testing'; -import { FixedTimeRange, IntervalDurationService, TimeDuration, TimeRangeService, TimeUnit } from '@hypertrace/common'; +import { + FeatureState, + FeatureStateResolver, + FixedTimeRange, + IntervalDurationService, + TimeDuration, + TimeRangeService, + TimeUnit +} from '@hypertrace/common'; import { patchRouterNavigateForTest, recordObservable, runFakeRxjs } from '@hypertrace/test-utils'; import { createServiceFactory, mockProvider, SpectatorService } from '@ngneat/spectator/jest'; import { of } from 'rxjs'; @@ -27,6 +35,9 @@ describe('Explore visualization builder', () => { }), mockProvider(IntervalDurationService, { getAutoDuration: () => new TimeDuration(3, TimeUnit.Minute) + }), + mockProvider(FeatureStateResolver, { + getFeatureState: () => of(FeatureState.Enabled) }) ] }); diff --git a/src/app/shared/navigation/navigation.component.ts b/src/app/shared/navigation/navigation.component.ts index 2c8a7ba2e..b309a2a09 100644 --- a/src/app/shared/navigation/navigation.component.ts +++ b/src/app/shared/navigation/navigation.component.ts @@ -1,7 +1,7 @@ import { ChangeDetectionStrategy, Component } from '@angular/core'; import { ActivatedRoute } from '@angular/router'; import { IconType } from '@hypertrace/assets-library'; -import { FixedTimeRange, PreferenceService, RelativeTimeRange, TimeRangeService } from '@hypertrace/common'; +import { PreferenceService, TimeRangeService } from '@hypertrace/common'; import { NavigationListComponentService, NavigationListService, @@ -10,7 +10,6 @@ import { NavItemType } from '@hypertrace/components'; import { ObservabilityIconType } from '@hypertrace/observability'; -import { isNil } from 'lodash-es'; import { Observable } from 'rxjs'; @Component({ @@ -24,7 +23,7 @@ import { Observable } from 'rxjs'; *htLetAsync="this.isCollapsed$ as isCollapsed" [collapsed]="isCollapsed" (collapsedChange)="this.onViewToggle($event)" - (activeItemChange)="this.setPageTimeRangeForActiveNavItem($event)" + (activeItemChange)="this.updateDefaultTimeRangeIfUnset($event)" > ` @@ -100,14 +99,11 @@ export class NavigationComponent { this.isCollapsed$ = this.preferenceService.get(NavigationComponent.COLLAPSED_PREFERENCE, false); } - public setPageTimeRangeForActiveNavItem(navItemLink: NavItemLinkConfig): void { - if (!isNil(navItemLink.timeRangeResolver) && navItemLink.pageLevelTimeRangeIsEnabled) { - const timeRange = navItemLink.timeRangeResolver(); - if (timeRange instanceof FixedTimeRange) { - this.timeRangeService.setFixedRange(timeRange.startTime, timeRange.endTime); - } else if (timeRange instanceof RelativeTimeRange) { - this.timeRangeService.setRelativeRange(timeRange.duration.value, timeRange.duration.unit); - } + public updateDefaultTimeRangeIfUnset(activeItem: NavItemLinkConfig): void { + // Initialize the time range service + // Depending on FF status, the TR will be either global or page level for the init + if (!this.timeRangeService.isInitialized()) { + this.timeRangeService.setDefaultTimeRange(activeItem.timeRangeResolver!()); } } From 40f55da7f16cfa7ec24a404278b9097bada5d9f6 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Tue, 29 Mar 2022 19:35:53 -0300 Subject: [PATCH 03/14] refactor: requested changes --- .../page-time-range-preference.service.ts | 4 +-- .../src/time/time-range.service.test.ts | 4 ++- .../common/src/time/time-range.service.ts | 30 +++++++------------ .../navigation/nav-item/nav-item.component.ts | 20 ++++++++----- .../page-time-range.component.ts | 2 +- .../cartesian-explorer-navigation.service.ts | 4 +-- 6 files changed, 30 insertions(+), 34 deletions(-) diff --git a/projects/common/src/time/page-time-range-preference.service.ts b/projects/common/src/time/page-time-range-preference.service.ts index 7dd82ad02..a30aae325 100644 --- a/projects/common/src/time/page-time-range-preference.service.ts +++ b/projects/common/src/time/page-time-range-preference.service.ts @@ -37,7 +37,7 @@ export class PageTimeRangePreferenceService { map(([pageTimeRangeStringDictionary, featureState]) => { if (featureState === FeatureState.Enabled) { if (isNil(pageTimeRangeStringDictionary[rootLevelPath])) { - return () => this.getDefaultTimeRangeForCurrentRoute(rootLevelPath); + return () => this.getDefaultTimeRangeForPath(rootLevelPath); } return () => this.timeRangeService.timeRangeFromUrlString(pageTimeRangeStringDictionary[rootLevelPath]); @@ -77,7 +77,7 @@ export class PageTimeRangePreferenceService { .pipe(shareReplay(1)); } - public getDefaultTimeRangeForCurrentRoute(rootLevelPath: string): TimeRange { + public getDefaultTimeRangeForPath(rootLevelPath: string): TimeRange { const routeConfigForPath = this.navigationService.getRouteConfig( [rootLevelPath], this.navigationService.rootRoute() diff --git a/projects/common/src/time/time-range.service.test.ts b/projects/common/src/time/time-range.service.test.ts index 51771ea31..ca45765f5 100644 --- a/projects/common/src/time/time-range.service.test.ts +++ b/projects/common/src/time/time-range.service.test.ts @@ -88,7 +88,9 @@ describe('Time range service', () => { 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() }); }); diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index e40cf281a..7929931ea 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -66,7 +66,7 @@ export class TimeRangeService { this.setTimeRange(this.getCurrentTimeRange()); } - private globalTimeRangeInitialization(): Observable { + private globalTimeRangeInitConfigAndChanges(): Observable { return this.navigationService.navigation$.pipe( take(1), // Wait for first navigation switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it @@ -78,15 +78,12 @@ export class TimeRangeService { ); } - private pageTimeRangeInitialization(): Observable { + private pageTimeRangeInitConfigAndChanges(): Observable { return this.navigationService.navigation$.pipe( - filter( - activatedRoute => !isNil(activatedRoute.snapshot.queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM)) - ), - map(activeRoute => { - const timeRangeQueryParamString = activeRoute.snapshot.queryParamMap.get( - TimeRangeService.TIME_RANGE_QUERY_PARAM - ); + switchMap(activeRoute => activeRoute.queryParamMap), + filter(queryParmaMap => !isNil(queryParmaMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))), + map(queryParmaMap => { + const timeRangeQueryParamString = queryParmaMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM); return this.timeRangeFromUrlString(timeRangeQueryParamString!); }) @@ -99,10 +96,10 @@ export class TimeRangeService { .pipe( switchMap(featureState => { if (featureState === FeatureState.Enabled) { - return this.pageTimeRangeInitialization(); + return this.pageTimeRangeInitConfigAndChanges(); } - return this.globalTimeRangeInitialization(); + return this.globalTimeRangeInitConfigAndChanges(); }) ) .subscribe(timeRange => { @@ -147,16 +144,9 @@ export class TimeRangeService { return new FixedTimeRange(startTime, endTime); } - public toQueryParams(startTime: Date, endTime: Date, timeRangeIfRelative?: TimeRange): QueryParamObject { - if (timeRangeIfRelative && timeRangeIfRelative instanceof RelativeTimeRange) { - return { - [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeIfRelative.toUrlString() - }; - } - const newFixedTimeRange = new FixedTimeRange(startTime, endTime); - + public toQueryParams(timeRange: TimeRange): QueryParamObject { return { - [TimeRangeService.TIME_RANGE_QUERY_PARAM]: newFixedTimeRange.toUrlString() + [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString() }; } diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index 4f56d0a0c..19463ef19 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -58,18 +58,22 @@ export class NavItemComponent { public readonly navItemViewStyle?: NavViewStyle; public buildNavigationParam = (item: NavItemLinkConfig): NavigationParams => { - const timeRange = this.config.timeRangeResolver ? this.config.timeRangeResolver() : undefined; - - return { + let navParams: NavigationParams = { navType: NavigationParamsType.InApp, path: item.matchPaths[0], relativeTo: this.activatedRoute, - replaceCurrentHistory: item.replaceCurrentHistory, - queryParams: - timeRange && this.config.pageLevelTimeRangeIsEnabled - ? this.timeRangeService.toQueryParams(timeRange.startTime, timeRange.endTime, timeRange) - : undefined + replaceCurrentHistory: item.replaceCurrentHistory }; + + if (this.config.pageLevelTimeRangeIsEnabled && this.config.timeRangeResolver) { + const timeRange = this.config.timeRangeResolver(); + navParams = { + ...navParams, + queryParams: this.timeRangeService.toQueryParams(timeRange) + }; + } + + return navParams; }; public constructor( diff --git a/projects/components/src/page-time-range/page-time-range.component.ts b/projects/components/src/page-time-range/page-time-range.component.ts index a5fccfa90..128a04012 100644 --- a/projects/components/src/page-time-range/page-time-range.component.ts +++ b/projects/components/src/page-time-range/page-time-range.component.ts @@ -35,7 +35,7 @@ export class PageTimeRangeComponent { } public shouldSavePageTimeRange(currentRoute: ActivatedRoute): boolean { - return !isNil(currentRoute.snapshot.data?.shouldSavePageTimeRange); + return currentRoute.snapshot.data?.shouldSavePageTimeRange ?? false; } public savePageTimeRange(selectedTimeRange: TimeRange, segment: UrlSegment): void { diff --git a/projects/observability/src/shared/dashboard/widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation.service.ts b/projects/observability/src/shared/dashboard/widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation.service.ts index d33c6d167..722f24d2e 100644 --- a/projects/observability/src/shared/dashboard/widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation.service.ts +++ b/projects/observability/src/shared/dashboard/widgets/charts/cartesian-widget/interactions/cartesian-explorer-navigation.service.ts @@ -1,5 +1,5 @@ import { Injectable } from '@angular/core'; -import { NavigationParamsType, NavigationService, TimeRangeService } from '@hypertrace/common'; +import { FixedTimeRange, NavigationParamsType, NavigationService, TimeRangeService } from '@hypertrace/common'; @Injectable({ providedIn: 'root' @@ -12,7 +12,7 @@ export class CartesainExplorerNavigationService { public navigateToExplorer(start: Date, end: Date): void { this.timeRangeService.setFixedRange(start, end); - const params = this.timeRangeService.toQueryParams(start, end); + const params = this.timeRangeService.toQueryParams(new FixedTimeRange(start, end)); this.navigationService.navigate({ navType: NavigationParamsType.InApp, From 31d4f0fbd4986e01dab9ee6d85cdc8850a9544ee Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Wed, 30 Mar 2022 11:23:20 -0300 Subject: [PATCH 04/14] refactor: added refresh query param --- .../common/src/time/time-range.service.ts | 25 ++++++++++++++++--- .../navigation/nav-item/nav-item.component.ts | 8 +++++- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 7929931ea..d24170f5e 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,7 +1,7 @@ import { Injectable } from '@angular/core'; import { isEmpty, isNil } from 'lodash-es'; import { EMPTY, Observable, ReplaySubject } from 'rxjs'; -import { catchError, filter, map, switchMap, take } from 'rxjs/operators'; +import { catchError, filter, map, pairwise, switchMap, take } from 'rxjs/operators'; import { ApplicationFeature } from '../constants/application-constants'; import { FeatureStateResolver } from '../feature/state/feature-state.resolver'; import { FeatureState } from '../feature/state/feature.state'; @@ -19,6 +19,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 = new ReplaySubject(1); private currentTimeRange?: TimeRange; @@ -81,9 +82,19 @@ export class TimeRangeService { private pageTimeRangeInitConfigAndChanges(): Observable { return this.navigationService.navigation$.pipe( switchMap(activeRoute => activeRoute.queryParamMap), - filter(queryParmaMap => !isNil(queryParmaMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))), - map(queryParmaMap => { - const timeRangeQueryParamString = queryParmaMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM); + filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))), + pairwise(), + filter(([prevParamMap, currParamMap]) => { + 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) ? false : JSON.parse(refreshQueryParam!)) + ); + }), + map(([, currQueryParamMap]) => { + const timeRangeQueryParamString = currQueryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM); return this.timeRangeFromUrlString(timeRangeQueryParamString!); }) @@ -150,6 +161,12 @@ export class TimeRangeService { }; } + public getRefreshTimeRangeQueryParam(): QueryParamObject { + return { + [TimeRangeService.REFRESH_ON_NAVIGATION]: true + }; + } + public isInitialized(): boolean { return !isNil(this.currentTimeRange); } diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index 19463ef19..0892bcb20 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -67,9 +67,15 @@ export class NavItemComponent { if (this.config.pageLevelTimeRangeIsEnabled && this.config.timeRangeResolver) { const timeRange = this.config.timeRangeResolver(); + const timeRangeQueryParam = this.timeRangeService.toQueryParams(timeRange); + const timeRangeRefreshQueryParam = this.timeRangeService.getRefreshTimeRangeQueryParam(); + navParams = { ...navParams, - queryParams: this.timeRangeService.toQueryParams(timeRange) + queryParams: { + ...timeRangeQueryParam, + ...timeRangeRefreshQueryParam + } }; } From 595017a237ddec26f34df352ec499c390b80761b Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Wed, 30 Mar 2022 11:26:07 -0300 Subject: [PATCH 05/14] refactor: name change --- projects/common/src/time/time-range.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index d24170f5e..c8b9873ee 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -67,7 +67,7 @@ export class TimeRangeService { this.setTimeRange(this.getCurrentTimeRange()); } - private globalTimeRangeInitConfigAndChanges(): Observable { + private getInitialTimeRange(): Observable { return this.navigationService.navigation$.pipe( take(1), // Wait for first navigation switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it @@ -79,7 +79,7 @@ export class TimeRangeService { ); } - private pageTimeRangeInitConfigAndChanges(): Observable { + private getPageTimeRangeChanges(): Observable { return this.navigationService.navigation$.pipe( switchMap(activeRoute => activeRoute.queryParamMap), filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))), @@ -107,10 +107,10 @@ export class TimeRangeService { .pipe( switchMap(featureState => { if (featureState === FeatureState.Enabled) { - return this.pageTimeRangeInitConfigAndChanges(); + return this.getPageTimeRangeChanges(); } - return this.globalTimeRangeInitConfigAndChanges(); + return this.getInitialTimeRange(); }) ) .subscribe(timeRange => { From e70957dccd6743829730a893e48f9a12476f8bf7 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Wed, 30 Mar 2022 15:11:56 -0300 Subject: [PATCH 06/14] refactor: requested changes --- .../common/src/time/time-range.service.ts | 39 ++++++++++--------- .../navigation/nav-item/nav-item.component.ts | 6 +-- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index c8b9873ee..b34a2c5c1 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,7 +1,8 @@ import { Injectable } from '@angular/core'; +import { ParamMap } from '@angular/router'; import { isEmpty, isNil } from 'lodash-es'; import { EMPTY, Observable, ReplaySubject } from 'rxjs'; -import { catchError, filter, map, pairwise, switchMap, take } from 'rxjs/operators'; +import { catchError, distinctUntilChanged, filter, map, switchMap, take } from 'rxjs/operators'; import { ApplicationFeature } from '../constants/application-constants'; import { FeatureStateResolver } from '../feature/state/feature-state.resolver'; import { FeatureState } from '../feature/state/feature.state'; @@ -83,17 +84,8 @@ export class TimeRangeService { return this.navigationService.navigation$.pipe( switchMap(activeRoute => activeRoute.queryParamMap), filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))), - pairwise(), - filter(([prevParamMap, currParamMap]) => { - 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) ? false : JSON.parse(refreshQueryParam!)) - ); - }), - map(([, currQueryParamMap]) => { + distinctUntilChanged((prevParamMap, currParamMap) => this.shouldNotUpdateTimeRange(prevParamMap, currParamMap)), + map(currQueryParamMap => { const timeRangeQueryParamString = currQueryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM); return this.timeRangeFromUrlString(timeRangeQueryParamString!); @@ -101,6 +93,15 @@ export class TimeRangeService { ); } + 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) + ); + } + private initializeTimeRange(): void { this.featureStateResolver .getFeatureState(ApplicationFeature.PageTimeRange) @@ -155,16 +156,16 @@ export class TimeRangeService { return new FixedTimeRange(startTime, endTime); } - public toQueryParams(timeRange: TimeRange): QueryParamObject { - return { + public toQueryParams(timeRange: TimeRange, refreshTimeOnNavigationParam: boolean): QueryParamObject { + let queryParams: QueryParamObject = { [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString() }; - } - public getRefreshTimeRangeQueryParam(): QueryParamObject { - return { - [TimeRangeService.REFRESH_ON_NAVIGATION]: true - }; + if (refreshTimeOnNavigationParam) { + queryParams = { ...queryParams, [TimeRangeService.REFRESH_ON_NAVIGATION]: true }; + } + + return queryParams; } public isInitialized(): boolean { diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index 0892bcb20..3b68e8da6 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -67,14 +67,12 @@ export class NavItemComponent { if (this.config.pageLevelTimeRangeIsEnabled && this.config.timeRangeResolver) { const timeRange = this.config.timeRangeResolver(); - const timeRangeQueryParam = this.timeRangeService.toQueryParams(timeRange); - const timeRangeRefreshQueryParam = this.timeRangeService.getRefreshTimeRangeQueryParam(); + const timeRangeQueryParam = this.timeRangeService.toQueryParams(timeRange, true); navParams = { ...navParams, queryParams: { - ...timeRangeQueryParam, - ...timeRangeRefreshQueryParam + ...timeRangeQueryParam } }; } From 898dc907b685102602a4ca1194ec84d7ce146943 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Wed, 30 Mar 2022 15:19:11 -0300 Subject: [PATCH 07/14] refactor: option param type for refresh flag --- projects/common/src/time/time-range.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index b34a2c5c1..08a892db9 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -156,7 +156,7 @@ export class TimeRangeService { return new FixedTimeRange(startTime, endTime); } - public toQueryParams(timeRange: TimeRange, refreshTimeOnNavigationParam: boolean): QueryParamObject { + public toQueryParams(timeRange: TimeRange, refreshTimeOnNavigationParam?: boolean): QueryParamObject { let queryParams: QueryParamObject = { [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString() }; From be3d30f8f8f860d8b598c60a5a6b9ba5ac9cb19a Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Wed, 30 Mar 2022 16:44:22 -0300 Subject: [PATCH 08/14] refactor: testing support --- projects/common/src/time/time-range.service.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/projects/common/src/time/time-range.service.test.ts b/projects/common/src/time/time-range.service.test.ts index ca45765f5..2b25bf4a3 100644 --- a/projects/common/src/time/time-range.service.test.ts +++ b/projects/common/src/time/time-range.service.test.ts @@ -19,9 +19,9 @@ describe('Time range service', () => { map( initialTrString => // tslint:disable-next-line: no-object-literal-type-assertion - (({ - snapshot: { queryParamMap: convertToParamMap({ time: initialTrString }) } - } as unknown) as ActivatedRoute) + ({ + queryParamMap: of(convertToParamMap({ time: initialTrString, refresh: 'true' })) + } as ActivatedRoute) ) ); } From 17fa1b3101be5a1934bf2cb270ffea8abc0dc69a Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Thu, 31 Mar 2022 09:54:02 -0300 Subject: [PATCH 09/14] fix: nit requested changes --- projects/common/src/time/time-range.service.ts | 4 ++-- .../src/navigation/nav-item/nav-item.component.ts | 11 +++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 08a892db9..679559d54 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -157,12 +157,12 @@ export class TimeRangeService { } public toQueryParams(timeRange: TimeRange, refreshTimeOnNavigationParam?: boolean): QueryParamObject { - let queryParams: QueryParamObject = { + const queryParams: QueryParamObject = { [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString() }; if (refreshTimeOnNavigationParam) { - queryParams = { ...queryParams, [TimeRangeService.REFRESH_ON_NAVIGATION]: true }; + return { ...queryParams, [TimeRangeService.REFRESH_ON_NAVIGATION]: true }; } return queryParams; diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index 3b68e8da6..92df8587b 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -58,7 +58,7 @@ export class NavItemComponent { public readonly navItemViewStyle?: NavViewStyle; public buildNavigationParam = (item: NavItemLinkConfig): NavigationParams => { - let navParams: NavigationParams = { + const navParams: NavigationParams = { navType: NavigationParamsType.InApp, path: item.matchPaths[0], relativeTo: this.activatedRoute, @@ -66,14 +66,9 @@ export class NavItemComponent { }; if (this.config.pageLevelTimeRangeIsEnabled && this.config.timeRangeResolver) { - const timeRange = this.config.timeRangeResolver(); - const timeRangeQueryParam = this.timeRangeService.toQueryParams(timeRange, true); - - navParams = { + return { ...navParams, - queryParams: { - ...timeRangeQueryParam - } + queryParams: this.timeRangeService.toQueryParams(this.config.timeRangeResolver(), true) }; } From 9680f55011918d8cc1a6526c711296f33cb15eb6 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Fri, 1 Apr 2022 12:57:57 -0300 Subject: [PATCH 10/14] refactor: aarons changes --- .../common/src/time/time-range.service.ts | 64 ++++++++++--------- .../src/time-range/time-range.component.ts | 8 +-- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 679559d54..7432db5b3 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,11 +1,8 @@ import { Injectable } from '@angular/core'; import { ParamMap } from '@angular/router'; import { isEmpty, isNil } from 'lodash-es'; -import { EMPTY, Observable, ReplaySubject } from 'rxjs'; +import { concat, EMPTY, Observable, ReplaySubject } from 'rxjs'; import { catchError, distinctUntilChanged, filter, map, switchMap, take } from 'rxjs/operators'; -import { ApplicationFeature } from '../constants/application-constants'; -import { FeatureStateResolver } from '../feature/state/feature-state.resolver'; -import { FeatureState } from '../feature/state/feature.state'; import { NavigationService, QueryParamObject } from '../navigation/navigation.service'; import { ReplayObservable } from '../utilities/rxjs/rxjs-utils'; import { FixedTimeRange } from './fixed-time-range'; @@ -27,8 +24,7 @@ export class TimeRangeService { public constructor( private readonly navigationService: NavigationService, - private readonly timeDurationService: TimeDurationService, - private readonly featureStateResolver: FeatureStateResolver + private readonly timeDurationService: TimeDurationService ) { this.initializeTimeRange(); this.navigationService.registerGlobalQueryParamKey(TimeRangeService.TIME_RANGE_QUERY_PARAM); @@ -57,15 +53,18 @@ export class TimeRangeService { } public setRelativeRange(value: number, unit: TimeUnit): this { - return this.setTimeRange(TimeRangeService.toRelativeTimeRange(value, unit)); + // 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.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 { @@ -82,7 +81,7 @@ export class TimeRangeService { private getPageTimeRangeChanges(): Observable { return this.navigationService.navigation$.pipe( - switchMap(activeRoute => activeRoute.queryParamMap), + map(activeRoute => activeRoute.snapshot.queryParamMap), filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))), distinctUntilChanged((prevParamMap, currParamMap) => this.shouldNotUpdateTimeRange(prevParamMap, currParamMap)), map(currQueryParamMap => { @@ -93,6 +92,21 @@ export class TimeRangeService { ); } + private initializeTimeRange(): void { + concat(this.getInitialTimeRange(), this.getPageTimeRangeChanges()).subscribe(timeRange => { + if ( + this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, '') !== + timeRange.toUrlString() + ) { + console.log('from init CALLING setTimeRangeInUrl'); + this.setTimeRangeInUrl(timeRange); + } else { + console.log('from init CALLING applyTimeRangeChange'); + this.applyTimeRangeChange(timeRange); + } + }); + } + private shouldNotUpdateTimeRange(prevParamMap: ParamMap, currParamMap: ParamMap): boolean { const refreshQueryParam = currParamMap.get(TimeRangeService.REFRESH_ON_NAVIGATION); @@ -102,23 +116,6 @@ export class TimeRangeService { ); } - private initializeTimeRange(): void { - this.featureStateResolver - .getFeatureState(ApplicationFeature.PageTimeRange) - .pipe( - switchMap(featureState => { - if (featureState === FeatureState.Enabled) { - return this.getPageTimeRangeChanges(); - } - - return this.getInitialTimeRange(); - }) - ) - .subscribe(timeRange => { - this.setTimeRange(timeRange); - }); - } - public timeRangeFromUrlString(timeRangeFromUrl: string): TimeRange { const duration = this.timeDurationService.durationFromString(timeRangeFromUrl); if (duration) { @@ -132,11 +129,18 @@ export class TimeRangeService { throw new Error(); // Caught in observable } - private setTimeRange(newTimeRange: TimeRange): this { + private applyTimeRangeChange(newTimeRange: TimeRange): this { + console.log('Emitting'); this.currentTimeRange = newTimeRange; this.timeRangeSubject$.next(newTimeRange); + + return this; + } + + private setTimeRangeInUrl(timeRange: TimeRange): this { + console.log('Adding query params'); this.navigationService.addQueryParametersToUrl({ - [TimeRangeService.TIME_RANGE_QUERY_PARAM]: newTimeRange.toUrlString() + [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString() }); return this; @@ -144,7 +148,7 @@ export class TimeRangeService { public setDefaultTimeRange(timeRange: TimeRange): void { if (!this.currentTimeRange) { - this.setTimeRange(timeRange); + this.setTimeRangeInUrl(timeRange); } } diff --git a/projects/components/src/time-range/time-range.component.ts b/projects/components/src/time-range/time-range.component.ts index 411d45311..49fb23c81 100644 --- a/projects/components/src/time-range/time-range.component.ts +++ b/projects/components/src/time-range/time-range.component.ts @@ -125,7 +125,7 @@ export class TimeRangeComponent { text$: of('Refresh'), role: ButtonRole.Tertiary, isEmphasized: false, - onClick: () => this.onRefresh(timeRange) + onClick: () => this.onRefresh() }), this.ngZone.runOutsideAngular(() => // Long running timer will prevent zone from stabilizing @@ -143,7 +143,7 @@ export class TimeRangeComponent { ), role: ButtonRole.Primary, isEmphasized: true, - onClick: () => this.onRefresh(timeRange) + onClick: () => this.onRefresh() })) ) ) @@ -153,8 +153,8 @@ export class TimeRangeComponent { return EMPTY; }; - private onRefresh(timeRange: RelativeTimeRange): void { - this.timeRangeService.setRelativeRange(timeRange.duration.value, timeRange.duration.unit); + private onRefresh(): void { + this.timeRangeService.refresh(); } } From 75f5be929bb654d9d7ff1eaae54066834573f6ce Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Fri, 1 Apr 2022 14:06:47 -0300 Subject: [PATCH 11/14] refactor: requested changes --- .../common/src/time/time-range.service.ts | 22 +++++++++++-------- .../feature-resolver.service.ts | 11 +++++++--- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 7432db5b3..dacfdc71c 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@angular/core'; import { ParamMap } from '@angular/router'; -import { isEmpty, isNil } from 'lodash-es'; +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'; @@ -94,15 +94,15 @@ export class TimeRangeService { private initializeTimeRange(): void { concat(this.getInitialTimeRange(), this.getPageTimeRangeChanges()).subscribe(timeRange => { - if ( - this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, '') !== - timeRange.toUrlString() - ) { - console.log('from init CALLING setTimeRangeInUrl'); + if (!this.timeRangeMatchesCurrentUrl(timeRange)) { this.setTimeRangeInUrl(timeRange); } else { - console.log('from init CALLING applyTimeRangeChange'); 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)); + } } }); } @@ -130,7 +130,6 @@ export class TimeRangeService { } private applyTimeRangeChange(newTimeRange: TimeRange): this { - console.log('Emitting'); this.currentTimeRange = newTimeRange; this.timeRangeSubject$.next(newTimeRange); @@ -138,7 +137,6 @@ export class TimeRangeService { } private setTimeRangeInUrl(timeRange: TimeRange): this { - console.log('Adding query params'); this.navigationService.addQueryParametersToUrl({ [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString() }); @@ -172,6 +170,12 @@ export class TimeRangeService { return queryParams; } + private timeRangeMatchesCurrentUrl(timeRange: TimeRange): boolean { + return ( + this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, '') === timeRange.toUrlString() + ); + } + public isInitialized(): boolean { return !isNil(this.currentTimeRange); } diff --git a/src/app/shared/feature-resolver/feature-resolver.service.ts b/src/app/shared/feature-resolver/feature-resolver.service.ts index c8e56b1cf..4f41101ec 100644 --- a/src/app/shared/feature-resolver/feature-resolver.service.ts +++ b/src/app/shared/feature-resolver/feature-resolver.service.ts @@ -1,10 +1,15 @@ import { Injectable } from '@angular/core'; -import { FeatureState, FeatureStateResolver } from '@hypertrace/common'; +import { ApplicationFeature, FeatureState, FeatureStateResolver } from '@hypertrace/common'; import { Observable, of } from 'rxjs'; @Injectable() export class FeatureResolverService extends FeatureStateResolver { - public getFeatureState(_: string): Observable { - return of(FeatureState.Enabled); + public getFeatureState(flag: string): Observable { + switch (flag) { + case ApplicationFeature.PageTimeRange: + return of(FeatureState.Disabled); + default: + return of(FeatureState.Enabled); + } } } From c8f632e6c4d470a6f3a0012c51872a0e70880993 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Fri, 1 Apr 2022 14:28:04 -0300 Subject: [PATCH 12/14] refactor: remove htMemoize from nav item --- .../src/navigation/nav-item/nav-item.component.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index 92df8587b..6aa695084 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -9,7 +9,7 @@ import { NavItemLinkConfig, NavViewStyle } from '../navigation.config'; styleUrls: ['./nav-item.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, template: ` - +
{ + public buildNavigationParam(item: NavItemLinkConfig): NavigationParams { const navParams: NavigationParams = { navType: NavigationParamsType.InApp, path: item.matchPaths[0], @@ -73,7 +73,7 @@ export class NavItemComponent { } return navParams; - }; + } public constructor( private readonly activatedRoute: ActivatedRoute, From a2e33fc53b1c9785655d208442076af7b8c375f9 Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Sat, 2 Apr 2022 15:44:32 -0300 Subject: [PATCH 13/14] test: added and fixed testing for new time range --- .../src/time/time-range.service.test.ts | 142 +++++++++++++----- .../common/src/time/time-range.service.ts | 2 - .../time-range/time-range.component.test.ts | 2 +- 3 files changed, 104 insertions(+), 42 deletions(-) diff --git a/projects/common/src/time/time-range.service.test.ts b/projects/common/src/time/time-range.service.test.ts index 2b25bf4a3..dde30abaa 100644 --- a/projects/common/src/time/time-range.service.test.ts +++ b/projects/common/src/time/time-range.service.test.ts @@ -1,14 +1,13 @@ import { ActivatedRoute, convertToParamMap } from '@angular/router'; -import { FeatureState, FeatureStateResolver } from '@hypertrace/common'; -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 = NEVER; const buildService = createServiceFactory({ service: TimeRangeService, @@ -19,15 +18,19 @@ describe('Time range service', () => { map( initialTrString => // tslint:disable-next-line: no-object-literal-type-assertion - ({ - queryParamMap: of(convertToParamMap({ time: initialTrString, refresh: 'true' })) - } as ActivatedRoute) + (({ + queryParamMap: of(convertToParamMap({ time: initialTrString, refresh: 'true' })), + snapshot: { queryParamMap: convertToParamMap({ time: initialTrString, refresh: 'true' }) } + } as unknown) as ActivatedRoute) ) ); - } - }), - mockProvider(FeatureStateResolver, { - getFeatureState: () => of(FeatureState.Enabled) + }, + getQueryParameter: jest + .fn() + .mockReturnValueOnce('1573255100253-1573255111159') + .mockReturnValue('1573255111159-1573455111990'), + getCurrentActivatedRoute: () => + (({ snapshot: { queryParams: { time: 'test-value' } } } as unknown) as ActivatedRoute) }) ] }); @@ -38,50 +41,111 @@ 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 }); }); }); diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index dacfdc71c..ead04524c 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -53,12 +53,10 @@ 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)); } diff --git a/projects/components/src/time-range/time-range.component.test.ts b/projects/components/src/time-range/time-range.component.test.ts index 7e54ccecf..3843dbe43 100644 --- a/projects/components/src/time-range/time-range.component.test.ts +++ b/projects/components/src/time-range/time-range.component.test.ts @@ -131,7 +131,7 @@ describe('Time range component', () => { test('should publish new time range when refresh is clicked', () => { const spectator = createComponent(); - const spy = jest.spyOn(spectator.inject(TimeRangeService), 'setRelativeRange'); + const spy = jest.spyOn(spectator.inject(TimeRangeService), 'refresh'); spectator.click('.refresh'); expect(spy).toHaveBeenCalled(); }); From 1321830f0400c34c850d82f83860506bd7355a2b Mon Sep 17 00:00:00 2001 From: Christian <35982795+Christian862@users.noreply.github.com> Date: Mon, 4 Apr 2022 10:31:18 -0300 Subject: [PATCH 14/14] refactor: restore momoization for nav item --- .../src/navigation/nav-item/nav-item.component.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/projects/components/src/navigation/nav-item/nav-item.component.ts b/projects/components/src/navigation/nav-item/nav-item.component.ts index 6aa695084..92df8587b 100644 --- a/projects/components/src/navigation/nav-item/nav-item.component.ts +++ b/projects/components/src/navigation/nav-item/nav-item.component.ts @@ -9,7 +9,7 @@ import { NavItemLinkConfig, NavViewStyle } from '../navigation.config'; styleUrls: ['./nav-item.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, template: ` - +
{ const navParams: NavigationParams = { navType: NavigationParamsType.InApp, path: item.matchPaths[0], @@ -73,7 +73,7 @@ export class NavItemComponent { } return navParams; - } + }; public constructor( private readonly activatedRoute: ActivatedRoute,