Workflow default dialog alert #4100

jelliotartz · 2017-10-17T16:26:39Z

Describe your changes

Fixes Don't let an admin accidentally make a default workflow inactive #4041.
- Related: WorkflowToggle ES6 conversion #4092
Adds a modal popup (WorkflowDefaultDialog) when an admin user sets a project's default workflow to inactive on the ProjectStatus page.
- RFC: styling on modal popup is currently pretty generic. Not sure what we'd like, would appreciate input.
Adds an asterisk on ProjectStatus page to indicate the default workflow, along with explanatory text.
- I didn't add a Translate component for this text, as I wasn't sure if it was desirable to have some of the text in Translate and some not, and if moving all of ProjectStatus' text to Translate was outside of the scope of this PR.
RFC: 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.
Still need to add tests for:
- RFC: Do I need to add tests for the conditional display of WorkflowDefaultDialog modal in ProjectStatus?
- ~~Content of WorkflowDefaultDialog.~~
Staged here.

Review Checklist

Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
Does it work on mobile?
Can you rm -rf node_modules/ && npm install and app works as expected?
Did you deploy a staging branch?
If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
Are the tests passing locally and on Travis?

Optional

Have you replaced any ChangeListener or PromiseRenderer components with code that updates component state?
If changes are made to the classifier, does the dev classifier still work?
Have you resized and compressed any image you've added?
Have you added in flow type annotations?
Have you followed the Springer guidelines for commit messages?

srallen

I have a few comments about the logic and answer your question about behavior inline.

For the test, I'm not sure which conditional you're referring to, can you be more specific?

srallen · 2017-10-17T20:46:25Z

app/components/workflow-default-dialog.jsx

+
+const WorkflowDefaultDialog = ({ onSuccess }) => (
+  <div>
+    <Translate content="workflowDefaultDialog.text" />


I'm not sure if this needs to be translated. The admin pages are only for Zooniverse team members.

srallen · 2017-10-17T20:47:47Z

app/pages/admin/project-status.jsx

@@ -25,7 +27,8 @@ class ProjectStatus extends Component {
      project: null,
      error: null,
      usedWorkflowLevels: [],
-      workflows: []
+      workflows: [],
+      workflowSetting: {}


Is this going to be used somewhere? I don't see it used in the code below.

srallen · 2017-10-17T20:49:50Z

app/pages/admin/project-status.jsx

+        <WorkflowDefaultDialog closeButton={true} required={true} />
+      )
+      .then(() => {
+        Promise.all(promises)


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.

srallen · 2017-10-17T20:51:09Z

app/pages/admin/project-status.jsx

+      .then(() => {
+        Promise.all(promises)
+          .then(() => {
+            if (!this.state.error) {


If there is an error, then the promise is rejected, which is what catch() is for.

srallen · 2017-10-17T20:52:50Z

app/pages/admin/project-status.jsx

-    return workflow.update({ 'active': checked }).save()
-      .then(() => this.getWorkflows())
-      .catch(error => this.setState({ error }))
+    workflow.update({ active: checked }).save();


Also related to the question in the PR: if (defaultWorkflowId !== workflow.id) then update the workflow. We don't want to always update the active state for the workflow.

srallen

One more organizational comment: The components folder is meant for reusable components. The new dialog is built for use with the admin pages and won't be reused so it should be put into the admin folder.

jelliotartz · 2017-10-17T21:06:13Z

Thanks :) Regarding the tests, do I need to create a spec file for ProjectStatus and then check whether WorkflowDefaultDialog component renders as expected, i.e. simulate a click on WorkflowToggle, set state so that defaultWorkflowId === workflow.id && workflow.active, then test whether the modal is present when it's supposed to be? (and not present when it shouldn't be there?)

- Remove Translation component and unused code. - Use Promise.all for update requests. - Add class method for wrkflow & proj GET req's. - Move WorkflowDefaultDialog to admin folder.

srallen

I have a few more minor comments inline.

srallen · 2017-10-18T14:36:48Z

app/pages/admin/project-status.jsx

+    const defaultWorkflowId = this.state.project.configuration.default_workflow;
+
+    if (defaultWorkflowId === workflow.id && workflow.active) {
+      const promises = [


Put this variable in the then block of the Dialog alert promise.

srallen · 2017-10-18T14:37:44Z

app/pages/admin/workflow-default-dialog.jsx

+    <div>
+      You are about to make the default workflow inactive,
+      which will remove the default setting from this workflow.
+      The default workflow can be set in the workflows page of the project builder.


Let's be specific that the option is on the workflow edit page.

srallen · 2017-10-18T14:38:07Z

app/pages/admin/workflow-default-dialog.jsx

+  <div>
+    <div>
+      You are about to make the default workflow inactive,
+      which will remove the default setting from this workflow.


It removes the default setting from the project.

srallen · 2017-10-18T14:39:30Z

app/pages/admin/workflow-default-dialog.spec.js

+    assert.equal(WorkflowDefaultDialogContainer.length, 1);
+  });
+
+  it('renders dialog text', () => {


Since there isn't a React component we're testing the presence of anymore. This is just testing that JSX works, and this isn't necessary for us to test again.

srallen · 2017-10-18T15:59:01Z

app/pages/admin/project-status.jsx

-      .then(() => this.getWorkflows())
-      .catch(error => this.setState({ error }))
+    if (defaultWorkflowId !== workflow.id) {
+      workflow.update({ active: checked }).save();


This should still have a catch

- Move promises object inside .then() block of Dialog.alert. - Add catch block for updating workflow status request. - Update modal text. - Remove test for modal text content.

jelliotartz · 2017-10-18T16:33:14Z

I changed the .catch block on Dialog.alert to console.error instead of this.setState({ error }) because clicking on the background or typing escape to cancel the modal is throwing this error:

console.error throws these errors in the same use case:

Not sure if there's a better way to gracefully handle the onCancel functionality without throwing errors, although the behavior of aborting a check upon reading the modal message is now working as desired when the user hits escape or clicks the background.

srallen

I've never been a fan of the modal-form's design to co-opt native browser form functionality and use it with in a Promise. It was designed to reject the promise on cancellation, but that means we can't really catch actual errors in a useful way. How about instead of using the alert built into modal-form, we just manually handle the open/close state? There should be an example in the modal-form readme on how to do this.

jelliotartz · 2017-10-19T20:24:07Z

RFC: I added an id on the Dialog success and cancel buttons, so that I could target them in unit tests - happy to change this if there's a better or preferred way to target buttons that don't have classes (or to change the ids to classes).

srallen

I have a few more comments and rethinking how we handle the project and workflow update order. Thanks!

srallen · 2017-10-20T16:08:39Z

app/pages/admin/project-status.jsx

@@ -10,6 +10,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'


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

srallen · 2017-10-20T16:16:17Z

app/pages/admin/project-status.jsx

@@ -164,6 +210,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()}


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}`}

srallen · 2017-10-20T16:28:23Z

app/pages/admin/project-status.jsx

+      this.state.project.update({ 'configuration.default_workflow': undefined }).save()
+    ];
+
+    Promise.all(promises)


I'm rethinking the use of Promise.all() since the possibility of a failure condition on the project PUT. We don't want the workflow changed if updating the default workflow fails. It made sense initially to have a single then() which Promise.all() allows us, but I'm thinking this will be safer:

this.state.project.update({ 'configuration.default_workflow': undefined }).save() .then(() -> { workflow.update({ active: false }).save() .then(this.getProjectAndWorkflows() // Only get project and workflows after successful project and workflow PUT ).catch(// its own catch); }).catch(// its own catch) .then(() => { this.setState({ dialogIsOpen: false }); })

This is just a rough idea without testing, so it might need tweaks.

srallen · 2017-10-20T16:32:14Z

app/pages/admin/project-status.jsx

+      this.setState({ dialogIsOpen: true });
+    }
+
+    if (defaultWorkflowId !== workflow.id) {


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))

srallen · 2017-10-20T16:36:06Z

app/pages/admin/workflow-default-dialog.jsx

+        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 />


I missed seeing the <br /> last review. If the goal is to have a bit of space, then perhaps use a <p> around the text instead of a<div> and a <br />. Paragraphs are also block-level and browsers default to given them margins too.

srallen · 2017-10-20T16:37:43Z

app/pages/admin/workflow-default-dialog.spec.js

+  });
+
+  it('calls the onSuccess handler', () => {
+    wrapper.find('button#workflowDefaultDialogSuccess').simulate('click');


I'd prefer not adding CSS ids or classes to elements just for the purposes of testing. Since there are only two buttons using enzyme's first() or last() is fine or at()

- Refactor handleDialogSuccess to remove Promise.all() - Update error rendering to display status and msg from error obj. - Update modal UI to separate buttons from text with <p>. - Remove button ids, target via first() and last() for tests. - Fix linter errors.

jelliotartz · 2017-10-20T18:18:33Z

I'm seeing some weird behavior when toggling a default workflow from active to inactive - when there is more than one workflow, it toggles the last workflow, rather than the one that was the default. The default setting is getting removed though. Wanted to check and see if you can reproduce this issue though, in case it's a cache bug again (I tried on Firefox though, got same result, so I don't think it is).

Also, on my local, the workflow being passed into handleDialogSuccess is always the last workflow. I'm trying different ways to pass the workflow to handleDialogSuccess, but so far haven't found a way to modify onSuccess={this.handleDialogSuccess.bind(null, workflow)}

jelliotartz · 2017-10-20T21:08:23Z

Got a working solution to this ^^ issue (with a little input from Mark - thx!) - I think what was happening was that because dialogIsOpen is initally false, binding handleDialogSuccess to the workflow wasn't doing anything on initial render. When I change state of dialogIsOpen in handleToggle, I think it's only triggering a render for WorkflowDefaultDialog, and not the whole page - and thus the workflows don't get mapped again, and the bind is getting set on the last workflow every time.

To fix this, I stored the default workflow id in state, and no longer pass it in the onSuccess event. In handleDialogSuccess I get the desired workflow by filtering all the workflows and matching the state-stored default workflow id.

Happy to change or consider a different solution of course :)

jelliotartz · 2017-10-20T21:22:18Z

app/pages/admin/project-status.jsx

@@ -85,17 +86,21 @@ class ProjectStatus extends Component {
    this.setState({ dialogIsOpen: false });
  }

-  handleDialogSuccess(workflow) {
+  handleDialogSuccess() {
+    const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.defaultWorkflowId);


I wanted to call this variable workflow, but that raised a linter error ('workflow' is already declared in the upper scope), so I went with defaultWorkflow, which I guess is more explicit anyway...

Good use of array.filter(), however, we can sidestep storing the default workflow id in the component state since it's already stored in the project resource:
const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.project.configuration.default_workflow);

srallen

Just have a minor change request: good idea to use the filter, but storing the default workflow id in the state is redundant.

srallen · 2017-10-23T14:36:24Z

app/pages/admin/project-status.jsx

@@ -85,17 +86,21 @@ class ProjectStatus extends Component {
    this.setState({ dialogIsOpen: false });
  }

-  handleDialogSuccess(workflow) {
+  handleDialogSuccess() {
+    const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.defaultWorkflowId);


Good use of array.filter(), however, we can sidestep storing the default workflow id in the component state since it's already stored in the project resource:
const defaultWorkflow = this.state.workflows.filter(workflow => workflow.id === this.state.project.configuration.default_workflow);

srallen · 2017-10-23T14:37:30Z

app/pages/admin/project-status.jsx

@@ -106,7 +111,10 @@ class ProjectStatus extends Component {
    const defaultWorkflowId = this.state.project.configuration.default_workflow;

    if (defaultWorkflowId === workflow.id && workflow.active) {
-      this.setState({ dialogIsOpen: true });
+      this.setState({
+        defaultWorkflowId,


This is redundant with the default_workflow already a property on the project resource which is already stored in the component state.

jelliotartz added 2 commits October 17, 2017 10:51

WIP: Add modal warning for removal of default workflow.

d2f9a3e

Refactor to use Promise.all() for requests.

79a8bee

jelliotartz requested a review from srallen October 17, 2017 16:27

jelliotartz force-pushed the default-workflow-alert branch from de248cc to 79a8bee Compare October 17, 2017 16:30

jelliotartz added 2 commits October 17, 2017 13:38

Cleanup linter errors.

b1f82a1

Add unit tests for WorkflowDefaultDialog.

335c892

srallen suggested changes Oct 17, 2017

View reviewed changes

srallen reviewed Oct 17, 2017

View reviewed changes

jelliotartz added 2 commits October 17, 2017 19:31

Address requested changes.

67ddefd

- Remove Translation component and unused code. - Use Promise.all for update requests. - Add class method for wrkflow & proj GET req's. - Move WorkflowDefaultDialog to admin folder.

Bind getProjectAndWorkflows() in constructor.

d420426

srallen suggested changes Oct 18, 2017

View reviewed changes

Address requested changes.

d436d4f

- Move promises object inside .then() block of Dialog.alert. - Add catch block for updating workflow status request. - Update modal text. - Remove test for modal text content.

srallen reviewed Oct 18, 2017

View reviewed changes

jelliotartz added 2 commits October 19, 2017 11:26

WIP: manually handle modal form open/close state.

3643627

Fix broken test. Close dialog in successful promise.

e32fbd0

srallen suggested changes Oct 20, 2017

View reviewed changes

Get defaultWorkflowId from state.

52c8723

jelliotartz force-pushed the default-workflow-alert branch from 8f7f9c3 to 52c8723 Compare October 20, 2017 21:19

jelliotartz commented Oct 20, 2017

View reviewed changes

srallen suggested changes Oct 23, 2017

View reviewed changes

Refactor defaultWorkflow filter.

c1d030c

srallen approved these changes Oct 24, 2017

View reviewed changes

srallen merged commit 34aa0b4 into master Oct 24, 2017

srallen deleted the default-workflow-alert branch October 24, 2017 15:24

This was referenced Oct 25, 2017

Add check for default workflow #4118

Merged

ProjectStatus unit tests #4127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow default dialog alert #4100

Workflow default dialog alert #4100

jelliotartz commented Oct 17, 2017 •

edited

Loading

srallen left a comment

srallen Oct 17, 2017

srallen Oct 17, 2017

srallen Oct 17, 2017

srallen Oct 17, 2017

srallen Oct 17, 2017

srallen left a comment

jelliotartz commented Oct 17, 2017

srallen left a comment

srallen Oct 18, 2017

srallen Oct 18, 2017

srallen Oct 18, 2017

srallen Oct 18, 2017

srallen Oct 18, 2017

jelliotartz commented Oct 18, 2017 •

edited

Loading

srallen left a comment

jelliotartz commented Oct 19, 2017

srallen left a comment

srallen Oct 20, 2017

srallen Oct 20, 2017

srallen Oct 20, 2017

srallen Oct 20, 2017

srallen Oct 20, 2017

srallen Oct 20, 2017

jelliotartz commented Oct 20, 2017

jelliotartz commented Oct 20, 2017 •

edited

Loading

jelliotartz Oct 20, 2017 •

edited

Loading

srallen Oct 23, 2017

srallen left a comment

srallen Oct 23, 2017

srallen Oct 23, 2017

Workflow default dialog alert #4100

Workflow default dialog alert #4100

Conversation

jelliotartz commented Oct 17, 2017 • edited Loading

Describe your changes

Review Checklist

Optional

srallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srallen left a comment

Choose a reason for hiding this comment

jelliotartz commented Oct 17, 2017

srallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelliotartz commented Oct 18, 2017 • edited Loading

srallen left a comment

Choose a reason for hiding this comment

jelliotartz commented Oct 19, 2017

srallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelliotartz commented Oct 20, 2017

jelliotartz commented Oct 20, 2017 • edited Loading

jelliotartz Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelliotartz commented Oct 17, 2017 •

edited

Loading

jelliotartz commented Oct 18, 2017 •

edited

Loading

jelliotartz commented Oct 20, 2017 •

edited

Loading

jelliotartz Oct 20, 2017 •

edited

Loading