Merge pull request #1370 from davemfish/bugfix/WB-1285-jobs-data

Workbench: validate metadata that is used to open invest models
natcap · Aug 16, 2023 · f14f60e · f14f60e
2 parents 6db2256 + 9ff067c
commit f14f60e
Show file tree

Hide file tree

Showing 9 changed files with 121 additions and 48 deletions.
diff --git a/HISTORY.rst b/HISTORY.rst
@@ -59,6 +59,9 @@ Unreleased Changes
     * Closing the main window will now close any user's guide windows that are
       open. Fixed a bug where the app could not be reopened after closing.
       (`#1258 <https://github.com/natcap/invest/issues/1258>`_)
+    * Fixed a bug where invalid metadata for a recent run would result
+      in an uncaught exception.
+      (`#1286 <https://github.com/natcap/invest/issues/1286>`_)
     * Middle clicking an InVEST model tab was opening a blank window. Now
       middle clicking will close that tab as expected.
       (`#1261 <https://github.com/natcap/invest/issues/1261>`_)

diff --git a/workbench/src/renderer/InvestJob.js b/workbench/src/renderer/InvestJob.js
@@ -58,8 +58,8 @@ export default class InvestJob {
       const lastKey = sortedJobHashes.pop();
       investJobStore.removeItem(lastKey);
     }
-    await investJobStore.setItem(HASH_ARRAY_KEY, sortedJobHashes);
     await investJobStore.setItem(job.hash, job);
+    await investJobStore.setItem(HASH_ARRAY_KEY, sortedJobHashes);
     return InvestJob.getJobStore();
   }
 

