ocket8888 commented on code in PR #7532: URL: https://github.com/apache/trafficcontrol/pull/7532#discussion_r1204672928
########## experimental/traffic-portal/src/app/api/export-attachment.service.ts: ########## @@ -0,0 +1,50 @@ +/* +* 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 { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import { ProfileExport, ResponseProfile } from "trafficops-types"; Review Comment: nit: can be `import type` as these are both types ########## experimental/traffic-portal/src/app/api/export-attachment.service.ts: ########## @@ -0,0 +1,50 @@ +/* +* 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 { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import { ProfileExport, ResponseProfile } from "trafficops-types"; + +import { environment } from "src/environments/environment"; + +/** + * Defines & handles api endpoints related to export attachments/json/text + * Here we are not using the base-api-service to tap the response. + */ +@Injectable() +export class ExportAttachmentService { Review Comment: I don't think we need a new service for this, I think exporting a profile should be in the Profile Service, which is also where you put Profile imports, so I think they ought to go together. ########## experimental/traffic-portal/src/app/shared/import-json-edit-txt/import-json-edit-txt.component.html: ########## @@ -0,0 +1,36 @@ +<!-- +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. +--> + +<h2 mat-dialog-title>{{title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> Review Comment: I think we should get rid of this component/functionality entirely. I know TPv1 does it, but text editors exist and they do this job a thousand times better than we ever could, so let's not even try. ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -103,4 +103,13 @@ export class ProfileService extends APIService { return this.delete<ResponseProfile>(`profiles/${id}`).toPromise(); } + /** + * Import profile + * + * @param importJSON JSON object for import. + * @returns profile response for imported object. + */ + public async importProfile(importJSON: ProfileImport): Promise<ProfileImportResponse>{ Review Comment: this service method needs test coverage ########## experimental/traffic-portal/src/app/shared/file-utils.service.ts: ########## @@ -153,4 +153,21 @@ export class FileUtilsService { this.window.open(url, "_blank"); URL.revokeObjectURL(url); } + + /** + * Exports File + * + * @param json object to be exported + * @param fileName name for the download file + * @param fileExtension extenstion of export file + */ + public exportFile(json: object, fileName: string, fileExtension: string): void { Review Comment: This method is unnecessary, it doesn't do anything that the `download` method doesn't already do. ########## experimental/traffic-portal/src/app/shared/interceptor/alerts.interceptor.ts: ########## @@ -41,7 +41,8 @@ export class AlertInterceptor implements HttpInterceptor { return next.handle(request).pipe(tap( r => { if (Object.prototype.hasOwnProperty.call(r, "body") && - Object.prototype.hasOwnProperty.call((r as {body: unknown}).body, "alerts")) { + Object.prototype.hasOwnProperty.call((r as {body: unknown}).body, "alerts") && + (r as {body: {alerts: Array<unknown>}}).body?.alerts !== null) { //Ignore alerts with null value Review Comment: the only thing I'd change about this is that your `?.` safe-access operator on the `alerts` property of `r.body` is unnecessary; the previous check ensures that `body` is non-null and has an `alerts` property ########## experimental/traffic-portal/src/app/core/profiles/profile-table/profile-table.component.ts: ########## @@ -17,22 +17,28 @@ import { FormControl, UntypedFormControl } from "@angular/forms"; import { MatDialog } from "@angular/material/dialog"; import { ActivatedRoute, Params } from "@angular/router"; import { BehaviorSubject } from "rxjs"; -import { ResponseProfile } from "trafficops-types"; +import { ProfileImport, ResponseProfile } from "trafficops-types"; import { ProfileService } from "src/app/api"; +import { ExportAttachmentService } from "src/app/api/export-attachment.service"; import { CurrentUserService } from "src/app/shared/current-user/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 { FileUtilsService } from "src/app/shared/file-utils.service"; +import { ContextMenuActionEvent, ContextMenuItem, TableTitleButton } from "src/app/shared/generic-table/generic-table.component"; +import { ImportJsonEditTxtComponent } from "src/app/shared/import-json-edit-txt/import-json-edit-txt.component"; import { NavigationService } from "src/app/shared/navigation/navigation.service"; /** * ProfileTableComponent is the controller for the profiles page - which * principally contains a table. */ @Component({ + providers: [ + FileUtilsService + ], Review Comment: You shouldn't need this, it's in the shared module, which is imported by the core module to which this component belongs ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -103,4 +103,13 @@ export class ProfileService extends APIService { return this.delete<ResponseProfile>(`profiles/${id}`).toPromise(); } + /** + * Import profile + * + * @param importJSON JSON object for import. + * @returns profile response for imported object. + */ + public async importProfile(importJSON: ProfileImport): Promise<ProfileImportResponse>{ Review Comment: The ProfileService in the API testing module needs to have this method added as well ########## experimental/traffic-portal/src/app/api/export-attachment.service.ts: ########## @@ -0,0 +1,50 @@ +/* +* 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 { HttpClient } from "@angular/common/http"; +import { Injectable } from "@angular/core"; +import { ProfileExport, ResponseProfile } from "trafficops-types"; + +import { environment } from "src/environments/environment"; + +/** + * Defines & handles api endpoints related to export attachments/json/text + * Here we are not using the base-api-service to tap the response. + */ +@Injectable() +export class ExportAttachmentService { + + /** + * The API version used by the service(s) - this will be overridden by the + * environment if a different API version is therein found. + */ + public apiVersion = "4.1"; Review Comment: this shouldn't be here, you should just extend the base class (if necessary) and also we can't use APIv4.1 as that's not released and `trafficops-types` doesn't support it. ########## experimental/traffic-portal/src/app/core/profiles/profile-table/profile-table.component.ts: ########## @@ -170,6 +184,37 @@ export class ProfileTableComponent implements OnInit { } }); break; + case "export-profile": + const response = await this.exportService.exportProfile(data.id); + Reflect.deleteProperty(response, "alerts"); Review Comment: I don't think this line is necessary ########## experimental/traffic-portal/src/app/core/profiles/profile-table/profile-table.component.ts: ########## @@ -125,14 +137,16 @@ export class ProfileTableComponent implements OnInit { private readonly route: ActivatedRoute, private readonly navSvc: NavigationService, private readonly dialog: MatDialog, - public readonly auth: CurrentUserService) { + public readonly auth: CurrentUserService, + private readonly exportService: ExportAttachmentService, + private readonly fileUtil: FileUtilsService) { this.fuzzySubject = new BehaviorSubject<string>(""); this.profiles = this.api.getProfiles(); this.navSvc.headerTitle.next("Profiles"); } /** Initializes table data, loading it from Traffic Ops. */ - public ngOnInit(): void { + public async ngOnInit(): Promise<void> { Review Comment: why change this? -- 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]
