fix: dashboard map fixes

src/components/plugin/LayerLoader.jsx

@@ -1,7 +1,7 @@
 import { Analytics, useDataOutputPeriodTypes } from '@dhis2/analytics'
 import { useDataEngine, useConfig } from '@dhis2/app-runtime'
 import PropTypes from 'prop-types'
-import { useState, useEffect } from 'react'
+import { useState, useEffect, useRef } from 'react'
 import { THEMATIC_LAYER, EVENT_LAYER } from '../../constants/layers.js'
 import earthEngineLoader from '../../loaders/earthEngineLoader.js'
 import eventLoader from '../../loaders/eventLoader.js'
@@ -38,6 +38,7 @@ const LayerLoader = ({ config, onLoad }) => {
         userOrgUnitIdsByKeyword,
     } = currentUser
     const periodTypeData = useDataOutputPeriodTypes()
+    const loadedConfig = useRef(null)
 
     useEffect(() => {
         if (
@@ -47,6 +48,11 @@ const LayerLoader = ({ config, onLoad }) => {
         ) {
             return
         }
+        // Loaders mutate config, so never load the same object twice
+        if (loadedConfig.current === config) {
+            return
+        }
+        loadedConfig.current = config
         const loader = loaders[config.layer]
         loader({
             config,

src/components/plugin/Map.jsx

@@ -140,14 +140,7 @@ const Map = forwardRef((props, ref) => {
     }
 
     if (!mapIsLoaded) {
-        const layersToLoad = layers.current.filter(
-            (config) => !config.isLoaded && !config.isLoading
-        )
-        layers.current = layers.current.map((layer) =>
-            layersToLoad.find((l) => l.id === layer.id)
-                ? { ...layer, isLoading: true }
-                : layer
-        )
+        const layersToLoad = layers.current.filter((config) => !config.isLoaded)
         return (
             <CenteredContent>
                 <CircularLoader />

src/components/plugin/__tests__/LayerLoader.spec.jsx

@@ -0,0 +1,99 @@
+import { useDataOutputPeriodTypes } from '@dhis2/analytics'
+import { render, waitFor } from '@testing-library/react'
+import React from 'react'
+import earthEngineLoader from '../../../loaders/earthEngineLoader.js'
+import thematicLoader from '../../../loaders/thematicLoader.js'
+import LayerLoader from '../LayerLoader.jsx'
+
+jest.mock('@dhis2/analytics', () => ({
+    Analytics: { getAnalytics: jest.fn(() => ({})) },
+    useDataOutputPeriodTypes: jest.fn(),
+}))
+jest.mock('@dhis2/app-runtime', () => ({
+    useConfig: () => ({ baseUrl: '', serverVersion: { minor: 43 } }),
+    useDataEngine: () => ({}),
+}))
+jest.mock('../../cachedDataProvider/CachedDataProvider.jsx', () => ({
+    useCachedData: () => ({
+        systemSettings: { keyAnalysisDigitGroupSeparator: 'SPACE' },
+        currentUser: {
+            keyAnalysisDisplayProperty: 'name',
+            id: 'user',
+            userOrgUnitIdsByKeyword: {},
+        },
+    }),
+}))
+jest.mock('../../../loaders/earthEngineLoader.js', () => jest.fn())
+jest.mock('../../../loaders/thematicLoader.js', () => jest.fn())
+
+const pendingPeriodTypes = {
+    supportsEnabledPeriodTypes: true,
+    enabledPeriodTypesData: null,
+}
+const loadedPeriodTypes = {
+    supportsEnabledPeriodTypes: true,
+    enabledPeriodTypesData: { enabledTypes: [] },
+}
+
+describe('LayerLoader', () => {
+    beforeEach(() => {
+        jest.clearAllMocks()
+        earthEngineLoader.mockResolvedValue({ id: 'ee', isLoaded: true })
+        thematicLoader.mockResolvedValue({ id: 'th', isLoaded: true })
+    })
+
+    test('loads an earth engine layer once when period types resolve later', async () => {
+        const config = { id: 'ee', layer: 'earthEngine' }
+        const onLoad = jest.fn()
+
+        useDataOutputPeriodTypes.mockReturnValue(pendingPeriodTypes)
+        const { rerender } = render(
+            <LayerLoader config={config} onLoad={onLoad} />
+        )
+
+        useDataOutputPeriodTypes.mockReturnValue(loadedPeriodTypes)
+        rerender(<LayerLoader config={config} onLoad={onLoad} />)
+
+        await waitFor(() => expect(onLoad).toHaveBeenCalled())
+        expect(earthEngineLoader).toHaveBeenCalledTimes(1)
+        expect(onLoad).toHaveBeenCalledTimes(1)
+    })
+
+    test('waits for period types before loading a thematic layer', async () => {
+        const config = { id: 'th', layer: 'thematic' }
+        const onLoad = jest.fn()
+
+        useDataOutputPeriodTypes.mockReturnValue(pendingPeriodTypes)
+        const { rerender } = render(
+            <LayerLoader config={config} onLoad={onLoad} />
+        )
+        expect(thematicLoader).not.toHaveBeenCalled()
+
+        useDataOutputPeriodTypes.mockReturnValue(loadedPeriodTypes)
+        rerender(<LayerLoader config={config} onLoad={onLoad} />)
+
+        await waitFor(() => expect(onLoad).toHaveBeenCalled())
+        expect(thematicLoader).toHaveBeenCalledTimes(1)
+    })
+
+    test('reloads when a new config is passed', async () => {
+        const onLoad = jest.fn()
+        useDataOutputPeriodTypes.mockReturnValue(loadedPeriodTypes)
+
+        const { rerender } = render(
+            <LayerLoader
+                config={{ id: 'ee', layer: 'earthEngine' }}
+                onLoad={onLoad}
+            />
+        )
+        rerender(
+            <LayerLoader
+                config={{ id: 'ee', layer: 'earthEngine' }}
+                onLoad={onLoad}
+            />
+        )
+
+        await waitFor(() => expect(onLoad).toHaveBeenCalledTimes(2))
+        expect(earthEngineLoader).toHaveBeenCalledTimes(2)
+    })
+})

src/components/plugin/__tests__/Map.spec.jsx

@@ -0,0 +1,85 @@
+/* eslint-disable react/prop-types */
+import { render, screen, act } from '@testing-library/react'
+import React from 'react'
+import Map from '../Map.jsx'
+
+const mockMounted = new Set()
+const mockOnLoad = {}
+const mockLoads = {}
+
+jest.mock('../LayerLoader.jsx', () => {
+    const { useEffect } = require('react')
+    const LayerLoader = ({ config, onLoad }) => {
+        // LayerLoader loads each config object it receives once
+        useEffect(() => {
+            mockLoads[config.id] = (mockLoads[config.id] || 0) + 1
+        }, [config])
+        useEffect(() => {
+            mockMounted.add(config.id)
+            mockOnLoad[config.id] = onLoad
+            return () => mockMounted.delete(config.id)
+        }, [config, onLoad])
+        return null
+    }
+    return LayerLoader
+})
+jest.mock('../../map/MapView.jsx', () => {
+    const MapView = () => <div>MapView</div>
+    return MapView
+})
+jest.mock('../Legend.jsx', () => () => null)
+jest.mock('../ContextMenu.jsx', () => () => null)
+
+const mapViews = [
+    { id: 'ee', layer: 'earthEngine' },
+    { id: 'th', layer: 'thematic', filters: [], rows: [] },
+]
+
+describe('Plugin Map', () => {
+    beforeEach(() => {
+        mockMounted.clear()
+        Object.keys(mockLoads).forEach((id) => delete mockLoads[id])
+    })
+
+    test('keeps loaders mounted for pending layers (e.g. waiting for period types) when re-rendered', () => {
+        render(<Map mapViews={mapViews} basemap={{}} />)
+        expect([...mockMounted].sort()).toEqual(['ee', 'th'])
+
+        act(() => mockOnLoad.ee({ id: 'ee', isLoaded: true }))
+        act(() => window.dispatchEvent(new Event('resize')))
+
+        expect(mockMounted.has('th')).toBe(true)
+
+        act(() => mockOnLoad.th({ id: 'th', isLoaded: true }))
+        expect(screen.getByText('MapView')).toBeInTheDocument()
+    })
+
+    test('loads each layer once when re-rendered while loading', () => {
+        const views = [
+            { id: 'ee', layer: 'earthEngine' },
+            { id: 'fa', layer: 'facility', filters: [], rows: [] },
+            { id: 'th', layer: 'thematic', filters: [], rows: [] },
+        ]
+        // Props from the dashboard arrive as fresh copies of the same views
+        const copyViews = () => JSON.parse(JSON.stringify(views))
+        const loaded = (view) => ({ ...view, isLoaded: true })
+        const resize = () =>
+            act(() => window.dispatchEvent(new Event('resize')))
+
+        const { rerender } = render(<Map mapViews={views} basemap={{}} />)
+        resize()
+        rerender(<Map mapViews={copyViews()} basemap={{}} />)
+
+        act(() => mockOnLoad.ee(loaded(views[0])))
+        resize()
+        rerender(<Map mapViews={copyViews()} basemap={{}} />)
+
+        act(() => mockOnLoad.fa(loaded(views[1])))
+        resize()
+
+        expect(mockLoads).toEqual({ ee: 1, fa: 1, th: 1 })
+
+        act(() => mockOnLoad.th(loaded(views[2])))
+        expect(screen.getByText('MapView')).toBeInTheDocument()
+    })
+})

src/components/plugin/styles/Legend.css

@@ -6,6 +6,14 @@
     box-shadow: 0 1px 5px rgba(0, 0, 0, 0.65);
     background-color: #fff;
     font-size: 14px;
+    display: flex;
+    flex-direction: column;
+}
+
+.dhis2-map-legend > div {
+    display: flex;
+    flex-direction: column;
+    min-height: 0;
 }
 
 .dhis2-map-legend.pinned {
@@ -27,6 +35,7 @@
     min-width: 150px;
     max-width: 260px;
     max-height: 400px;
+    min-height: 0;
     overflow-y: auto;
     padding: 6px 6px 0;
     color: #333;