From 178e39eada3c25c133cd48ed823b75951352e211 Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Fri, 13 Sep 2024 17:52:09 -0600
Subject: [PATCH 01/10] Handle & display error on save paramset failure

---
 src/natcap/invest/datastack.py                | 20 +++++++--
 src/natcap/invest/ui_server.py                | 21 +++++++--
 tests/test_ui_server.py                       |  5 ++-
 .../renderer/components/SaveAsModal/index.jsx |  1 +
 .../renderer/components/SetupTab/index.jsx    | 44 +++++++++++++++----
 workbench/src/renderer/server_requests.js     | 12 +++--
 workbench/tests/renderer/investtab.test.js    |  7 ++-
 7 files changed, 86 insertions(+), 24 deletions(-)

diff --git a/src/natcap/invest/datastack.py b/src/natcap/invest/datastack.py
index 461a12d60e..a68b153078 100644
--- a/src/natcap/invest/datastack.py
+++ b/src/natcap/invest/datastack.py
@@ -535,6 +535,9 @@ def build_parameter_set(args, model_name, paramset_path, relative=False):
 
     Returns:
         ``None``
+
+    Raises:
+        ValueError if creating a relative path fails.
     """
     def _recurse(args_param):
         if isinstance(args_param, dict):
@@ -552,10 +555,19 @@ def _recurse(args_param):
                     if (normalized_path == '.' or
                             os.path.dirname(paramset_path) == normalized_path):
                         return '.'
-                    temp_rel_path = os.path.relpath(
-                        normalized_path, os.path.dirname(paramset_path))
-                    # Always save unix paths.
-                    linux_style_path = temp_rel_path.replace('\\', '/')
+                    try:
+                        temp_rel_path = os.path.relpath(
+                            normalized_path, os.path.dirname(paramset_path))
+                    except ValueError:
+                        # On Windows, ValueError is raised when ``path`` and
+                        # ``start`` are on different drives
+                        raise ValueError(
+                            """Error: Cannot save datastack with relative
+                            paths across drives. Choose a different save
+                            location, or use absolute paths.""")
+                    else:
+                        # Always save unix paths.
+                        linux_style_path = temp_rel_path.replace('\\', '/')
                 else:
                     # Always save unix paths.
                     linux_style_path = normalized_path.replace('\\', '/')
diff --git a/src/natcap/invest/ui_server.py b/src/natcap/invest/ui_server.py
index bfbe1874ec..f7be83d66f 100644
--- a/src/natcap/invest/ui_server.py
+++ b/src/natcap/invest/ui_server.py
@@ -175,7 +175,9 @@ def write_parameter_set_file():
         relativePaths: boolean
 
     Returns:
-        A string.
+        A dictionary with the following key/value pairs:
+        - message (string): for logging and/or rendering in the UI.
+        - error (boolean): True if an error occurred, otherwise False.
     """
     payload = request.get_json()
     filepath = payload['filepath']
@@ -183,9 +185,20 @@ def write_parameter_set_file():
     args = json.loads(payload['args'])
     relative_paths = payload['relativePaths']
 
-    datastack.build_parameter_set(
-        args, modulename, filepath, relative=relative_paths)
-    return 'parameter set saved'
+    try:
+        datastack.build_parameter_set(
+            args, modulename, filepath, relative=relative_paths)
+    except ValueError as message:
+        LOGGER.error(str(message))
+        return {
+            'message': str(message),
+            'error': True
+        }
+    else:
+        return {
+            'message': 'Parameter set saved',
+            'error': False
+        }
 
 
 @app.route(f'/{PREFIX}/save_to_python', methods=['POST'])
diff --git a/tests/test_ui_server.py b/tests/test_ui_server.py
index 426dc57005..15e93bbd4b 100644
--- a/tests/test_ui_server.py
+++ b/tests/test_ui_server.py
@@ -122,8 +122,11 @@ def test_write_parameter_set_file(self):
             }),
             'relativePaths': True,
         }
