From ab60389bc0d93f51a193811a333c422f770728b4 Mon Sep 17 00:00:00 2001 From: Jen Jones Arnesen Date: Thu, 12 Nov 2020 09:50:28 +0100 Subject: [PATCH] fix: switching from table to chart shows wrong data [DHIS2-9599] (#1196) This commit includes a fair amount of refactoring. The goal of the refactoring was to move functionality out of plugin.js that had nothing to do with the plugins: * extractFavorite was moved to a separate module and renamed getVisualizationFromItem * extracted Map-specific code out of the VisualizationItem/Item to a separate component. This because VisualizationItem/Item is becoming a very large file * ItemHeaderButtons - get d2 from context rather than passing around as a property * extracted a NoVisualizationMessage component that contains its style * moved content height functionality to one place in the file VisualizationItem/Item rather than spread out (no functional changes to that code) * moved function getWithoutId to be together with the only code that actually uses that function * as suggested by the TODO in VisualizationItem/Item, call the apiFetchVisualization directly rather than using the pluginManager. Bug fix: * View as should show the same as what DV would show. This fix is in the file getVisualizationConfig.js - and uses the function getAdaptedUiLayoutByType from @dhis2/analytics. --- package.json | 2 +- src/__tests__/util.spec.js | 12 +- src/actions/selected.js | 4 +- src/api/metadata.js | 30 +++-- .../Item/VisualizationItem/DefaultPlugin.js | 46 +++---- src/components/Item/VisualizationItem/Item.js | 107 +++++---------- .../Item/VisualizationItem/ItemFooter.js | 4 +- .../VisualizationItem/ItemHeaderButtons.js | 13 +- .../Item/VisualizationItem/MapPlugin.js | 52 ++++++++ .../NoVisualizationMessage.js | 14 ++ .../VisualizationItem/__tests__/Item.spec.js | 25 ++-- .../__tests__/__snapshots__/Item.spec.js.snap | 4 +- .../__tests__/getVisualizationConfig.spec.js | 125 ++++++++++++++++++ .../__tests__/plugin.spec.js | 119 ----------------- .../getVisualizationConfig.js | 51 +++++++ .../Item/VisualizationItem/plugin.js | 78 +---------- ....css => NoVisualizationMessage.module.css} | 2 +- src/modules/__tests__/item.spec.js | 72 ++++++++++ src/modules/item.js | 24 ++++ src/modules/util.js | 5 - yarn.lock | 20 ++- 21 files changed, 462 insertions(+), 347 deletions(-) create mode 100644 src/components/Item/VisualizationItem/MapPlugin.js create mode 100644 src/components/Item/VisualizationItem/NoVisualizationMessage.js create mode 100644 src/components/Item/VisualizationItem/__tests__/getVisualizationConfig.spec.js delete mode 100644 src/components/Item/VisualizationItem/__tests__/plugin.spec.js create mode 100644 src/components/Item/VisualizationItem/getVisualizationConfig.js rename src/components/Item/VisualizationItem/styles/{Item.module.css => NoVisualizationMessage.module.css} (89%) create mode 100644 src/modules/__tests__/item.spec.js create mode 100644 src/modules/item.js diff --git a/package.json b/package.json index 1513066e0..6bda25331 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "private": true, "license": "BSD-3-Clause", "dependencies": { - "@dhis2/analytics": "^11.1.5", + "@dhis2/analytics": "^11.2.0", "@dhis2/app-runtime": "^2.4.0", "@dhis2/app-runtime-adapter-d2": "^1.0.1", "@dhis2/d2-i18n": "^1.0.6", diff --git a/src/__tests__/util.spec.js b/src/__tests__/util.spec.js index 2505a4385..5744e1dc2 100644 --- a/src/__tests__/util.spec.js +++ b/src/__tests__/util.spec.js @@ -1,4 +1,4 @@ -import { validateReducer, getBaseUrl, getWithoutId } from '../modules/util' +import { validateReducer, getBaseUrl } from '../modules/util' describe('util', () => { describe('validateReducer', () => { @@ -50,14 +50,4 @@ describe('util', () => { expect(actual).toEqual(baseUrl) }) }) - - describe('getWithoutId', () => { - it('removes id property from an object', () => { - const object = { id: 'SOME_ID', someProp: 'someValue' } - const expectedResult = { someProp: 'someValue' } - const actualResult = getWithoutId(object) - - expect(actualResult).toEqual(expectedResult) - }) - }) }) diff --git a/src/actions/selected.js b/src/actions/selected.js index ffaa13b26..8d39ded3f 100644 --- a/src/actions/selected.js +++ b/src/actions/selected.js @@ -24,7 +24,7 @@ import { } from '../api/description' import { withShape } from '../components/ItemGrid/gridUtil' -import { extractFavorite } from '../components/Item/VisualizationItem/plugin' +import { getVisualizationFromItem } from '../modules/item' import { REPORT_TABLE, @@ -104,7 +104,7 @@ export const tSetSelectedDashboardById = id => async (dispatch, getState) => { case MAP: case EVENT_REPORT: case EVENT_CHART: - dispatch(acAddVisualization(extractFavorite(item))) + dispatch(acAddVisualization(getVisualizationFromItem(item))) break case MESSAGES: dispatch(tGetMessages(id)) diff --git a/src/api/metadata.js b/src/api/metadata.js index 3e13036f7..40a079bd6 100644 --- a/src/api/metadata.js +++ b/src/api/metadata.js @@ -1,7 +1,8 @@ import { getInstance } from 'd2' import arrayClean from 'd2-utilizr/lib/arrayClean' -import { getEndPointName } from '../modules/itemTypes' +import { getEndPointName, MAP } from '../modules/itemTypes' +import { getVisualizationId } from '../modules/item' // Id, name export const getIdNameFields = ({ rename } = {}) => [ @@ -96,16 +97,19 @@ export const getMapFields = () => [ ] // Api +export const apiFetchVisualization = async item => { + const id = getVisualizationId(item) + const fields = + item.type === MAP + ? getMapFields() + : getFavoriteFields({ + withDimensions: true, + withOptions: true, + }) -// Get more info about the favorite of a dashboard item -export const apiFetchFavorite = (id, type, { fields }) => - getInstance().then(d2 => - d2.Api.getApi().get(`${getEndPointName(type)}/${id}`, { - fields: - fields || - getFavoriteFields({ - withDimensions: true, - withOptions: true, - }), - }) - ) + const d2 = await getInstance() + + return await d2.Api.getApi().get(`${getEndPointName(item.type)}/${id}`, { + fields, + }) +} diff --git a/src/components/Item/VisualizationItem/DefaultPlugin.js b/src/components/Item/VisualizationItem/DefaultPlugin.js index e6afbdf35..72b4f849e 100644 --- a/src/components/Item/VisualizationItem/DefaultPlugin.js +++ b/src/components/Item/VisualizationItem/DefaultPlugin.js @@ -3,6 +3,7 @@ import PropTypes from 'prop-types' import i18n from '@dhis2/d2-i18n' +import NoVisualizationMessage from './NoVisualizationMessage' import * as pluginManager from './plugin' import { getBaseUrl, orObject } from '../../../modules/util' import { getGridItemDomId } from '../../ItemGrid/gridUtil' @@ -23,6 +24,11 @@ class DefaultPlugin extends Component { this.d2 = context.d2 } + pluginIsAvailable = () => + pluginManager.pluginIsAvailable( + this.props.activeType || this.props.item.type + ) + shouldPluginReload = prevProps => { // TODO - fix this hack, to handle bug with multiple // rerendering while switching between dashboards. @@ -35,19 +41,14 @@ class DefaultPlugin extends Component { const vis = orObject(this.props.visualization) const prevVis = orObject(prevProps.visualization) const visChanged = - vis.id !== prevVis.id || vis.activeType !== prevVis.activeType + vis.id !== prevVis.id || + prevProps.activeType !== this.props.activeType return reloadAllowed && (visChanged || filtersChanged) } reloadPlugin = prevProps => { - if ( - pluginManager.pluginIsAvailable( - this.props.item, - this.props.visualization - ) && - this.shouldPluginReload(prevProps) - ) { + if (this.pluginIsAvailable() && this.shouldPluginReload(prevProps)) { if ( this.props.activeType !== prevProps.activeType || this.props.itemFilters !== prevProps.itemFilters @@ -65,12 +66,7 @@ class DefaultPlugin extends Component { componentDidMount() { this.pluginCredentials = pluginCredentials(this.d2) - if ( - pluginManager.pluginIsAvailable( - this.props.item, - this.props.visualization - ) - ) { + if (this.pluginIsAvailable()) { pluginManager.load(this.props.item, this.props.visualization, { credentials: this.pluginCredentials, activeType: this.props.activeType, @@ -84,29 +80,20 @@ class DefaultPlugin extends Component { } componentWillUnmount() { - if ( - pluginManager.pluginIsAvailable( - this.props.item, - this.props.visualization - ) - ) { + if (this.pluginIsAvailable()) { pluginManager.unmount(this.props.item, this.props.activeType) } } render() { - const { classes, item, visualization, style } = this.props - const pluginIsAvailable = pluginManager.pluginIsAvailable( - item, - visualization - ) + const { item, style } = this.props - return pluginIsAvailable ? ( + return this.pluginIsAvailable() ? (
) : ( -
- {i18n.t('Unable to load the plugin for this item')} -
+ ) } } @@ -117,7 +104,6 @@ DefaultPlugin.contextTypes = { DefaultPlugin.propTypes = { activeType: PropTypes.string, - classes: PropTypes.object, item: PropTypes.object, itemFilters: PropTypes.object, options: PropTypes.object, diff --git a/src/components/Item/VisualizationItem/Item.js b/src/components/Item/VisualizationItem/Item.js index 4376fd7cb..ef2e560de 100644 --- a/src/components/Item/VisualizationItem/Item.js +++ b/src/components/Item/VisualizationItem/Item.js @@ -6,13 +6,16 @@ import VisualizationPlugin from '@dhis2/data-visualizer-plugin' import i18n from '@dhis2/d2-i18n' import DefaultPlugin from './DefaultPlugin' +import MapPlugin from './MapPlugin' import FatalErrorBoundary from './FatalErrorBoundary' import ItemHeader, { HEADER_MARGIN_HEIGHT } from '../ItemHeader/ItemHeader' import ItemHeaderButtons from './ItemHeaderButtons' import ItemFooter from './ItemFooter' import LoadingMask from './LoadingMask' +import NoVisualizationMessage from './NoVisualizationMessage' -import * as pluginManager from './plugin' +import { apiFetchVisualization } from '../../../api/metadata' +import getVisualizationConfig from './getVisualizationConfig' import { sGetVisualization } from '../../../reducers/visualizations' import { sGetSelectedItemActiveType } from '../../../reducers/selected' import { sGetIsEditing } from '../../../reducers/editDashboard' @@ -28,6 +31,7 @@ import { CHART, REPORT_TABLE, } from '../../../modules/itemTypes' +import { getVisualizationId, getVisualizationName } from '../../../modules/item' import memoizeOne from '../../../modules/memoizeOne' import { isEditMode, @@ -37,8 +41,6 @@ import { import { ITEM_CONTENT_PADDING_BOTTOM } from '../../ItemGrid/ItemGrid' -import classes from './styles/Item.module.css' - export class Item extends Component { state = { showFooter: false, @@ -56,17 +58,14 @@ export class Item extends Component { this.memoizedApplyFilters = memoizeOne(this.applyFilters) - this.memoizedGetVisualizationConfig = memoizeOne( - pluginManager.getVisualizationConfig - ) + this.memoizedGetVisualizationConfig = memoizeOne(getVisualizationConfig) - this.memoizedGetContentStyle = memoizeOne(this.getContentStyle) + this.memoizedGetContentHeight = memoizeOne(this.getContentHeight) } async componentDidMount() { this.props.updateVisualization( - // TODO do not call fetch on the pluginManager, do it here as the manager will eventually be removed... - await pluginManager.fetch(this.props.item) + await apiFetchVisualization(this.props.item) ) this.setState({ @@ -141,29 +140,18 @@ export class Item extends Component { if (!visualization) { return ( -
- {i18n.t('No data to display')} -
+ ) } - const calculatedHeight = - this.props.item.originalHeight - - this.headerRef.current.clientHeight - - HEADER_MARGIN_HEIGHT - - ITEM_CONTENT_PADDING_BOTTOM - const props = { - ...this.props, + item: this.props.item, + itemFilters: this.props.itemFilters, activeType, visualization, - classes, - style: this.memoizedGetContentStyle( - calculatedHeight, - this.contentRef ? this.contentRef.offsetHeight : null, - isEditMode(this.props.dashboardMode) || - isPrintMode(this.props.dashboardMode) - ), + style: this.getPluginStyle(), } switch (activeType) { @@ -191,43 +179,12 @@ export class Item extends Component { ) } case MAP: { - if (props.item.type === MAP) { - // apply filters only to thematic and event layers - // for maps AO - const mapViews = props.visualization.mapViews.map(obj => { - if ( - obj.layer.includes('thematic') || - obj.layer.includes('event') - ) { - return this.memoizedApplyFilters( - obj, - props.itemFilters - ) - } - - return obj - }) - - props.visualization = { - ...props.visualization, - mapViews, - } - } else { - // this is the case of a non map AO passed to the maps plugin - // due to a visualization type switch in dashboard item - // maps plugin takes care of converting the AO to a suitable format - props.visualization = this.memoizedApplyFilters( - props.visualization, - props.itemFilters - ) - } - - props.options = { - ...props.options, - hideTitle: true, - } - - return + return ( + + ) } default: { props.visualization = this.memoizedApplyFilters( @@ -265,13 +222,22 @@ export class Item extends Component { return this.props.activeType || this.props.item.type } - pluginIsAvailable = () => - pluginManager.pluginIsAvailable( - this.props.item, - this.props.visualization + getPluginStyle = () => { + const calculatedHeight = + this.props.item.originalHeight - + this.headerRef.current.clientHeight - + HEADER_MARGIN_HEIGHT - + ITEM_CONTENT_PADDING_BOTTOM + + return this.memoizedGetContentHeight( + calculatedHeight, + this.contentRef ? this.contentRef.offsetHeight : null, + isEditMode(this.props.dashboardMode) || + isPrintMode(this.props.dashboardMode) ) + } - getContentStyle = (calculatedHeight, measuredHeight, preferMeasured) => { + getContentHeight = (calculatedHeight, measuredHeight, preferMeasured) => { const height = preferMeasured ? measuredHeight || calculatedHeight : calculatedHeight @@ -289,7 +255,6 @@ export class Item extends Component { visualization={this.props.visualization} onSelectActiveType={this.selectActiveType} onToggleFooter={this.onToggleFooter} - d2={this.d2} activeType={this.getActiveType()} activeFooter={this.state.showFooter} /> @@ -298,7 +263,7 @@ export class Item extends Component { return ( <> { itemFilters, visualization: sGetVisualization( state, - pluginManager.extractFavorite(ownProps.item).id + getVisualizationId(ownProps.item) ), } } diff --git a/src/components/Item/VisualizationItem/ItemFooter.js b/src/components/Item/VisualizationItem/ItemFooter.js index 345989dbd..c811489a9 100644 --- a/src/components/Item/VisualizationItem/ItemFooter.js +++ b/src/components/Item/VisualizationItem/ItemFooter.js @@ -1,7 +1,7 @@ import React, { Component } from 'react' import PropTypes from 'prop-types' import { colors } from '@dhis2/ui' -import { getId } from './plugin' +import { getVisualizationId } from '../../../modules/item' import InterpretationsComponent from '@dhis2/d2-ui-interpretations' const style = { @@ -19,7 +19,7 @@ const style = { class ItemFooter extends Component { render() { - const objectId = getId(this.props.item) + const objectId = getVisualizationId(this.props.item) return (
diff --git a/src/components/Item/VisualizationItem/ItemHeaderButtons.js b/src/components/Item/VisualizationItem/ItemHeaderButtons.js index 494a10149..5f83a066b 100644 --- a/src/components/Item/VisualizationItem/ItemHeaderButtons.js +++ b/src/components/Item/VisualizationItem/ItemHeaderButtons.js @@ -30,10 +30,10 @@ import { const iconFill = { fill: colors.grey600 } -const ItemHeaderButtons = props => { +const ItemHeaderButtons = (props, context) => { const [anchorEl, setAnchorEl] = useState(null) - const { item, visualization, onSelectActiveType, d2, activeType } = props + const { item, visualization, onSelectActiveType, activeType } = props const isTrackerType = isTrackerDomainType(item.type) @@ -105,7 +105,7 @@ const ItemHeaderButtons = props => { ) - return pluginIsAvailable(item, visualization) ? ( + return pluginIsAvailable(activeType || item.type) ? ( <>