diff --git a/workbench/src/renderer/app.jsx b/workbench/src/renderer/app.jsx
@@ -57,7 +57,12 @@ export default class App extends React.Component {
     const recentJobs = await InvestJob.getJobStore();
     this.setState({
       investList: investList,
-      recentJobs: recentJobs,
+      // filter out models that do not exist in current version of invest
+      recentJobs: recentJobs.filter((job) => (
+        Object.values(investList)
+          .map((m) => m.model_name)
+          .includes(job.modelRunName)
+      )),
       showDownloadModal: this.props.isFirstRun,
     });
     await i18n.changeLanguage(window.Workbench.LANGUAGE);

diff --git a/workbench/src/renderer/components/HomeTab/index.jsx b/workbench/src/renderer/components/HomeTab/index.jsx
@@ -11,6 +11,8 @@ import { useTranslation } from 'react-i18next';
 import OpenButton from '../OpenButton';
 import InvestJob from '../../InvestJob';
 
+const { logger } = window.Workbench;
+
 /**
  * Renders a table of buttons for each invest model and
  * a list of cards for each cached invest job.
@@ -110,46 +112,51 @@ HomeTab.propTypes = {
  */
 function RecentInvestJobs(props) {
   const { recentJobs, openInvestModel } = props;
-  const handleClick = (jobMetadata) => {
-    openInvestModel(new InvestJob(jobMetadata));
-  }
   const { t, i18n } = useTranslation();
 
-  // Buttons to load each recently saved state
+  const handleClick = (jobMetadata) => {
+    try {
+      openInvestModel(new InvestJob(jobMetadata));
+    } catch (error) {
+      logger.debug(error);
+    }
+  };
+
   const recentButtons = [];
   recentJobs.forEach((job) => {
-    if (!job.argsValues) { return; }
-    recentButtons.push(
-      <Card
-        className="text-left recent-job-card"
-        as="button"
-        key={job.hash}
-        onClick={() => handleClick(job)}
-      >
-        <Card.Body>
-          <Card.Header>
-            <span className="header-title">{job.modelHumanName}</span>
-          </Card.Header>
-          <Card.Title>
-            <span className="text-heading">{'Workspace: '}</span>
-            <span className="text-mono">{job.argsValues.workspace_dir}</span>
-          </Card.Title>
-          <Card.Title>
-            <span className="text-heading">{'Suffix: '}</span>
-            <span className="text-mono">{job.argsValues.results_suffix}</span>
-          </Card.Title>
-          <Card.Footer className="text-muted">
-            <span className="timestamp">{job.humanTime}</span>
-            <span className="status">
-              {(job.status === 'success'
-                ? <span className="status-success">{t('Model Complete')}</span>
-                : <span className="status-error">{job.status}</span>
-              )}
-            </span>
-          </Card.Footer>
-        </Card.Body>
-      </Card>
-    );
+    if (job && job.argsValues && job.modelHumanName) {
+      recentButtons.push(
+        <Card
+          className="text-left recent-job-card"
+          as="button"
+          key={job.hash}
+          onClick={() => handleClick(job)}
+        >
+          <Card.Body>
+            <Card.Header>
+              <span className="header-title">{job.modelHumanName}</span>
+            </Card.Header>
+            <Card.Title>
+              <span className="text-heading">{'Workspace: '}</span>
+              <span className="text-mono">{job.argsValues.workspace_dir}</span>
+            </Card.Title>
+            <Card.Title>
+              <span className="text-heading">{'Suffix: '}</span>
+              <span className="text-mono">{job.argsValues.results_suffix}</span>
+            </Card.Title>
+            <Card.Footer className="text-muted">
+              <span className="timestamp">{job.humanTime}</span>
+              <span className="status">
+                {(job.status === 'success'
+                  ? <span className="status-success">{t('Model Complete')}</span>
+                  : <span className="status-error">{job.status}</span>
+                )}
+              </span>
+            </Card.Footer>
+          </Card.Body>
+        </Card>
+      );
+    }
   });
 
   return (

diff --git a/workbench/src/renderer/components/OpenButton/index.jsx b/workbench/src/renderer/components/OpenButton/index.jsx
@@ -11,6 +11,7 @@ import { fetchDatastackFromFile } from '../../server_requests';
 import { ipcMainChannels } from '../../../main/ipcMainChannels';
 
 const { ipcRenderer } = window.Workbench.electron;
+const { logger } = window.Workbench;
 
 /**
  * Render a button that loads args from a datastack, parameterset, or logfile.
@@ -23,9 +24,22 @@ class OpenButton extends React.Component {
   }
 
   async browseFile() {
+    const { t } = this.props;
     const data = await ipcRenderer.invoke(ipcMainChannels.SHOW_OPEN_DIALOG);
     if (!data.canceled) {
-      const datastack = await fetchDatastackFromFile(data.filePaths[0]);
+      let datastack;
+      try {
+        datastack = await fetchDatastackFromFile(data.filePaths[0]);
+      } catch (error) {
+        logger.error(error);
+        alert(
+          t(
+            'No InVEST model data can be parsed from the file:\n {{filepath}}',
+            { filepath: data.filePaths[0] }
+          )
+        );
+        return;
+      }
       const job = new InvestJob({
         modelRunName: datastack.model_run_name,
         modelHumanName: datastack.model_human_name,

diff --git a/workbench/src/renderer/components/SetupTab/index.jsx b/workbench/src/renderer/components/SetupTab/index.jsx
@@ -26,6 +26,7 @@ import { argsDictFromObject } from '../../utils';
 import { ipcMainChannels } from '../../../main/ipcMainChannels';
 
 const { ipcRenderer } = window.Workbench.electron;
+const { logger } = window.Workbench;
 
 /** Initialize values of InVEST args based on the model's UI Spec.
  *
@@ -54,7 +55,7 @@ function initializeArgValues(argsSpec, uiSpec, argsDict) {
     if (argsSpec[argkey].type === 'boolean') {
       value = argsDict[argkey] || false;
     } else if (argsSpec[argkey].type === 'option_string') {
-      if  (argsDict[argkey]) {
+      if (argsDict[argkey]) {
         value = argsDict[argkey];
       } else { // default to first
         if (Array.isArray(argsSpec[argkey].options)) {
@@ -274,8 +275,20 @@ class SetupTab extends React.Component {
   }
 
   async loadParametersFromFile(filepath) {
-    const datastack = await fetchDatastackFromFile(filepath);
     const { pyModuleName, switchTabs, t } = this.props;
+    let datastack;
+    try {
+      datastack = await fetchDatastackFromFile(filepath);
+    } catch (error) {
+      logger.error(error);
+      alert( // eslint-disable-line no-alert
+        t(
+          'No InVEST model data can be parsed from the file:\n {{filepath}}',
+          { filepath: filepath }
+        )
+      );
+      return;
+    }
     if (datastack.module_name === pyModuleName) {
       this.batchUpdateArgs(datastack.args);
       switchTabs('setup');

diff --git a/workbench/src/renderer/server_requests.js b/workbench/src/renderer/server_requests.js
@@ -82,7 +82,6 @@ export function fetchDatastackFromFile(payload) {
       headers: { 'Content-Type': 'application/json' },
     })
       .then((response) => response.json())
-      .catch((error) => logger.error(error.stack))
   );
 }
 

diff --git a/workbench/tests/renderer/app.test.js b/workbench/tests/renderer/app.test.js
@@ -268,7 +268,7 @@ describe('Various ways to open and close InVEST models', () => {
 
 describe('Display recently executed InVEST jobs on Home tab', () => {
   beforeEach(() => {
-    getInvestModelNames.mockResolvedValue({});
+    getInvestModelNames.mockResolvedValue(MOCK_INVEST_LIST);
   });
 
   afterEach(async () => {
@@ -277,7 +277,7 @@ describe('Display recently executed InVEST jobs on Home tab', () => {
 
   test('Recent Jobs: each has a button', async () => {
     const job1 = new InvestJob({
-      modelRunName: 'carbon',
+      modelRunName: MOCK_MODEL_RUN_NAME,
       modelHumanName: 'Carbon Sequestration',
       argsValues: {
         workspace_dir: 'work1',
@@ -286,7 +286,7 @@ describe('Display recently executed InVEST jobs on Home tab', () => {
     });
     await InvestJob.saveJob(job1);
     const job2 = new InvestJob({
-      modelRunName: 'sdr',
+      modelRunName: MOCK_MODEL_RUN_NAME,
       modelHumanName: 'Sediment Ratio Delivery',
       argsValues: {
         workspace_dir: 'work2',
@@ -328,7 +328,7 @@ describe('Display recently executed InVEST jobs on Home tab', () => {
 
   test('Recent Jobs: a job with incomplete data is skipped', async () => {
     const job1 = new InvestJob({
-      modelRunName: 'carbon',
+      modelRunName: MOCK_MODEL_RUN_NAME,
       modelHumanName: 'invest A',
       argsValues: {
         workspace_dir: 'dir',
@@ -337,7 +337,7 @@ describe('Display recently executed InVEST jobs on Home tab', () => {
     });
     const job2 = new InvestJob({
       // argsValues is missing
-      modelRunName: 'sdr',
+      modelRunName: MOCK_MODEL_RUN_NAME,
       modelHumanName: 'invest B',
       status: 'success',
     });
@@ -350,6 +350,23 @@ describe('Display recently executed InVEST jobs on Home tab', () => {
     expect(queryByText(job2.modelHumanName)).toBeNull();
   });
 
+  test('Recent Jobs: a job from a deprecated model is not displayed', async () => {
+    const job1 = new InvestJob({
+      modelRunName: 'does not exist',
+      modelHumanName: 'invest A',
+      argsValues: {
+        workspace_dir: 'dir',
+      },
+      status: 'success',
+    });
+    await InvestJob.saveJob(job1);
+    const { findByText, queryByText } = render(<App />);
+
+    expect(queryByText(job1.modelHumanName)).toBeNull();
+    expect(await findByText(/Set up a model from a sample datastack file/))
+      .toBeInTheDocument();
+  });
+
   test('Recent Jobs: placeholder if there are no recent jobs', async () => {
     const { findByText } = render(
       <App />
@@ -361,7 +378,7 @@ describe('Display recently executed InVEST jobs on Home tab', () => {
 
   test('Recent Jobs: cleared by button', async () => {
     const job1 = new InvestJob({
-      modelRunName: 'carbon',
+      modelRunName: MOCK_MODEL_RUN_NAME,
       modelHumanName: 'Carbon Sequestration',
       argsValues: {
         workspace_dir: 'work1',

diff --git a/workbench/tests/renderer/setuptab.test.js b/workbench/tests/renderer/setuptab.test.js
@@ -163,6 +163,21 @@ describe('Arguments form input types', () => {
     expect(input).toHaveValue('a');
     expect(input).not.toHaveValue('b');
   });
+
+  test('initial arg values can contain extra args', async () => {
+    const spec = baseArgsSpec('number');
+    const displayedValue = '1';
+    const missingValue = '0';
+    const initArgs = {
+      [Object.keys(spec.args)[0]]: displayedValue,
+      paramZ: missingValue, // paramZ is not in the ARGS_SPEC or UI_SPEC
+    };
+
+    const { findByLabelText, queryByText } = renderSetupFromSpec(spec, UI_SPEC, initArgs);
+    const input = await findByLabelText(`${spec.args.arg.name} (${spec.args.arg.units})`);
+    await waitFor(() => expect(input).toHaveValue(displayedValue));
+    expect(queryByText(missingValue)).toBeNull();
+  });
 });
 
 describe('Arguments form interactions', () => {