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

Pages Editor: implement Delete Page action #7046

Merged
merged 11 commits into from
Feb 29, 2024
Merged

Conversation

shaunanoordin
Copy link
Member

@shaunanoordin shaunanoordin commented Feb 16, 2024

PR Overview

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

This PR adds the ability to delete a Step/Page.

  • Clicking on the "trash can" icon on a Page will prompt a confirmation dialog, before deleting a Step/Page.
  • Any Branching Task with answers that pointed to the now-deleted Page will be updated, so the answers now point to nothing (i.e. "Submit").
    • A new cleanupTasksAndSteps() helper function has been added to remove orphaned references in branching tasks, (this has been implemented). This maaay be updated in the future to also remove Tasks that aren't associated with any Steps, and remove Steps that have 0 Tasks. We'll see.

Other changes:

  • Fixed an issue where deleting Answers in a Single Question Task (by clicking on the "minus" button) didn't actually register changes.

Notes:

  • ⚠️ The "Preview Workflow" button is still hardcoded to WF 3711. Shaun: please remember to fix this by loading the Project resource in DataManager in the next PR.

Status

Ready for review!

@shaunanoordin shaunanoordin requested a review from a team February 16, 2024 19:42
@coveralls
Copy link

coveralls commented Feb 16, 2024

Coverage Status

coverage: 56.98%. remained the same
when pulling 0257819 on pages-editor-pt14
into 3c979c8 on master.

@shaunanoordin
Copy link
Member Author

PR Update

  • Bug & Fix: deleting Pages caused invalid values to persist in the previous step's stepX.next. cleanupTasksAndSteps() has now been updated to also call linkStepsInWorkflow() to ensure linked pages remain valid.

@kieftrav kieftrav requested review from kieftrav and removed request for a team February 22, 2024 16:15
@shaunanoordin
Copy link
Member Author

shaunanoordin commented Feb 23, 2024

I can't believe I forgot to add in a Testing section.

Testing

In this PR, we're testing the "Delete Pages" action. (Dev Reminder: pages == steps. "Page" is what the volunteer sees. "step" is what's in the code.)

Testing steps:

  • Open the testing URL. You should be viewing the Tasks of a test Workflow (e.g. staging WF 3711), on the Pages Editor.
  • Create a simple workflow with multiple Pages, with at least one branching Page.
    • (For convenience, a "Quick Setup" button is available in the debug panel at the bottom of the page.)
  • Click on the Delete button (trashcan icon) to delete a Page. There should be a confirmation dialog before this happens, tho.
    • Expectation 1: the Page and all its Tasks should be removed from the workflow.
      • e.g. when deleting page P0 with task T0, check that workflow.tasks and workflow.steps in the Workflow resource have P0 and T0 removed. This can only be checked via observing network requests or inspecting the resource.
    • Expectation 2: "Any task.next = deletedPage should become task.next = undefined". Any branching options that pointed to a now-deleted Page should now point to "Submit". This should be visible immediately on the Pages Editor.
    • Expectation 3: "Any step.next = deletedPage should become step.next = undefined". Any non-branching Page automatically point to the next Page in the sequence. If P2 -> P3 -> P4, and P3 is deleted, the sequence of pages should now be P2 -> P4. This can be tested by classifying the workflow.

Basically, the big hidden rule that Expectations 1 to 3 attempts to codify is that, when you delete a Page and all its Tasks, then ALL SECONDARY REFERENCES to that Page and Task should ALSO be nuked, and the workflow should continue working as if they never existed.

Example: a 3 Page workflow created via Quick Setup. The first page (with T0) is a branching Page that points to the second page (with T1) and the third page (with T2).

image

Copy link
Contributor

@kieftrav kieftrav left a comment

Choose a reason for hiding this comment

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

LGTM! I like the clever use of experimentalQuickSetup(). It made me think about testing strategy once this gets plopped over into FEM. Might be useful to create a document of interesting edge cases / concerns / outline of all useful tests to be written when this gets migrated to FEM.

@shaunanoordin
Copy link
Member Author

Thanks Travis! 👍

Might be useful to create a document of interesting edge cases / concerns / outline of all useful tests to be written when this gets migrated to FEM.

Ooo good point, I'm going to need to bring up the idea of starting this doc, to Sam & Sean next Monday.

@shaunanoordin shaunanoordin merged commit a8bcd3d into master Feb 29, 2024
5 checks passed
@shaunanoordin shaunanoordin deleted the pages-editor-pt14 branch February 29, 2024 16:04
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.

None yet

3 participants