Skip to content
9 changes: 8 additions & 1 deletion projects/common/src/navigation/navigation.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' })
Expand Down Expand Up @@ -80,11 +82,16 @@ export class NavigationService {

if (params.navType === NavigationParamsType.External) {
// External
const queryParamString = getQueryParamStringFromObject(params.queryParams ?? {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to let angular do any url building for us (such as url tree). This would break if we passed it a a url that contained query params, since it wouldn't merge. Say http://website.com?param1=value and a second param via query params { param2=value }, we'd get http://website.com?param1=value?param2=value instead of http://website.com?param1=value&param2=value.

I think you can combine router.parseUrl, router.serializeUrl and maybe router.createUrlTree to get all the right behavior here.

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
}
],
Expand Down
3 changes: 3 additions & 0 deletions projects/common/src/public-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,6 @@ export * from './time/time';

// Validators
export * from './utilities/validators/email-validator';

// URL Utilities
export * from './utilities/url/url-utilities';
13 changes: 11 additions & 2 deletions projects/common/src/time/time-range.service.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
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';
Expand All @@ -30,15 +32,22 @@ 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 = 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()}`;

return this.navigationService.getAbsoluteCurrentUrl().replace(timeRangeParam, newTimeRangeParam);
}

public getTimeRangeParams(): Params {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the fuller picture is coming together here for me - I'm guessing this was left public to later use as query params in an open in new tab case that should maintain the time range, and that's also why queryParams handling was added in nav service. The problem here is we do not want callers to be concerned about this. Time range is not the only global param that needs to be preserved, and we don't want callers to even have to be aware of global params by design.

My suggestion would be to

  • revert the changes here (or at least make this method private)
  • update NavigationService.buildNavigationParams to use buildQueryParam for relative external urls, similar to what we do in navigateWithinApp
  • remove queryParams from ExternalNavigationParamsNew - it's never used today and shouldn't be for this use case, so no reason to build logic for it

const timeRangeParamValue = this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, '');

return {
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeParamValue
};
}

public getTimeRangeAndChanges(): ReplayObservable<TimeRange> {
return this.timeRangeSubject$.asObservable();
}
Expand Down
13 changes: 13 additions & 0 deletions projects/common/src/utilities/url/url-utilities.test.ts
Original file line number Diff line number Diff line change
@@ -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`);
});
});
5 changes: 5 additions & 0 deletions projects/common/src/utilities/url/url-utilities.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { HttpParams } from '@angular/common/http';
import { Params } from '@angular/router';

export const getQueryParamStringFromObject = (params: Params): string =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed or should we use the below function directly?

Copy link
Contributor Author

@alok-traceable alok-traceable May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I will remove this function as it is not being used anywhere, but It is better to have a separate function like this, firstly consumers didn't have to write a new instance of HttpParams every time, and secondly if later we want to standardize queryParams key-value pair we need to change this function only.

new HttpParams({ fromObject: params }).toString();
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import { fakeAsync } from '@angular/core/testing';
import {
ExternalNavigationWindowHandling,
NavigationParamsType,
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 { ButtonComponent } from '../button/button.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';

describe('Open in new tab component', () => {
Expand All @@ -16,42 +13,60 @@ 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('<ht-open-in-new-tab></ht-open-in-new-tab>');

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(`<ht-open-in-new-tab [paramsOrUrl]="paramsOrUrl"></ht-open-in-new-tab>`, {
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('<ht-open-in-new-tab [url]="url"></ht-open-in-new-tab>', {
test(`Open in new tab component should exist if paramsOrUrl is not undefined`, fakeAsync(() => {
spectator = createHost(`<ht-open-in-new-tab [paramsOrUrl]="paramsOrUrl"></ht-open-in-new-tab>`, {
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(
`<ht-open-in-new-tab [paramsOrUrl]="paramsOrUrl" [iconSize]="iconSize" ></ht-open-in-new-tab>`,
{
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);
}));
});
Original file line number Diff line number Diff line change
@@ -1,46 +1,23 @@
import { ChangeDetectionStrategy, Component, Input } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import {
ExternalNavigationWindowHandling,
NavigationParamsType,
NavigationService,
TimeRangeService
} from '@hypertrace/common';
import { ButtonSize, ButtonStyle } from '../button/button';
import { ExternalNavigationParamsNew } from '@hypertrace/common';
import { IconSize } from '../icon/icon-size';

@Component({
selector: 'ht-open-in-new-tab',
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div class="open-in-new-tab" htTooltip="Open in a new tab">
<ht-button
class="open-in-new-tab-button"
display="${ButtonStyle.Outlined}"
[size]="this.size"
icon="${IconType.OpenInNewTab}"
(click)="this.openInNewTab()"
></ht-button>
<div *ngIf="this.paramsOrUrl" class="open-in-new-tab" htTooltip="Open in a new tab">
<ht-link [paramsOrUrl]="this.paramsOrUrl">
<ht-icon icon="${IconType.OpenInNewTab}" [size]="this.iconSize"></ht-icon>
</ht-link>
</div>
`
})
export class OpenInNewTabComponent {
@Input()
public size?: ButtonSize = ButtonSize.Small;
public paramsOrUrl?: ExternalNavigationParamsNew | string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still have the same default behavior of the current url?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Because this component is named as OpenInNewTab, i feel it should be able to take bothInApp and External params or a url and build a local External Param object. This would mean that no matter what url or params you pass, this component would always open it in a new tab. I think we can build the local param inside ngOnchanges with reasonable defaults for missing properties.
  • Could we please rename ExternalNavigationParamsNew to ExternalNavigationParams ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts? cc @aaron-steinfeld

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the rename - how did that even get in?!

Also, yes - I think ideally the open in new tab component should be able to support and convert inApp params to external ones, but I personally don't feel it needs to hold up this PR, since we can add it later with fully backwards compatible support as it's a union type already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed it when i was migrating to the new interface. But you approved the PR, so....

Copy link
Contributor

@anandtiwary anandtiwary May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can make #1 change separately


@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()
});
}
public iconSize: IconSize = IconSize.Medium;
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
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, TooltipModule, LinkModule, IconModule]
})
export class OpenInNewTabModule {}