[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6339: Cert expirations tp

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

ocket8888 commented on a change in pull request #6339:
URL: https://github.com/apache/trafficcontrol/pull/6339#discussion_r744000153



##########
File path: traffic_portal/app/src/common/modules/table/certExpirations/TableCertExpirationsController.js
##########
@@ -0,0 +1,317 @@
+/*
+ * 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.
+ */
+
+var TableCertExpirationsController = function(tableName, certExpirations, filter, $document, $scope, $state, $filter, locationUtils, certExpirationsService) {
+
+    let table;

Review comment:
       indentation is inconsistent in this file, some lines uses spaces and some lines use the One True indentation. There's no mandate on which to use throughout TP, but you should pick one to stick with for the file to prevent unintentionally massive diffs if the file is ever changed again by someone who's editor consolidates the indentation to a single style (fwiw tabs are mandated in TPv2, so it's a solid choice IMO).

##########
File path: traffic_portal/app/src/common/api/index.js
##########
@@ -24,7 +24,8 @@ module.exports = angular.module('trafficPortal.api', [])
     .service('cacheStatsService', require('./CacheStatsService'))
     .service('capabilityService', require('./CapabilityService'))
     .service('cdnService', require('./CDNService'))
-    .service('changeLogService', require('./ChangeLogService'))
+	.service('certExpirationsService', require('./CertExpirationsService'))
+	.service('changeLogService', require('./ChangeLogService'))

Review comment:
       same as above RE: indentation

##########
File path: traffic_portal/app/src/app.js
##########
@@ -372,6 +374,7 @@ var trafficPortal = angular.module('trafficPortal', [
         require('./common/modules/table/cdnNotifications').name,
         require('./common/modules/table/cdnProfiles').name,
         require('./common/modules/table/cdnServers').name,
+		require('./common/modules/table/certExpirations').name,

Review comment:
       the rest of the file (unfortunately) uses spaces for indentation, as do your other two added lines. If you wanna change the indentation of the file to use actual indentation, I'd support that fully. As-is, though, this should conform.

##########
File path: traffic_portal/app/src/common/modules/table/certExpirations/TableCertExpirationsController.js
##########
@@ -0,0 +1,317 @@
+/*
+ * 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.
+ */
+
+var TableCertExpirationsController = function(tableName, certExpirations, filter, $document, $scope, $state, $filter, locationUtils, certExpirationsService) {
+
+    let table;
+
+	$scope.certExpirations = certExpirations;
+	$scope.days;
+
+	$scope.editCertExpirations = function(dsId) {
+		locationUtils.navigateToPath('/delivery-services/' + dsId + '/ssl-keys');
+	}
+
+	/**
+	 * Formats the contents of a 'expiration' column cell as just the date.
+	 */
+	function dateCellFormatter(params) {
+		return params.value ? $filter('date')(params.value, 'MM/dd/yyyy') : params.value;
+	}
+
+	/**
+	 * Formats the contents of a 'expiration' column cell as just the date.
+	 */
+	function federatedCellFormatter(params) {
+		if (!params.value) {
+			return '';
+		} else {
+			return params.value;
+		}
+	}
+
+    $scope.refresh = function() {
+        $state.reload(); // reloads all the resolves for the view
+    };
+
+	/** Toggles the visibility of a column that has the ID provided as 'col'. */
+	$scope.toggleVisibility = function(col) {
+		const visible = $scope.gridOptions.columnApi.getColumn(col).isVisible();
+		$scope.gridOptions.columnApi.setColumnVisible(col, !visible);
+	};
+
+	/** The columns of the ag-grid table */
+	const columns = [
+		{
+			headerName: "Delivery Service",
+			field: "deliveryservice",
+			hide: false
+		},
+		{
+			headerName: "CDN",
+			field: "cdn",
+			hide: false
+		},
+		{
+			headerName: "Provider",
+			field: "provider",
+			hide: false
+		},
+		{
+			headerName: "Expiration",
+			field: "expiration",
+			hide: false,
+			valueFormatter: dateCellFormatter
+		},
+		{
+			headerName: "Federated",
+			field: "federated",
+			hide: false,
+			valueFormatter: federatedCellFormatter
+		},
+	];
+
+	$scope.pageSize = 100;
+
+	/** Options, configuration, data and callbacks for the ag-grid table. */
+	$scope.gridOptions = {

Review comment:
       It looks like you might be doing more work here than you need to. Can you make use of the existing AG-Grid table component?

##########
File path: traffic_portal/test/integration/PageObjects/SideNavigationPage.po.ts
##########
@@ -36,6 +36,7 @@ export class SideNavigationPage extends BasePage{
     private lnkDeliveryServices = element(by.xpath("//a[@href='/#!/delivery-services']"));
     private lnkDeliveryServiceRequest = element(by.xpath("//a[@href='/#!/delivery-service-requests']"));
     private lnkServiceCategories = element(by.xpath("//a[@href='/#!/service-categories']"));
+    private lnkCertExpirations = element(by.xpath("//a[@href='/#!/cert-expirations']"));

Review comment:
       CSS selectors are much more familiar and easy to read than XPath, should use those instead




-- 
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