ocket8888 commented on code in PR #7514: URL: https://github.com/apache/trafficcontrol/pull/7514#discussion_r1201122261
########## experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.spec.ts: ########## @@ -0,0 +1,143 @@ +/* +* 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 { MatDialog, MatDialogModule, MatDialogRef } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { of, ReplaySubject } from "rxjs"; + +import { UserService } from "src/app/api"; +import { APITestingModule } from "src/app/api/testing"; +import { RoleDetailComponent } from "src/app/core/users/roles/detail/role-detail.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +describe("RoleDetailComponent", () => { + let component: RoleDetailComponent; + let fixture: ComponentFixture<RoleDetailComponent>; + let route: ActivatedRoute; + let paramMap: jasmine.Spy; + // const name = "test"; + + const headerSvc = jasmine.createSpyObj([],{headerHidden: new ReplaySubject<boolean>(), headerTitle: new ReplaySubject<string>()}); + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [ RoleDetailComponent ], + imports: [ + APITestingModule, + RouterTestingModule.withRoutes([ + {component: RoleDetailComponent, path: "core/roles/:name"}, + // This route is never actually loaded, but the tests + // complain that it can't be routed to, so it doesn't matter + // that it's loading the wrong component, only that it has a + // route definition. + {component: RoleDetailComponent, path: "core/roles"} + ]), + MatDialogModule + ], + providers: [ { provide: NavigationService, useValue: headerSvc } ] + }) + .compileComponents(); + + route = TestBed.inject(ActivatedRoute); + paramMap = spyOn(route.snapshot.paramMap, "get"); + // paramMap.and.returnValue(name); Review Comment: unused commented-out code should just be removed ########## experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.ts: ########## @@ -0,0 +1,127 @@ +/* +* 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, Router} from "@angular/router"; +import { ResponseRole } from "trafficops-types"; + +import { UserService } from "src/app/api"; +import { DecisionDialogComponent } from "src/app/shared/dialogs/decision-dialog/decision-dialog.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +/** + * RoleDetailComponent is the controller for the Role add/edit form. + */ +@Component({ + selector: "tp-role-detail", + styleUrls: ["../../../styles/form.page.scss"], + templateUrl: "./role-detail.component.html" +}) +export class RoleDetailComponent implements OnInit { + public new = false; + public permissions = ""; + public role!: ResponseRole; + + /** + * This caches the original name of the Role, so that updates can be + * made. + */ + private name = ""; + + constructor(private readonly route: ActivatedRoute, private readonly router: Router, + private readonly userService: UserService, private readonly location: Location, + private readonly dialog: MatDialog, private readonly header: NavigationService) { + } + + /** + * Angular lifecycle hook where data is initialized. + */ + public async ngOnInit(): Promise<void> { + const role = this.route.snapshot.paramMap.get("name"); + if (role === null) { + this.header.headerTitle.next("New Role"); + this.new = true; + this.role = { + description: "", + name: "", + permissions: [] + }; + return; + } + + this.role = await this.userService.getRoles(role); + this.name = this.role.name; + this.permissions = this.role.permissions?.join("\n")??""; + this.header.headerTitle.next(`Role: ${this.role.name}`); Review Comment: nit but lines 65 and 67 repeat the contents of `setHeader` with the argument `this.role.name`; should just call the method instead. ########## experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.spec.ts: ########## @@ -0,0 +1,143 @@ +/* +* 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 { MatDialog, MatDialogModule, MatDialogRef } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { of, ReplaySubject } from "rxjs"; + +import { UserService } from "src/app/api"; +import { APITestingModule } from "src/app/api/testing"; +import { RoleDetailComponent } from "src/app/core/users/roles/detail/role-detail.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +describe("RoleDetailComponent", () => { + let component: RoleDetailComponent; + let fixture: ComponentFixture<RoleDetailComponent>; + let route: ActivatedRoute; + let paramMap: jasmine.Spy; + // const name = "test"; + + const headerSvc = jasmine.createSpyObj([],{headerHidden: new ReplaySubject<boolean>(), headerTitle: new ReplaySubject<string>()}); + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [ RoleDetailComponent ], + imports: [ + APITestingModule, + RouterTestingModule.withRoutes([ + {component: RoleDetailComponent, path: "core/roles/:name"}, + // This route is never actually loaded, but the tests + // complain that it can't be routed to, so it doesn't matter + // that it's loading the wrong component, only that it has a + // route definition. + {component: RoleDetailComponent, path: "core/roles"} + ]), + MatDialogModule + ], + providers: [ { provide: NavigationService, useValue: headerSvc } ] + }) + .compileComponents(); + + route = TestBed.inject(ActivatedRoute); + paramMap = spyOn(route.snapshot.paramMap, "get"); + // paramMap.and.returnValue(name); + fixture = TestBed.createComponent(RoleDetailComponent); + component = fixture.componentInstance; + component.role = {...await TestBed.inject(UserService).createRole(component.role)}; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + // expect(paramMap).toHaveBeenCalled(); + }); + + it("new role", async () => { + paramMap.and.returnValue(null); + + fixture = TestBed.createComponent(RoleDetailComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + await fixture.whenStable(); + expect(paramMap).toHaveBeenCalled(); + expect(component.role).not.toBeNull(); + expect(component.role.name).toBe(""); + expect(component.new).toBeTrue(); + }); + + it("existing role", async () => { + paramMap.and.returnValue("admin"); + + fixture = TestBed.createComponent(RoleDetailComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + await fixture.whenStable(); + expect(paramMap).toHaveBeenCalled(); + expect(component.role).not.toBeNull(); + expect(component.role.name).toBe("admin"); + expect(component.new).toBeFalse(); + }); + + it("opens a dialog for role deletion", async () => { + const api = TestBed.inject(UserService); + const spy = spyOn(api, "deleteRole").and.callThrough(); + expect(spy).not.toHaveBeenCalled(); + + const dialogService = TestBed.inject(MatDialog); + const openSpy = spyOn(dialogService, "open").and.returnValue({ + afterClosed: () => of(true) + } as MatDialogRef<unknown>); Review Comment: I don't think you need the `as` expression here? ########## experimental/traffic-portal/src/app/api/user.service.ts: ########## @@ -267,6 +268,38 @@ export class UserService extends APIService { return this.get<Array<ResponseRole>>(path).toPromise(); } + /** + * Creates a new Role. + * + * @param role The role to create. + * @returns The created role along with lastUpdated field. + */ + public async createRole(role: RequestRole): Promise<ResponseRole> { + return this.post<ResponseRole>("roles", role).toPromise(); + } + + /** + * Updates an existing Role. + * + * @param name The original role name + * @param role The role to update. + * @returns The updated role without lastUpdated field. + */ + public async updateRole(name: string, role: ResponseRole): Promise<ResponseRole> { + return this.put<ResponseRole>("roles?", role, {name}).toPromise(); + } + + /** + * Deletes an existing role. + * + * @param role The role to be deleted. + * @returns The deleted role. + */ + public async deleteRole(role: string | ResponseRole): Promise<void> { + const name = typeof(role) === "string" ? role : role.name; + return this.delete("roles", undefined, {name}).toPromise(); + } + Review Comment: these methods need tests ########## experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.spec.ts: ########## @@ -0,0 +1,143 @@ +/* +* 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 { MatDialog, MatDialogModule, MatDialogRef } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { of, ReplaySubject } from "rxjs"; + +import { UserService } from "src/app/api"; +import { APITestingModule } from "src/app/api/testing"; +import { RoleDetailComponent } from "src/app/core/users/roles/detail/role-detail.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +describe("RoleDetailComponent", () => { + let component: RoleDetailComponent; + let fixture: ComponentFixture<RoleDetailComponent>; + let route: ActivatedRoute; + let paramMap: jasmine.Spy; + // const name = "test"; + + const headerSvc = jasmine.createSpyObj([],{headerHidden: new ReplaySubject<boolean>(), headerTitle: new ReplaySubject<string>()}); + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [ RoleDetailComponent ], + imports: [ + APITestingModule, + RouterTestingModule.withRoutes([ + {component: RoleDetailComponent, path: "core/roles/:name"}, + // This route is never actually loaded, but the tests + // complain that it can't be routed to, so it doesn't matter + // that it's loading the wrong component, only that it has a + // route definition. + {component: RoleDetailComponent, path: "core/roles"} + ]), + MatDialogModule + ], + providers: [ { provide: NavigationService, useValue: headerSvc } ] + }) + .compileComponents(); + + route = TestBed.inject(ActivatedRoute); + paramMap = spyOn(route.snapshot.paramMap, "get"); + // paramMap.and.returnValue(name); + fixture = TestBed.createComponent(RoleDetailComponent); + component = fixture.componentInstance; + component.role = {...await TestBed.inject(UserService).createRole(component.role)}; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + // expect(paramMap).toHaveBeenCalled(); Review Comment: unused commented-out code should just be removed ########## experimental/traffic-portal/src/app/api/testing/user.service.ts: ########## @@ -398,34 +396,78 @@ export class UserService { } } - /** Fetches the Role with the given ID. */ - public async getRoles (nameOrID: number | string): Promise<ResponseRole>; + /** Fetches the Role with the given name. */ + public async getRoles (name: string): Promise<ResponseRole>; /** Fetches all Roles. */ public async getRoles (): Promise<Array<ResponseRole>>; /** * Fetches one or all Roles from Traffic Ops. * - * @param nameOrID Optionally, the name or integral, unique identifier of a single Role which will be fetched + * @param name unique identifier (name) of a single Role which will be fetched * @throws {TypeError} When called with an improper argument. * @returns Either an Array of Roles, or a single Role, depending on whether - * `name`/`id` was passed + * name was passed */ - public async getRoles(nameOrID?: string | number): Promise<Array<ResponseRole> | ResponseRole> { - if (nameOrID !== undefined) { + public async getRoles(name?: string | number): Promise<Array<ResponseRole> | ResponseRole> { Review Comment: there is no overload call signature that supports passing a number here; since it can't be used it'll make the function simpler to just remove the ` | number`. ########## experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.html: ########## @@ -0,0 +1,37 @@ +<!-- +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="!role"></tp-loading> + <form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="role"> + <mat-card-content> + <mat-form-field> + <mat-label>Name</mat-label> + <input matInput type="text" name="name" required [(ngModel)]="role.name" [readonly]="role.name === 'admin'"/> + </mat-form-field> + <mat-form-field> + <mat-label>Description</mat-label> + <input matInput type="text" name="description" [(ngModel)]="role.description" [readonly]="role.name === 'admin'" /> + </mat-form-field> + <mat-form-field> + <mat-label>Permissions</mat-label> + <textarea matInput name="permissions" [(ngModel)]="permissions" (input)="updatePermissions()" [readonly]="role.name === 'admin'" cdkTextareaAutosize></textarea> Review Comment: instead of doing this on `(input)` - every time the user presses a key - you can just wait until the form is submitted to set the Role's `permissions` based on this text box. ########## experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.spec.ts: ########## @@ -0,0 +1,143 @@ +/* +* 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 { MatDialog, MatDialogModule, MatDialogRef } from "@angular/material/dialog"; +import { ActivatedRoute } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { of, ReplaySubject } from "rxjs"; + +import { UserService } from "src/app/api"; +import { APITestingModule } from "src/app/api/testing"; +import { RoleDetailComponent } from "src/app/core/users/roles/detail/role-detail.component"; +import { NavigationService } from "src/app/shared/navigation/navigation.service"; + +describe("RoleDetailComponent", () => { + let component: RoleDetailComponent; + let fixture: ComponentFixture<RoleDetailComponent>; + let route: ActivatedRoute; + let paramMap: jasmine.Spy; + // const name = "test"; Review Comment: unused commented-out code should just be removed -- 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]
