ocket8888 commented on code in PR #7532: URL: https://github.com/apache/trafficcontrol/pull/7532#discussion_r1218505485
########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.scss: ########## @@ -0,0 +1,56 @@ +/* +* 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. +*/ + +$backgroundColor: #fafaec; + +.dropzone { + min-height: 75px; + min-width: 400px; + margin: 1rem auto; + display: table; + width: 99%; + background-color: $backgroundColor; + border: solid 1px rgb(231, 230, 230); + text-align: center; + color: rgb(133, 132, 132); + border-image: initial; + border-radius: 5px; + box-shadow: 0 .125rem .25rem rgba(0, 0, 0, .075); + cursor: pointer; + + .text-wrapper { + display: table-cell; + vertical-align: middle; + + .hint { + color: rgb(251, 192, 82); + } + } +} + +.mdc-dialog__content{ Review Comment: do not use Angular Material-internal class selectors; e.g. this should just be `mat-dialog-content`. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.spec.ts: ########## @@ -0,0 +1,45 @@ +/* +* 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 { DatePipe } from "@angular/common"; +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { MatDialogModule, MatDialogRef } from "@angular/material/dialog"; + +import { ImportJsonTxtComponent } from "./import-json-txt.component"; + +describe("ImportJsonTxtComponent", () => { + let component: ImportJsonTxtComponent; + let fixture: ComponentFixture<ImportJsonTxtComponent>; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [ ImportJsonTxtComponent ], + imports: [ + MatDialogModule + ], + providers: [ + DatePipe, + {provide: MatDialogRef, useValue: {}} + ] + }).compileComponents(); + + fixture = TestBed.createComponent(ImportJsonTxtComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("should create", () => { + expect(component).toBeTruthy(); + }); Review Comment: this should have at least one non-trivial test case ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.html: ########## @@ -0,0 +1,38 @@ +<!-- +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>{{data.title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> + <input type='file' id="imageUpload" accept=".json, .txt" #fileInput (change)="uploadFile($event)" hidden/> Review Comment: The `accept` attribute of an `input[type="file"]` is for specifying MIME types, not file extensions. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.html: ########## @@ -0,0 +1,38 @@ +<!-- +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>{{data.title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> + <input type='file' id="imageUpload" accept=".json, .txt" #fileInput (change)="uploadFile($event)" hidden/> + <a type="button" class="dropzone" (click)="fileInput.click()"> + <div class="text-wrapper"> + <div class="label"> + Drop your file here!<br> + <small class="hint">*only json and text files are allowed to upload.</small> + </div> + </div> + </a> + + <ul> + <li *ngIf="fileData">{{fileData}}</li> + </ul> + + <mat-form-field class="json-txt-editor" appearance="outline"> + <textarea matInput [value]="inputTxt" required readonly></textarea> Review Comment: if you're gonna do this, you should add a label ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.ts: ########## @@ -0,0 +1,159 @@ +/* +* 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 { DatePipe } from "@angular/common"; +import { Component, HostListener, Inject } from "@angular/core"; +import { MAT_DIALOG_DATA, MatDialogRef } from "@angular/material/dialog"; +import { AlertLevel } from "trafficops-types"; + +import { AlertService } from "../alert/alert.service"; + +/** + * Contains the structure of the data that DecisionDialogComponent accepts. + */ +export interface ImportJsonTxtComponentModel { + title: string; +} + +/** + * Component for import of json or text + */ +@Component({ + selector: "tp-import-json-txt", + styleUrls: ["./import-json-txt.component.scss"], + templateUrl: "./import-json-txt.component.html", +}) +export class ImportJsonTxtComponent { + + /** + * Allowed import file types + */ + public allowedType: string[] = ["application/json", "text/plain"]; + + /** Text editor value */ + public inputTxt = ""; + + /** File data imported */ + public fileData = ""; + + /** Monitor whether any file is being drag over the dialog */ + public dragOn = false; + + /** + * Creates an instance of import json edit txt component. + * + * @param dialogRef Dialog manager + * @param alertService Alert service manager + * @param datePipe Default angular date pipe for formating date + */ + constructor( + @Inject(MAT_DIALOG_DATA) public readonly data: ImportJsonTxtComponentModel, + private readonly dialogRef: MatDialogRef<ImportJsonTxtComponent>, + private readonly alertService: AlertService, + private readonly datePipe: DatePipe) { } + + /** + * Emits the json value for import as profile data + */ + public submit(): void { + this.dialogRef.close(this.inputTxt); + } + + /** + * Hosts listener for drag over + * + * @param evt Drag events data + */ + @HostListener("dragover", ["$event"]) public onDragOver(evt: DragEvent): void { + evt.preventDefault(); + evt.stopPropagation(); + + this.dragOn = true; + } + + /** + * Hosts listener for drag leave + * + * @param evt Drag events data + */ + @HostListener("dragleave", ["$event"]) public onDragLeave(evt: DragEvent): void { + evt.preventDefault(); + evt.stopPropagation(); + + this.dragOn = false; + } + + /** + * Hosts listener for drop + * + * @param evt Drag events data + */ + @HostListener("drop", ["$event"]) public onDrop(evt: DragEvent): void { + evt.preventDefault(); + evt.stopPropagation(); + + this.dragOn = false; + + const file = evt.dataTransfer?.files[0]; + + // returns on when there is no file attachment is there + if (!file) { + return; + } + + this.docReader(file); + } + + /** + * Uploads file + * + * @param event Event object for upload file + */ + public uploadFile(event: Event): void { + const file = (event.target as HTMLInputElement).files?.[0]; + + // returns on when there is no file attachment is there + if (!file) { + return; + } + this.docReader(file); + } + + /** + * Docs reader + * + * @param file that is uploaded + */ + public docReader(file: File): void { + + /** + * Check whether expected file is being uploaded + * returns on file wrong file type is uploaded + */ + if (!this.allowedType.find(type => type === file.type)) { + this.alertService.newAlert({ level: AlertLevel.ERROR, text: "Only JSON or text file is allowed." }); + return; + } + + /** Format text with data from file data and formated date with date pipe */ + this.fileData = `${file.name} - ${file.size} bytes, last modified: ${this.datePipe.transform(file.lastModified, "MM-dd-yyyy")}`; + + const reader = new FileReader(); + + reader.onload = (event): void => { + this.inputTxt = event.target?.result as string; Review Comment: please add a comment explaining why you need to use `as` here. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.ts: ########## @@ -0,0 +1,159 @@ +/* +* 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 { DatePipe } from "@angular/common"; +import { Component, HostListener, Inject } from "@angular/core"; +import { MAT_DIALOG_DATA, MatDialogRef } from "@angular/material/dialog"; +import { AlertLevel } from "trafficops-types"; + +import { AlertService } from "../alert/alert.service"; + +/** + * Contains the structure of the data that DecisionDialogComponent accepts. + */ +export interface ImportJsonTxtComponentModel { + title: string; +} + +/** + * Component for import of json or text + */ +@Component({ + selector: "tp-import-json-txt", + styleUrls: ["./import-json-txt.component.scss"], + templateUrl: "./import-json-txt.component.html", +}) +export class ImportJsonTxtComponent { + + /** + * Allowed import file types + */ + public allowedType: string[] = ["application/json", "text/plain"]; + + /** Text editor value */ + public inputTxt = ""; + + /** File data imported */ + public fileData = ""; + + /** Monitor whether any file is being drag over the dialog */ + public dragOn = false; + + /** + * Creates an instance of import json edit txt component. + * + * @param dialogRef Dialog manager + * @param alertService Alert service manager + * @param datePipe Default angular date pipe for formating date + */ + constructor( + @Inject(MAT_DIALOG_DATA) public readonly data: ImportJsonTxtComponentModel, + private readonly dialogRef: MatDialogRef<ImportJsonTxtComponent>, + private readonly alertService: AlertService, + private readonly datePipe: DatePipe) { } + + /** + * Emits the json value for import as profile data + */ + public submit(): void { + this.dialogRef.close(this.inputTxt); + } + + /** + * Hosts listener for drag over + * + * @param evt Drag events data + */ + @HostListener("dragover", ["$event"]) public onDragOver(evt: DragEvent): void { + evt.preventDefault(); + evt.stopPropagation(); + + this.dragOn = true; + } + + /** + * Hosts listener for drag leave + * + * @param evt Drag events data + */ + @HostListener("dragleave", ["$event"]) public onDragLeave(evt: DragEvent): void { + evt.preventDefault(); + evt.stopPropagation(); + + this.dragOn = false; + } + + /** + * Hosts listener for drop + * + * @param evt Drag events data + */ + @HostListener("drop", ["$event"]) public onDrop(evt: DragEvent): void { + evt.preventDefault(); + evt.stopPropagation(); + + this.dragOn = false; + + const file = evt.dataTransfer?.files[0]; + + // returns on when there is no file attachment is there + if (!file) { + return; + } + + this.docReader(file); + } + + /** + * Uploads file + * + * @param event Event object for upload file + */ + public uploadFile(event: Event): void { + const file = (event.target as HTMLInputElement).files?.[0]; + + // returns on when there is no file attachment is there + if (!file) { + return; + } + this.docReader(file); + } + + /** + * Docs reader + * + * @param file that is uploaded + */ + public docReader(file: File): void { + + /** + * Check whether expected file is being uploaded + * returns on file wrong file type is uploaded + */ + if (!this.allowedType.find(type => type === file.type)) { Review Comment: `this.allowedType.find(type => type === file.type)` seems like a longer way to type `this.allowedType.includes(file.type)`. ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -103,4 +103,24 @@ export class ProfileService extends APIService { return this.delete<ResponseProfile>(`profiles/${id}`).toPromise(); } + /** + * Exports profile + * + * @param profileId Id of the profile to export. + * @returns profile export object. + */ + public async exportProfile(profileId: number | ResponseProfile): Promise<ProfileExport>{ Review Comment: You should still be able to pass a `ResponseProfile`, it just needs to be covered by the tests. ########## 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}"> + <div class="dropzone" > + <div class="text-wrapper"> + <div class="label"> + Drop your file here!<br> + <small class="hint">*only json and text files are allowed to upload.</small> Review Comment: we absolutely must support users that require accessibility tools. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.html: ########## @@ -0,0 +1,38 @@ +<!-- +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>{{data.title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> + <input type='file' id="imageUpload" accept=".json, .txt" #fileInput (change)="uploadFile($event)" hidden/> + <a type="button" class="dropzone" (click)="fileInput.click()"> Review Comment: The `type` attribute of an anchor tag is used for indicating the MIME type of the linked resource. Because anchors are supposed to be links. "button" is not a MIME type, and this anchor is not a link, so it should instead be a button. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.html: ########## @@ -0,0 +1,38 @@ +<!-- +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>{{data.title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> + <input type='file' id="imageUpload" accept=".json, .txt" #fileInput (change)="uploadFile($event)" hidden/> Review Comment: this should not be hidden, as that prevents its use. Also missing any label that could be used to control it instead. ########## experimental/traffic-portal/src/app/api/testing/profile.service.ts: ########## @@ -220,6 +230,34 @@ export class ProfileService { return this.profiles.splice(index, 1)[0]; } + /** + * Export Profile object from the API. + * + * @param id Specify unique identifier (number) of a specific Profile to retrieve the export object. + * @returns The requested Profile as attachment. + */ + public async exportProfile(id?: number): Promise<ProfileExport> { Review Comment: this call signature doesn't match the "concrete" service's call signature. It is _assignable_ to that call signature, but since it's impossible to call the real method without supplying an ID, there's no reason to support calling it with an undefined ID in the mock service. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.html: ########## @@ -0,0 +1,38 @@ +<!-- +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>{{data.title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> + <input type='file' id="imageUpload" accept=".json, .txt" #fileInput (change)="uploadFile($event)" hidden/> + <a type="button" class="dropzone" (click)="fileInput.click()"> + <div class="text-wrapper"> + <div class="label"> + Drop your file here!<br> + <small class="hint">*only json and text files are allowed to upload.</small> + </div> + </div> + </a> + + <ul> + <li *ngIf="fileData">{{fileData}}</li> + </ul> + + <mat-form-field class="json-txt-editor" appearance="outline"> + <textarea matInput [value]="inputTxt" required readonly></textarea> + </mat-form-field> +</mat-dialog-content> +<mat-dialog-actions align="end"> + <button mat-raised-button color="warn" type="button" mat-dialog-close>Cancel</button> + <button mat-raised-button type="button" [mat-dialog-close]="true" (click)="submit()" [disabled]="!fileData">Submit</button> Review Comment: not using a `form` for this makes it a bit less accessible. The submit button doesn't submit anything as far as any screen reader can tell, and it won't be able to build an association between the input fields and the button that submits them. ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.ts: ########## @@ -0,0 +1,159 @@ +/* +* 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 { DatePipe } from "@angular/common"; +import { Component, HostListener, Inject } from "@angular/core"; +import { MAT_DIALOG_DATA, MatDialogRef } from "@angular/material/dialog"; +import { AlertLevel } from "trafficops-types"; + +import { AlertService } from "../alert/alert.service"; + +/** + * Contains the structure of the data that DecisionDialogComponent accepts. Review Comment: Copypasta ########## experimental/traffic-portal/src/app/api/profile.service.ts: ########## @@ -14,7 +14,7 @@ import { HttpClient } from "@angular/common/http"; import { Injectable } from "@angular/core"; -import {RequestParameter, RequestProfile, ResponseParameter, ResponseProfile} from "trafficops-types"; +import { ProfileExport, ProfileImport, ProfileImportResponse, RequestProfile, ResponseProfile, RequestParameter, ResponseParameter } from "trafficops-types"; Review Comment: linting error ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.scss: ########## @@ -0,0 +1,56 @@ +/* +* 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. +*/ + +$backgroundColor: #fafaec; + +.dropzone { + min-height: 75px; + min-width: 400px; + margin: 1rem auto; + display: table; + width: 99%; + background-color: $backgroundColor; + border: solid 1px rgb(231, 230, 230); + text-align: center; + color: rgb(133, 132, 132); + border-image: initial; + border-radius: 5px; + box-shadow: 0 .125rem .25rem rgba(0, 0, 0, .075); + cursor: pointer; + + .text-wrapper { + display: table-cell; + vertical-align: middle; + + .hint { + color: rgb(251, 192, 82); + } + } +} + +.mdc-dialog__content{ + &.active { + background: $backgroundColor; + } + +} + +.mat-mdc-form-field.json-txt-editor { Review Comment: class `json-txt-editor` should be reworded; it doesn't edit anything ########## experimental/traffic-portal/src/app/shared/import-json-txt/import-json-txt.component.html: ########## @@ -0,0 +1,38 @@ +<!-- +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>{{data.title}}</h2> +<mat-dialog-content [ngClass]="{'active':dragOn}"> + <input type='file' id="imageUpload" accept=".json, .txt" #fileInput (change)="uploadFile($event)" hidden/> + <a type="button" class="dropzone" (click)="fileInput.click()"> + <div class="text-wrapper"> + <div class="label"> + Drop your file here!<br> + <small class="hint">*only json and text files are allowed to upload.</small> Review Comment: grammar: > <del>o</del><ins>O</ins>nly <del>json</del><ins>JSON</ins> and text files are allowed<del> to upload</del>. -- 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]
