Converts Edge/Mid header rewrite, regexremap, remaptext and tr request/response header text inputs to textareas by mitchell852 · Pull Request #3474 · apache/trafficcontrol

mitchell852 · 2019-04-08T21:44:27Z

This PR fixes Mid/Edge header rewrite rules per DS should not need __RETURN__ #3426

By converting the edge/mid header rewrite, regex remap, remap text and tr request/response header fields of the DS forms to textareas instead of text input, line breaks can be preserved and eliminate the need to use an explicit __RETURN__ to indicate a line break. (although __RETURN__ will still be respected and recognized as a line break for backwards compatability).

This PR also changes the presentation of delivery service request diffs to account for multiline values.

Which Traffic Control components are affected by this PR?

Traffic Portal

What is the best way to verify this PR?

Run automated tests

Run the TO unit tests -

cd trafficcontrol/traffic_ops/traffic_ops_golang
go test $(go list ./...)

Run the API tests - https://github.com/apache/trafficcontrol/blob/master/traffic_ops/testing/api/README.md

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

Manual validation

a. set dsRequests.enabled=true in traffic_portal_properties.json and restart TP
b. in TP, create/edit a dns* (you can do http*, steering or any_map as well but only some field apply to each) delivery service and modify the edge/mid header rewrite rules, regex remap, remap text and tr request/response headers to include multiline rules.
c. save the changes which will create a ds request.
d. review the ds request and fulfill it. fulfilling the ds request persists the ds changes to the database.
e. verify the contents of the header rewrite files and ensure the multiline rules are presented on separate lines:

edge header rewrite rules can be viewed by GET api/1.4/cdns/cdn-name/configfiles/ats/hdr_rw_xml-id.config
mid header rewrite rules can be view by GET api/1.4/cdns/cdn-name/configfiles/ats/hdr_rw_mid_xml-id.config

f. verify the contents of the regex remap config file and ensure the multiline rules are presented on separate lines:

GET api/1.4/cdns/cdn-name/configfiles/ats/regex_remap_xml-id.config

g. verify the tr request/response rules show up in the snapshot correctly

add multiline rules for tr request/response headers
do a snapshot diff in TP, look in the ds section for your tr request/response header change. ensure that multiple lines resulted in multiple header rules
perform a snapshot then you can inspect manually if you want as follows:

GET api/1.4/cdns/cdn-name/snapshot (current snapshot)
GET api/1.4/cdns/cdn-name/snapshot/new (next snapshot)

Note: we don't currently have any API tests to parse and validate the contents of config files.

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-04-08T21:52:38Z

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

asfgit · 2019-04-09T03:24:42Z

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

ocket8888 · 2019-04-10T16:00:33Z

I am +8 million on this

asfgit · 2019-04-10T22:28:16Z

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

asfgit · 2019-04-10T22:32:31Z

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

asfgit · 2019-04-17T18:54:24Z

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

asfgit · 2019-04-24T14:35:54Z

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

asfgit · 2019-04-24T20:47:24Z

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

asfgit · 2019-04-24T20:51:51Z

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

asfgit · 2019-04-24T21:15:47Z

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

ocket8888 · 2019-04-24T21:19:45Z

docs/source/admin/traffic_ops/using.rst

I don't really think it's necessary to update the docs for a dead UI :P

Not that I'm saying you should revert this - it's fine, just for future reference.

yeah, people still might stumble here so i updated it. we need an effort to purge everything old TO UI related however.

ocket8888 · 2019-04-24T21:34:39Z

traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go

This regex eats all whitespace around a __RETURN__, which turns out to be unnecessary. Later as the values are iterated over they are individually trimmed anyway, so strings.Split works fine. Of course, you still wanna split on newlines instead of just __RETURN__, so you probably wanna use strings.Replace first to turn all of those into newlines.

yeah, you're right, it's redundant as it trims later. removed that.

ocket8888 · 2019-04-24T21:48:25Z

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

semantically a <fieldset> encloses a set of controls, of which the "Current Value" is not one. Plus the current value is totally immutable, so not very well represented as an <input> - even if it is readonly. Honestly I think <small> was fine here, but you could also use an <output> (optionally with a <label>).

"The <output> tag represents the result of a calculation (like one performed by a script)."

that's a definition I found so not sure output is the right answer but i did try it and i was getting weird wrapping issues if the value was long.

