Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow 'Open Workspace' to open workspace whenever possible, even if logfile does not exist #1618

9 changes: 7 additions & 2 deletions HISTORY.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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)
-------------------
Expand Down
1 change: 1 addition & 0 deletions workbench/src/main/ipcMainChannels.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
6 changes: 6 additions & 0 deletions workbench/src/main/setupDialogs.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,10 @@ export default function setupDialogs() {
shell.showItemInFolder(filepath);
}
);

ipcMain.handle(
ipcMainChannels.OPEN_PATH, async (event, dirpath) => {
return await shell.openPath(dirpath);
}
);
}
199 changes: 117 additions & 82 deletions workbench/src/renderer/components/InvestTab/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.
Expand All @@ -64,13 +62,17 @@ 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.openWorkspaceDir = this.openWorkspaceDir.bind(this);
this.showErrorModal = this.showErrorModal.bind(this);
}

async componentDidMount() {
Expand Down Expand Up @@ -154,6 +156,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(
Expand Down Expand Up @@ -186,13 +189,34 @@ class InvestTab extends React.Component {
);
}

handleOpenWorkspace(workspace_dir) {
if (workspace_dir) {
this.openWorkspaceDir(workspace_dir);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a benefit to defining both handleOpenWorkspace and openWorkspaceDir? As opposed to doing all the logic in one function? It's fairly inconsequential so if you prefer it this way feel free to leave it as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. I think this is a holdover from when handleOpenWorkspace also called shell.showItemInFolder when the logfile was valid. With that complexity gone, it does look a little silly now.


async openWorkspaceDir(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,
Expand All @@ -213,88 +237,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}
<>
<TabContainer activeKey={activeTab} id="invest-tab">
<Row className="flex-nowrap">
<Col
className="invest-sidebar-col"
>
<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}
>
{
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)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
</>
);
}
}
Expand Down
1 change: 1 addition & 0 deletions workbench/tests/main/main.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
];
Expand Down
68 changes: 65 additions & 3 deletions workbench/tests/renderer/investtab.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
emilyanndavis marked this conversation as resolved.
Show resolved Hide resolved
});
});

Expand Down
Loading