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


##########
dev/tpv2/run.sh:
##########
@@ -35,5 +35,5 @@ if [[ "$(id -u)" != "$uid" ]]; then
        exec su "$user" -- "$0"
 fi
 
-npm ci --ignore-scripts
+npm i --save-dev --ignore-scripts

Review Comment:
   Installing dev dependencies is the implicit default behavior, so you don't 
need to use `--save-dev` here. I don't think it has any effect at all, actually.



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

Review Comment:
   Ideally anywhere you pass an identifier for something, you should also be 
able to just pass the thing itself in its response representation (except in 
the `get*` methods because if you already have the thing what do you need to 
fetch it for?)



##########
experimental/traffic-portal/nightwatch/tests/cacheGroups/physLoc/detail.spec.ts:
##########
@@ -0,0 +1,52 @@
+/*

Review Comment:
   I'm not sure it's proper to have physical locations as a subdirectory of 
cache groups - the two are unrelated, if similar in concept



##########
experimental/traffic-portal/src/app/api/cache-group.service.ts:
##########
@@ -91,6 +98,77 @@ export class CacheGroupService extends APIService {
                );
        }
 
+       public async getPhysicalLocations(): 
Promise<Array<ResponsePhysicalLocation>>;
+       public async getPhysicalLocations(nameOrID: string | number): 
Promise<ResponsePhysicalLocation>;

Review Comment:
   I think Physical Locations should either have their own service or be in the 
servers service, because they're unrelated to cache groups



##########
experimental/traffic-portal/nightwatch/globals/globals.ts:
##########
@@ -44,28 +30,55 @@ import {
        ResponseDivision,
        RequestDivision,
        ResponseRegion,
-       RequestRegion
+       RequestRegion, ResponsePhysicalLocation, RequestPhysicalLocation

Review Comment:
   These should be on separate lines, like the other imports



##########
experimental/traffic-portal/src/app/core/cache-groups/phys-loc/detail/phys-loc-detail.component.html:
##########
@@ -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.
+-->
+
+<mat-card>
+       <tp-loading *ngIf="!physLocation"></tp-loading>
+       <form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="physLocation">
+               <mat-card-content>
+                       <mat-form-field>
+                               <mat-label>Name</mat-label>
+                               <input matInput type="text" name="name" 
required [(ngModel)]="physLocation.name" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Short Name</mat-label>
+                               <input matInput type="text" name="shortName" 
required [(ngModel)]="physLocation.shortName" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Address</mat-label>
+                               <input matInput type="text" name="address" 
required [(ngModel)]="physLocation.address" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>City</mat-label>
+                               <input matInput type="text" name="city" 
required [(ngModel)]="physLocation.city" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>State</mat-label>
+                               <input matInput type="text" name="state" 
required [(ngModel)]="physLocation.state" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Zip</mat-label>
+                               <input matInput type="text" name="zip" required 
[(ngModel)]="physLocation.zip" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Poc</mat-label>

Review Comment:
   Poc stands for "Point of Contact", which should be spelled out because 
otherwise nobody knows what that means.



##########
experimental/traffic-portal/src/app/core/cache-groups/cache-group-table/cache-group-table.component.ts:
##########
@@ -32,7 +32,7 @@ import {TpHeaderService} from 
"src/app/shared/tp-header/tp-header.service";
 })
 export class CacheGroupTableComponent implements OnInit {
 
-       /** All of the servers which should appear in the table. */
+       /** All of the cache- groups1 which should appear in the table. */

Review Comment:
   typo: "cache- groups1" -> "Cache Groups"



##########
experimental/traffic-portal/src/app/core/cache-groups/phys-loc/detail/phys-loc-detail.component.html:
##########
@@ -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.
+-->
+
+<mat-card>
+       <tp-loading *ngIf="!physLocation"></tp-loading>
+       <form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="physLocation">
+               <mat-card-content>
+                       <mat-form-field>
+                               <mat-label>Name</mat-label>
+                               <input matInput type="text" name="name" 
required [(ngModel)]="physLocation.name" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Short Name</mat-label>
+                               <input matInput type="text" name="shortName" 
required [(ngModel)]="physLocation.shortName" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Address</mat-label>
+                               <input matInput type="text" name="address" 
required [(ngModel)]="physLocation.address" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>City</mat-label>
+                               <input matInput type="text" name="city" 
required [(ngModel)]="physLocation.city" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>State</mat-label>
+                               <input matInput type="text" name="state" 
required [(ngModel)]="physLocation.state" />
+                       </mat-form-field>
+                       <mat-form-field>
+                               <mat-label>Zip</mat-label>

Review Comment:
   This should be "Postal Code" - "ZIP" is an America-specific acronym



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

Reply via email to