CLOSED - TP: Adds Server Capabilities UI #3977

mitchell852 · 2019-10-10T15:17:15Z

What does this PR (Pull Request) do?

DO NOT MERGE UNTIL #3966 is merged

This PR enhances TP to support the creation, viewing and deletion of server capabilities. See blueprint for a better explanation of server capabilities.

It also includes UI tests to view, create and delete a server capability
It also includes some documentation under the quick how to section. Note: the docs are a little light at the moment but will fill in as the other feature are complete (assigning a server capability to a server and delivery service).

This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

Traffic Portal

What is the best way to verify this PR?

Launch TP
Navigate to Configure > Server Capabilities
Create, view and delete a server capability

The following criteria are ALL met by this PR

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR ensures that database migration sequence is correct OR this PR does not include a database migration
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

asf-ci · 2019-10-10T16:04:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4441/

mhoppa

Pulled and tested this locally it works as expected. Can view/remove/create server capabilities and it correctly handles errors. On the code side havent been in the TP code much so I'll defer to others but I did not personally see any issues with it.

lbathina · 2019-10-10T22:16:26Z

leading and trailing spaces are ignore and doesn't throw errors.

lbathina · 2019-10-10T22:19:00Z

I guess we do not have requirement on what the values in Server capabilities can be. However, the UI indicates that Must be alphamumeric with no spaces. Dash, dot and underscore also allowed. given that, the system accepts when only numbers are entered.
Also there should be a check on only special characters - system should not accept only special characters.

rob05c · 2019-10-10T22:23:36Z

I'd like to remove allowing dot, if no one objects. While it's valid in most parts of a URL, if we limit to alpha/digit/_/-, it can be exactly the set of characters in Base64 encoding (RFC 4648). Which seems like it might come in handy. And adding allowed things later is easier than removing them.

mitchell852 · 2019-10-11T15:21:16Z

leading and trailing spaces are ignore and doesn't throw errors.

i think that's fine. the api strips them out.

mitchell852 · 2019-10-11T15:24:25Z

I'd like to remove allowing dot, if no one objects.

I'm good with that. Is this a good UI error message?

Must be alphanumeric with no spaces. Dash and underscore are also allowed.

based on that. This is fine:

-------
-_-_-_
222222

not really sure anyone would do that....but they could...

mitchell852 · 2019-10-11T15:26:13Z

Also there should be a check on only special characters - system should not accept only special characters.

define special

asf-ci · 2019-10-11T18:07:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4461/

asf-ci · 2019-10-11T19:22:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4463/

lbathina · 2019-10-11T20:15:42Z

Also there should be a check on only special characters - system should not accept only special characters.

define special

for example following names for server capabilities doesn't make sense.
-------
-_-_-_

lbathina · 2019-10-11T20:18:32Z

I'd like to remove allowing dot, if no one objects.

I'm good with that. Is this a good UI error message?

Must be alphanumeric with no spaces. Dash and underscore are also allowed.
Name may contain alphabets, numbers, special character Dash and underscore.
I guess Alphanumberic is different than saying can contain numbers, alphabets.

based on that. This is fine:

-------
-_-_-_
222222

not really sure anyone would do that....but they could...

asf-ci · 2019-10-11T22:00:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4466/

asf-ci · 2019-10-12T00:19:17Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4471/

asf-ci · 2019-10-12T00:31:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4472/

ocket8888 · 2019-10-14T23:34:37Z

traffic_portal/app/src/common/api/ServerCapabilityService.js

+	};
+
+	this.getServerCapability = function(name) {
+		return $http.get(ENV.api['root'] + 'server_capabilities?name=' + name).then(


instead of string concatenation, use the params key of the optional args object, like you do above in getServerCapabilty. This ensures proper URI Encoding.

return $http.get(ENV.api['root'] + 'server_capabilities', {params: {"name": name}}).then(r=>r.data.response[0],e=>{throw e;})

ocket8888 · 2019-10-14T23:35:35Z

traffic_portal/app/src/common/api/ServerCapabilityService.js

+	};
+
+	this.deleteServerCapability = function(name) {
+		return $http.delete(ENV.api['root'] + 'server_capabilities?name=' + name).then(


same as above RE: proper query param usage

ocket8888 · 2019-10-14T23:36:36Z

traffic_portal/app/src/common/modules/form/serverCapability/form.serverCapability.tpl.html

+    <div class="x_title">
+        <ol class="breadcrumb pull-left">
+            <li><a ng-click="navigateToPath('/server-capabilities')">Server Capabilities</a></li>
+            <li class="active">{{serverCapabilityName}}</li>


:: for one-time binding?

true. i forgot you can't change the name on a server cap

ocket8888 · 2019-10-14T23:43:25Z

...rtal/app/src/common/modules/form/serverCapability/view/FormViewServerCapabilityController.js

+			});
+	};
+
+	$scope.serverCapabilityName = angular.copy(serverCapability.name);


