Fix/fetch promise refactor (#32)

* Add refactored fetch using promises - Use promise rather than callback to make code more concise - Leave in old accessAPI method until other *Actions are refactored * Add refactore InfoActions using promises - Fix bug where spinner would continue showing if an error had occured * Fix bug relating to catching failed promises * Remove old apiAccess method - Refactor *Actions to use new accessAPI method * Fix display of ErrorModal - Convert to pure component - Fix child ref table to use ApiAccess for API requests
ONSdigital · Mar 13, 2018 · d36b34d · d36b34d
1 parent e2bd3b6
commit d36b34d
Show file tree

Hide file tree

Showing 8 changed files with 123 additions and 159 deletions.
diff --git a/src/actions/ApiActions.js b/src/actions/ApiActions.js
@@ -1,4 +1,4 @@
-import { SET_RESULTS, SET_FORMATTED_QUERY, SET_SEARCH_ERROR_MESSAGE, SENDING_SEARCH_REQUEST, SET_QUERY, SET_HEADERS } from '../constants/ApiConstants';
+import { SET_RESULTS, SET_FORMATTED_QUERY, SET_SEARCH_ERROR_MESSAGE, SENDING_SEARCH_REQUEST, SET_QUERY } from '../constants/ApiConstants';
 import accessAPI from '../utils/accessAPI';
 import config from '../config/api-urls';
 
@@ -17,24 +17,20 @@ export function search(query, formQuery, jsonKey) {
     accessAPI(REROUTE_URL, 'POST', sessionStorage.accessToken, JSON.stringify({
       method: 'GET',
       endpoint: `${API_VERSION}/${formattedQuery}`,
-    }), (success, data) => {
+    })).then(json => {
       dispatch(sendingRequest(SENDING_SEARCH_REQUEST, false, jsonKey));
-      if (success) {
-        // This is a workaround for the API returning 200 {} for no results, should be 404
-        if (Object.keys(data.json).length === 0 && data.json.constructor === Object) {
-          dispatch(setErrorMessage(SET_SEARCH_ERROR_MESSAGE, '404: No results found.', jsonKey));
-        } else {
-          if (jsonKey === 'ubrn') {
-            // Wrap the results in an array as we only get {} from the API
-            dispatch(setResults(SET_RESULTS, [data.json], jsonKey));
-          } else {
-            dispatch(setResults(SET_RESULTS, data.json, jsonKey));
-          }
-          dispatch(setHeaders(SET_HEADERS, data.response, jsonKey));
-        }
+      // This is a workaround for the API returning 200 {} for no results, should be 404
+      if (Object.keys(json).length === 0 && json.constructor === Object) {
+        dispatch(setErrorMessage(SET_SEARCH_ERROR_MESSAGE, '404: No results found.', jsonKey));
+      } else if (jsonKey === 'ubrn') {
+        // Wrap the results in an array as we only get {} from the API
+        dispatch(setResults(SET_RESULTS, [json], jsonKey));
       } else {
-        dispatch(setErrorMessage(SET_SEARCH_ERROR_MESSAGE, data.message, jsonKey));
+        dispatch(setResults(SET_RESULTS, json, jsonKey));
       }
+    }).catch(msg => {
+      dispatch(sendingRequest(SENDING_SEARCH_REQUEST, false, jsonKey));
+      dispatch(setErrorMessage(SET_SEARCH_ERROR_MESSAGE, msg.toString(), jsonKey));
     });
   };
 }
@@ -51,10 +47,6 @@ function setFormattedQuery(type, query, jsonKey) {
   return { type, query, jsonKey };
 }
 
-function setHeaders(type, headers, jsonKey) {
-  return { type, headers, jsonKey };
-}
-
 function sendingRequest(type, sending, jsonKey) {
   return { type, sending, jsonKey };
 }

