[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6967: TPv2 Add Change Logs Table

[GitBox <gi...@apache.org>]

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


##########
experimental/traffic-portal/src/app/core/change-logs/change-logs.component.ts:
##########
@@ -0,0 +1,194 @@
+/*
+* 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 { formatDate } from "@angular/common";
+import { Component, OnInit } from "@angular/core";
+import { MatDialog } from "@angular/material/dialog";
+import { ActivatedRoute } from "@angular/router";
+import { ColDef } from "ag-grid-community";
+import { BehaviorSubject } from "rxjs";
+
+import { ChangeLogsService } from "src/app/api/change-logs.service";
+import { LastDaysComponent } from "src/app/core/change-logs/last-days/last-days.component";
+import { ChangeLog } from "src/app/models/change-logs";
+import { TableTitleButton } from "src/app/shared/generic-table/generic-table.component";
+import { TpHeaderService } from "src/app/shared/tp-header/tp-header.service";
+
+/**
+ * AugmentedChangeLog has fields for access to processed times.
+ */
+interface AugmentedChangeLog extends ChangeLog {
+	longTime: string;
+	relativeTime: string;
+}
+
+/**
+ * Converts a changelog to an augmented one.
+ *
+ * @param cl Changelog to convert.
+ * @returns Converted changelog
+ */
+export function augment(cl: ChangeLog): AugmentedChangeLog {
+	const aug = {longTime: "", relativeTime: "", ...cl};
+
+	const d = new Date(aug.lastUpdated);
+	const delta = new Date().getTime() - d.getTime();
+	const SEC = 1000;
+	const MIN = SEC * 60;
+	const HOUR = MIN * 60;
+	const DAY = HOUR * 24;
+	const WEEK = DAY * 7;
+	const MONTH = WEEK * 4;
+	const YEAR = MONTH * 12;
+	if (delta > YEAR) {
+		aug.relativeTime = `${(delta / YEAR).toFixed(2)} years ago`;
+	} else if (delta > MONTH) {
+		aug.relativeTime =  `${(delta / MONTH).toFixed(2)} months ago`;
+	} else if (delta > WEEK) {
+		aug.relativeTime = `${(delta/ WEEK).toFixed(2)} weeks ago`;
+	} else if (delta > DAY) {
+		aug.relativeTime = `${(delta / DAY).toFixed(2)} days ago`;
+	} else if (delta > HOUR) {
+		aug.relativeTime = `${(delta / HOUR).toFixed(2)} hours ago`;
+	} else if (delta > MIN) {
+		aug.relativeTime = `${(delta / MIN).toFixed(2)} minutes ago`;
+	} else {
+		aug.relativeTime = `${(delta / SEC).toFixed(0)} seconds ago`;
+	}
+
+	aug.longTime = formatDate(aug.lastUpdated, "long", "en-US");
+	return aug;
+}

Review Comment:
   I think the <code><var>N</var> <var>time unit</var> ago</code> part should probably be pulled out into something under `utils/`, because other things (e.g. a Date cell renderer) will probably want to use it.



##########
experimental/traffic-portal/src/app/core/change-logs/change-logs.component.ts:
##########
@@ -0,0 +1,194 @@
+/*
+* 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 { formatDate } from "@angular/common";
+import { Component, OnInit } from "@angular/core";
+import { MatDialog } from "@angular/material/dialog";
+import { ActivatedRoute } from "@angular/router";
+import { ColDef } from "ag-grid-community";
+import { BehaviorSubject } from "rxjs";
+
+import { ChangeLogsService } from "src/app/api/change-logs.service";
+import { LastDaysComponent } from "src/app/core/change-logs/last-days/last-days.component";
+import { ChangeLog } from "src/app/models/change-logs";
+import { TableTitleButton } from "src/app/shared/generic-table/generic-table.component";
+import { TpHeaderService } from "src/app/shared/tp-header/tp-header.service";
+
+/**
+ * AugmentedChangeLog has fields for access to processed times.
+ */
+interface AugmentedChangeLog extends ChangeLog {
+	longTime: string;
+	relativeTime: string;
+}
+
+/**
+ * Converts a changelog to an augmented one.
+ *
+ * @param cl Changelog to convert.
+ * @returns Converted changelog
+ */
+export function augment(cl: ChangeLog): AugmentedChangeLog {
+	const aug = {longTime: "", relativeTime: "", ...cl};
+
+	const d = new Date(aug.lastUpdated);
+	const delta = new Date().getTime() - d.getTime();
+	const SEC = 1000;
+	const MIN = SEC * 60;
+	const HOUR = MIN * 60;
+	const DAY = HOUR * 24;
+	const WEEK = DAY * 7;
+	const MONTH = WEEK * 4;
+	const YEAR = MONTH * 12;
+	if (delta > YEAR) {
+		aug.relativeTime = `${(delta / YEAR).toFixed(2)} years ago`;
+	} else if (delta > MONTH) {
+		aug.relativeTime =  `${(delta / MONTH).toFixed(2)} months ago`;
+	} else if (delta > WEEK) {
+		aug.relativeTime = `${(delta/ WEEK).toFixed(2)} weeks ago`;
+	} else if (delta > DAY) {
+		aug.relativeTime = `${(delta / DAY).toFixed(2)} days ago`;
+	} else if (delta > HOUR) {
+		aug.relativeTime = `${(delta / HOUR).toFixed(2)} hours ago`;
+	} else if (delta > MIN) {
+		aug.relativeTime = `${(delta / MIN).toFixed(2)} minutes ago`;
+	} else {
+		aug.relativeTime = `${(delta / SEC).toFixed(0)} seconds ago`;
+	}
+
+	aug.longTime = formatDate(aug.lastUpdated, "long", "en-US");
+	return aug;
+}
+
+/**
+ *  ChangeLogsComponent is the controller for the change logs page
+ */
+@Component({
+	selector: "tp-change-logs",
+	styleUrls: ["./change-logs.component.scss"],
+	templateUrl: "./change-logs.component.html"
+})
+export class ChangeLogsComponent implements OnInit {
+	/** Emits changes to the fuzzy search text. */
+	public fuzzySubj: BehaviorSubject<string>;
+
+	/** The current search text. */
+	public searchText = "";
+
+	public changeLogs: Promise<Array<AugmentedChangeLog>> | null = null;
+
+	public lastDays = 7;
+
+	public titleBtns: Array<TableTitleButton> = [
+		{
+			action: "lastDays",
+			text: `Last ${this.lastDays} days`,
+		}
+	];
+
+	public columnDefs: Array<ColDef> = [
+		{
+			field: "relativeTime",
+			headerName: "Occurred",
+		},
+		{
+			field: "longTime",
+			headerName: "Created (UTC)",
+		},
+		{
+			field: "user",
+			headerName: "User"
+		},
+		{
+			field: "level",
+			headerName: "Level",
+			hide: true
+		},
+		{
+			field: "message",
+			headerName: "Message"
+		}
+	];
+
+	/** Whether user data is still loading. */
+	public loading = true;
+
+	constructor(private readonly headerSvc: TpHeaderService, private readonly api: ChangeLogsService,
+		private readonly route: ActivatedRoute, private readonly dialog: MatDialog) {
+		this.fuzzySubj = new BehaviorSubject<string>("");
+	}
+
+	/**
+	 * Loads the table data based on lastDays.
+	 */
+	public async loadData(): Promise<void> {
+		this.loading = true;
+		this.changeLogs = this.api.getChangeLogs({days: this.lastDays.toString()}).then(data => {

Review Comment:
   This method is `async`, so why not use `await` to avoid inline callbacks?



##########
experimental/traffic-portal/src/app/api/change-logs.service.ts:
##########
@@ -0,0 +1,40 @@
+/*
+* 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 { APIService } from "src/app/api/base-api.service";
+import { ChangeLog } from "src/app/models/change-logs";
+
+/**
+ * Defines & handles api endpoints related to change logs
+ */
+@Injectable()
+export class ChangeLogsService extends APIService {

Review Comment:
   This should have a corresponding class in `app/api/testing/` so that the `APITestingModule` provides the whole API. That would also make the spy you make in the change-logs.component.spec.ts file unnecessary.



##########
experimental/traffic-portal/src/app/core/change-logs/last-days/last-days.component.spec.ts:
##########
@@ -0,0 +1,43 @@
+/*
+* 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 { MAT_DIALOG_DATA, MatDialogRef } from "@angular/material/dialog";
+
+import { LastDaysComponent } from "./last-days.component";
+
+describe("LastDaysComponent", () => {
+	let component: LastDaysComponent;
+	let fixture: ComponentFixture<LastDaysComponent>;
+	let mockMatDialog: jasmine.SpyObj<MatDialogRef<number>>;

Review Comment:
   why bother making a spy you're not using? Did you forget to write a test that you had planned to use this?



##########
experimental/traffic-portal/src/app/api/change-logs.service.ts:
##########
@@ -0,0 +1,40 @@
+/*
+* 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 { APIService } from "src/app/api/base-api.service";
+import { ChangeLog } from "src/app/models/change-logs";
+
+/**
+ * Defines & handles api endpoints related to change logs
+ */
+@Injectable()
+export class ChangeLogsService extends APIService {
+
+	constructor(http: HttpClient) {
+		super(http);
+	}
+
+	/**
+	 * Calls api logs endpoint
+	 *
+	 * @param params Request paramteres to add
+	 * @returns Change logs
+	 */
+	public async getChangeLogs(params: Record<string, string>): Promise<Array<ChangeLog>> {
+		return this.get<[ChangeLog]>("logs", undefined, params).toPromise();

Review Comment:
   This is returning a tuple of one ChangeLog item, not an array. It's working because `[T]` is assignable to `T[]`, but it'd be more accurate if they were the same type.



##########
experimental/traffic-portal/src/app/models/change-logs.ts:
##########
@@ -0,0 +1,24 @@
+/*
+* 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.
+*/
+
+/**
+ * Represents the ChangeLog api object
+ */
+export interface ChangeLog {
+	id: number;
+	lastUpdated: Date;
+	level: string;
+	message: string;
+	ticketNum?: number | null;
+}

Review Comment:
   This definition is provided by the `trafficops-types` library; any reason you made your own?



-- 
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: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org