TP - delivery service form layout redesign by mitchell852 · Pull Request #3658 · apache/trafficcontrol

mitchell852 · 2019-06-04T21:16:42Z

What does this PR (Pull Request) do?

The delivery service forms for HTTP*, DNS*, Steering and any map delivery services have been reorganized into 3 groups - general config, cache config and routing config. Unlike before, none of the fields are hidden, however, each group can be collapsed if desired. This should help communicate the purpose of each delivery service attribute.

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?

Run the UI tests - https://github.com/apache/trafficcontrol/tree/master/traffic_portal/test/end_to_end

Note (tests): there are no new UI tests as functionality did not change. However, running the existing UI tests will ensure that the changes made to the ds forms do not prevent create, update or delete of a ds.

Note (docs): There are currently no docs that show what the ds forms looks like, therefore, none to update.

Note (changelog.md): I do not believe a changelog.md entry is required as functionality did not change in any way.

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)

asfgit · 2019-06-04T21:30:24Z

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

asfgit · 2019-06-05T18:17:26Z

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

asfgit · 2019-06-05T20:50:55Z

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

asfgit · 2019-06-05T21:50:56Z

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

asfgit · 2019-06-05T22:25:00Z

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

asfgit · 2019-06-07T16:50:41Z

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

asfgit · 2019-06-07T17:49:09Z

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

mitchell852 · 2019-06-07T17:50:46Z

retest this pleaase

asfgit · 2019-06-10T15:10:08Z

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

asfgit · 2019-06-10T16:15:05Z

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

ocket8888

Still need to check out the UI test updates.

By the way, as long as we're re-doing the DS forms, it'd be neat to use input[type="checkbox"] instead of select option[ng-value="true"], select option[ng-value="false"] for inputs that represent boolean concepts. Though that may be out of scope for this PR.

ocket8888 · 2019-06-10T13:59:41Z

traffic_portal/app/src/common/modules/form/deliveryService/FormDeliveryServiceController.js

Maybe this isn't the time to change this, but you can do this whole loop thing in one line:

const URLstring = ds.exampleURLs.join(' ');

which has the added benefit of not including a trailing  . Unless you want that, in which case you can append it with + on that same line. But that shouldn't be necessary, because paragraphs should force a line break between them anyway.

ocket8888 · 2019-06-10T15:34:53Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

Instead of two ngShow listeners on two separate  tags, you could accomplish this by toggling the state of exactly one chevron like so:



You may also want to consider the HTML5 <details> tag - although that isn't supported by Edge and I'm never really clear on which browsers TP is purported to support.

ocket8888 · 2019-06-10T15:39:14Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

this ng-show could be combined with the ng-if - since that's what the other buttons do

ocket8888 · 2019-06-10T15:39:41Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

This ng-show could be an ng-if

ocket8888 · 2019-06-10T15:48:59Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

This should probably no longer be autofocused now that it's not the first control encountered by the user.

ocket8888 · 2019-06-10T15:52:09Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

This URL no longer works - if you want to fix it it would be https://traffic-control-cdn.readthedocs.io/en/latest/overview/delivery_services.html#ds-types. Or you could just remove it entirely.

i'll fix it. i think it's probably helpful

ocket8888 · 2019-06-10T15:54:52Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

hrefs that use AngularJS string interpolation should instead use the ngHref directive.

i'm guessing there are a lot of places where this could be fixed...is this issue part of your TP best practices issue?

Not currently - you think it's common enough to be added?

i'm guessing it probably is

ocket8888 · 2019-06-10T16:12:16Z

...ic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html

The readthedocs page will redirect this to HTTPS automatically - but it'd be nice to let users skip that step by taking them to the HTTPS page from the start.

confused. is this not correct?

See <a href="https://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_router.html#tr-ngb" target="_blank">GeoLimit Failure Redirect</a> feature...

The issue is with http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_router.html#pattern-based-consistenthash - but because you updated your PR (several times) while I was reviewing, GitHub became very confused about where certain comments are anchored.

oh, i see. will fix

i'll look for all instances of http on the ds forms

ocket8888 · 2019-06-10T16:14:51Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.anyMap.tpl.html

