frontend: Route Details - Updated TLS Settings to allow Copy and Mask settings #87

dtaylor113 · 2018-06-01T13:46:13Z

Updated TLS Settings section of Route Details page:

@openshift/team-ux-review

Related PRs for hide/reveal secrets; lots of design conversations:
#23
#50

spadgett · 2018-06-01T13:51:32Z

Only key is sensitive. I'm not sure I'd conceal them all (even if we do in OpenShift console)

spadgett · 2018-06-01T13:52:42Z

frontend/public/components/configmap-and-secret-data.jsx

    const { showSecret } = this.state;
    const dl = [];
    Object.keys(data || {}).sort().forEach(k => {
-      const value = window.atob(data[k]);
+      const value = type === 'route.openshift.io' ? data[k] : window.atob(data[k]);


I'd rather not have a special check like this. We should generalize the component if we want to reuse it.

I think it's cleaner just not to use SecretData since it's only one field we want to mask.

Also, this is still cluttered way to make a new array...(but that's a lingering comment).

+1 for generalizing component.

spadgett · 2018-06-01T13:54:21Z

It would be nice to still be able to copy the other certs even if we don't mask them.

spadgett · 2018-06-01T14:32:52Z

Suggest moving termination type and allow insecure traffic above the certificates

jhadvig · 2018-06-01T16:07:04Z

It would be nice to still be able to copy the other certs even if we don't mask them.

For that either use the ConfigMapData component or extract the functionality into a separate component which would be used for both for TLSSettings and ConfigMapData.

spadgett · 2018-06-01T16:09:54Z

Why not just use the CopyToClipboard component directly?

jhadvig · 2018-06-01T16:13:29Z

frontend/public/components/configmap-and-secret-data.jsx

    const { showSecret } = this.state;
    const dl = [];
    Object.keys(data || {}).sort().forEach(k => {
-      const value = window.atob(data[k]);
+      const value = type === 'route.openshift.io' ? data[k] : window.atob(data[k]);


+1 for generalizing component.

spadgett · 2018-06-01T16:14:15Z

frontend/public/components/routes.tsx

+  let secrets = _.omit(props.tls, ['termination', 'insecureEdgeTerminationPolicy']);
+
+  return <React.Fragment>
+    <SecretData heading="TLS Settings" data={secrets} type="route.openshift.io"/>


Switching to SecretData also changes properties to alphabetical order, which is not really what we want here.

beanh66 · 2018-06-01T16:15:49Z

LGTM @dtaylor113

dtaylor113 · 2018-06-01T18:11:57Z

Hi @spadgett, @beanh66, just updated code and screenshots. Only major difference is now we have field level hide/reveal, not sure of the placement of the 'hide/show' link though... @openshift/team-ux-review

spadgett · 2018-06-01T18:23:13Z

This looks really complicated for what we want and we also lose the ordering of the fields. Is it possible to create a simple show/hide component we can use for the one field we want to hide? And just use CopyToClipboard directly for the other 3 fields?

beanh66 · 2018-06-04T15:20:22Z

Agreed @spadgett

dtaylor113 · 2018-06-04T16:32:45Z

Hi @spadgett & @beanh66, updated code and screenshots. PTAL

dtaylor113 · 2018-06-04T16:33:37Z

frontend/public/components/utils/mask-data.jsx

+
+    return <React.Fragment>
+      <dt>{title} {value &&
+        <button className="btn btn-link co-mask-data__btn" style={{paddingLeft: '24px'}} type="button" onClick={this.toggleSecret}>


I can't get co-mask-data__btn to reflect in the browser thus the hard-coded style=....

I'd rather add a class and we can help you test if necessary. (We should figure out what's wrong in your environment, though.)

I would rather add a class as well, but at this point I can't even change an existing scss value. Here I changed top to 10px, but when I inspect it it still says 0:

When I click on the link to the .scss file, it does display the updated value:

I've tried starting/stopping server and dev, ./build.sh, ./build-frontend.sh, yarn run build, etc.. Very frustrating, I just can't get scss changes/additions to show up in browser :-(

spadgett

Can you add a screenshot?

spadgett · 2018-06-04T17:42:49Z

frontend/public/components/routes.tsx

    <dt>CA Certificate</dt>
-    <dd>{props.tls.caCertificate || '-'}</dd>
+    <dd>{props.tls.caCertificate ? <CopyToClipboard value={props.tls.caCertificate}/> : '-'}</dd>
    {_.get(props.tls, 'termination') === 'reencrypt' && <span>


not new, but this should be a React.Fragment instead of span

spadgett · 2018-06-04T17:44:50Z

frontend/public/components/utils/mask-data.jsx

+    const visibleValue = showSecret ? value : <MaskedData /> ;
+
+    return <React.Fragment>
+      <dt>{title} {value &&


Let's not have the dt/dd be part of this. Better to make this just the value so that it can be used outside of a dl element.

spadgett · 2018-06-04T17:45:19Z

frontend/public/components/utils/mask-data.jsx

+export const MaskedData = () => <React.Fragment>
+  <span className="sr-only">Value hidden</span>
+  <span aria-hidden="true">&bull;&bull;&bull;&bull;&bull;</span>
+</React.Fragment>;


Since this is exactly the same as the one in secret data, we should be able to reuse this there instead of copying.

I moved it from secretData to the new MaskData component, seems more appropriate in the new component.

spadgett · 2018-06-04T17:47:24Z

frontend/public/components/utils/mask-data.jsx

+
+    return <React.Fragment>
+      <dt>{title} {value &&
+        <button className="btn btn-link co-mask-data__btn" style={{paddingLeft: '24px'}} type="button" onClick={this.toggleSecret}>


I'd rather add a class and we can help you test if necessary. (We should figure out what's wrong in your environment, though.)

dtaylor113 · 2018-06-04T19:24:48Z

Hi @spadgett, I think this is the format you want. Also, I think the .scss problem I was having was some issue processing scss in a .jsx file and using it in a .tsx file?...if I had to guess. Now that I've declared the component in .tsx, rather than a separate component in a .jsx, my scss changes now appear in the browser.

spadgett

Thanks @dtaylor113. This looks a lot simpler.

spadgett · 2018-06-04T19:37:26Z

frontend/public/components/routes.tsx

+  }
+
+  render() {
+    const { showPrivateKey } = this.state;


I'd add

const { tls } = this.props;

to avoid having to repeat this.props.tls everywhere

spadgett · 2018-06-04T19:40:18Z

frontend/public/components/routes.tsx

+        <dt>Certificate</dt>
+        <dd>{this.props.tls.certificate ? <CopyToClipboard value={this.props.tls.certificate}/> : '-'}</dd>
+        <dt>Key {this.props.tls.key &&
+        <button className="btn btn-link co-mask-data__btn" type="button" onClick={this.toggleKey}>


Do we want the button right-aligned like the secret page?

Seems like a @openshift/team-ux-review question :-) My only concern is that perhaps users would think they would have to 'Reveal' the value in order to 'Copy' it -if the buttons are close together.

spadgett · 2018-06-04T19:41:21Z

frontend/public/components/routes.tsx

+        <dd>{this.props.tls.key ? <CopyToClipboard value={this.props.tls.key} visibleValue={visibleKeyValue}/> : '-'}</dd>
+        <dt>CA Certificate</dt>
+        <dd>{this.props.tls.caCertificate ? <CopyToClipboard value={this.props.tls.caCertificate}/> : '-'}</dd>
+        {_.get(this.props.tls, 'termination') === 'reencrypt' && <React.Fragment>


We already know tls is defined from above, so you don't need to use _.get.

{this.props.tls.termination === 'reencrypt' && <React.Fragment>

spadgett · 2018-06-04T19:43:36Z

frontend/public/components/routes.tsx

 import { registerTemplate } from '../yaml-templates';
 // eslint-disable-next-line no-unused-vars
 import {K8sResourceKind, K8sResourceKindReference} from '../module/k8s';
+import {SafetyFirst} from './safety-first';


nit: I'd change this lines 10-11 to have spaces inside the curly braces to use a consistent style as above.

dtaylor113 · 2018-06-05T13:08:11Z

Hi @beanh66, any update on this? I think we're just questioning whether the 'Reveal Value' link/button should be right-aligned over the Copy button. -thanks :-)

beanh66 · 2018-06-05T13:47:03Z

@dtaylor113 yes can you change the reveal for an individual value to be right aligned and drop the word "value" since in this case it is clear which field you will be revealing.

dtaylor113 · 2018-06-05T14:37:56Z

can you change the reveal for an individual value to be right aligned and drop the word "value" since in this case it is clear which field you will be revealing

@beanh66 updated screenshots in PR description. -thanks!

cshinn · 2018-06-05T14:49:45Z

@dtaylor113 Looks good to me

Code has significantly changed

dtaylor113 · 2018-06-05T19:55:08Z

@spadgett PTAL when you get a moment, thanks

benjaminapetersen · 2018-06-06T14:18:18Z

frontend/public/components/routes.tsx

+          <button className="btn btn-link co-m-route-tls-reveal__btn" type="button" onClick={this.toggleKey}>
+            {
+              showPrivateKey
+                ? <React.Fragment><i className="fa fa-eye-slash" aria-hidden="true"></i> Hide</React.Fragment>


Does this need a <React.Fragment> wrapper? I thought that was only necessary if you were going to render sibling nodes. It looks like you are returning a single <i> node.

The <React.Fragment> encapsulates the icon and the text ('Hide' or 'Reveal')

Ah, i see it, sorry about that :)

benjaminapetersen · 2018-06-06T14:20:29Z

frontend/public/components/routes.tsx

+    const visibleKeyValue = showPrivateKey ? tls.key : <MaskedData /> ;
+
+    return !tls ? 'TLS is not enabled.'
+      : <dl>


Care to indent the <dt> under the <dl>?

return !tls ? 'TLS is not enabled' : <dl> <dt>stuff</dt> <dt>things</dt> </dl>;

benjaminapetersen · 2018-06-06T14:30:20Z

frontend/public/components/routes.tsx

  }
-</span>;
+}



Missed on the prev PR for this, but you might consider upgrading your _.each to a _.reduce & eliminate some bookkeeping in calcTrafficPercentage:

const calcTrafficPercentage = (weight: number, route: any) => { return _.reduce(route.spec.alternateBackends, (result, alternate) => { return result += alternate.weight; }, 0 + route.spec.to.weight); };

Hi @benjaminapetersen, still trying to understand this formula. When I plug it in, all percentages come back as 100%:

Whereas, they should equal the Origin Console Route's Traffic table:

Got it:

let totalWeight = _.reduce(route.spec.alternateBackends, (result, alternate) => { return result += alternate.weight; }, 0 + route.spec.to.weight); return (weight/totalWeight*100).toFixed(1);

Sorry for oops on syntax, didn't pull your code & test, but was just pointing out an alt to .each to abstract some of bookkeeping.

dtaylor113 · 2018-06-06T15:35:07Z

jenkins rebuild

spadgett · 2018-06-06T16:41:22Z

frontend/public/components/routes.tsx

+  let totalWeight = _.reduce(route.spec.alternateBackends, (result, alternate) => {
+    return result += alternate.weight;
+  }, 0 + route.spec.to.weight);
+
  return (weight/totalWeight*100).toFixed(1);


@dtaylor113 Can you confirm it's not possible to set all weights to 0? Otherwise this could be a divide by zero error.

Weights can be set to 0, adjusted code to handle

spadgett · 2018-06-06T16:44:24Z

Ignore the ci/* failures. It's a CI problem we're working to fix.

spadgett · 2018-06-06T20:01:02Z

frontend/public/components/routes.tsx

-  _.each(route.spec.alternateBackends, alternate => totalWeight += alternate.weight);
+  let totalWeight = _.reduce(route.spec.alternateBackends, (result, alternate) => {
+    return result += alternate.weight;
+  }, 0 + route.spec.to.weight);


Why do we need 0 +?

spadgett · 2018-06-06T22:44:43Z

frontend/public/components/routes.tsx

+    return result += alternate.weight;
+  }, route.spec.to.weight);
+
+  return totalWeight ? (weight/totalWeight*100).toFixed(1) : (weight*100).toFixed(1);


If totalWeight is 0, we know weight is 0, too. So we can just return 0.

const percentage = totalWeight ? ((weight / totalWeight) * 100) : 0; // Format to one decimal place. return percentage.toFixed(1);

But if weight is 0, we can return early without calculating since percentage will always be 0. (We might just use - in that case since the function is already returning a string and I think it displays better.)

const calcTrafficPercentage = (weight: number, route: any) => { if (!weight) { return '-'; }

Not new, but it's a little strange that we do some formatting here, but add the % below. toFixed already converts to a string, so we can't use the result as a number. Might as well add the % here.

const percentage = (weight / totalWeight) * 100; // Format to one decimal place. return `${percentage.toFixed(1)}%`;

(Sorry I should have caught that on previous review.)

fixed, thanks Sam

spadgett · 2018-06-06T22:57:37Z

frontend/public/components/routes.tsx

-  totalWeight += route.spec.to.weight;
-  _.each(route.spec.alternateBackends, alternate => totalWeight += alternate.weight);
-  return (weight/totalWeight*100).toFixed(1);
+  let totalWeight = _.reduce(route.spec.alternateBackends, (result, alternate) => {


…data

dtaylor113 · 2018-06-08T15:07:01Z

Hi @spadgett, PTAL when you get a moment. -thanks

spadgett

👍

/lgtm

* added action and reducer for storing and updating activeApplication * created application-switcher * show nothing in title when app switcher is disabled * remove application specific logic from label-dropdown * fixed bugs in label-dropdown and dropdown * show all apps when namespace is changed * added changes according to review comments

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 1, 2018

dtaylor113 requested review from spadgett and jhadvig June 1, 2018 13:46

spadgett requested changes Jun 1, 2018

View reviewed changes

dtaylor113 changed the title ~~frontend: Route Details - Added show/hide TLS certificates, keys~~ [WIP] frontend: Route Details - Added show/hide TLS certificates, keys Jun 1, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2018

jhadvig previously requested changes Jun 1, 2018

View reviewed changes

spadgett reviewed Jun 1, 2018

View reviewed changes

dtaylor113 force-pushed the routes-show-hide-certs branch from ace7b9a to 28bd895 Compare June 1, 2018 18:03

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2018

dtaylor113 changed the title ~~[WIP] frontend: Route Details - Added show/hide TLS certificates, keys~~ [WIP] frontend: Route Details - Updated TLS Settings to allow Copy and Mask settings Jun 1, 2018

dtaylor113 force-pushed the routes-show-hide-certs branch from 28bd895 to 1569925 Compare June 4, 2018 16:27

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 4, 2018

dtaylor113 commented Jun 4, 2018

View reviewed changes

spadgett previously requested changes Jun 4, 2018

View reviewed changes

dtaylor113 force-pushed the routes-show-hide-certs branch from 1569925 to 6af825d Compare June 4, 2018 19:22

spadgett reviewed Jun 4, 2018

View reviewed changes

dtaylor113 force-pushed the routes-show-hide-certs branch from 5c57458 to ec34e65 Compare June 5, 2018 14:35

benjaminapetersen reviewed Jun 6, 2018

View reviewed changes

dtaylor113 force-pushed the routes-show-hide-certs branch from ec34e65 to f692a32 Compare June 6, 2018 15:13

spadgett reviewed Jun 6, 2018

View reviewed changes

dtaylor113 force-pushed the routes-show-hide-certs branch from f692a32 to 1a81840 Compare June 6, 2018 20:45

spadgett requested changes Jun 6, 2018

View reviewed changes

Updated Route Details TLS Settings section, copy-to-clipboard & mask-…

40b045b

…data

dtaylor113 force-pushed the routes-show-hide-certs branch from 1a81840 to 40b045b Compare June 7, 2018 13:30

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2018

spadgett approved these changes Jun 8, 2018

View reviewed changes

openshift-ci-robot assigned spadgett Jun 8, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2018

dtaylor113 merged commit 8928128 into openshift:master Jun 8, 2018

dtaylor113 mentioned this pull request Jul 5, 2018

Route Details: TLS Section - CopyToClipboard display issue #204

Closed

dtaylor113 deleted the routes-show-hide-certs branch January 16, 2019 14:43

frontend: Route Details - Updated TLS Settings to allow Copy and Mask settings #87

frontend: Route Details - Updated TLS Settings to allow Copy and Mask settings #87

Conversation

dtaylor113 commented Jun 1, 2018 • edited

Updated TLS Settings section of Route Details page:

spadgett commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jun 1, 2018

spadgett commented Jun 1, 2018

jhadvig commented Jun 1, 2018

spadgett commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beanh66 commented Jun 1, 2018

dtaylor113 commented Jun 1, 2018

spadgett commented Jun 1, 2018

beanh66 commented Jun 4, 2018

dtaylor113 commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a 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

dtaylor113 commented Jun 4, 2018

spadgett left a 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

dtaylor113 commented Jun 5, 2018

beanh66 commented Jun 5, 2018

dtaylor113 commented Jun 5, 2018

cshinn commented Jun 5, 2018

dtaylor113 commented Jun 5, 2018

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

dtaylor113 commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jun 6, 2018

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

dtaylor113 commented Jun 8, 2018

spadgett left a comment

Choose a reason for hiding this comment

dtaylor113 commented Jun 1, 2018 •

edited