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

Nextflow Samplesheet - Pagination #913

Merged
merged 41 commits into from
Feb 5, 2025
Merged

Conversation

ChrisHuynh333
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 commented Jan 24, 2025

What does this PR do and why?

This PR is for story STRY0016721.

This is a complete refactor on how the nextflow samplesheet samples table handles its data, to improve performance and responsiveness of the dialog when large sample numbers are selected by utilization of pagination and elimination of excess DOM elements.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other pull requests.

How to set up and validate locally

Because this PR entirely changes how the functionality of the samplesheet is handled, all previous samplesheet testing should be redone:

  1. Verify all the steps in PR879 still behave as described. Note: Error handling is now simply a text box above the table that lists the samples with an error
  2. Create and submit a workflow execution with edited parameters (project_name, assembler, random_seed), and verify after submission that the workflow contains the edited parameters and not the default values.
  3. Create an automated workflow execution as described in PR585 and verify it still works/behaves as expected
  4. Edit an automated workflow execution as described in PR594 and verify it still works/behaves as expected

Test Pagination:

  1. Navigate to a Group with many samples (over 20).
  2. For at least two samples, add paired end attachments to one, and a single end attachment to the other (note these sample IDs).
  3. Select a large number of the group samples (use a total sample count that is not divisible by 5). Note how many samples there are and divide by 5 (and round up) for number of pages
  4. Start a workflow
  5. Verify on the pagination just below the samples table that the Previous button is disabled, and click the page number and check that the dropdown selection goes down to the expected page number
  6. Test the dropdown page selection, previous and next buttons and ensure the expected pagination behaviour.
  7. Navigate to the last page and verify the next button becomes disabled.
  8. Verify on the last page that the correct number of 'remaining' samples was rendered. (If you selected 23 samples, you would expect 3 samples to render on page 5).
  9. To the samples that you uploaded attachments to, find those samples and select the uploaded attachments for the respective sample.
  10. Change pages and then return to the page containing those samples, and verify that the sample attachment listing is still the same as your selection (did not revert back to the initial file selection).
  11. Launch the workflow
  12. Verify on the workflow's samplesheet that the changed attachments are listed on the respective samples.
  13. Navigate back to any project/group with samples.
  14. Select less than 5 samples
  15. Start a workflow and verify that pagination does not render as there is only 1 page.

PR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

@ChrisHuynh333 ChrisHuynh333 changed the title Nextflow Samplesheet - Pagination (Part 2) Nextflow Samplesheet - Pagination Jan 24, 2025
@ChrisHuynh333 ChrisHuynh333 force-pushed the nextflow-samplesheet/pagination branch from 6974efd to 24e68b3 Compare January 27, 2025 19:15
@ChrisHuynh333 ChrisHuynh333 self-assigned this Jan 27, 2025
@ChrisHuynh333 ChrisHuynh333 force-pushed the nextflow-samplesheet/pagination branch from f3869f7 to 8caaee3 Compare January 29, 2025 17:00

This comment has been minimized.

@ChrisHuynh333 ChrisHuynh333 marked this pull request as ready for review January 29, 2025 22:48
@ChrisHuynh333 ChrisHuynh333 added the ready for review Pull request is ready for review label Jan 29, 2025
@ericenns ericenns requested a review from joshsadam January 30, 2025 13:57
@joshsadam
Copy link
Contributor

Styling issue if there is only one page.  Maybe just hide the pagination if just one page.

@ChrisHuynh333
Copy link
Collaborator Author

Styling issue if there is only one page.  Maybe just hide the pagination if just one page.

Fixed in dde84a3

@ChrisHuynh333 ChrisHuynh333 force-pushed the nextflow-samplesheet/pagination branch from dde84a3 to aed494e Compare January 30, 2025 17:52

This comment has been minimized.

Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This is a lot of really good work.  I would like to see a lot less JavaScript dependency, but that is beyond the scope of this PR.  I have one request.

This comment has been minimized.

@joshsadam joshsadam requested a review from deepsidhu85 February 3, 2025 16:09
@ChrisHuynh333 ChrisHuynh333 force-pushed the nextflow-samplesheet/pagination branch from 388e764 to be6dd66 Compare February 4, 2025 21:15

This comment has been minimized.

Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This works exactly as it is supposed. This PR was a lot of work and you did a really great job on it.

Copy link
Member

@ericenns ericenns left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! It works as I expect and looks great!

Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

This looks great!

@deepsidhu85 deepsidhu85 merged commit d2e7bbd into main Feb 5, 2025
4 checks passed
@deepsidhu85 deepsidhu85 deleted the nextflow-samplesheet/pagination branch February 5, 2025 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants