From a2bfd203cb53f174106d8b570cea52cbfc6136f7 Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Fri, 18 Oct 2024 11:43:15 +0200 Subject: [PATCH] fix: compute totals and cumulative values for numeric/boolean types respecting totalAggregationType (DHIS2-9155) (#1700) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- i18n/en.pot | 10 +- package.json | 1 - .../PivotTable/PivotTableValueCell.js | 9 +- .../PivotTable/styles/PivotTable.style.js | 5 + src/modules/pivotTable/PivotTableEngine.js | 127 ++++++++++++++---- src/modules/pivotTable/pivotTableConstants.js | 4 + src/modules/valueTypes.js | 11 ++ 7 files changed, 134 insertions(+), 33 deletions(-) diff --git a/i18n/en.pot b/i18n/en.pot index 2e98715e2..8f0bb1884 100644 --- a/i18n/en.pot +++ b/i18n/en.pot @@ -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" @@ -855,6 +855,9 @@ msgstr "Financial Years" msgid "Years" msgstr "Years" +msgid "Value: {{value}}" +msgstr "Value: {{value}}" + msgid "Bold text" msgstr "Bold text" @@ -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" diff --git a/package.json b/package.json index 49ad4513a..2d024f2e7 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/src/components/PivotTable/PivotTableValueCell.js b/src/components/PivotTable/PivotTableValueCell.js index 78d204f2c..f20fe554d 100644 --- a/src/components/PivotTable/PivotTableValueCell.js +++ b/src/components/PivotTable/PivotTableValueCell.js @@ -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' @@ -74,7 +75,13 @@ export const PivotTableValueCell = ({ { switch (overrideTotalAggregationType || totalAggregationType) { case AGGREGATE_TYPE_NA: - return 'N/A' + return VALUE_NA case AGGREGATE_TYPE_AVERAGE: return ( ((numerator || value) * multiplier) / @@ -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, @@ -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, @@ -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]) @@ -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 @@ -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( @@ -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] @@ -1049,7 +1118,7 @@ export class PivotTableEngine { } return acc - }, 0) + }, '') }) } else { this.accumulators = { rows: {} } diff --git a/src/modules/pivotTable/pivotTableConstants.js b/src/modules/pivotTable/pivotTableConstants.js index 1221972c9..1ab1b290d 100644 --- a/src/modules/pivotTable/pivotTableConstants.js +++ b/src/modules/pivotTable/pivotTableConstants.js @@ -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' @@ -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' diff --git a/src/modules/valueTypes.js b/src/modules/valueTypes.js index 89462b5c6..1097ac84f 100644 --- a/src/modules/valueTypes.js +++ b/src/modules/valueTypes.js @@ -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)