[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7243: Tpv2/regions

[GitBox <gi...@apache.org>]

ocket8888 commented on code in PR #7243:
URL: https://github.com/apache/trafficcontrol/pull/7243#discussion_r1047781026


##########
experimental/traffic-portal/nightwatch/globals/globals.ts:
##########
@@ -36,7 +38,7 @@ import {
 	ResponseCDN,
 	ResponseDeliveryService,
 	RequestTenant,
-	ResponseTenant, TypeFromResponse, RequestSteeringTarget, ResponseDivision, RequestDivision
+	ResponseTenant, TypeFromResponse, RequestSteeringTarget, ResponseDivision, RequestDivision, ResponseRegion, RequestRegion

Review Comment:
   idk how/when this started, but these should all be on one line, or each on its own line. I suspect it won't fit on one line.



##########
experimental/traffic-portal/src/app/api/cache-group.service.ts:
##########
@@ -162,6 +162,77 @@ export class CacheGroupService extends APIService {
 		return this.delete<ResponseDivision>(`divisions/${id}`).toPromise();
 	}
 
+	public async getRegions(): Promise<Array<ResponseRegion>>;
+	public async getRegions(nameOrID: string | number): Promise<ResponseRegion>;
+
+	/**
+	 * Gets an array of regions from Traffic Ops.
+	 *
+	 * @param nameOrID If given, returns only the Region with the given name
+	 * (string) or ID (number).
+	 * @returns An Array of Region objects - or a single Region object if 'nameOrID'
+	 * was given.
+	 */
+	public async getRegions(nameOrID?: string | number): Promise<Array<ResponseRegion> | ResponseRegion> {
+		const path = "regions";
+		if(nameOrID) {
+			let params;
+			switch (typeof nameOrID) {
+				case "string":
+					params = {name: nameOrID};
+					break;
+				case "number":
+					params = {id: String(nameOrID)};
+			}
+			const r = await this.get<[ResponseRegion]>(path, undefined, params).toPromise();
+			return {...r[0], lastUpdated: new Date((r[0].lastUpdated as unknown as string).replace("+00", "Z"))};
+
+		}
+		const regions = await this.get<Array<ResponseRegion>>(path).toPromise();
+		return regions.map(
+			d => ({...d, lastUpdated: new Date((d.lastUpdated as unknown as string).replace("+00", "Z"))})
+		);
+	}
+
+	/**
+	 * Replaces the current definition of a region with the one given.
+	 *
+	 * @param region The new region.
+	 * @returns The updated region.
+	 */
+	public async updateRegion(region: ResponseRegion): Promise<ResponseRegion> {
+		const path = `regions/${region.id}`;
+		const response = await this.put<ResponseRegion>(path, region).toPromise();
+		return {
+			...response,
+			lastUpdated: new Date((response.lastUpdated as unknown as string).replace(" ", "T").replace("+00", "Z"))
+		};
+	}
+
+	/**
+	 * Creates a new region.
+	 *
+	 * @param region The region to create.
+	 * @returns The created region.
+	 */
+	public async createRegion(region: RequestRegion): Promise<ResponseRegion> {
+		const response = await this.post<ResponseRegion>("regions", region).toPromise();
+		return {
+			...response,
+			lastUpdated: new Date((response.lastUpdated as unknown as string).replace(" ", "T").replace("+00", "Z"))
+		};
+	}
+
+	/**
+	 * Deletes an existing region.
+	 *
+	 * @param id Id of the region to delete.
+	 * @returns The deleted region.
+	 */
+	public async deleteRegion(id: number): Promise<ResponseRegion> {

Review Comment:
   Ideally, anywhere you can pass the ID of something, you should be able to pass that thing - so in this case `number | ResponseRegion`.
   
   also, R.E. the return type: the docs for this method of the endpoint don't seem to indicate that it returns a representation of the deleted Region - so shouldn't this just be `void`? I could be wrong, I didn't make the request myself to verify.



##########
experimental/traffic-portal/src/app/core/cache-groups/regions/table/regions-table.component.ts:
##########
@@ -0,0 +1,139 @@
+/*
+* 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, Router } from "@angular/router";
+import { BehaviorSubject } from "rxjs";
+import { Region, ResponseRegion } from "trafficops-types";
+
+import { CacheGroupService } from "src/app/api";
+import { CurrentUserService } from "src/app/shared/currentUser/current-user.service";
+import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component";
+import { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component";
+import { TpHeaderService } from "src/app/shared/tp-header/tp-header.service";
+
+/**
+ * RegionsTableComponent is the controller for the "Regions" table.
+ */
+@Component({
+	selector: "tp-regions",
+	styleUrls: ["./regions-table.component.scss"],
+	templateUrl: "./regions-table.component.html"
+})
+export class RegionsTableComponent implements OnInit {
+	/** List of regions */
+	public readonly regions: Promise<Array<ResponseRegion>>;
+
+	constructor(private readonly route: ActivatedRoute, private readonly headerSvc: TpHeaderService, private readonly router: Router,
+		private readonly api: CacheGroupService, private readonly dialog: MatDialog, public readonly auth: CurrentUserService) {
+		this.fuzzySubject = new BehaviorSubject<string>("");
+		this.regions = this.api.getRegions();
+	}
+
+	/** 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();
+				}
+			},
+			e => {
+				console.error("Failed to get query parameters:", e);
+			}
+		);
+		this.headerSvc.headerTitle.next("Regions");
+	}
+
+	/** Definitions of the table's columns according to the ag-grid API */
+	public columnDefs = [
+		{
+			field: "name",
+			headerName: "Name"
+		},
+		{
+			field: "divisionName",
+			headerName: "Division"
+		},
+		{
+			field: "id",
+			headerName:" ID",
+			hide: true
+		},
+		{
+			field: "lastUpdated",
+			headerName: "Last Updated"
+		}
+	];
+
+	/** Definitions for the context menu items (which act on augmented region data). */
+	public contextMenuItems: Array<ContextMenuItem<Region>> = [
+		{
+			action: "edit",
+			multiRow: false,
+			name: "Edit"
+		},
+		{
+			action: "delete",
+			multiRow: false,
+			name: "Delete"
+		},
+		{
+			action: "viewRegions",
+			multiRow: false,
+			name: "View Regions"
+		}
+	];
+
+	/** 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: FormControl = new FormControl<string>("");
+
+	/** 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<ResponseRegion>): Promise<void> {
+		const data = evt.data as ResponseRegion;
+		switch(evt.action) {
+			case "delete":
+				const ref = this.dialog.open(DecisionDialogComponent, {
+					data: {message: `Are you sure you want to delete region ${data.name} with id ${data.id}`, title: "Confirm Delete"}
+				});
+				ref.afterClosed().subscribe(result => {
+					if(result) {
+						this.api.deleteRegion(data.id);
+					}
+				});
+				break;
+			case "edit":
+				await this.router.navigate(["/core/region", data.id]);
+				break;
+			case "viewRegions":

Review Comment:
   regions shouldn't have a button for viewing its regions. They might have a context menu entry for viewing the parent division, and should probably have one for viewing its child physical locations, though. You don't need to implement either of those, but atm doing the Division one is possible and shouldn't be too much more code, if you'd like to give it a try.



##########
experimental/traffic-portal/src/app/core/cache-groups/regions/table/regions-table.component.ts:
##########
@@ -0,0 +1,139 @@
+/*
+* 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, Router } from "@angular/router";
+import { BehaviorSubject } from "rxjs";
+import { Region, ResponseRegion } from "trafficops-types";
+
+import { CacheGroupService } from "src/app/api";
+import { CurrentUserService } from "src/app/shared/currentUser/current-user.service";
+import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component";
+import { ContextMenuActionEvent, ContextMenuItem } from "src/app/shared/generic-table/generic-table.component";
+import { TpHeaderService } from "src/app/shared/tp-header/tp-header.service";
+
+/**
+ * RegionsTableComponent is the controller for the "Regions" table.
+ */
+@Component({
+	selector: "tp-regions",
+	styleUrls: ["./regions-table.component.scss"],
+	templateUrl: "./regions-table.component.html"
+})
+export class RegionsTableComponent implements OnInit {
+	/** List of regions */
+	public readonly regions: Promise<Array<ResponseRegion>>;
+
+	constructor(private readonly route: ActivatedRoute, private readonly headerSvc: TpHeaderService, private readonly router: Router,
+		private readonly api: CacheGroupService, private readonly dialog: MatDialog, public readonly auth: CurrentUserService) {
+		this.fuzzySubject = new BehaviorSubject<string>("");
+		this.regions = this.api.getRegions();
+	}
+
+	/** 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();
+				}
+			},
+			e => {
+				console.error("Failed to get query parameters:", e);
+			}
+		);
+		this.headerSvc.headerTitle.next("Regions");
+	}
+
+	/** Definitions of the table's columns according to the ag-grid API */
+	public columnDefs = [
+		{
+			field: "name",
+			headerName: "Name"
+		},
+		{
+			field: "divisionName",
+			headerName: "Division"
+		},
+		{
+			field: "id",
+			headerName:" ID",
+			hide: true
+		},
+		{
+			field: "lastUpdated",
+			headerName: "Last Updated"
+		}
+	];
+
+	/** Definitions for the context menu items (which act on augmented region data). */
+	public contextMenuItems: Array<ContextMenuItem<Region>> = [
+		{
+			action: "edit",
+			multiRow: false,
+			name: "Edit"
+		},
+		{
+			action: "delete",
+			multiRow: false,
+			name: "Delete"
+		},
+		{
+			action: "viewRegions",
+			multiRow: false,
+			name: "View Regions"
+		}
+	];
+
+	/** 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: FormControl = new FormControl<string>("");

Review Comment:
   I _think_ this is abandoning any typing on the `FormControl` because it's not specifying the type parameter. That may be wrong, but in any case the type of a variable shouldn't be broader than the the type of the expression to which it's being assigned, in general. On the other hand, the expression is a simple constructor call, so the type is trivially inferred from the type of the expression, so you could also just leave the type annotation off entirely.



##########
experimental/traffic-portal/src/app/core/cache-groups/regions/detail/region-detail.component.ts:
##########
@@ -0,0 +1,111 @@
+/*
+* 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 {ResponseDivision, ResponseRegion} from "trafficops-types";
+
+import { CacheGroupService } from "src/app/api";
+import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component";
+import { TpHeaderService } from "src/app/shared/tp-header/tp-header.service";
+
+/**
+ * RegionDetailsComponent is the controller for the region add/edit form.
+ */
+@Component({
+	selector: "tp-regions-detail",
+	styleUrls: ["./region-detail.component.scss"],
+	templateUrl: "./region-detail.component.html"
+})
+export class RegionDetailComponent implements OnInit {
+	public new = false;
+	public region!: ResponseRegion;
+	public divisions!: Array<ResponseDivision>;
+
+	constructor(private readonly route: ActivatedRoute, private readonly cacheGroupService: CacheGroupService,
+		private readonly location: Location, private readonly dialog: MatDialog,
+		private readonly header: TpHeaderService) {
+	}
+
+	/**
+	 * Angular lifecycle hook where data is initialized.
+	 */
+	public async ngOnInit(): Promise<void> {
+		this.divisions = await this.cacheGroupService.getDivisions();
+		const ID = this.route.snapshot.paramMap.get("id");
+		if (ID === null) {
+			console.error("missing required route parameter 'id'");
+			return;
+		}
+
+		if (ID === "new") {
+			this.header.headerTitle.next("New Region");
+			this.new = true;
+			this.region = {
+				division: -1,
+				divisionName: "",
+				id: -1,
+				lastUpdated: new Date(),
+				name: ""
+			};
+			return;
+		}
+		const numID = parseInt(ID, 10);
+		if (Number.isNaN(numID)) {
+			console.error("route parameter 'id' was non-number:", ID);
+			return;
+		}
+
+		this.region = await this.cacheGroupService.getRegions(numID);
+		this.header.headerTitle.next(`Region: ${this.region.name}`);
+	}
+
+	/**
+	 * Deletes the current region.
+	 */
+	public async deleteRegion(): Promise<void> {
+		if (this.new) {
+			console.error("Unable to delete new region");
+			return;
+		}
+		const ref = this.dialog.open(DecisionDialogComponent, {
+			data: {message: `Are you sure you want to delete region ${this.region.name} with id ${this.region.id}`,
+				title: "Confirm Delete"}
+		});
+		ref.afterClosed().subscribe(result => {
+			if(result) {
+				this.cacheGroupService.deleteRegion(this.region.id);
+				this.location.back();
+			}
+		});
+	}
+
+	/**
+	 * Submits new/updated region.
+	 *
+	 * @param e HTML click event.

Review Comment:
   You don't need to change this, but it's actually a form submission event, not a click. Click bubbles to completion before submit fires, only if the click's default is not prevented when the target or an ancestor of the target is a `button[type=submit]` and/or `input[type=submit]`.



##########
experimental/traffic-portal/src/app/core/cache-groups/regions/detail/region-detail.component.ts:
##########
@@ -0,0 +1,111 @@
+/*
+* 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 {ResponseDivision, ResponseRegion} from "trafficops-types";

Review Comment:
   two nits here: these are only used as types, so they could be imported using `import type`, and also the brace spacing is inconsistent. I really need to set up prettier or something.



##########
experimental/traffic-portal/src/app/api/cache-group.service.ts:
##########
@@ -162,6 +162,77 @@ export class CacheGroupService extends APIService {
 		return this.delete<ResponseDivision>(`divisions/${id}`).toPromise();
 	}
 
+	public async getRegions(): Promise<Array<ResponseRegion>>;
+	public async getRegions(nameOrID: string | number): Promise<ResponseRegion>;
+
+	/**
+	 * Gets an array of regions from Traffic Ops.
+	 *
+	 * @param nameOrID If given, returns only the Region with the given name
+	 * (string) or ID (number).
+	 * @returns An Array of Region objects - or a single Region object if 'nameOrID'
+	 * was given.
+	 */
+	public async getRegions(nameOrID?: string | number): Promise<Array<ResponseRegion> | ResponseRegion> {
+		const path = "regions";
+		if(nameOrID) {
+			let params;
+			switch (typeof nameOrID) {
+				case "string":
+					params = {name: nameOrID};
+					break;
+				case "number":
+					params = {id: String(nameOrID)};

Review Comment:
   Casting to a `string` like this is unnecessary, the HTTP client will check that for you.



-- 
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