Merge pull request #1618 from emilyanndavis/bugfix/1598-improved-erro…

…r-handling-when-logfile-does-not-exist Allow 'Open Workspace' to open workspace whenever possible, even if logfile does not exist
natcap · Sep 5, 2024 · c056170 · c056170
2 parents 4b39db1 + 0846c72
commit c056170
Show file tree

Hide file tree

Showing 6 changed files with 192 additions and 87 deletions.
diff --git a/HISTORY.rst b/HISTORY.rst
@@ -38,8 +38,13 @@
 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)
-  * Fixed a bug that was allowing readonly workspace directories on Windows (https://github.com/natcap/invest/issues/1599)
+    * Several small updates to the model input form UI to improve usability
+      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)
+    * Fixed a bug that was allowing readonly workspace directories on Windows
+      (https://github.com/natcap/invest/issues/1599)
 
 3.14.2 (2024-05-29)
 -------------------

diff --git a/workbench/src/main/ipcMainChannels.js b/workbench/src/main/ipcMainChannels.js
@@ -14,6 +14,7 @@ export const ipcMainChannels = {
   IS_FIRST_RUN: 'is-first-run',
   LOGGER: 'logger',
   OPEN_EXTERNAL_URL: 'open-external-url',
+  OPEN_PATH: 'open-path',
   OPEN_LOCAL_HTML: 'open-local-html',
   SET_SETTING: 'set-setting',
   SHOW_ITEM_IN_FOLDER: 'show-item-in-folder',

diff --git a/workbench/src/main/setupDialogs.js b/workbench/src/main/setupDialogs.js
@@ -26,4 +26,10 @@ export default function setupDialogs() {
       shell.showItemInFolder(filepath);
     }
   );
+
+  ipcMain.handle(
+    ipcMainChannels.OPEN_PATH, async (event, dirpath) => {
+      return await shell.openPath(dirpath);
+    }
+  );
 }
diff --git a/workbench/src/renderer/components/InvestTab/index.jsx b/workbench/src/renderer/components/InvestTab/index.jsx
@@ -7,6 +7,8 @@ import TabContainer from 'react-bootstrap/TabContainer';
 import Nav from 'react-bootstrap/Nav';
 import Row from 'react-bootstrap/Row';
 import Col from 'react-bootstrap/Col';
+import Modal from 'react-bootstrap/Modal';
+import Button from 'react-bootstrap/Button';
 import {
   MdKeyboardArrowRight,
 } from 'react-icons/md';
@@ -45,10 +47,6 @@ async function investGetSpec(modelName) {
   return undefined;
 }
 
