Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: show error UI when plugin has empty rows [DHIS2-16793] #3131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 2 additions & 28 deletions src/components/Visualization/StartScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@ import React, { useEffect, useState } from 'react'
import { connect } from 'react-redux'
import { apiFetchMostViewedVisualizations } from '../../api/mostViewedVisualizations.js'
import { apiFetchVisualizations } from '../../api/visualization.js'
import { GenericError } from '../../assets/ErrorIcons.js'
import { VisualizationError, genericErrorTitle } from '../../modules/error.js'
import history from '../../modules/history.js'
import { sGetLoadError } from '../../reducers/loader.js'
import { sGetUsername } from '../../reducers/user.js'
import styles from './styles/StartScreen.module.css'
import { matchVisualizationWithType } from './utils.js'
import { VisualizationErrorInfo } from './VisualizationErrorInfo.js'

const StartScreen = ({ error, username }) => {
const [mostViewedVisualizations, setMostViewedVisualizations] = useState([])
Expand Down Expand Up @@ -44,7 +43,7 @@ const StartScreen = ({ error, username }) => {

const getContent = () =>
error ? (
getErrorContent()
<VisualizationErrorInfo error={error} />
) : (
<div
data-test="start-screen"
Expand Down Expand Up @@ -108,31 +107,6 @@ const StartScreen = ({ error, username }) => {
</div>
)

const getErrorContent = () => (
<div
className={styles.errorContainer}
data-test="start-screen-error-container"
>
{error instanceof VisualizationError ? (
<>
<div className={styles.errorIcon}>{error.icon()}</div>
<p className={styles.errorTitle}>{error.title}</p>
<p className={styles.errorDescription}>
{error.description}
</p>
</>
) : (
<>
<div className={styles.errorIcon}>{GenericError()}</div>
<p className={styles.errorTitle}>{genericErrorTitle}</p>
<p className={styles.errorDescription}>
{error.message || error}
</p>
</>
)}
</div>
)

return (
<div className={styles.outer}>
<div className={styles.inner}>{getContent()}</div>
Expand Down
22 changes: 6 additions & 16 deletions src/components/Visualization/Visualization.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
import {
DIMENSION_ID_DATA,
VIS_TYPE_OUTLIER_TABLE,
VIS_TYPE_PIVOT_TABLE,
} from '@dhis2/analytics'
import { DIMENSION_ID_DATA, VIS_TYPE_PIVOT_TABLE } from '@dhis2/analytics'
import debounce from 'lodash-es/debounce'
import PropTypes from 'prop-types'
import React, { Component, Fragment } from 'react'
Expand All @@ -16,10 +12,10 @@ import {
acSetUiDataSorting,
acAddParentGraphMap,
} from '../../actions/ui.js'
import { ensureAnalyticsResponsesContainData } from '../../modules/analytics.js'
import {
AssignedCategoriesDataElementsError,
GenericServerError,
EmptyResponseError,
AssignedCategoriesAsFilterError,
MultipleIndicatorAsFilterError,
NoDataOrDataElementGroupSetError,
Expand All @@ -29,7 +25,6 @@ import {
ValueTypeError,
AnalyticsGenerationError,
AnalyticsRequestError,
NoOutliersError,
} from '../../modules/error.js'
import { removeLastPathSegment } from '../../modules/orgUnit.js'
import { sGetCurrent } from '../../reducers/current.js'
Expand Down Expand Up @@ -144,15 +139,10 @@ export class UnconnectedVisualization extends Component {

this.props.addMetadata(forMetadata)

if (
!responses.some((response) => response.rows && response.rows.length)
) {
if (this.props.visualization.type === VIS_TYPE_OUTLIER_TABLE) {
throw new NoOutliersError()
}

throw new EmptyResponseError()
}
ensureAnalyticsResponsesContainData(
responses,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this wasn't previously in a try/catch either. Does this ever throw? What does the user experience in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked on this quite a while ago, but I am pretty sure it is like this (quite complicated):

  1. onResponsesReceived is defined in Visualization and calls ensureAnalyticsResponsesContainData. It is passed as a prop to PluginWrapper.
  2. It is and then called by the PluginWrapper when a response comes in: it is called by doFetchData which in turn is called in doFetchAll.
  3. doFetchAll() returns a Promise and the catch method of this promise is called (doFetchAll().catch(..)). In the catch calback onError() is being called with the error. So if ensureAnalyticsResponsesContainData() throws an error this will be the parameter passed to onError(error)
  4. onError itself is another prop called by the PluginWrapper but defined in the Visualization. It ends up calling this.props.setLoadError(error) and this will ensure the StartScreen is rendered with the appropriate message.

this.props.visualization.type
)
}

onDrill = (drillData) => {
Expand Down
35 changes: 35 additions & 0 deletions src/components/Visualization/VisualizationErrorInfo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import PropTypes from 'prop-types'
import React from 'react'
import { GenericError } from '../../assets/ErrorIcons.js'
import { VisualizationError, genericErrorTitle } from '../../modules/error.js'
import styles from './styles/VisualizationErrorInfo.module.css'

export const VisualizationErrorInfo = ({ error }) => (
<div
className={styles.errorContainer}
data-test="start-screen-error-container"
>
{error instanceof VisualizationError ? (
<>
<div className={styles.errorIcon}>{error.icon()}</div>
<p className={styles.errorTitle}>{error.title}</p>
<p className={styles.errorDescription}>{error.description}</p>
</>
) : (
<>
<div className={styles.errorIcon}>{GenericError()}</div>
<p className={styles.errorTitle}>{genericErrorTitle}</p>
<p className={styles.errorDescription}>
{error.message || error}
</p>
</>
)}
</div>
)

VisualizationErrorInfo.propTypes = {
error: PropTypes.oneOfType([
PropTypes.instanceOf(Error),
PropTypes.instanceOf(VisualizationError),
]),
}
28 changes: 0 additions & 28 deletions src/components/Visualization/styles/StartScreen.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,6 @@
display: flex;
align-items: center;
}
.errorContainer {
display: flex;
flex-direction: column;
align-items: center;
text-align: center;
}
.errorIcon {
width: 136px;
height: 136px;
margin: 0 auto var(--spacers-dp24);
}
.errorTitle {
font-weight: 500;
font-size: 20px;
color: var(--colors-grey800);
letter-spacing: 0.15px;
line-height: 24px;
width: 360px;
margin: 0 auto var(--spacers-dp12);
}
.errorDescription {
font-weight: 400;
font-size: 14px;
color: var(--colors-grey700);
line-height: 19px;
width: 360px;
margin: 0 auto;
}
.title {
font-weight: 500;
font-size: 17px;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
.errorContainer {
display: flex;
flex-direction: column;
align-items: center;
text-align: center;
justify-content: center;
width: 100%;
}
.errorIcon {
width: 136px;
height: 136px;
margin: 0 auto var(--spacers-dp24);
}
.errorTitle {
font-weight: 500;
font-size: 20px;
color: var(--colors-grey800);
letter-spacing: 0.15px;
line-height: 24px;
width: 360px;
margin: 0 auto var(--spacers-dp12);
}
.errorDescription {
font-weight: 400;
font-size: 14px;
color: var(--colors-grey700);
line-height: 19px;
width: 360px;
margin: 0 auto;
}
21 changes: 21 additions & 0 deletions src/components/VisualizationPlugin/VisualizationPluginWrapper.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { CenteredContent, CircularLoader, ComponentCover } from '@dhis2/ui'
import PropTypes from 'prop-types'
import React, { useCallback, useEffect, useState } from 'react'
import { ensureAnalyticsResponsesContainData } from '../../modules/analytics.js'
import { VisualizationErrorInfo } from '../Visualization/VisualizationErrorInfo.js'
import { VisualizationPlugin } from '../VisualizationPlugin/VisualizationPlugin.js'

// handle internal state for features that need to work without the app's Redux store
const VisualizationPluginWrapper = (props) => {
const [pluginProps, setPluginProps] = useState(props)
const [isLoading, setIsLoading] = useState(true)
const [error, setError] = useState(null)

const onDataSorted = useCallback(
(sorting) => {
Expand Down Expand Up @@ -64,6 +67,23 @@ const VisualizationPluginWrapper = (props) => {

const onLoadingComplete = () => setIsLoading(false)

const onResponsesReceived = useCallback(
(responses) => {
try {
ensureAnalyticsResponsesContainData(
responses,
props.visualization.type
)
} catch (error) {
setError(error)
}
},
[props.visualization.type]
)

if (error) {
return <VisualizationErrorInfo error={error} />
}
return (
<>
{isLoading && (
Expand All @@ -77,6 +97,7 @@ const VisualizationPluginWrapper = (props) => {
{...pluginProps}
onDataSorted={onDataSorted}
onLoadingComplete={onLoadingComplete}
onResponsesReceived={onResponsesReceived}
/>
</>
)
Expand Down
18 changes: 18 additions & 0 deletions src/modules/analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ import {
DIMENSION_ID_PERIOD,
WEEKLY,
DAILY,
VIS_TYPE_OUTLIER_TABLE,
} from '@dhis2/analytics'
import i18n from '@dhis2/d2-i18n'
import { EmptyResponseError, NoOutliersError } from './error.js'

export const outlierTableHeadersMap = {
[DIMENSION_ID_DATA]: 'dxname',
Expand Down Expand Up @@ -281,3 +283,19 @@ export const getRelativePeriodTypeUsed = (periodItems) => {
return DAILY
}
}

export const ensureAnalyticsResponsesContainData = (
responses,
visualizationType
) => {
const hasPopulatedRows = responses.some(
(response) => response.rows && response.rows.length > 0
)
if (!hasPopulatedRows) {
if (visualizationType === VIS_TYPE_OUTLIER_TABLE) {
throw new NoOutliersError()
}

throw new EmptyResponseError()
}
}
Loading