fix: navigating by the changing url ignored download mode and interpr…

…etation id parameters (#3125)

Fixes https://dhis2.atlassian.net/browse/DHIS2-16810

Always dispatch setDownloadMode and setInterpretationId based on the current location parameters.
---------

Co-authored-by: Joseph John Aas Cooper <joe@dhis2.org>
Co-authored-by: Joe Cooper <33054985+cooper-joe@users.noreply.github.com>

cypress/integration/routes.cy.js

@@ -1,7 +1,22 @@
 import { ThematicLayer } from '../elements/thematic_layer.js'
 import { EXTENDED_TIMEOUT } from '../support/util.js'
 
-context('Routes', () => {
+const assertDownloadMode = () => {
+    cy.getByDataTest('download-settings').should('be.visible')
+    cy.get('canvas.maplibregl-canvas').should('be.visible')
+    cy.get('button').contains('Exit download mode').should('be.visible')
+    cy.url().should('include', 'download')
+}
+
+const assertViewMode = () => {
+    cy.get('button').contains('Add layer').should('be.visible')
+    cy.get('canvas.maplibregl-canvas').should('be.visible')
+    cy.getByDataTest('download-settings').should('not.exist')
+    cy.get('button').contains('Exit download mode').should('not.exist')
+    cy.url().should('not.include', 'download')
+}
+
+describe('Routes', () => {
     it('loads root route', () => {
         cy.visit('/', { timeout: 50000 })
         cy.get('canvas', EXTENDED_TIMEOUT).should('be.visible')
@@ -146,9 +161,7 @@ context('Routes', () => {
             .its('response.statusCode')
             .should('eq', 201)
 
-        cy.getByDataTest('download-settings').should('be.visible')
-        cy.get('canvas.maplibregl-canvas').should('be.visible')
-        cy.get('button').contains('Exit download mode').should('be.visible')
+        assertDownloadMode()
     })
 
     it('loads download page currentAnalyticalObject (hash)', () => {
@@ -160,9 +173,7 @@ context('Routes', () => {
 
         cy.contains('button', 'Proceed').click()
 
-        cy.getByDataTest('download-settings').should('be.visible')
-        cy.get('canvas.maplibregl-canvas').should('be.visible')
-        cy.get('button').contains('Exit download mode').should('be.visible')
+        assertDownloadMode()
     })
 
     it('loads download page for new map', () => {
@@ -171,15 +182,65 @@ context('Routes', () => {
         cy.get('canvas.maplibregl-canvas').should('be.visible')
         cy.get('button').contains('Download').click()
 
-        cy.getByDataTest('download-settings').should('be.visible')
-        cy.get('canvas.maplibregl-canvas').should('be.visible')
-        cy.get('button').contains('Exit download mode').should('be.visible')
-        cy.url().should('include', '#/download')
+        assertDownloadMode()
 
         cy.get('button').contains('Exit download mode').click()
 
-        cy.url().should('not.include', 'download')
+        assertViewMode()
+    })
+
+    describe('navigation by url changes', () => {
+        it('navigates to and away from download page', () => {
+            cy.visit('/', EXTENDED_TIMEOUT)
+            assertViewMode()
+
+            cy.visit('/#/ZBjCfSaLSqD/download', EXTENDED_TIMEOUT) //ANC: LLITN coverage district and facility
+            assertDownloadMode()
+
+            cy.visit('/', EXTENDED_TIMEOUT)
+            assertViewMode()
+        })
+
+        it('navigates away from interpretation modal', () => {
+            cy.visit(
+                '/#/ZBjCfSaLSqD?interpretationId=yKqhXZdeJ6a',
+                EXTENDED_TIMEOUT
+            ) //ANC: LLITN coverage district and facility
+
+            cy.getByDataTest('interpretation-modal')
+                .find('h1')
+                .contains(
+                    'Viewing interpretation: ANC: LLITN coverage district and facility'
+                )
+                .should('be.visible')
 
-        cy.get('button').contains('Add layer').should('be.visible')
+            cy.visit('/#/ZBjCfSaLSqD')
+
+            assertViewMode
+        })
+
+        it('navigates from currentAnalyticalObject to saved map in download mode', () => {
+            cy.intercept('**/userDataStore/analytics/settings', {
+                fixture: 'analyticalObject.json',
+            })
+
+            cy.visit('/#/currentAnalyticalObject', EXTENDED_TIMEOUT)
+            cy.get('canvas', EXTENDED_TIMEOUT).should('be.visible')
+
+            cy.contains('button', 'Proceed').click()
+
+            const Layer = new ThematicLayer()
+            Layer.validateCardTitle('ANC 1 Coverage')
+            cy.get('canvas.maplibregl-canvas').should('be.visible')
+
+            // now go to a saved map in download mode
+            cy.visit('/#/eDlFx0jTtV9/download', EXTENDED_TIMEOUT) //ANC: LLITN Cov chiefdom this year
+            assertDownloadMode()
+
+            cy.getByDataTest('download-map-info')
+                .find('h1')
+                .contains('ANC: LLITN Cov Chiefdom this year')
+                .should('be.visible')
+        })
     })
 })

cypress/integration/ui.cy.js

@@ -0,0 +1,39 @@
+import { EXTENDED_TIMEOUT } from '../support/util.js'
+
+const map = {
+    id: 'eDlFx0jTtV9',
+    name: 'ANC: LLITN Cov Chiefdom this year',
+    downloadFileNamePrefix: 'ANC LLITN Cov Chiefdom this year',
+    cardTitle: 'ANC LLITN coverage',
+    cardSelector: 'card-ANCLLITNcoverage',
+}
+
+describe('ui', () => {
+    it('collapses and expands the Layers Panel', () => {
+        cy.visit(`/#/${map.id}`, EXTENDED_TIMEOUT)
+        cy.get('canvas', EXTENDED_TIMEOUT).should('be.visible')
+
+        // check that Layers Panel is visible
+        cy.getByDataTest('add-layer-button').should('be.visible')
+        cy.getByDataTest('layers-panel').should('be.visible')
+        cy.getByDataTest(map.cardSelector).should('be.visible')
+
+        // collapse Layers Panel
+        cy.getByDataTest('layers-toggle-button').click()
+
+        // check that Layers Panel is not visible
+        cy.getByDataTest('add-layer-button').should('be.visible') // this button is always visible
+        // should have class collapsed
+        cy.getByDataTest('layers-panel').should('not.be.visible')
+        cy.getByDataTest('layers-panel').should('have.css', 'width', '0px')
+        cy.getByDataTest(map.cardSelector).should('not.exist')
+
+        // expand Layers Panel
+        cy.getByDataTest('layers-toggle-button').click()
+
+        // check that Layers Panel is visible
+        cy.getByDataTest('add-layer-button').should('be.visible')
+        cy.getByDataTest('layers-panel').should('be.visible')
+        cy.getByDataTest(map.cardSelector).should('be.visible')
+    })
+})

src/actions/download.js

@@ -1,10 +1,5 @@
 import * as types from '../constants/actionTypes.js'
 
-export const setDownloadMode = (payload) => ({
-    type: types.DOWNLOAD_MODE_SET,
-    payload,
-})
-
 export const setDownloadConfig = (payload) => ({
     type: types.DOWNLOAD_CONFIG_SET,
     payload,

src/actions/ui.js

@@ -15,3 +15,11 @@ export const openInterpretationsPanel = () => ({
 export const closeInterpretationsPanel = () => ({
     type: types.INTERPRETATIONS_PANEL_CLOSE,
 })
+
+export const openDownloadMode = () => ({
+    type: types.DOWNLOAD_MODE_OPEN,
+})
+
+export const closeDownloadMode = () => ({
+    type: types.DOWNLOAD_MODE_CLOSE,
+})

src/components/app/App.js

@@ -21,9 +21,7 @@ const App = () => {
         useState(1)
 
     const dataTableOpen = useSelector((state) => !!state.dataTable)
-    const downloadModeOpen = useSelector(
-        (state) => !!state.download.downloadMode
-    )
+    const downloadModeOpen = useSelector((state) => !!state.ui.downloadMode)
     const detailsPanelOpen = useSelector(
         (state) => state.ui.rightPanelOpen && !state.orgUnitProfile
     )

src/components/app/useLoadMap.js

@@ -4,9 +4,9 @@ import log from 'loglevel'
 import { useRef, useEffect, useCallback } from 'react'
 import { useDispatch } from 'react-redux'
 import { setAnalyticalObject } from '../../actions/analyticalObject.js'
-import { setDownloadMode } from '../../actions/download.js'
 import { setInterpretation } from '../../actions/interpretations.js'
 import { newMap, setMap } from '../../actions/map.js'
+import { openDownloadMode, closeDownloadMode } from '../../actions/ui.js'
 import { getFallbackBasemap } from '../../constants/basemaps.js'
 import { CURRENT_AO_KEY } from '../../util/analyticalObject.js'
 import { dataStatisticsMutation } from '../../util/apiDataStatistics.js'
@@ -26,55 +26,51 @@ export const useLoadMap = () => {
 
     const loadMap = useCallback(
         async (hashLocation) => {
-            previousParamsRef.current = getHashUrlParams(hashLocation)
+            const params = getHashUrlParams(hashLocation)
 
-            if (hashLocation.pathname === '/') {
+            if (params.mapId === '') {
                 dispatch(newMap())
-                return
-            }
-
-            const { mapId, isDownload, interpretationId } =
-                previousParamsRef.current
-
-            if (mapId === CURRENT_AO_KEY) {
+            } else if (params.mapId === CURRENT_AO_KEY) {
                 dispatch(newMap())
                 dispatch(setAnalyticalObject(true))
-                if (isDownload) {
-                    dispatch(setDownloadMode(true))
-                }
-                return
-            }
-
-            try {
-                const map = await fetchMap(mapId, engine, defaultBasemap)
-
-                engine.mutate(dataStatisticsMutation, {
-                    variables: { id: mapId },
-                    onError: (error) => log.error('Error: ', error),
-                })
-
-                const basemapConfig =
-                    basemaps.find(({ id }) => id === map.basemap.id) ||
-                    basemaps.find(({ id }) => id === defaultBasemap) ||
-                    getFallbackBasemap()
-
-                dispatch(
-                    setMap({
-                        ...map,
-                        mapViews: addOrgUnitPaths(map.mapViews),
-                        basemap: { ...map.basemap, ...basemapConfig },
+            } else {
+                try {
+                    const map = await fetchMap(
+                        params.mapId,
+                        engine,
+                        defaultBasemap
+                    )
+
+                    engine.mutate(dataStatisticsMutation, {
+                        variables: { id: params.mapId },
+                        onError: (error) => log.error('Error: ', error),
                     })
-                )
 
-                if (interpretationId) {
-                    dispatch(setInterpretation(interpretationId))
-                } else if (isDownload) {
-                    dispatch(setDownloadMode(true))
+                    const basemapConfig =
+                        basemaps.find(({ id }) => id === map.basemap.id) ||
+                        basemaps.find(({ id }) => id === defaultBasemap) ||
+                        getFallbackBasemap()
+
+                    dispatch(
+                        setMap({
+                            ...map,
+                            mapViews: addOrgUnitPaths(map.mapViews),
+                            basemap: { ...map.basemap, ...basemapConfig },
+                        })
+                    )
+                } catch (e) {
+                    log.error(e)
+                    dispatch(newMap())
                 }
-            } catch (e) {
-                log.error(e)
-                dispatch(newMap())
             }
+            if (params.isDownload) {
+                dispatch(openDownloadMode())
+            } else {
+                dispatch(closeDownloadMode())
+            }
+            dispatch(setInterpretation(params.interpretationId))
+
+            previousParamsRef.current = params
         },
         [basemaps, defaultBasemap, dispatch, engine]
     )
@@ -85,27 +81,26 @@ export const useLoadMap = () => {
 
     useEffect(() => {
         const unlisten = history.listen(({ action, location }) => {
-            const {
-                mapId: prevMapId,
-                interpretationId: prevInterpretationId,
-                isDownload: prevIsDownload,
-            } = previousParamsRef.current
-
-            const { mapId, interpretationId, isDownload } =
-                getHashUrlParams(location)
+            const params = getHashUrlParams(location)
 
-            if (action === 'REPLACE' || prevMapId !== mapId) {
+            if (
+                action === 'REPLACE' ||
+                previousParamsRef.current.mapId !== params.mapId
+            ) {
                 loadMap(location)
                 return
             }
 
-            if (isDownload !== prevIsDownload) {
-                dispatch(setDownloadMode(isDownload))
-                previousParamsRef.current.isDownload = isDownload
-            } else if (interpretationId !== prevInterpretationId) {
-                dispatch(setInterpretation(interpretationId))
-                previousParamsRef.current.interpretationId = interpretationId
+            if (params.isDownload !== previousParamsRef.current.isDownload) {
+                if (params.isDownload) {
+                    dispatch(openDownloadMode())
+                } else {
+                    dispatch(closeDownloadMode())
+                }
             }
+            dispatch(setInterpretation(params.interpretationId))
+
+            previousParamsRef.current = params
         })
 
         return () => unlisten && unlisten()

src/components/layers/LayersPanel.js

@@ -33,6 +33,7 @@ const LayersPanel = () => {
             className={cx(styles.layersPanel, {
                 [styles.collapsed]: !layersPanelOpen,
             })}
+            data-test="layers-panel"
         >
             <div className={styles.layersPanelInner}>
                 {layersPanelOpen ? (