You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/07/15 13:18:41 UTC
[GitHub] [trafficcontrol] shamrickus opened a new pull request, #6967: TPv2 Add Change Logs Table
shamrickus opened a new pull request, #6967:
URL: https://github.com/apache/trafficcontrol/pull/6967
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR adds a change-logs table to TPv2. It adds functionality to the generic table to allow for adding more header buttons. It also enables the right click context menu on the users table to view change-logs for a given user.
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Portal v2
## What is the best way to verify this PR?
Verify the table works, can see a users change logs (currently just does this with a search param, should use param to filter the column but that is not implemented in the tpv2 version) and that the tests pass.
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you 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.
-->
--
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
[GitHub] [trafficcontrol] ocket8888 merged pull request #6967: TPv2 Add Change Logs Table
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #6967:
URL: https://github.com/apache/trafficcontrol/pull/6967
--
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
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6967: TPv2 Add Change Logs Table
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6967:
URL: https://github.com/apache/trafficcontrol/pull/6967#discussion_r926092748
##########
experimental/traffic-portal/src/app/core/users/users.component.ts:
##########
@@ -229,8 +229,8 @@ export class UsersComponent implements OnInit {
newTab: true
},
{
- action: "viewChangelogs",
- disabled: (): true =>true,
+ disabled: (us: GetResponseUser | Array<GetResponseUser>): boolean => Array.isArray(us),
Review Comment:
if `multiRow` isn't `true`, the action is implicitly disabled when multiple rows are selected, so you shouldn't need to define this. If you do, that's a bug in the generic table component.
##########
experimental/traffic-portal/src/app/utils/time.ts:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.
+ */
+
+/**
+ * Takes the difference between two times (in milliseconds) and returns a formatted string with relative time
+ *
+ * @param delta time delta in milliseconds
+ * @returns Formatted string in the form of 'N X Ago' where X is anything between Seconds and Years
+ */
+export function relativeTimeString(delta: number): string {
+ const SEC = 1000;
+ const MIN = SEC * 60;
+ const HOUR = MIN * 60;
+ const DAY = HOUR * 24;
+ const YEAR = 365 * DAY;
+ const MONTH = YEAR / 12;
+ const WEEK = MONTH / 4;
Review Comment:
This probably doesn't really have any performance impact at all, but I'd define these outside the function so they aren't recalculated every time it's called. Do it or don't, just wanted to point it out.
##########
experimental/traffic-portal/src/app/api/testing/index.ts:
##########
@@ -17,7 +17,7 @@ import { NgModule } from "@angular/core";
import {
CacheGroupService,
- CDNService,
+ CDNService, ChangeLogsService,
Review Comment:
nit: this should be on the next line
--
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
[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #6967: TPv2 Add Change Logs Table
Posted by GitBox <gi...@apache.org>.
shamrickus commented on code in PR #6967:
URL: https://github.com/apache/trafficcontrol/pull/6967#discussion_r924901427
##########
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:
Because the version we were using didn't have it.
--
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
[GitHub] [trafficcontrol] ocket8888 commented on pull request #6967: TPv2 Add Change Logs Table
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on PR #6967:
URL: https://github.com/apache/trafficcontrol/pull/6967#issuecomment-1192844229
When I click on "View user changelogs" from the context menu on a user, I see changelogs for all users, not just that user
--
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
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6967: TPv2 Add Change Logs Table
Posted by 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
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6967: TPv2 Add Change Logs Table
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6967:
URL: https://github.com/apache/trafficcontrol/pull/6967#discussion_r925100006
##########
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:
It does, you probably just didn't realize it. It's called `Log`, not `ChangeLog`, named after the API endpoint - which is `/logs` not e.g. `/change_logs`
--
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