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

Create Volumetric Task #7227

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Create Volumetric Task #7227

wants to merge 2 commits into from

Conversation

kieftrav
Copy link
Contributor

@kieftrav kieftrav commented Nov 27, 2024

Staging branch URL: https://pr-7227.pfe-preview.zooniverse.org

Describe your changes.

  • Rename experimental flag for the Volumetric Viewer to "volumetricProject" from "volumetricViewer"
  • Create Volumetric task that it can be added to any project with the "volumetricProject" and "FemLab" flags enabled

Required Manual Testing

  • Open the Admin Page and the Workflow Editor in separate tabs for my test/staging Volumetric Viewer project Volumetric Viewer project for testing
  • As of this writing, "FemLab" and "volumetricViewer" are enabled and you should see a Volumetric task existing on workflow 3831

Things you should be able to do:

  • Removing the Volumetric Task
  • Adding a new Volumetric Task
  • Saving / refreshing and the Volumetric Task still exists + works
  • Disabling the "volumetricViewer" flag should prevent the use from adding a Volumetric task
  • You should be able to add the Volumetric to any other project of your choosing as long as the two experimental flags are enabled

Requests for PR Review

  • The Volumetric task was heavily inspired by the Highlighter task. I do not know if everything in that file is necessary/right/appropriate/correct. This is the file that might need the most review/iteration because I lack context on all its implications elsewhere in the codebase
  • If there's something glaringly obvious/missing, please schedule a call/screenshare to walk through the details (I prefer that to a voluminous wall of text).
  • If there is a place to write a test, I'm happy to do that. I'm not sure what/where would make sense though, if so.

Standard Tests

  • Does the non-logged in home page render correctly?
  • Does the logged in home page render correctly?
  • Does the projects page render correctly?
  • Can you load project home pages?
  • Can you load the classification page?
  • Can you submit a classification?
  • Does talk load correctly?
  • Can you post a talk comment?

Review Checklist

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you npm ci and app works as expected?
  • 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 GitHub Actions?

Optional

@coveralls
Copy link

coveralls commented Nov 27, 2024

Coverage Status

coverage: 56.699% (-0.006%) from 56.705%
when pulling 3f00f50 on VV-Task
into 39a09e8 on master.

@shaunanoordin shaunanoordin self-requested a review November 27, 2024 16:04
@shaunanoordin shaunanoordin self-assigned this Nov 27, 2024
Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

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

PR Review

Affects: Project Builder -> Workflow Editor (FEM Lab only)

This PR adds the "VolumetricTask" Task to the Workflow Editor.

On the project builder:

  • If a project has the volumetricViewer and FEM Lab experimental tools enabled, then the "Volumetric Task" will be listed as an option when adding a new Task.

On the PFE classifier:

  • If a volunteer attempts to load a Volumetric Task on the PFE classifier, then the Task area will just show a placeholder with no input controls.
  • The Volumetric Task is only valid on the FEM classifier.

Functionality checks out fine, but I've added a few comments to improve the code.

Testing

Testing project builder, on localhost:

I'm able to create and edit a VolumetricTask with no issues.

⚠️ NOTE: whether or not the structure of the VolumetricTask is correct for the needs of the corresponding code on FEM is a separate question.

Testing PFE classifier, on localhost:

Workflow loads fine. A placeholder is show in place of the Task. Nothing crashes.

Testing FEM classifier, on localhost, with app-project:

Workflow loads fine.

⚠️ NOTE: a more thorough testing of the interaction between VolumetricTask on PFE project builder + Volumetric Task on FEM Classifier should be done with a project/workflow that's FULLY set up to use the Volumetric Viewer + Volumetric Viewer Data + Volumetric Task (e.g. https://local.zooniverse.org:3000/projects/kieftrav/volumetric-viewer/classify/workflow/3831?env=staging) but that's a step too far for this PR review.

Status

LGTM, this meets minimum safety checks - functionality works as advertised for the project builder, and nothing seems to crash on PFE as a whole. 👍

I understand that the VolumetricTask is still in development, so my questions about e.g. "what's the structure of the VolumetricTask annotation?" should be deferred until those details are confirmed. (Or maybe I missed the PR/ADR on FEM.) We can always revisit this code to tweak, anyhoo.

Regarding my comments on the code in this PR:

  • my main concern is that the default props, prop types, etc shouldn't add code that's unused, e.g. I don't think VolumetricTask uses tools.
    • I'm assuming things like tools, _toolIndex etc are leftovers from using the code pattern from similar Tasks (Drawing Task?)
    • If I'm wrong about this, please let me know - perhaps I need to those questions about annotation structure earlier.
  • I'm also in favour of making the code clearer for future devs to read, so some things like the placeholder text on render() could use a minor text improvement.

app/classifier/tasks/volumetric/index.jsx Outdated Show resolved Hide resolved
Comment on lines +33 to +35
workflow: {
tasks: [],
},
Copy link
Member

Choose a reason for hiding this comment

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

Query: what's the workflow used for?

If we're following the same Task pattern in PFE, then this parameter isn't required. We only really need task and showRequiredNotice.

(In practice, it double doesn't matter, since this Task shouldn't be working in PFE. 🤔 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome - thank you for the context. Removed.


VolumetricTask.getTaskText = (task) => task.instruction;

VolumetricTask.getDefaultAnnotation = () => ({ _toolIndex: 0, value: [] });
Copy link
Member

Choose a reason for hiding this comment

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

Does this task annotation actually use _toolIndex? If not, then this could just be VolumetricTask.getDefaultAnnotation = () => ({}); to avoid any confusion.

Comment on lines 38 to 50
VolumetricTask.propTypes = {
showRequiredNotice: PropTypes.bool,
task: PropTypes.shape({
help: PropTypes.string,
instruction: PropTypes.string,
required: PropTypes.bool,
tools: PropTypes.array,
type: PropTypes.string,
}),
workflow: PropTypes.shape({
tasks: PropTypes.object,
}),
};
Copy link
Member

Choose a reason for hiding this comment

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

Query: are propTypes.task.tools and propTypes.workflow actually needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed - didn't need them.

super(props);
}
render() {
return <div>{"VolumetricTask Tool"}</div>;
Copy link
Member

Choose a reason for hiding this comment

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

[minor] This could be a bit more descriptive. e.g. "This is a placeholder for the Volumetric Task, which is only supposed to work on the FEM Classifier."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed and implemented.

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.

3 participants