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

Add check for default workflow #4118

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Add check for default workflow #4118

merged 2 commits into from
Oct 25, 2017

Conversation

jelliotartz
Copy link
Contributor

Describe your changes

  • Adds a check for presence of configuration attribute (where default_workflow is stored) on a project before conditionally rendering an asterisk next to the default workflow on the admin/project-status page.
    • If no default workflow is set, it will display 'No workflows found' (same behavior as before this PR), but now should throw no errors in the console or interfere with the UI.
    • Issues were found with toggles freezing on new projects that have no default workflow set. (tagging @mrniaboc here for reference).
  • Staged here.
  • Related: Workflow default dialog alert #4100.

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

@jelliotartz
Copy link
Contributor Author

PR Update

Was able to produce the same error (can't read property default_workflow of undefined) in the console when attempting to toggle the workflow active setting of a test project with one workflow and no default_workflow set - so adding a check for this.state.project.configuration in the handleToggle function to handle this use case.

Copy link
Contributor

@srallen srallen left a comment

Choose a reason for hiding this comment

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

Can this be rebased with master? Other than that, it looks ready to go. Thanks!

@jelliotartz
Copy link
Contributor Author

Rebased :)

@srallen
Copy link
Contributor

srallen commented Oct 25, 2017

@jelliotartz Would you be up for writing unit tests for the project status page? Not a high priority, but another thing to do if you have time. Thanks!

@srallen srallen merged commit 8c73f22 into master Oct 25, 2017
@srallen srallen deleted the default-workflow-check branch October 25, 2017 16:30
@jelliotartz
Copy link
Contributor Author

sure, will do!

@jelliotartz jelliotartz mentioned this pull request Oct 30, 2017
11 tasks
mcbouslog pushed a commit to mcbouslog/Panoptes-Front-End that referenced this pull request Nov 13, 2017
* Add check for default workflow.

* Add project configuration check in handleToggle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants