Changing the statusText to be an object with more fields, then displa… (#1395)

* Changing the statusText to be an object with more fields, then displaying them in the ErrorBanner

Signed-off-by: Adam Christian <adam@buoyant.io>

Refactoring karma tests and propTypes and defaultProps per the code review from @rmars

Signed-off-by: Adam Christian <adam@buoyant.io>

Changing the default message to pass the ServiceMeshTest ErrorBanner assertion

Revert "Changing the default message to pass the ServiceMeshTest ErrorBanner assertion"

This reverts commit 2415b7099b03ad7a8deda9f67218bb531111b3ec.

Fixing the failing karma unit tests because the statusMessage wasn't being properly passed into the component rendering stub context

Signed-off-by: Adam Christian <adam@buoyant.io>

merging master in

Signed-off-by: Adam Christian <adam@buoyant.io>

* Export api error type independently from ApiHelpers

Signed-off-by: Adam Christian <adam@buoyant.io>

web/app/js/components/ErrorBanner.jsx

@@ -1,17 +1,22 @@
 import _ from 'lodash';
+import { apiErrorPropType } from './util/ApiHelpers.jsx';
 import PropTypes from 'prop-types';
 import React from 'react';
 import { Col, Row } from 'antd';
 
-const defaultErrorMsg = "An error has occurred";
-
 class ErrorMessage extends React.Component {
   static defaultProps = {
+    message: {
+      status: null,
+      statusText: "An error occured",
+      url: "",
+      error: ""
+    },
     onHideMessage: _.noop
   }
 
   static propTypes = {
-    message: PropTypes.string.isRequired,
+    message: apiErrorPropType,
     onHideMessage: PropTypes.func
   }
 
@@ -37,11 +42,15 @@ class ErrorMessage extends React.Component {
   }
 
   render() {
+    const { statusText, error, url, status } = this.props.message;
     return !this.state.visible ? null : (
       <Row gutter={0}>
         <div className="error-message-container">
           <Col span={20}>
-            {this.props.message || defaultErrorMsg}
+            <div><b>Error:</b> {status} {statusText}</div>
+            { !error ? null : <div><b>Message:</b> {error}</div> }
+            <div><b>URL:</b> {url}</div>
+
           </Col>
           <Col span={4}>
             <div className="dismiss" onClick={this.hideMessage} role="presentation">Dismiss X</div>

web/app/js/components/Namespace.jsx

@@ -89,20 +89,20 @@ class Namespaces extends React.Component {
           metrics: metrics,
           loaded: true,
           pendingRequests: false,
-          error: ''
+          error: null
         });
       })
       .catch(this.handleApiError);
   }
 
-  handleApiError(e) {
+  handleApiError = e => {
     if (e.isCanceled) {
       return;
     }
 
     this.setState({
       pendingRequests: false,
-      error: `Error getting data from server: ${e.message}`
+      error: e
     });
   }
 

web/app/js/components/ResourceList.jsx

@@ -1,4 +1,5 @@
 import _ from 'lodash';
+import { apiErrorPropType } from './util/ApiHelpers.jsx';
 import ErrorBanner from './ErrorBanner.jsx';
 import { friendlyTitle } from './util/Utils.js';
 import MetricsTable from './MetricsTable.jsx';
@@ -13,12 +14,12 @@ import 'whatwg-fetch';
 
 export class ResourceListBase extends React.Component {
   static defaultProps = {
-    error: null,
+    error: null
   }
 
   static propTypes = {
     data: PropTypes.arrayOf(metricsPropType.isRequired).isRequired,
-    error: PropTypes.oneOfType([PropTypes.string, PropTypes.instanceOf(Error)]),
+    error:  apiErrorPropType,
     loading: PropTypes.bool.isRequired,
     resource: PropTypes.string.isRequired,
   }

web/app/js/components/ServiceMesh.jsx

@@ -149,7 +149,7 @@ class ServiceMesh extends React.Component {
       components: [],
       pendingRequests: false,
       loaded: false,
-      error: ''
+      error: null
     };
   }
 
@@ -237,7 +237,7 @@ class ServiceMesh extends React.Component {
           nsStatuses: this.extractNsStatuses(nsStats),
           pendingRequests: false,
           loaded: true,
-          error: ''
+          error: null
         });
       })
       .catch(this.handleApiError);
@@ -250,7 +250,7 @@ class ServiceMesh extends React.Component {
 
     this.setState({
       pendingRequests: false,
-      error: `Error getting data from server: ${e.message}`
+      error: e
     });
   }
 

web/app/js/components/Tap.jsx

