ocket8888 commented on code in PR #7358:
URL: https://github.com/apache/trafficcontrol/pull/7358#discussion_r1162053543
##########
experimental/traffic-portal/src/app/api/testing/server.service.ts:
##########
@@ -302,4 +317,55 @@ export class ServerService {
srv.statusId = status.id;
srv.offlineReason = offlineReason ?? null;
}
+
+ /**
+ * Creates a status.
+ *
+ * @param status The status details (name & description) to create.
+ * @returns The status as created and returned by the API.
+ */
+ public async createStatus(status: RequestStatus):
Promise<ResponseStatus> {
+ const newStatus = {
+ ...status,
+ id: ++this.statusIdCounter,
+ lastUpdated: new Date()
+ } as { description: string; id: number; lastUpdated: Date;
name: string };
Review Comment:
Why do you need this `as` statement?
##########
experimental/traffic-portal/src/app/api/server.service.ts:
##########
@@ -186,4 +187,33 @@ export class ServerService extends APIService {
return this.put(`servers/${id}/status`, {offlineReason,
status}).toPromise();
}
+
+ /**
+ * Creating new Status.
+ *
+ * @param status The status to create.
+ * @returns The created status.
+ */
+ public async createStatus(status: RequestStatus):
Promise<ResponseStatus> {
+ return this.post<ResponseStatus>("statuses",
status).toPromise();
+ }
+
+ /**
+ * Updates status Details.
+ *
+ * @param status The status to update.
+ * @returns The updated status.
+ */
+ public async updateStatusDetail(status: ResponseStatus):
Promise<ResponseStatus> {
+ return this.put<ResponseStatus>(`statuses/${status.id}`,
status).toPromise();
+ }
+
+ /**
+ * Deletes an existing Status.
+ *
+ * @param id The Status ID
+ */
+ public async deleteStatus(id: number | ResponseStatus):
Promise<ResponseStatus> {
+ return
this.delete<ResponseStatus>(`statuses/${id}`).toPromise();
+ }
Review Comment:
This method won't work for all call signatures; if you pass in a
`ResponseStatus` it will attempt to use the API endpoint
`/statuses/%5Bobject%20Object%5D` which does not exist because
`%5Bobject%20Object%5D` is not a valid Status ID.
##########
experimental/traffic-portal/.eslintrc.json:
##########
@@ -237,7 +237,6 @@
"enableFixer": false
}
],
- "@typescript-eslint/unbound-method": "error",
Review Comment:
Please don't remove this check
##########
experimental/traffic-portal/src/app/api/testing/server.service.ts:
##########
@@ -302,4 +317,55 @@ export class ServerService {
srv.statusId = status.id;
srv.offlineReason = offlineReason ?? null;
}
+
+ /**
+ * Creates a status.
+ *
+ * @param status The status details (name & description) to create.
+ * @returns The status as created and returned by the API.
+ */
+ public async createStatus(status: RequestStatus):
Promise<ResponseStatus> {
+ const newStatus = {
+ ...status,
+ id: ++this.statusIdCounter,
+ lastUpdated: new Date()
+ } as { description: string; id: number; lastUpdated: Date;
name: string };
+ this.statuses.push(newStatus);
+ return newStatus;
+ }
+
+ /**
+ * Updates status Details.
+ *
+ * @param payload containes name and description for the status.,
unique identifier thereof.
+ * @param id The Status ID
+ */
+ public async updateStatusDetail(payload: ResponseStatus, id: number):
Promise<ResponseStatus> {
+ const index = this.statuses.findIndex(u => u.id === id);
+ if (index < 0) {
+ throw new Error(`no such status with id: ${id}`);
+ }
+ const updated = {
+ ...payload,
+ lastUpdated: new Date()
+ } as { description: string; id: number; lastUpdated: Date;
name: string };
+ this.statuses[index] = updated;
+
+ return updated;
+ }
+
+ /**
+ * Deletes a Status.
+ *
+ * @param id The ID of the Status to delete.
+ * @returns The response.
+ */
+ public async deleteStatus(id: number): Promise<HttpResponse<object> |
null> {
Review Comment:
This function's call signature is not assignable to the call signature of
the concrete implementation; `HttpResponse<object> | null` is not assignable to
`ResponseStatus`. In order for the testing services to work as drop-in
replacements for the "real" services, they need to return the same kind of data.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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 } from "@angular/core";
+import { FormControl, FormGroup, Validators } from "@angular/forms";
+import { MatDialog } from "@angular/material/dialog";
+import { ActivatedRoute } from "@angular/router";
+import { ResponseStatus } from "trafficops-types";
+
+import { ServerService } from "src/app/api";
+import { DecisionDialogComponent, DecisionDialogData } from
"src/app/shared/dialogs/decision-dialog/decision-dialog.component";
+import { NavigationService } from
"src/app/shared/navigation/navigation.service";
+
+/**
+ * StatusDetailsComponent is the controller for a status "details" page.
+ */
+@Component({
+ selector: "tp-status-details",
+ styleUrls: ["./status-details.component.scss"],
+ templateUrl: "./status-details.component.html",
+})
+export class StatusDetailsComponent {
+ public new = false;
+
+ /** Loader status for the actions */
+ public loading = false;
Review Comment:
Shouldn't `loading` initially be `true`, until the data is loaded?
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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 } from "@angular/core";
+import { FormControl, FormGroup, Validators } from "@angular/forms";
+import { MatDialog } from "@angular/material/dialog";
+import { ActivatedRoute } from "@angular/router";
+import { ResponseStatus } from "trafficops-types";
+
+import { ServerService } from "src/app/api";
+import { DecisionDialogComponent, DecisionDialogData } from
"src/app/shared/dialogs/decision-dialog/decision-dialog.component";
+import { NavigationService } from
"src/app/shared/navigation/navigation.service";
+
+/**
+ * StatusDetailsComponent is the controller for a status "details" page.
+ */
+@Component({
+ selector: "tp-status-details",
+ styleUrls: ["./status-details.component.scss"],
+ templateUrl: "./status-details.component.html",
+})
+export class StatusDetailsComponent {
+ public new = false;
+
+ /** Loader status for the actions */
+ public loading = false;
+
+ /** All details of status requested */
+ public statusDetails!: ResponseStatus;
+
+ /** Reactive form intialized to creat / edit status details */
+ public statusDetailsForm: FormGroup = new FormGroup({
+ description: new FormControl("", Validators.required),
+ name: new FormControl("", Validators.required),
+ });
+
+ /**
+ * Constructor.
+ *
+ * @param api The Servers API which is used to provide row data.
+ * @param route A reference to the route of this view which is used to
get the 'id' query parameter of status.
+ * @param router Angular router
+ * @param dialog Dialog manager
+ * @param fb Form builder
+ * @param navSvc Manages the header
+ */
+ constructor(
+ private readonly api: ServerService,
+ private readonly route: ActivatedRoute,
+ private readonly dialog: MatDialog,
+ private readonly navSvc: NavigationService, private readonly
location: Location,
+ ) {
+ // Getting id from the route
+ const id = this.route.snapshot.paramMap.get("id");
+
+ /**
+ * Initializes table data, loading it from Traffic Ops.
+ * we check whether params is a number if not we shall assume
user wants to add a new status.
+ */
+ if (id !== "new") {
+ this.loading = true;
+ this.statusDetailsForm.addControl("id", new
FormControl(""));
+ this.statusDetailsForm.addControl("lastUpdated", new
FormControl(""));
+ this.getStatusDetails();
+ } else {
+ this.navSvc.headerTitle.next("New Status");
+ this.new = true;
+ }
+ }
+
+ /**
+ * Get status details for the id
+ * patch the form with status details
+ */
+ public async getStatusDetails(): Promise<void> {
+ this.statusDetails = await
this.api.getStatuses(this.statusDetails.id);
+
+ // Set page title with status Name
+ this.navSvc.headerTitle.next(`Status
#${this.statusDetails.name}`);
+
+ // Patch the form with existing data we got from service
requested above.
+ this.statusDetailsForm.patchValue(this.statusDetails);
+ this.loading = false;
+ }
+
+ /**
+ * On submitting the form we check for whether we are performing Create
or Edit
+ *
+ * @param event HTML form submission event.
+ */
+ public async onSubmit(event: Event): Promise<void> {
+ event.preventDefault();
+ event.stopPropagation();
+
+ if (this.statusDetailsForm.valid) {
+ if (this.new) {
+ this.statusDetails = await
this.api.createStatus(this.statusDetailsForm.value);
+ this.location.back();
Review Comment:
Why are we going back to the user's previous location when a new status is
created?
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.ts:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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 { ResponseStatus } from "trafficops-types";
+
+import { ServerService } 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 { ContextMenuActionEvent, ContextMenuItem } from
"src/app/shared/generic-table/generic-table.component";
+import { NavigationService } from
"src/app/shared/navigation/navigation.service";
+
+/**
+ * StatusesTableComponent is the controller for the statuses page - which
+ * principally contains a table.
+ */
+@Component({
+ selector: "tp-statuses-table",
+ styleUrls: ["./statuses-table.component.scss"],
+ templateUrl: "./statuses-table.component.html",
+})
+export class StatusesTableComponent implements OnInit {
+
+ /** All of the statues which should appear in the table. */
+ public statuses: Promise<Array<ResponseStatus>>;
+
+ /** Definitions of the table's columns according to the ag-grid API */
+ public columnDefs = [
+ {
+ field: "name",
+ headerName: "Name",
+ hide: false
+ },
+ {
+ field: "description",
+ headerName: "Description",
+ hide: false
+ }];
+
+ /** The current search text. */
+ public searchText = "";
+
+ /** Definitions for the context menu items (which act on statuses
data). */
+ public contextMenuItems: Array<ContextMenuItem<ResponseStatus>> = [
+ {
+ href: (u: ResponseStatus): string => `${u.id}`,
+ name: "View Status Details"
+ },
+ {
+ href: (u: ResponseStatus): string => `${u.id}`,
+ name: "Edit"
+ },
+ {
+ action: "delete",
+ multiRow: false,
+ name: "Delete"
+ }
+ ];
+
+ /** 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});
+
+ /**
+ * Constructs the component with its required injections.
+ *
+ * @param api The Servers API which is used to provide row data.
+ * @param navSvc Manages the header
+ */
+ constructor(
+ private readonly dialog: MatDialog,
+ private readonly route: ActivatedRoute,
+ private readonly api: ServerService,
+ private readonly navSvc: NavigationService, public readonly
auth: CurrentUserService
+ ) {
+ this.fuzzySubject = new BehaviorSubject<string>("");
+ this.statuses = this.api.getStatuses();
+ this.navSvc.headerTitle.next("Statuses");
+ }
+
+ /** 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();
+ }
+ }
+ );
+ }
+
+ /**
+ * Updates the "search" query parameter in the URL every time the search
+ * text input changes.
+ */
+ public updateURL(): void {
+ this.fuzzySubject.next(this.searchText);
+ }
+
+ /**
+ * Handles a context menu event.
+ *
+ * @param evt The action selected from the context menu.
+ */
+ public async handleContextMenu(evt:
ContextMenuActionEvent<ResponseStatus>): Promise<void> {
+ const data = evt.data as ResponseStatus;
+ switch(evt.action) {
+ case "delete":
+ const ref =
this.dialog.open(DecisionDialogComponent, {
+ data: {message: `Are you sure you want
to delete status ${data.name} with id ${data.id} ?`, title: "Confirm Delete"}
Review Comment:
`status` should be capitalized. Also, nit: "ID" is an initialism, so should
also be capitalized.
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.html:
##########
@@ -0,0 +1,28 @@
+<!--
+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 class="table-page-content">
+ <div class="search-container">
+ <input type="search" name="fuzzControl" aria-label="Fuzzy
Search Types" autofocus inputmode="search" role="search" accesskey="/"
placeholder="Fuzzy Search" [formControl]="fuzzControl" (input)="updateURL()" />
Review Comment:
`Fuzzy Search Types` is a bad label for an input used to search for
_Statuses_.
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.ts:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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 { ResponseStatus } from "trafficops-types";
+
+import { ServerService } 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 { ContextMenuActionEvent, ContextMenuItem } from
"src/app/shared/generic-table/generic-table.component";
+import { NavigationService } from
"src/app/shared/navigation/navigation.service";
+
+/**
+ * StatusesTableComponent is the controller for the statuses page - which
+ * principally contains a table.
+ */
+@Component({
+ selector: "tp-statuses-table",
+ styleUrls: ["./statuses-table.component.scss"],
+ templateUrl: "./statuses-table.component.html",
+})
+export class StatusesTableComponent implements OnInit {
+
+ /** All of the statues which should appear in the table. */
+ public statuses: Promise<Array<ResponseStatus>>;
+
+ /** Definitions of the table's columns according to the ag-grid API */
+ public columnDefs = [
+ {
+ field: "name",
+ headerName: "Name",
+ hide: false
+ },
+ {
+ field: "description",
+ headerName: "Description",
+ hide: false
+ }];
Review Comment:
What about `id` and `lastUpdated`?
##########
experimental/traffic-portal/src/app/core/core.module.ts:
##########
@@ -126,10 +130,12 @@ export const ROUTES: Routes = [
RegionsTableComponent,
RegionDetailComponent,
CacheGroupDetailsComponent,
- CoordinatesTableComponent,
- CoordinateDetailComponent,
+ StatusesTableComponent,
+ StatusDetailsComponent,
TypesTableComponent,
TypeDetailComponent,
+ CoordinatesTableComponent,
+ CoordinateDetailComponent,
Review Comment:
Coordinates components don't need to be moved, do they?
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.spec.ts:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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 { NavigationService } from
"src/app/shared/navigation/navigation.service";
+
+import { StatusDetailsComponent } from "./status-details.component";
+
+describe("StatusDetailsComponent", () => {
+ let component: StatusDetailsComponent;
+ let fixture: ComponentFixture<StatusDetailsComponent>;
+ 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: [StatusDetailsComponent],
+ 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(StatusDetailsComponent);
+ component = fixture.componentInstance;
+ fixture.detectChanges();
+ });
+
+ it("should create", () => {
+ expect(component).toBeTruthy();
+ });
+
+ it("new status", async () => {
+ paramMap.and.returnValue("new");
+
+ fixture = TestBed.createComponent(StatusDetailsComponent);
+ component = fixture.componentInstance;
+ fixture.detectChanges();
+ await fixture.whenStable();
+ expect(paramMap).toHaveBeenCalled();
+ expect(component.statusDetails).not.toBeNull();
+ expect(component.new).toBeTrue();
+ });
+
+ it("existing status", async () => {
+ paramMap.and.returnValue("1");
+
+ fixture = TestBed.createComponent(StatusDetailsComponent);
+ component = fixture.componentInstance;
+ fixture.detectChanges();
+ await fixture.whenStable();
+ expect(paramMap).toHaveBeenCalled();
+ expect(component.statusDetails).not.toBeNull();
+ expect(component.statusDetails.name).toBe("OFFLINE");
Review Comment:
We shouldn't depend on hard-coding a name-to-ID relationship like the
"1"-to-"OFFLINE" one used here. You can get a Status from the service and
set/check things based on that (which would be best), or just check that the
value is truth-y and make the somewhat safer assumption that a Status with an
ID of `1` exists.
##########
experimental/traffic-portal/src/app/api/testing/server.service.ts:
##########
@@ -214,7 +222,14 @@ export class ServerService {
}
return status;
}
- return this.statuses;
+ return this.statuses.map(
+ p => ({
+ description: p.description,
+ id: p.id,
+ lastUpdated: p.lastUpdated,
+ name: p.name
+ })
+ );
Review Comment:
This mapping is not operating on each status in any way, returning an object
reconstructed in exactly the same way as the original, so: why?
--
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]
