From 3f9a722f37cc501cccbd11df16987ad0b720bdb0 Mon Sep 17 00:00:00 2001
From: Edoardo Sabadelli <edoardo@dhis2.org>
Date: Tue, 15 Sep 2020 10:49:09 +0200
Subject: [PATCH 1/3] fix: confirm dialog when navigating to/from
 interpretation

Since no change is actually lost when navigating in and out of an
interpretation's view, remove the confirm dialog.

Also, when answering No to the confirm dialog (meaning i don't want to
navigate to the new URL) restore the previous URL with history.goBack()
so that the app really is in the same state as before the confirm
dialog.
---
 packages/app/src/components/App.js | 45 ++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/packages/app/src/components/App.js b/packages/app/src/components/App.js
index f356981117..1541312a13 100644
--- a/packages/app/src/components/App.js
+++ b/packages/app/src/components/App.js
@@ -72,6 +72,13 @@ export class App extends Component {
         return false
     }
 
+    parseLocation = location => {
+        const pathParts = location.pathname.slice(1).split('/')
+        const id = pathParts[0]
+        const interpretationId = pathParts[2]
+        return { id, interpretationId }
+    }
+
     loadVisualization = async location => {
         const { store } = this.context
 
@@ -79,9 +86,8 @@ export class App extends Component {
             // /currentAnalyticalObject
             // /${id}/
             // /${id}/interpretation/${interpretationId}
-            const pathParts = location.pathname.slice(1).split('/')
-            const id = pathParts[0]
-            const interpretationId = pathParts[2]
+            const { id, interpretationId } = this.parseLocation(location)
+
             const urlContainsCurrentAOKey = id === CURRENT_AO_KEY
 
             if (urlContainsCurrentAOKey) {
@@ -153,18 +159,28 @@ export class App extends Component {
 
         this.unlisten = history.listen(location => {
             const isSaving = location.state?.isSaving
+            const { interpretationId } = this.parseLocation(location)
+
             if (
+                // currently editing
                 getVisualizationState(
                     this.props.visualization,
                     this.props.current
                 ) === STATE_DIRTY &&
-                this.state.locationToConfirm !== location &&
+                // wanting to navigate elsewhere
+                this.state.previousLocation !== location.pathname &&
+                // currently *not* viewing an interpretation
+                !(this.props.interpretation.id || interpretationId) &&
+                // not saving
                 !isSaving
             ) {
                 this.setState({ locationToConfirm: location })
             } else {
+                if (this.state.previousLocation !== location.pathname) {
+                    this.loadVisualization(location)
+                }
+
                 this.setState({ locationToConfirm: null })
-                this.loadVisualization(location)
             }
         })
 
@@ -258,21 +274,27 @@ export class App extends Component {
                             <ButtonStrip end>
                                 <Button
                                     secondary
-                                    onClick={() =>
+                                    onClick={() => {
                                         this.setState({
                                             locationToConfirm: null,
                                         })
-                                    }
+
+                                        history.goBack()
+                                    }}
                                 >
                                     {i18n.t('No, cancel')}
                                 </Button>
 
                                 <Button
-                                    onClick={() =>
-                                        history.push(
+                                    onClick={() => {
+                                        this.loadVisualization(
                                             this.state.locationToConfirm
                                         )
-                                    }
+
+                                        this.setState({
+                                            locationToConfirm: null,
+                                        })
+                                    }}
                                     primary
                                 >
                                     {i18n.t('Yes, leave')}
@@ -291,7 +313,7 @@ export class App extends Component {
 const mapStateToProps = state => ({
     settings: fromReducers.fromSettings.sGetSettings(state),
     current: fromReducers.fromCurrent.sGetCurrent(state),
-    interpretations: fromReducers.fromVisualization.sGetInterpretations(state),
+    interpretation: fromReducers.fromUi.sGetUiInterpretation(state),
     ui: fromReducers.fromUi.sGetUi(state),
     visualization: sGetVisualization(state),
     snackbar: fromReducers.fromSnackbar.sGetSnackbar(state),
@@ -326,6 +348,7 @@ App.propTypes = {
     clearVisualization: PropTypes.func,
     current: PropTypes.object,
     d2: PropTypes.object,
+    interpretation: PropTypes.object,
     location: PropTypes.object,
     ouLevels: PropTypes.array,
     setCurrentFromUi: PropTypes.func,

From 7f8774362e5adb6449145e86dc78eda1b6b9af49 Mon Sep 17 00:00:00 2001
From: Edoardo Sabadelli <edoardo@dhis2.org>
Date: Tue, 15 Sep 2020 11:07:58 +0200
Subject: [PATCH 2/3] fix: make sure vis is reloaded when saving

---
 packages/app/src/components/App.js                | 5 ++++-
 packages/app/src/components/__tests__/App.spec.js | 7 +++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/packages/app/src/components/App.js b/packages/app/src/components/App.js
index 1541312a13..7399e50985 100644
--- a/packages/app/src/components/App.js
+++ b/packages/app/src/components/App.js
@@ -176,7 +176,10 @@ export class App extends Component {
             ) {
                 this.setState({ locationToConfirm: location })
             } else {
-                if (this.state.previousLocation !== location.pathname) {
+                if (
+                    isSaving ||
+                    this.state.previousLocation !== location.pathname
+                ) {
                     this.loadVisualization(location)
                 }
 
diff --git a/packages/app/src/components/__tests__/App.spec.js b/packages/app/src/components/__tests__/App.spec.js
index ec0b09e3d9..56c3f65495 100644
--- a/packages/app/src/components/__tests__/App.spec.js
+++ b/packages/app/src/components/__tests__/App.spec.js
@@ -112,13 +112,16 @@ describe('App', () => {
             })
         })
 
-        it('reloads visualization when same pathname pushed', done => {
+        it('reloads visualization when same pathname pushed when saving', done => {
             props.location.pathname = '/fluttershy'
 
             app()
 
             setTimeout(() => {
-                history.replace('/fluttershy')
+                history.replace({
+                    pathname: '/fluttershy',
+                    state: { isSaving: true },
+                })
                 expect(actions.tDoLoadVisualization).toBeCalledTimes(2)
 
                 done()

From 668da00ee57c8c668334ec82c3b9fd15e4136429 Mon Sep 17 00:00:00 2001
From: Edoardo Sabadelli <edoardo@dhis2.org>
Date: Tue, 15 Sep 2020 11:33:47 +0200
Subject: [PATCH 3/3] fix: make sure the visualization is reloaded when
 re-opening it

---
 packages/app/src/components/App.js               |  2 ++
 packages/app/src/components/MenuBar/MenuBar.js   |  2 +-
 .../app/src/components/__tests__/App.spec.js     | 16 ++++++++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/packages/app/src/components/App.js b/packages/app/src/components/App.js
index 7399e50985..d1c63b1a99 100644
--- a/packages/app/src/components/App.js
+++ b/packages/app/src/components/App.js
@@ -159,6 +159,7 @@ export class App extends Component {
 
         this.unlisten = history.listen(location => {
             const isSaving = location.state?.isSaving
+            const isOpening = location.state?.isOpening
             const { interpretationId } = this.parseLocation(location)
 
             if (
@@ -178,6 +179,7 @@ export class App extends Component {
             } else {
                 if (
                     isSaving ||
+                    isOpening ||
                     this.state.previousLocation !== location.pathname
                 ) {
                     this.loadVisualization(location)
diff --git a/packages/app/src/components/MenuBar/MenuBar.js b/packages/app/src/components/MenuBar/MenuBar.js
index 098a5872fd..649895762b 100644
--- a/packages/app/src/components/MenuBar/MenuBar.js
+++ b/packages/app/src/components/MenuBar/MenuBar.js
@@ -19,7 +19,7 @@ import styles from './styles/MenuBar.module.css'
 const onOpen = id => {
     const path = `/${id}`
     if (history.location.pathname === path) {
-        history.replace(path)
+        history.replace({ pathname: path, state: { isOpening: true } })
     } else {
         history.push(path)
     }
diff --git a/packages/app/src/components/__tests__/App.spec.js b/packages/app/src/components/__tests__/App.spec.js
index 56c3f65495..f34daa3152 100644
--- a/packages/app/src/components/__tests__/App.spec.js
+++ b/packages/app/src/components/__tests__/App.spec.js
@@ -112,6 +112,22 @@ describe('App', () => {
             })
         })
 
+        it('reloads visualization when opening the same visualization', done => {
+            props.location.pathname = '/fluttershy'
+
+            app()
+
+            setTimeout(() => {
+                history.replace({
+                    pathname: '/fluttershy',
+                    state: { isOpening: true },
+                })
+                expect(actions.tDoLoadVisualization).toBeCalledTimes(2)
+
+                done()
+            })
+        })
+
         it('reloads visualization when same pathname pushed when saving', done => {
             props.location.pathname = '/fluttershy'