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

Optimize all workflows request #3556

Merged
merged 8 commits into from Mar 17, 2017

Conversation

Projects
None yet
3 participants
@srallen
Member

srallen commented Mar 7, 2017

Builds on #3545. This refactors the workflow fetch to be in the project page controller component. I still need to do some work on it, but wanted some initial thoughts. I wanted to eliminate as much as possible fetching for all active workflows that has an arbitrary page size number. For getting a random workflow, I've decided to just pick out of the project.links.workflows array, trying to fetch it with the active: true param and if it comes back undefined, try again. But I'm not sure if this is better than current behavior.

https://optimize-all-workflows-request.pfe-preview.zooniverse.org

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?
  • Did you get any type checking errors from Babel Typecheck

Optional

  • If it's a new component, is it in ES6? Is it clear of warnings from ESLint?
  • 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 added in flow type annotations?

@srallen srallen requested review from eatyourgreens and camallen Mar 7, 2017

@mention-bot

This comment has been minimized.

mention-bot commented Mar 7, 2017

@srallen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @amyrebecca, @eatyourgreens and @camallen to be potential reviewers.

@srallen srallen added the SGL label Mar 7, 2017

@srallen srallen force-pushed the optimize-all-workflows-request branch from a5fdbf9 to 3b64510 Mar 8, 2017

@srallen srallen changed the base branch from optimize-home-requests to master Mar 8, 2017

@srallen srallen force-pushed the optimize-all-workflows-request branch from 8eb6f69 to b0a6dec Mar 9, 2017

@srallen

This comment has been minimized.

Member

srallen commented Mar 9, 2017

I've rebased and updated the staging link. I think I'll give writing a few useful tests a try because I'm not confident I'm not breaking something somewhere that I didn't think to check.

@@ -33,6 +35,15 @@ export default class ProjectHomeContainer extends React.Component {
}
}
fetchAllWorkflows(props = this.props, query = { active: true, fields: 'active,completeness,configuration,display_name' }) {

This comment has been minimized.

@eatyourgreens

eatyourgreens Mar 10, 2017

Member

Are default params ok to use here? MDN says no support in IE or Opera. https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Default_parameters

@srallen srallen force-pushed the optimize-all-workflows-request branch from b0a6dec to 8d56473 Mar 10, 2017

@srallen

This comment has been minimized.

Member

srallen commented Mar 10, 2017

I removed the default params. I was unaware of the lack of support in those browsers. There are several other places in the code we should probably rework that I know are using default params. Thanks for the heads up about that.

@srallen

This comment has been minimized.

Member

srallen commented Mar 10, 2017

FYI, I'm working on a bug with the tutorial getting null preferences sometimes.

@srallen srallen force-pushed the optimize-all-workflows-request branch from 8d56473 to 4985c17 Mar 11, 2017

@srallen srallen changed the title from [WIP] Optimize all workflows request to Optimize all workflows request Mar 11, 2017

@srallen

This comment has been minimized.

Member

srallen commented Mar 11, 2017

There's more work to do with reworking the request for preferences and the listener for it as well as moving the actual workflow request into the classifier, but those should be separate branches to stay in scope here.

For the testing, I'm having difficulty with figuring out good tests and API mocking, so I'd like to do that separately too. I don't think I'm breaking anything with the changes in this as is, but we'll find out quickly once merged like with the task rewrite.

@eatyourgreens

This comment has been minimized.

Member

eatyourgreens commented Mar 14, 2017

Not logged in, I'm able to get a GZ3D workflow showing in Muon Hunters, if I go to GZ3D, pick a workflow, then navigate to Projects > Muon Hunters > Get Started.

@srallen srallen force-pushed the optimize-all-workflows-request branch 2 times, most recently from 9b0eb7a to 122f749 Mar 14, 2017

@srallen

This comment has been minimized.

Member

srallen commented Mar 15, 2017

@eatyourgreens that bug should be fixed now. The preferences handling needs a lot of work and some thought and this is partially related to unexpected behavior from the API client's handling of resource caching.

Basically what's happening is this: fetchProjectData is called in componentWillReceiveProps when the path is changed. When the project data is fetched, this includes the call to get the user project preferences. For logged out users, the API client is leveraged to create temporary guest preferences that can be written to. What I'm finding is that when you visit a workflow that allows you to choose, like GZ:3D, the selection is written to the guest preferences. Then, when you navigate to a new project, what should be happening is new guest preferences are created with an empty object in the preferences property.

Instead what is happening is that it still has the selected_workflow that was stored from GZ:3D, though the project.id in its links is correctly updated to the new project. I'm not sure why this would be happening unless that call to create is hitting the client's cache and only partially updating the guest preferences on the create call.

So the preferences still have a selected_workflow of the last project you visited, it attempts to load that workflow. I fixed the workflow fetch to include the project id, so at least this will now fail and then call clearInactiveWorkflow, clear out the selected_workflow id in the guest preferences and attempt the workflow selection again.

@@ -60,11 +60,11 @@ ProjectPageController = React.createClass
componentWillUpdate: (nextProps, nextState) ->
if nextProps.location.query?.workflow? and @canFetchWorkflowByQuery(nextProps.project, nextProps.user)

This comment has been minimized.

@eatyourgreens

eatyourgreens Mar 17, 2017

Member

Should nextProps.project be nextState.project here?

This comment has been minimized.

@srallen

srallen Mar 17, 2017

Member

Yep. Thanks for catching that.

@eatyourgreens

This comment has been minimized.

Member

eatyourgreens commented Mar 17, 2017

Looks good to me. One small change where I think you've missed a change from props to state.

Personally, I still find all the if statements in getSelectedWorkflow very hard to follow, and potentially very hard to extend or maintain, and I think workflow selection should be split out into its own class or component, but that is a discussion for zooniverse/rfc#12.

@eatyourgreens

This comment has been minimized.

Member

eatyourgreens commented Mar 17, 2017

Might need an update to include #3621

@srallen srallen force-pushed the optimize-all-workflows-request branch from affb4c0 to d0fcc0a Mar 17, 2017

@srallen

This comment has been minimized.

Member

srallen commented Mar 17, 2017

@eatyourgreens this is rebased now to include #3621. I also agree about extracting out workflow selection into its own class. Good idea.

@eatyourgreens eatyourgreens merged commit f68b6b2 into master Mar 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@eatyourgreens eatyourgreens deleted the optimize-all-workflows-request branch Mar 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment