ocket8888 commented on code in PR #7213:
URL: https://github.com/apache/trafficcontrol/pull/7213#discussion_r1029909191
##########
docs/source/api/v3/server_capabilities.rst:
##########
@@ -70,6 +71,7 @@ Response Structure
"response": [
{
"name": "RAM",
+ "description": "ram server capability",
Review Comment:
We can't add properties to released API versions; i.e. 3.0, 3.1, and 4.0
##########
lib/go-tc/server_capabilities.go:
##########
@@ -28,6 +28,7 @@ type ServerCapabilitiesResponse struct {
// ServerCapability contains information about a given ServerCapability in
Traffic Ops.
type ServerCapability struct {
Name string `json:"name" db:"name"`
+ Description string `json:"description" db:"description"`
Review Comment:
This change should result in a new Server Capability structure instead of
modifying the old one. Using this structure in old API versions will result in
them having a `description` property, which they can't have.
##########
traffic_portal/app/src/common/modules/form/serverCapability/form.serverCapability.tpl.html:
##########
@@ -41,6 +41,13 @@
<span ng-show="hasError(serverCapabilityForm.name)"
class="form-control-feedback"><i class="fa fa-times"></i></span>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error':
hasError(serverCapabilityForm.description), 'has-feedback':
hasError(serverCapabilityForm.description)}">
Review Comment:
I don't think it's possible for this field to have errors or feedback
##########
traffic_ops/testing/api/v3/tc-fixtures.json:
##########
@@ -4822,22 +4822,28 @@
],
"serverCapabilities": [
{
- "name": "foo"
+ "name": "foo",
+ "description": "server capability foo"
Review Comment:
This won't fail tests, so I feel like I should call it out so it's not
accidentally overlooked - extraneous fields shouldn't be present in the testing
data.
##########
traffic_ops/app/db/migrations/2022112116304800_add_description_in_sc_table.up.sql:
##########
@@ -0,0 +1,18 @@
+/*
+ * 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.
+ */
+
+ALTER TABLE public.server_capability ADD COLUMN IF NOT EXISTS description text
DEFAULT NULL;
Review Comment:
The structure that models this uses `string` which cannot store a `NULL`
driver value. If a server capability exists with a null description, attempting
to read it into a pointer to a `string` will fail and return an error. Instead,
this should actually be `NOT NULL` and have a default value of `''`.
##########
docs/source/api/v4/server_capabilities.rst:
##########
@@ -50,6 +50,7 @@ Request Structure
Response Structure
------------------
:name: The name of this :term:`Server Capability`
+:description: The description of this :term:`Server Capability`
Review Comment:
This should have a `..versionadded::` directive specifying that this was
added in 4.1 - because it can't go into 4.0
##########
traffic_portal/app/src/common/modules/form/serverCapability/form.serverCapability.tpl.html:
##########
@@ -41,6 +41,13 @@
<span ng-show="hasError(serverCapabilityForm.name)"
class="form-control-feedback"><i class="fa fa-times"></i></span>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error':
hasError(serverCapabilityForm.description), 'has-feedback':
hasError(serverCapabilityForm.description)}">
+ <label class="control-label col-md-2 col-sm-2 col-xs-12"
>Description *</label>
Review Comment:
labels should have a `for` attribute that links them to the thing they label
##########
traffic_portal/app/src/common/modules/form/serverCapability/form.serverCapability.tpl.html:
##########
@@ -41,6 +41,13 @@
<span ng-show="hasError(serverCapabilityForm.name)"
class="form-control-feedback"><i class="fa fa-times"></i></span>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error':
hasError(serverCapabilityForm.description), 'has-feedback':
hasError(serverCapabilityForm.description)}">
+ <label class="control-label col-md-2 col-sm-2 col-xs-12"
>Description *</label>
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <textarea id="description" name="description" rows="3"
cols="17" class="form-control" ng-model="serverCapability.description"
maxlength="256" autofocus></textarea>
+ <span ng-show="hasError(serverCapabilityForm.description)"
class="form-control-feedback"><i class="fa fa-times"></i></span>
Review Comment:
I don't think it's possible for the textarea to have an error. Its only
constraint is maxlength, and that means that user agents won't allow the user
to type more than that, so it'll never violate that.
##########
traffic_portal/app/src/common/modules/table/serverCapabilities/table.serverCapabilities.tpl.html:
##########
@@ -34,11 +34,13 @@
<thead>
<tr>
<th>Name</th>
+ <th>Description</th>
</tr>
</thead>
<tbody>
<tr ng-click="editServerCapability(sc.name)" ng-repeat="sc in
::serverCapabilities" context-menu="contextMenuItems">
<td name="name"
data-search="^{{::sc.name}}$">{{::sc.name}}</td>
+ <td name="name"
data-search="^{{::sc.description}}$">{{::sc.description}}</td>
Review Comment:
this is a small enough change that you probably don't need to bother with
it, but for what it's worth we should convert that table to use the common grid
controller at some point.
##########
traffic_portal/app/src/common/modules/form/serverCapability/form.serverCapability.tpl.html:
##########
@@ -41,6 +41,13 @@
<span ng-show="hasError(serverCapabilityForm.name)"
class="form-control-feedback"><i class="fa fa-times"></i></span>
</div>
</div>
+ <div class="form-group" ng-class="{'has-error':
hasError(serverCapabilityForm.description), 'has-feedback':
hasError(serverCapabilityForm.description)}">
+ <label class="control-label col-md-2 col-sm-2 col-xs-12"
>Description *</label>
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <textarea id="description" name="description" rows="3"
cols="17" class="form-control" ng-model="serverCapability.description"
maxlength="256" autofocus></textarea>
Review Comment:
This should not have the `autofocus` attribute; the name input already does
and appears before it in the form.
##########
traffic_portal/test/integration/Data/servercapabilities.ts:
##########
@@ -47,67 +48,75 @@ export const serverCapabilities = {
],
check: [
{
- description: "check CSV link from
Server Capabilities page",
+ caseDescription: "check CSV link from
Server Capabilities page",
Name: "Export as CSV"
}
],
add: [
{
- description: "can create a server
capability",
+ caseDescription: "can create a server
capability",
name: "TP_SC",
+ description: "Server Capability",
Review Comment:
These descriptions aren't being used in the spec. The add cases should be
setting the descriptions from these values.
--
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]