as far as styling, i was just looking for the look of a disabled input/textarea and those elements give me that by simply using the readonly attribute. whereas, if i wanted to use small i'd have to wrestle with some styling.

and yeah, the fieldset, though probably not technically the correct element, was simply used to achieve the style/layout.

I'm not sold on <textarea> either, since it's defined as an editing control that gets submitted along with the parent form. Not that we actually do form submission, but semantically user-agents will expect that it is an input control that simply cannot be changed. You could try <pre> or <samp> instead.

Technically anything that would be a previous value is the result of some user's actions, but not necessarily recently, or by the current user, or even using the TP UI at all. Plus, a previous value can only exist if there's a new current value. So maybe <output> isn't the best way to represent it, but I could see an argument for it.

Styling of a fieldset appear consistent across Chrome, Firefox and Safari so there doesn't really seem to be any harm in using it (unless you want its contents to display with flex or grid, which is bugged in Edge and Chrome). My only concern is that it appears to the user-agent as a set of grouped controls, and actually does contain <input>/<textarea> form controls that aren't really part of the form.

let me try pre or samp

ocket8888 · 2019-04-24T21:55:13Z

traffic_portal/app/src/common/modules/form/_form.scss

This rule doesn't actually match anything as far as I can tell - though I could've missed something. The <textarea>s seem to only have the readonly class. But you don't really need either of them, since the things you're trying to style will match textarea[readonly][autosize].

This matches the following elements in the ds forms:

<textarea id="edgeFQDNs" name="edgeFQDNs" rows="{{deliveryService.exampleURLs.length}}" class="form-control autosize" readonly>{{edgeFQDNs(deliveryService)}}</textarea>

but i made a change to _form.scss to use the pseudo selector read-only and removed the readonly class.

ooo even better

ocket8888 · 2019-04-24T22:01:02Z

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

autofocus doesn't make much sense on a control that's halfway down the page and hidden in an "Advanced" submenu. type="text" is also meaningless on a <textarea> element.

ocket8888 · 2019-04-24T22:18:41Z

Most of my comments apply to several dozen repeated areas, but to avoid spamming the PR I just made each comment once.

asfgit · 2019-04-25T14:15:49Z

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

asfgit · 2019-04-25T14:39:57Z

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

asfgit · 2019-04-25T14:55:05Z

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

ocket8888

Missed this - there are actually a few flubs like this in the changelog file. Should probably open a PR to fix that - or is that considered editing history?

ocket8888 · 2019-04-26T15:13:04Z

CHANGELOG.md

This is markdown, so the __s on that __RETURN__ are gonna get eaten as formatting - wrap in backticks to make it pre-formatted.

…eserve line breaks and eliminate the need for __RETURN__

… more investigation

…oying

…ine values are displayed.

…ced with \n and than the full value of tr_request/response_header is split on \n

…ce a line break

…rmed subsequently already

… properties

ocket8888 · 2019-04-26T19:48:23Z

traffic_portal/app/src/common/modules/form/_form.scss

+	position: relative;
+	border-radius: 4px;
+	background-color: #f5f5f5;
+	padding-left: 10px !important;


were border and padding-left being overridden by something?

nope. i'll fix that. i was just copying some stying from the fieldset

asfgit · 2019-04-26T19:58:07Z

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

asfgit · 2019-04-26T20:02:01Z

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

ocket8888 · 2019-04-26T19:52:13Z

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

-                    <small class="input-diff" ng-if="settings.isRequest" ng-show="open() && deliveryService.active != dsCurrent.active">Current Value: [ {{dsCurrent.active ? 'Active' : 'Not Active'}} ]</small>
+                    <aside class="current-value" ng-if="settings.isRequest" ng-show="open() && deliveryService.active != dsCurrent.active">
+                        <label for="current-value-active">Current Value</label>
+                        <pre id="current-value-active">{{::dsCurrent.active ? 'Active' : 'Not Active'}}</pre>