-        _ = test_client.post(
+        response = test_client.post(
             f'{ROUTE_PREFIX}/write_parameter_set_file', json=payload)
+        self.assertEqual(
+            response.json,
+            {'message': 'Parameter set saved', 'error': False})
         with open(filepath, 'r') as file:
             actual_data = json.loads(file.read())
         self.assertEqual(
diff --git a/workbench/src/renderer/components/SaveAsModal/index.jsx b/workbench/src/renderer/components/SaveAsModal/index.jsx
index 06707a846d..a2e5e9917e 100644
--- a/workbench/src/renderer/components/SaveAsModal/index.jsx
+++ b/workbench/src/renderer/components/SaveAsModal/index.jsx
@@ -67,6 +67,7 @@ class SaveAsModal extends React.Component {
   }
 
   handleShow() {
+    this.props.removeSaveErrors();
     this.setState({
       relativePaths: false,
       show: true,
diff --git a/workbench/src/renderer/components/SetupTab/index.jsx b/workbench/src/renderer/components/SetupTab/index.jsx
index c241dd5706..5914c66ff4 100644
--- a/workbench/src/renderer/components/SetupTab/index.jsx
+++ b/workbench/src/renderer/components/SetupTab/index.jsx
@@ -100,6 +100,7 @@ class SetupTab extends React.Component {
     this.savePythonScript = this.savePythonScript.bind(this);
     this.saveJsonFile = this.saveJsonFile.bind(this);
     this.setSaveAlert = this.setSaveAlert.bind(this);
+    this.removeSaveErrors = this.removeSaveErrors.bind(this);
     this.wrapInvestExecute = this.wrapInvestExecute.bind(this);
     this.investValidate = this.investValidate.bind(this);
     this.debouncedValidate = this.debouncedValidate.bind(this);
@@ -234,8 +235,8 @@ class SetupTab extends React.Component {
       relativePaths: relativePaths,
       args: JSON.stringify(args),
     };
-    const response = await writeParametersToFile(payload);
-    this.setSaveAlert(response);
+    const {message, error} = await writeParametersToFile(payload);
+    this.setSaveAlert(message, error);
   }
 
   async saveDatastack(datastackPath) {
@@ -249,9 +250,9 @@ class SetupTab extends React.Component {
       args: JSON.stringify(args),
     };
     const key = window.crypto.getRandomValues(new Uint16Array(1))[0].toString();
-    this.setSaveAlert('archiving...', key);
+    this.setSaveAlert('archiving...', false, key);
     const response = await archiveDatastack(payload);
-    this.setSaveAlert(response, key);
+    this.setSaveAlert(response, false, key);
   }
 
   /** State updater for alert messages from various save buttons.
@@ -262,15 +263,35 @@ class SetupTab extends React.Component {
    *        1. display: because a new save occurred, or
    *        2. not display: on a re-render after `Expire` expired, or
    *        3. update: because 'archiving...' alert changes to final message
+   * @param {boolean} error - true if message was generated by an error,
+   *        false otherwise. Defaults to false.
    *
    * @returns {undefined}
    */
   setSaveAlert(
     message,
+    error = false,
     key = window.crypto.getRandomValues(new Uint16Array(1))[0].toString()
   ) {
     this.setState({
-      saveAlerts: { ...this.state.saveAlerts, ...{ [key]: message } }
+      saveAlerts: {
+        ...this.state.saveAlerts,
+        ...{ [key]: {
+          message,
+          error
+      }}}
+    });
+  }
+
+  removeSaveErrors() {
+    const alerts = this.state.saveAlerts;
+    Object.keys(alerts).forEach((key) => {
+      if (alerts[key].error) {
+        delete alerts[key];
+      }
+    });
+    this.setState({
+      saveAlerts: alerts
     });
   }
 
@@ -490,18 +511,22 @@ class SetupTab extends React.Component {
 
       const SaveAlerts = [];
       Object.keys(saveAlerts).forEach((key) => {
-        const message = saveAlerts[key];
+        const { message, error } = saveAlerts[key];
         if (message) {
           // Alert won't expire during archiving; will expire 2s after completion
-          const alertExpires = (message === 'archiving...') ? 1e7 : 2000;
+          // Alert won't expire when an error has occurred;
+          // will be hidden next time save modal opens
+          const alertExpires = (error || message === 'archiving...') ? 1e7 : 2000;
           SaveAlerts.push(
             <Expire
               key={key}
               className="d-inline"
               delay={alertExpires}
             >
-              <Alert variant="success">
-                {message}
+              <Alert
+                variant={error ? 'danger' : 'success'}
+              >
+                {t(message)}
               </Alert>
             </Expire>
           );
@@ -564,6 +589,7 @@ class SetupTab extends React.Component {
               savePythonScript={this.savePythonScript}
               saveJsonFile={this.saveJsonFile}
               saveDatastack={this.saveDatastack}
+              removeSaveErrors={this.removeSaveErrors}
             />
             <React.Fragment>
               {SaveAlerts}
diff --git a/workbench/src/renderer/server_requests.js b/workbench/src/renderer/server_requests.js
index 286bcd690b..90ca4125aa 100644
--- a/workbench/src/renderer/server_requests.js
+++ b/workbench/src/renderer/server_requests.js
@@ -173,10 +173,14 @@ export function writeParametersToFile(payload) {
       body: JSON.stringify(payload),
       headers: { 'Content-Type': 'application/json' },
     })
-      .then((response) => response.text())
-      .then((text) => {
-        logger.debug(text);
-        return text;
+      .then((response) => response.json())
+      .then(({message, error}) => {
+        if (error) {
+          logger.error(message);
+        } else {
+          logger.debug(message);
+        }
+        return {message, error};
       })
       .catch((error) => logger.error(error.stack))
   );
diff --git a/workbench/tests/renderer/investtab.test.js b/workbench/tests/renderer/investtab.test.js
index 2a125fbddc..e3b6afdc07 100644
--- a/workbench/tests/renderer/investtab.test.js
+++ b/workbench/tests/renderer/investtab.test.js
@@ -217,7 +217,10 @@ describe('Sidebar Buttons', () => {
   });
 
   test('Save to JSON: requests endpoint with correct payload', async () => {
-    const response = 'saved';
+    const response = {
+      message: 'saved',
+      error: false,
+    };
     writeParametersToFile.mockResolvedValue(response);
     const mockDialogData = { canceled: false, filePath: 'foo.json' };
     ipcRenderer.invoke.mockResolvedValueOnce(mockDialogData);
@@ -230,7 +233,7 @@ describe('Sidebar Buttons', () => {
     const saveButton = await findByRole('button', { name: 'Save' });
     await userEvent.click(saveButton);
 
-    expect(await findByRole('alert')).toHaveTextContent(response);
+    expect(await findByRole('alert')).toHaveTextContent(response.message);
     const payload = writeParametersToFile.mock.calls[0][0];
     expect(Object.keys(payload)).toEqual(expect.arrayContaining(
       ['filepath', 'moduleName', 'relativePaths', 'args']

From cf56816a32ba7dd64b3ce52fecff17b92aa88bfc Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Mon, 16 Sep 2024 14:18:19 -0600
Subject: [PATCH 02/10] Update Python paramset tests

---
 tests/test_datastack.py | 22 +++++++++++++++++++++-
 tests/test_ui_server.py | 24 ++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/tests/test_datastack.py b/tests/test_datastack.py
index 45d2e590bd..a471b8e5e3 100644
--- a/tests/test_datastack.py
+++ b/tests/test_datastack.py
@@ -6,10 +6,10 @@
 import pprint
 import shutil
 import sys
-import tarfile
 import tempfile
 import textwrap
 import unittest
+from unittest.mock import patch
 
 import numpy
 import pandas
@@ -506,6 +506,26 @@ def test_relative_parameter_set(self):
         self.assertEqual(invest_version, __version__)
         self.assertEqual(callable_name, modelname)
 
+    def test_relative_path_failure(self):
+        """Datastack: raise error when relative path creation fails."""
+        from natcap.invest import datastack
+
+        params = {
+            'data_dir': os.path.join(self.workspace, 'data_dir'),
+        }
+        modelname = 'natcap.invest.foo'
+        paramset_filename = os.path.join(self.workspace, 'paramset.json')
+
+        # make the sample data so filepaths are interpreted correctly
+        os.makedirs(params['data_dir'])
+
+        # Call build_parameter_set and force it into an error state
+        with self.assertRaises(ValueError):
+            with patch('natcap.invest.os.path.relpath',
+                       side_effect=ValueError):
+                datastack.build_parameter_set(
+                    params, modelname, paramset_filename, relative=True)
+
     @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows")
     def test_relative_parameter_set_windows(self):
         """Datastack: test relative parameter set paths saved linux style."""
diff --git a/tests/test_ui_server.py b/tests/test_ui_server.py
index 15e93bbd4b..02d9436ebe 100644
--- a/tests/test_ui_server.py
+++ b/tests/test_ui_server.py
@@ -133,6 +133,30 @@ def test_write_parameter_set_file(self):
             set(actual_data),
             {'args', 'invest_version', 'model_name'})
 
+    def test_write_parameter_set_file_error_handling(self):
+        """UI server: write_parameter_set_file endpoint
+        should catch a ValueError and return an error message.
+        """
+        test_client = ui_server.app.test_client()
+        self.workspace_dir = tempfile.mkdtemp()
+        filepath = os.path.join(self.workspace_dir, 'datastack.json')
+        payload = {
+            'filepath': filepath,
+            'moduleName': 'natcap.invest.carbon',
+            'args': json.dumps({
+                'workspace_dir': 'foo'
+            }),
+            'relativePaths': True,
+        }
+        error_message = 'Error saving datastack'
+        with patch('natcap.invest.datastack.build_parameter_set',
+                   side_effect=ValueError(error_message)):
+            response = test_client.post(
+                f'{ROUTE_PREFIX}/write_parameter_set_file', json=payload)
+            self.assertEqual(
+                response.json,
+                {'message': error_message, 'error': True})
+
     def test_save_to_python(self):
         """UI server: save_to_python endpoint."""
         test_client = ui_server.app.test_client()

From 73f283feaf95f78d1ef0f95beaca4b91c2d579d3 Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Mon, 16 Sep 2024 16:22:26 -0600
Subject: [PATCH 03/10] Handle & display error on save datastack failure

---
 src/natcap/invest/datastack.py                | 12 +++++--
 src/natcap/invest/ui_server.py                | 27 +++++++++++----
 tests/test_datastack.py                       | 19 +++++++++++
 tests/test_ui_server.py                       | 33 ++++++++++++++++++-
 .../renderer/components/SetupTab/index.jsx    |  4 +--
 workbench/src/renderer/server_requests.js     | 12 ++++---
 workbench/tests/renderer/investtab.test.js    |  7 ++--
 7 files changed, 96 insertions(+), 18 deletions(-)

diff --git a/src/natcap/invest/datastack.py b/src/natcap/invest/datastack.py
index a68b153078..da0a7eca2b 100644
--- a/src/natcap/invest/datastack.py
+++ b/src/natcap/invest/datastack.py
@@ -226,6 +226,10 @@ def build_datastack_archive(args, model_name, datastack_path):
 
     Returns:
         ``None``
+
+    Raises:
+        ValueError if raised by build_parameter_set
+        (i.e., if creating a relative path fails).
     """
     module = importlib.import_module(name=model_name)
 
@@ -453,8 +457,12 @@ def build_datastack_archive(args, model_name, datastack_path):
     # write parameters to a new json file in the temp workspace
     param_file_uri = os.path.join(temp_workspace,
                                   'parameters' + PARAMETER_SET_EXTENSION)
-    build_parameter_set(
-        rewritten_args, model_name, param_file_uri, relative=True)
+    try:
+        build_parameter_set(
+            rewritten_args, model_name, param_file_uri, relative=True)
+    except ValueError as message:
+        # Pass through for handling by ui_server
+        raise ValueError(message)
 
     # Remove the handler before archiving the working dir (and the logfile)
     archive_filehandler.close()
diff --git a/src/natcap/invest/ui_server.py b/src/natcap/invest/ui_server.py
index f7be83d66f..dd009c3a16 100644
--- a/src/natcap/invest/ui_server.py
+++ b/src/natcap/invest/ui_server.py
@@ -234,15 +234,27 @@ def build_datastack_archive():
         args: JSON string of InVEST model args keys and values
 
     Returns:
-        A string.
+        A dictionary with the following key/value pairs:
+        - message (string): for logging and/or rendering in the UI.
+        - error (boolean): True if an error occurred, otherwise False.
     """
     payload = request.get_json()
-    datastack.build_datastack_archive(
-        json.loads(payload['args']),
-        payload['moduleName'],
-        payload['filepath'])
-
-    return 'datastack archive created'
+    try:
+        datastack.build_datastack_archive(
+            json.loads(payload['args']),
+            payload['moduleName'],
+            payload['filepath'])
+    except ValueError as message:
+        LOGGER.error(str(message))
+        return {
+            'message': str(message),
+            'error': True
+        }
+    else:
+        return {
+            'message': 'Datastack archive created',
+            'error': False
+        }
 
 
 @app.route(f'/{PREFIX}/log_model_start', methods=['POST'])
@@ -264,6 +276,7 @@ def log_model_exit():
         payload['status'])
     return 'OK'
 
+
 @app.route(f'/{PREFIX}/languages', methods=['GET'])
 def get_supported_languages():
     """Return a mapping of supported languages to their display names."""
diff --git a/tests/test_datastack.py b/tests/test_datastack.py
index a471b8e5e3..35d0695fa7 100644
--- a/tests/test_datastack.py
+++ b/tests/test_datastack.py
@@ -397,6 +397,25 @@ def test_archive_extraction(self):
             os.path.join(spatial_csv_dir, spatial_csv_dict[4]['path']),
             target_csv_vector_path)
 
+    def test_relative_path_failure(self):
+        """Datastack: raise error when relative path creation fails."""
+        from natcap.invest import datastack
+        params = {
+            'workspace_dir': os.path.join(self.workspace),
+        }
+
+        archive_path = os.path.join(self.workspace, 'archive.invs.tar.gz')
+
+        # Call build_datastack_archive and force build_parameter_set
+        # to raise an error
+        error_message = 'Error saving datastack'
+        with self.assertRaises(ValueError):
+            with patch('natcap.invest.datastack.build_parameter_set',
+                       side_effect=ValueError(error_message)):
+                datastack.build_datastack_archive(
+                    params, 'test_datastack_modules.simple_parameters',
+                    archive_path)
+
 
 class ParameterSetTest(unittest.TestCase):
     """Test Datastack."""
diff --git a/tests/test_ui_server.py b/tests/test_ui_server.py
index 02d9436ebe..78cc86cd01 100644
--- a/tests/test_ui_server.py
+++ b/tests/test_ui_server.py
@@ -190,11 +190,42 @@ def test_build_datastack_archive(self):
                 'carbon_pools_path': data_path
             }),
         }
-        _ = test_client.post(
+        response = test_client.post(
             f'{ROUTE_PREFIX}/build_datastack_archive', json=payload)
+        self.assertEqual(
+            response.json,
+            {'message': 'Datastack archive created', 'error': False})
         # test_datastack.py asserts the actual archiving functionality
         self.assertTrue(os.path.exists(target_filepath))
 
+    def test_build_datastack_archive_error_handling(self):
+        """UI server: build_datastack_archive endpoint
+        should catch a ValueError and return an error message.
+        """
+        test_client = ui_server.app.test_client()
+        self.workspace_dir = tempfile.mkdtemp()
+        target_filepath = os.path.join(self.workspace_dir, 'data.tgz')
+        data_path = os.path.join(self.workspace_dir, 'data.csv')
+        with open(data_path, 'w') as file:
+            file.write('hello')
+
+        payload = {
+            'filepath': target_filepath,
+            'moduleName': 'natcap.invest.carbon',
+            'args': json.dumps({
+                'workspace_dir': 'foo',
+                'carbon_pools_path': data_path
+            }),
+        }
+        error_message = 'Error saving datastack'
+        with patch('natcap.invest.datastack.build_datastack_archive',
+                   side_effect=ValueError(error_message)):
+            response = test_client.post(
+                f'{ROUTE_PREFIX}/build_datastack_archive', json=payload)
+            self.assertEqual(
+                response.json,
+                {'message': error_message, 'error': True})
+
     @patch('natcap.invest.ui_server.usage.requests.post')
     @patch('natcap.invest.ui_server.usage.requests.get')
     def test_log_model_start(self, mock_get, mock_post):
diff --git a/workbench/src/renderer/components/SetupTab/index.jsx b/workbench/src/renderer/components/SetupTab/index.jsx
index 5914c66ff4..7577cbce59 100644
--- a/workbench/src/renderer/components/SetupTab/index.jsx
+++ b/workbench/src/renderer/components/SetupTab/index.jsx
@@ -251,8 +251,8 @@ class SetupTab extends React.Component {
     };
     const key = window.crypto.getRandomValues(new Uint16Array(1))[0].toString();
     this.setSaveAlert('archiving...', false, key);
-    const response = await archiveDatastack(payload);
-    this.setSaveAlert(response, false, key);
+    const {message, error} = await archiveDatastack(payload);
+    this.setSaveAlert(message, error, key);
   }
 
   /** State updater for alert messages from various save buttons.
diff --git a/workbench/src/renderer/server_requests.js b/workbench/src/renderer/server_requests.js
index 90ca4125aa..0e8be93fb3 100644
--- a/workbench/src/renderer/server_requests.js
+++ b/workbench/src/renderer/server_requests.js
@@ -146,10 +146,14 @@ export function archiveDatastack(payload) {
       body: JSON.stringify(payload),
       headers: { 'Content-Type': 'application/json' },
     })
-      .then((response) => response.text())
-      .then((text) => {
-        logger.debug(text);
-        return text;
+      .then((response) => response.json())
+      .then(({message, error}) => {
+        if (error) {
+          logger.error(message);
+        } else {
+          logger.debug(message);
+        }
+        return {message, error};
       })
       .catch((error) => logger.error(error.stack))
   );
diff --git a/workbench/tests/renderer/investtab.test.js b/workbench/tests/renderer/investtab.test.js
index e3b6afdc07..e68a5e4e8b 100644
--- a/workbench/tests/renderer/investtab.test.js
+++ b/workbench/tests/renderer/investtab.test.js
@@ -289,7 +289,10 @@ describe('Sidebar Buttons', () => {
   });
 
   test('Save datastack: requests endpoint with correct payload', async () => {
-    const response = 'saved';
+    const response = {
+      message: 'saved',
+      error: false,
+    };
     archiveDatastack.mockImplementation(() => new Promise(
       (resolve) => {
         setTimeout(() => resolve(response), 500);
@@ -308,7 +311,7 @@ describe('Sidebar Buttons', () => {
 
     expect(await findByRole('alert')).toHaveTextContent('archiving...');
     await waitFor(() => {
-      expect(getByRole('alert')).toHaveTextContent(response);
+      expect(getByRole('alert')).toHaveTextContent(response.message);
     });
     const payload = archiveDatastack.mock.calls[0][0];
     expect(Object.keys(payload)).toEqual(expect.arrayContaining(

From 35ee58f122152d79a76295bd13e1749bf46d804a Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Tue, 17 Sep 2024 15:01:28 -0600
Subject: [PATCH 04/10] Update React tests to cover save alerts

---
 workbench/tests/renderer/investtab.test.js | 170 ++++++++++++++++-----
 1 file changed, 129 insertions(+), 41 deletions(-)

diff --git a/workbench/tests/renderer/investtab.test.js b/workbench/tests/renderer/investtab.test.js
index e68a5e4e8b..bee50202b3 100644
--- a/workbench/tests/renderer/investtab.test.js
+++ b/workbench/tests/renderer/investtab.test.js
@@ -221,19 +221,18 @@ describe('Sidebar Buttons', () => {
       message: 'saved',
       error: false,
     };
-    writeParametersToFile.mockResolvedValue(response);
+    writeParametersToFile.mockResolvedValueOnce(response);
     const mockDialogData = { canceled: false, filePath: 'foo.json' };
     ipcRenderer.invoke.mockResolvedValueOnce(mockDialogData);
 
     const { findByText, findByLabelText, findByRole } = renderInvestTab();
     const saveAsButton = await findByText('Save as...');
     await userEvent.click(saveAsButton);
-    const jsonOption = await findByLabelText((content, element) => content.startsWith('Parameters only'));
+    const jsonOption = await findByLabelText((content) => content.startsWith('Parameters only'));
     await userEvent.click(jsonOption);
     const saveButton = await findByRole('button', { name: 'Save' });
     await userEvent.click(saveButton);
 
-    expect(await findByRole('alert')).toHaveTextContent(response.message);
     const payload = writeParametersToFile.mock.calls[0][0];
     expect(Object.keys(payload)).toEqual(expect.arrayContaining(
       ['filepath', 'moduleName', 'relativePaths', 'args']
@@ -254,19 +253,18 @@ describe('Sidebar Buttons', () => {
 
   test('Save to Python script: requests endpoint with correct payload', async () => {
     const response = 'saved';
-    saveToPython.mockResolvedValue(response);
+    saveToPython.mockResolvedValueOnce(response);
     const mockDialogData = { canceled: false, filePath: 'foo.py' };
     ipcRenderer.invoke.mockResolvedValueOnce(mockDialogData);
 
     const { findByText, findByLabelText, findByRole } = renderInvestTab();
     const saveAsButton = await findByText('Save as...');
     await userEvent.click(saveAsButton);
-    const pythonOption = await findByLabelText((content, element) => content.startsWith('Python script'));
+    const pythonOption = await findByLabelText((content) => content.startsWith('Python script'));
     await userEvent.click(pythonOption);
     const saveButton = await findByRole('button', { name: 'Save' });
     await userEvent.click(saveButton);
 
-    expect(await findByRole('alert')).toHaveTextContent(response);
     const payload = saveToPython.mock.calls[0][0];
     expect(Object.keys(payload)).toEqual(expect.arrayContaining(
       ['filepath', 'modelname', 'args']
@@ -293,26 +291,18 @@ describe('Sidebar Buttons', () => {
       message: 'saved',
       error: false,
     };
-    archiveDatastack.mockImplementation(() => new Promise(
-      (resolve) => {
-        setTimeout(() => resolve(response), 500);
-      }
-    ));
+    archiveDatastack.mockResolvedValueOnce(response);
     const mockDialogData = { canceled: false, filePath: 'data.tgz' };
     ipcRenderer.invoke.mockResolvedValue(mockDialogData);
 
     const { findByText, findByLabelText, findByRole, getByRole } = renderInvestTab();
     const saveAsButton = await findByText('Save as...');
     await userEvent.click(saveAsButton);
-    const datastackOption = await findByLabelText((content, element) => content.startsWith('Parameters and data'));
+    const datastackOption = await findByLabelText((content) => content.startsWith('Parameters and data'));
     await userEvent.click(datastackOption);
     const saveButton = await findByRole('button', { name: 'Save' });
     await userEvent.click(saveButton);
 
-    expect(await findByRole('alert')).toHaveTextContent('archiving...');
-    await waitFor(() => {
-      expect(getByRole('alert')).toHaveTextContent(response.message);
-    });
     const payload = archiveDatastack.mock.calls[0][0];
     expect(Object.keys(payload)).toEqual(expect.arrayContaining(
       ['filepath', 'moduleName', 'args']
@@ -334,6 +324,129 @@ describe('Sidebar Buttons', () => {
     expect(archiveDatastack).toHaveBeenCalledTimes(1);
   });
 
+  test.each([
+    ['Parameters only', 'saveJsonFile'],
+    ['Parameters and data', 'saveDatastack'],
+    ['Python script', 'savePythonScript']
+  ])('%s: does nothing when canceled', async (label, method) => {
+    // callback data if the OS dialog was canceled
+    const mockDialogData = {
+      canceled: true,
+      filePaths: []
+    };
+    ipcRenderer.invoke.mockResolvedValue(mockDialogData);
+    const spy = jest.spyOn(SetupTab.WrappedComponent.prototype, method);
+
+    const { findByText, findByLabelText, findByRole } = renderInvestTab();
+    const saveAsButton = await findByText('Save as...');
+    await userEvent.click(saveAsButton);
+    const option = await findByLabelText((content, element) => content.startsWith(label));
+    await userEvent.click(option);
+    const saveButton = await findByRole('button', { name: 'Save' });
+    await userEvent.click(saveButton);
+
+    // Calls that would have triggered if a file was selected
+    expect(spy).toHaveBeenCalledTimes(0);
+  });
+
+  test.each([
+    [
+      'Parameters only',
+      writeParametersToFile,
+      {message: 'Parameter set saved', error: false}
+    ],
+    [
+      'Parameters and data',
+      archiveDatastack,
+      {message: 'Datastack archive created', error: false}
+    ],
+    [
+      'Python script',
+      saveToPython,
+      'Python script saved'
+    ]
+  ])('%s: renders success message', async (label, method, response) => {
+    ipcRenderer.invoke.mockResolvedValueOnce({canceled: false, filePath: 'example.txt'});
+    method.mockImplementation(() => new Promise(
+      (resolve) => {
+        setTimeout(() => resolve(response), 10);
+      }
+    ));
+
+    const { findByText, findByLabelText, findByRole } = renderInvestTab();
+    const saveAsButton = await findByText('Save as...');
+    await userEvent.click(saveAsButton);
+    const option = await findByLabelText((content) => content.startsWith(label));
+    await userEvent.click(option);
+    const saveButton = await findByRole('button', { name: 'Save' });
+    await userEvent.click(saveButton);
+
+    const saveAlert = await findByRole('alert');
+    if (method == archiveDatastack) {
+      expect(saveAlert).toHaveTextContent('archiving...');
+    }
+    await waitFor(() => {
+      expect(saveAlert).toHaveTextContent(response.message ?? response);
+    });
+    expect(saveAlert).toHaveClass('alert-success');
+  });
+
+  test.each([
+    [
+      'Parameters only',
+      writeParametersToFile,
+      {message: 'Error saving parameter set', error: true}
+    ],
+    [
+      'Parameters and data',
+      archiveDatastack,
+      {message: 'Error creating datastack archive', error: true}
+    ],
+  ])('%s: renders error message', async (label, method, response) => {
+    ipcRenderer.invoke.mockResolvedValueOnce({canceled: false, filePath: 'example.txt'});
+    method.mockImplementation(() => new Promise(
+      (resolve) => {
+        setTimeout(() => resolve(response), 10);
+      }
+    ));
+
+    const { findByText, findByLabelText, findByRole } = renderInvestTab();
+    const saveAsButton = await findByText('Save as...');
+    await userEvent.click(saveAsButton);
+    const option = await findByLabelText((content) => content.startsWith(label));
+    await userEvent.click(option);
+    const saveButton = await findByRole('button', { name: 'Save' });
+    await userEvent.click(saveButton);
+
+    const saveAlert = await findByRole('alert');
+    if (method == archiveDatastack) {
+      expect(saveAlert).toHaveTextContent('archiving...');
+    }
+    await waitFor(() => {
+      expect(saveAlert).toHaveTextContent(response.message);
+    });
+    expect(saveAlert).toHaveClass('alert-danger');
+  });
+
+  test('Save errors are cleared when save modal opens', async () => {
+    ipcRenderer.invoke.mockResolvedValueOnce({canceled: false, filePath: 'example.txt'});
+    writeParametersToFile.mockResolvedValueOnce({message: 'Error saving parameter set', error: true});
+
+    // Trigger error alert
+    const { findByText, findByLabelText, findByRole, queryByRole } = renderInvestTab();
+    const saveAsButton = await findByText('Save as...');
+    await userEvent.click(saveAsButton);
+    const jsonOption = await findByLabelText((content) => content.startsWith('Parameters only'));
+    await userEvent.click(jsonOption);
+    const saveButton = await findByRole('button', { name: 'Save' });
+    await userEvent.click(saveButton);
+    expect(await findByRole('alert')).toHaveClass('alert-danger');
+
+    // Re-open save modal
+    await userEvent.click(saveAsButton);
+    expect(queryByRole('alert')).toBe(null);
+  });
+
   test('Load parameters from file: loads parameters', async () => {
     const mockDatastack = {
       module_name: spec.pyname,
@@ -391,31 +504,6 @@ describe('Sidebar Buttons', () => {
     expect(spy).toHaveBeenCalledTimes(0);
   });
 
-  test.each([
-    ['Parameters only', 'saveJsonFile'],
-    ['Parameters and data', 'saveDatastack'],
-    ['Python script', 'savePythonScript']
-  ])('%s: does nothing when canceled', async (label, method) => {
-    // callback data if the OS dialog was canceled
-    const mockDialogData = {
-      canceled: true,
-      filePaths: []
-    };
-    ipcRenderer.invoke.mockResolvedValue(mockDialogData);
-    const spy = jest.spyOn(SetupTab.WrappedComponent.prototype, method);
-
-    const { findByText, findByLabelText, findByRole } = renderInvestTab();
-    const saveAsButton = await findByText('Save as...');
-    await userEvent.click(saveAsButton);
-    const option = await findByLabelText((content, element) => content.startsWith(label));
-    await userEvent.click(option);
-    const saveButton = await findByRole('button', { name: 'Save' });
-    await userEvent.click(saveButton);
-
-    // Calls that would have triggered if a file was selected
-    expect(spy).toHaveBeenCalledTimes(0);
-  });
-
   test('Load parameters button has hover text', async () => {
     const {
       findByText,

From ec5f90eae3c2335c789310f8a768314058dfc538 Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Tue, 17 Sep 2024 15:06:11 -0600
Subject: [PATCH 05/10] Update HISTORY

---
 HISTORY.rst | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/HISTORY.rst b/HISTORY.rst
index dfa49b3025..46eb4aee70 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -39,15 +39,17 @@ Unreleased Changes
 ------------------
 * Workbench
     * Several small updates to the model input form UI to improve usability
-      and visual consistency (https://github.com/natcap/invest/issues/912)
+      and visual consistency (https://github.com/natcap/invest/issues/912).
     * Fixed a bug that caused the application to crash when attempting to
       open a workspace without a valid logfile
-      (https://github.com/natcap/invest/issues/1598)
+      (https://github.com/natcap/invest/issues/1598).
     * Fixed a bug that was allowing readonly workspace directories on Windows
-      (https://github.com/natcap/invest/issues/1599)
+      (https://github.com/natcap/invest/issues/1599).
     * Fixed a bug that, in certain scenarios, caused a datastack to be saved
       with relative paths when the Relative Paths checkbox was left unchecked
-      (https://github.com/natcap/invest/issues/1609)
+      (https://github.com/natcap/invest/issues/1609).
+    * Improved error handling when a datastack cannot be saved with relative
+      paths across drives (https://github.com/natcap/invest/issues/1608).
 
 3.14.2 (2024-05-29)
 -------------------

From f8f889033a0ced475ab3234aa55faaaa0ccbd9da Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Tue, 17 Sep 2024 15:46:49 -0600
Subject: [PATCH 06/10] Increase timeout for archiveDatastack alert test

---
 workbench/tests/renderer/investtab.test.js | 27 +++++++++-------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/workbench/tests/renderer/investtab.test.js b/workbench/tests/renderer/investtab.test.js
index bee50202b3..0fff31abec 100644
--- a/workbench/tests/renderer/investtab.test.js
+++ b/workbench/tests/renderer/investtab.test.js
@@ -367,11 +367,15 @@ describe('Sidebar Buttons', () => {
     ]
   ])('%s: renders success message', async (label, method, response) => {
     ipcRenderer.invoke.mockResolvedValueOnce({canceled: false, filePath: 'example.txt'});
-    method.mockImplementation(() => new Promise(
-      (resolve) => {
-        setTimeout(() => resolve(response), 10);
-      }
-    ));
+    if (method == archiveDatastack) {
+      method.mockImplementationOnce(() => new Promise(
+        (resolve) => {
+          setTimeout(() => resolve(response), 500);
+        }
+      ));
+    } else {
+      method.mockResolvedValueOnce(response);
+    }
 
     const { findByText, findByLabelText, findByRole } = renderInvestTab();
     const saveAsButton = await findByText('Save as...');
@@ -404,11 +408,7 @@ describe('Sidebar Buttons', () => {
     ],
   ])('%s: renders error message', async (label, method, response) => {
     ipcRenderer.invoke.mockResolvedValueOnce({canceled: false, filePath: 'example.txt'});
-    method.mockImplementation(() => new Promise(
-      (resolve) => {
-        setTimeout(() => resolve(response), 10);
-      }
-    ));
+    method.mockResolvedValueOnce(response);
 
     const { findByText, findByLabelText, findByRole } = renderInvestTab();
     const saveAsButton = await findByText('Save as...');
@@ -419,12 +419,7 @@ describe('Sidebar Buttons', () => {
     await userEvent.click(saveButton);
 
     const saveAlert = await findByRole('alert');
-    if (method == archiveDatastack) {
-      expect(saveAlert).toHaveTextContent('archiving...');
-    }
-    await waitFor(() => {
-      expect(saveAlert).toHaveTextContent(response.message);
-    });
+    expect(saveAlert).toHaveTextContent(response.message);
     expect(saveAlert).toHaveClass('alert-danger');
   });
 

From bfff17b636c7308d9625145eb17fa1db09a8f9d6 Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Mon, 7 Oct 2024 17:40:03 -0600
Subject: [PATCH 07/10] Remove unneeded error pass-through and unneeded else
 blocks

---
 src/natcap/invest/datastack.py | 15 +++------------
 src/natcap/invest/ui_server.py | 18 ++++++++----------
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/src/natcap/invest/datastack.py b/src/natcap/invest/datastack.py
index da0a7eca2b..610cac4841 100644
--- a/src/natcap/invest/datastack.py
+++ b/src/natcap/invest/datastack.py
@@ -226,10 +226,6 @@ def build_datastack_archive(args, model_name, datastack_path):
 
     Returns:
         ``None``
-
-    Raises:
-        ValueError if raised by build_parameter_set
-        (i.e., if creating a relative path fails).
     """
     module = importlib.import_module(name=model_name)
 
@@ -457,12 +453,8 @@ def build_datastack_archive(args, model_name, datastack_path):
     # write parameters to a new json file in the temp workspace
     param_file_uri = os.path.join(temp_workspace,
                                   'parameters' + PARAMETER_SET_EXTENSION)
-    try:
-        build_parameter_set(
+    build_parameter_set(
             rewritten_args, model_name, param_file_uri, relative=True)
-    except ValueError as message:
-        # Pass through for handling by ui_server
-        raise ValueError(message)
 
     # Remove the handler before archiving the working dir (and the logfile)
     archive_filehandler.close()
@@ -573,9 +565,8 @@ def _recurse(args_param):
                             """Error: Cannot save datastack with relative
                             paths across drives. Choose a different save
                             location, or use absolute paths.""")
-                    else:
-                        # Always save unix paths.
-                        linux_style_path = temp_rel_path.replace('\\', '/')
+                    # Always save unix paths.
+                    linux_style_path = temp_rel_path.replace('\\', '/')
                 else:
                     # Always save unix paths.
                     linux_style_path = normalized_path.replace('\\', '/')
diff --git a/src/natcap/invest/ui_server.py b/src/natcap/invest/ui_server.py
index dd009c3a16..7c4ad72af2 100644
--- a/src/natcap/invest/ui_server.py
+++ b/src/natcap/invest/ui_server.py
@@ -194,11 +194,10 @@ def write_parameter_set_file():
             'message': str(message),
             'error': True
         }
-    else:
-        return {
-            'message': 'Parameter set saved',
-            'error': False
-        }
+    return {
+        'message': 'Parameter set saved',
+        'error': False
+    }
 
 
 @app.route(f'/{PREFIX}/save_to_python', methods=['POST'])
@@ -250,11 +249,10 @@ def build_datastack_archive():
             'message': str(message),
             'error': True
         }
-    else:
-        return {
-            'message': 'Datastack archive created',
-            'error': False
-        }
+    return {
+        'message': 'Datastack archive created',
+        'error': False
+    }
 
 
 @app.route(f'/{PREFIX}/log_model_start', methods=['POST'])

From 1d592d09bf4b4ae06f34b07bfdb7ceee98f84484 Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Mon, 7 Oct 2024 17:47:30 -0600
Subject: [PATCH 08/10] Change duration of save alerts to 4s, and update
 documentation for setSaveAlert

---
 workbench/src/renderer/components/SetupTab/index.jsx | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/workbench/src/renderer/components/SetupTab/index.jsx b/workbench/src/renderer/components/SetupTab/index.jsx
index 7577cbce59..fee9ebecb4 100644
--- a/workbench/src/renderer/components/SetupTab/index.jsx
+++ b/workbench/src/renderer/components/SetupTab/index.jsx
@@ -258,13 +258,13 @@ class SetupTab extends React.Component {
   /** State updater for alert messages from various save buttons.
    *
    * @param {string} message - the message to display
+   * @param {boolean} error - true if message was generated by an error,
+   *        false otherwise. Defaults to false.
    * @param {string} key - a key to uniquely identify each save action,
    *        passed as prop to `Expire` so that it can be aware of whether to,
    *        1. display: because a new save occurred, or
    *        2. not display: on a re-render after `Expire` expired, or
    *        3. update: because 'archiving...' alert changes to final message
-   * @param {boolean} error - true if message was generated by an error,
-   *        false otherwise. Defaults to false.
    *
    * @returns {undefined}
    */
@@ -516,7 +516,7 @@ class SetupTab extends React.Component {
           // Alert won't expire during archiving; will expire 2s after completion
           // Alert won't expire when an error has occurred;
           // will be hidden next time save modal opens
-          const alertExpires = (error || message === 'archiving...') ? 1e7 : 2000;
+          const alertExpires = (error || message === 'archiving...') ? 1e7 : 4000;
           SaveAlerts.push(
             <Expire
               key={key}

From 3f0cd82f8e242b80a64f02ffc753c1b037becc26 Mon Sep 17 00:00:00 2001
From: Emily Davis <eadavis@stanford.edu>
Date: Mon, 7 Oct 2024 17:52:35 -0600
Subject: [PATCH 09/10] Remove extra whitespace & update comment

---
 src/natcap/invest/datastack.py                       | 2 +-
 workbench/src/renderer/components/SetupTab/index.jsx | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/natcap/invest/datastack.py b/src/natcap/invest/datastack.py
index 610cac4841..0fb2cabed4 100644
--- a/src/natcap/invest/datastack.py
+++ b/src/natcap/invest/datastack.py
@@ -454,7 +454,7 @@ def build_datastack_archive(args, model_name, datastack_path):
     param_file_uri = os.path.join(temp_workspace,
                                   'parameters' + PARAMETER_SET_EXTENSION)
     build_parameter_set(
-            rewritten_args, model_name, param_file_uri, relative=True)
+        rewritten_args, model_name, param_file_uri, relative=True)
 
     # Remove the handler before archiving the working dir (and the logfile)
     archive_filehandler.close()
diff --git a/workbench/src/renderer/components/SetupTab/index.jsx b/workbench/src/renderer/components/SetupTab/index.jsx
index fee9ebecb4..02256f5a40 100644
--- a/workbench/src/renderer/components/SetupTab/index.jsx
+++ b/workbench/src/renderer/components/SetupTab/index.jsx
@@ -513,7 +513,7 @@ class SetupTab extends React.Component {
       Object.keys(saveAlerts).forEach((key) => {
         const { message, error } = saveAlerts[key];
         if (message) {
-          // Alert won't expire during archiving; will expire 2s after completion
+          // Alert won't expire during archiving; will expire 4s after completion
           // Alert won't expire when an error has occurred;
           // will be hidden next time save modal opens
           const alertExpires = (error || message === 'archiving...') ? 1e7 : 4000;

From f82f01d7ccfd56c00771420e99628c3572c4ea6e Mon Sep 17 00:00:00 2001
From: dcdenu4 <dcdenu4@gmail.com>
Date: Fri, 11 Oct 2024 16:29:22 -0400
Subject: [PATCH 10/10] Restrict pygeoprocessing to see if this the issue.

---
 requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/requirements.txt b/requirements.txt
index cf2d3aeb76..b60c525f7c 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -17,7 +17,7 @@ numpy>=1.11.0,!=1.16.0,<2.0
 Rtree>=0.8.2,!=0.9.1
 shapely>=2.0.0
 scipy>=1.9.0,!=1.12.*
-pygeoprocessing>=2.4.2  # pip-only
+pygeoprocessing>=2.4.2,<2.4.5  # pip-only
 taskgraph>=0.11.0
 psutil>=5.6.6
 chardet>=3.0.4