Skip to content

Commit

Permalink
Fix error handling when switching viz type (#317)
Browse files Browse the repository at this point in the history 
Layout validation is run when the update
button is clicked.
This should avoid to load the plugin when there is an invalid layout
which will likely cause an error due to missing expected data/config.

Also, in the reducers we cannot always assume the layout has all the
required dimensions/items, ie. when starting a viz from scratch and
switch viz type or click the update button before selecting some
required dimension item.
  • Loading branch information
@edoardo
edoardo committed Sep 23, 2019
1 parent e5d5893 commit bf494b3
Show file tree
Hide file tree
Showing 11 changed files with 77 additions and 87 deletions.
7 changes: 5 additions & 2 deletions packages/app/i18n/en.pot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: 2019-08-08T13:09:25.099Z\n"
"PO-Revision-Date: 2019-08-08T13:09:25.099Z\n"
"POT-Creation-Date: 2019-09-16T07:28:39.022Z\n"
"PO-Revision-Date: 2019-09-16T07:28:39.022Z\n"

msgid "Rename successful"
msgstr ""
Expand Down Expand Up @@ -129,6 +129,9 @@ msgstr ""
msgid "Viewing interpretation from {{interpretationDate}}"
msgstr ""

msgid "Error validating layout"
msgstr ""

msgid "Create a new visualization by adding dimensions to the layout"
msgstr ""

Expand Down
4 changes: 2 additions & 2 deletions packages/app/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@
"webpack-bundle-analyzer": "^3.0.3"
},
"dependencies": {
"@dhis2/analytics": "2.3.4",
"@dhis2/analytics": "^2.3.10",
"@dhis2/d2-i18n": "^1.0.3",
"@dhis2/d2-ui-core": "^6.2.1",
"@dhis2/d2-ui-file-menu": "^6.2.1",
"@dhis2/d2-ui-interpretations": "^6.2.1",
"@dhis2/data-visualizer-plugin": "33.0.3",
"@dhis2/data-visualizer-plugin": "33.1.0",
"@dhis2/ui": "^1.0.0-beta.11",
"@dhis2/ui-core": "^3.4.0",
"@material-ui/core": "^3.1.2",
Expand Down
9 changes: 1 addition & 8 deletions packages/app/src/components/App.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import VisualizationTypeSelector from './VisualizationTypeSelector/Visualization
import DimensionsPanel from './DimensionsPanel/DimensionsPanel';
import Interpretations from './Interpretations/Interpretations';
import Visualization from './Visualization/Visualization';
import BlankCanvas from './Visualization/BlankCanvas';
import Layout from './Layout/Layout';
import * as fromReducers from '../reducers';
import * as fromActions from '../actions';
Expand Down Expand Up @@ -159,11 +158,6 @@ export class App extends Component {
}

render() {
const showVis =
this.props.current &&
Object.keys(this.props.current).length > 0 &&
!this.props.loadError;

return (
<FatalErrorBoundary>
<AxisSetup />
Expand Down Expand Up @@ -191,7 +185,7 @@ export class App extends Component {
<TitleBar />
</div>
<div className="main-center-canvas flex-1">
{showVis ? <Visualization /> : <BlankCanvas />}
<Visualization />
</div>
</div>
{this.props.ui.rightSidebarOpen && this.props.current && (
Expand All @@ -214,7 +208,6 @@ const mapStateToProps = state => ({
settings: fromReducers.fromSettings.sGetSettings(state),
current: fromReducers.fromCurrent.sGetCurrent(state),
interpretations: fromReducers.fromVisualization.sGetInterpretations(state),
loadError: fromReducers.fromLoader.sGetLoadError(state),
ui: fromReducers.fromUi.sGetUi(state),
});

Expand Down
27 changes: 23 additions & 4 deletions packages/app/src/components/UpdateButton/UpdateButton.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,41 @@ import Button from '@material-ui/core/Button';
import i18n from '@dhis2/d2-i18n';

import { sGetUi } from '../../reducers/ui';
import { sGetCurrent } from '../../reducers/current';
import { sGetCurrent, sGetCurrentFromUi } from '../../reducers/current';
import * as fromActions from '../../actions';
import history from '../../modules/history';
import styles from './styles/UpdateButton.style';
import { CURRENT_AO_KEY } from '../../api/userDataStore';

import { acSetLoadError, acClearLoadError } from '../../actions/loader';
import { validateLayout } from '../../modules/layoutValidation';

const UpdateButton = ({
classes,
clearLoadError,
acClearLoadError,
acSetLoadError,
onUpdate,
ui,
current,
currentFromUi,
onClick,
flat,
...props
}) => {
const wrappedOnClick = () => {
clearLoadError();
// validate layout on update
// validation error message will be shown without loading the plugin first
try {
validateLayout(currentFromUi);

acClearLoadError();
} catch (err) {
acSetLoadError(
err && err.message
? err.message
: i18n.t('Error validating layout')
);
}
onUpdate(ui);

const urlContainsCurrentAOKey =
Expand Down Expand Up @@ -63,11 +80,13 @@ const UpdateButton = ({
const mapStateToProps = state => ({
ui: sGetUi(state),
current: sGetCurrent(state),
currentFromUi: sGetCurrentFromUi(state),
});

const mapDispatchToProps = {
onUpdate: fromActions.fromCurrent.acSetCurrentFromUi,
clearLoadError: fromActions.fromLoader.acClearLoadError,
acSetLoadError,
acClearLoadError,
};

UpdateButton.propTypes = {
Expand Down
31 changes: 2 additions & 29 deletions packages/app/src/components/Visualization/Visualization.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,7 @@ import { sGetLoadError } from '../../reducers/loader';

import { acAddMetadata } from '../../actions/metadata';
import { acSetChart } from '../../actions/chart';
import {
acSetLoadError,
acSetLoading,
acClearLoadError,
} from '../../actions/loader';

import { validateLayout } from '../../modules/layoutValidation';
import { acSetLoadError } from '../../actions/loader';

import BlankCanvas from './BlankCanvas';

Expand All @@ -35,22 +29,8 @@ export class Visualization extends Component {
this.state = {
renderId: null,
};

if (props.chartConfig) {
this.validate(props.chartConfig);
}
}

validate = visualization => {
try {
validateLayout(visualization);

this.props.acClearLoadError();
} catch (err) {
this.onError(err);
}
};

onError = err => {
const error =
(err && err.message) ||
Expand Down Expand Up @@ -99,11 +79,6 @@ export class Visualization extends Component {
}

componentDidUpdate(prevProps) {
if (this.props.chartConfig !== prevProps.chartConfig) {
this.validate(this.props.chartConfig);
return;
}

// open sidebar
if (this.props.rightSidebarOpen !== prevProps.rightSidebarOpen) {
this.getNewRenderId();
Expand All @@ -114,7 +89,7 @@ export class Visualization extends Component {
const { chartConfig, chartFilters, error } = this.props;
const { renderId } = this.state;

return error ? (
return Boolean(!chartConfig || error) ? (
<BlankCanvas />
) : (
<ChartPlugin
Expand Down Expand Up @@ -162,7 +137,5 @@ export default connect(
acAddMetadata,
acSetChart,
acSetLoadError,
acSetLoading,
acClearLoadError,
}
)(Visualization);
34 changes: 2 additions & 32 deletions packages/app/src/components/Visualization/__tests__/Visualization.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
chartFiltersSelector,
} from '../Visualization';
import BlankCanvas from '../BlankCanvas';
import * as validator from '../../../modules/layoutValidation';

jest.mock('@dhis2/data-visualizer-plugin', () => () => <div />);

Expand All @@ -24,7 +23,7 @@ describe('Visualization', () => {

beforeEach(() => {
props = {
chartConfig: null,
chartConfig: {},
chartFilters: null,
error: null,
rightSidebarOpen: false,
Expand All @@ -43,7 +42,7 @@ describe('Visualization', () => {
expect(vis().find(BlankCanvas).length).toEqual(1);
});

it('renders a ChartPlugin when no error', () => {
it('renders a ChartPlugin when no error and chartConfig available', () => {
expect(vis().find(ChartPlugin).length).toEqual(1);
});

Expand Down Expand Up @@ -78,23 +77,6 @@ describe('Visualization', () => {
expect(props.acSetLoadError).toHaveBeenCalledWith(errorMsg);
});

it('triggers clearLoadError when chart config has valid layout', () => {
props.chartConfig = { name: 'rainbowDash' };
validator.validateLayout = () => 'valid';
vis();
expect(props.acClearLoadError).toHaveBeenCalled();
});

it('triggers setLoadError when chart config has invalid layout', () => {
props.chartConfig = { name: 'non-valid rainbowDash' };
validator.validateLayout = () => {
throw new Error('not valid');
};
vis();

expect(props.acSetLoadError).toHaveBeenCalled();
});

it('renders chart with new id when rightSidebarOpen prop changes', () => {
const wrapper = vis();

Expand All @@ -104,18 +86,6 @@ describe('Visualization', () => {

expect(initialId).not.toEqual(updatedId);
});

it('triggers clearLoadError when chart changed to a different, valid chart', () => {
validator.validateLayout = () => 'valid';
const wrapper = vis();

wrapper.setProps({
...props,
chartConfig: { name: 'rainbowDash' },
});

expect(props.acClearLoadError).toHaveBeenCalledTimes(1);
});
});

describe('reselectors', () => {
Expand Down
8 changes: 1 addition & 7 deletions packages/app/src/components/__tests__/App.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,7 @@ describe('App', () => {
expect(app().find('div').length).toBeGreaterThan(0);
});

it('does not render a Visualization component when current is not populated', () => {
expect(app().find(Visualization).length).toEqual(0);
});

it('renders Visualization component when current is populated', () => {
props.current = { visProp: {} };

it('renders Visualization component', () => {
expect(app().find(Visualization).length).toBeGreaterThan(0);
});

Expand Down
4 changes: 3 additions & 1 deletion packages/app/src/modules/current.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ export const getSingleValueCurrentFromUi = (state, action) => {

// only save the first dx item
const axesFromUi = getAxesFromUi(ui);
axesFromUi.columns[0].items = [axesFromUi.columns[0].items[0]];
if (axesFromUi.columns.length && axesFromUi.columns[0].items.length) {
axesFromUi.columns[0].items = [axesFromUi.columns[0].items[0]];
}

return {
...state,
Expand Down
17 changes: 17 additions & 0 deletions packages/app/src/reducers/current.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,20 @@ export default (state = DEFAULT_CURRENT, action) => {
// Selectors

export const sGetCurrent = state => state.current;
export const sGetCurrentFromUi = state => {
const ui = state.ui;

switch (ui.type) {
case PIE:
case GAUGE:
return getPieCurrentFromUi(state, { value: ui });
case SINGLE_VALUE:
return getSingleValueCurrentFromUi(state, { value: ui });
case YEAR_OVER_YEAR_LINE:
case YEAR_OVER_YEAR_COLUMN:
return getYearOverYearCurrentFromUi(state, { value: ui });
default: {
return getDefaultFromUi(state, { value: ui });
}
}
};
2 changes: 1 addition & 1 deletion packages/plugin/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"main": "./build/index.js",
"license": "BSD-3-Clause",
"dependencies": {
"@dhis2/analytics": "2.3.4",
"@dhis2/analytics": "^2.3.10",
"@material-ui/core": "^3.1.2",
"lodash-es": "^4.17.11",
"react": "^16.6.0",
Expand Down
21 changes: 20 additions & 1 deletion yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@
debug "^3.1.0"
lodash.once "^4.1.1"

"@dhis2/analytics@2.3.4", "@dhis2/analytics@^2.1.0":
"@dhis2/analytics@^2.1.0":
version "2.3.4"
resolved "https://registry.yarnpkg.com/@dhis2/analytics/-/analytics-2.3.4.tgz#7132738b957f7342423f039f31b871465c9f4966"
integrity sha512-eLbCGtRRMUWZpvjlAAAfj0zr32J9CpS3oE3yXJlRVcPJwIT7S3xFAkjcgRwazchr5pZnKR/TBwoXmm84mF18zQ==
Expand All @@ -183,6 +183,25 @@
react-beautiful-dnd "^10.1.1"
styled-jsx "^3.2.1"

"@dhis2/analytics@^2.3.10":
version "2.3.10"
resolved "https://registry.yarnpkg.com/@dhis2/analytics/-/analytics-2.3.10.tgz#53b06d47d1fbd263d46abd1f21151fba8cdbcc9b"
integrity sha512-qUynHRTfaMcEKchg5koWJNGLv5kRLhh9kvsfxJmMkRQ/X5svh9HrDc99fzl0oEjq9Lv2KOnBxlrN86WqqsjEvQ==
dependencies:
"@dhis2/d2-i18n" "^1.0.4"
"@dhis2/d2-ui-org-unit-dialog" "^6.1.0"
"@dhis2/d2-ui-period-selector-dialog" "^6.2.0"
"@dhis2/ui-core" "^3.4.0"
"@material-ui/core" "^3.9.3"
"@material-ui/icons" "^3.0.2"
classnames "^2.2.6"
d2-utilizr "^0.2.16"
d3-color "^1.2.3"
highcharts "^7.1.2"
lodash "^4.17.11"
react-beautiful-dnd "^10.1.1"
styled-jsx "^3.2.1"

"@dhis2/d2-i18n-extract@^1.0.6", "@dhis2/d2-i18n-extract@^1.0.7":
version "1.0.8"
resolved "https://registry.yarnpkg.com/@dhis2/d2-i18n-extract/-/d2-i18n-extract-1.0.8.tgz#9d98690d522a51895c8ef3fe7136f026b0f8dacd"
Expand Down

0 comments on commit bf494b3

Please sign in to comment.