Pages Editor: implement Associated Subject Sets & fetch more data #7048

shaunanoordin · 2024-02-23T18:14:10Z

PR Overview

Part of: Pages Editor MVP project and FEM Lab super-project
Follows #7046
Staging branch URL: https://pr-7048.pfe-preview.zooniverse.org/lab/1982/workflows/editor/3711?env=staging

This PR implements the "Associated Subject Sets" feature on the Workflow Settings sub-page.

New component: AssociatedSubjectSets.
New behaviour/feature: on Pages Editor (for any Workflow) -> Workflow Settings...
- The Associated Subject Sets feature should list all Subject Sets available to the Project (caveat: up to a max of 250. Paging may be a future problem to solve.)
- The Associated Subject Sets feature (and no, I will not shorten it to an acronym) should show which Subject Sets are linked to the Workflow via ticked checkboxes.
- Clicking on the checkboxes should link/unlink Subject Sets to/from the Workflow.

Also, some background work was required to fetch the Subject Sets, so the DataManager was updated as well.

Updated behaviour: DataManager now fetches the project resource.
Misc update: the Preview Workflow link now works as intended.

Screenshot: example workflow with 3 available subject sets, 2 of which are linked.

Testing

Open the staging URL. You should be looking at a test workflow (e.g. staging wf 3711)
Click on the Workflow Settings tab/button to view the sub-page.
Look for the Associated Subject Sets feature.
- It should list all available subject sets, and show which ones are already linked.
- Click on a subject set to link/unlink them
- Reload the page to confirm
- Expectation: you should be able to link or unlink as many sets as you want. Changes should be committed to Panoptes.

Status

Ready for review!

This PR is currently targeting pages-editor-pt14 for review purposes. Do not merge until 6974 is ready and this branch's merge is re-targeted to master.

kieftrav

LGTM with only a couple comments. One thing I wanted to surface is that I expected refreshing the page to keep me on the same tab "workflow settings".. Is this a bug? Or desired behavior? Personally, when a tab changes 50%+ of the page I assume refreshing the page / pushing the back button will take me back to the previous state. In this case, the back button also doesn't take me back to the "workflow settings" tab. This also might be future work so take this comment as is helpful!

kieftrav · 2024-02-27T14:01:09Z

app/pages/lab-pages-editor/DataManager.jsx

          workflow: null,
          status: 'error'
        });
      }
    }

-    fetchWorkflow();
+    fetchProjectAndWorkflow();


Do we want an await here?

An await is not needed here, as we want the fetch action to run asynchronously in the background; the state.apiData.status is set to fetching so users get to see a loading message while the data fetch occurs. 👌

kieftrav · 2024-02-27T14:04:52Z

app/pages/lab-pages-editor/components/TasksPage/TasksPage.jsx

@@ -140,7 +140,8 @@ export default function TasksPage() {
    update({ tasks: newTasks });
  }

-  const previewUrl = 'https://frontend.preview.zooniverse.org/projects/darkeshard/example-1982/classify/workflow/3711?env=staging';
+  const previewEnv = getPreviewEnv();
+  const previewUrl = `https://frontend.preview.zooniverse.org/projects/${project?.slug}/classify/workflow/${workflow?.id}${previewEnv}`;


Should we maybe do something different here if there isn't a project slug and workflow id? Because if its missing and it gets clicked on I would rather it send them somewhere predictable/debuggable instead of a default weird url with null/undefined/ect in it.

Let me think about this. 🤔

You're right that a URL like https://frontend.preview.zooniverse.org/projects/undefined/classify/workflow/undefined would be pretty wonky. But it's a fairly safe assumption every project has a slug and every workflow has an ID. But but it's still good to have a safety for when those assumptions bork.

kieftrav · 2024-02-27T14:07:48Z

app/pages/lab-pages-editor/components/TasksPage/TasksPage.jsx

@@ -230,3 +231,21 @@ export default function TasksPage() {
    </div>
  );
 }