diff --git a/src/actions/InfoActions.js b/src/actions/InfoActions.js
@@ -11,16 +11,15 @@ export function getUiInfo() {
   return (dispatch) => {
     dispatch(sendingRequest(SENDING_UI_REQUEST, true));
 
-    accessAPI(`${AUTH_URL}/api/info`, 'GET', sessionStorage.accessToken, {}, (success, data) => {
+    accessAPI(`${AUTH_URL}/api/info`, 'GET', sessionStorage.accessToken, {}).then(json => {
       dispatch(sendingRequest(SENDING_UI_REQUEST, false));
-      if (success) {
-        dispatch(setInfo(SET_UI_INFO, {
-          version: data.json.version,
-          lastUpdate: data.json.lastUpdate,
-        }));
-      } else {
-        dispatch(setErrorMessage(SET_UI_ERROR_MESSAGE, data.message));
-      }
+      dispatch(setInfo(SET_UI_INFO, {
+        version: json.version,
+        lastUpdate: json.lastUpdate,
+      }));
+    }).catch(msg => {
+      dispatch(sendingRequest(SENDING_UI_REQUEST, false));
+      dispatch(setErrorMessage(SET_UI_ERROR_MESSAGE, msg));
     });
   };
 }
@@ -35,17 +34,16 @@ export function getApiInfo() {
     accessAPI(REROUTE_URL, 'POST', sessionStorage.accessToken, JSON.stringify({
       method: 'GET',
       endpoint: 'version',
-    }), (success, data) => {
+    })).then(json => {
+      dispatch(sendingRequest(SENDING_API_REQUEST, false));
+      dispatch(setInfo(SET_API_INFO, {
+        version: json.version,
+        lastApiUpdate: json.builtAtString,
+        lastDataUpdate: json.lastDataUpdate,
+      }));
+    }).catch(msg => {
       dispatch(sendingRequest(SENDING_API_REQUEST, false));
-      if (success) {
-        dispatch(setInfo(SET_API_INFO, {
-          version: data.json.version,
-          lastApiUpdate: data.json.builtAtString,
-          lastDataUpdate: data.json.lastDataUpdate,
-        }));
-      } else {
-        dispatch(setErrorMessage(SET_API_ERROR_MESSAGE, data.message));
-      }
+      dispatch(setErrorMessage(SET_API_ERROR_MESSAGE, msg));
     });
   };
 }

diff --git a/src/actions/LoginActions.js b/src/actions/LoginActions.js
@@ -29,26 +29,24 @@ export function login(username, password) {
 
     accessAPI(`${AUTH_URL}/auth/login`, 'POST', `Basic ${basicAuth}`, JSON.stringify({
       username,
-    }), (success, data) => {
-      // When the request is finished, hide the loading indicator
+    })).then(json => {
       dispatch(sendingRequest(false));
-      dispatch(setAuthState(success));
-      if (success) {
-        dispatch(setConfetti(data.json.showConfetti));
-        // If the login worked, forward the user to the dashboard and clear the form
-        dispatch(setUserState({
-          username,
-          role: data.json.role,
-          accessToken: data.json.accessToken,
-        }));
-        sessionStorage.setItem('accessToken', data.json.accessToken);
-        sessionStorage.setItem('username', username);
-        dispatch(getUiInfo());
-        dispatch(getApiInfo());
-        forwardTo('/Home');
-      } else {
-        dispatch(setErrorMessage(errorMessages.GENERAL_ERROR));
-      }
+      dispatch(setAuthState(true));
+      dispatch(setConfetti(json.showConfetti));
+      // If the login worked, forward the user to the dashboard and clear the form
+      dispatch(setUserState({
+        username,
+        role: json.role,
+        accessToken: json.accessToken,
+      }));
+      sessionStorage.setItem('accessToken', json.accessToken);
+      sessionStorage.setItem('username', username);
+      dispatch(getUiInfo());
+      dispatch(getApiInfo());
+      forwardTo('/Home');
+    }).catch(() => {
+      dispatch(sendingRequest(false));
+      dispatch(setErrorMessage(errorMessages.GENERAL_ERROR));
     });
   };
 }
@@ -58,24 +56,20 @@ export function login(username, password) {
  */
 export function checkAuth() {
   return (dispatch) => {
-    accessAPI(`${AUTH_URL}/auth/checkToken`, 'POST', sessionStorage.accessToken, {}, (success, data) => {
-      dispatch(setAuthState(success));
-      if (!success) {
-        sessionStorage.clear();
-        forwardTo('/');
-      } else if (success) {
-        if (window.location.pathname === '/') {
-          forwardTo('/Home');
-        }
-        dispatch(getUiInfo());
-        dispatch(getApiInfo());
-        dispatch(setUserState({
-          username: data.json.username,
-          accessToken: data.json.accessToken,
-          role: data.json.role,
-        }));
-        sessionStorage.setItem('accessToken', data.json.accessToken);
-      }
+    accessAPI(`${AUTH_URL}/auth/checkToken`, 'POST', sessionStorage.accessToken, {}).then(json => {
+      dispatch(setAuthState(true));
+      if (window.location.pathname === '/') forwardTo('/Home');
+      dispatch(getUiInfo());
+      dispatch(getApiInfo());
+      dispatch(setUserState({
+        username: json.username,
+        accessToken: json.accessToken,
+        role: json.role,
+      }));
+      sessionStorage.setItem('accessToken', json.accessToken);
+    }).catch(() => {
+      sessionStorage.clear();
+      forwardTo('/');
     });
   };
 }
@@ -86,22 +80,20 @@ export function checkAuth() {
 export function logout() {
   return (dispatch) => {
     dispatch(sendingRequest(true));
-    accessAPI(`${AUTH_URL}/auth/logout`, 'POST', sessionStorage.accessToken, {}, (success, data) => {
-      dispatch(sendingRequest(false));
-      if (success) {
-        dispatch(setAuthState(false));
-        sessionStorage.clear();
-        browserHistory.push('/');
-        // This needs to go at the end, or else if we logout whilst on a page
-        // that uses the redux store, an error will occur before the user
-        // is redirected to '/'.
-        dispatch(resetState(undefined));
-      } else {
-        dispatch(setErrorMessage(errorMessages.GENERAL_ERROR));
-        sessionStorage.clear();
-        browserHistory.push('/');
-        dispatch(resetState(undefined));
-      }
+    accessAPI(`${AUTH_URL}/auth/logout`, 'POST', sessionStorage.accessToken, {}).then(json => {
+      dispatch(setAuthState(false));
+      sessionStorage.clear();
+      browserHistory.push('/');
+      // This needs to go at the end, or else if we logout whilst on a page
+      // that uses the redux store, an error will occur before the user
+      // is redirected to '/'.
+      dispatch(resetState(undefined));
+    }).catch(() => {
+      dispatch(setAuthState(false));
+      dispatch(setErrorMessage(errorMessages.GENERAL_ERROR));
+      sessionStorage.clear();
+      browserHistory.push('/');
+      dispatch(resetState(undefined));
     });
   };
 }

diff --git a/src/components/ChildRefTable.js b/src/components/ChildRefTable.js
@@ -5,6 +5,7 @@ import ErrorModal from '../components/ErrorModal';
 import industryCodeDescription from '../utils/siccode';
 import config from '../config/api-urls';
 import { formatData } from '../utils/helperMethods';
+import accessAPI from '../utils/accessAPI';
 
 const { REROUTE_URL, API_VERSION, BUSINESS_ENDPOINT } = config;
 
@@ -20,33 +21,16 @@ class ChildRefTable extends React.Component {
     this.closeModal = this.closeModal.bind(this);
     this.fetchData = this.fetchData.bind(this);
   }
-  componentDidMount() {
-    this.fetchData(this.props.row);
-  }
-  fetchData(row) {
-    fetch(`${REROUTE_URL}`, {
-      method: 'POST',
-      headers: {
-        'Content-Type': 'application/json',
-        'Authorization': sessionStorage.getItem('accessToken'),
-      },
-      body: JSON.stringify({
-        method: 'GET',
-        endpoint: `${API_VERSION}/${BUSINESS_ENDPOINT}/${row.original.id}`,
-      }),
-    })
-    .then(response => {
-      if (response.ok) {
-        return response.json();
-      }
-      throw new Error(`Error: ${response.status} ${response.statusText}`);
-    })
-    .then(data => this.setState({ data: formatData(data), isLoading: false }))
-    .catch(error => this.setState({ errorMessage: error.message, error: true, isLoading: false }));
-  }
-  closeModal() {
-    this.setState({ error: false, errorMessage: '' });
+  componentDidMount = () => this.fetchData(this.props.row);
+  fetchData = (row) => {
+    accessAPI(REROUTE_URL, 'POST', sessionStorage.accessToken, JSON.stringify({
+      method: 'GET',
+      endpoint: `${API_VERSION}/${BUSINESS_ENDPOINT}/${row.original.id}`,
+    }))
+    .then(json => this.setState({ data: formatData(json), isLoading: false }))
+    .catch(() => this.setState({ errorMessage: 'Error: Unable to get child references.', error: true, isLoading: false }));
   }
+  closeModal = () => this.setState({ error: false, errorMessage: '' });
   render() {
     const business = this.props.row.original;
     const description = (industryCodeDescription[business.industryCode] === undefined)

diff --git a/src/components/ErrorModal.js b/src/components/ErrorModal.js
@@ -2,28 +2,20 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import { ModalContainer, ModalDialog } from 'react-modal-dialog';
 
-class ErrorModal extends React.Component {
-  componentWillMount() {
-    window.addEventListener('keydown', this.props.close);
-  }
-  componentWillUnmount() {
-    window.removeEventListener('keydown', this.props.close);
-  }
-  render() {
-    const modal = (this.props.show) ? (<ModalContainer onClose={this.props.close()}>
-      <ModalDialog style={{ width: '80%' }} onClose={this.props.close()}>
-        <h1 style={{ margin: '10px' }}>{this.props.message}</h1>
-        <br />
-        <button className="btn btn--primary" autoFocus onClick={this.props.close()}>Close</button>
-      </ModalDialog>
-    </ModalContainer>) : <div></div>;
-    return (
-      <div>
-        {modal}
-      </div>
-    );
-  }
-}
+const ErrorModal = ({ show, message, close }) => {
+  const modal = (show) ? (<ModalContainer onClose={() => close()}>
+    <ModalDialog style={{ width: '80%' }} onClose={() => close()}>
+      <h1 style={{ margin: '10px' }}>{message}</h1>
+      <br />
+      <button className="btn btn--primary" autoFocus onClick={() => close()}>Close</button>
+    </ModalDialog>
+  </ModalContainer>) : <div></div>;
+  return (
+    <div>
+      {modal}
+    </div>
+  );
+};
 
 ErrorModal.propTypes = {
   show: PropTypes.bool.isRequired,

diff --git a/src/components/SearchHOC.js b/src/components/SearchHOC.js
@@ -85,7 +85,7 @@ export default function withSearch(Form, settings, actions, formQuery) {
       document.getElementsByClassName('wrapper')[0].scrollIntoView(false);
       this.child.childTextInput.myInput.focus();
     }
-    closeModal() {
+    closeModal = () => {
       this.setState({ show: false, errorMessage: '' });
       this.focusAndScroll();
     }
@@ -150,7 +150,7 @@ export default function withSearch(Form, settings, actions, formQuery) {
               <ErrorModal
                 show={this.state.show}
                 message={this.state.errorMessage}
-                close={() => this.closeModal}
+                close={this.closeModal}
               />
               <br />
               {this.props.data.results.length !== 0 &&

diff --git a/src/utils/accessAPI.js b/src/utils/accessAPI.js
@@ -11,22 +11,28 @@
  * @return {Function}
  *
  */
-const accessAPI = (url, method, auth, body, callback) => {
-  return fetch(url, {
-    method,
-    headers: {
-      'Content-Type': 'application/json',
-      Authorization: auth,
-    },
-    body,
-  }).then((response) => {
-    switch (response.status) {
-      case 200: return response.json().then((json) => callback(true, { json }));
-      case 401: return callback(false, { message: 'Authentication problem. Please ensure you are logged in.' });
-      case 500: return callback(false, { message: 'Server error. Please contact your system administrator.' });
-      default: return callback(false, { message: `${response.status} error.` });
-    }
-  }).catch((err) => callback(false, { message: `Server error: request timed out. ${err}` }));
+const accessAPI = (url, method, auth, body) => {
+  return new Promise((resolve, reject) => {
+    fetch(url, {
+      method,
+      headers: {
+        'Content-Type': 'application/json',
+        Authorization: auth,
+      },
+      body,
+    }).then((response) => {
+      // We don't need to return the promise below, but it gets rid of an ESLint error
+      // relating to not using a 'break' after each case
+      switch (response.status) {
+        case 200: return resolve(response.json());
+        case 400: return reject('Malformed query. Please review your input parameters.');
+        case 404: return reject('Not found. Please review your input parameters.');
+        case 401: return reject('Authentication problem. Please ensure you are logged in.');
+        case 500: return reject('Server error. Please contact your system administrator.');
+        default: return reject(`${response.status} error.`);
+      }
+    }).catch((err) => reject(`Server error: request timed out. ${err}`));
+  });
 };
 
 export default accessAPI;
diff --git a/src/views/Login.js b/src/views/Login.js
@@ -37,7 +37,7 @@ class Login extends React.Component {
               <input type="text" placeholder="username" ref={(ref) => (this.usernameInput = ref)} />
               <input type="password" placeholder="password" ref={(ref) => (this.passwordInput = ref)} />
               <Button id="loginButton" size="wide" text="Login" onClick={!this.props.data.currentlySending ? this.onSubmit : null} ariaLabel="Login Button" type="submit" loading={this.props.data.currentlySending} />
-              <ErrorModal show={this.state.show && this.props.data.errorMessage !== ''} message={this.props.data.errorMessage} close={() => this.closeModal} />
+              <ErrorModal show={this.state.show && this.props.data.errorMessage !== ''} message={this.props.data.errorMessage} close={this.closeModal} />
             </form>
           </div>
         </div>