From 85adc37341c6538dfb0b5cf49c76e4ad85c9fd71 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Mon, 17 May 2021 15:46:02 +0800 Subject: [PATCH 01/14] feature: adding open in new tab icon on security overlay --- .../common/src/time/time-range.service.ts | 11 ++++- .../open-in-new-tab.component.ts | 43 +++---------------- .../open-in-new-tab/open-in-new-tab.module.ts | 4 +- 3 files changed, 19 insertions(+), 39 deletions(-) diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index f9e96ca8e..bc524fc99 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,4 +1,5 @@ import { Injectable } from '@angular/core'; +import { Dictionary } from 'lodash'; import { isEmpty } from 'lodash-es'; import { EMPTY, ReplaySubject } from 'rxjs'; import { catchError, defaultIfEmpty, filter, map, switchMap, take } from 'rxjs/operators'; @@ -30,8 +31,7 @@ export class TimeRangeService { } public getShareableCurrentUrl(): string { - const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, ''); - const timeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${timeRangeParamValue}`; + const timeRangeParam = Object.entries(this.getTimeRangeParams()).map((key, value) => `${key}=${value}`).join('&'); const timeRange = this.getCurrentTimeRange(); const fixedTimeRange: FixedTimeRange = TimeRangeService.toFixedTimeRange(timeRange.startTime, timeRange.endTime); const newTimeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${fixedTimeRange.toUrlString()}`; @@ -39,6 +39,13 @@ export class TimeRangeService { return this.navigationService.getAbsoluteCurrentUrl().replace(timeRangeParam, newTimeRangeParam); } + public getTimeRangeParams(): Dictionary { + const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, ''); + return { + [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeParamValue + }; + } + public getTimeRangeAndChanges(): ReplayObservable { return this.timeRangeSubject$.asObservable(); } diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts index 48099a1bb..730ca213d 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts @@ -1,46 +1,17 @@ -import { ChangeDetectionStrategy, Component, Input } from '@angular/core'; +import { ChangeDetectionStrategy, Component } from '@angular/core'; import { IconType } from '@hypertrace/assets-library'; -import { - ExternalNavigationWindowHandling, - NavigationParamsType, - NavigationService, - TimeRangeService -} from '@hypertrace/common'; -import { ButtonSize, ButtonStyle } from '../button/button'; +import { IconSize } from '../icon/icon-size'; +import { LinkComponent } from '../link/link.component'; @Component({ selector: 'ht-open-in-new-tab', changeDetection: ChangeDetectionStrategy.OnPush, template: `
- + + +
` }) -export class OpenInNewTabComponent { - @Input() - public size?: ButtonSize = ButtonSize.Small; - - @Input() - public url?: string; - - public constructor( - private readonly navigationService: NavigationService, - private readonly timeRangeService: TimeRangeService - ) {} - - public openInNewTab(): void { - this.navigationService.navigate({ - navType: NavigationParamsType.External, - windowHandling: ExternalNavigationWindowHandling.NewWindow, - // Use input url if available. Else construct a shareable URL for the page - url: this.url ?? this.timeRangeService.getShareableCurrentUrl() - }); - } -} +export class OpenInNewTabComponent extends LinkComponent {} diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts index a4f7b5ee8..b3bb7dc12 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts @@ -1,12 +1,14 @@ import { CommonModule } from '@angular/common'; import { NgModule } from '@angular/core'; import { ButtonModule } from '../button/button.module'; +import { IconModule } from '../icon/icon.module'; +import { LinkModule } from '../link/link.module'; import { TooltipModule } from '../tooltip/tooltip.module'; import { OpenInNewTabComponent } from './open-in-new-tab.component'; @NgModule({ declarations: [OpenInNewTabComponent], exports: [OpenInNewTabComponent], - imports: [CommonModule, ButtonModule, TooltipModule] + imports: [CommonModule, ButtonModule, TooltipModule, LinkModule, IconModule] }) export class OpenInNewTabModule {} From 20087d1c160e88103c6cd008c82b7afaf4b785d0 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Tue, 18 May 2021 17:15:02 +0800 Subject: [PATCH 02/14] feat: changes for open in new tab component --- .../external/external-url-navigator.test.ts | 23 +++++- .../src/external/external-url-navigator.ts | 68 +++++++++--------- .../src/navigation/navigation.service.ts | 3 +- projects/common/src/public-api.ts | 3 + .../common/src/time/time-range.service.ts | 3 +- .../common/src/utilities/url/url.utilities.ts | 23 ++++++ .../open-in-new-tab.component.test.ts | 71 ++++++++++++------- .../open-in-new-tab.component.ts | 11 +-- .../open-in-new-tab/open-in-new-tab.module.ts | 3 +- 9 files changed, 139 insertions(+), 69 deletions(-) create mode 100644 projects/common/src/utilities/url/url.utilities.ts diff --git a/projects/common/src/external/external-url-navigator.test.ts b/projects/common/src/external/external-url-navigator.test.ts index 230a654aa..dd45926e3 100644 --- a/projects/common/src/external/external-url-navigator.test.ts +++ b/projects/common/src/external/external-url-navigator.test.ts @@ -1,4 +1,4 @@ -import { ActivatedRouteSnapshot, convertToParamMap } from '@angular/router'; +import { ActivatedRouteSnapshot, convertToParamMap, Params } from '@angular/router'; import { createServiceFactory, mockProvider, SpectatorService } from '@ngneat/spectator/jest'; import { ExternalNavigationPathParams, @@ -22,7 +22,7 @@ describe('External URL navigator', () => { test('goes back when unable to detect a url on navigation', () => { // tslint:disable-next-line: no-object-literal-type-assertion spectator.service.canActivate({ - paramMap: convertToParamMap({}) + paramMap: convertToParamMap({}), } as ActivatedRouteSnapshot); expect(spectator.inject(NavigationService).navigateBack).toHaveBeenCalledTimes(1); @@ -57,4 +57,23 @@ describe('External URL navigator', () => { expect(window.open).toHaveBeenNthCalledWith(2, 'https://www.bing.com', undefined); expect(spectator.inject(NavigationService).navigateBack).not.toHaveBeenCalled(); }); + + test('navigates when a url is provided with query params', () => { + const queryParams: Params = { + time: '1h' + }; + // tslint:disable-next-line: no-object-literal-type-assertion + spectator.service.canActivate({ + paramMap: convertToParamMap({ + [ExternalNavigationPathParams.Url]: 'https://www.bing.com', + [ExternalNavigationPathParams.WindowHandling]: ExternalNavigationWindowHandling.NewWindow, + }), + queryParams + } as ActivatedRouteSnapshot); + + expect(window.open).toHaveBeenCalledWith('https://www.bing.com?time=1h', undefined); + expect(spectator.inject(NavigationService).navigateBack).not.toHaveBeenCalled(); + }); + + }); diff --git a/projects/common/src/external/external-url-navigator.ts b/projects/common/src/external/external-url-navigator.ts index 98bba6439..afd096016 100644 --- a/projects/common/src/external/external-url-navigator.ts +++ b/projects/common/src/external/external-url-navigator.ts @@ -2,45 +2,49 @@ import { Injectable } from '@angular/core'; import { ActivatedRouteSnapshot, CanActivate } from '@angular/router'; import { Observable, of } from 'rxjs'; import { - ExternalNavigationPathParams, - ExternalNavigationWindowHandling, - NavigationService + ExternalNavigationPathParams, + ExternalNavigationWindowHandling, + NavigationService } from '../navigation/navigation.service'; import { assertUnreachable } from '../utilities/lang/lang-utils'; +import { getQueryParamStringFromObject } from '../utilities/url/url.utilities'; @Injectable({ providedIn: 'root' }) export class ExternalUrlNavigator implements CanActivate { - public constructor(private readonly navService: NavigationService) {} + public constructor(private readonly navService: NavigationService) { } - public canActivate(route: ActivatedRouteSnapshot): Observable { - const encodedUrl = route.paramMap.get(ExternalNavigationPathParams.Url); - const windowHandling = route.paramMap.has(ExternalNavigationPathParams.WindowHandling) - ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) - : undefined; - if (encodedUrl !== null && encodedUrl.length > 0) { - this.navigateToUrl(encodedUrl, windowHandling); - } else { - this.navService.navigateBack(); - } + public canActivate(route: ActivatedRouteSnapshot): Observable { + const url = route.paramMap.get(ExternalNavigationPathParams.Url); + const queryParamString = getQueryParamStringFromObject(route.queryParams ?? {}); + const encodedUrl = url ? `${url}${queryParamString ? `?${queryParamString}` : ``}` : url; - return of(false); // Can't navigate, but we've already navigated anyway - } + const windowHandling = route.paramMap.has(ExternalNavigationPathParams.WindowHandling) + ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) + : undefined; + if (encodedUrl !== null && encodedUrl.length > 0) { + this.navigateToUrl(encodedUrl, windowHandling); + } else { + this.navService.navigateBack(); + } - private navigateToUrl( - url: string, - windowHandling: ExternalNavigationWindowHandling = ExternalNavigationWindowHandling.SameWindow - ): void { - window.open(url, this.asWindowName(windowHandling)); - } + return of(false); // Can't navigate, but we've already navigated anyway + } - private asWindowName(windowHandling: ExternalNavigationWindowHandling): string | undefined { - switch (windowHandling) { - case ExternalNavigationWindowHandling.SameWindow: - return '_self'; - case ExternalNavigationWindowHandling.NewWindow: - return undefined; - default: - assertUnreachable(windowHandling); - } - } + private navigateToUrl( + url: string, + windowHandling: ExternalNavigationWindowHandling = ExternalNavigationWindowHandling.SameWindow + ): void { + window.open(url, this.asWindowName(windowHandling)); + } + + private asWindowName(windowHandling: ExternalNavigationWindowHandling): string | undefined { + switch (windowHandling) { + case ExternalNavigationWindowHandling.SameWindow: + return '_self'; + case ExternalNavigationWindowHandling.NewWindow: + return undefined; + default: + assertUnreachable(windowHandling); + } + } } diff --git a/projects/common/src/navigation/navigation.service.ts b/projects/common/src/navigation/navigation.service.ts index 837164a8e..29cc83a83 100644 --- a/projects/common/src/navigation/navigation.service.ts +++ b/projects/common/src/navigation/navigation.service.ts @@ -89,7 +89,8 @@ export class NavigationService { } ], extras: { - skipLocationChange: true // Don't bother showing the updated location, we're going external anyway + skipLocationChange: true, // Don't bother showing the updated location, we're going external anyway + queryParams: params.queryParams } }; } diff --git a/projects/common/src/public-api.ts b/projects/common/src/public-api.ts index 9df58361b..4c0ae3882 100644 --- a/projects/common/src/public-api.ts +++ b/projects/common/src/public-api.ts @@ -111,3 +111,6 @@ export * from './time/time'; // Validators export * from './utilities/validators/email-validator'; + +// URL Utilities +export * from './utilities/url/url.utilities'; diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index bc524fc99..84037da0e 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -5,6 +5,7 @@ import { EMPTY, ReplaySubject } from 'rxjs'; import { catchError, defaultIfEmpty, filter, map, switchMap, take } from 'rxjs/operators'; import { NavigationService } from '../navigation/navigation.service'; import { ReplayObservable } from '../utilities/rxjs/rxjs-utils'; +import { getQueryParamStringFromObject } from '../utilities/url/url.utilities'; import { FixedTimeRange } from './fixed-time-range'; import { RelativeTimeRange } from './relative-time-range'; import { TimeDuration } from './time-duration'; @@ -31,7 +32,7 @@ export class TimeRangeService { } public getShareableCurrentUrl(): string { - const timeRangeParam = Object.entries(this.getTimeRangeParams()).map((key, value) => `${key}=${value}`).join('&'); + const timeRangeParam = getQueryParamStringFromObject(this.getTimeRangeParams()); const timeRange = this.getCurrentTimeRange(); const fixedTimeRange: FixedTimeRange = TimeRangeService.toFixedTimeRange(timeRange.startTime, timeRange.endTime); const newTimeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${fixedTimeRange.toUrlString()}`; diff --git a/projects/common/src/utilities/url/url.utilities.ts b/projects/common/src/utilities/url/url.utilities.ts new file mode 100644 index 000000000..2369845ad --- /dev/null +++ b/projects/common/src/utilities/url/url.utilities.ts @@ -0,0 +1,23 @@ +import { Dictionary, isEmpty } from "lodash" + +export const getQueryParamStringFromObject = (params: Dictionary): string => { + try { + const paramString: string = Object.entries(params).map(([key, value]) => `${key}=${value}`).join('&'); + return isEmpty(paramString) ? '' : paramString; + } catch(error) { + return ``; + } +}; + +export const getQueryParamObjectFromString = (string: string): Dictionary => { + try { + string = string[0] === '?' ? string.substring(1) : string; + return string.split('&').reduce((acc: Dictionary, item: string): Dictionary => { + const [key, value] = item.split('='); + acc[key] = value; + return acc; + }, {}); + } catch (error) { + return {}; + } +}; \ No newline at end of file diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts index ab7b2373b..761e59384 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts @@ -1,13 +1,13 @@ import { fakeAsync } from '@angular/core/testing'; import { - ExternalNavigationWindowHandling, - NavigationParamsType, NavigationService, TimeRangeService } from '@hypertrace/common'; import { createHostFactory, mockProvider, Spectator } from '@ngneat/spectator/jest'; import { MockComponent } from 'ng-mocks'; -import { ButtonComponent } from '../button/button.component'; +import { IconComponent } from '../icon/icon.component'; +import { IconSize } from '../icon/icon-size'; +import { LinkComponent } from '../link/link.component'; import { OpenInNewTabComponent } from './open-in-new-tab.component'; describe('Open in new tab component', () => { @@ -16,42 +16,59 @@ describe('Open in new tab component', () => { const createHost = createHostFactory({ shallow: true, component: OpenInNewTabComponent, - declarations: [MockComponent(ButtonComponent)], + declarations: [MockComponent(LinkComponent), MockComponent(IconComponent)], providers: [ mockProvider(TimeRangeService, { getShareableCurrentUrl: () => 'url-from-timerangeservice' }), - mockProvider(NavigationService) - ] + mockProvider(NavigationService, { + buildNavigationParams: jest.fn().mockReturnValue({ + path: [ + '/external', + { + url: 'http://test.hypertrace.ai', + navType: 'same_window' + } + ], + extras: { skipLocationChange: true } + }) + }) + ], }); - test('should call navigate as expected when URL input is not specified', fakeAsync(() => { - spectator = createHost(''); - - spectator.click('.open-in-new-tab-button'); - spectator.tick(); - - expect(spectator.inject(NavigationService).navigate).toHaveBeenCalledWith({ - navType: NavigationParamsType.External, - windowHandling: ExternalNavigationWindowHandling.NewWindow, - url: 'url-from-timerangeservice' + test('Open in new tab component should not be displayed if paramsOrUrl is undefined', () => { + spectator = createHost(``, { + hostProps: { + paramsOrUrl: undefined + } }); - })); + expect(spectator.query('.open-in-new-tab')).not.toExist(); + }); - test('should call navigate as expected when URL input is specified', fakeAsync(() => { - spectator = createHost('', { + test(`Open in new tab component should exist if paramsOrUrl is not undefined`, fakeAsync(() => { + spectator = createHost(``, { hostProps: { - url: 'input-url' + paramsOrUrl: {} } }); + expect(spectator.query('.open-in-new-tab')).toExist(); + expect(spectator.query('ht-link')).toExist(); + // default value of icon size + expect(spectator.component.iconSize).toBe(IconSize.Medium); + })); - spectator.click('.open-in-new-tab-button'); - spectator.tick(); - - expect(spectator.inject(NavigationService).navigate).toHaveBeenCalledWith({ - navType: NavigationParamsType.External, - windowHandling: ExternalNavigationWindowHandling.NewWindow, - url: 'input-url' + + test(`Open in new tab component should contain icon of passed size`, fakeAsync(() => { + spectator = createHost(``, { + hostProps: { + paramsOrUrl: {}, + iconSize: IconSize.Small + } }); + expect(spectator.query('.open-in-new-tab')).toExist(); + expect(spectator.query('ht-link')).toExist(); + // expected value of icon size if passed + expect(spectator.component.iconSize).toBe(IconSize.Small); })); + }); diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts index 730ca213d..5681e9f61 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectionStrategy, Component } from '@angular/core'; +import { ChangeDetectionStrategy, Component, Input } from '@angular/core'; import { IconType } from '@hypertrace/assets-library'; import { IconSize } from '../icon/icon-size'; import { LinkComponent } from '../link/link.component'; @@ -7,11 +7,14 @@ import { LinkComponent } from '../link/link.component'; selector: 'ht-open-in-new-tab', changeDetection: ChangeDetectionStrategy.OnPush, template: ` -
+
- +
` }) -export class OpenInNewTabComponent extends LinkComponent {} +export class OpenInNewTabComponent extends LinkComponent { + @Input() + public iconSize: IconSize = IconSize.Medium; +} diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts index b3bb7dc12..0c3bffa3f 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.module.ts @@ -1,6 +1,5 @@ import { CommonModule } from '@angular/common'; import { NgModule } from '@angular/core'; -import { ButtonModule } from '../button/button.module'; import { IconModule } from '../icon/icon.module'; import { LinkModule } from '../link/link.module'; import { TooltipModule } from '../tooltip/tooltip.module'; @@ -9,6 +8,6 @@ import { OpenInNewTabComponent } from './open-in-new-tab.component'; @NgModule({ declarations: [OpenInNewTabComponent], exports: [OpenInNewTabComponent], - imports: [CommonModule, ButtonModule, TooltipModule, LinkModule, IconModule] + imports: [CommonModule, TooltipModule, LinkModule, IconModule] }) export class OpenInNewTabModule {} From 6dfa494370ad34f7a35fc54bb742cfc457fc09c2 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Tue, 18 May 2021 17:52:49 +0800 Subject: [PATCH 03/14] fix: file rename and unti tests --- .../src/external/external-url-navigator.ts | 2 +- projects/common/src/public-api.ts | 2 +- .../common/src/time/time-range.service.ts | 2 +- .../src/utilities/url/url-utilities.spec.ts | 47 +++++++++++++++++++ .../{url.utilities.ts => url-utilities.ts} | 7 ++- 5 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 projects/common/src/utilities/url/url-utilities.spec.ts rename projects/common/src/utilities/url/{url.utilities.ts => url-utilities.ts} (76%) diff --git a/projects/common/src/external/external-url-navigator.ts b/projects/common/src/external/external-url-navigator.ts index afd096016..a57edcf9b 100644 --- a/projects/common/src/external/external-url-navigator.ts +++ b/projects/common/src/external/external-url-navigator.ts @@ -7,7 +7,7 @@ import { NavigationService } from '../navigation/navigation.service'; import { assertUnreachable } from '../utilities/lang/lang-utils'; -import { getQueryParamStringFromObject } from '../utilities/url/url.utilities'; +import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; @Injectable({ providedIn: 'root' }) export class ExternalUrlNavigator implements CanActivate { diff --git a/projects/common/src/public-api.ts b/projects/common/src/public-api.ts index 4c0ae3882..55ecf2609 100644 --- a/projects/common/src/public-api.ts +++ b/projects/common/src/public-api.ts @@ -113,4 +113,4 @@ export * from './time/time'; export * from './utilities/validators/email-validator'; // URL Utilities -export * from './utilities/url/url.utilities'; +export * from './utilities/url/url-utilities'; diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 84037da0e..f3b68d117 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -5,7 +5,7 @@ import { EMPTY, ReplaySubject } from 'rxjs'; import { catchError, defaultIfEmpty, filter, map, switchMap, take } from 'rxjs/operators'; import { NavigationService } from '../navigation/navigation.service'; import { ReplayObservable } from '../utilities/rxjs/rxjs-utils'; -import { getQueryParamStringFromObject } from '../utilities/url/url.utilities'; +import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; import { FixedTimeRange } from './fixed-time-range'; import { RelativeTimeRange } from './relative-time-range'; import { TimeDuration } from './time-duration'; diff --git a/projects/common/src/utilities/url/url-utilities.spec.ts b/projects/common/src/utilities/url/url-utilities.spec.ts new file mode 100644 index 000000000..9cff69cba --- /dev/null +++ b/projects/common/src/utilities/url/url-utilities.spec.ts @@ -0,0 +1,47 @@ +import { Params } from '@angular/router'; +import { + getQueryParamStringFromObject, + getQueryParamObjectFromString +} from './url-utilities'; + +describe('getQueryParamStringFromObject', () => { + it('should return string from a string dictionary', () => { + const params: Params = {}; + expect(getQueryParamStringFromObject(params)).toBe(''); + params.a = undefined; + expect(getQueryParamStringFromObject(params)).toBe(''); + params.a = null; + expect(getQueryParamStringFromObject(params)).toBe(`a=null`); + params.a = true; + expect(getQueryParamStringFromObject(params)).toBe(`a=true`); + params.a = false; + expect(getQueryParamStringFromObject(params)).toBe(`a=false`); + params.b = false; + expect(getQueryParamStringFromObject(params)).toBe(`a=false&b=false`); + params.a = 'value_1'; + params.b = 'value_2'; + expect(getQueryParamStringFromObject(params)).toBe(`a=value_1&b=value_2`); + }); +}); + +describe('getQueryParamObjectFromString', () => { + it('should return object from a string', () => { + let queryParam = '?queryparam1=value'; + expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); + expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value'); + + queryParam = '?queryparam1='; + expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); + expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe(''); + + queryParam = 'queryparam1=value'; + expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); + expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value'); + + queryParam = '?queryparam1=value1&queryparam2=value2'; + expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); + expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value1'); + expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam2'); + expect(getQueryParamObjectFromString(queryParam).queryparam2).toBe('value2'); + }); +}); \ No newline at end of file diff --git a/projects/common/src/utilities/url/url.utilities.ts b/projects/common/src/utilities/url/url-utilities.ts similarity index 76% rename from projects/common/src/utilities/url/url.utilities.ts rename to projects/common/src/utilities/url/url-utilities.ts index 2369845ad..d26b40e13 100644 --- a/projects/common/src/utilities/url/url.utilities.ts +++ b/projects/common/src/utilities/url/url-utilities.ts @@ -2,9 +2,12 @@ import { Dictionary, isEmpty } from "lodash" export const getQueryParamStringFromObject = (params: Dictionary): string => { try { - const paramString: string = Object.entries(params).map(([key, value]) => `${key}=${value}`).join('&'); + const paramString: string = Object.entries(params) + .map(([key, value]) => value !== undefined ? `${key}=${value}` : '') + .filter(item => item) + .join('&'); return isEmpty(paramString) ? '' : paramString; - } catch(error) { + } catch (error) { return ``; } }; From aa347911f891bf734afb7cb1e9545f21b7b62d35 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Tue, 18 May 2021 19:17:45 +0800 Subject: [PATCH 04/14] fix: lint fixes --- package.json | 2 +- .../external/external-url-navigator.test.ts | 8 ++-- .../common/src/time/time-range.service.ts | 1 + .../src/utilities/url/url-utilities.spec.ts | 11 ++--- .../common/src/utilities/url/url-utilities.ts | 45 ++++++++++--------- .../open-in-new-tab.component.test.ts | 28 ++++++------ 6 files changed, 45 insertions(+), 50 deletions(-) diff --git a/package.json b/package.json index 3f5bfd39e..5b9ef1fe3 100644 --- a/package.json +++ b/package.json @@ -60,6 +60,7 @@ "graphql": "^15.5.0", "graphql-tag": "^2.12.3", "iso8601-duration": "^1.3.0", + "lodash": "^4.17.21", "lodash-es": "^4.17.21", "rxjs": "~6.6.7", "tslib": "^2.2.0", @@ -101,7 +102,6 @@ "jest-html-reporter": "^3.2.0", "jest-junit": "^12.0.0", "jest-preset-angular": "^8.4.0", - "lodash": "^4.17.21", "ng-mocks": "^11.10.0", "ng-packagr": "^11.2.4", "prettier": "^2.2.1", diff --git a/projects/common/src/external/external-url-navigator.test.ts b/projects/common/src/external/external-url-navigator.test.ts index dd45926e3..5872be832 100644 --- a/projects/common/src/external/external-url-navigator.test.ts +++ b/projects/common/src/external/external-url-navigator.test.ts @@ -22,7 +22,7 @@ describe('External URL navigator', () => { test('goes back when unable to detect a url on navigation', () => { // tslint:disable-next-line: no-object-literal-type-assertion spectator.service.canActivate({ - paramMap: convertToParamMap({}), + paramMap: convertToParamMap({}) } as ActivatedRouteSnapshot); expect(spectator.inject(NavigationService).navigateBack).toHaveBeenCalledTimes(1); @@ -66,14 +66,12 @@ describe('External URL navigator', () => { spectator.service.canActivate({ paramMap: convertToParamMap({ [ExternalNavigationPathParams.Url]: 'https://www.bing.com', - [ExternalNavigationPathParams.WindowHandling]: ExternalNavigationWindowHandling.NewWindow, + [ExternalNavigationPathParams.WindowHandling]: ExternalNavigationWindowHandling.NewWindow }), - queryParams + queryParams: queryParams } as ActivatedRouteSnapshot); expect(window.open).toHaveBeenCalledWith('https://www.bing.com?time=1h', undefined); expect(spectator.inject(NavigationService).navigateBack).not.toHaveBeenCalled(); }); - - }); diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index f3b68d117..53fa2d600 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -42,6 +42,7 @@ export class TimeRangeService { public getTimeRangeParams(): Dictionary { const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, ''); + return { [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeParamValue }; diff --git a/projects/common/src/utilities/url/url-utilities.spec.ts b/projects/common/src/utilities/url/url-utilities.spec.ts index 9cff69cba..dcae330bd 100644 --- a/projects/common/src/utilities/url/url-utilities.spec.ts +++ b/projects/common/src/utilities/url/url-utilities.spec.ts @@ -1,8 +1,5 @@ import { Params } from '@angular/router'; -import { - getQueryParamStringFromObject, - getQueryParamObjectFromString -} from './url-utilities'; +import { getQueryParamObjectFromString, getQueryParamStringFromObject } from './url-utilities'; describe('getQueryParamStringFromObject', () => { it('should return string from a string dictionary', () => { @@ -10,8 +7,6 @@ describe('getQueryParamStringFromObject', () => { expect(getQueryParamStringFromObject(params)).toBe(''); params.a = undefined; expect(getQueryParamStringFromObject(params)).toBe(''); - params.a = null; - expect(getQueryParamStringFromObject(params)).toBe(`a=null`); params.a = true; expect(getQueryParamStringFromObject(params)).toBe(`a=true`); params.a = false; @@ -29,7 +24,7 @@ describe('getQueryParamObjectFromString', () => { let queryParam = '?queryparam1=value'; expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value'); - + queryParam = '?queryparam1='; expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe(''); @@ -44,4 +39,4 @@ describe('getQueryParamObjectFromString', () => { expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam2'); expect(getQueryParamObjectFromString(queryParam).queryparam2).toBe('value2'); }); -}); \ No newline at end of file +}); diff --git a/projects/common/src/utilities/url/url-utilities.ts b/projects/common/src/utilities/url/url-utilities.ts index d26b40e13..98724c24b 100644 --- a/projects/common/src/utilities/url/url-utilities.ts +++ b/projects/common/src/utilities/url/url-utilities.ts @@ -1,26 +1,29 @@ -import { Dictionary, isEmpty } from "lodash" +import { Dictionary, isEmpty } from 'lodash'; export const getQueryParamStringFromObject = (params: Dictionary): string => { - try { - const paramString: string = Object.entries(params) - .map(([key, value]) => value !== undefined ? `${key}=${value}` : '') - .filter(item => item) - .join('&'); - return isEmpty(paramString) ? '' : paramString; - } catch (error) { - return ``; - } + try { + const paramString: string = Object.entries(params) + .map(([key, value]) => (value !== undefined ? `${key}=${value}` : '')) + .filter(item => item) + .join('&'); + + return isEmpty(paramString) ? '' : paramString; + } catch (error) { + return ``; + } }; export const getQueryParamObjectFromString = (string: string): Dictionary => { - try { - string = string[0] === '?' ? string.substring(1) : string; - return string.split('&').reduce((acc: Dictionary, item: string): Dictionary => { - const [key, value] = item.split('='); - acc[key] = value; - return acc; - }, {}); - } catch (error) { - return {}; - } -}; \ No newline at end of file + try { + string = string[0] === '?' ? string.substring(1) : string; + + return string.split('&').reduce((acc: Dictionary, item: string): Dictionary => { + const [key, value] = item.split('='); + acc[key] = value; + + return acc; + }, {}); + } catch (error) { + return {}; + } +}; diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts index 761e59384..f8875be97 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts @@ -1,12 +1,9 @@ import { fakeAsync } from '@angular/core/testing'; -import { - NavigationService, - TimeRangeService -} from '@hypertrace/common'; +import { NavigationService, TimeRangeService } from '@hypertrace/common'; import { createHostFactory, mockProvider, Spectator } from '@ngneat/spectator/jest'; import { MockComponent } from 'ng-mocks'; -import { IconComponent } from '../icon/icon.component'; import { IconSize } from '../icon/icon-size'; +import { IconComponent } from '../icon/icon.component'; import { LinkComponent } from '../link/link.component'; import { OpenInNewTabComponent } from './open-in-new-tab.component'; @@ -33,7 +30,7 @@ describe('Open in new tab component', () => { extras: { skipLocationChange: true } }) }) - ], + ] }); test('Open in new tab component should not be displayed if paramsOrUrl is undefined', () => { @@ -53,22 +50,23 @@ describe('Open in new tab component', () => { }); expect(spectator.query('.open-in-new-tab')).toExist(); expect(spectator.query('ht-link')).toExist(); - // default value of icon size + // Default value of icon size expect(spectator.component.iconSize).toBe(IconSize.Medium); })); - test(`Open in new tab component should contain icon of passed size`, fakeAsync(() => { - spectator = createHost(``, { - hostProps: { - paramsOrUrl: {}, - iconSize: IconSize.Small + spectator = createHost( + ``, + { + hostProps: { + paramsOrUrl: {}, + iconSize: IconSize.Small + } } - }); + ); expect(spectator.query('.open-in-new-tab')).toExist(); expect(spectator.query('ht-link')).toExist(); - // expected value of icon size if passed + // Expected value of icon size if passed expect(spectator.component.iconSize).toBe(IconSize.Small); })); - }); From b9e31b77fa241600521398740a6606126a849817 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Tue, 18 May 2021 19:55:24 +0800 Subject: [PATCH 05/14] fix: lint fixes --- .../src/external/external-url-navigator.ts | 69 ++++++++++--------- .../common/src/utilities/url/url-utilities.ts | 8 +-- 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/projects/common/src/external/external-url-navigator.ts b/projects/common/src/external/external-url-navigator.ts index a57edcf9b..090769710 100644 --- a/projects/common/src/external/external-url-navigator.ts +++ b/projects/common/src/external/external-url-navigator.ts @@ -1,50 +1,51 @@ import { Injectable } from '@angular/core'; import { ActivatedRouteSnapshot, CanActivate } from '@angular/router'; +import { isEmpty } from 'lodash-es'; import { Observable, of } from 'rxjs'; import { - ExternalNavigationPathParams, - ExternalNavigationWindowHandling, - NavigationService + ExternalNavigationPathParams, + ExternalNavigationWindowHandling, + NavigationService } from '../navigation/navigation.service'; import { assertUnreachable } from '../utilities/lang/lang-utils'; import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; @Injectable({ providedIn: 'root' }) export class ExternalUrlNavigator implements CanActivate { - public constructor(private readonly navService: NavigationService) { } + public constructor(private readonly navService: NavigationService) {} - public canActivate(route: ActivatedRouteSnapshot): Observable { - const url = route.paramMap.get(ExternalNavigationPathParams.Url); - const queryParamString = getQueryParamStringFromObject(route.queryParams ?? {}); - const encodedUrl = url ? `${url}${queryParamString ? `?${queryParamString}` : ``}` : url; + public canActivate(route: ActivatedRouteSnapshot): Observable { + const url = route.paramMap.get(ExternalNavigationPathParams.Url); + const queryParamString = getQueryParamStringFromObject(route.queryParams ?? {}); + const encodedUrl = isEmpty(url) ? url : `${url}${queryParamString ? `?${queryParamString}` : ``}`; - const windowHandling = route.paramMap.has(ExternalNavigationPathParams.WindowHandling) - ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) - : undefined; - if (encodedUrl !== null && encodedUrl.length > 0) { - this.navigateToUrl(encodedUrl, windowHandling); - } else { - this.navService.navigateBack(); - } + const windowHandling = route.paramMap.has(ExternalNavigationPathParams.WindowHandling) + ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) + : undefined; + if (encodedUrl !== null && encodedUrl.length > 0) { + this.navigateToUrl(encodedUrl, windowHandling); + } else { + this.navService.navigateBack(); + } - return of(false); // Can't navigate, but we've already navigated anyway - } + return of(false); // Can't navigate, but we've already navigated anyway + } - private navigateToUrl( - url: string, - windowHandling: ExternalNavigationWindowHandling = ExternalNavigationWindowHandling.SameWindow - ): void { - window.open(url, this.asWindowName(windowHandling)); - } + private navigateToUrl( + url: string, + windowHandling: ExternalNavigationWindowHandling = ExternalNavigationWindowHandling.SameWindow + ): void { + window.open(url, this.asWindowName(windowHandling)); + } - private asWindowName(windowHandling: ExternalNavigationWindowHandling): string | undefined { - switch (windowHandling) { - case ExternalNavigationWindowHandling.SameWindow: - return '_self'; - case ExternalNavigationWindowHandling.NewWindow: - return undefined; - default: - assertUnreachable(windowHandling); - } - } + private asWindowName(windowHandling: ExternalNavigationWindowHandling): string | undefined { + switch (windowHandling) { + case ExternalNavigationWindowHandling.SameWindow: + return '_self'; + case ExternalNavigationWindowHandling.NewWindow: + return undefined; + default: + assertUnreachable(windowHandling); + } + } } diff --git a/projects/common/src/utilities/url/url-utilities.ts b/projects/common/src/utilities/url/url-utilities.ts index 98724c24b..b75a27fae 100644 --- a/projects/common/src/utilities/url/url-utilities.ts +++ b/projects/common/src/utilities/url/url-utilities.ts @@ -3,7 +3,7 @@ import { Dictionary, isEmpty } from 'lodash'; export const getQueryParamStringFromObject = (params: Dictionary): string => { try { const paramString: string = Object.entries(params) - .map(([key, value]) => (value !== undefined ? `${key}=${value}` : '')) + .map(([key, value]) => `${key}=${value}`) .filter(item => item) .join('&'); @@ -13,11 +13,11 @@ export const getQueryParamStringFromObject = (params: Dictionary => { +export const getQueryParamObjectFromString = (query: string): Dictionary => { try { - string = string[0] === '?' ? string.substring(1) : string; + const queryString = query[0] === '?' ? query.substring(1) : query; - return string.split('&').reduce((acc: Dictionary, item: string): Dictionary => { + return queryString.split('&').reduce((acc: Dictionary, item: string): Dictionary => { const [key, value] = item.split('='); acc[key] = value; From 4266e8bc6fc0d087ab147362454a28ae59ddea77 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Wed, 19 May 2021 11:17:13 +0800 Subject: [PATCH 06/14] fix: pr suggested changes --- package.json | 2 +- .../src/utilities/url/url-utilities.spec.ts | 42 ------------------- .../src/utilities/url/url-utilities.test.ts | 13 ++++++ .../common/src/utilities/url/url-utilities.ts | 37 ++++------------ 4 files changed, 23 insertions(+), 71 deletions(-) delete mode 100644 projects/common/src/utilities/url/url-utilities.spec.ts create mode 100644 projects/common/src/utilities/url/url-utilities.test.ts diff --git a/package.json b/package.json index 5b9ef1fe3..3f5bfd39e 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,6 @@ "graphql": "^15.5.0", "graphql-tag": "^2.12.3", "iso8601-duration": "^1.3.0", - "lodash": "^4.17.21", "lodash-es": "^4.17.21", "rxjs": "~6.6.7", "tslib": "^2.2.0", @@ -102,6 +101,7 @@ "jest-html-reporter": "^3.2.0", "jest-junit": "^12.0.0", "jest-preset-angular": "^8.4.0", + "lodash": "^4.17.21", "ng-mocks": "^11.10.0", "ng-packagr": "^11.2.4", "prettier": "^2.2.1", diff --git a/projects/common/src/utilities/url/url-utilities.spec.ts b/projects/common/src/utilities/url/url-utilities.spec.ts deleted file mode 100644 index dcae330bd..000000000 --- a/projects/common/src/utilities/url/url-utilities.spec.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { Params } from '@angular/router'; -import { getQueryParamObjectFromString, getQueryParamStringFromObject } from './url-utilities'; - -describe('getQueryParamStringFromObject', () => { - it('should return string from a string dictionary', () => { - const params: Params = {}; - expect(getQueryParamStringFromObject(params)).toBe(''); - params.a = undefined; - expect(getQueryParamStringFromObject(params)).toBe(''); - params.a = true; - expect(getQueryParamStringFromObject(params)).toBe(`a=true`); - params.a = false; - expect(getQueryParamStringFromObject(params)).toBe(`a=false`); - params.b = false; - expect(getQueryParamStringFromObject(params)).toBe(`a=false&b=false`); - params.a = 'value_1'; - params.b = 'value_2'; - expect(getQueryParamStringFromObject(params)).toBe(`a=value_1&b=value_2`); - }); -}); - -describe('getQueryParamObjectFromString', () => { - it('should return object from a string', () => { - let queryParam = '?queryparam1=value'; - expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); - expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value'); - - queryParam = '?queryparam1='; - expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); - expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe(''); - - queryParam = 'queryparam1=value'; - expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); - expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value'); - - queryParam = '?queryparam1=value1&queryparam2=value2'; - expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam1'); - expect(getQueryParamObjectFromString(queryParam).queryparam1).toBe('value1'); - expect(getQueryParamObjectFromString(queryParam)).toHaveProperty('queryparam2'); - expect(getQueryParamObjectFromString(queryParam).queryparam2).toBe('value2'); - }); -}); diff --git a/projects/common/src/utilities/url/url-utilities.test.ts b/projects/common/src/utilities/url/url-utilities.test.ts new file mode 100644 index 000000000..6d7123316 --- /dev/null +++ b/projects/common/src/utilities/url/url-utilities.test.ts @@ -0,0 +1,13 @@ +import { Params } from '@angular/router'; +import { getQueryParamStringFromObject } from './url-utilities'; + +describe('getQueryParamStringFromObject', () => { + test('should return string from a string dictionary', () => { + const params: Params = {}; + expect(getQueryParamStringFromObject(params)).toBe(''); + params.a = 'value_1'; + expect(getQueryParamStringFromObject(params)).toBe('a=value_1'); + params.b = 'value_2'; + expect(getQueryParamStringFromObject(params)).toBe(`a=value_1&b=value_2`); + }); +}); diff --git a/projects/common/src/utilities/url/url-utilities.ts b/projects/common/src/utilities/url/url-utilities.ts index b75a27fae..74a9f9973 100644 --- a/projects/common/src/utilities/url/url-utilities.ts +++ b/projects/common/src/utilities/url/url-utilities.ts @@ -1,29 +1,10 @@ -import { Dictionary, isEmpty } from 'lodash'; - -export const getQueryParamStringFromObject = (params: Dictionary): string => { - try { - const paramString: string = Object.entries(params) - .map(([key, value]) => `${key}=${value}`) - .filter(item => item) - .join('&'); - - return isEmpty(paramString) ? '' : paramString; - } catch (error) { - return ``; - } -}; - -export const getQueryParamObjectFromString = (query: string): Dictionary => { - try { - const queryString = query[0] === '?' ? query.substring(1) : query; - - return queryString.split('&').reduce((acc: Dictionary, item: string): Dictionary => { - const [key, value] = item.split('='); - acc[key] = value; - - return acc; - }, {}); - } catch (error) { - return {}; - } +import { Params } from '@angular/router'; +import { HttpParams } from '@angular/common/http'; + +export const getQueryParamStringFromObject = (params: Params): string => { + let urlParams = new HttpParams(); + Object.entries(params).forEach(([key, value]) => { + urlParams = urlParams.set(key, value); + }); + return urlParams.toString(); }; From 616931126661b9061e44c9321a9886cb22b537cd Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Wed, 19 May 2021 12:11:49 +0800 Subject: [PATCH 07/14] fix: pr suggested changes --- projects/common/src/external/external-url-navigator.ts | 5 ++--- projects/common/src/time/time-range.service.ts | 4 ++-- projects/common/src/utilities/url/url-utilities.ts | 3 ++- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/projects/common/src/external/external-url-navigator.ts b/projects/common/src/external/external-url-navigator.ts index 090769710..502db9601 100644 --- a/projects/common/src/external/external-url-navigator.ts +++ b/projects/common/src/external/external-url-navigator.ts @@ -15,15 +15,14 @@ export class ExternalUrlNavigator implements CanActivate { public constructor(private readonly navService: NavigationService) {} public canActivate(route: ActivatedRouteSnapshot): Observable { - const url = route.paramMap.get(ExternalNavigationPathParams.Url); + const encodedUrl = route.paramMap.get(ExternalNavigationPathParams.Url); const queryParamString = getQueryParamStringFromObject(route.queryParams ?? {}); - const encodedUrl = isEmpty(url) ? url : `${url}${queryParamString ? `?${queryParamString}` : ``}`; const windowHandling = route.paramMap.has(ExternalNavigationPathParams.WindowHandling) ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) : undefined; if (encodedUrl !== null && encodedUrl.length > 0) { - this.navigateToUrl(encodedUrl, windowHandling); + this.navigateToUrl(`${encodedUrl}${isEmpty(queryParamString) ? `?${queryParamString}` : ``}`, windowHandling); } else { this.navService.navigateBack(); } diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index 53fa2d600..a297462c7 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,5 +1,5 @@ import { Injectable } from '@angular/core'; -import { Dictionary } from 'lodash'; +import { Params } from '@angular/router'; import { isEmpty } from 'lodash-es'; import { EMPTY, ReplaySubject } from 'rxjs'; import { catchError, defaultIfEmpty, filter, map, switchMap, take } from 'rxjs/operators'; @@ -40,7 +40,7 @@ export class TimeRangeService { return this.navigationService.getAbsoluteCurrentUrl().replace(timeRangeParam, newTimeRangeParam); } - public getTimeRangeParams(): Dictionary { + public getTimeRangeParams(): Params { const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, ''); return { diff --git a/projects/common/src/utilities/url/url-utilities.ts b/projects/common/src/utilities/url/url-utilities.ts index 74a9f9973..40ec513ed 100644 --- a/projects/common/src/utilities/url/url-utilities.ts +++ b/projects/common/src/utilities/url/url-utilities.ts @@ -1,10 +1,11 @@ -import { Params } from '@angular/router'; import { HttpParams } from '@angular/common/http'; +import { Params } from '@angular/router'; export const getQueryParamStringFromObject = (params: Params): string => { let urlParams = new HttpParams(); Object.entries(params).forEach(([key, value]) => { urlParams = urlParams.set(key, value); }); + return urlParams.toString(); }; From 101e0ce6863ed5ad7efb37587f03665aae187a67 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Wed, 19 May 2021 12:23:09 +0800 Subject: [PATCH 08/14] fix: lint fixes --- projects/common/src/external/external-url-navigator.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/projects/common/src/external/external-url-navigator.ts b/projects/common/src/external/external-url-navigator.ts index 502db9601..94341f218 100644 --- a/projects/common/src/external/external-url-navigator.ts +++ b/projects/common/src/external/external-url-navigator.ts @@ -1,6 +1,5 @@ import { Injectable } from '@angular/core'; import { ActivatedRouteSnapshot, CanActivate } from '@angular/router'; -import { isEmpty } from 'lodash-es'; import { Observable, of } from 'rxjs'; import { ExternalNavigationPathParams, @@ -22,7 +21,7 @@ export class ExternalUrlNavigator implements CanActivate { ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) : undefined; if (encodedUrl !== null && encodedUrl.length > 0) { - this.navigateToUrl(`${encodedUrl}${isEmpty(queryParamString) ? `?${queryParamString}` : ``}`, windowHandling); + this.navigateToUrl(`${encodedUrl}${queryParamString !== '' ? `?${queryParamString}` : ``}`, windowHandling); } else { this.navService.navigateBack(); } From 532fe1f7b303e40bfae63ab4929e82ed2d6c08d2 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Wed, 19 May 2021 14:28:06 +0800 Subject: [PATCH 09/14] fix: pr suggested changes --- .../external/external-url-navigator.test.ts | 19 +------------------ .../src/external/external-url-navigator.ts | 5 +---- .../src/navigation/navigation.service.ts | 12 +++++++++--- .../common/src/utilities/url/url-utilities.ts | 10 ++-------- .../open-in-new-tab.component.ts | 11 +++++++---- 5 files changed, 20 insertions(+), 37 deletions(-) diff --git a/projects/common/src/external/external-url-navigator.test.ts b/projects/common/src/external/external-url-navigator.test.ts index 5872be832..230a654aa 100644 --- a/projects/common/src/external/external-url-navigator.test.ts +++ b/projects/common/src/external/external-url-navigator.test.ts @@ -1,4 +1,4 @@ -import { ActivatedRouteSnapshot, convertToParamMap, Params } from '@angular/router'; +import { ActivatedRouteSnapshot, convertToParamMap } from '@angular/router'; import { createServiceFactory, mockProvider, SpectatorService } from '@ngneat/spectator/jest'; import { ExternalNavigationPathParams, @@ -57,21 +57,4 @@ describe('External URL navigator', () => { expect(window.open).toHaveBeenNthCalledWith(2, 'https://www.bing.com', undefined); expect(spectator.inject(NavigationService).navigateBack).not.toHaveBeenCalled(); }); - - test('navigates when a url is provided with query params', () => { - const queryParams: Params = { - time: '1h' - }; - // tslint:disable-next-line: no-object-literal-type-assertion - spectator.service.canActivate({ - paramMap: convertToParamMap({ - [ExternalNavigationPathParams.Url]: 'https://www.bing.com', - [ExternalNavigationPathParams.WindowHandling]: ExternalNavigationWindowHandling.NewWindow - }), - queryParams: queryParams - } as ActivatedRouteSnapshot); - - expect(window.open).toHaveBeenCalledWith('https://www.bing.com?time=1h', undefined); - expect(spectator.inject(NavigationService).navigateBack).not.toHaveBeenCalled(); - }); }); diff --git a/projects/common/src/external/external-url-navigator.ts b/projects/common/src/external/external-url-navigator.ts index 94341f218..98bba6439 100644 --- a/projects/common/src/external/external-url-navigator.ts +++ b/projects/common/src/external/external-url-navigator.ts @@ -7,7 +7,6 @@ import { NavigationService } from '../navigation/navigation.service'; import { assertUnreachable } from '../utilities/lang/lang-utils'; -import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; @Injectable({ providedIn: 'root' }) export class ExternalUrlNavigator implements CanActivate { @@ -15,13 +14,11 @@ export class ExternalUrlNavigator implements CanActivate { public canActivate(route: ActivatedRouteSnapshot): Observable { const encodedUrl = route.paramMap.get(ExternalNavigationPathParams.Url); - const queryParamString = getQueryParamStringFromObject(route.queryParams ?? {}); - const windowHandling = route.paramMap.has(ExternalNavigationPathParams.WindowHandling) ? (route.paramMap.get(ExternalNavigationPathParams.WindowHandling) as ExternalNavigationWindowHandling) : undefined; if (encodedUrl !== null && encodedUrl.length > 0) { - this.navigateToUrl(`${encodedUrl}${queryParamString !== '' ? `?${queryParamString}` : ``}`, windowHandling); + this.navigateToUrl(encodedUrl, windowHandling); } else { this.navService.navigateBack(); } diff --git a/projects/common/src/navigation/navigation.service.ts b/projects/common/src/navigation/navigation.service.ts index 29cc83a83..a4d6cc035 100644 --- a/projects/common/src/navigation/navigation.service.ts +++ b/projects/common/src/navigation/navigation.service.ts @@ -13,10 +13,12 @@ import { UrlSegment, UrlTree } from '@angular/router'; +import { isEmpty } from 'lodash-es'; import { from, Observable, of } from 'rxjs'; import { filter, map, share, skip, take } from 'rxjs/operators'; import { throwIfNil } from '../utilities/lang/lang-utils'; import { Dictionary } from '../utilities/types/types'; +import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; import { TraceRoute } from './trace-route'; @Injectable({ providedIn: 'root' }) @@ -80,17 +82,21 @@ export class NavigationService { if (params.navType === NavigationParamsType.External) { // External + const queryParamString = getQueryParamStringFromObject(params.queryParams ?? {}); + const url = isEmpty(params.url) + ? params.url + : `${params.url}${queryParamString !== '' ? `?${queryParamString}` : ``}`; + return { path: [ '/external', { - [ExternalNavigationPathParams.Url]: params.url, + [ExternalNavigationPathParams.Url]: url, [ExternalNavigationPathParams.WindowHandling]: params.windowHandling } ], extras: { - skipLocationChange: true, // Don't bother showing the updated location, we're going external anyway - queryParams: params.queryParams + skipLocationChange: true // Don't bother showing the updated location, we're going external anyway } }; } diff --git a/projects/common/src/utilities/url/url-utilities.ts b/projects/common/src/utilities/url/url-utilities.ts index 40ec513ed..5a13ddbae 100644 --- a/projects/common/src/utilities/url/url-utilities.ts +++ b/projects/common/src/utilities/url/url-utilities.ts @@ -1,11 +1,5 @@ import { HttpParams } from '@angular/common/http'; import { Params } from '@angular/router'; -export const getQueryParamStringFromObject = (params: Params): string => { - let urlParams = new HttpParams(); - Object.entries(params).forEach(([key, value]) => { - urlParams = urlParams.set(key, value); - }); - - return urlParams.toString(); -}; +export const getQueryParamStringFromObject = (params: Params): string => + new HttpParams({ fromObject: params }).toString(); diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts index 5681e9f61..c376257c7 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts @@ -1,20 +1,23 @@ import { ChangeDetectionStrategy, Component, Input } from '@angular/core'; import { IconType } from '@hypertrace/assets-library'; +import { ExternalNavigationParamsNew } from '@hypertrace/common'; import { IconSize } from '../icon/icon-size'; -import { LinkComponent } from '../link/link.component'; @Component({ selector: 'ht-open-in-new-tab', changeDetection: ChangeDetectionStrategy.OnPush, template: ` -
+
- +
` }) -export class OpenInNewTabComponent extends LinkComponent { +export class OpenInNewTabComponent { + @Input() + public paramsOrUrl?: ExternalNavigationParamsNew | string; + @Input() public iconSize: IconSize = IconSize.Medium; } From 65f9181cc51d66bd2526a8a5bc2d69762abefcda Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Thu, 20 May 2021 23:08:35 +0800 Subject: [PATCH 10/14] feat: pr suggested changes --- .../src/navigation/navigation.service.ts | 30 ++++++++++++------- .../open-in-new-tab.component.ts | 4 +-- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/projects/common/src/navigation/navigation.service.ts b/projects/common/src/navigation/navigation.service.ts index a4d6cc035..5579fef3b 100644 --- a/projects/common/src/navigation/navigation.service.ts +++ b/projects/common/src/navigation/navigation.service.ts @@ -13,12 +13,10 @@ import { UrlSegment, UrlTree } from '@angular/router'; -import { isEmpty } from 'lodash-es'; import { from, Observable, of } from 'rxjs'; import { filter, map, share, skip, take } from 'rxjs/operators'; import { throwIfNil } from '../utilities/lang/lang-utils'; import { Dictionary } from '../utilities/types/types'; -import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; import { TraceRoute } from './trace-route'; @Injectable({ providedIn: 'root' }) @@ -75,6 +73,21 @@ export class NavigationService { return this.currentParamMap.getAll(parameterName); } + public constructExternalUrl(urlString: string): string { + const inputUrlTree: UrlTree = this.router.parseUrl(urlString); + const globalQueryParams: Params = {}; + + this.globalQueryParams.forEach(key => { + const paramValue = this.getQueryParameter(key, ''); + if (paramValue !== '') { + globalQueryParams[key] = paramValue; + } + }); + + inputUrlTree.queryParams = { ...inputUrlTree.queryParams, ...globalQueryParams }; + return this.router.serializeUrl(inputUrlTree); + } + public buildNavigationParams( paramsOrUrl: NavigationParams | string ): { path: NavigationPath; extras?: NavigationExtras } { @@ -82,16 +95,11 @@ export class NavigationService { if (params.navType === NavigationParamsType.External) { // External - const queryParamString = getQueryParamStringFromObject(params.queryParams ?? {}); - const url = isEmpty(params.url) - ? params.url - : `${params.url}${queryParamString !== '' ? `?${queryParamString}` : ``}`; - return { path: [ '/external', { - [ExternalNavigationPathParams.Url]: url, + [ExternalNavigationPathParams.Url]: params.useGlobalParams ? this.constructExternalUrl(params.url) : params.url, [ExternalNavigationPathParams.WindowHandling]: params.windowHandling } ], @@ -311,7 +319,7 @@ export interface QueryParamObject extends Params { export type NavigationPath = string | (string | Dictionary)[]; -export type NavigationParams = InAppNavigationParams | ExternalNavigationParamsNew; +export type NavigationParams = InAppNavigationParams | ExternalNavigationParams; export interface InAppNavigationParams { navType: NavigationParamsType.InApp; path: NavigationPath; @@ -321,11 +329,11 @@ export interface InAppNavigationParams { relativeTo?: ActivatedRoute; } -export interface ExternalNavigationParamsNew { +export interface ExternalNavigationParams { navType: NavigationParamsType.External; url: string; windowHandling: ExternalNavigationWindowHandling; // Currently an enum called NavigationType - queryParams?: QueryParamObject; + useGlobalParams?: boolean; } export const enum ExternalNavigationPathParams { diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts index c376257c7..026cba11f 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.ts @@ -1,6 +1,6 @@ import { ChangeDetectionStrategy, Component, Input } from '@angular/core'; import { IconType } from '@hypertrace/assets-library'; -import { ExternalNavigationParamsNew } from '@hypertrace/common'; +import { ExternalNavigationParams } from '@hypertrace/common'; import { IconSize } from '../icon/icon-size'; @Component({ @@ -16,7 +16,7 @@ import { IconSize } from '../icon/icon-size'; }) export class OpenInNewTabComponent { @Input() - public paramsOrUrl?: ExternalNavigationParamsNew | string; + public paramsOrUrl?: ExternalNavigationParams | string; @Input() public iconSize: IconSize = IconSize.Medium; From 98f65cdd3973180ddd8aecbf35af3ada6ccae3fd Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Thu, 20 May 2021 23:18:08 +0800 Subject: [PATCH 11/14] feat: pr suggested changes --- projects/common/src/navigation/navigation.service.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/projects/common/src/navigation/navigation.service.ts b/projects/common/src/navigation/navigation.service.ts index 5579fef3b..fdec9e880 100644 --- a/projects/common/src/navigation/navigation.service.ts +++ b/projects/common/src/navigation/navigation.service.ts @@ -85,6 +85,7 @@ export class NavigationService { }); inputUrlTree.queryParams = { ...inputUrlTree.queryParams, ...globalQueryParams }; + return this.router.serializeUrl(inputUrlTree); } @@ -99,7 +100,9 @@ export class NavigationService { path: [ '/external', { - [ExternalNavigationPathParams.Url]: params.useGlobalParams ? this.constructExternalUrl(params.url) : params.url, + [ExternalNavigationPathParams.Url]: params.useGlobalParams + ? this.constructExternalUrl(params.url) + : params.url, [ExternalNavigationPathParams.WindowHandling]: params.windowHandling } ], From f59b14f09a154e293573f7f74b838c3f9270d5d0 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Fri, 21 May 2021 13:19:12 +0800 Subject: [PATCH 12/14] fix: merging main --- .../src/open-in-new-tab/open-in-new-tab.component.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts b/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts index f8875be97..0028ba734 100644 --- a/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts +++ b/projects/components/src/open-in-new-tab/open-in-new-tab.component.test.ts @@ -66,7 +66,7 @@ describe('Open in new tab component', () => { ); expect(spectator.query('.open-in-new-tab')).toExist(); expect(spectator.query('ht-link')).toExist(); - // Expected value of icon size if passed + // Expected value of icon size if pass expect(spectator.component.iconSize).toBe(IconSize.Small); })); }); From 8ff748c21769e29d31d81f292ca39f30adde5a6e Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Sat, 22 May 2021 14:07:48 +0800 Subject: [PATCH 13/14] fix: imporving code coverage --- .../src/navigation/navigation.service.test.ts | 53 ++++++++++++++++++- .../common/src/time/time-range.service.ts | 13 +---- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/projects/common/src/navigation/navigation.service.test.ts b/projects/common/src/navigation/navigation.service.test.ts index 6d7a55baa..90ca857e2 100644 --- a/projects/common/src/navigation/navigation.service.test.ts +++ b/projects/common/src/navigation/navigation.service.test.ts @@ -3,7 +3,13 @@ import { Router, UrlSegment } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; import { patchRouterNavigateForTest } from '@hypertrace/test-utils'; import { createServiceFactory, mockProvider, SpectatorService } from '@ngneat/spectator/jest'; -import { NavigationParamsType, NavigationService } from './navigation.service'; +import { + ExternalNavigationPathParams, + ExternalNavigationWindowHandling, + NavigationParams, + NavigationParamsType, + NavigationService +} from './navigation.service'; describe('Navigation Service', () => { const firstChildRouteConfig = { @@ -234,4 +240,49 @@ describe('Navigation Service', () => { ]); expect(spectator.service.getCurrentActivatedRoute().snapshot.queryParams).toEqual({ global: 'foo' }); }); + + test('construct external url in case useGlobalParams is set to true', () => { + const externalNavigationParams: NavigationParams = { + navType: NavigationParamsType.External, + useGlobalParams: true, + url: '/some/internal/path/of/app', + windowHandling: ExternalNavigationWindowHandling.NewWindow + }; + + spectator.service.addQueryParametersToUrl({ time: '1h', environment: 'development' }); + spectator.service.registerGlobalQueryParamKey('time'); + spectator.service.registerGlobalQueryParamKey('environment'); + + externalNavigationParams.useGlobalParams = true; + + expect(Array.isArray(spectator.service.buildNavigationParams(externalNavigationParams).path)).toBe(true); + expect(spectator.service.buildNavigationParams(externalNavigationParams).path[1]).toHaveProperty( + ExternalNavigationPathParams.Url + ); + + let pathParam = spectator.service.buildNavigationParams(externalNavigationParams).path[1]; + expect(typeof pathParam).not.toBe('string'); + + if (typeof pathParam !== 'string') { + expect(pathParam[ExternalNavigationPathParams.Url]).toBe( + `${externalNavigationParams.url}?time=1h&environment=development` + ); + } + + externalNavigationParams.url = '/some/internal/path/of/app?type=json'; + + expect(Array.isArray(spectator.service.buildNavigationParams(externalNavigationParams).path)).toBe(true); + expect(spectator.service.buildNavigationParams(externalNavigationParams).path[1]).toHaveProperty( + ExternalNavigationPathParams.Url + ); + + pathParam = spectator.service.buildNavigationParams(externalNavigationParams).path[1]; + expect(typeof pathParam).not.toBe('string'); + + if (typeof pathParam !== 'string') { + expect(pathParam[ExternalNavigationPathParams.Url]).toBe( + `/some/internal/path/of/app?type=json&time=1h&environment=development` + ); + } + }); }); diff --git a/projects/common/src/time/time-range.service.ts b/projects/common/src/time/time-range.service.ts index a297462c7..f9e96ca8e 100644 --- a/projects/common/src/time/time-range.service.ts +++ b/projects/common/src/time/time-range.service.ts @@ -1,11 +1,9 @@ import { Injectable } from '@angular/core'; -import { Params } from '@angular/router'; import { isEmpty } from 'lodash-es'; import { EMPTY, ReplaySubject } from 'rxjs'; import { catchError, defaultIfEmpty, filter, map, switchMap, take } from 'rxjs/operators'; import { NavigationService } from '../navigation/navigation.service'; import { ReplayObservable } from '../utilities/rxjs/rxjs-utils'; -import { getQueryParamStringFromObject } from '../utilities/url/url-utilities'; import { FixedTimeRange } from './fixed-time-range'; import { RelativeTimeRange } from './relative-time-range'; import { TimeDuration } from './time-duration'; @@ -32,7 +30,8 @@ export class TimeRangeService { } public getShareableCurrentUrl(): string { - const timeRangeParam = getQueryParamStringFromObject(this.getTimeRangeParams()); + const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, ''); + const timeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${timeRangeParamValue}`; const timeRange = this.getCurrentTimeRange(); const fixedTimeRange: FixedTimeRange = TimeRangeService.toFixedTimeRange(timeRange.startTime, timeRange.endTime); const newTimeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${fixedTimeRange.toUrlString()}`; @@ -40,14 +39,6 @@ export class TimeRangeService { return this.navigationService.getAbsoluteCurrentUrl().replace(timeRangeParam, newTimeRangeParam); } - public getTimeRangeParams(): Params { - const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, ''); - - return { - [TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeParamValue - }; - } - public getTimeRangeAndChanges(): ReplayObservable { return this.timeRangeSubject$.asObservable(); } From b5d74ba3132cbffbcdfc7437ce144577adbd0f40 Mon Sep 17 00:00:00 2001 From: Alok Singh Date: Mon, 24 May 2021 11:44:27 +0800 Subject: [PATCH 14/14] fix: removing unused utilities --- projects/common/src/public-api.ts | 3 --- .../common/src/utilities/url/url-utilities.test.ts | 13 ------------- projects/common/src/utilities/url/url-utilities.ts | 5 ----- 3 files changed, 21 deletions(-) delete mode 100644 projects/common/src/utilities/url/url-utilities.test.ts delete mode 100644 projects/common/src/utilities/url/url-utilities.ts diff --git a/projects/common/src/public-api.ts b/projects/common/src/public-api.ts index 83b0f9949..fe2efa56e 100644 --- a/projects/common/src/public-api.ts +++ b/projects/common/src/public-api.ts @@ -112,6 +112,3 @@ export * from './time/time'; // Validators export * from './utilities/validators/email-validator'; - -// URL Utilities -export * from './utilities/url/url-utilities'; diff --git a/projects/common/src/utilities/url/url-utilities.test.ts b/projects/common/src/utilities/url/url-utilities.test.ts deleted file mode 100644 index 6d7123316..000000000 --- a/projects/common/src/utilities/url/url-utilities.test.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { Params } from '@angular/router'; -import { getQueryParamStringFromObject } from './url-utilities'; - -describe('getQueryParamStringFromObject', () => { - test('should return string from a string dictionary', () => { - const params: Params = {}; - expect(getQueryParamStringFromObject(params)).toBe(''); - params.a = 'value_1'; - expect(getQueryParamStringFromObject(params)).toBe('a=value_1'); - params.b = 'value_2'; - expect(getQueryParamStringFromObject(params)).toBe(`a=value_1&b=value_2`); - }); -}); diff --git a/projects/common/src/utilities/url/url-utilities.ts b/projects/common/src/utilities/url/url-utilities.ts deleted file mode 100644 index 5a13ddbae..000000000 --- a/projects/common/src/utilities/url/url-utilities.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { HttpParams } from '@angular/common/http'; -import { Params } from '@angular/router'; - -export const getQueryParamStringFromObject = (params: Params): string => - new HttpParams({ fromObject: params }).toString();