I don't think you need to use angular.copy here.

yep i think you are right because you can't change a server cap name even if you wanted to

ocket8888 · 2019-10-14T23:44:27Z

...rtal/app/src/common/modules/form/serverCapability/view/FormViewServerCapabilityController.js

+		modalInstance.result.then(function() {
+			deleteServerCapability(serverCapability);
+		}, function () {
+			// do nothing


Please don't do nothing; at least log the error

it's not an error but i can log something like "action cancelled". actually maybe i just kill this handler.

ocket8888 · 2019-10-14T23:45:44Z

..._portal/app/src/common/modules/table/serverCapabilities/TableServerCapabilitiesController.js

+		modalInstance.result.then(function() {
+			deleteServerCapability(serverCapability);
+		}, function () {
+			// do nothing


Same as above, RE doing nothing.

ocket8888 · 2019-10-14T23:48:27Z

...fic_portal/app/src/common/modules/table/serverCapabilities/table.serverCapabilities.tpl.html

+        </ol>
+        <div class="pull-right">
+            <button name="createServerCapabilityButton" class="btn btn-primary" title="Create Server Capability" ng-click="createServerCapability()"><i class="fa fa-plus"></i></button>
+            <button class="btn btn-default" title="Refresh" ng-click="refresh()"><i class="fa fa-refresh"></i></button>


The types of these buttons are both submit, and I don't think they ought to be

ocket8888 · 2019-10-14T23:49:27Z

...fic_portal/app/src/common/modules/table/serverCapabilities/table.serverCapabilities.tpl.html

+        <br>
+        <table id="serverCapabilitiesTable" class="table responsive-utilities jambo_table">
+            <thead>
+            <tr class="headings">


what's this class do? couldn't you just use the selector: thead tr?

this is a jquery datatables thing. i'll see if it's not needed.

i took it out. seems to have not changed anything.

ocket8888 · 2019-10-14T23:54:11Z

docs/source/admin/quick_howto/server_capabililty.rst

+**************************
+Manage Server Capabilities
+**************************
+Server capabilities are designed to enable system operators to control the flow of delivery service traffic through caches (Edges or Mids) with ONLY the required capabilities. For example, delivery services that serve large binary files should only be routed to caches with sufficient disk cache. Currently, this can be controlled at the Edge tier where system operators can explicitly assign only Edge caches with sufficient disk cache to the delivery service. However, the system operators do not have control of the Mid tier and cannot dictate which Mid caches are qualified to serve these large binary files. This will cause a problem if a Mid cache with insufficient disk cache is asked to serve the large binary files.


You've got a bunch of unlinked terms in here: "delivery service", "cache" (should be "cache server" in many cases), maybe a couple of others.
Verbal stress on a word in a sentence should be indicated with emphasis roles. Typically you'd do that with asterisks:

*this text is emphasized*

What's an "operator"? I think you mean

... users with the "operator" :term:`Role` ("operators") ...

ocket8888 · 2019-10-14T23:58:43Z

traffic_portal/test/end_to_end/serverCapabilities/pageData.js

+	this.name=element(by.name('name'));
+	this.createButton=element(by.buttonText('Create'));
+	this.deleteButton=element(by.buttonText('Delete'));
+	this.searchFilter=element(by.id('serverCapabilitiesTable_filter')).element(by.css('label')).element(by.css('input'));


can't you combine this into one by.css? by.css('label input')?

maybe but they seem fine separate to me. i'm sure all of this UI test framework could be cleaned up.

asf-ci · 2019-10-15T15:48:44Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4515/

mitchell852 · 2019-10-15T16:10:27Z

I got into rebase hell so I'm closing this PR and opened #3993

Rawlin Peters added 8 commits October 4, 2019 13:18

Add server_capability goose migration

46eb2b5

Add server_capabilities API endpoints

23ff788

Add server capabilities methods to TO-Go client

845b9d9

Add TO API tests for server capabilities

c46a7f0

Add CHANGELOG entry for server_capabilities

021fa66

Fix godoc comments on server_capabilities structs

bfcf82f

Addressed review comments

83862fc

Address PR review comments

e6abc19

mitchell852 added Traffic Portal related to Traffic Portal new feature A new feature, capability or behavior labels Oct 10, 2019

mhoppa approved these changes Oct 10, 2019

View reviewed changes

Rawlin Peters added 2 commits October 11, 2019 10:17

Address more PR feedback

efcaab2

Fix godoc comment before someone yells at me

13625ca

mitchell852 marked this pull request as ready for review October 11, 2019 19:28

mitchell852 added 6 commits October 11, 2019 13:34

adds view for managing server capabilities

a3ad379

fixes case differences with ui test directories

0bacd5c

adds ui tests for create/view/delete server capabilities

7ae0634

prevents dots in server capability name

9ff4b4f

adds changelog entry

f38ad44

adds documentation for create, view and delete server capabilities

e8bbac3

mitchell852 force-pushed the server-cap-ui branch from 6468a23 to e8bbac3 Compare October 11, 2019 19:40

ocket8888 requested changes Oct 15, 2019

View reviewed changes

mitchell852 added 5 commits October 15, 2019 09:37

server capability name is immutable

d2c6ac2

adds glossary terms to server capability docs

97a2072

proper use of query params

3898b36

sets button types and removes uneccessary css class

3d88df6

removes handlers for dialog cancel events

db6fe46

mitchell852 closed this Oct 15, 2019

mitchell852 changed the title ~~TP: Adds Server Capabilities UI~~ CLOSED - TP: Adds Server Capabilities UI Oct 24, 2019

mitchell852 added the abandoned label Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOSED - TP: Adds Server Capabilities UI #3977

CLOSED - TP: Adds Server Capabilities UI #3977

mitchell852 commented Oct 10, 2019 •

edited

asf-ci commented Oct 10, 2019

mhoppa left a comment

lbathina commented Oct 10, 2019

lbathina commented Oct 10, 2019 •

edited

rob05c commented Oct 10, 2019

mitchell852 commented Oct 11, 2019

mitchell852 commented Oct 11, 2019 •

edited

mitchell852 commented Oct 11, 2019

asf-ci commented Oct 11, 2019

asf-ci commented Oct 11, 2019

lbathina commented Oct 11, 2019 •

edited

lbathina commented Oct 11, 2019

asf-ci commented Oct 11, 2019

asf-ci commented Oct 12, 2019

asf-ci commented Oct 12, 2019

ocket8888 Oct 14, 2019

ocket8888 Oct 14, 2019

ocket8888 Oct 14, 2019

mitchell852 Oct 15, 2019

ocket8888 Oct 14, 2019

mitchell852 Oct 15, 2019

ocket8888 Oct 14, 2019

mitchell852 Oct 15, 2019

ocket8888 Oct 14, 2019

ocket8888 Oct 14, 2019

ocket8888 Oct 14, 2019

mitchell852 Oct 15, 2019

mitchell852 Oct 15, 2019

ocket8888 Oct 14, 2019

ocket8888 Oct 14, 2019

mitchell852 Oct 15, 2019

asf-ci commented Oct 15, 2019

mitchell852 commented Oct 15, 2019

CLOSED - TP: Adds Server Capabilities UI #3977

CLOSED - TP: Adds Server Capabilities UI #3977

Conversation

mitchell852 commented Oct 10, 2019 • edited

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

The following criteria are ALL met by this PR

asf-ci commented Oct 10, 2019

mhoppa left a comment

Choose a reason for hiding this comment

lbathina commented Oct 10, 2019

lbathina commented Oct 10, 2019 • edited

rob05c commented Oct 10, 2019

mitchell852 commented Oct 11, 2019

mitchell852 commented Oct 11, 2019 • edited

mitchell852 commented Oct 11, 2019

asf-ci commented Oct 11, 2019

asf-ci commented Oct 11, 2019

lbathina commented Oct 11, 2019 • edited

lbathina commented Oct 11, 2019

asf-ci commented Oct 11, 2019

asf-ci commented Oct 12, 2019

asf-ci commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asf-ci commented Oct 15, 2019

mitchell852 commented Oct 15, 2019

mitchell852 commented Oct 10, 2019 •

edited

lbathina commented Oct 10, 2019 •

edited

mitchell852 commented Oct 11, 2019 •

edited

lbathina commented Oct 11, 2019 •

edited