ocket8888 commented on code in PR #7358: URL: https://github.com/apache/trafficcontrol/pull/7358#discussion_r1122091979
########## experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html: ########## @@ -0,0 +1,36 @@ +<!-- +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="statuses-details"> + <h3>{{title}}</h3> + <tp-loading *ngIf="loading"></tp-loading> + <form [formGroup]="statusDetailsForm" (ngSubmit)="onSubmit()" *ngIf="!loading"> + <mat-card-content> + <mat-form-field> + <mat-label>Name</mat-label> + <input matInput placeholder="Name" formControlName="name"> + </mat-form-field> + <mat-form-field> + <mat-label>Description</mat-label> + <input matInput placeholder="Description" formControlName="description"> Review Comment: Placeholders that reiterate the label are unnecessary. Also, shouldn't these be required? ########## experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts: ########## @@ -0,0 +1,132 @@ +/* + * 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 { FormBuilder, FormControl, FormGroup, Validators } from "@angular/forms"; +import { MatDialog } from "@angular/material/dialog"; +import { ActivatedRoute, Router } from "@angular/router"; +import { ResponseStatus } from "trafficops-types"; + +import { StatusesService } from "src/app/api/statuses.service"; +import { DecisionDialogComponent, DecisionDialogData } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; + +@Component({ + selector: "tp-status-details", + templateUrl: "./status-details.component.html", + styleUrls: ["./status-details.component.scss"] +}) +export class StatusDetailsComponent implements OnInit { + + id: string | null = null; + statusDetails: ResponseStatus | null = null; + statusDetailsForm!: FormGroup; + loading = false; + submitting = false; + submitted = false; + + constructor( + private readonly route: ActivatedRoute, + private readonly router: Router, + private readonly fb: FormBuilder, + private readonly dialog: MatDialog, + private readonly statusesService: StatusesService) { } + + ngOnInit(): void { + // Form is built here + this.statusDetailsForm = this.fb.group({ + name: ["", Validators.required], + description: ["", Validators.required], + }); + + // Getting id from the route + this.id = this.route.snapshot.paramMap.get("id"); + + // we check whether params is a number if not we shall assume user wants to add a new status. + if (!this.isNew) { + this.loading = true; + this.statusDetailsForm.addControl("id", new FormControl("")); + this.statusDetailsForm.addControl("lastUpdated", new FormControl("")); + this.getStatusDetails(); + } + } + + /* + * Reloads the servers table data. + * @param id is the id passed in route for this page if this is a edit view. + */ + async getStatusDetails(): Promise<void> { + const id = this.id as string; // id Type 'null' is not assignable to type 'string' + this.statusDetails = await this.statusesService.getStatuses(id); + const data: ResponseStatus = { + name: this.statusDetails.name, + description: this.statusDetails.description, + lastUpdated: new Date(), + id: this.statusDetails.id + }; + this.statusDetailsForm.patchValue(data); + this.loading = false; + } + + // On submitting the form we check for whether we are performing Create or Edit + onSubmit() { + if (this.isNew) { + this.createStatus(); + + } else { + this.updateStatus(); + } + } + + // For Creating a new status + createStatus() { + this.statusesService.createStatus(this.statusDetailsForm.value).then((res: any) => { + if (res) { + this.id = res?.id; + this.router.navigate([`/core/statuses/${this.id}`]); + } + }); + } + + // For updating the Status + updateStatus() { + this.statusesService.updateStatus(this.statusDetailsForm.value, Number(this.id)); + } + + // Deleteting status + async deleteStatus() { + const ref = this.dialog.open<DecisionDialogComponent, DecisionDialogData, boolean>(DecisionDialogComponent, { + data: { + message: `This action CANNOT be undone. This will permanently delete '${this.statusDetails?.name}'.`, + title: `Delete Status: ${this.statusDetails?.name}` + } + }); + + if (await ref.afterClosed().toPromise()) { + const id = Number(this.id); + this.statusesService.deleteStatus(id).then(() => { Review Comment: Why not just `await` this like you did with the promise from `afterClosed` above? ########## experimental/traffic-portal/src/styles.scss: ########## @@ -151,4 +151,4 @@ button { position: fixed; bottom: 16px; right: 16px; -} +} Review Comment: please don't remove the terminating newline character in this file ########## experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.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 { Component, OnInit } from "@angular/core"; +import { BehaviorSubject } from "rxjs"; +import { ResponseStatus } from "trafficops-types"; + +import { ServerService } from "src/app/api"; +import { ContextMenuItem } from "src/app/shared/generic-table/generic-table.component"; + +@Component({ + selector: "tp-statuses-table", + templateUrl: "./statuses-table.component.html", + styleUrls: ["./statuses-table.component.scss"] +}) +export class StatusesTableComponent implements OnInit { + + statuses: ResponseStatus[] = []; + 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 user data). */ + public contextMenuItems: Array<ContextMenuItem<ResponseStatus>> = [ + { + href: (u: ResponseStatus): string => `${u.id}`, + name: "View Status Details" + }, + { + href: (): string => "new", Review Comment: `href` is allowed to just be a string if it's a constant value ########## experimental/traffic-portal/src/app/api/statuses.service.ts: ########## @@ -0,0 +1,68 @@ +import { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import { ResponseStatus } from "trafficops-types"; + +import { APIService } from "./base-api.service"; + +@Injectable() +export class StatusesService 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 getStatuses(idOrName: number | string): Promise<ResponseStatus>; + public async getStatuses(): Promise<Array<ResponseStatus>>; + /** + * @param id Specify either the integral, unique identifier (number. + * @returns The requested status(s). + */ + public async getStatuses(id?: number | string): Promise<Array<ResponseStatus> | ResponseStatus> { + const path = "statuses"; + if (id !== undefined) { + let statuses; + statuses = await this.get<[ResponseStatus]>(path, undefined, { id: String(id) }).toPromise(); + if (statuses.length < 1) { + throw new Error(`no such statuses '${id}'`); + } + return statuses[0]; + } + return this.get<Array<ResponseStatus>>(path).toPromise(); + } + + /** + * Creating new Status. + * + * @param data containes name and description for the status. + * @returns The 'response' property of the TO status response. See TO API docs. + */ + public async createStatus(data: ResponseStatus) { + const path = "statuses"; + return this.post<ResponseStatus>(path, data).toPromise(); + } + + /** + * Updates status. + * + * @param data containes name and description for the status., unique identifier thereof. + * @param id The Status ID + */ + public async updateStatus(data: ResponseStatus, id: number): Promise<ResponseStatus | undefined> { + const path = `statuses/${id}`; + return this.put(path, data).toPromise(); + } + + /** + * Deletes an existing Status. + * + * @param id The Status ID + */ + public async deleteStatus(id: number): Promise<void> { + return this.delete(`statuses/${id}`).toPromise(); + } +} Review Comment: This file shouldn't exist. Statuses are managed in the Server service, which already implements at least one of these methods for you. ########## experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.spec.ts: ########## @@ -0,0 +1,94 @@ +/* + * 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 { HttpClientTestingModule } from "@angular/common/http/testing"; +import { ComponentFixture, fakeAsync, TestBed } from "@angular/core/testing"; +import { ReactiveFormsModule } from "@angular/forms"; +import { MatButtonModule } from "@angular/material/button"; +import { MatCardModule } from "@angular/material/card"; +import { MatFormFieldModule } from "@angular/material/form-field"; +import { MatGridListModule } from "@angular/material/grid-list"; +import { MatInputModule } from "@angular/material/input"; +import { BrowserDynamicTestingModule } from "@angular/platform-browser-dynamic/testing"; +import { Router } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { StatusesService } from "src/app/api/statuses.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import { SharedModule } from "src/app/shared/shared.module"; +import { StatusDetailsComponent } from "./status-details.component"; + +const status = { id: 1, name: 'test', description: 'test', lastUpdated: new Date('02/02/2023') }; + +describe("StatusDetailsComponent", () => { + let component: StatusDetailsComponent; + let fixture: ComponentFixture<StatusDetailsComponent>; + let router: Router; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ + HttpClientTestingModule, + RouterTestingModule.withRoutes([ + { path: 'core/statuses/:id', component: StatusDetailsComponent } + ]), + ReactiveFormsModule, + MatFormFieldModule, + MatInputModule, + MatGridListModule, + MatCardModule, + MatButtonModule, + SharedModule + ], + declarations: [StatusDetailsComponent, DecisionDialogComponent], + providers: [StatusesService] + }) + .compileComponents(); + TestBed.overrideModule(BrowserDynamicTestingModule, { + set: { + entryComponents: [DecisionDialogComponent] + } + }); + router = TestBed.inject(Router); + fixture = TestBed.createComponent(StatusDetailsComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + }); + + it("submits a update status request", fakeAsync(() => { + const service = TestBed.inject(StatusesService); + component.statusDetailsForm.setValue(status); + spyOn(service, 'updateStatus').and.returnValue(Promise.resolve(status)); + component.updateStatus(); + + service.updateStatus(component.statusDetailsForm.value, 1).then((result) => { + expect(result).toEqual(status); + }) + })); + + it("submits a status creation request", fakeAsync(() => { Review Comment: There's no reason to use `fakeAsync` if you aren't calling `tick` to synchronously progress the clock or `flushMicrotasks` to manually execute microtasks. ########## experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.spec.ts: ########## @@ -0,0 +1,60 @@ +/* + * 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 { HttpClientTestingModule } from "@angular/common/http/testing"; +import { ComponentFixture, fakeAsync, TestBed } from "@angular/core/testing"; +import { FormsModule } from "@angular/forms"; +import { MatCardModule } from "@angular/material/card"; +import { RouterTestingModule } from "@angular/router/testing"; +import { ServerService } from "src/app/api"; +import { SharedModule } from "src/app/shared/shared.module"; + +import { StatusesTableComponent } from "./statuses-table.component"; + +const statuses = [{id:1,name:'test',description:'test',lastUpdated:new Date('02/02/2023')}]; +describe("StatusesTableComponent", () => { + let component: StatusesTableComponent; + let fixture: ComponentFixture<StatusesTableComponent>; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports:[ + RouterTestingModule, + HttpClientTestingModule, + FormsModule, + MatCardModule, + SharedModule + ], + declarations: [ StatusesTableComponent ], + providers:[ServerService] + }) + .compileComponents(); + + fixture = TestBed.createComponent(StatusesTableComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + }); + + it("should get all statuses from getStatuses",fakeAsync(()=>{ Review Comment: There's no reason to use `fakeAsync` if you aren't calling `tick` to synchronously progress the clock or `flushMicrotasks` to manually execute microtasks. ########## experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.spec.ts: ########## @@ -0,0 +1,94 @@ +/* + * 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 { HttpClientTestingModule } from "@angular/common/http/testing"; +import { ComponentFixture, fakeAsync, TestBed } from "@angular/core/testing"; +import { ReactiveFormsModule } from "@angular/forms"; +import { MatButtonModule } from "@angular/material/button"; +import { MatCardModule } from "@angular/material/card"; +import { MatFormFieldModule } from "@angular/material/form-field"; +import { MatGridListModule } from "@angular/material/grid-list"; +import { MatInputModule } from "@angular/material/input"; +import { BrowserDynamicTestingModule } from "@angular/platform-browser-dynamic/testing"; +import { Router } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { StatusesService } from "src/app/api/statuses.service"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import { SharedModule } from "src/app/shared/shared.module"; +import { StatusDetailsComponent } from "./status-details.component"; + +const status = { id: 1, name: 'test', description: 'test', lastUpdated: new Date('02/02/2023') }; + +describe("StatusDetailsComponent", () => { + let component: StatusDetailsComponent; + let fixture: ComponentFixture<StatusDetailsComponent>; + let router: Router; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ + HttpClientTestingModule, + RouterTestingModule.withRoutes([ + { path: 'core/statuses/:id', component: StatusDetailsComponent } + ]), + ReactiveFormsModule, + MatFormFieldModule, + MatInputModule, + MatGridListModule, + MatCardModule, + MatButtonModule, + SharedModule + ], + declarations: [StatusDetailsComponent, DecisionDialogComponent], + providers: [StatusesService] + }) + .compileComponents(); + TestBed.overrideModule(BrowserDynamicTestingModule, { + set: { + entryComponents: [DecisionDialogComponent] + } + }); + router = TestBed.inject(Router); + fixture = TestBed.createComponent(StatusDetailsComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + }); + + it("submits a update status request", fakeAsync(() => { Review Comment: There's no reason to use `fakeAsync` if you aren't calling `tick` to synchronously progress the clock or `flushMicrotasks` to manually execute microtasks. -- 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]
