You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "ocket8888 (via GitHub)" <gi...@apache.org> on 2023/02/24 00:56:58 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7358: Kannan victory 1 - statuses table, details
ocket8888 commented on code in PR #7358:
URL: https://github.com/apache/trafficcontrol/pull/7358#discussion_r1116381711
##########
experimental/traffic-portal/src/app/api/statuses.service.ts:
##########
@@ -0,0 +1,71 @@
+import { HttpClient } from '@angular/common/http';
+import { Injectable } from '@angular/core';
+import { APIService } from './base-api.service';
+
+export interface StatusesModel {
+ description?: string;
+ id?: number;
+ lastUpdated?: Date;
+ name?: string;
+}
Review Comment:
Instead of modeling this yourself, just import the relevant types from the `trafficops-types` package
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html:
##########
@@ -0,0 +1,26 @@
+<mat-card class="statuses-details">
+ <ul class="breadcrumb">
+ <li><a routerLink="/core/statuses">Statuses</a></li>
+ <li>{{title}}</li>
+ </ul>
Review Comment:
This is re-implementing the TP header component; it's in TPv1 I understand, but you don't need to copy this part of that page.
##########
experimental/traffic-portal/src/app/api/statuses.service.ts:
##########
@@ -0,0 +1,71 @@
+import { HttpClient } from '@angular/common/http';
+import { Injectable } from '@angular/core';
+import { APIService } from './base-api.service';
+
+export interface StatusesModel {
+ description?: string;
+ id?: number;
+ lastUpdated?: Date;
+ name?: string;
+}
+
+@Injectable({
+ providedIn: 'root'
Review Comment:
New API services must not be provided in the root scope, they are to be provided by the API module. That way, it can be easily swapped out as the API Testing module has mock providers for all of them. Similar to how the Angular `RouterModule` and `RouterTestingModule` modules work.
##########
experimental/traffic-portal/src/app/api/statuses.service.ts:
##########
@@ -0,0 +1,71 @@
+import { HttpClient } from '@angular/common/http';
+import { Injectable } from '@angular/core';
+import { APIService } from './base-api.service';
+
+export interface StatusesModel {
+ description?: string;
+ id?: number;
+ lastUpdated?: Date;
+ name?: string;
+}
+
+@Injectable({
+ providedIn: 'root'
+})
+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<StatusesModel>;
+ public async getStatuses(): Promise<Array<StatusesModel>>;
+ /**
+ * @param id Specify either the integral, unique identifier (number.
+ * @returns The requested status(s).
+ */
+ public async getStatuses(id?: number | string): Promise<Array<StatusesModel> | StatusesModel> {
+ const path = "statuses";
+ if (id !== undefined) {
+ let statuses;
+ statuses = await this.get<[StatusesModel]>(path, undefined, { id: String(id) }).toPromise();
+ if (statuses.length < 1) {
+ throw new Error(`no such statuses '${id}'`);
+ }
+ return statuses[0];
+ }
+ return this.get<Array<StatusesModel>>(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: StatusesModel) {
+ const path = "statuses";
+ return this.post<StatusesModel>(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: StatusesModel, id: number): Promise<StatusesModel | 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:
Statuses are, ostensibly, properties of servers, and they therefore are managed through the `ServerService`, which already implements methods for `getStatuses` and `updateStatus`.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html:
##########
@@ -0,0 +1,26 @@
+<mat-card class="statuses-details">
Review Comment:
This file is missing a license header; see other template HTML files for a comment at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html:
##########
@@ -0,0 +1,26 @@
+<mat-card class="statuses-details">
+ <ul class="breadcrumb">
+ <li><a routerLink="/core/statuses">Statuses</a></li>
+ <li>{{title}}</li>
+ </ul>
+ <tp-loading *ngIf="loading"></tp-loading>
+ <form [formGroup]="statusDetailsForm" (ngSubmit)="onSubmit()" *ngIf="!loading">
+ <mat-card-content>
+ <mat-form-field class="example-full-width">
+ <mat-label>Name</mat-label>
+ <input matInput placeholder="Ex. Pizza" formControlName="name">
Review Comment:
"Ex." is short for "Exercise", so you probably meant "E.g.". However, it's redundant in any case, because the purpose of a placeholder is to provide an example of valid input, so "E.g." would be implied. I'm also not sure how much information "Pizza" can really be conveying as an example here, especially since it's used for both the name and the description.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html:
##########
@@ -0,0 +1,26 @@
+<mat-card class="statuses-details">
+ <ul class="breadcrumb">
+ <li><a routerLink="/core/statuses">Statuses</a></li>
+ <li>{{title}}</li>
+ </ul>
+ <tp-loading *ngIf="loading"></tp-loading>
+ <form [formGroup]="statusDetailsForm" (ngSubmit)="onSubmit()" *ngIf="!loading">
+ <mat-card-content>
+ <mat-form-field class="example-full-width">
+ <mat-label>Name</mat-label>
+ <input matInput placeholder="Ex. Pizza" formControlName="name">
+ </mat-form-field>
+ <mat-form-field class="example-full-width">
+ <mat-label>Description</mat-label>
+ <input matInput placeholder="Ex. Pizza" formControlName="description">
+ </mat-form-field>
+ </mat-card-content>
+ <mat-card-actions align="end">
+ <button mat-raised-button type="button" color="warn" (click)="deleteStatus()" *ngIf="!isNew">Delete</button>
+ <button mat-raised-button type="button" routerLink="/core/statuses">Cancel</button>
+ <button mat-raised-button type="submit" color="primary" [disabled]="statusDetailsForm.invalid">
+ {{isNew ? 'Create' : 'Save'}}
+ </button>
+ </mat-card-actions>
+ </form>
+</mat-card>
Review Comment:
Missing newline at EOF
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.scss:
##########
@@ -0,0 +1,12 @@
+mat-card {
+ margin: 1em auto;
+ width: 95%;
+ min-width: 350px;
+
+ mat-card-content {
+ display: grid;
+ grid-template-columns: 1fr;
+ grid-row-gap: 2em;
Review Comment:
`grid-row-gap` is deprecated; use `row-gap` instead.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html:
##########
@@ -0,0 +1,26 @@
+<mat-card class="statuses-details">
+ <ul class="breadcrumb">
+ <li><a routerLink="/core/statuses">Statuses</a></li>
+ <li>{{title}}</li>
+ </ul>
+ <tp-loading *ngIf="loading"></tp-loading>
+ <form [formGroup]="statusDetailsForm" (ngSubmit)="onSubmit()" *ngIf="!loading">
+ <mat-card-content>
+ <mat-form-field class="example-full-width">
+ <mat-label>Name</mat-label>
+ <input matInput placeholder="Ex. Pizza" formControlName="name">
+ </mat-form-field>
+ <mat-form-field class="example-full-width">
+ <mat-label>Description</mat-label>
+ <input matInput placeholder="Ex. Pizza" formControlName="description">
Review Comment:
Same as the two above comments
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.scss:
##########
@@ -0,0 +1,12 @@
+mat-card {
+ margin: 1em auto;
+ width: 95%;
+ min-width: 350px;
+
+ mat-card-content {
+ display: grid;
+ grid-template-columns: 1fr;
+ grid-row-gap: 2em;
+ margin: 1em auto 10px;
+ }
+}
Review Comment:
Missing newline at EOF
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.spec.ts:
##########
@@ -0,0 +1,23 @@
+import { ComponentFixture, TestBed } from '@angular/core/testing';
+
+import { StatusDetailsComponent } from './status-details.component';
+
+describe('StatusDetailsComponent', () => {
+ let component: StatusDetailsComponent;
+ let fixture: ComponentFixture<StatusDetailsComponent>;
+
+ beforeEach(async () => {
+ await TestBed.configureTestingModule({
+ declarations: [ StatusDetailsComponent ]
+ })
+ .compileComponents();
+
+ fixture = TestBed.createComponent(StatusDetailsComponent);
+ component = fixture.componentInstance;
+ fixture.detectChanges();
+ });
+
+ it('should create', () => {
+ expect(component).toBeTruthy();
+ });
+});
Review Comment:
This should include more than just the default generated test to make sure it can be instantiated. Personally I'm trying to get 100% coverage in my new files, but I wouldn't ask that much of you - although you are welcome to it if you want. Just try to cover as much as you can, but if something's complicated and would take a lot of time to work out you can skip it, stuff like testing route parameter subscriptions and whatnot can get pretty messy pretty quickly.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.scss:
##########
@@ -0,0 +1,12 @@
+mat-card {
Review Comment:
This file is missing a license header. See other stylesheets for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.module.ts:
##########
@@ -0,0 +1,36 @@
+import { NgModule } from '@angular/core';
+import { CommonModule } from '@angular/common';
+import { StatusDetailsComponent } from './status-details.component';
+import { RouterModule } from '@angular/router';
+import { ReactiveFormsModule } from '@angular/forms';
+import { MatFormFieldModule } from '@angular/material/form-field';
+import { MatInputModule } from '@angular/material/input';
+import {MatGridListModule} from '@angular/material/grid-list';
+import { MatCardModule } from '@angular/material/card';
+import { SharedModule } from 'src/app/shared/shared.module';
+import { MatButtonModule } from '@angular/material/button';
+
+const StatusDetailRouting = RouterModule.forChild([
+ {
+ path: '',
+ component: StatusDetailsComponent
+ }
+]);
+
+@NgModule({
+ declarations: [
+ StatusDetailsComponent
+ ],
+ imports: [
+ CommonModule,
+ StatusDetailRouting,
+ ReactiveFormsModule,
+ MatFormFieldModule,
+ MatInputModule,
+ MatGridListModule,
+ MatCardModule,
+ MatButtonModule,
+ SharedModule
+ ]
+})
+export class StatusDetailsModule { }
Review Comment:
Why did you decide to make a whole module for each single component?
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts:
##########
@@ -0,0 +1,117 @@
+import { Component, OnInit } from '@angular/core';
Review Comment:
This file is missing a license header. See other typescript files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.spec.ts:
##########
@@ -0,0 +1,23 @@
+import { ComponentFixture, TestBed } from '@angular/core/testing';
Review Comment:
This file is missing a license header. See other typescript files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.html:
##########
@@ -0,0 +1,9 @@
+<mat-card class="table-page-content">
+ <div>
+ <input type="search" name="fuzzControl" aria-label="Fuzzy Search Users" autofocus inputmode="search"
+ role="search" accesskey="/" placeholder="Fuzzy Search" [(ngModel)]="searchText" (input)="updateURL()" />
+ </div>
+ <tp-generic-table [data]="statuses" [cols]="columnDefs" [fuzzySearch]="fuzzySubject" context="statuses"
+ [contextMenuItems]="contextMenuItems">
+ </tp-generic-table>
+</mat-card>
Review Comment:
Missing newline at EOF
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts:
##########
@@ -0,0 +1,117 @@
+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 { StatusesModel, 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: StatusesModel | null = null;
+ statusDetailsForm!: FormGroup;
+ loading = false;
+ submitting = false;
+ submitted = false;
+
+ constructor(
+ private route: ActivatedRoute,
+ private router: Router,
+ private fb: FormBuilder,
+ private readonly dialog: MatDialog,
+ private 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
+ this.statusDetails = await this.statusesService.getStatuses(id);
+ const data = {
+ name: this.statusDetails.name,
+ description: this.statusDetails.description,
+ lastUpdated: new Date(),
+ id: this.statusDetails.id
+ } as any;
Review Comment:
Nothing may ever be typed as `any`. Even implicitly, `any` should never be the type of anything.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.module.ts:
##########
@@ -0,0 +1,36 @@
+import { NgModule } from '@angular/core';
Review Comment:
This file is missing a license header. See other typescript files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts:
##########
@@ -0,0 +1,117 @@
+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 { StatusesModel, 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: StatusesModel | null = null;
+ statusDetailsForm!: FormGroup;
+ loading = false;
+ submitting = false;
+ submitted = false;
+
+ constructor(
+ private route: ActivatedRoute,
+ private router: Router,
+ private fb: FormBuilder,
+ private readonly dialog: MatDialog,
+ private 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
Review Comment:
don't use `as`, in general. Sometimes it's very hard to avoid, but in those cases I'd ask that you add a comment explaining why it's necessary
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.html:
##########
@@ -0,0 +1,9 @@
+<mat-card class="table-page-content">
Review Comment:
This file is missing a license header. See other template HTML files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.spec.ts:
##########
@@ -0,0 +1,23 @@
+import { ComponentFixture, TestBed } from '@angular/core/testing';
+
+import { StatusesTableComponent } from './statuses-table.component';
+
+describe('StatusesTableComponent', () => {
+ let component: StatusesTableComponent;
+ let fixture: ComponentFixture<StatusesTableComponent>;
+
+ beforeEach(async () => {
+ await TestBed.configureTestingModule({
+ declarations: [ StatusesTableComponent ]
+ })
+ .compileComponents();
+
+ fixture = TestBed.createComponent(StatusesTableComponent);
+ component = fixture.componentInstance;
+ fixture.detectChanges();
+ });
+
+ it('should create', () => {
+ expect(component).toBeTruthy();
+ });
Review Comment:
the tests should cover more than just what Angular generates for you
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.spec.ts:
##########
@@ -0,0 +1,23 @@
+import { ComponentFixture, TestBed } from '@angular/core/testing';
Review Comment:
This file is missing a license header. See other typescript files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.ts:
##########
@@ -0,0 +1,63 @@
+import { Component, OnInit } from '@angular/core';
Review Comment:
This file is missing a license header. See other typescript files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.component.ts:
##########
@@ -0,0 +1,63 @@
+import { Component, OnInit } from '@angular/core';
+import { BehaviorSubject } from 'rxjs';
+import { StatusesModel, StatusesService } from 'src/app/api/statuses.service';
+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: any | null = null;
Review Comment:
Do not use the `any` type
##########
experimental/traffic-portal/src/app/core/statuses/statuses-table/statuses-table.module.ts:
##########
@@ -0,0 +1,32 @@
+import { NgModule } from '@angular/core';
Review Comment:
This file is missing a license header. See other typescript files for a comment block at the top that you can copy.
##########
experimental/traffic-portal/src/styles.scss:
##########
@@ -152,3 +152,33 @@ button {
bottom: 16px;
right: 16px;
}
+// Breadcrumbs
+/* Style the list */
+ul.breadcrumb {
+ padding: 5px 0px 10px 0px;
+ margin-bottom: 1.5em;
+ list-style: none;
+ border-bottom: 1px solid #efefef;
+ }
+
+ /* Display list items side by side */
+ ul.breadcrumb li {
+ display: inline;
+ font-size: 16px;
+ }
+
+ /* Add a slash symbol (/) before/behind each list item */
+ ul.breadcrumb li+li:before {
+ padding: 8px;
+ content: "/\00a0";
+ }
+
+ /* Add a color to all links inside the list */
+ ul.breadcrumb li a {
+ text-decoration: none;
+ }
+
+ /* Add a color on mouse-over */
+ ul.breadcrumb li a:hover {
+ text-decoration: underline;
+ }
Review Comment:
missing newline at EOFmany of these declarations could be collapsed by using SCSS syntax instead of just putting plain CSS in an SCSS file e.g.
```scss
ul.breadcrumb {
padding: 5px 0px 10px 0px;
margin-bottom: 1.5em;
list-style: none;
border-bottom: 1px solid #efefef;
li {
display: inline;
font-size: 16px;
/* Add a slash symbol (/) before/behind each list item */
&+li:before {
padding: 8px;
content: "/\00a0";
}
/* Add a color to all links inside the list */
a {
text-decoration: none;
/* Add a color on mouse-over */
&:hover {
text-decoration: underline;
}
}
}
```
##########
experimental/traffic-portal/src/app/shared/navigation/navigation.service.ts:
##########
@@ -143,6 +143,12 @@ export class NavigationService {
name: "Tenants"
}],
name: "Users"
+ }, {
+ children: [{
+ href: "/core/statuses",
+ name: "Statuses"
+ }],
+ name: "Configure"
Review Comment:
This is fine, because it's what TPv1 did, but fwiw I'd put it under Servers. If we want to do that it'll be trivial to do later, so it's not a big deal either way.
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.html:
##########
@@ -0,0 +1,26 @@
+<mat-card class="statuses-details">
+ <ul class="breadcrumb">
+ <li><a routerLink="/core/statuses">Statuses</a></li>
+ <li>{{title}}</li>
+ </ul>
+ <tp-loading *ngIf="loading"></tp-loading>
+ <form [formGroup]="statusDetailsForm" (ngSubmit)="onSubmit()" *ngIf="!loading">
+ <mat-card-content>
+ <mat-form-field class="example-full-width">
+ <mat-label>Name</mat-label>
+ <input matInput placeholder="Ex. Pizza" formControlName="name">
Review Comment:
Shouldn't this be a `required` input?
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org