@@ -37,9 +37,9 @@ class Tap extends React.Component {
     this.loadFromServer = this.loadFromServer.bind(this);
 
     this.state = {
-      error: "",
       tapResultsById: {},
       tapResultFilterOptions: this.getInitialTapFilterOptions(),
+      error: null,
       resourcesByNs: {},
       authoritiesByNs: {},
       query: {
@@ -404,7 +404,7 @@ class Tap extends React.Component {
 
     this.setState({
       pendingRequests: false,
-      error: `Error getting data from server: ${e.message}`
+      error: e
     });
   }
 

web/app/js/components/Version.jsx

@@ -1,3 +1,4 @@
+import { apiErrorPropType } from './util/ApiHelpers.jsx';
 import { Link } from 'react-router-dom';
 import PropTypes from 'prop-types';
 import React from 'react';
@@ -12,7 +13,7 @@ class Version extends React.Component {
   }
 
   static propTypes = {
-    error: PropTypes.instanceOf(Error),
+    error: apiErrorPropType,
     isLatest: PropTypes.bool.isRequired,
     latestVersion: PropTypes.string,
     productName: PropTypes.string,
@@ -26,7 +27,7 @@ class Version extends React.Component {
       return (
         <div>
           Version check failed
-          {error ? `: ${error}` : ''}
+          {error ? `: ${error.statusText}` : ''}
         </div>
       );
     }

web/app/js/components/util/ApiHelpers.jsx

@@ -5,11 +5,17 @@ import React from 'react';
 import 'whatwg-fetch';
 
 const checkFetchOk = resp => {
-  if (!resp.ok) {
-    throw Error(resp.statusText);
-  } else {
-    return resp;
-  }
+  if (resp.ok) { return resp; }
+
+  return resp.json().then(error => {
+    throw {
+      status: resp.status,
+      url: resp.url,
+      statusText:resp.statusText,
+      error: error.error
+    };
+  });
+
 };
 
 // makeCancelable from @istarkov
@@ -37,6 +43,13 @@ const makeCancelable = (promise, onSuccess) => {
   };
 };
 
+export const apiErrorPropType =  PropTypes.shape({
+  status: PropTypes.number,
+  url: PropTypes.string,
+  statusText: PropTypes.string,
+  error: PropTypes.string
+});
+
 const ApiHelpers = (pathPrefix, defaultMetricsWindow = '1m') => {
   let metricsWindow = defaultMetricsWindow;
   const podsPath = `/api/pods`;
@@ -102,6 +115,7 @@ const ApiHelpers = (pathPrefix, defaultMetricsWindow = '1m') => {
     });
   };
 
+
   // prefix all links in the app with `pathPrefix`
   class PrefixedLink extends React.Component {
     static defaultProps = {

web/app/js/components/util/withREST.jsx

@@ -37,7 +37,7 @@ const withREST = (WrappedComponent, componentPromises, options={}) => {
       data: [],
       pendingRequests: false,
       loading: true,
-      error: ''
+      error: null
     });
 
     componentDidMount() {
@@ -92,7 +92,7 @@ const withREST = (WrappedComponent, componentPromises, options={}) => {
             data: data,
             loading: false,
             pendingRequests: false,
-            error: '',
+            error: null,
           });
         })
         .catch(this.handleApiError);
@@ -103,7 +103,7 @@ const withREST = (WrappedComponent, componentPromises, options={}) => {
 
       this.setState({
         pendingRequests: false,
-        error: `Error getting data from server: ${e.message}`
+        error: e
       });
     }
 
@@ -121,4 +121,4 @@ const withREST = (WrappedComponent, componentPromises, options={}) => {
   return withContext(RESTWrapper);
 };
 
-export default withREST;
+export default withREST;

web/app/test/ApiHelpersTest.jsx

@@ -199,7 +199,9 @@ describe('ApiHelpers', () => {
       let errorMessage = "do or do not. there is no try.";
       fetchStub.returnsPromise().resolves({
         ok: false,
-        statusText: errorMessage
+        json: () => Promise.resolve({
+          error: errorMessage
+        }),
       });
 
       api = ApiHelpers('');
@@ -212,7 +214,7 @@ describe('ApiHelpers', () => {
           return Promise.reject('Expected method to reject.');
         }, errorHandler)
         .then(() => {
-          expect(errorHandler.args[0][0].message).to.equal(errorMessage);
+          expect(errorHandler.args[0][0].error).to.equal(errorMessage);
           expect(errorHandler.calledOnce).to.be.true;
         });
     });

web/app/test/ResourceListTest.jsx

@@ -23,7 +23,7 @@ describe('Tests for <ResourceListBase>', () => {
       <ResourceListBase
         {...defaultProps}
         data={[]}
-        error={msg}
+        error={{ statusText: msg}}
         loading={false} />
     );
 
@@ -32,8 +32,7 @@ describe('Tests for <ResourceListBase>', () => {
     expect(component.find(PageHeader)).to.have.length(1);
     expect(component.find(Spin)).to.have.length(0);
     expect(component.find(MetricsTable)).to.have.length(1);
-
-    expect(err.props().message).to.equal(msg);
+    expect(err.props().message.statusText).to.equal(msg);
   });
 
   it('shows a loading spinner', () => {

web/app/test/ServiceMeshTest.jsx

@@ -31,10 +31,14 @@ describe('ServiceMesh', () => {
   it("displays an error if the api call didn't go well", () => {
     let errorMsg = "Something went wrong!";
 
-    fetchStub.resolves({
+    fetchStub.returnsPromise().resolves({
       ok: false,
+      json: () => Promise.resolve({
+        error: errorMsg
+      }),
       statusText: errorMsg
     });
+
     component = mount(routerWrap(ServiceMesh));
 
     return withPromise(() => {

web/app/test/VersionTest.jsx

@@ -122,7 +122,10 @@ describe('Version', () => {
 
     fetchStub.returnsPromise().resolves({
       ok: false,
-      statusText: errMsg
+      json: () => Promise.resolve({
+        error: errMsg
+      }),
+      statusText: errMsg,
     });
 
     component = mount(