refactor(classifier): make task.required a boolean #6094

eatyourgreens · 2024-05-15T13:16:40Z

Remove types.string from task.required. Fixes a small bug where a task with required: 'false' would be required. String values are now coerced to booleans in a snapshot preprocessor.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

lib-classifier/plugins/tasks

Linked Issue and/or Talk Post

similar to fix(classifier): surveyTask.alwaysShowThumbnails must be a boolean #6085, this handles any cases where the project builder accidentally saved boolean values as strings.

How to Review

This shouldn't change any live projects. The project builder nowadays should be saving task.required as a boolean, so the string type was only needed for very old workflows. You could deliberately create a workflow task with required: 'false', with an API client, but I've added tests to verify that legacy string values are handled correctly now.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

Refactoring

The PR creator has described the reason for refactoring
The refactored component(s) continue to work as expected

coveralls · 2024-05-15T13:23:32Z

coverage: 79.032% (+0.004%) from 79.028%
when pulling 3a1b004 on eatyourgreens:refactor-task-required
into e299c17 on zooniverse:master.

eatyourgreens · 2024-05-15T13:24:58Z

packages/lib-classifier/src/plugins/tasks/models/Task.js

@@ -6,10 +6,22 @@ const Task = types.model('Task', {
  // override annotation in individual task models with specific annotation types
  annotation: types.safeReference(Annotation),
  taskKey: types.identifier,
-  required: types.maybe(types.union(types.string, types.boolean)), // text task required default = false
+  required: types.optional(types.boolean, false), // text task required default = false


Does anyone remember what the comment means here?

Mentioned in a PR description here: #1768 (comment)

That bug (the project builder stringifying everything) was fixed a long time ago, so the comment can be deleted.

goplayoutside3 · 2024-06-26T17:51:36Z

The project builder nowadays should be saving task.required as a boolean...

Per zooniverse/Panoptes-Front-End#6251

goplayoutside3 · 2024-06-26T17:59:48Z

packages/lib-classifier/src/plugins/drawingTools/models/tools/Tool/Tool.spec.js

@@ -22,22 +22,22 @@ describe('Model > DrawingTools > Tool', function () {
      {
        type: 'multiple',
        answers: ['apples', 'oranges', 'pears'],
-        required: '',
+        required: false,


This line was initially written as required: false in #1376, but changed to required: '' in #1768. Why?

Lots of other spec files still use required: '' such as Mark.spec, DrawingTask.spec, Task.spec, TextTask.spec, AnnotationStore.spec, and more. So which is correct?

Back in 2020, the project builder had a bug that converted all values to strings, including booleans from checkboxes. That was fixed a while ago. Checkboxes should be saved as true/false now.

This is the PR that fixed handleInputChange in the project builder, so checkboxes have been fixed since October 2020.

zooniverse/Panoptes-Front-End#5815

Shouldn't all other files (Mark.spec, DrawingTask.spec, Task.spec, TextTask.spec, AnnotationStore.spec, etc) be updated to required: false in this PR too?

This PR is agnostic about whether Panoptes stored task.required as a string or a boolean, so mocks can use either and should still be represented internally as booleans in the classifier (via the snapshot preprocessor.)

I've updated the tests here, where the tests expected task.required to be an empty string. Otherwise, they should be unchanged.

Shouldn't all other files (Mark.spec, DrawingTask.spec, Task.spec, TextTask.spec, AnnotationStore.spec, etc) be updated to required: false in this PR too?

That's a good question. I'd say not in this PR, since the tests are passing and a refactor must not change public tested behaviour, but maybe those mocks could be updated as maintenance for the classifier package? Those mocks mirror how the project builder behaved when they were written (five years ago, maybe?) Since then, bugs in the project builder have been fixed, and the new editor has been written. The mocks are now out of step with the tasks and steps that are created by current project builder code, but do reflect legacy workflows that were created by buggy code and may still exist in Panoptes.

Depending on how old live projects are, the mocks might need to reflect both the state of the project builder now, and the state of the project builder five years ago, in order to catch regressions that break backwards compatibility.

I think this is also an argument for breaking tasks out into their own NPM package, so that the project builder can import and use them along with the classifier. At the moment, task code is being edited in at least three places: the old PFE workflow editor, the new Pages Editor, and the task models here in the monorepo.

packages/lib-classifier/src/plugins/tasks/models/Task.js

Remove `types.string` from `task.required`. Fixes a small bug where a task with `required: 'false'` would be required. String values are now coerced to booleans in a snapshot preprocessor.

https://jrsinclair.com/articles/2021/rethinking-the-javascript-ternary-operator/

Tests should only be updated where they're failing on this branch.

eatyourgreens commented May 15, 2024

View reviewed changes

eatyourgreens force-pushed the refactor-task-required branch 7 times, most recently from be8797c to fca9cc2 Compare May 20, 2024 20:46

eatyourgreens force-pushed the refactor-task-required branch from fca9cc2 to 0b28270 Compare June 8, 2024 08:58

eatyourgreens force-pushed the refactor-task-required branch from 0b28270 to 9603648 Compare June 22, 2024 10:17

goplayoutside3 reviewed Jun 26, 2024

View reviewed changes

goplayoutside3 self-assigned this Jun 26, 2024

eatyourgreens commented Jun 26, 2024

View reviewed changes

packages/lib-classifier/src/plugins/tasks/models/Task.js Outdated Show resolved Hide resolved

eatyourgreens force-pushed the refactor-task-required branch 5 times, most recently from bc94077 to 692b35c Compare July 18, 2024 16:29

eatyourgreens and others added 4 commits July 23, 2024 13:41

refactor(classifier): make task.required a boolean

8116183

Remove `types.string` from `task.required`. Fixes a small bug where a task with `required: 'false'` would be required. String values are now coerced to booleans in a snapshot preprocessor.

replace if with a ternary operator

8ea4291

https://jrsinclair.com/articles/2021/rethinking-the-javascript-ternary-operator/

Remove an outdated comment

863da3a

revert changes to passing tests

3a1b004

Tests should only be updated where they're failing on this branch.

eatyourgreens force-pushed the refactor-task-required branch from 692b35c to 3a1b004 Compare July 23, 2024 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(classifier): make task.required a boolean #6094

refactor(classifier): make task.required a boolean #6094

eatyourgreens commented May 15, 2024 •

edited

Loading

coveralls commented May 15, 2024 •

edited

Loading

eatyourgreens May 15, 2024

goplayoutside3 Jun 26, 2024

eatyourgreens Jun 26, 2024

goplayoutside3 commented Jun 26, 2024

goplayoutside3 Jun 26, 2024

eatyourgreens Jun 26, 2024

eatyourgreens Jun 26, 2024

goplayoutside3 Jul 11, 2024

eatyourgreens Jul 12, 2024 •

edited

Loading

eatyourgreens Jul 14, 2024 •

edited

Loading

eatyourgreens Jul 14, 2024 •

edited

Loading

eatyourgreens Jul 14, 2024 •

edited

Loading

refactor(classifier): make task.required a boolean #6094

Are you sure you want to change the base?

refactor(classifier): make task.required a boolean #6094

Conversation

eatyourgreens commented May 15, 2024 • edited Loading

Package

Linked Issue and/or Talk Post

How to Review

Checklist

General

General UX

Refactoring

coveralls commented May 15, 2024 • edited Loading

eatyourgreens May 15, 2024

Choose a reason for hiding this comment

goplayoutside3 Jun 26, 2024

Choose a reason for hiding this comment

eatyourgreens Jun 26, 2024

Choose a reason for hiding this comment

goplayoutside3 commented Jun 26, 2024

goplayoutside3 Jun 26, 2024

Choose a reason for hiding this comment

eatyourgreens Jun 26, 2024

Choose a reason for hiding this comment

eatyourgreens Jun 26, 2024

Choose a reason for hiding this comment

goplayoutside3 Jul 11, 2024

Choose a reason for hiding this comment

eatyourgreens Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens commented May 15, 2024 •

edited

Loading

coveralls commented May 15, 2024 •

edited

Loading

eatyourgreens Jul 12, 2024 •

edited

Loading

eatyourgreens Jul 14, 2024 •

edited

Loading

eatyourgreens Jul 14, 2024 •

edited

Loading

eatyourgreens Jul 14, 2024 •

edited

Loading