From 25d9700a9a4cb5e30b5794a6a5269de88e1d0079 Mon Sep 17 00:00:00 2001 From: braimbault Date: Tue, 10 Dec 2024 14:43:54 +0100 Subject: [PATCH] fix: refactor RenderingStrategy and add test --- src/components/periods/RenderingStrategy.js | 198 +++++++----------- .../__tests__/RenderingStrategy.spec.js | 88 ++++++++ 2 files changed, 169 insertions(+), 117 deletions(-) create mode 100644 src/components/periods/__tests__/RenderingStrategy.spec.js diff --git a/src/components/periods/RenderingStrategy.js b/src/components/periods/RenderingStrategy.js index f8aa0b2ab..5a8403123 100644 --- a/src/components/periods/RenderingStrategy.js +++ b/src/components/periods/RenderingStrategy.js @@ -1,7 +1,6 @@ -import { getRelativePeriodsDetails } from '@dhis2/analytics' import i18n from '@dhis2/d2-i18n' import PropTypes from 'prop-types' -import React, { useEffect } from 'react' +import React, { useEffect, useMemo } from 'react' import { useSelector } from 'react-redux' import { RENDERING_STRATEGY_SINGLE, @@ -15,59 +14,7 @@ import { import usePrevious from '../../hooks/usePrevious.js' import { getPeriodsFromFilters } from '../../util/analytics.js' import { Radio, RadioGroup } from '../core/index.js' - -const countPeriods = (periods) => { - const periodsDetails = getRelativePeriodsDetails() - - const total_v1 = periods.reduce( - (sum, period) => - sum + - (periodsDetails[period.id] !== undefined - ? periodsDetails[period.id].duration - : 1), - 0 - ) - - const durationByType = periods.reduce((acc, period) => { - const periodDetails = periodsDetails[period.id] - if (acc['FIXED_PERIOD'] === undefined) { - acc['FIXED_PERIOD'] = { - any: 0, - } - } - if (periodDetails === undefined) { - acc['FIXED_PERIOD'].any += 1 - return acc - } - const type = periodDetails.type - if (acc[type] === undefined) { - acc[type] = { - first: 0, - last: 0, - } - } - acc[type].first = Math.max(acc[type].first, 1 + periodDetails.offset) - acc[type].last = Math.max( - acc[type].last, - periodDetails.duration - (1 + periodDetails.offset) - ) - return acc - }, {}) - - const sumObjectValues = (obj) => - Object.values(obj).reduce((sum, value) => { - if (typeof value === 'object') { - return sum + sumObjectValues(value) - } else if (typeof value === 'number') { - return sum + value - } - return sum - }, 0) - - const total_v2 = sumObjectValues(durationByType) - - return total_v2 -} +import { countPeriods } from '../../util/periods.js' const RenderingStrategy = ({ layerId, @@ -75,69 +22,90 @@ const RenderingStrategy = ({ periods = [], onChange, }) => { - const hasOtherLayers = useSelector( - ({ map }) => !!map.mapViews.filter(({ id }) => id !== layerId).length + const prevPeriods = usePrevious(periods) + const totalPeriods = useMemo(() => countPeriods(periods), [periods]) + + const hasOtherLayers = useSelector(({ map }) => + map.mapViews.some(({ id }) => id !== layerId) ) - const hasOtherTimelineLayers = useSelector( - ({ map }) => - !!map.mapViews.find( - (layer) => - layer.renderingStrategy === RENDERING_STRATEGY_TIMELINE && - layer.id !== layerId - ) + const hasOtherTimelineLayers = useSelector(({ map }) => + map.mapViews.some( + (layer) => + layer.renderingStrategy === RENDERING_STRATEGY_TIMELINE && + layer.id !== layerId + ) ) const hasTooManyPeriods = useSelector(({ layerEdit }) => { const periods = getPeriodsFromFilters(layerEdit.filters) return countPeriods(periods) > MULTIMAP_MAX_PERIODS }) - const prevPeriods = usePrevious(periods) - useEffect(() => { - if (periods !== prevPeriods) { - if ( - countPeriods(periods) < MULTIMAP_MIN_PERIODS && - value !== RENDERING_STRATEGY_SINGLE - ) { - onChange(RENDERING_STRATEGY_SINGLE) - } else if ( - countPeriods(periods) > MULTIMAP_MAX_PERIODS && - value === RENDERING_STRATEGY_SPLIT_BY_PERIOD - ) { - onChange(RENDERING_STRATEGY_SINGLE) - } + if (periods === prevPeriods) return + + if ( + totalPeriods < MULTIMAP_MIN_PERIODS && + value !== RENDERING_STRATEGY_SINGLE + ) { + onChange(RENDERING_STRATEGY_SINGLE) + } else if ( + totalPeriods > MULTIMAP_MAX_PERIODS && + value === RENDERING_STRATEGY_SPLIT_BY_PERIOD + ) { + onChange(RENDERING_STRATEGY_SINGLE) } }, [value, periods, prevPeriods, onChange]) - let helpText = [] - - if (countPeriods(periods) < MULTIMAP_MIN_PERIODS) { - helpText.push( - i18n.t( - 'Select {{number}} or more periods to enable timeline or split map views.', - { - number: MULTIMAP_MIN_PERIODS, - } + const helpText = useMemo(() => { + const messages = [] + if (totalPeriods < MULTIMAP_MIN_PERIODS) { + messages.push( + i18n.t( + 'Select {{number}} or more periods to enable timeline or split map views.', + { + number: MULTIMAP_MIN_PERIODS, + } + ) ) - ) - } - if (hasOtherTimelineLayers) { - helpText.push(i18n.t('Only one timeline is allowed.')) - } - if (hasOtherLayers) { - helpText.push(i18n.t('Remove other layers to enable split map views.')) - } - if (hasTooManyPeriods) { - helpText.push( - i18n.t( - 'Only up to {{number}} periods can be selected to enable split map views.', - { - number: MULTIMAP_MAX_PERIODS, - } + } + if (hasOtherTimelineLayers) { + messages.push(i18n.t('Only one timeline is allowed.')) + } + if (hasOtherLayers) { + messages.push( + i18n.t('Remove other layers to enable split map views.') ) - ) - } - helpText = helpText.join(' ') + } + if (hasTooManyPeriods) { + messages.push( + i18n.t( + 'Only up to {{number}} periods can be selected to enable split map views.', + { + number: MULTIMAP_MAX_PERIODS, + } + ) + ) + } + return messages.join(' ') + }, [ + totalPeriods, + hasOtherTimelineLayers, + hasOtherLayers, + hasTooManyPeriods, + ]) + + const isTimelineDisabled = useMemo( + () => totalPeriods < MULTIMAP_MIN_PERIODS || hasOtherTimelineLayers, + [totalPeriods, hasOtherTimelineLayers] + ) + + const isSplitViewDisabled = useMemo( + () => + totalPeriods < MULTIMAP_MIN_PERIODS || + hasTooManyPeriods || + hasOtherLayers, + [totalPeriods, hasTooManyPeriods, hasOtherLayers] + ) return ( - + ) diff --git a/src/components/periods/__tests__/RenderingStrategy.spec.js b/src/components/periods/__tests__/RenderingStrategy.spec.js new file mode 100644 index 000000000..01295e767 --- /dev/null +++ b/src/components/periods/__tests__/RenderingStrategy.spec.js @@ -0,0 +1,88 @@ +import { mount } from 'enzyme' +import React from 'react' +import { Provider } from 'react-redux' +import configureMockStore from 'redux-mock-store' +import RenderingStrategy from '../RenderingStrategy' +import { + RENDERING_STRATEGY_SINGLE, + RENDERING_STRATEGY_TIMELINE, + RENDERING_STRATEGY_SPLIT_BY_PERIOD, +} from '../../../constants/layers' +import { countPeriods } from '../../../util/periods' + +const mockStore = configureMockStore() + +const store = mockStore({ + map: { + mapViews: [{ id: 'layer1', renderingStrategy: 'SINGLE' }], + }, + layerEdit: { + filters: [], + }, +}) + +jest.mock('../../../util/periods', () => ({ + countPeriods: jest.fn(), +})) + +describe('RenderingStrategy', () => { + const renderWithProps = (props) => + mount( + + + + ) + + const layerId = 'layer1' + const value = RENDERING_STRATEGY_SPLIT_BY_PERIOD + const periods = [] + const mockOnChange = jest.fn() + let props + + beforeEach(() => { + jest.clearAllMocks() + props = { + layerId, + value, + periods, + onChange: mockOnChange, + } + }) + + it('renders all radio buttons with correct labels', () => { + countPeriods.mockReturnValue(5) + const wrapper = renderWithProps(props) + expect(wrapper.find('input').length).toBe(3) // Three radio buttons + expect(wrapper.find('label').at(0).text()).toBe('Period display mode') + expect(wrapper.find('label').at(1).text()).toBe( + 'Single (combine periods)' + ) + expect(wrapper.find('label').at(2).text()).toBe('Timeline') + expect(wrapper.find('label').at(3).text()).toBe('Split map views') + }) + + it('disables timeline and split map views when total periods are below the minimum', () => { + countPeriods.mockReturnValue(1) + const wrapper = renderWithProps(props) + expect(wrapper.find('input').at(1).prop('disabled')).toBeDefined() // Timeline should be disabled + expect(wrapper.find('input').at(2).prop('disabled')).toBeDefined() + }) + + it('calls onChange with correct value when a radio button is clicked', () => { + countPeriods.mockReturnValue(5) + const wrapper = renderWithProps(props) + wrapper + .find('input') + .at(1) + .simulate('change', { + target: { value: RENDERING_STRATEGY_TIMELINE }, + }) + expect(mockOnChange).toHaveBeenCalledWith(RENDERING_STRATEGY_TIMELINE) + }) + + it('automatically switches to SINGLE when conditions are not met', () => { + countPeriods.mockReturnValue(1) + const wrapper = renderWithProps(props) + expect(mockOnChange).toHaveBeenCalledWith(RENDERING_STRATEGY_SINGLE) + }) +})