<pre> tags are not labelable, so if you wanna use those you should maybe change the <label>s to <h3> (I think that's the appropriate level without looking - basically <h{{n}}> where n is one greater than the lowest-level section heading already used in the document) and getting rid of those for and id attributes.

yeah, i can switch to <h{{n}}> but i guess i'll just pick n because there are no other instance of header tags used in the document except h6

odd, I swear there was at least one. In general you're not supposed to skip levels, but I think it's fine since h1 and h2 don't really seem appropriate at that level.

asfgit · 2019-04-29T16:45:18Z

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

mitchell852 · 2019-04-29T18:05:31Z

@ocket8888 - thanks for the code review / suggestions. were you able to confirm the expected behavior as outlined in the description?

ocket8888 · 2019-04-30T18:36:08Z

yep, it all appears to still work.

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

mitchell852 changed the title ~~Converts text inputs to text areas to preserve line breaks and eliminate the need for __RETURN__~~ Converts Edge/Mid header rewrite text inputs to text areas to preserve line breaks and eliminate the need for __RETURN__ Apr 8, 2019

mitchell852 force-pushed the issue-3426-header-rewrite-textareas branch from 992f208 to 5badaff Compare April 24, 2019 14:27

mitchell852 changed the title ~~Converts Edge/Mid header rewrite text inputs to text areas to preserve line breaks and eliminate the need for __RETURN__~~ Converts Edge/Mid header rewrite, regexremap, remaptext and tr request/response header text inputs to textareas Apr 24, 2019

mitchell852 added the Traffic Ops related to Traffic Ops label Apr 24, 2019

mitchell852 marked this pull request as ready for review April 24, 2019 21:07

ocket8888 suggested changes Apr 24, 2019

View reviewed changes

ocket8888 suggested changes Apr 26, 2019

View reviewed changes

mitchell852 added 7 commits April 26, 2019 10:23

for a handful of ds properties converts text input to text area to pr…

64da07f

…eserve line breaks and eliminate the need for __RETURN__

more textareas to preserve line breaks

27b83a4

removed some fields that are outside of the scope of this PR and need…

8e3995e

… more investigation

fixes broken form field

88f7e62

disables scrolling on readonly autosized textareas because it was ann…

c046cca

…oying

changes the way ds request diffs are displayed. especially how multil…

69c8b3d

…ine values are displayed.

for tr_request/response_header values, __RETURN__ instances are repla…

978c6e7

…ced with \n and than the full value of tr_request/response_header is split on \n

mitchell852 added 6 commits April 26, 2019 10:23

cleans up documentation indicating that __RETURN__ is required to for…

d17ef0b

…ce a line break

adds changelog entry for deprecation of __RETURN__

02367d1

cleans up textareas

29f2aec

removes redundant readonly class

619eb87

no need to remove any spaces around the __RETURN__ as a trim is perfo…

c738df8

…rmed subsequently already

removes fieldsets and textarea/input used to show current value of ds…

f22cc8a

… properties

mitchell852 force-pushed the issue-3426-header-rewrite-textareas branch from 5cc2dfe to f22cc8a Compare April 26, 2019 19:39

adds tick marks around __RETURN__

b5fc0dc

ocket8888 reviewed Apr 26, 2019

View reviewed changes

ocket8888 suggested changes Apr 26, 2019

View reviewed changes

removes labels on pre tags

7376dd9

ocket8888 approved these changes Apr 29, 2019

View reviewed changes

mitchell852 assigned rawlinp May 1, 2019

rawlinp approved these changes May 1, 2019

View reviewed changes

rawlinp merged commit 1ce9526 into apache:master May 1, 2019

mitchell852 added the cache-config Cache config generation label Jun 27, 2019

Conversation

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

Uh oh!

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 Apr 8, 2019

Uh oh!

asfgit commented Apr 9, 2019

Uh oh!

ocket8888 commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfgit commented Apr 10, 2019

Uh oh!

asfgit commented Apr 10, 2019

Uh oh!

asfgit commented Apr 17, 2019

Uh oh!

asfgit commented Apr 24, 2019

Uh oh!

asfgit commented Apr 24, 2019

Uh oh!

asfgit commented Apr 24, 2019

Uh oh!

asfgit commented Apr 24, 2019

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!

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

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!

ocket8888 commented Apr 24, 2019

Uh oh!

asfgit commented Apr 25, 2019

Uh oh!

asfgit commented Apr 25, 2019

Uh oh!

asfgit commented Apr 25, 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!

asfgit commented Apr 26, 2019

Uh oh!

asfgit commented Apr 26, 2019

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

mitchell852 commented Apr 8, 2019 •

edited

Loading

ocket8888 commented Apr 10, 2019 •

edited

Loading

mitchell852 Apr 25, 2019 •

edited

Loading

mitchell852 commented Apr 29, 2019 •

edited

Loading