Skip to content

Commit

Permalink
fix: switching from table to chart shows wrong data [DHIS2-9599] (#1196)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jenniferarnesen
jenniferarnesen authored Nov 12, 2020
1 parent b9f5e72 commit ab60389
Show file tree
Hide file tree
Showing 21 changed files with 462 additions and 347 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
12 changes: 1 addition & 11 deletions src/__tests__/util.spec.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { validateReducer, getBaseUrl, getWithoutId } from '../modules/util'
import { validateReducer, getBaseUrl } from '../modules/util'

describe('util', () => {
describe('validateReducer', () => {
Expand Down Expand Up @@ -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)
})
})
})
4 changes: 2 additions & 2 deletions src/actions/selected.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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))
Expand Down
30 changes: 17 additions & 13 deletions src/api/metadata.js
View file
Original file line number Diff line number Diff line change
@@ -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 } = {}) => [
Expand Down Expand Up @@ -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,
})
}
46 changes: 16 additions & 30 deletions src/components/Item/VisualizationItem/DefaultPlugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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() ? (
<div id={getGridItemDomId(item.id)} style={style} />
) : (
<div className={classes.textDiv}>
{i18n.t('Unable to load the plugin for this item')}
</div>
<NoVisualizationMessage
message={i18n.t('Unable to load the plugin for this item')}
/>
)
}
}
Expand All @@ -117,7 +104,6 @@ DefaultPlugin.contextTypes = {

DefaultPlugin.propTypes = {
activeType: PropTypes.string,
classes: PropTypes.object,
item: PropTypes.object,
itemFilters: PropTypes.object,
options: PropTypes.object,
Expand Down
107 changes: 36 additions & 71 deletions src/components/Item/VisualizationItem/Item.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -28,6 +31,7 @@ import {
CHART,
REPORT_TABLE,
} from '../../../modules/itemTypes'
import { getVisualizationId, getVisualizationName } from '../../../modules/item'
import memoizeOne from '../../../modules/memoizeOne'
import {
isEditMode,
Expand All @@ -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,
Expand All @@ -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({
Expand Down Expand Up @@ -141,29 +140,18 @@ export class Item extends Component {

if (!visualization) {
return (
<div className={classes.textDiv}>
{i18n.t('No data to display')}
</div>
<NoVisualizationMessage
message={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) {
Expand Down Expand Up @@ -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 <DefaultPlugin {...props} />
return (
<MapPlugin
applyFilters={this.memoizedApplyFilters}
{...props}
/>
)
}
default: {
props.visualization = this.memoizedApplyFilters(
Expand Down Expand Up @@ -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
Expand All @@ -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}
/>
Expand All @@ -298,7 +263,7 @@ export class Item extends Component {
return (
<>
<ItemHeader
title={pluginManager.getName(item)}
title={getVisualizationName(item)}
itemId={item.id}
actionButtons={actionButtons}
ref={this.headerRef}
Expand Down Expand Up @@ -355,7 +320,7 @@ const mapStateToProps = (state, ownProps) => {
itemFilters,
visualization: sGetVisualization(
state,
pluginManager.extractFavorite(ownProps.item).id
getVisualizationId(ownProps.item)
),
}
}
Expand Down
Loading

0 comments on commit ab60389

Please sign in to comment.