Skip to content

Y25-617 - Flexible pool file upload#2626

Open
BenTopping wants to merge 25 commits intodevelopfrom
Y25-617-flexible-pool-file-upload
Open

Y25-617 - Flexible pool file upload#2626
BenTopping wants to merge 25 commits intodevelopfrom
Y25-617-flexible-pool-file-upload

Conversation

@BenTopping
Copy link
Contributor

@BenTopping BenTopping commented Feb 16, 2026

Closes #2507

Changes proposed in this pull request

  • Adds loading spinner during file upload processing for multi pools.
  • Adds multiPool csv lib helper for parsing and validating the file.
  • Adds fileupload logic to create pacbio pools on successful file parse.
  • Adds e2e test for file upload, pool edit and creation for flexible pool.

@BenTopping
Copy link
Contributor Author

BenTopping commented Feb 27, 2026

Todo:

  • Feature: Pacbio Libraries should have their data automatically filled if its not provided in the csv but csv data should take priority.
  • Test: e2e test for file upload and errors etc.
  • Test: Thorough testing with 'view individual pools' branch.

Copy link
Contributor

@StephenHulme StephenHulme left a comment

Choose a reason for hiding this comment

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

Looks good to me. Lots of lines of new code can make it hard to properly review. Is it possible to build the functionality up in layers over 2-5 PRs?

Like the docstrings and comments, thank you :)

@BenTopping
Copy link
Contributor Author

Looks good to me. Lots of lines of new code can make it hard to properly review. Is it possible to build the functionality up in layers over 2-5 PRs?

Like the docstrings and comments, thank you :)

Yes its a valid point, if I was to do this again I would have had an epic branch and made smaller merge requests into it. I think its going to be difficult to pull this apart. Im happy to go over it in a call?

@BenTopping
Copy link
Contributor Author

I have pulled library attributes logic into a new story #2659

@StephenHulme
Copy link
Contributor

Thanks for the walkthrough 🙏
A few comments I made along the way, most of which we discussed. Please shout if you have any questions:

  1. Download template icon: down (or download) arrow, not info circle
  2. Only use red text for errors, suggest grey or blue/purple otherwise
  3. Template prep kit box barcode incorrect number format 123E4 - suggest string to handle leading 0s
  4. ParsePoolingCSVFile is a really big function, can it be split?
  5. PoolNumber check for !(pool_num >= 0) rather than pool_num < 1, that way the check matches the error message and neighbouring checks
  6. Replace !success with errors where possible
  7. Check capitalisation equality of tag and tag set names
  8. Disambiguate library and request requests and responses
  9. Look into leading underscore of aliquot hashes

I'll play with it myself and let you know how it goes.

Overall, nice work!

@BenTopping
Copy link
Contributor Author

Thanks for the walkthrough 🙏 A few comments I made along the way, most of which we discussed. Please shout if you have any questions:

  1. Download template icon: down (or download) arrow, not info circle - Addressed in 8ad807c
  2. Only use red text for errors, suggest grey or blue/purple otherwise
  3. Template prep kit box barcode incorrect number format 123E4 - suggest string to handle leading 0s - This is an excel config rather than the csv file upload issue. Excel formats it if you don't disable it. I will see if they run into it in UAT and go from there
  4. ParsePoolingCSVFile is a really big function, can it be split? - Valid concern, as discussed its partially a tradeoff between readability and abstraction
  5. PoolNumber check for !(pool_num >= 0) rather than pool_num < 1, that way the check matches the error message and neighbouring checks Addressed in b18a015
  6. Replace !success with errors where possible
  7. Check capitalisation equality of tag and tag set names It is case sensitive. Possible future nice to have if users flag it
  8. Disambiguate library and request requests and responses Addressed in 8ad807c
  9. Look into leading underscore of aliquot hashes PacbioPoolCreate store issue. This store needs some work, ill raise a story

I'll play with it myself and let you know how it goes.

Overall, nice work!

StephenHulme
StephenHulme previously approved these changes Mar 12, 2026
Copy link
Contributor

@StephenHulme StephenHulme 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 the discussion and changes, approved 🚀

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.

Y25-617 - As a developer (Ben) I would like to be able to create flexible pools from a file upload.

2 participants