ocket8888 commented on code in PR #7480: URL: https://github.com/apache/trafficcontrol/pull/7480#discussion_r1185274838
########## experimental/traffic-portal/src/app/api/parameter.service.ts: ########## @@ -0,0 +1,95 @@ +/* +* 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 { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import {RequestParameter, ResponseParameter} from "trafficops-types"; + +import { APIService } from "./base-api.service"; + +/** + * ParameterService exposes API functionality related to Parameters. + */ +@Injectable() +export class ParameterService extends APIService { + + /** + * Injects the Angular HTTP client service into the parent constructor. + * + * @param http The Angular HTTP client service. + */ + constructor(http: HttpClient) { + super(http); + } + + public async getParameters(idOrName: number | string): Promise<ResponseParameter>; + public async getParameters(): Promise<Array<ResponseParameter>>; + /** + * Retrieves Parameters from the API. + * + * @param idOrName Specify either the integral, unique identifier (number) of a specific Parameter to retrieve, or its name (string). + * @returns The requested Parameter(s). + */ + public async getParameters(idOrName?: number | string): Promise<Array<ResponseParameter> | ResponseParameter> { + const path = "parameters"; + if (idOrName !== undefined) { + let params; + switch (typeof idOrName) { + case "number": + params = {id: String(idOrName)}; Review Comment: you can just leave this as `{id}` ########## experimental/traffic-portal/src/app/api/parameter.service.ts: ########## @@ -0,0 +1,95 @@ +/* +* 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 { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import {RequestParameter, ResponseParameter} from "trafficops-types"; + +import { APIService } from "./base-api.service"; + +/** + * ParameterService exposes API functionality related to Parameters. + */ +@Injectable() +export class ParameterService extends APIService { + + /** + * Injects the Angular HTTP client service into the parent constructor. + * + * @param http The Angular HTTP client service. + */ + constructor(http: HttpClient) { + super(http); + } + + public async getParameters(idOrName: number | string): Promise<ResponseParameter>; + public async getParameters(): Promise<Array<ResponseParameter>>; + /** + * Retrieves Parameters from the API. + * + * @param idOrName Specify either the integral, unique identifier (number) of a specific Parameter to retrieve, or its name (string). + * @returns The requested Parameter(s). + */ + public async getParameters(idOrName?: number | string): Promise<Array<ResponseParameter> | ResponseParameter> { + const path = "parameters"; + if (idOrName !== undefined) { + let params; + switch (typeof idOrName) { + case "number": + params = {id: String(idOrName)}; + break; + case "string": + params = {name: idOrName}; + } + const r = await this.get<[ResponseParameter]>(path, undefined, params).toPromise(); + if (r.length !== 1) { + throw new Error(`Traffic Ops responded with ${r.length} Types by identifier ${idOrName}`); + } + return r[0]; + } + return this.get<Array<ResponseParameter>>(path).toPromise(); + } + + /** + * Deletes an existing parameter. + * + * @param typeOrId Id of the parameter to delete. + * @returns The deleted parameter. + */ + public async deleteParameter(typeOrId: number | ResponseParameter): Promise<ResponseParameter> { Review Comment: responses from this endpoint don't contain a parameter (against API guidelines), so this should be `void` ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -71,6 +71,22 @@ export class ProfileService extends APIService { return this.get<Array<ResponseProfile>>(path).toPromise(); } + /** + * Retrieves Profiles associated with a Parameter from the API. + * + * @param id Specify the integral, unique identifier (number) of a specific Parameter, for which the Profiles are to be retrieved. + * @returns The requested Profile(s). + */ + public async getProfilesByParam(id: number): Promise<Array<ResponseProfile>> { Review Comment: (Almost) Anywhere you can pass the ID of something, you should also just be able to pass the thing itself (a `ResponseParameter` in this case). ########## experimental/traffic-portal/src/app/api/parameter.service.ts: ########## @@ -0,0 +1,95 @@ +/* +* 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 { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import {RequestParameter, ResponseParameter} from "trafficops-types"; + +import { APIService } from "./base-api.service"; + +/** + * ParameterService exposes API functionality related to Parameters. + */ +@Injectable() +export class ParameterService extends APIService { + + /** + * Injects the Angular HTTP client service into the parent constructor. + * + * @param http The Angular HTTP client service. + */ + constructor(http: HttpClient) { + super(http); + } + + public async getParameters(idOrName: number | string): Promise<ResponseParameter>; + public async getParameters(): Promise<Array<ResponseParameter>>; + /** + * Retrieves Parameters from the API. + * + * @param idOrName Specify either the integral, unique identifier (number) of a specific Parameter to retrieve, or its name (string). + * @returns The requested Parameter(s). + */ + public async getParameters(idOrName?: number | string): Promise<Array<ResponseParameter> | ResponseParameter> { + const path = "parameters"; + if (idOrName !== undefined) { + let params; + switch (typeof idOrName) { + case "number": + params = {id: String(idOrName)}; + break; + case "string": + params = {name: idOrName}; + } + const r = await this.get<[ResponseParameter]>(path, undefined, params).toPromise(); + if (r.length !== 1) { + throw new Error(`Traffic Ops responded with ${r.length} Types by identifier ${idOrName}`); + } + return r[0]; + } + return this.get<Array<ResponseParameter>>(path).toPromise(); + } + + /** + * Deletes an existing parameter. + * + * @param typeOrId Id of the parameter to delete. + * @returns The deleted parameter. + */ + public async deleteParameter(typeOrId: number | ResponseParameter): Promise<ResponseParameter> { + const id = typeof(typeOrId) === "number" ? typeOrId : typeOrId.id; + return this.delete<ResponseParameter>(`parameters/${id}`).toPromise(); + } + + /** + * Creates a new parameter. + * + * @param parameter The parameter to create. + * @returns The created parameter. + */ + public async createParameter(parameter: RequestParameter): Promise<ResponseParameter> { + return this.post<ResponseParameter>("parameters", parameter).toPromise(); + } + + /** + * Replaces the current definition of a parameter with the one given. + * + * @param parameter The new parameter. + * @returns The updated parameter. + */ + public async updateParameter(parameter: ResponseParameter): Promise<ResponseParameter> { + const path = `parameters/${parameter.id}`; + return this.put<ResponseParameter>(path, parameter).toPromise(); + } +} Review Comment: I think this service's functionality should just be added to the ProfileService, since Parameters have no meaning outside of their respective Profiles (anymore). ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -71,6 +71,22 @@ export class ProfileService extends APIService { return this.get<Array<ResponseProfile>>(path).toPromise(); } + /** + * Retrieves Profiles associated with a Parameter from the API. + * + * @param id Specify the integral, unique identifier (number) of a specific Parameter, for which the Profiles are to be retrieved. + * @returns The requested Profile(s). + */ + public async getProfilesByParam(id: number): Promise<Array<ResponseProfile>> { + const path = "profiles"; + if (id !== undefined) { + const params = {param: String(id)}; Review Comment: params no longer need to be strings, as above ########## experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.scss: ########## @@ -0,0 +1,26 @@ +/* +* 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 { + margin: 1em auto; + width: 80%; + min-width: 350px; + + mat-card-content { + display: grid; + grid-template-columns: 1fr; + grid-row-gap: 2em; + margin: 1em auto 50px; + } +} Review Comment: This styling is highly repetitive, so instead the component's style URL should just point to [`core/styles/form.page.scss`](https://github.com/apache/trafficcontrol/blob/master/experimental/traffic-portal/src/app/core/styles/form.page.scss) ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, + { + field: "secure", + headerName: "Secure" + }, + { + field: "value", + headerName: "Value" + }, + { + field: "lastUpdated", + headerName: "Last Updated" + } + ]; Review Comment: None of these columns are hidden by default, but I'd argue most are pretty irrelevant to the use-case of your average user. The TPv1 table doesn't include ID or Last Updated at all, so those can probably be hidden. I also suggest hiding "secure" by default, since that's obvious from the value anyway (and hardly used at any rate) ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, Review Comment: this should probably have a valueFormatter to make it more human-readable ########## experimental/traffic-portal/src/app/core/core.module.ts: ########## @@ -92,10 +94,13 @@ export const ROUTES: Routes = [ { component: CoordinatesTableComponent, path: "coordinates" }, { component: TypesTableComponent, path: "types" }, { component: TypeDetailComponent, path: "types/:id"}, + { component: ParametersTableComponent, path: "parameters" }, + { component: ParameterDetailComponent, path: "parameters/:id" }, { component: StatusesTableComponent, path: "statuses" }, { component: StatusDetailsComponent, path: "statuses/:id" }, { component: ISOGenerationFormComponent, path: "iso-gen"}, { component: ProfileTableComponent, path: "profiles"}, + { component: ProfileTableComponent, path: "profiles/param/:id"}, Review Comment: I don't believe this route is actually necessary. You can just implement a custom query string parameter on the profiles table at the normal `profiles` route in pretty much exactly the same way. ########## experimental/traffic-portal/src/app/shared/navigation/navigation.service.ts: ########## @@ -176,6 +166,22 @@ export class NavigationService { } ], name: "Other" + }, { + children: [ + { + href: "/core/types", + name: "Types" + }, + { + href: "/core/profiles", + name: "Profiles" + }, + { + href: "/core/parameters", + name: "Parameters" + } + ], + name: "Configuration" Review Comment: Why move "Configuration" below "Other"? ########## experimental/traffic-portal/nightwatch/globals/globals.ts: ########## @@ -72,10 +73,11 @@ import { RequestStatus, ResponseProfile, RequestProfile, - ProfileType + ProfileType, ResponseParameter, RequestParameter Review Comment: These should be on separate lines, in keeping with the current format. ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -71,6 +71,22 @@ export class ProfileService extends APIService { return this.get<Array<ResponseProfile>>(path).toPromise(); } + /** + * Retrieves Profiles associated with a Parameter from the API. + * + * @param id Specify the integral, unique identifier (number) of a specific Parameter, for which the Profiles are to be retrieved. + * @returns The requested Profile(s). + */ + public async getProfilesByParam(id: number): Promise<Array<ResponseProfile>> { + const path = "profiles"; + if (id !== undefined) { + const params = {param: String(id)}; + const r = await this.get<[ResponseProfile]>(path, undefined, params).toPromise(); Review Comment: This templated type says that this will always be a length = 1 array, but I don't think that's true, in general. Could be empty, or could have a thousand entries. ########## experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.spec.ts: ########## @@ -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. +*/ + +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { MatDialogModule } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { ReplaySubject } from "rxjs"; + +import { APITestingModule } from "src/app/api/testing"; +import { ParameterDetailComponent } from "src/app/core/parameters/detail/parameter-detail.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +describe("ParameterDetailComponent", () => { + let component: ParameterDetailComponent; + let fixture: ComponentFixture<ParameterDetailComponent>; + let route: ActivatedRoute; + let paramMap: jasmine.Spy; + + const navSvc = jasmine.createSpyObj([],{headerHidden: new ReplaySubject<boolean>(), headerTitle: new ReplaySubject<string>()}); + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [ ParameterDetailComponent ], + imports: [ APITestingModule, RouterTestingModule, MatDialogModule ], + providers: [ { provide: NavigationService, useValue: navSvc } ] + }) + .compileComponents(); + + route = TestBed.inject(ActivatedRoute); + paramMap = spyOn(route.snapshot.paramMap, "get"); + paramMap.and.returnValue(null); + fixture = TestBed.createComponent(ParameterDetailComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + expect(paramMap).toHaveBeenCalled(); + }); + + it("new parameter", async () => { + paramMap.and.returnValue("new"); + + fixture = TestBed.createComponent(ParameterDetailComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + await fixture.whenStable(); + expect(paramMap).toHaveBeenCalled(); + expect(component.parameter).not.toBeNull(); + expect(component.parameter.name).toBe(""); + expect(component.new).toBeTrue(); + }); + + it("existing parameter", async () => { + paramMap.and.returnValue("1"); + + fixture = TestBed.createComponent(ParameterDetailComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + await fixture.whenStable(); + expect(paramMap).toHaveBeenCalled(); + expect(component.parameter).not.toBeNull(); + expect(component.parameter.name).toBe("param1"); Review Comment: We shouldn't rely on hard-coding the name of a Parameter with the ID 1. Instead, you can get a Parameter from the ParameterService and set up the paramMap to return its ID, or you could use the ParameterService to create a new Parameter and start from scratch using that. ########## experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.ts: ########## @@ -0,0 +1,115 @@ +/* +* 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 { Location } from "@angular/common"; +import { Component, OnInit } from "@angular/core"; +import { MatDialog } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { ResponseParameter } from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParameterDetailsComponent is the controller for the parameter add/edit form. + */ +@Component({ + selector: "tp-parameters-detail", + styleUrls: ["./parameter-detail.component.scss"], + templateUrl: "./parameter-detail.component.html" +}) +export class ParameterDetailComponent implements OnInit { + public new = false; + public parameter!: ResponseParameter; + public secure = [ + { label: "true", value: true }, + { label: "false", value: false } + ]; + + constructor(private readonly route: ActivatedRoute, private readonly parameterService: ParameterService, + private readonly location: Location, private readonly dialog: MatDialog, private readonly navSvc: NavigationService) { } + + /** + * Angular lifecycle hook where data is initialized. + */ + public async ngOnInit(): Promise<void> { + const ID = this.route.snapshot.paramMap.get("id"); + if (ID === null) { + console.error("missing required route parameter 'id'"); + return; + } + + if (ID === "new") { + this.navSvc.headerTitle.next("New Parameter"); + this.new = true; + this.parameter = { + configFile: "", + id: -1, + lastUpdated: new Date(), + name: "", + profiles: [], + secure: false, + value: "", + }; + return; + } + + const numID = parseInt(ID, 10); + if (Number.isNaN(numID)) { + console.error("route parameter 'id' was non-number: ", ID); + return; + } + + this.parameter = await this.parameterService.getParameters(numID); + this.navSvc.headerTitle.next(`Parameter: ${this.parameter.name}`); Review Comment: So, this isn't enough information to identify a Parameter (unfortunately). There can be any number of Parameters with the same name. Therefore, I think it'd be best to add the ID after the name, maybe in parenthesis e.g. "Parameter: test (#420)" ########## experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.html: ########## @@ -0,0 +1,51 @@ +<!-- +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="!parameter"></tp-loading> + <form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="parameter"> + <mat-card-content> + <mat-form-field *ngIf="!new"> + <mat-label>ID</mat-label> + <input matInput type="text" name="id" disabled readonly [defaultValue]="parameter.id" /> + </mat-form-field> + <mat-form-field> + <mat-label>Name</mat-label> + <input matInput type="text" name="name" required [(ngModel)]="parameter.name" /> + </mat-form-field> + <mat-form-field> + <mat-label>Config File</mat-label> + <input matInput type="text" name="configFile" required [(ngModel)]="parameter.configFile" /> + </mat-form-field> + <mat-form-field> + <mat-label>Value</mat-label> + <input matInput type="text" name="value" required [(ngModel)]="parameter.value" /> + </mat-form-field> + <mat-form-field> + <mat-label>Secure</mat-label> + <mat-select name="secure"[(ngModel)]="parameter.secure" required> + <mat-option *ngFor="let s of secure" [value]="s.value">{{s.label}}</mat-option> + </mat-select> + </mat-form-field> Review Comment: for boolean properties, we should use a [checkbox](https://material.angular.io/components/checkbox/overview) or a [slide-toggle](https://material.angular.io/components/slide-toggle/overview) instead of a select. ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, Review Comment: This should use a numeric filter ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, + { + field: "secure", + headerName: "Secure" + }, + { + field: "value", + headerName: "Value" + }, + { + field: "lastUpdated", + headerName: "Last Updated" + } + ]; + + /** Definitions for the context menu items (which act on augmented parameter data). */ + public contextMenuItems: Array<ContextMenuItem<ResponseParameter>> = [ + { + href: (responseParameter: ResponseParameter): string => `${responseParameter.id}`, + name: "Edit" + }, + { + href: (responseParameter: ResponseParameter): string => `${responseParameter.id}`, + name: "Open in New Tab", + newTab: true + }, + { + action: "delete", + multiRow: false, + name: "Delete" + }, + { + href: (selectedRow: ResponseParameter): string => `/core/profiles/param/${selectedRow.id}`, + name: "Manage Profiles", + newTab: true, + } + ]; + + /** A subject that child components can subscribe to for access to the fuzzy search query text */ + public fuzzySubject: BehaviorSubject<string>; + + /** Form controller for the user search input. */ + public fuzzControl = new FormControl<string>("", {nonNullable: true}); + + constructor(private readonly route: ActivatedRoute, private readonly navSvc: NavigationService, + private readonly api: ParameterService, private readonly dialog: MatDialog, public readonly auth: CurrentUserService) { + this.fuzzySubject = new BehaviorSubject<string>(""); + this.parameters = this.api.getParameters(); + this.navSvc.headerTitle.next("Parameter"); + } + + /** Initializes table data, loading it from Traffic Ops. */ + public ngOnInit(): void { + this.route.queryParamMap.subscribe( + m => { + const search = m.get("search"); + if (search) { + this.fuzzControl.setValue(decodeURIComponent(search)); + this.updateURL(); + } + } + ); + } + + /** Update the URL's 'search' query parameter for the user's search input. */ + public updateURL(): void { + this.fuzzySubject.next(this.fuzzControl.value); + } + + /** + * Handles a context menu event. + * + * @param evt The action selected from the context menu. + */ + public async handleContextMenu(evt: ContextMenuActionEvent<ResponseParameter>): Promise<void> { + const data = evt.data as ResponseParameter; + switch(evt.action) { + case "delete": + const ref = this.dialog.open(DecisionDialogComponent, { + data: {message: `Are you sure you want to delete parameter ${data.name} with id ${data.id} ?`, title: "Confirm Delete"} Review Comment: "Parameter" should be capitalized, "ID" is an initialism, so it should be in all caps. Also, extraneous space between the ID and the punctuation (<kbd>?</kbd>) ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, + { + field: "secure", + headerName: "Secure" + }, + { + field: "value", + headerName: "Value" + }, + { + field: "lastUpdated", + headerName: "Last Updated" + } + ]; + + /** Definitions for the context menu items (which act on augmented parameter data). */ + public contextMenuItems: Array<ContextMenuItem<ResponseParameter>> = [ + { + href: (responseParameter: ResponseParameter): string => `${responseParameter.id}`, + name: "Edit" + }, + { + href: (responseParameter: ResponseParameter): string => `${responseParameter.id}`, + name: "Open in New Tab", + newTab: true + }, + { + action: "delete", + multiRow: false, + name: "Delete" + }, + { + href: (selectedRow: ResponseParameter): string => `/core/profiles/param/${selectedRow.id}`, + name: "Manage Profiles", + newTab: true, + } + ]; + + /** A subject that child components can subscribe to for access to the fuzzy search query text */ + public fuzzySubject: BehaviorSubject<string>; + + /** Form controller for the user search input. */ + public fuzzControl = new FormControl<string>("", {nonNullable: true}); + + constructor(private readonly route: ActivatedRoute, private readonly navSvc: NavigationService, + private readonly api: ParameterService, private readonly dialog: MatDialog, public readonly auth: CurrentUserService) { + this.fuzzySubject = new BehaviorSubject<string>(""); + this.parameters = this.api.getParameters(); + this.navSvc.headerTitle.next("Parameter"); Review Comment: The title should be plural, since ostensibly we're looking at a table of multiple parameters. ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, + { + field: "secure", + headerName: "Secure" + }, Review Comment: This should use a boolean filter ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, + { + field: "secure", + headerName: "Secure" + }, + { + field: "value", + headerName: "Value" + }, + { + field: "lastUpdated", + headerName: "Last Updated" + } Review Comment: this should use a date filter ########## experimental/traffic-portal/src/app/core/profiles/profile-table/profile-table.component.ts: ########## @@ -115,6 +115,11 @@ export class ProfileTableComponent implements OnInit { } } ); + const ID = this.route.snapshot.paramMap.get("id"); + if (ID === null) { + return; + } + this.profiles = this.api.getProfilesByParam(+ID); Review Comment: Won't this leave us unable to filter by ID, as it'll be interpreted as a parameter ID? I'd suggest using a query string parameter like "hasParameter={{number}}". ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts: ########## @@ -0,0 +1,146 @@ +/* +* 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, type 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 {ResponseParameter} from "trafficops-types"; + +import { ParameterService } from "src/app/api"; +import { CurrentUserService } from "src/app/shared/current-user/current-user.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import type { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * ParametersTableComponent is the controller for the "Parameters" table. + */ +@Component({ + selector: "tp-parameters", + styleUrls: ["./parameters-table.component.scss"], + templateUrl: "./parameters-table.component.html" +}) +export class ParametersTableComponent implements OnInit { + /** List of parameters */ + public parameters: Promise<Array<ResponseParameter>>; + + /** Definitions of the table's columns according to the ag-grid API */ + public columnDefs = [ + { + field: "id", + headerName: "ID" + }, + { + field: "configFile", + headerName: "Config File" + }, + { + field: "name", + headerName: "Name" + }, + { + field: "profiles", + headerName: "Profiles" + }, + { + field: "secure", + headerName: "Secure" + }, + { + field: "value", + headerName: "Value" + }, + { + field: "lastUpdated", + headerName: "Last Updated" + } + ]; + + /** Definitions for the context menu items (which act on augmented parameter data). */ + public contextMenuItems: Array<ContextMenuItem<ResponseParameter>> = [ + { + href: (responseParameter: ResponseParameter): string => `${responseParameter.id}`, + name: "Edit" + }, + { + href: (responseParameter: ResponseParameter): string => `${responseParameter.id}`, + name: "Open in New Tab", + newTab: true + }, + { + action: "delete", + multiRow: false, + name: "Delete" + }, + { + href: (selectedRow: ResponseParameter): string => `/core/profiles/param/${selectedRow.id}`, + name: "Manage Profiles", Review Comment: This actually only _shows_ you the profiles, doesn't allow you to manage them, so I think a more appropriate name would be "<ins>View</ins> Profiles" ########## experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.spec.ts: ########## @@ -0,0 +1,168 @@ +/* +* 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 { MatDialog, MatDialogModule, type MatDialogRef } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { of } from "rxjs"; + +import { ParameterService } from "src/app/api"; +import { APITestingModule } from "src/app/api/testing"; +import { ParametersTableComponent } from "src/app/core/parameters/table/parameters-table.component"; +import { isAction } from "src/app/shared/generic-table/generic-table.component"; + +const testParameter = { + configFile: "cfg.txt", + id: 1, + lastUpdated: new Date(), + name: "TestQuest", + profiles: [], + secure: false, + value: "", +}; + +describe("ParametersTableComponent", () => { + let component: ParametersTableComponent; + let fixture: ComponentFixture<ParametersTableComponent>; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [ ParametersTableComponent ], + imports: [ + APITestingModule, + RouterTestingModule, + MatDialogModule + ] + }).compileComponents(); + + fixture = TestBed.createComponent(ParametersTableComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + }); + + it("sets the fuzzy search subject based on the search query param", fakeAsync(() => { + const router = TestBed.inject(ActivatedRoute); + const searchString = "testquest"; + spyOnProperty(router, "queryParamMap").and.returnValue(of(new Map([["search", searchString]]))); + + let searchValue = "not the right string"; + component.fuzzySubject.subscribe( + s => searchValue = s + ); + + component.ngOnInit(); + tick(); + + expect(searchValue).toBe(searchString); + })); + + it("updates the fuzzy search output", fakeAsync(() => { + let called = false; + const text = "testquest"; + const spy = jasmine.createSpy("subscriber", (txt: string): void =>{ + if (!called) { + expect(txt).toBe(""); + called = true; + } else { + expect(txt).toBe(text); + } + }); + component.fuzzySubject.subscribe(spy); + tick(); + expect(spy).toHaveBeenCalled(); + component.fuzzControl.setValue(text); + component.updateURL(); + tick(); + expect(spy).toHaveBeenCalledTimes(2); + })); + + it("handles unrecognized contextmenu events", () => { + expect(async () => component.handleContextMenu({ + action: component.contextMenuItems[0].name, + data: {configFile: "cfg.txt", id: 1, lastUpdated: new Date(), name: "TestQuest", profiles: [], secure: false, value: ""} + })).not.toThrow(); + }); + + it("handles the 'delete' context menu item", fakeAsync(async () => { + const item = component.contextMenuItems.find(c => c.name === "Delete"); + if (!item) { + return fail("missing 'Delete' context menu item"); + } + if (!isAction(item)) { + return fail("expected an action, not a link"); + } + expect(item.multiRow).toBeFalsy(); + expect(item.disabled).toBeUndefined(); + + const api = TestBed.inject(ParameterService); + const spy = spyOn(api, "deleteParameter").and.callThrough(); + expect(spy).not.toHaveBeenCalled(); + + const dialogService = TestBed.inject(MatDialog); + const openSpy = spyOn(dialogService, "open").and.returnValue({ + afterClosed: () => of(true) + } as MatDialogRef<unknown>); + + const parameter = await api.createParameter({configFile: "cfg.txt", name: "test", secure: false, value: ""}); + expect(openSpy).not.toHaveBeenCalled(); + const asyncExpectation = expectAsync(component.handleContextMenu({action: "delete", data: parameter})).toBeResolvedTo(undefined); + tick(); + + expect(openSpy).toHaveBeenCalled(); + tick(); + + expect(spy).toHaveBeenCalled(); + + await asyncExpectation; + })); + + it("generates 'Edit' context menu item href", () => { + const item = component.contextMenuItems.find(i => i.name === "Edit"); + if (!item) { + return fail("missing 'Edit' context menu item"); + } + if (isAction(item)) { + return fail("expected a link, not an action"); + } + if (typeof(item.href) !== "function") { + return fail(`'Edit' context menu item should use a function to determine href, instead uses: ${item.href}`); + } + expect(item.href(testParameter)).toBe(String(testParameter.id)); + expect(item.queryParams).toBeUndefined(); + expect(item.fragment).toBeUndefined(); + expect(item.newTab).toBeFalsy(); + }); + + it("generates 'Open in New Tab' context menu item href", () => { + const item = component.contextMenuItems.find(i => i.name === "Open in New Tab"); + if (!item) { + return fail("missing 'Open in New Tab' context menu item"); + } + if (isAction(item)) { + return fail("expected a link, not an action"); + } + if (typeof(item.href) !== "function") { + return fail(`'Open in New Tab' context menu item should use a function to determine href, instead uses: ${item.href}`); + } + expect(item.href(testParameter)).toBe(String(testParameter.id)); + expect(item.queryParams).toBeUndefined(); + expect(item.fragment).toBeUndefined(); + expect(item.newTab).toBeTrue(); + }); Review Comment: Can you add a test for the profiles context menu item? ########## experimental/traffic-portal/src/app/shared/navigation/navigation.service.ts: ########## @@ -176,6 +166,22 @@ export class NavigationService { } ], name: "Other" + }, { + children: [ + { + href: "/core/types", + name: "Types" + }, + { + href: "/core/profiles", + name: "Profiles" + }, + { + href: "/core/parameters", + name: "Parameters" + } Review Comment: I'd put Parameters as a child of Profiles, personally. ########## experimental/traffic-portal/nightwatch/tests/parameters/detail.spec.ts: ########## @@ -0,0 +1,48 @@ +/* + * 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. + */ + +describe("Parameter Detail Spec", () => { + it("Test parameter", () => { + const page = browser.page.parameters.parameterDetail(); + browser.url(`${page.api.launchUrl}/core/parameters/${browser.globals.testData.parameter.id}`, res => { + browser.assert.ok(res.status === 0); + console.log("Srijeet"); + console.log(res.value); Review Comment: very proud of this code, eh? -- 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]
