ocket8888 commented on code in PR #7303:
URL: https://github.com/apache/trafficcontrol/pull/7303#discussion_r1086002557
##########
experimental/traffic-portal/src/app/api/physical-location.service.ts:
##########
@@ -23,33 +22,75 @@ import { APIService } from "./base-api.service";
*/
@Injectable()
export class PhysicalLocationService extends APIService {
- public async getPhysicalLocations(idOrName: number | string):
Promise<PhysicalLocation>;
- public async getPhysicalLocations(): Promise<Array<PhysicalLocation>>;
+ public async getPhysicalLocations():
Promise<Array<ResponsePhysicalLocation>>;
+ public async getPhysicalLocations(nameOrID: string | number):
Promise<ResponsePhysicalLocation>;
+
/**
- * Gets one or all PhysicalLocations from Traffic Ops
+ * Gets an array of physicalLocations from Traffic Ops.
Review Comment:
The response is not always an array - and the object is
`[Response]PhysicalLocation` and the concept it represents is a "Physical
Location," either of which is more appropriate than `physicalLocation`.
##########
experimental/traffic-portal/src/app/api/physical-location.service.ts:
##########
@@ -23,33 +22,75 @@ import { APIService } from "./base-api.service";
*/
@Injectable()
export class PhysicalLocationService extends APIService {
- public async getPhysicalLocations(idOrName: number | string):
Promise<PhysicalLocation>;
- public async getPhysicalLocations(): Promise<Array<PhysicalLocation>>;
+ public async getPhysicalLocations():
Promise<Array<ResponsePhysicalLocation>>;
+ public async getPhysicalLocations(nameOrID: string | number):
Promise<ResponsePhysicalLocation>;
+
/**
- * Gets one or all PhysicalLocations from Traffic Ops
+ * Gets an array of physicalLocations from Traffic Ops.
*
- * @param idOrName Either the integral, unique identifier (number) or
name (string) of a single PhysicalLocation to be returned.
- * @returns The requested PhysicalLocation(s).
+ * @param nameOrID If given, returns only the PhysicalLocation with the
given name
+ * (string) or ID (number).
+ * @returns An Array of PhysicalLocation objects - or a single
PhysicalLocation object if 'nameOrID'
+ * was given.
*/
- public async getPhysicalLocations(idOrName?: number | string):
Promise<PhysicalLocation | Array<PhysicalLocation>> {
+ public async getPhysicalLocations(nameOrID?: string | number):
Promise<Array<ResponsePhysicalLocation> | ResponsePhysicalLocation> {
const path = "phys_locations";
- let prom;
- if (idOrName !== undefined) {
+ if(nameOrID) {
let params;
- switch (typeof idOrName) {
+ switch (typeof nameOrID) {
case "string":
- params = {name: idOrName};
+ params = {name: nameOrID};
break;
case "number":
- params = {id: String(idOrName)};
+ params = {id: String(nameOrID)};
}
- prom = this.get<[PhysicalLocation]>(path, undefined,
params).toPromise().then(
- r => r[0]
- );
- } else {
- prom =
this.get<Array<PhysicalLocation>>(path).toPromise();
+ const r = await
this.get<[ResponsePhysicalLocation]>(path, undefined, params).toPromise();
+ return {...r[0], lastUpdated: new
Date((r[0].lastUpdated as unknown as string).replace("+00", "Z"))};
+
}
- return prom;
+ const physicalLocations = await
this.get<Array<ResponsePhysicalLocation>>(path).toPromise();
+ return physicalLocations.map(
+ d => ({...d, lastUpdated: new Date((d.lastUpdated as
unknown as string).replace("+00", "Z"))})
+ );
+ }
+
+ /**
+ * Replaces the current definition of a physicalLocation with the one
given.
+ *
+ * @param physicalLocation The new physicalLocation.
+ * @returns The updated physicalLocation.
+ */
+ public async updatePhysicalLocation(physicalLocation:
ResponsePhysicalLocation): Promise<ResponsePhysicalLocation> {
+ const path = `phys_locations/${physicalLocation.id}`;
+ const response = await this.put<ResponsePhysicalLocation>(path,
physicalLocation).toPromise();
+ return {
+ ...response,
+ lastUpdated: new Date((response.lastUpdated as unknown
as string).replace(" ", "T").replace("+00", "Z"))
+ };
+ }
+
+ /**
+ * Creates a new physicalLocation.
+ *
+ * @param physicalLocation The physicalLocation to create.
+ * @returns The created physicalLocation.
+ */
+ public async createPhysicalLocation(physicalLocation:
RequestPhysicalLocation): Promise<ResponsePhysicalLocation> {
+ const response = await
this.post<ResponsePhysicalLocation>("physicalLocations",
physicalLocation).toPromise();
+ return {
+ ...response,
+ lastUpdated: new Date((response.lastUpdated as unknown
as string).replace(" ", "T").replace("+00", "Z"))
+ };
+ }
+
+ /**
+ * Deletes an existing physicalLocation.
+ *
+ * @param id Id of the physicalLocation to delete.
+ * @returns The deleted physicalLocation.
+ */
+ public async deletePhysicalLocation(id: number): Promise<void> {
Review Comment:
Ideally anywhere you can pass an identifier, you should also just be able to
pass the object reference for the thing it identifies.
##########
experimental/traffic-portal/src/app/shared/navigation/tp-sidebar/tp-sidebar.component.ts:
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { NestedTreeControl } from "@angular/cdk/tree";
+import { Component, OnInit, ViewChild } from "@angular/core";
+import { MatSidenav } from "@angular/material/sidenav";
+import { MatTreeNestedDataSource } from "@angular/material/tree";
+import { Router, RouterEvent, Event } from "@angular/router";
+import { filter } from "rxjs/operators";
+
+import { NavigationService, TreeNavNode } from
"src/app/shared/navigation/navigation.service";
+
+/**
+ * TpSidebarComponent is the controller for the sidebar.
+ */
+@Component({
+ selector: "tp-sidebar",
+ styleUrls: ["./tp-sidebar.component.scss"],
+ templateUrl: "./tp-sidebar.component.html",
+})
+export class TpSidebarComponent implements OnInit {
+ public dataSource = new MatTreeNestedDataSource<TreeNavNode>();
+ public treeCtrl = new NestedTreeControl<TreeNavNode>(node =>
node.children);
+
+ public hidden = false;
+ private lastRoute = "";
+ private lastChild?: TreeNavNode;
+
+ /**
+ * Used in the sidebar to ensure the active page is visible.1
+ *
+ * @private
+ */
+ private childToParent = new Map<string, TreeNavNode>();
+
+ @ViewChild("sidenav") public sidenav!: MatSidenav;
+
+ /**
+ * Used by angular to determine if this node should be a nested tree
node.
+ *
+ * @param _ Index of the current node.
+ * @param node Node to test.
+ * @returns If the node has children.
+ */
+ public hasChild(_: number, node: TreeNavNode): boolean {
+ return node.children !== undefined && node.children.length > 0;
+ }
+
+ constructor(private readonly navService: NavigationService, private
readonly route: Router) {
+ }
+
+ /**
+ * Adds to childToParent from the given node.
+ *
+ * @param node The node to map.
+ * @private
+ */
+ private mapChild(node: TreeNavNode): void {
+ if(node.children !== undefined) {
+ for(const child of node.children) {
+ this.childToParent.set(this.nodeHandle(child),
node);
+ this.mapChild(child);
+ }
+ }
+ }
+
+ /**
+ * Angular lifecycle hook.
+ */
+ public ngOnInit(): void {
+ this.navService.sidebarHidden.subscribe(hidden => {
+ if(this.sidenav) {
+ this.hidden = hidden;
+ if(hidden && this.sidenav.opened) {
+ this.sidenav.close().catch(err => {
+ console.error(`Unable to close
sidebar: ${err}`);
+ });
+ } else if (!this.sidenav.opened) {
+ this.sidenav.open().catch(err => {
+ console.error(`Unable to open
sidebar: ${err}`);
+ });
+ }
+ }
+ });
+ this.navService.sidebarNavs.subscribe(navs => {
+ this.dataSource.data = navs;
+
+ this.childToParent = new Map<string, TreeNavNode>();
+ navs.forEach(nav => this.mapChild(nav));
+ });
+
+ this.route.events.pipe(
+ filter((e: Event): e is RouterEvent => e instanceof
RouterEvent)
+ ).subscribe((e: RouterEvent) => {
+ const path = e.url.split("?")[0];
+ if(path !== this.lastRoute) {
+ this.lastRoute = path;
+ for(const node of this.dataSource.data) {
+ for(const child of
this.treeCtrl.getDescendants(node)) {
+ if(child.href === path) {
+ if(this.lastChild) {
+
this.lastChild.active = false;
+ }
+ child.active = true;
+ this.lastChild = child;
+
this.treeCtrl.expand(node);
+ let parent =
this.childToParent.get(this.nodeHandle(child));
+ let depth = 0;
+ while(parent !==
undefined && depth++ < 5) {
+
this.treeCtrl.expand(parent);
+ parent =
this.childToParent.get(this.nodeHandle(parent));
+ }
+ return;
+ }
+ }
+ }
+ }
+ });
+ }
Review Comment:
Surely we could find a way to make use of
[`RouterLinkActive`](https://angular.io/api/router/RouterLinkActive) for this?
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.html:
##########
@@ -49,7 +50,7 @@
<ul>
<li *ngFor="let item of contextMenuItems" role="menuitem">
<ng-container *ngIf="!isAction(item)">
- <a *ngIf="!isDisabled(item)"
[href]="href(item)" [target]="item.newTab ? '_blank' :
'_self'">{{item.name}}</a>
+ <a *ngIf="!isDisabled(item)"
[href]="href(item)" (click)="clickContextMenu($event, item)"
[target]="item.newTab ? '_blank' : '_self'">{{item.name}}</a>
Review Comment:
anchors should not use click event handlers. the way that handler works, the
browser doesn't handle navigation, which essentially renders the `href`
attribute useless.
##########
experimental/traffic-portal/src/app/core/servers/phys-loc/table/phys-loc-table.component.spec.ts:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { ComponentFixture, fakeAsync, TestBed, tick } from
"@angular/core/testing";
+import { MatDialogModule } from "@angular/material/dialog";
+import { RouterTestingModule } from "@angular/router/testing";
+import { BehaviorSubject } from "rxjs";
+
+import { APITestingModule } from "src/app/api/testing";
+import { PhysLocTableComponent } from
"src/app/core/servers/phys-loc/table/phys-loc-table.component";
+import { CurrentUserService } from
"src/app/shared/currentUser/current-user.service";
+
+describe("PhysLocTableComponent", () => {
+ let component: PhysLocTableComponent;
+ let fixture: ComponentFixture<PhysLocTableComponent>;
+
+ beforeEach(async () => {
+ await TestBed.configureTestingModule({
+ declarations: [ PhysLocTableComponent ],
+ imports: [ APITestingModule, RouterTestingModule,
MatDialogModule ],
+ providers: [
+ {
+ provide: CurrentUserService,
+ useValue: {
+ currentUser: {
+ },
+ hasPermission: (): true => true,
+ userChanged: new
BehaviorSubject({})
+ }
+ }
+ ]
+ })
Review Comment:
Do you need to mock this service? I don't think you need to mock this
service.
##########
experimental/traffic-portal/src/app/core/servers/phys-loc/detail/phys-loc-detail.component.html:
##########
@@ -0,0 +1,78 @@
+<!--
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+
+<mat-card>
+ <tp-loading *ngIf="!physLocation"></tp-loading>
+ <form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="physLocation">
+ <mat-card-content>
+ <mat-form-field>
+ <mat-label>Name</mat-label>
+ <input matInput type="text" name="name"
required [(ngModel)]="physLocation.name" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>Short Name</mat-label>
+ <input matInput type="text" name="shortName"
required [(ngModel)]="physLocation.shortName" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>Address</mat-label>
+ <input matInput type="text" name="address"
required [(ngModel)]="physLocation.address" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>City</mat-label>
+ <input matInput type="text" name="city"
required [(ngModel)]="physLocation.city" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>State</mat-label>
+ <input matInput type="text" name="state"
required [(ngModel)]="physLocation.state" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>Postal Code</mat-label>
+ <input matInput type="text" name="postalCode"
required [(ngModel)]="physLocation.zip" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>Person of Contact</mat-label>
Review Comment:
The phrase is ["**Point** of
Contact"](https://en.wiktionary.org/wiki/point_of_contact) not "**Person** of
Content".
##########
experimental/traffic-portal/src/app/core/servers/phys-loc/table/phys-loc-table.component.ts:
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { Component, OnInit } from "@angular/core";
+import { FormControl } from "@angular/forms";
+import { MatDialog } from "@angular/material/dialog";
+import { ActivatedRoute } from "@angular/router";
+import { BehaviorSubject } from "rxjs";
+import { ResponsePhysicalLocation } from "trafficops-types";
+
+import { CacheGroupService, PhysicalLocationService } from "src/app/api";
+import { CurrentUserService } from
"src/app/shared/currentUser/current-user.service";
+import { DecisionDialogComponent } from
"src/app/shared/dialogs/decision-dialog/decision-dialog.component";
+import { ContextMenuActionEvent, ContextMenuItem } from
"src/app/shared/generic-table/generic-table.component";
+import { NavigationService } from
"src/app/shared/navigation/navigation.service";
+
+/**
+ * PHysLocTableComponent is the controller for the phys loc table.
+ */
+@Component({
+ selector: "tp-phys-loc-table",
+ styleUrls: ["./phys-loc-table.component.scss"],
+ templateUrl: "./phys-loc-table.component.html"
+})
+export class PhysLocTableComponent implements OnInit {
+ /** All the physical locations which should appear in the table. */
+ public physLocations: Promise<Array<ResponsePhysicalLocation>>;
+
+ /** Definitions of the table's columns according to the ag-grid API */
+ public columnDefs = [{
+ field: "name",
+ headerName: "Name"
+ }, {
+ field: "id",
+ headerName: "ID",
+ hide: true
+ }, {
+ field: "shortName",
+ headerName: "Short Name"
Review Comment:
I strongly suggest we hide the "short name" by default
##########
experimental/traffic-portal/src/app/api/testing/physical-location.service.ts:
##########
@@ -12,56 +12,105 @@
* limitations under the License.
*/
import { Injectable } from "@angular/core";
-
-import type { PhysicalLocation } from "src/app/models";
+import { RequestPhysicalLocation, ResponsePhysicalLocation } from
"trafficops-types";
/**
* PhysicalLocationService exposes API functionality relating to
PhysicalLocations.
*/
@Injectable()
export class PhysicalLocationService {
- private readonly locs = [
- {
- address: "1600 Pennsylvania Avenue NW",
- city: "Washington",
- comments: "",
- email: "",
- id: 1,
- lastUpdated: new Date(),
- name: "test",
- phone: "",
- poc: "",
- region: "Washington, D.C",
- regionId: 1,
- shortName: "test",
- state: "DC",
- zip: "20500"
- }
- ];
+ private lastID = 1;
+ private readonly physicalLocations: Array<ResponsePhysicalLocation> = [{
+ address: "street",
+ city: "city",
+ comments: null,
+ email: null,
+ id: 1,
+ lastUpdated: new Date(),
+ name: "phys",
+ phone: null,
+ poc: null,
+ region: "Region",
+ regionId: 1,
+ shortName: "short",
+ state: "st",
+ zip: "0000"
+ }];
+
+ public async getPhysicalLocations():
Promise<Array<ResponsePhysicalLocation>>;
+ public async getPhysicalLocations(nameOrID: string | number):
Promise<ResponsePhysicalLocation>;
- public async getPhysicalLocations(idOrName: number | string):
Promise<PhysicalLocation>;
- public async getPhysicalLocations(): Promise<Array<PhysicalLocation>>;
/**
- * Gets one or all PhysicalLocations from Traffic Ops
+ * Gets an array of physicalLocations from Traffic Ops.
*
- * @param idOrName Either the integral, unique identifier (number) or
name (string) of a single PhysicalLocation to be returned.
- * @returns The requested PhysicalLocation(s).
+ * @param nameOrID If given, returns only the ResponsePhysicalLocation
with the given name
+ * (string) or ID (number).
+ * @returns An Array of ResponsePhysicalLocation objects - or a single
ResponsePhysicalLocation object if 'nameOrID'
+ * was given.
*/
- public async getPhysicalLocations(idOrName?: number | string):
Promise<PhysicalLocation | Array<PhysicalLocation>> {
- if (idOrName !== undefined) {
- let loc;
- switch (typeof idOrName) {
+ public async getPhysicalLocations(nameOrID?: string | number):
Promise<Array<ResponsePhysicalLocation> | ResponsePhysicalLocation> {
+ if(nameOrID) {
+ let physicalLocation;
+ switch (typeof nameOrID) {
case "string":
- loc = this.locs.find(l=>l.name ===
idOrName);
+ physicalLocation =
this.physicalLocations.find(d=>d.name === nameOrID);
break;
case "number":
- loc = this.locs.find(l=>l.id ===
idOrName);
+ physicalLocation =
this.physicalLocations.find(d=>d.id === nameOrID);
}
- if (!loc) {
- throw new Error(`no such Physical Location:
${idOrName}`);
+ if (!physicalLocation) {
+ throw new Error(`no such PhysicalLocation:
${nameOrID}`);
}
- return loc;
+ return physicalLocation;
+ }
+ return this.physicalLocations;
+ }
+
+ /**
+ * Replaces the current definition of a physicalLocation with the one
given.
+ *
+ * @param physicalLocation The new physicalLocation.
+ * @returns The updated physicalLocation.
+ */
+ public async updatePhysicalLocation(physicalLocation:
ResponsePhysicalLocation): Promise<ResponsePhysicalLocation> {
+ const id = this.physicalLocations.findIndex(d => d.id ===
physicalLocation.id);
+ if (id === -1) {
+ throw new Error(`no such PhysicalLocation:
${physicalLocation.id}`);
+ }
+ this.physicalLocations[id] = physicalLocation;
+ return physicalLocation;
+ }
+
+ /**
+ * Creates a new physicalLocation.
+ *
+ * @param physicalLocation The physicalLocation to create.
+ * @returns The created physicalLocation.
+ */
+ public async createPhysicalLocation(physicalLocation:
RequestPhysicalLocation): Promise<ResponsePhysicalLocation> {
+ return {
+ ...physicalLocation,
+ comments: physicalLocation.comments ?? null,
+ email: physicalLocation.email ?? null,
+ id: ++this.lastID,
+ lastUpdated: new Date(),
+ phone: physicalLocation.phone ?? null,
+ poc: physicalLocation.poc ?? null,
+ region: ""
+ };
+ }
+
+ /**
+ * Deletes an existing physicalLocation.
+ *
+ * @param id Id of the physicalLocation to delete.
+ * @returns The deleted physicalLocation.
+ */
+ public async deletePhysicalLocation(id: number): Promise<void> {
+ const index = this.physicalLocations.findIndex(d => d.id ===
id);
+ if (index === -1) {
+ throw new Error(`no such PhysicalLocation: ${id}`);
}
- return this.locs;
+ return;
Review Comment:
`return` is unnecessary here
Also, this doesn't look like it actually removes the passed Physical
Location from the service's collection.
##########
experimental/traffic-portal/src/app/core/servers/phys-loc/detail/phys-loc-detail.component.html:
##########
@@ -0,0 +1,78 @@
+<!--
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+
+<mat-card>
+ <tp-loading *ngIf="!physLocation"></tp-loading>
+ <form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="physLocation">
+ <mat-card-content>
+ <mat-form-field>
+ <mat-label>Name</mat-label>
+ <input matInput type="text" name="name"
required [(ngModel)]="physLocation.name" />
+ </mat-form-field>
+ <mat-form-field>
+ <mat-label>Short Name</mat-label>
Review Comment:
I despise the fact that things have "short names". I get that it's part of
the API and you're faithfully recreating that model here, but idk if you
noticed but with Cache Groups I hide short name and just make it always equal
to the name. It's not used for anything at all and shouldn't exist IMO. But do
what you think is best.
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.ts:
##########
@@ -629,7 +660,7 @@ export class GenericTableComponent<T> implements OnInit,
OnDestroy {
}
/**
- * Builds a link for a link context menu item.
+ * Builds a link for a link context menu item. Used to prevent page
reload when navigating on the same tab.
Review Comment:
it's actually not used for anything because the href it builds is never used
because you're preventing default behavior on the click event
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