+
+function getPreviewEnv() {


This function feels too important to bury in a file... I'd personally want something like this (with clear exceptions / responsive to our development URL schema) surfaced a bit higher. I'ts own file, part of a helper, or something that makes its purpose more surfaceable.

kieftrav · 2024-02-27T14:10:50Z

app/pages/lab-pages-editor/components/WorkflowSettingsPage/components/AssociatedSubjectSets.jsx

+        });
+
+      } catch (err) {
+        console.error('AssosicatedSubjectSets: ', err);


Misspelling and would be useful to indicate its an error in the console.log(). I personally like using the following format:

console.log("AssociatedSubjectSets() fetchSubjectSets() error", err);

That way you have a guide in the console on how to find that console log (my future self really appreciates these guideposts). Only a suggestion though.

Derp! I definitely need to fix that misspelling, thank you.

And thank you for the suggestion, I might also need to revise how I write our error messages. (At the moment I'm making the standards up as I go.)

shaunanoordin · 2024-02-29T17:46:41Z

Thanks Travis! I'm going to rebase, then address some of the smaller feedback in this PR, before merging. 👍

To respond to the point you raised:

I expected refreshing the page to keep me on the same tab "workflow settings".

Good point. Perhaps ideally, each "tab" should be its own sub-page with a URL, like https://local.zooniverse.org:3735/lab/1982/workflows/editor/3711/workflow-settings. However, at the moment the constraint I've imposed on myself is that the PagesEditor should be as self-contained as possible, so using a single path (/lab/PROJECT_ID/workflows/editor/WORKFLOW_ID) for all the PagesEditor tabs/sub-pages/features allows me to side step adding router dependencies inside the PagesEditor (which would've been somewhat annoying should we plan to port this to FEM.)

EDIT: Wait, a thought occurs: ?tab=workflowSettings param instead of /workflow-settings subpath to circumvent having to use react-router? Worth experimenting with later.

…t project resource.

coveralls · 2024-02-29T17:51:27Z

coverage: 56.98%. remained the same
when pulling 8dc7206 on pages-editor-pt15
into a8bcd3d on master.

shaunanoordin requested a review from a team February 23, 2024 18:14

kieftrav requested review from kieftrav and removed request for a team February 27, 2024 13:55

kieftrav approved these changes Feb 27, 2024

View reviewed changes

shaunanoordin mentioned this pull request Feb 28, 2024

Pages Editor: implement Associated Tutorials #7052

Merged

shaunanoordin force-pushed the pages-editor-pt14 branch from a0361b0 to 0257819 Compare February 29, 2024 15:52

Base automatically changed from pages-editor-pt14 to master February 29, 2024 16:04

shaunanoordin added 13 commits February 29, 2024 17:47

pages-editor-pt15: prepare WorkflowSettingsPage for setting Subject Sets

1d4c3f7

Add AssociatedSubjectSets component

da72392

DataManager: fetch project AND workflow resources at the same time

0cc9cdc

DataManager: fix project missing after update. Add documentation abou…

cfa82f5

…t project resource.

TasksPage: dynamically generate Preview URL

4850bf7

AssociatedSubjectSets: fetch Subject Sets. Add status-banner design

78a9c72

DataManager: add status banners for fetching/error states

dbe3a03

PagesEditor: tweak style to add vertical padding to main container

44fec93

AssociatedSubjectSets: enable checkboxes for linking Subject Sets

ef0ad20

AssociatedSubjectSets: implement linking/unlinking Subject Sets

15e1b6e

WorkflowSettingsPage: style input-group element

88936c8

WorkflowSettingsPage: remove test sections

85aa71b

PagesEditor: revert default tab to be the Tasks tab

f3e5f37

shaunanoordin force-pushed the pages-editor-pt15 branch from 3a488d3 to f3e5f37 Compare February 29, 2024 17:48

shaunanoordin added 2 commits March 1, 2024 12:18

TasksPage: extract getPreviewEnv() into an external helper file

7a4ed2b

AssociatedSubjectSets: fix typo

8dc7206

shaunanoordin enabled auto-merge (squash) March 1, 2024 12:21

shaunanoordin merged commit 6742908 into master Mar 1, 2024
5 checks passed

shaunanoordin deleted the pages-editor-pt15 branch March 1, 2024 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pages Editor: implement Associated Subject Sets & fetch more data #7048

Pages Editor: implement Associated Subject Sets & fetch more data #7048

shaunanoordin commented Feb 23, 2024 •

edited

kieftrav left a comment

kieftrav Feb 27, 2024

shaunanoordin Feb 29, 2024

kieftrav Feb 27, 2024

shaunanoordin Feb 29, 2024

kieftrav Feb 27, 2024

kieftrav Feb 27, 2024

shaunanoordin Feb 29, 2024

shaunanoordin commented Feb 29, 2024

coveralls commented Feb 29, 2024 •

edited

Pages Editor: implement Associated Subject Sets & fetch more data #7048

Pages Editor: implement Associated Subject Sets & fetch more data #7048

Conversation

shaunanoordin commented Feb 23, 2024 • edited

PR Overview

Testing

Status

kieftrav left a comment

Choose a reason for hiding this comment

kieftrav Feb 27, 2024

Choose a reason for hiding this comment

shaunanoordin Feb 29, 2024

Choose a reason for hiding this comment

kieftrav Feb 27, 2024

Choose a reason for hiding this comment

shaunanoordin Feb 29, 2024

Choose a reason for hiding this comment

kieftrav Feb 27, 2024

Choose a reason for hiding this comment

kieftrav Feb 27, 2024

Choose a reason for hiding this comment

shaunanoordin Feb 29, 2024

Choose a reason for hiding this comment

shaunanoordin commented Feb 29, 2024

coveralls commented Feb 29, 2024 • edited

shaunanoordin commented Feb 23, 2024 •

edited

coveralls commented Feb 29, 2024 •

edited