ocket8888 commented on code in PR #7514:
URL: https://github.com/apache/trafficcontrol/pull/7514#discussion_r1192658562
##########
experimental/traffic-portal/src/app/api/testing/user.service.ts:
##########
@@ -85,6 +86,15 @@ export class UserService {
privLevel: 30
}
];
+ private readonly roleDetail: Array<ResponseRole> = [{
Review Comment:
This property needs to be consolidated with `roles` - right now, if you
create a role, you'll be unable to later get that role from `getRoles` because
they refer to different arrays. Specifically, it looks like this Role should
just replace the Role in `roles`.
##########
experimental/traffic-portal/src/app/api/testing/user.service.ts:
##########
@@ -428,6 +438,50 @@ export class UserService {
return this.roles;
}
+ /**
+ * Creates a new role.
+ *
+ * @param role The role to create.
+ * @returns The created role.
+ */
+ public async createRole(role: RequestRole): Promise<ResponseRole> {
+ const resp = {
+ ...role,
+ };
+ this.roleDetail.push(resp);
+ return resp;
+ }
+
+ /**
+ * Updates an existing role.
+ *
+ * @param role The role to update.
+ * @returns The updated role.
+ */
+ public async updateRole(role: ResponseRole): Promise<ResponseRole> {
+ const roleName = this.roleDetail.findIndex(r => r.name ===
role.name);
+ if (roleName < 0 ) {
+ throw new Error(`no such Role: ${role.name}`);
+ }
+ this.roleDetail[roleName] = role;
+ return role;
+ }
+
+ /**
+ * Deletes an existing role.
+ *
+ * @param role The role to be deleted.
+ * @returns The deleted role.
+ */
+ public async deleteRole(role: string | ResponseRole):
Promise<ResponseRole> {
Review Comment:
The call signature of this testing method is not compatible with the
"concrete" service; according to the documentation, your concrete service
method is correct and this should return `Promise<void>`.
##########
experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.ts:
##########
@@ -0,0 +1,110 @@
+/*
+* 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 { 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";
+
+/**
+ * AsnDetailComponent is the controller for the ASN add/edit form.
+ */
+@Component({
+ selector: "tp-role-detail",
+ styleUrls: ["./role-detail.component.scss"],
+ templateUrl: "./role-detail.component.html"
+})
+export class RoleDetailComponent implements OnInit {
+ public new = false;
+ public permissions = "";
+ public role!: ResponseRole;
+ constructor(private readonly route: ActivatedRoute, 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) {
+ console.error("missing required route parameter
'name'");
+ return;
+ }
+
+ if (role === "new") {
Review Comment:
This means that I can never create a Role named `new`. Even worse, if such a
Role were created either through TPv1 or the API, I'd be unable to edit it.
You should instead add a special route that takes _no_ parameters (e.g.
`new-role`) and when the `name` route parameter is `null` you know to create a
new Role. It's actually somewhat easier than the handling that needs to be done
for ID-based objects, since it's got one fewer unreachable error states
(technically; the component can be incorrectly embedded in a view where it may
load on the wrong route, but we don't do that here).
For an example of this, see the Capability component I added.
##########
experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.scss:
##########
@@ -0,0 +1,30 @@
+/*
+* 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 {
+ margin: 1em auto;
+ width: 80%;
+ min-width: 350px;
+
+ mat-card-content {
+ display: grid;
+ grid-template-columns: 1fr;
+ row-gap: 2em;
+ margin: 1em auto 50px;
+ textarea{
+ white-space: pre-wrap;
+ max-height: 250px;
+ }
+ }
+}
Review Comment:
Instead of repeating this here, we should make use of the
`src/app/core/styles/form.page.scss` stylesheet in the component's
`stylesheetUrls` decorator property.
##########
experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.ts:
##########
@@ -0,0 +1,110 @@
+/*
+* 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 { 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";
+
+/**
+ * AsnDetailComponent is the controller for the ASN add/edit form.
+ */
+@Component({
+ selector: "tp-role-detail",
+ styleUrls: ["./role-detail.component.scss"],
+ templateUrl: "./role-detail.component.html"
+})
+export class RoleDetailComponent implements OnInit {
+ public new = false;
+ public permissions = "";
+ public role!: ResponseRole;
+ constructor(private readonly route: ActivatedRoute, 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) {
+ console.error("missing required route parameter
'name'");
+ return;
+ }
+
+ if (role === "new") {
+ this.header.headerTitle.next("New Role");
+ this.new = true;
+ this.role = {
+ description: "",
+ name: "",
+ permissions: []
+ };
+ return;
+ }
+
+ this.role = await this.userService.getRoles(role);
+ this.permissions = this.role.permissions?.join("\n")??"";
+ this.header.headerTitle.next(`Role: ${this.role.name}`);
+ }
+
+ /**
+ * Deletes the current ASN.
+ */
+ public async deleteRole(): Promise<void> {
+ if (this.new) {
+ console.error("Unable to delete new role");
+ return;
+ }
+ const ref = this.dialog.open(DecisionDialogComponent, {
+ data: {message: `Are you sure you want to delete role
${this.role.name} with description ${this.role.description}`,
+ title: "Confirm Delete"}
+ });
+ ref.afterClosed().subscribe(result => {
+ if(result) {
+ this.userService.deleteRole(this.role);
+ this.location.back();
+ }
+ });
+ }
+
+ /**
+ * Updates permissions list from a string to an array.
+ */
+ public async updatePermissions(): Promise<void> {
+ this.role.permissions = this.permissions.split("\n");
+ }
+
+ /**
+ * Submits new/updated ASN.
+ *
+ * @param e HTML form submission event.
+ */
+ public async submit(e: Event): Promise<void> {
+ e.preventDefault();
+ e.stopPropagation();
+ if(this.new) {
+ this.role = await
this.userService.createRole(this.role);
+ this.new = false;
+ } else {
+ this.role = await
this.userService.updateRole(this.role);
+ }
Review Comment:
When you're done either creating or updating the role, you should use the
router and the navigation service to change the URL (without adding to history)
and update the page title, because the Role's name may have changed (or been
decided, if it was created).
##########
experimental/traffic-portal/src/app/api/user.service.ts:
##########
@@ -267,6 +268,37 @@ 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.
+ */
+ public async createRole(role: RequestRole): Promise<ResponseRole> {
+ return this.post<ResponseRole>("roles", role).toPromise();
+ }
+
+ /**
+ * Updates an existing Role.
+ *
+ * @param role The role to update.
+ * @returns The updated role.
+ */
+ public async updateRole(role: ResponseRole): Promise<ResponseRole> {
Review Comment:
This, unfortunately isn't enough information to update a Role. If you change
the Role's name, this method won't work because it relies on the name of the
Role _after_ editing - which must necessarily not exist. Therefore, the
signature must be of the form
```typescript
public async updateRole(name: string, role: Role): Promise<PutResponseRole>
```
(`Role` is fine to use without `Response` on the front - in general that's
going to be safe for any object that doesn't expose an ID).
##########
experimental/traffic-portal/src/app/api/user.service.ts:
##########
@@ -267,6 +268,37 @@ 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.
+ */
+ public async createRole(role: RequestRole): Promise<ResponseRole> {
Review Comment:
we can be more specific than this - a response from a POST request to
`/roles` has the `PostResponseRole` type. That lets callers use the
`lastUpdated` property, which `PUT` responses for whatever reason don't
provide.
##########
experimental/traffic-portal/src/app/api/user.service.ts:
##########
@@ -267,6 +268,37 @@ 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.
+ */
+ public async createRole(role: RequestRole): Promise<ResponseRole> {
+ return this.post<ResponseRole>("roles", role).toPromise();
+ }
+
+ /**
+ * Updates an existing Role.
+ *
+ * @param role The role to update.
+ * @returns The updated role.
+ */
+ public async updateRole(role: ResponseRole): Promise<ResponseRole> {
Review Comment:
we can be more specific than this - a response from a PUT request to
`/roles` has the `PutResponseRole` type. That lets callers know they can never
use the `lastUpdated` property, which `PUT` responses for whatever reason don't
provide.
##########
experimental/traffic-portal/src/app/core/users/roles/detail/role-detail.component.ts:
##########
@@ -0,0 +1,110 @@
+/*
+* 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 { 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";
+
+/**
+ * AsnDetailComponent is the controller for the ASN add/edit form.
+ */
+@Component({
+ selector: "tp-role-detail",
+ styleUrls: ["./role-detail.component.scss"],
+ templateUrl: "./role-detail.component.html"
+})
+export class RoleDetailComponent implements OnInit {
+ public new = false;
+ public permissions = "";
+ public role!: ResponseRole;
+ constructor(private readonly route: ActivatedRoute, 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) {
+ console.error("missing required route parameter
'name'");
+ return;
+ }
+
+ if (role === "new") {
+ this.header.headerTitle.next("New Role");
+ this.new = true;
+ this.role = {
+ description: "",
+ name: "",
+ permissions: []
+ };
+ return;
+ }
+
+ this.role = await this.userService.getRoles(role);
+ this.permissions = this.role.permissions?.join("\n")??"";
+ this.header.headerTitle.next(`Role: ${this.role.name}`);
+ }
+
+ /**
+ * Deletes the current ASN.
+ */
+ public async deleteRole(): Promise<void> {
+ if (this.new) {
+ console.error("Unable to delete new role");
+ return;
+ }
+ const ref = this.dialog.open(DecisionDialogComponent, {
+ data: {message: `Are you sure you want to delete role
${this.role.name} with description ${this.role.description}`,
Review Comment:
I think you should leave out the description, that could get pretty long.
##########
experimental/traffic-portal/src/app/api/user.service.ts:
##########
@@ -267,6 +268,37 @@ 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.
+ */
+ public async createRole(role: RequestRole): Promise<ResponseRole> {
+ return this.post<ResponseRole>("roles", role).toPromise();
+ }
+
+ /**
+ * Updates an existing Role.
+ *
+ * @param role The role to update.
+ * @returns The updated role.
+ */
+ public async updateRole(role: ResponseRole): Promise<ResponseRole> {
+ return this.put<ResponseRole>(`roles?name=${role.name}`,
role).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 roleName = typeof(role) === "string" ? role : role.name;
+ return this.delete(`roles?name=${roleName}`).toPromise();
+ }
Review Comment:
These functions are not covered by tests, which brings the test coverage for
the user service down to 94.02% of statements from 100% of statements.
##########
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]="!new"
cdkTextareaAutosize></textarea>
Review Comment:
So, this is readonly if the Role already exists, so you can never change the
Permissions of any Role? That doesn't seem right.
--
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]