-function handleOpenWorkspace(logfile) {
-  ipcRenderer.send(ipcMainChannels.SHOW_ITEM_IN_FOLDER, logfile);
-}
-
 /**
  * Render an invest model setup form, log display, etc.
  * Manage launching of an invest model in a child process.
@@ -64,13 +62,16 @@ class InvestTab extends React.Component {
       uiSpec: null,
       userTerminated: false,
       executeClicked: false,
+      showErrorModal: false,
     };
 
     this.investExecute = this.investExecute.bind(this);
     this.switchTabs = this.switchTabs.bind(this);
     this.terminateInvestProcess = this.terminateInvestProcess.bind(this);
     this.investLogfileCallback = this.investLogfileCallback.bind(this);
     this.investExitCallback = this.investExitCallback.bind(this);
+    this.handleOpenWorkspace = this.handleOpenWorkspace.bind(this);
+    this.showErrorModal = this.showErrorModal.bind(this);
   }
 
   async componentDidMount() {
@@ -154,6 +155,7 @@ class InvestTab extends React.Component {
     updateJobProperties(tabID, {
       argsValues: args,
       status: undefined, // in case of re-run, clear an old status
+      logfile: undefined, // in case of re-run where logfile may no longer exist, clear old logfile path
     });
 
     ipcRenderer.send(
@@ -186,13 +188,30 @@ class InvestTab extends React.Component {
     );
   }
 
+  async handleOpenWorkspace(workspace_dir) {
+    if (workspace_dir) {
+      const error = await ipcRenderer.invoke(ipcMainChannels.OPEN_PATH, workspace_dir);
+      if (error) {
+        logger.error(`Error opening workspace (${workspace_dir}). ${error}`);
+        this.showErrorModal(true);
+      }
+    }
+  }
+
+  showErrorModal(shouldShow) {
+    this.setState({
+      showErrorModal: shouldShow,
+    });
+  }
+
   render() {
     const {
       activeTab,
       modelSpec,
       argsSpec,
       uiSpec,
       executeClicked,
+      showErrorModal,
     } = this.state;
     const {
       status,
@@ -213,88 +232,99 @@ class InvestTab extends React.Component {
     const sidebarFooterElementId = `sidebar-footer-${tabID}`;
 
     return (
-      <TabContainer activeKey={activeTab} id="invest-tab">
-        <Row className="flex-nowrap">
-          <Col
-            className="invest-sidebar-col"
-          >
-            <Nav
-              className="flex-column"
-              id="vertical tabs"
-              variant="pills"
-              activeKey={activeTab}
-              onSelect={this.switchTabs}
-            >
-              <Nav.Link eventKey="setup">
-                {t('Setup')}
-                <MdKeyboardArrowRight />
-              </Nav.Link>
-              <Nav.Link eventKey="log" disabled={logDisabled}>
-                {t('Log')}
-                <MdKeyboardArrowRight />
-              </Nav.Link>
-            </Nav>
-            <div
-              className="sidebar-row sidebar-buttons"
-              id={sidebarSetupElementId}
-            />
-            <div className="sidebar-row sidebar-links">
-              <ResourcesLinks
-                moduleName={modelRunName}
-                docs={modelSpec.userguide}
-              />
-            </div>
-            <div
-              className="sidebar-row sidebar-footer"
-              id={sidebarFooterElementId}
+      <>
+        <TabContainer activeKey={activeTab} id="invest-tab">
+          <Row className="flex-nowrap">
+            <Col
+              className="invest-sidebar-col"
             >
-              {
-                status
-                  ? (
-                    <ModelStatusAlert
-                      status={status}
-                      handleOpenWorkspace={() => handleOpenWorkspace(logfile)}
-                      terminateInvestProcess={this.terminateInvestProcess}
-                    />
-                  )
-                  : null
-              }
-            </div>
-          </Col>
-          <Col className="invest-main-col">
-            <TabContent>
-              <TabPane
-                eventKey="setup"
-                aria-label="model setup tab"
+              <Nav
+                className="flex-column"
+                id="vertical tabs"
+                variant="pills"
+                activeKey={activeTab}
+                onSelect={this.switchTabs}
               >
-                <SetupTab
-                  pyModuleName={modelSpec.pyname}
-                  userguide={modelSpec.userguide}
-                  modelName={modelRunName}
-                  argsSpec={argsSpec}
-                  uiSpec={uiSpec}
-                  argsInitValues={argsValues}
-                  investExecute={this.investExecute}
-                  sidebarSetupElementId={sidebarSetupElementId}
-                  sidebarFooterElementId={sidebarFooterElementId}
-                  executeClicked={executeClicked}
-                  switchTabs={this.switchTabs}
+                <Nav.Link eventKey="setup">
+                  {t('Setup')}
+                  <MdKeyboardArrowRight />
+                </Nav.Link>
+                <Nav.Link eventKey="log" disabled={logDisabled}>
+                  {t('Log')}
+                  <MdKeyboardArrowRight />
+                </Nav.Link>
+              </Nav>
+              <div
+                className="sidebar-row sidebar-buttons"
+                id={sidebarSetupElementId}
+              />
+              <div className="sidebar-row sidebar-links">
+                <ResourcesLinks
+                  moduleName={modelRunName}
+                  docs={modelSpec.userguide}
                 />
-              </TabPane>
-              <TabPane
-                eventKey="log"
-                aria-label="model log tab"
+              </div>
+              <div
+                className="sidebar-row sidebar-footer"
+                id={sidebarFooterElementId}
               >
-                <LogTab
-                  logfile={logfile}
-                  executeClicked={executeClicked}
-                  tabID={tabID}
-                />
-              </TabPane>
-            </TabContent>
-          </Col>
-        </Row>
-      </TabContainer>
+                {
+                  status
+                    ? (
+                      <ModelStatusAlert
+                        status={status}
+                        handleOpenWorkspace={() => this.handleOpenWorkspace(argsValues?.workspace_dir)}
+                        terminateInvestProcess={this.terminateInvestProcess}
+                      />
+                    )
+                    : null
+                }
+              </div>
+            </Col>
+            <Col className="invest-main-col">
+              <TabContent>
+                <TabPane
+                  eventKey="setup"
+                  aria-label="model setup tab"
+                >
+                  <SetupTab
+                    pyModuleName={modelSpec.pyname}
+                    userguide={modelSpec.userguide}
+                    modelName={modelRunName}
+                    argsSpec={argsSpec}
+                    uiSpec={uiSpec}
+                    argsInitValues={argsValues}
+                    investExecute={this.investExecute}
+                    sidebarSetupElementId={sidebarSetupElementId}
+                    sidebarFooterElementId={sidebarFooterElementId}
+                    executeClicked={executeClicked}
+                    switchTabs={this.switchTabs}
+                  />
+                </TabPane>
+                <TabPane
+                  eventKey="log"
+                  aria-label="model log tab"
+                >
+                  <LogTab
+                    logfile={logfile}
+                    executeClicked={executeClicked}
+                    tabID={tabID}
+                  />
+                </TabPane>
+              </TabContent>
+            </Col>
+          </Row>
+        </TabContainer>
+        <Modal show={showErrorModal} onHide={() => this.showErrorModal(false)} aria-labelledby="error-modal-title">
+          <Modal.Header closeButton>
+            <Modal.Title id="error-modal-title">{t('Error opening workspace')}</Modal.Title>
+          </Modal.Header>
+          <Modal.Body>{t('Failed to open workspace directory. Make sure the directory exists and that you have write access to it.')}</Modal.Body>
+          <Modal.Footer>
+            <Button variant="primary" onClick={() => this.showErrorModal(false)}>{t('OK')}</Button>
+          </Modal.Footer>
+        </Modal>
+      </>
     );
   }
 }

diff --git a/workbench/tests/main/main.test.js b/workbench/tests/main/main.test.js
@@ -187,6 +187,7 @@ describe('createWindow', () => {
       ipcMainChannels.GET_N_CPUS,
       ipcMainChannels.INVEST_VERSION,
       ipcMainChannels.IS_FIRST_RUN,
+      ipcMainChannels.OPEN_PATH,
       ipcMainChannels.SHOW_OPEN_DIALOG,
       ipcMainChannels.SHOW_SAVE_DIALOG,
     ];

diff --git a/workbench/tests/renderer/investtab.test.js b/workbench/tests/renderer/investtab.test.js
@@ -118,9 +118,71 @@ describe('Run status Alert renders with status from a recent run', () => {
     });
 
     const { findByRole } = renderInvestTab(job);
-    const openWorkspace = await findByRole('button', { name: 'Open Workspace' })
-    openWorkspace.click();
-    expect(shell.showItemInFolder).toHaveBeenCalledTimes(1);
+    const openWorkspaceBtn = await findByRole('button', { name: 'Open Workspace' });
+    expect(openWorkspaceBtn).toBeTruthy();
+  });
+});
+
+describe('Open Workspace button', () => {
+  const spec = {
+    args: {},
+  };
+
+  const baseJob = {
+    ...DEFAULT_JOB,
+    status: 'success',
+  };
+
+  beforeEach(() => {
+    getSpec.mockResolvedValue(spec);
+    fetchValidation.mockResolvedValue([]);
+    uiConfig.UI_SPEC = mockUISpec(spec);
+    setupDialogs();
+  });
+
+  afterEach(() => {
+    removeIpcMainListeners();
+  });
+
+  test('should open workspace', async () => {
+    const job = {
+      ...baseJob,
+      argsValues: {
+        workspace_dir: '/workspace',
+      },
+    };
+
+    jest.spyOn(ipcRenderer, 'invoke');
+
+    const { findByRole } = renderInvestTab(job);
+    const openWorkspaceBtn = await findByRole('button', { name: 'Open Workspace' })
+    openWorkspaceBtn.click();
+
+    expect(ipcRenderer.invoke).toHaveBeenCalledTimes(1);
+    expect(ipcRenderer.invoke).toHaveBeenCalledWith(ipcMainChannels.OPEN_PATH, job.argsValues.workspace_dir);
+  });
+
+  test('should present an error message to the user if workspace cannot be opened (e.g., if it does not exist)', async () => {
+    const job = {
+      ...baseJob,
+      status: 'error',
+      argsValues: {
+        workspace_dir: '/nonexistent-workspace',
+      },
+    };
+
+    jest.spyOn(ipcRenderer, 'invoke');
+    ipcRenderer.invoke.mockResolvedValue('Error opening workspace');
+
+    const { findByRole } = renderInvestTab(job);
+    const openWorkspaceBtn = await findByRole('button', { name: 'Open Workspace' });
+    openWorkspaceBtn.click();
+
+    expect(ipcRenderer.invoke).toHaveBeenCalledTimes(1);
+    expect(ipcRenderer.invoke).toHaveBeenCalledWith(ipcMainChannels.OPEN_PATH, job.argsValues.workspace_dir);
+
+    const errorModal = await findByRole('dialog', { name: 'Error opening workspace'});
+    expect(errorModal).toBeTruthy();
   });
 });