Skip to content

Commit

Permalink
fix: compute totals and cumulative values for numeric/boolean types r…
Browse files Browse the repository at this point in the history 
…especting totalAggregationType (DHIS2-9155) (#1700)

* fix: accumulate numeric (not PERCENTAGE, UNIT_INTERVAL) and boolean values

This makes the cumulative values feature more in line with the way
totals are computed.
The difference is that there is no accumulation for PERCENTAGE and
UNIT_INTERVAL types as these don't accumulate with a simple sum.

* fix: allow totals for all numeric/boolean, respect totalAggregationType

For row totals where 1 or more columns have a non-numeric/boolean data element, N/A is returned for the total cell.
For column totals, the totalAggregationType of the data element is used to compute
the total value.

* fix: style N/A differently than a normal value

* feat: allow custom title for cells

Normally the title is the same as the cell's content.
When cumulative values are used it help to see the original value in the
title and the accumulated value in the cell.
It's also useful to give some more info about a particular value (ie.
N/A).

* fix: avoid to show 0 for non cumulative types when cumulative is enabled

* refactor: replace ||= operator, not transformed by Babel

* fix: fix regression for DHIS2-17297

* fix: always use "Value:" prefix in cell tooltip

* fix: handle better mixed values when accumulating

* fix: do not fill the table with N/A with cumulative values

Simply render the original value for non cumulative types.
The tooltip can be used when in doubt to know if a cell value is
accumulated.

* fix: only accumulate when total agg type is SUM

---------

Co-authored-by: Jan Henrik Øverland <[email protected]>
  • Loading branch information
@edoardo @janhenrikoverland
edoardo and janhenrikoverland authored Oct 18, 2024
1 parent b478ff0 commit a2bfd20
Show file tree
Hide file tree
Showing 7 changed files with 134 additions and 33 deletions.
10 changes: 8 additions & 2 deletions 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: 2024-08-27T11:29:09.031Z\n"
"PO-Revision-Date: 2024-08-27T11:29:09.033Z\n"
"POT-Creation-Date: 2024-10-11T12:49:26.846Z\n"
"PO-Revision-Date: 2024-10-11T12:49:26.847Z\n"

msgid "view only"
msgstr "view only"
Expand Down Expand Up @@ -855,6 +855,9 @@ msgstr "Financial Years"
msgid "Years"
msgstr "Years"

msgid "Value: {{value}}"
msgstr "Value: {{value}}"

msgid "Bold text"
msgstr "Bold text"

Expand Down Expand Up @@ -1125,6 +1128,9 @@ msgstr "{{thresholdFactor}} × Z-score low"
msgid "{{thresholdFactor}} × Z-score high"
msgstr "{{thresholdFactor}} × Z-score high"

msgid "Not applicable"
msgstr "Not applicable"

