Y26-021 - Add accession feedback to sample manifests #5492
Conversation
StephenHulme
added
UX
StephenHulme
requested review from
andrewsparkes,
dasunpubudumal,
neilsycamore and
yoldas
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5492 +/- ##
===========================================
- Coverage 87.40% 87.39% -0.01%
===========================================
Files 1457 1457
Lines 32703 32705 +2
Branches 3422 3423 +1
===========================================
Hits 28584 28584
- Misses 4104 4106 +2
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andrewsparkes
left a comment
andrewsparkes
left a comment
There was a problem hiding this comment.
Looks good.
I'm confused about when sample generation happens is all.
|
|
||
| <% if samples.size == 0 %> | ||
| <div class="help"> | ||
| Samples will be generated on upload. |
There was a problem hiding this comment.
I'm getting confused again. Was this just wrong? Samples are generated when you generate the manifest right? It's the sample metadata that gets filled in at upload?
There was a problem hiding this comment.
I don't fully understand the inner workings here, but looking at an existing BIOSCAN sample manifest that hasn't been uploaded, I can see plate barcodes, but no linked samples:
https://training.sequencescape.psd.sanger.ac.uk/labware/27653432
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| <% samples.each do |sample| %> |
There was a problem hiding this comment.
how well does this perform if there are many samples in a manifest (thousands)? It has pagination right? I see a comment on line 65
There was a problem hiding this comment.
I haven't tested with thousands, only up to 200 - but closer inspection reveals that pagination only kicks in at 500. I'll test again to confirm...
Thanks for asking!
There was a problem hiding this comment.
Pagination supported from 500 records, small changes made in 975dcce and confirmed working
|
|
||
| <%= panel_no_body :info, title:'Samples' do %> | ||
| <% if @samples.empty? %> | ||
| <%= tag.div(class: 'p-3') { "Samples will be generated on upload" } %> |
There was a problem hiding this comment.
See comment above.
So is it we reserve sample ids at manifest creation somehow but don't create the sample records?
There was a problem hiding this comment.
I only see barcodes reserved prior to a manifest being uploaded?
So is it we reserve sample ids at manifest creation somehow but don't create the sample records?
I agree, this seems like a strange scenario... more investigation required maybe?
There was a problem hiding this comment.
Ah, the magic here appears to be a SampleManifestAsset:
# Keeps track of sanger_sample_ids which have been allocated to a {SampleManifest}
# and associates them with the corresponding {Receptacle}
class SampleManifestAsset < ApplicationRecord
StephenHulme
deleted the
y25-021-add-accession-feedback-to-sample-manifests
branch
Contributes towards closing #5481
Changes proposed in this pull request
Screenshots
Before:
After:
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main]- Check story numbers included
- Check for debug code
- Check version