From ef16c687d02a8733a6a1768198a1879793fcd64a Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 20 Jan 2020 16:13:08 +0530 Subject: [PATCH] fix: provide useful error messages (DHIS2-5029) (#552) --- packages/app/i18n/en.pot | 61 ++++---- packages/app/src/assets/ErrorIcons.js | 78 ++++++++++ .../UpdateVisualizationContainer.js | 11 +- .../components/Visualization/StartScreen.js | 29 +++- .../components/Visualization/Visualization.js | 31 +++- .../__tests__/Visualization.spec.js | 13 +- .../Visualization/styles/StartScreen.style.js | 27 +++- packages/app/src/modules/error.js | 141 ++++++++++++++++++ packages/app/src/modules/layoutValidation.js | 107 +++---------- 9 files changed, 349 insertions(+), 149 deletions(-) create mode 100644 packages/app/src/assets/ErrorIcons.js diff --git a/packages/app/i18n/en.pot b/packages/app/i18n/en.pot index 03fbd6b05d..cd8c0f6840 100644 --- a/packages/app/i18n/en.pot +++ b/packages/app/i18n/en.pot @@ -5,8 +5,8 @@ msgstr "" "Content-Type: text/plain; charset=utf-8\n" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=2; plural=(n != 1)\n" -"POT-Creation-Date: 2020-01-16T07:40:30.667Z\n" -"PO-Revision-Date: 2020-01-16T07:40:30.667Z\n" +"POT-Creation-Date: 2020-01-20T08:17:07.464Z\n" +"PO-Revision-Date: 2020-01-20T08:17:07.464Z\n" msgid "Rename successful" msgstr "" @@ -122,15 +122,6 @@ msgstr "" msgid "Viewing interpretation from {{interpretationDate}}" msgstr "" -msgid "Error validating layout" -msgstr "" - -msgid "Visualization error" -msgstr "" - -msgid "Error generating chart, please try again" -msgstr "" - msgid "Aggregation type" msgstr "" @@ -445,55 +436,69 @@ msgstr "" msgid "Expected reports" msgstr "" -msgid "Add to series" +msgid "No data available" msgstr "" -msgid "Add to category" +msgid "" +"The selected dimensions didn’t return any data. There may be no data, or\n" +" you may not have access to it." msgstr "" -msgid "Add to filter" +msgid "Series is empty" msgstr "" -msgid "series" +msgid "Add at least one item to Series." msgstr "" -msgid "category" +msgid "Category is empty" msgstr "" -msgid "filter" +msgid "Add at least one item to Category." msgstr "" -msgid "Please add at least one {{series}} dimension" +msgid "No period set" msgstr "" -msgid "Please add at least one {{category}} dimension" +msgid "{{visType}} must have at least one period set in {{axes}}." msgstr "" -msgid "Please add at least one period as {{series}}, {{category}} or {{filter}}" +msgid "No data set" msgstr "" -msgid "Please add {{data}} as {{category}} or {{filter}}" +msgid "{{visType}} must have at least one data item in {{axes}}." msgstr "" -msgid "Please add at least one period as {{series}} or {{filter}}" +msgid "There is a problem with this {{visType}} visualization." msgstr "" -msgid "Please add at least one {{filter}} dimension" +msgid "There was a problem getting the data from the server." msgstr "" -msgid "Please add at least one period as a {{series}} dimension" +msgid "Assigned Categories can only be used with data elements" msgstr "" -msgid "Please add at least one period as a {{category}} dimension" +msgid "Fix this problem by selecting data elements or removing Assigned Categories." msgstr "" -msgid "Please add {{data}} as a filter dimension" +msgid "or" msgstr "" -msgid "Please add one {{series}} dimension" +msgid "Add to series" msgstr "" -msgid "Please add at least one period as {{filter}}" +msgid "Add to category" +msgstr "" + +msgid "Add to filter" +msgstr "" + +msgid "series" +msgstr "" + +msgid "category" +msgstr "" + +msgid "filter" msgstr "" msgid "Today" diff --git a/packages/app/src/assets/ErrorIcons.js b/packages/app/src/assets/ErrorIcons.js new file mode 100644 index 0000000000..415c83f548 --- /dev/null +++ b/packages/app/src/assets/ErrorIcons.js @@ -0,0 +1,78 @@ +import React from 'react' + +export const EmptyBox = () => ( + + + +) + +export const PeriodError = () => ( + + + + + + + + +) + +export const DataError = () => ( + + + + + + + + +) + +export const EmptySeries = () => ( + + + + + + +) + +export const EmptyCategory = () => ( + + + + + + +) + +export const GenericError = () => ( + + + + + + + +) diff --git a/packages/app/src/components/UpdateButton/UpdateVisualizationContainer.js b/packages/app/src/components/UpdateButton/UpdateVisualizationContainer.js index d5c7b7743b..b379d3d88e 100644 --- a/packages/app/src/components/UpdateButton/UpdateVisualizationContainer.js +++ b/packages/app/src/components/UpdateButton/UpdateVisualizationContainer.js @@ -1,5 +1,4 @@ import { connect } from 'react-redux' -import i18n from '@dhis2/d2-i18n' import { sGetCurrent, sGetCurrentFromUi } from '../../reducers/current' import * as fromActions from '../../actions' @@ -7,6 +6,7 @@ import { validateLayout } from '../../modules/layoutValidation' import { acSetLoadError, acClearLoadError } from '../../actions/loader' import history from '../../modules/history' import { CURRENT_AO_KEY } from '../../api/userDataStore' +import { GenericClientError } from '../../modules/error' const UpdateVisualizationContainer = ({ renderComponent, @@ -22,14 +22,9 @@ const UpdateVisualizationContainer = ({ const onClick = () => { try { validateLayout(getCurrentFromUi()) - acClearLoadError() - } catch (err) { - acSetLoadError( - err && err.message - ? err.message - : i18n.t('Error validating layout') - ) + } catch (error) { + acSetLoadError(error || new GenericClientError()) } onUpdate() diff --git a/packages/app/src/components/Visualization/StartScreen.js b/packages/app/src/components/Visualization/StartScreen.js index d193f4964e..669b94cefb 100644 --- a/packages/app/src/components/Visualization/StartScreen.js +++ b/packages/app/src/components/Visualization/StartScreen.js @@ -4,11 +4,12 @@ import i18n from '@dhis2/d2-i18n' import styles from './styles/StartScreen.style' import { sGetLoadError } from '../../reducers/loader' import PropTypes from 'prop-types' -import visualizationErrorImg from '../../assets/chart-error-graphic.png' import { apiFetchMostViewedVisualizations } from '../../api/mostViewedVisualizations' import history from '../../modules/history' import { withStyles } from '@material-ui/core/styles' import { useDataEngine } from '@dhis2/app-runtime' +import { VisualizationError } from '../../modules/error' +import { GenericError } from '../../assets/ErrorIcons' const StartScreen = ({ error, classes }) => { const [mostViewedVisualizations, setMostViewedVisualizations] = useState([]) @@ -25,13 +26,7 @@ const StartScreen = ({ error, classes }) => { const getContent = () => error ? ( -
- {i18n.t('Visualization -

{error}

-
+ getErrorContent() ) : (
@@ -64,6 +59,24 @@ const StartScreen = ({ error, classes }) => {
) + const getErrorContent = () => { + return error instanceof VisualizationError ? ( +
+
{error.icon()}
+

{error.title}

+

{error.description}

+
+ ) : ( +
+
{GenericError()}
+

+ {i18n.t('Something went wrong')} +

+

{error.message || error}

+
+ ) + } + return (
{getContent()}
diff --git a/packages/app/src/components/Visualization/Visualization.js b/packages/app/src/components/Visualization/Visualization.js index 6bb42d873c..27e69f1129 100644 --- a/packages/app/src/components/Visualization/Visualization.js +++ b/packages/app/src/components/Visualization/Visualization.js @@ -2,7 +2,6 @@ import React, { Component } from 'react' import PropTypes from 'prop-types' import { connect } from 'react-redux' import { createSelector } from 'reselect' -import i18n from '@dhis2/d2-i18n' import debounce from 'lodash-es/debounce' import styles from './styles/Visualization.style' @@ -17,6 +16,11 @@ import { acSetChart } from '../../actions/chart' import { acSetLoadError } from '../../actions/loader' import StartScreen from './StartScreen' +import { + AssignedCategoriesError, + GenericServerError, + EmptyResponseError, +} from '../../modules/error' export class Visualization extends Component { constructor(props) { @@ -27,10 +31,20 @@ export class Visualization extends Component { } } - onError = err => { - const error = - (err && err.message) || - i18n.t('Error generating chart, please try again') + onError = response => { + let error + if (response) { + if ( + response.message === + 'Assigned categories can only be specified together with data elements, not indicators or reporting rates' + ) { + error = new AssignedCategoriesError() + } else { + error = response + } + } else { + error = new GenericServerError() + } this.props.acSetLoadError(error) } @@ -40,7 +54,10 @@ export class Visualization extends Component { onResponsesReceived = responses => { const forMetadata = {} - responses.forEach(response => + responses.forEach(response => { + if (!response.rows || !response.rows.length) { + throw new EmptyResponseError() + } Object.entries(response.metaData.items).forEach(([id, item]) => { forMetadata[id] = { id, @@ -48,7 +65,7 @@ export class Visualization extends Component { displayName: item.displayName, } }) - ) + }) this.props.acAddMetadata(forMetadata) } diff --git a/packages/app/src/components/Visualization/__tests__/Visualization.spec.js b/packages/app/src/components/Visualization/__tests__/Visualization.spec.js index 9cf76e9d09..ad0de18adc 100644 --- a/packages/app/src/components/Visualization/__tests__/Visualization.spec.js +++ b/packages/app/src/components/Visualization/__tests__/Visualization.spec.js @@ -53,7 +53,9 @@ describe('Visualization', () => { c: { id: 'c', name: 'c' }, } - vis().simulate('responsesReceived', [{ metaData: { items } }]) + vis().simulate('responsesReceived', [ + { metaData: { items }, rows: [1, 2, 3] }, + ]) expect(props.acAddMetadata).toHaveBeenCalled() expect(props.acAddMetadata).toHaveBeenCalledWith(items) @@ -68,15 +70,6 @@ describe('Visualization', () => { expect(props.acSetChart).toHaveBeenCalledWith(svg) }) - it('triggers setLoadError when error received from chart plugin', () => { - const errorMsg = 'catastrophic error' - - vis().simulate('error', { message: errorMsg }) - - expect(props.acSetLoadError).toHaveBeenCalled() - expect(props.acSetLoadError).toHaveBeenCalledWith(errorMsg) - }) - it('renders visualization with new id when rightSidebarOpen prop changes', () => { const wrapper = vis() diff --git a/packages/app/src/components/Visualization/styles/StartScreen.style.js b/packages/app/src/components/Visualization/styles/StartScreen.style.js index 690a6744ea..c668b5a1ea 100644 --- a/packages/app/src/components/Visualization/styles/StartScreen.style.js +++ b/packages/app/src/components/Visualization/styles/StartScreen.style.js @@ -12,19 +12,36 @@ export default { boxSizing: 'border-box', color: colors.grey900, position: 'relative', + display: 'flex', + alignItems: 'center', }, errorContainer: { display: 'flex', flexDirection: 'column', alignItems: 'center', - marginTop: '20px', + textAlign: 'center', + }, + errorIcon: { + width: '136px', + height: '136px', + margin: '0 auto 24px', }, errorTitle: { - fontWeight: 'bold', - marginTop: '30px', - lineHeight: '22px', + fontWeight: '500', + fontSize: '20px', + color: colors.grey800, + letterSpacing: '0.15px', + lineHeight: '24px', + width: '360px', + margin: '0 auto 12px', + }, + errorDescription: { + fontWeight: '400', + fontSize: '14px', color: colors.grey700, - textAlign: 'center', + lineHeight: '19px', + width: '360px', + margin: '0 auto', }, title: { fontWeight: 'bold', diff --git a/packages/app/src/modules/error.js b/packages/app/src/modules/error.js index 5671c2f2d7..9ab018f788 100644 --- a/packages/app/src/modules/error.js +++ b/packages/app/src/modules/error.js @@ -1,3 +1,144 @@ +import i18n from '@dhis2/d2-i18n' +import { + getDisplayNameByVisType, + getAvailableAxes, + getAxisName, + getAxisPerLockedDimension, + DIMENSION_ID_DATA, +} from '@dhis2/analytics' + +import { + EmptySeries, + EmptyCategory, + PeriodError, + DataError, + GenericError, + EmptyBox, +} from '../assets/ErrorIcons' + +export class VisualizationError { + constructor(icon, title, description) { + this.icon = icon + this.title = title + this.description = description + } +} + +export class EmptyResponseError extends VisualizationError { + constructor() { + super( + EmptyBox, + i18n.t('No data available'), + i18n.t( + 'The selected dimensions didn’t return any data. There may be no data, or you may not have access to it.' + ) + ) + } +} + +export class NoSeriesError extends VisualizationError { + constructor() { + super( + EmptySeries, + i18n.t('Series is empty'), + i18n.t('Add at least one item to Series.') + ) + } +} + +export class NoCategoryError extends VisualizationError { + constructor() { + super( + EmptyCategory, + i18n.t('Category is empty'), + i18n.t('Add at least one item to Category.') + ) + } +} + +export class NoPeriodError extends VisualizationError { + constructor(visType) { + super( + PeriodError, + i18n.t('No period set'), + i18n.t( + '{{visType}} must have at least one period set in {{axes}}.', + { + visType: getDisplayNameByVisType(visType), + axes: getAvailableAxesDescription(visType), + } + ) + ) + } +} + +export class NoDataError extends VisualizationError { + constructor(visType) { + const lockedAxis = getAxisPerLockedDimension(visType, DIMENSION_ID_DATA) + super( + DataError, + i18n.t('No data set'), + i18n.t( + '{{visType}} must have at least one data item in {{axes}}.', + { + visType: getDisplayNameByVisType(visType), + axes: lockedAxis + ? getAxisName(lockedAxis) + : getAvailableAxesDescription(visType), + } + ) + ) + } +} + +export class GenericClientError extends VisualizationError { + constructor(visType) { + super( + GenericError, + i18n.t('Something went wrong'), + i18n.t('There is a problem with this {{visType}} visualization.', { + visType: getDisplayNameByVisType(visType), + }) + ) + } +} + +export class GenericServerError extends VisualizationError { + constructor() { + super( + GenericError, + i18n.t('Something went wrong'), + i18n.t('There was a problem getting the data from the server.') + ) + } +} + +export class AssignedCategoriesError extends VisualizationError { + constructor() { + super( + GenericError, + i18n.t('Assigned Categories can only be used with data elements'), + i18n.t( + 'Fix this problem by selecting data elements or removing Assigned Categories.' + ) + ) + } +} + +const getAvailableAxesDescription = visType => { + const axes = getAvailableAxes(visType) + let axesDescription = '' + for (let index = 0; index < axes.length; index++) { + axesDescription += getAxisName(axes[index]) + if (index < axes.length - 2) { + axesDescription += ', ' + } else if (index < axes.length - 1) { + axesDescription += ` ${i18n.t('or')} ` + } + } + return axesDescription +} + export const parseError = ({ message, httpStatusCode }) => ({ message, type: String(httpStatusCode).match(/50\d/) ? 'error' : 'warning', diff --git a/packages/app/src/modules/layoutValidation.js b/packages/app/src/modules/layoutValidation.js index 405c8b5387..6e080bf8b1 100644 --- a/packages/app/src/modules/layoutValidation.js +++ b/packages/app/src/modules/layoutValidation.js @@ -1,10 +1,5 @@ -import i18n from '@dhis2/d2-i18n' import { AXIS, - AXIS_ID_COLUMNS, - AXIS_ID_ROWS, - AXIS_ID_FILTERS, - DIMENSION_ID_DATA, DIMENSION_ID_PERIOD, VIS_TYPE_YEAR_OVER_YEAR_LINE, VIS_TYPE_YEAR_OVER_YEAR_COLUMN, @@ -14,71 +9,16 @@ import { getFixedDimensionProp, dimensionIsValid, layoutGetDimension, - getAxisName, + DIMENSION_ID_DATA, } from '@dhis2/analytics' import { BASE_FIELD_YEARLY_SERIES } from './fields/baseFields' - -const dxName = getFixedDimensionProp(DIMENSION_ID_DATA, name) - -const errorLabels = { - defaultSeries: i18n.t('Please add at least one {{series}} dimension', { - series: getAxisName(AXIS_ID_COLUMNS), - }), - defaultCategory: i18n.t('Please add at least one {{category}} dimension', { - category: getAxisName(AXIS_ID_ROWS), - }), - defaultPe: i18n.t( - 'Please add at least one period as {{series}}, {{category}} or {{filter}}', - { - series: getAxisName(AXIS_ID_COLUMNS), - category: getAxisName(AXIS_ID_ROWS), - filter: getAxisName(AXIS_ID_FILTERS), - } - ), - pie: { - dx: i18n.t('Please add {{data}} as {{category}} or {{filter}}', { - data: dxName, - category: getAxisName(AXIS_ID_ROWS), - filter: getAxisName(AXIS_ID_FILTERS), - }), - pe: i18n.t( - 'Please add at least one period as {{series}} or {{filter}}', - { - series: getAxisName(AXIS_ID_COLUMNS), - filter: getAxisName(AXIS_ID_FILTERS), - } - ), - filter: i18n.t('Please add at least one {{filter}} dimension', { - filter: getAxisName(AXIS_ID_FILTERS), - }), - }, - yearOverYear: { - seriesPeriod: i18n.t( - 'Please add at least one period as a {{series}} dimension', - { - series: getAxisName(AXIS_ID_COLUMNS), - } - ), - categoryPeriod: i18n.t( - 'Please add at least one period as a {{category}} dimension', - { - category: getAxisName(AXIS_ID_ROWS), - } - ), - dx: i18n.t('Please add {{data}} as a filter dimension', { - data: dxName, - }), - }, - singleValue: { - dx: i18n.t('Please add one {{series}} dimension', { - series: getAxisName(AXIS_ID_COLUMNS), - }), - pe: i18n.t('Please add at least one period as {{filter}}', { - filter: getAxisName(AXIS_ID_FILTERS), - }), - }, -} +import { + NoSeriesError, + NoCategoryError, + NoPeriodError, + NoDataError, +} from './error' // Layout validation helper functions const isAxisValid = axis => @@ -89,25 +29,25 @@ const isAxisValid = axis => }) ) -const validateDimension = (dimension, message) => { +const validateDimension = (dimension, error) => { if (!(dimension && dimensionIsValid(dimension, { requireItems: true }))) { - throw new Error(message) + throw error } } -const validateAxis = (axis, message) => { +const validateAxis = (axis, error) => { if (!isAxisValid(axis)) { - throw new Error(message) + throw error } } // Layout validation const validateDefaultLayout = layout => { - validateAxis(layout.columns, errorLabels.defaultSeries) - validateAxis(layout.rows, errorLabels.defaultCategory) + validateAxis(layout.columns, new NoSeriesError()) + validateAxis(layout.rows, new NoCategoryError()) validateDimension( layoutGetDimension(layout, DIMENSION_ID_PERIOD), // TODO: old validation rule, refactor - errorLabels.defaultPe + new NoPeriodError(layout.type) ) } @@ -118,34 +58,35 @@ const validateYearOverYearLayout = layout => { typeof layout[BASE_FIELD_YEARLY_SERIES][0] === 'string' ) ) { - throw new Error(errorLabels.yearOverYear.seriesPeriod) + throw new NoSeriesError() } - validateAxis(layout.rows, errorLabels.yearOverYear.categoryPeriod) - - validateAxis(layout.columns, errorLabels.yearOverYear.dx) + validateAxis(layout.rows, new NoCategoryError()) } const validatePieLayout = layout => { - validateAxis(layout.columns, errorLabels.defaultSeries) - validateAxis(layout.filters, errorLabels.pie.filter) + validateAxis(layout.columns, new NoSeriesError()) validateDimension( layoutGetDimension(layout, DIMENSION_ID_PERIOD), - errorLabels.pie.pe + new NoPeriodError(layout.type) ) } const validateSingleValueLayout = layout => { - validateAxis(layout.columns, errorLabels.singleValue.dx) validateDimension( layoutGetDimension(layout, DIMENSION_ID_PERIOD), - errorLabels.singleValue.pe + new NoPeriodError(layout.type) ) } // TODO: Add validatePivotLayout export const validateLayout = layout => { + validateDimension( + layoutGetDimension(layout, DIMENSION_ID_DATA), + new NoDataError(layout.type) + ) + switch (layout.type) { case VIS_TYPE_PIE: return validatePieLayout(layout)