msgid "Data"
msgstr "Data"

Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
},
"scripts": {
"build": "d2-app-scripts build",
"postbuild": "yarn build-storybook",
"build-storybook": "build-storybook",
"start-storybook": "start-storybook --port 5000",
"start": "yarn start-storybook",
Expand Down
9 changes: 8 additions & 1 deletion src/components/PivotTable/PivotTableValueCell.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import i18n from '@dhis2/d2-i18n'
import PropTypes from 'prop-types'
import React, { useRef } from 'react'
import { applyLegendSet } from '../../modules/pivotTable/applyLegendSet.js'
Expand Down Expand Up @@ -74,7 +75,13 @@ export const PivotTableValueCell = ({
<PivotTableCell
key={column}
classes={classes}
title={cellContent.renderedValue}
title={
cellContent.titleValue ??
i18n.t('Value: {{value}}', {
value: cellContent.renderedValue,
nsSeparator: '^^',
})
}
style={style}
onClick={isClickable ? onClick : undefined}
ref={cellRef}
Expand Down
5 changes: 5 additions & 0 deletions src/components/PivotTable/styles/PivotTable.style.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,11 @@ export const cell = css`
.TRUE_ONLY {
text-align: right;
}
.N_A {
text-align: center;
color: ${colors.grey600};
}
.clickable {
cursor: pointer;
}
Expand Down
127 changes: 98 additions & 29 deletions src/modules/pivotTable/PivotTableEngine.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
VALUE_TYPE_NUMBER,
VALUE_TYPE_TEXT,
isBooleanValueType,
isCumulativeValueType,
isNumericValueType,
} from '../valueTypes.js'
import { AdaptiveClippingController } from './AdaptiveClippingController.js'
Expand Down Expand Up @@ -41,6 +42,8 @@ import {
NUMBER_TYPE_COLUMN_PERCENTAGE,
NUMBER_TYPE_ROW_PERCENTAGE,
NUMBER_TYPE_VALUE,
VALUE_TYPE_NA,
VALUE_NA,
} from './pivotTableConstants.js'

const dataFields = [
Expand Down Expand Up @@ -245,7 +248,7 @@ const applyTotalAggregationType = (
) => {
switch (overrideTotalAggregationType || totalAggregationType) {
case AGGREGATE_TYPE_NA:
return 'N/A'
return VALUE_NA
case AGGREGATE_TYPE_AVERAGE:
return (
((numerator || value) * multiplier) /
Expand Down Expand Up @@ -401,19 +404,46 @@ export class PivotTableEngine {
rawCell.renderedValue = renderedValue
}

if (
[CELL_TYPE_TOTAL, CELL_TYPE_SUBTOTAL].includes(rawCell.cellType) &&
rawCell.rawValue === AGGREGATE_TYPE_NA
) {
rawCell.titleValue = i18n.t('Not applicable')
}

if (this.options.cumulativeValues) {
let titleValue

if (this.data[row] && this.data[row][column]) {
const dataRow = this.data[row][column]

const rawValue =
cellType === CELL_TYPE_VALUE
? dataRow[this.dimensionLookup.dataHeaders.value]
: dataRow.value

titleValue = i18n.t('Value: {{value}}', {
value: renderValue(rawValue, valueType, this.visualization),
nsSeparator: '^^',
})
}

const cumulativeValue = this.getCumulative({
row,
column,
})

if (cumulativeValue !== undefined && cumulativeValue !== null) {
// force to NUMBER for accumulated values
// force to TEXT for N/A (accumulated) values
// force to NUMBER for accumulated values if no valueType present
rawCell.valueType =
valueType === undefined || valueType === null
cumulativeValue === VALUE_NA
? VALUE_TYPE_NA
: valueType === undefined || valueType === null
? VALUE_TYPE_NUMBER
: valueType
rawCell.empty = false
rawCell.titleValue = titleValue
rawCell.rawValue = cumulativeValue
rawCell.renderedValue = renderValue(
cumulativeValue,
Expand Down Expand Up @@ -523,16 +553,12 @@ export class PivotTableEngine {

const cellValue = this.data[row][column]

// empty cell
if (!cellValue) {
// Empty cell
// The cell still needs to get the valueType to render correctly 0 and cumulative values
return {
valueType: VALUE_TYPE_NUMBER,
totalAggregationType: AGGREGATE_TYPE_SUM,
}
return undefined
}

if (!Array.isArray(cellValue)) {
if (cellValue && !Array.isArray(cellValue)) {
// This is a total cell
return {
valueType: cellValue.valueType,
Expand Down Expand Up @@ -741,23 +767,30 @@ export class PivotTableEngine {
totalCell.totalAggregationType = currentAggType
}

const currentValueType = dxDimension?.valueType
// Force value type of total cells to NUMBER for value cells with numeric or boolean types.
// This is to simplify the code below where we compare the previous value type.
// All numeric/boolean value types use the same style for rendering the total cell (right aligned content)
// and using NUMBER for the total cell is enough for that.
// (see DHIS2-9155)
const currentValueType =
isNumericValueType(dxDimension?.valueType) ||
isBooleanValueType(dxDimension?.valueType)
? VALUE_TYPE_NUMBER
: dxDimension?.valueType

const previousValueType = totalCell.valueType
if (previousValueType && currentValueType !== previousValueType) {
totalCell.valueType = AGGREGATE_TYPE_NA
totalCell.valueType = VALUE_TYPE_NA
} else {
totalCell.valueType = currentValueType
}

// compute subtotals and totals for all numeric and boolean value types
// in that case, force value type of subtotal and total cells to NUMBER to format them correctly
// Compute totals for all numeric and boolean value types only.
// In practice valueType here is NUMBER (see the comment above).
// When is not, it means there is some value cell with a valueType other than numeric/boolean,
// the total should not be computed then.
// (see DHIS2-9155)
if (
isNumericValueType(dxDimension?.valueType) ||
isBooleanValueType(dxDimension?.valueType)
) {
totalCell.valueType = VALUE_TYPE_NUMBER

if (isNumericValueType(totalCell.valueType)) {
dataFields.forEach((field) => {
const headerIndex = this.dimensionLookup.dataHeaders[field]
const value = parseValue(dataRow[headerIndex])
Expand Down Expand Up @@ -882,6 +915,28 @@ export class PivotTableEngine {
}
}
}

computeOverrideTotalAggregationType(totalCell, visualization) {
// Avoid undefined on total cells with valueTypes that cannot be totalized.
// This happens for example when a column/row has all value cells of type TEXT.
if (
!(
isNumericValueType(totalCell.valueType) ||
isBooleanValueType(totalCell.valueType)
)
) {
return AGGREGATE_TYPE_NA
}

// DHIS2-15698: do not override total aggregation type when numberType option is not present
// (numberType option default is VALUE)
return (
visualization.numberType &&
visualization.numberType !== NUMBER_TYPE_VALUE &&
AGGREGATE_TYPE_SUM
)
}

finalizeTotal({ row, column }) {
if (!this.data[row]) {
return
Expand All @@ -890,12 +945,17 @@ export class PivotTableEngine {
if (totalCell && totalCell.count) {
totalCell.value = applyTotalAggregationType(
totalCell,
// DHIS2-15698: do not override total aggregation type when numberType option is not present
// (numberType option default is VALUE)
this.visualization.numberType &&
this.visualization.numberType !== NUMBER_TYPE_VALUE &&
AGGREGATE_TYPE_SUM
this.computeOverrideTotalAggregationType(
totalCell,
this.visualization
)
)

// override valueType for styling cells with N/A value
if (totalCell.value === AGGREGATE_TYPE_NA) {
totalCell.valueType = VALUE_TYPE_NA
}

this.adaptiveClippingController.add(
{ row, column },
renderValue(
Expand Down Expand Up @@ -1028,10 +1088,19 @@ export class PivotTableEngine {
column,
})
const valueType = dxDimension?.valueType || VALUE_TYPE_TEXT
const totalAggregationType =
dxDimension?.totalAggregationType

// only accumulate numeric (except for PERCENTAGE and UNIT_INTERVAL) and boolean values
// accumulating other value types like text values does not make sense
if (
isCumulativeValueType(valueType) &&
totalAggregationType === AGGREGATE_TYPE_SUM
) {
// initialise to 0 for cumulative types
// (||= is not transformed correctly in Babel with the current setup)
acc || (acc = 0)

// only accumulate numeric values
// accumulating text values does not make sense
if (valueType === VALUE_TYPE_NUMBER) {
if (this.data[row] && this.data[row][column]) {
const dataRow = this.data[row][column]

Expand All @@ -1049,7 +1118,7 @@ export class PivotTableEngine {
}

return acc
}, 0)
}, '')
})
} else {
this.accumulators = { rows: {} }
Expand Down
4 changes: 4 additions & 0 deletions src/modules/pivotTable/pivotTableConstants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ export const AGGREGATE_TYPE_SUM = 'SUM'
export const AGGREGATE_TYPE_AVERAGE = 'AVERAGE'
export const AGGREGATE_TYPE_NA = 'N/A'

export const VALUE_TYPE_NA = 'N_A' // this ends up as CSS class and / is problematic

export const NUMBER_TYPE_VALUE = 'VALUE'
export const NUMBER_TYPE_ROW_PERCENTAGE = 'ROW_PERCENTAGE'
export const NUMBER_TYPE_COLUMN_PERCENTAGE = 'COLUMN_PERCENTAGE'
Expand All @@ -35,3 +37,5 @@ export const WRAPPED_TEXT_JUSTIFY_BUFFER = 25
export const WRAPPED_TEXT_LINE_HEIGHT = 1.0

export const CLIPPED_AXIS_PARTITION_SIZE_PX = 1000

export const VALUE_NA = 'N/A'
11 changes: 11 additions & 0 deletions src/modules/valueTypes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,16 @@ const NUMERIC_VALUE_TYPES = [

const BOOLEAN_VALUE_TYPES = [VALUE_TYPE_BOOLEAN, VALUE_TYPE_TRUE_ONLY]

const CUMULATIVE_VALUE_TYPES = [
VALUE_TYPE_NUMBER,
VALUE_TYPE_INTEGER,
VALUE_TYPE_INTEGER_POSITIVE,
VALUE_TYPE_INTEGER_NEGATIVE,
VALUE_TYPE_INTEGER_ZERO_OR_POSITIVE,
...BOOLEAN_VALUE_TYPES,
]

export const isCumulativeValueType = (type) =>
CUMULATIVE_VALUE_TYPES.includes(type)
export const isNumericValueType = (type) => NUMERIC_VALUE_TYPES.includes(type)
export const isBooleanValueType = (type) => BOOLEAN_VALUE_TYPES.includes(type)

0 comments on commit a2bfd20

Please sign in to comment.