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

Workflow default dialog alert #4100

Merged
merged 12 commits into from
Oct 24, 2017
40 changes: 36 additions & 4 deletions app/pages/admin/project-status.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { Component } from 'react';
import apiClient from 'panoptes-client/lib/api-client';
import Dialog from 'modal-form/dialog';

import ProjectIcon from '../../components/project-icon';
import getWorkflowsInOrder from '../../lib/get-workflows-in-order';
Expand All @@ -10,6 +11,7 @@ import VersionList from './project-status/version-list';
import ExperimentalFeatures from './project-status/experimental-features';
import Toggle from './project-status/toggle';
import RedirectToggle from './project-status/redirect-toggle';
import WorkflowDefaultDialog from './workflow-default-dialog'
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but this is missing a semi-colon.


class ProjectStatus extends Component {
constructor(props) {
Expand All @@ -20,6 +22,7 @@ class ProjectStatus extends Component {
this.renderError = this.renderError.bind(this);
this.renderWorkflows = this.renderWorkflows.bind(this);
this.handleToggle = this.handleToggle.bind(this);
this.getProjectAndWorkflows = this.getProjectAndWorkflows.bind(this);

this.state = {
project: null,
Expand All @@ -30,7 +33,7 @@ class ProjectStatus extends Component {
}

componentDidMount() {
this.getProject().then(() => this.getWorkflows());
this.getProjectAndWorkflows();
}

componentWillUnmount() {
Expand Down Expand Up @@ -58,6 +61,10 @@ class ProjectStatus extends Component {
});
}

getProjectAndWorkflows() {
this.getProject().then(() => this.getWorkflows());
}

getUsedWorkflowLevels(workflows) {
return workflows
.map(workflow => workflow.configuration.level)
Expand All @@ -76,10 +83,31 @@ class ProjectStatus extends Component {
handleToggle(event, workflow) {
this.setState({ error: null });
const checked = event.target.checked;
const defaultWorkflowId = this.state.project.configuration.default_workflow;

if (defaultWorkflowId === workflow.id && workflow.active) {
Dialog.alert(
<WorkflowDefaultDialog closeButton={true} required={true} />
)
.then(() => {
const promises = [
workflow.update({ active: checked }).save(),
this.state.project.update({ 'configuration.default_workflow': undefined }).save()
];
Promise.all(promises)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is related to your question in the PR:

if a user clicks the background or presses escape, the workflow retains its default status, but workflow.active still gets toggled (ie, it is still possible to have a default workflow that is inactive). Not sure if this is the desired UX, would appreciate input.

My suggestion to use Promise.all() was for the project update and the workflow update. If a workflow is active and the project default, we want to prompt the admin user asking to confirm the switch and if they confirm, then remove that workflow as the default and make it inactive. If they cancel, then no action is taken.

At this point, you shouldn't first need to do a get request for the project and workflows again as you should already have them since they are called in componentDidMount. The get request for the project and workflows should be after the successful Promise.all(). What's defined in componentDidMount could be moved into a class method to be reused here as well as in componentDidMount.

.then(() => {
this.getProjectAndWorkflows();
})
.catch(error => this.setState({ error }));
})
.catch(error => console.error(error)); // eslint-disable-line no-console
}

return workflow.update({ 'active': checked }).save()
.then(() => this.getWorkflows())
.catch(error => this.setState({ error }))
if (defaultWorkflowId !== workflow.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I went back and looked at the lab code and if a project isn't live, then the default workflow button can still be enabled for inactive workflows. I think this should change, but to make sure we still allow admins to toggle the checkbox as needed, this conditional needs modification:
((defaultWorkflowId !== workflow.id) || (defaultWorkflowId === workflow.id && !workflow.active))

workflow.update({ active: checked })
.save()
.catch(error => this.setState({ error }));
}
}

renderError() {
Expand All @@ -98,6 +126,8 @@ class ProjectStatus extends Component {
{this.state.workflows.map((workflow) => {
return (
<li key={workflow.id} className="section-list__item">
{this.state.project.configuration.default_workflow === workflow.id ?
' * ' : ''}
<WorkflowToggle
workflow={workflow}
name="active"
Expand Down Expand Up @@ -164,6 +194,8 @@ class ProjectStatus extends Component {
<div className="project-status__section">
<h4>Workflow Settings</h4>
<small>The workflow level dropdown is for the workflow assignment experimental feature.</small>
<br />
<small>An asterisk (*) denotes a default workflow.</small>
{this.renderError()}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a React error being thrown, Error: Objects are not valid as a React child when an error is set to the component because this is expecting a this.state.error to be a string. We changed how errors are handled in the javascript client to send along the full error object instead, so how the error renders should be modified too. Perhaps the status and message together?:

{`Error ${this.state.error.status}: ${this.state.error.message}`}

{this.renderWorkflows()}
</div>
Expand Down
23 changes: 23 additions & 0 deletions app/pages/admin/workflow-default-dialog.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React from 'react';

const WorkflowDefaultDialog = ({ onSuccess }) => (
<div>
<div>
You are about to make the default workflow inactive,
which will remove the default workflow setting from this project.
The default workflow can be set in the edit workflows page of the project builder.
</div>
<br />
<button type="submit" onSubmit={onSuccess}>ok</button>
</div>
);

WorkflowDefaultDialog.defaultProps = {
onSuccess: () => {}
};

WorkflowDefaultDialog.propTypes = {
onSuccess: React.PropTypes.func
};

export default WorkflowDefaultDialog;
25 changes: 25 additions & 0 deletions app/pages/admin/workflow-default-dialog.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import React from 'react';
import assert from 'assert';
import { shallow } from 'enzyme';
import sinon from 'sinon';
import WorkflowDefaultDialog from './workflow-default-dialog';

describe('WorkflowDefaultDialog', () => {
let wrapper;
let onSuccessSpy;

before(() => {
onSuccessSpy = sinon.spy();
wrapper = shallow(<WorkflowDefaultDialog onSuccess={onSuccessSpy} />);
});

it('renders without crashing', () => {
const WorkflowDefaultDialogContainer = wrapper.find('div').first();
assert.equal(WorkflowDefaultDialogContainer.length, 1);
});

it('calls the onSuccess handler', () => {
wrapper.find('button').simulate('submit');
sinon.assert.calledOnce(onSuccessSpy);
});
});