A lot of these routing configuration changes don't get used by ANY_MAP Delivery Services - should they still be presented to the user? e.g. Geolocation Provider is used by TR to route client requests, but since TR doesn't route ANY_MAPs, the field has no effect on them.

good point. these fields have always been present on the any_map form and like you said, probably ignored.

can you create a separate issue for this as these fields have been present on the any map form for so long and i'd like to address their removal separate from this PR. good catch!

It's already tracked by #1943

oh wow. look at that. :)

ocket8888 · 2019-06-10T16:28:42Z

traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html

This link no longer exists. If you want to fix it, the new location is: https://traffic-control-cdn.readthedocs.io/en/latest/overview/delivery_services.html#ds-multi-site-origin. Or you could just get rid of it.

asfgit · 2019-06-10T17:20:15Z

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

asfgit · 2019-06-10T18:36:06Z

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

asfgit · 2019-06-10T18:54:58Z

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

ocket8888

UI tests pass, page looks good.

…straints

…ering)

asfgit · 2019-07-25T10:52:10Z

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

dangogh · 2019-07-25T15:03:21Z

I see @ocket8888 reviewed the hell out of this.. merging..

mitchell852 added Traffic Portal v1 related to Traffic Portal version 1 improvement The functionality exists but it could be improved in some way. labels Jun 4, 2019

mitchell852 force-pushed the ds-form-redesign branch from 4a15c2a to 35e102c Compare June 5, 2019 18:09

mitchell852 force-pushed the ds-form-redesign branch from 35e102c to d1f58eb Compare June 5, 2019 20:44

mitchell852 force-pushed the ds-form-redesign branch from d1f58eb to d5e1695 Compare June 5, 2019 21:44

mitchell852 marked this pull request as ready for review June 5, 2019 21:45

mitchell852 force-pushed the ds-form-redesign branch from d5e1695 to 2d867ba Compare June 5, 2019 22:20

mitchell852 force-pushed the ds-form-redesign branch from 6a3ab73 to 93ad7f6 Compare June 7, 2019 17:41

mitchell852 force-pushed the ds-form-redesign branch from 93ad7f6 to 6d35890 Compare June 10, 2019 15:04

mitchell852 force-pushed the ds-form-redesign branch from 6d35890 to 9050d01 Compare June 10, 2019 16:06

ocket8888 suggested changes Jun 10, 2019

View reviewed changes

mitchell852 force-pushed the ds-form-redesign branch from 9050d01 to 076da45 Compare June 10, 2019 17:12

ocket8888 approved these changes Jun 10, 2019

View reviewed changes

mitchell852 added 4 commits July 24, 2019 15:12

ds form layout redesign

1e11c1d

fixes comparison of new and old consistenthashquery params

d41a2aa

if ds request, remove action buttons from top of form due to size con…

35c1d67

…straints

adds UI tests to test all 4 ds categories (any map, dsn, http and ste…

4c919fb

…ering)

mitchell852 added 2 commits July 24, 2019 15:12

some general cleanup/simplification per PR review feedback

346f511

changes http to https for docs link

d257642

mitchell852 force-pushed the ds-form-redesign branch from 3baabd3 to d257642 Compare July 24, 2019 21:56

dangogh merged commit 384c237 into apache:master Jul 25, 2019

Conversation

mitchell852 commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

asfgit commented Jun 4, 2019

Uh oh!

asfgit commented Jun 5, 2019

Uh oh!

asfgit commented Jun 5, 2019

Uh oh!

asfgit commented Jun 5, 2019

Uh oh!

asfgit commented Jun 5, 2019

Uh oh!

asfgit commented Jun 7, 2019

Uh oh!

asfgit commented Jun 7, 2019

Uh oh!

mitchell852 commented Jun 7, 2019

Uh oh!

asfgit commented Jun 10, 2019

Uh oh!

asfgit commented Jun 10, 2019

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jun 10, 2019

Uh oh!

asfgit commented Jun 10, 2019

Uh oh!

asfgit commented Jun 10, 2019

mitchell852 commented Jun 4, 2019 •

edited

Loading