Merge pull request #1927 from ProjectMirador/1873-load-window-error

Add an error message when add manifest request fails

Merge pull request #1927 from ProjectMirador/1873-load-window-error
06613178 · Jack Reed · GitHub · 15612390 · 199eed3e · 06613178
Unverified Commit 06613178 authored 6 years ago by Jack Reed Committed by GitHub 6 years ago
--- a/__tests__/fixtures/version-2/broken.json
+++ b/__tests__/fixtures/version-2/broken.json
+<html>
+  <title>Error</title>
+  <body>
+    <p>Something went wrong</p>
+  </body>
+</html>
--- a/__tests__/integration/mirador/invalid-api-response.test.js
+++ b/__tests__/integration/mirador/invalid-api-response.test.js
 describe('Mirador Invalid API Response Handler Test', () => {
-  beforeAll(async () => {
+  beforeEach(async () => {
    await page.goto('http://127.0.0.1:4488/__tests__/integration/mirador/');
  });
  it('breaks Mirador', async () => {
@@ -14,4 +14,25 @@ describe('Mirador Invalid API Response Handler Test', () => {
      expect(e.name).toMatch('TimeoutError');
    }
  });
+  it('renders an error message when a manifest cannot be loaded (and allows it to be dismissed)', async () => {
+    await expect(page).toClick('#addBtn');
+    await expect(page).toFill('#manifestURL', 'http://localhost:5000/api/broken');
+    await expect(page).toClick('#fetchBtn');
+    await page.waitFor(1000);
+    await expect(page).toMatchElement(
+      'p', { text: 'The resource cannot be added:' },
+    );
+    await expect(page).toMatchElement(
+      'p', { text: 'http://localhost:5000/api/broken' },
+    );
+    await expect(page).toClick('button', { text: 'Dismiss' });
+    await expect(page).not.toMatchElement(
+      'p',
+      { text: 'The resource http://localhost:5000/api/broken cannot be added.' },
+    );
+  });
 });
--- a/__tests__/src/components/ManifestListItem.test.js
+++ b/__tests__/src/components/ManifestListItem.test.js
 import React from 'react';
 import { shallow } from 'enzyme';
 import ManifestListItem from '../../../src/components/ManifestListItem';
+import ManifestListItemError from '../../../src/containers/ManifestListItemError';
 /** */
 function createWrapper(props) {
@@ -34,7 +35,7 @@ describe('ManifestListItem', () => {
    const wrapper = createWrapper({ error: 'This is an error message' });
    expect(wrapper.find('WithStyles(Paper)').length).toBe(1);
-    expect(wrapper.find('WithStyles(Paper)').children().text()).toEqual('This is an error message');
+    expect(wrapper.find(ManifestListItemError).length).toBe(1);
  });
  it('updates and adds window when button clicked', () => {
    const addWindow = jest.fn();

--- a/__tests__/src/components/ManifestListItemError.test.js
+++ b/__tests__/src/components/ManifestListItemError.test.js
+import React from 'react';
+import { shallow } from 'enzyme';
+import Typography from '@material-ui/core/Typography';
+import ManifestListItemError from '../../../src/components/ManifestListItemError';
+/**
+ * Helper function to wrap creating a ManifestListItemError component
+*/
+function createWrapper(props) {
+  return shallow(
+    <ManifestListItemError
+      manifestId="http://example.com"
+      onDismissClick={() => {}}
+      onTryAgainClick={() => {}}
+      t={key => key}
+      {...props}
+    />,
+  ).dive(); // unwrap HOC created by withStyles()
+}
+describe('ManifestListItemError', () => {
+  let wrapper;
+  let mockFn;
+  it('renders the failed manifest url and error key', () => {
+    wrapper = createWrapper();
+    expect(
+      wrapper.find(Typography).children().first().text(),
+    ).toEqual('manifestError'); // the i18n key
+    expect(
+      wrapper.find(Typography).children().last().text(),
+    ).toEqual('http://example.com');
+  });
+  it('has a dismiss button that fires the onDismissClick prop', () => {
+    mockFn = jest.fn();
+    wrapper = createWrapper({ onDismissClick: mockFn });
+    wrapper.find('WithStyles(Button)').first().simulate('click');
+    expect(mockFn).toHaveBeenCalledTimes(1);
+    expect(mockFn).toHaveBeenCalledWith('http://example.com');
+  });
+  it('has a try again button that fires the onTryAgainClick prop', () => {
+    mockFn = jest.fn();
+    wrapper = createWrapper({ onTryAgainClick: mockFn });
+    wrapper.find('WithStyles(Button)').last().simulate('click');
+    expect(mockFn).toHaveBeenCalledTimes(1);
+    expect(mockFn).toHaveBeenCalledWith('http://example.com');
+  });
+});
--- a/__tests__/src/reducers/manifests.test.js
+++ b/__tests__/src/reducers/manifests.test.js
@@ -18,6 +18,7 @@ describe('manifests reducer', () => {
        abc123: {
          id: 'abc123',
          isFetching: true,
+          error: 'Error fetching manifest',
        },
      },
      {
@@ -34,9 +35,11 @@ describe('manifests reducer', () => {
        id: 'abc123',
        isFetching: false,
        manifestation: {},
+        error: null,
      },
    });
  });
  it('should handle RECEIVE_MANIFEST_FAILURE', () => {
    expect(manifestsReducer(
      {

--- a/locales/en/translation.json
+++ b/locales/en/translation.json
@@ -16,12 +16,14 @@
    "closeMenu": "Close Menu",
    "closeWindow": "Close window",
    "dark": "Dark",
+    "dismiss": "Dismiss",
    "downloadExport": "Download/Export",
    "downloadExportWorkspace": "Download/export workspace",
    "fetchManifest": "Add",
    "fullScreen": "Full Screen",
    "light": "Light",
    "listAllOpenWindows": "List all open windows",
+    "manifestError": "The resource cannot be added:",
    "menu": "Menu",
    "off": "Off",
    "openInfoCompanionWindow": "Open information companion window",
@@ -35,6 +37,7 @@
    "theme": "Theme",
    "thumbnails": "Thumbnails",
    "toggleWindowSideBar": "Toggle window sidebar",
+    "tryAgain": "Try again",
    "untitled": "[Untitled]",
    "view": "View",
    "zoomIn": "Zoom in",

--- a/src/components/ManifestListItem.js
+++ b/src/components/ManifestListItem.js
@@ -7,6 +7,7 @@ import Grid from '@material-ui/core/Grid';
 import Typography from '@material-ui/core/Typography';
 import ReactPlaceholder from 'react-placeholder';
 import { TextBlock, TextRow, RectShape } from 'react-placeholder/lib/placeholders';
+import ManifestListItemError from '../containers/ManifestListItemError';
 import WindowIcon from './WindowIcon';
 import ns from '../config/css-ns';
 import 'react-placeholder/lib/reactPlaceholder.css';
@@ -71,7 +72,7 @@ class ManifestListItem extends React.Component {
    if (error) {
      return (
        <Paper elevation={1} className={classes.root} data-manifestid={manifestId}>
-          {error}
+          <ManifestListItemError manifestId={manifestId} />
        </Paper>
      );
    }

--- a/src/components/ManifestListItemError.js
+++ b/src/components/ManifestListItemError.js
+import React, { Component } from 'react';
+import PropTypes from 'prop-types';
+import Button from '@material-ui/core/Button';
+import ErrorIcon from '@material-ui/icons/ErrorOutline';
+import Grid from '@material-ui/core/Grid';
+import Typography from '@material-ui/core/Typography';
+import { withStyles } from '@material-ui/core/styles';
+/**
+ * ManifestListItemError renders a component displaying a
+ * message to the user about a problem loading a manifest
+*/
+class ManifestListItemError extends Component {
+  /**
+   * Returns the rendered component
+  */
+  render() {
+    const {
+      classes, manifestId, onDismissClick, onTryAgainClick, t,
+    } = this.props;
+    return (
+      <Grid container>
+        <Grid container>
+          <Grid container item xs={12} sm={6}>
+            <Grid item xs={4} sm={3}>
+              <Grid container justify="center">
+                <ErrorIcon className={classes.errorIcon} />
+              </Grid>
+            </Grid>
+            <Grid item xs={8} sm={9}>
+              <Typography>{t('manifestError')}</Typography>
+              <Typography className={classes.manifestIdText}>{manifestId}</Typography>
+            </Grid>
+          </Grid>
+        </Grid>
+        <Grid container>
+          <Grid container item xs={12} sm={6} justify="flex-end">
+            <Grid item>
+              <Button onClick={() => { onDismissClick(manifestId); }}>
+                {t('dismiss')}
+              </Button>
+              <Button onClick={() => { onTryAgainClick(manifestId); }}>
+                {t('tryAgain')}
+              </Button>
+            </Grid>
+          </Grid>
+        </Grid>
+      </Grid>
+    );
+  }
+}
+ManifestListItemError.propTypes = {
+  classes: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
+  manifestId: PropTypes.string.isRequired,
+  onDismissClick: PropTypes.func.isRequired,
+  onTryAgainClick: PropTypes.func.isRequired,
+  t: PropTypes.func.isRequired,
+};
+/**
+ Material UI styles
+ @private
+ */
+const styles = theme => ({
+  errorIcon: {
+    color: theme.palette.error.main,
+    height: '2rem',
+    width: '2rem',
+  },
+  manifestIdText: {
+    wordBreak: 'break-all',
+  },
+});
+export default withStyles(styles)(ManifestListItemError);
--- a/src/containers/ManifestListItemError.js
+++ b/src/containers/ManifestListItemError.js
+import { compose } from 'redux';
+import { connect } from 'react-redux';
+import { withNamespaces } from 'react-i18next';
+import { fetchManifest, removeManifest } from '../state/actions/manifest';
+import ManifestListItemError from '../components/ManifestListItemError';
+/** */
+const mapDispatchToProps = {
+  onDismissClick: removeManifest,
+  onTryAgainClick: fetchManifest,
+};
+const enhance = compose(
+  connect(null, mapDispatchToProps),
+  withNamespaces(),
+  // further HOC
+);
+export default enhance(ManifestListItemError);
--- a/src/state/actions/manifest.js
+++ b/src/state/actions/manifest.js
@@ -58,7 +58,13 @@ export function fetchManifest(manifestId, properties) {
    return fetch(manifestId)
      .then(response => response.json())
      .then(json => dispatch(receiveManifest(manifestId, json)))
-      .catch(error => dispatch(receiveManifestFailure(manifestId, error)));
+      .catch((error) => {
+        if (typeof error === 'object') { // Returned by JSON parse failure
+          dispatch(receiveManifestFailure(manifestId, String(error)));
+        } else {
+          dispatch(receiveManifestFailure(manifestId, error));
+        }
+      });
  });
 }

--- a/src/state/reducers/manifests.js
+++ b/src/state/reducers/manifests.js
@@ -23,6 +23,7 @@ export const manifestsReducer = (state = {}, action) => {
          id: action.manifestId,
          manifestation: manifesto.create(action.manifestJson),
          isFetching: false,
+          error: null, // Explicitly set the error to null in case this is a re-fetch
        },
      };
    case ActionTypes.RECEIVE_MANIFEST_FAILURE: