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

Classifier: Simple dropdown task adapter #1830

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

eatyourgreens
Copy link
Contributor

This adds an adapter (and tests) for the simple dropdown task, so that we can test it out in staging. Dropdown tasks are converted to simple dropdown tasks if they contain only one select menu. More complex tasks are not transformed, and should throw an error in the classifier.

Snapshot of the dev classifier with a dropdown task that has three answers: One, Two or Three.
https://localhost:8080/?project=908&workflow=3339

Package:
lib-classifier

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

This adds an adapter (and tests) for the simple dropdown task, so that we can test it out in staging. Dropdown tasks are converted to simple dropdown tasks if they contain only one select menu. More complex tasks are not transformed, and should throw an error in the classifier.
@eatyourgreens eatyourgreens added the experiment Something to try out and gather more information on label Sep 29, 2020
@eatyourgreens
Copy link
Contributor Author

Trying this out in the dev classifier, I'm noticing that the simple dropdown from #1772 throws a store error if you use it to make an annotation.

@eatyourgreens
Copy link
Contributor Author

I guessed at the structure of the dropdown task when writing the tests, but a complex dropdown does (correctly) error when I try to load it in the dev classifier.

I'm not sure why submitting a classification crashes the classifier.

@eatyourgreens
Copy link
Contributor Author

Here's the store error, on submitting a classification and loading a new subject.

You are trying to read or write to an object that is no longer part of a state tree. 
(Object type: 'SimpleDropdownAnnotation', 
Path upon death: '/classifications/resources/ckfo2dagk00022a6a2hxqqkn4/annotations/ckfo2csub00012a6aa3d7c6ou', 
Subpath: 'task', 
Action: '/classifications/resources/ckfo2dagk00022a6a2hxqqkn4.addAnnotation()'). 
Either detach nodes first, or don't use objects after removing / replacing them in the tree.

@eatyourgreens
Copy link
Contributor Author

I'm seeing that crash on a workflow that uses a single choice question, so I don't think it has anything to do with the dropdown task.
https://localhost:8080/?project=908&workflow=3223

@eatyourgreens
Copy link
Contributor Author

I’m thinking we can use this to get the Engaging Crowds projects up and running, but I’ve tried to keep it out of the main classifier code so that it’s easily removed/reverted when it’s no longer needed.

Simple dropdowns should not be changed by the adapter.
@shaunanoordin shaunanoordin self-requested a review October 21, 2020 12:30
@srallen
Copy link
Contributor

srallen commented Oct 21, 2020

@shaunanoordin are you reviewing this? Just want to be sure and don't want to double up on work already being done. Thanks!

@shaunanoordin
Copy link
Member

@srallen I am reviewing this, but I'm focusing mostly on functionality and practical tests across different workflow permutations. Please feel free to add your own review; you have a much better eye for how things fit into the larger monorepo code, which I can easily miss. 👍

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

Requires: #1772
Package: lib-classifier

PR #1772 introduces the "simple-dropdown" Task (data model) and its corresponding "Simple Dropdown" Task component (the UI). This PR adds an adapter that handily transforms classic "dropdown" Tasks (data model) into the "simple-dropdown" Tasks.

This in turn allows classic dropdown tasks (created in the PFE project builder) to "work" using the new Simple Dropdown Task system, without having to 1. make the new Simple Dropdown Task system too backwards-compatible, or 2. change every existing classic dropdown Task in PFE to match the new standard.

  • The adapter only works on very basic (non-cascading) classic dropdown configurations.

Testing

Tested on localhost with macOS + Chrome

Case 1: simple-dropdown Task

URL: https://local.zooniverse.org:8080/?env=production&project=12754&workflow=16106&subjectSet=85771

  • We're testing against a simple-dropdown Task workflow.
  • This ensures that changes introduced in 1830 doesn't adversely affect the functionality introduced by 1772.
  • Tests look good. 👍 Simple Dropdown tasks continue to function properly. The panoptesAdapter doesn't trigger when the workflow Task is a simple dropdown. This is correct. 👍

Case 2: classic dropdown Task

URL: https://local.zooniverse.org:8080/?env=production&project=12754&workflow=15764
Compare with PFE: https://www.zooniverse.org/projects/darkeshard/engaging-the-crowds-2020/classify?reload=0&workflow=15764

  • We're testing the main functionality of this PR.
  • Baseline: without this PR, a classic dropdown Task will not show on the Task panel (i.e. it has no corresponding Task component)
  • With this PR: a classic (non-cascading) dropdown Task will appear on the Task panel using the Simple Dropdown component.
  • Tests look good. 👍 Classic dropdown tasks are 'transmuted' into Simple Dropdown tasks. The adapter triggers as expected.

⚠️ Important thing to note for any PFE projects being 'imported' to the monorepo: on PFE, dropdown tasks will have an annotation that looks like { value: "ad49e86d82968", option: true } while on monorepo, the same task will have annotations like { option: true, selection: 1 }

Dev Notes

Functionality looks good! 👍 I can't think of any other workflow permutations that need testing.

One thing I'd add is that it's probably a good idea to add a quick bit of documentation to explain panoptesAdapter.js and the context of its creation. Reading if (selects?.length === 1) makes sense to me, but others might scratch their head for longer than necessary.

Some // inline comments would be ace, a small README.md would be overkill but good, or maybe even...

const { selects } = snapshot
const isThisAClassicDropdownTaskThatNeedsToBeTransmogrifiedIntoASimpleDropdownTask = selects?.length === 1

if (isThisAClassicDropdownTaskThatNeedsToBeTransmogrifiedIntoASimpleDropdownTask) {
...

Status

This PR LGTM, though I do want to add a note that additional documentation to help people understand the context (i.e. currently very specific use) of the very general-sounding Panoptes Adapter.

@github-actions github-actions bot added the approved This PR is approved for merging label Oct 21, 2020
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.

I would suggest that the snapshot processor check for min and max number of options that is proposed from the ADR (See: #1767) and to not allow those that are less than or greater than the amount we've agreed to. An error could be thrown, an error state set, and a useful UI message communicated.

Secondly, I agree with @shaunanoordin that a read me for the snapshot preocessor would be helpful and that the naming panoptesAdapter isn't really descriptive.

I don't consider these blocking, but would highly recommend it.

@eatyourgreens
Copy link
Contributor Author

Let's merge these, so that Engaging Crowds can get going, then open a new PR with the suggestions.

@eatyourgreens eatyourgreens merged commit 99c2138 into classifier-dropdown-v3-simple Oct 22, 2020
@eatyourgreens eatyourgreens deleted the simple-dropdown-adapter branch October 22, 2020 08:24
eatyourgreens added a commit that referenced this pull request Oct 22, 2020
This PR adds a Simple Dropdown (type: dropdown-simple) task to the monorepo Classifier.
The Simple Dropdown functions like a HTML <select> which allows a single value to be selected
The Simple Dropdown is separate from the classic Dropdown (type: dropdown) from PFE
At the moment, the dropdown-simple task type cannot be built in the PFE Project Builder, and has to be set up by a dev.

* Classifier: Simple dropdown task adapter (#1830)

This adds an adapter (and tests) for the simple dropdown task, so that we can test it out in staging. Dropdown tasks are converted to simple dropdown tasks if they contain only one select menu. More complex tasks are not transformed, and should throw an error in the classifier.

Co-authored-by: Jim O'Donnell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging experiment Something to try out and gather more information on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants